Bug 352810

Comment 1 Steffen Pingel 2011-07-27 17:41:14 EDT

We deliberately made the context listener class abstract to allow us to evolve it without breaking binary backwards compatibility. This avoid the platform anti-pattern of creating 9 versions ITextViewerExtension. I am not sure that I understand the benefit of the suggested refactoring?

Regarding other changes in the commits, the factory methods in ContextChangeEvent look good to me. This would be a nice enhancement that we could consider for 3.7.

Generally, before making larger changes to the existing code, please always bring this up in a bug report for discussion first. Rejecting a contribution is never good but often there are reasons for the current code structure that are not always obvious or documented.

Comment 2 Miles Parker 2011-07-27 18:01:29 EDT

(In reply to comment #1)
> We deliberately made the context listener class abstract to allow us to evolve
> it without breaking binary backwards compatibility. This avoid the platform
> anti-pattern of creating 9 versions ITextViewerExtension.

OK, I'm not really conversant with this issue as it relates to extensions -- I'm used to seeing people providing interfaces for extension implementations. Is there a reference I could look at to clarify? Because you can't change the Abstract API anyway, as it isn't internal.. What about supporting the interface for potential API users but exposing the AbstractContextListener only for extension?

> I am not sure that I understand the benefit of the suggested refactoring?

I think it's important for users that *aren't* making use of the extensions. The API specifically contemplates this. In my case, it's the old story of having someone else's Abstract class that I need to mix in. I could have delegated, but because AbstractContextListener has so many legacy methods, this would have become ugly.

> Regarding other changes in the commits, the factory methods in
> ContextChangeEvent look good to me. This would be a nice enhancement that we
> could consider for 3.7.

Yes, they prevent the API user from creating an event that isn't consistent. I'm not completely sure that I've handled all of the potential creation cases correctly but I did look at each of the current API uses.

> Generally, before making larger changes to the existing code, please always
> bring this up in a bug report for discussion first. Rejecting a contribution is
> never good but often there are reasons for the current code structure that are
> not always obvious or documented.

Completely reasonable. I got started down the road because I needed it and got a bit carried away.

Comment 3 Steffen Pingel 2011-07-27 18:11:54 EDT

(In reply to comment #2)
> (In reply to comment #1)
> > We deliberately made the context listener class abstract to allow us to evolve
> > it without breaking binary backwards compatibility. This avoid the platform
> > anti-pattern of creating 9 versions ITextViewerExtension.
> 
> OK, I'm not really conversant with this issue as it relates to extensions -- I'm
> used to seeing people providing interfaces for extension implementations. Is
> there a reference I could look at to clarify? Because you can't change the
> Abstract API anyway, as it isn't internal.. What about supporting the interface
> for potential API users but exposing the AbstractContextListener only for
> extension?

That certainly makes sense. We follow this pattern where the interface is part of the API and also implemented by extensions. In those cases we mark the interface as @noextend and @noimplement to indicate that only the abstract class should be extended.

In the case of AbstractContextListener I don't see an obvious use case for API clients though that wouldn't require extending the class?

> > I am not sure that I understand the benefit of the suggested refactoring?
> 
> I think it's important for users that *aren't* making use of the extensions. The
> API specifically contemplates this. In my case, it's the old story of having
> someone else's Abstract class that I need to mix in. I could have delegated, but
> because AbstractContextListener has so many legacy methods, this would have
> become ugly.

Can you elaborate on your specific use case?

> > Regarding other changes in the commits, the factory methods in
> > ContextChangeEvent look good to me. This would be a nice enhancement that we
> > could consider for 3.7.
> 
> Yes, they prevent the API user from creating an event that isn't consistent. I'm
> not completely sure that I've handled all of the potential creation cases
> correctly but I did look at each of the current API uses.

Great. Could we track this on a separate bug?

Comment 4 Miles Parker 2011-07-27 18:58:34 EDT

(In reply to comment #3)
> In the case of AbstractContextListener I don't see an obvious use case for API
> clients though that wouldn't require extending the class?

Well, the class doesn't really do anything, it's just an adapter. So any time I wanted to listen to arbitrary changes in the context I could simply 

> Can you elaborate on your specific use case?

Sure...

https://github.com/MilesParker/mylyn.incubator/blob/master/org.eclipse.mylyn.gmf.ui/src/org/eclipse/mylyn/gmf/ui/MylynDecoratorProvider.java

This code is very loose right now, but I want to be able to mix together functionality from a GMF decoration service and the Mylyn functionality. This is similar to many of the Eclipse UI usage patterns, where an integrating class implements a number of simple interfaces to coordinate their activities. In this case, the DecoratorProvider is also registered by an extension:

https://github.com/MilesParker/mylyn.incubator/blob/master/org.eclipse.mylyn.gmf.ui/plugin.xml

So I didn't want to extend it. I've been considering re-providing an extension for diagram context management, and this is sort of part of the strategy.  So, in retrospect I probably should have just delegated it, but the code was arguably cleaner this way and I figured that I wasn't the only person who might want to use it in this way.

Comment 5 Miles Parker 2011-07-27 19:02:46 EDT

(In reply to comment #4)
> (In reply to comment #3)
> > In the case of AbstractContextListener I don't see an obvious use case for API
> > clients though that wouldn't require extending the class?
> 
> Well, the class doesn't really do anything, it's just an adapter. So any time I
> wanted to listen to arbitrary changes in the context I could simply 

Trailed off there. Anyway, the point is that it doesn't actually do anything *at all* now that the other methods are deprecated. So it just didn't make sense to me not to have it as an interface -- not being aware of this issue with abstract classes for extensions. I didn't bother getting rid of the existing deprecated usages because I'd already spent too much time on it, but with a tiny bit more work there wouldn't have been any references to AbstractContextListener in the context implementation at all except for the extension support and legacy API.

Comment 6 Steffen Pingel 2011-08-09 15:29:31 EDT

We could consider converting the abstract class to an interface since there is only one method now which is extensible through ContextChangeEvent parameter. I am not sure that it would be worth it considering the amount of changes but it's something we should revisit for 4.0 when we are able to break backwards compatibility.

Comment 7 Miles Parker 2011-08-09 23:27:05 EDT

(In reply to comment #6)
> We could consider converting the abstract class to an interface since there is
> only one method now which is extensible through ContextChangeEvent parameter. I
> am not sure that it would be worth it considering the amount of changes but
> it's something we should revisit for 4.0 when we are able to break backwards
> compatibility.

This is probably already clear, but the version I introduced doesn't break java API compatibility (I think :)) "just" plugin compatibility, but Abstract could still be used for that. But this certainly isn't worth losing sleep over. For my case I just used the delegate and it wasn't that awkward.

Comment 8 Steffen Pingel 2012-07-15 12:24:08 EDT

This enhancement request is being addressed as part of bug 384255 and https://git.eclipse.org/r/#/c/6625/.