Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 349924

Summary:

[api] provide extension point for task activation listeners

Product:

z_Archived

Reporter:

Manuel Doninger <manuel.doninger>

Component:

Mylyn

Assignee:

Manuel Doninger <manuel.doninger>

Status:

RESOLVED FIXED

QA Contact:

Severity:

enhancement

Priority:

CC:

b.muskalla, mauersberger, steffen.pingel

Version:

unspecified

Keywords:

api, contributed, noteworthy, plan

Target Milestone:

3.7

Hardware:

All

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Patch for extension point	none
mylyn/context/zip	none
Added tests, iterate over all configuration elements	none

Description Manuel Doninger

2011-06-21 06:08:04 EDT

This patch provides a new extension point to register task activation listeners. This enables plugins to register their listeners without the need to start the plugin itself, which would be necessary with programmatic registering.

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