[squeak-dev] Re: MVC-Morphic refactoring, review needed

Andreas Raab andreas.raab at gmx.de
Tue Nov 24 04:34:42 UTC 2009


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

> 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
> 
> 
> 




More information about the Squeak-dev mailing list