Bug 302061

Comment 1 Alexander Nyßen 2010-07-18 17:59:24 EDT

Inspired by as it is done by GMF runtime, we could introduce some registerModelListeners/unregisterModelListeners hook methods, that could be called from within activate/deactivate. This way, activate and deactivate would not have to be overwritten in a lot of cases, where only a model listener is needed. It would thus make the code more robust.

Comment 2 Randy Hudson 2010-07-18 19:39:35 EDT

This would assume a specific model/notification pattern. Not every GEF application registers model listeners between each editpart and model object. I've worked on a GEF-based product which even used EMF, but had no need to override activate() and deactivate().  Instead, there was just one domain listener for the entire resource set which batched (and combined) updates and then used the editpart registry to send the updates to the necessary editparts.  This notification pattern turned out to be much efficient while enabling features like inheriting visual styles from your parent (which was missing from GMF).

The point being, it is not valid to say subclasses of AGEP have to overwrite activate()/deactivate(). Even when you do, extending a method from your superclass is straightforward. Also, there's another alternative already available, you can listen for EditPartListener#partActivated()/partDeactivated() instead of overriding activate()/deactivate().  Or, you could install an editpolicy which gets activated and deactivated along with its host.

Comment 3 Alexander Nyßen 2010-07-19 04:40:28 EDT

Randy, I see your point. Let me thus say that my concern is not the registration of model listeners. It is far more about how to separate client activation code (e.g. the registering of a model listener) from framework activation code (related to the EditPart lifecycle) so that unintentionally breaking the EditPart lifecycle is not as easy as it currently is for GEF-beginners. I just thought that registering a model listener would be the most likely case for having to overwrite activate/deactivate, thus my proposal. One could of course also think of naming such a hook method a bit more "general", e.g. something like "doActivate"/"doDeactivate".

Comment 4 Randy Hudson 2010-07-19 09:27:54 EDT

And what if the custom subclass has a subclass that needs to do additional stuff also?

Comment 5 Alexander Nyßen 2010-07-19 09:34:35 EDT

(In reply to comment #4)
> And what if the custom subclass has a subclass that needs to do additional
> stuff also?

Well it could overwrite doActivate/doDeactivate and could thus of course corrupt the client-specific initialization in its superclass, but never the framework specific within AbstractGraphicalEditPart's activate/deactivate. What problem do you see?

Comment 6 Anthony Hunter 2010-07-19 11:40:04 EDT

(In reply to comment #0)
> Build Identifier: M20090917-0800
> 
> Up to now, custom subclasses of AbstractGraphicalEditPart have to overwrite
> activate() and deactivate() to register their model listeners. 
> 
> This approach has the shortcoming that the API-contract w.r.t.
> activation/deactivation of edit parts may get violated if subclasses:
> - miss to call the super implementations, and/or
> - miss to query the active state (getActive())
> 
> Both I think is a typical pitfall. It could be prevented by introducing
> dedicated hook methods (with empty implementations) that subclasses could
> overwrite to register their listeners, calling these from activate/deactivate
> and making activate/deactivate final.
> 
> Reproducible: Always

What is the business requirements here? The API being debated has existed in GEF for over 10 years and I am not seeing any overwhelming complaints in Bugzilla that would suggest changing anything.

Comment 7 Alexander Nyßen 2010-07-19 14:06:33 EDT

Anthony, you are right, Bugzilla doesn't state much concerning this issue. Yet, from the experiences I have gained when performing GEF-related coachings and trainings (and from my personal experiences as well), I inferred that overwriting activate()/deactivate() without querying the active state or forgetting to call the super-implementation is a potential pitfall that especially GEF-beginners often tend to run into. As the Javadoc of AbstractEditPart#activate() explicitly states that subclasses should overwrite to register model listeners, this is quite likely to happen.

My intention is thus to make the framework more robust w.r.t this, as I think a framework should try to prevent that clients can easily break framework mechanism by accident. A dedicated hook method for the purpose of application specific activation/deactivation code could prevent clients from running into those issues, as clients could implement it without having to care about the edit part lifecycle. Unlike stated in the first comment, I would not tend to make activate()/deactivate() final (if somebody knows what he does, he should be able to still do so), so any existing implementations would not be affected.

Comment 8 Alexander Nyßen 2010-07-27 14:02:10 EDT

Also, activate and deactivate in AbstractEditPart and AbstractGraphicalEditPart are not really save against multiple invocations. Indeed this is something added by all concrete subclasses in the provided GEF examples. I think it would be a good idea to make them robust against this misuse as well (by adding something like if(isActive() {return} to activate and if(!isActive(){return} to deactivate. Then subclasses could very much stick to overwriting a respective hook to register/unregister their model listeners (and could rely on this code being called more than once).

Comment 9 Randy Hudson 2010-07-28 11:08:19 EDT

And maybe a null check in addChildVisual(), in case the child's getFigure() method returns null.

Comment 10 Alexander Nyßen 2011-03-14 13:17:21 EDT

I think it would be good to combine this with code to prevent recursive activation of edit parts. As this will definitely affect the behavior of existing clients, I think it is best to include it in the major release, even if it will not literally break API. Thus scheduling to Future.

Comment 11 Alexander Nyßen 2014-07-16 07:52:11 EDT

I have incorporated this into GEF4 MVC and will resolve it as WORKSFORME here, as changing the GEF 3.x MVC code to this extend does not seem to be worthwhile any more.