MVC-Morphic refactoring, review needed (was: The Trunk: System-dtl.170.mcz)

I updated Project>>okToChange to remove Morphic/MVC dependencies. In the
process I elimated some logic that appears to serve no purpose. However,
this is code that existed in earlier versions of the method with a "di"
author stamp, so I am concerned that it may have been there for a reason
that I don't understand. If someone can review the change, I'd appreciate it.

Brief explanation: Project>>okToChange is called when the window representing
a project is being deleted. The sender may be a SystemWindow (Morphic) or
a ProjectController (MVC), and the project to be deleted may be an MVCProject
of a MorphicProject. The original code does this:

        ok := world isMorph not and: [world scheduledControllers size <= 1].
        ok ifFalse: [self isMorphic ifTrue:
                [self parent == CurrentProject
                        ifFalse: [^ true]]].  "view from elsewhere.  just delete it."


I could find no path through the code for which this did anything useful,
so I changed it to this:

        self parent == CurrentProject
                        ifFalse: [^ true].  "view from elsewhere.  just delete it."

Hopefully this was just leftover cruft that needed cleaning. Am I missing anything?

Thanks,
Dave

David T. Lewis wrote:
> I updated Project>>okToChange to remove Morphic/MVC dependencies. In the
> process I elimated some logic that appears to serve no purpose. However,
> this is code that existed in earlier versions of the method with a "di"
> author stamp, so I am concerned that it may have been there for a reason
> that I don't understand. If someone can review the change, I'd appreciate it.

Your change is perfectly fine as far as compatibility goes but I think
it's missing the mark somewhat, so let me explain:

That method currently determines when to delete a project based on
deleting its project view. I.e., when you create a project we're
currently creating a project link to enter the project, but you can also
create a link to the project from elsewhere. The logic assumes that it's
okay to delete project views from other projects than the original
parent, but will nuke the underlying project if the view is in the
original parent project. That seems flawed in more than one way - it
assumes that a view that comes from the original parent project it is
necessarily the "primary" view for this project and that deletion of
that view should also cause the project to be deleted. I think both are
wrong assumptions.

Instead, what we should do is to make it so that deletion of projects is
explicit, via the menu action called "close project" (with confirmation
etc). Then you can get rid of all of that logic - it's always safe to
delete a project view since the underlying project will only be deleted
when you explicitly choose to.

Cheers,
   - Andreas

Andreas, thank you for the feedback.

On Mon, Nov 23, 2009 at 08:34:42PM -0800, Andreas Raab wrote:
Good, my primary concern was to make sure I did not break anything.

Yes, I see what you mean.

> Instead, what we should do is to make it so that deletion of projects is
> explicit, via the menu action called "close project" (with confirmation
> etc). Then you can get rid of all of that logic - it's always safe to
> delete a project view since the underlying project will only be deleted
> when you explicitly choose to.

I'm not sure I understand. Are you thinking of a menu action in the
world menu of the current project, or a menu action in the window menu
of the view on the project? (Or something that already exists that
I am overlooking?)

I think that it is still necessary to ensure that the project
is finalized when the last open view has been closed, at least in
the case of a Morphic project with Players and Wonderlands (I don't
know what the issues are, but the cleanup code must be there for
a reason).

So if the user closes the last open view on a project, I expect that
we still want the project to be automatically finalized, and we would
not want finalization to occur unless the view being closed was the
last open view on that project.

Does that sound right?

Dave

David T. Lewis wrote:
The former. I think the "Projects" menu should say:
   New Project
   Load Project
   Save Project
   Close Project
and "Close Project" is the only action that ever nukes the project for
real. Project views are just project views, they can be deleted without
any further implications for the original project.

> I think that it is still necessary to ensure that the project
> is finalized when the last open view has been closed, at least in
> the case of a Morphic project with Players and Wonderlands (I don't
> know what the issues are, but the cleanup code must be there for
> a reason).

My point is that a project can exist without any currently visible
project views. The project remains accessible via the "jump to project"
menu and its "primary" reference is through Project>>allProjects.

> So if the user closes the last open view on a project, I expect that
> we still want the project to be automatically finalized, and we would
> not want finalization to occur unless the view being closed was the
> last open view on that project.
>
> Does that sound right?

Not quite (from my perspective - I'm not saying you have to agree :-)
Views are views, they're not the real thing. We could conceivably offer
to close the project when the last view is closed, but going forward I
think we'll end up with more projects that have no dedicated views and
are only accessible via places like the "jump to" menu.

Cheers,
   - Andreas

On Wednesday 25 November 2009 11:03:57 am Andreas Raab wrote:
> My point is that a project can exist without any currently visible
> project views. The project remains accessible via the "jump to project"
> menu and its "primary" reference is through Project>>allProjects.
This distinction between project view and project was the most confusing
feature for me when I started with Squeak. All other types of objects have a
fully featured browser - packages, changesets, classes, methods etc. but not
projects.

The prev-next hints at a list of projects but the menu brings up a 'hierarchy'
of projects. But this brings up a hierarchy of references, not containment.

How difficult is it to whip up a browser (like Monticello Browser or Page
Sorter) for Project Repositories (including active image) and Projects? We
should be able to add project repositories, browse, load, edit, mark changed
projects, save or dispose projects?

Subbu

On 2009-11-25 07:53, K. K. Subramaniam wrote: There is the ProjectHistoryMorph
It would be nice if this could hold both projects in the image and
projects on disk, server etc ,
with some kind of indication of location, like a bookmark ?

Karl