Comment 1 Manuel Doninger 2011-06-21 06:09:43 EDT

Created attachment 198316 [details]
Patch for extension point

Comment 2 Manuel Doninger 2011-06-21 06:09:48 EDT

Created attachment 198317 [details]
mylyn/context/zip

Comment 3 Steffen Pingel 2011-07-27 18:45:51 EDT

Thanks for the patch! The proposed change looks good. I have a few minor suggestions for improvement:

* Construct listeners lazily, i.e. on the first event.
* Iterate over all IConfigurationElement objects.

Would you be able to iterate on this?

Comment 4 Manuel Doninger 2011-07-27 18:54:53 EDT

I'll look into this.

Comment 5 Benjamin Muskalla 2011-07-28 08:20:37 EDT

Yep, looks good. Minor comments from my side:

* I'd also add a test for this, should not be too hard to provide a simple mock extension in the tests bundle and ensure it is called.

* Extracting @ITasksCoreConstants.ID_PLUGIN + ".taskActivationListeners"@ should also go into a constant

Comment 6 Manuel Doninger 2011-08-03 05:50:51 EDT

(In reply to comment #3)
> * Iterate over all IConfigurationElement objects.

One question here: Is there a specific reason to iterate over all objects? Wouldn't that have a lower performance? Or doesn't the implementation of getConfigurationElementsFor guarantee, that i get really all configuration elements for an extension point?

Comment 7 Benjamin Muskalla 2011-08-03 05:54:19 EDT

> One question here: Is there a specific reason to iterate over all objects?
Equinox gives no guarantee about the order extensions are listed. Only adding the first one may work in your config but as soon as another extension comes along, it may break yours. Thus we should read all extensions and add all available listeners.
I don't see a performance degradation in this case once you do it lazily.

Comment 8 Manuel Doninger 2011-08-03 17:01:46 EDT

Created attachment 200857 [details]
Added tests, iterate over all configuration elements

Tests added, iterate over all configuration elements, extracted string to constant, initialize listeners on first task activation or deactivation.

Comment 9 Steffen Pingel 2011-08-22 20:50:17 EDT

Thanks a lot for the updated patch! 

I have applied it with a few minor modifications: 
* I renamed the schema attribute from "listenerClass" to "class"
* I guarded the creation of extensions with a catch Throwable. Not pretty but some extensions have the bad habit of throwing LinkageErrors which would propagate otherwise.
* Moved the constant to TaskActivityManager
* The test stub is now an inner class and test cases are separate