Bug 386117 - [api] tasks.core relevant extensions should not be declared in tasks.ui
Summary: [api] tasks.core relevant extensions should not be declared in tasks.ui
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P2 enhancement with 1 vote (vote)
Target Milestone: 3.10   Edit
Assignee: Steffen Pingel CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy, plan
Depends on:
Blocks:
 
Reported: 2012-07-27 07:42 EDT by Tomas Stupka CLA
Modified: 2013-09-26 18:18 EDT (History)
5 users (show)

See Also:


Attachments
Register AbstractRepositoryConnector via OSGi services (7.65 KB, patch)
2012-08-13 10:41 EDT, Jaroslav Tulach CLA
no flags Details | Diff
Registering Trac's connector using declarative services (6.05 KB, patch)
2012-08-14 06:27 EDT, Jaroslav Tulach CLA
no flags Details | Diff
mylyn/context/zip (7.21 KB, application/octet-stream)
2012-08-28 16:59 EDT, David Green CLA
no flags Details
Sample support for migrators (9.35 KB, patch)
2012-08-30 06:15 EDT, Jaroslav Tulach CLA
jtulach: review?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Stupka CLA 2012-07-27 07:42:57 EDT
Build Identifier: 

A Mylyn connectors AbstractRepositoryConnector extension is made available by the connectorCore extension point, which is defined in the tasks.core.ui bundle. 

In case somebody wants to write an alternative UI based on mylyn.tasks.core and someconnector.core, it still is necessary to depend on mylyn.tasks.ui (and typically also someconnector.ui). It would be great if (a) the extension point dedicated to <code>AbstractRepositoryConnector</code>, would be defined (also) in the tasks.core bundle. 


Reproducible: Always
Comment 1 Steffen Pingel CLA 2012-07-31 06:12:52 EDT
The connector life-cycle is managed in tasks.ui and hence the extension was declared in the UI and the usecase of running connectors in an headless OSGi environment was not considered at the time. I agree though that this isn't optimal and should be changed. I am afraid since moving the extension would break existing connectors we would not be able to take this on prior to a major revision (i.e. Mylyn 4.0).
Comment 2 Tomas Stupka CLA 2012-07-31 07:23:51 EDT
(In reply to comment #1)
> I am afraid since moving the extension would
> break existing connectors we would not be able to take this on prior to a
> major revision (i.e. Mylyn 4.0).
don't know if it is even possible as i'm not very familiar how exactly the extension point mechanism in eclipse works, but wouldn't it be just enough for the beginning to have also a second definition in tasks.core? With a different id etc. - tasks.ui could then query the tasks.ui extensions first and eventually also what's defined in tasks.core. Whoever else, who doesn't want to rely on tasks.ui, would then query only the extensions defined in tasks.core.
Comment 3 David Green CLA 2012-07-31 12:58:20 EDT
(In reply to comment #1)
> I am afraid since moving the extension would
> break existing connectors we would not be able to take this on prior to a major revision (i.e. Mylyn 4.0).

Steffen, would it be possible to add a tasks.core extension point without removing the tasks.ui extension point?  That would enable connectors to transition without breaking API.  The tasks.ui extension point could be marked as deprecated, to be removed in 4.0.
Comment 4 Steffen Pingel CLA 2012-08-01 10:02:48 EDT
We can't deprecate the UI extension point since it's still needed for the UI contribution. I am not a big fan of adding redundant API of this sort since it tends to cause confusion for integrators but we could consider it was contributed. Tomas, let us know if you are providing a contribution.
Comment 5 Jaroslav Tulach CLA 2012-08-07 10:35:26 EDT
(In reply to comment #4)
> We can't deprecate the UI extension point since it's still needed for the UI
> contribution. I am not a big fan of adding redundant API of this sort since
> it tends to cause confusion for integrators but we could consider it was
> contributed. Tomas, let us know if you are providing a contribution.

Can't you just move the definition of the extension point from task.ui to task.core (and keep its name) and move the registration from trac.ui to trac.core? As far as I understand when task.ui is installed, task.core needs to be installed as well and when trac.ui is installed trac.core needs to be installed as well. So all definitions would still be present (keeps compatibility) and gives us more flexibility.
Comment 6 Jaroslav Tulach CLA 2012-08-13 10:41:53 EDT
Created attachment 219809 [details]
Register AbstractRepositoryConnector via OSGi services

We had a chat with David Green last week and we concluded that

- we need a solution in 3.x time frame
- my idea of moving the extension point to tasks.core in not appropriate
- we need a second registration mechanism

While at it I suggested to use OSGi (declarative) services for registration. David mentioned this has come up in the past, but nobody had a reason to really implement it. Actually I have two reasons:

- to separate the registration from the tasks.ui
- to lower number of dependencies of connectors (no need for eclipse.registry)

I am attaching first version of my patch. Please share your comments, so we can proceed with integration. Java one is at the end of September and it would be excellent to use not patched version of the JAR during the demo.
Comment 7 Jaroslav Tulach CLA 2012-08-14 06:27:39 EDT
Created attachment 219846 [details]
Registering Trac's connector using declarative services

Yet another patch that is really using declarative services. 

As far as I can say the patch works OK in Eclipse. I can stop/start the trac.core bundle and the connector properly hides/shows among the list of available connectors.
Comment 8 David Green CLA 2012-08-28 16:58:22 EDT
The DS approach demonstrated in comment 7 and attachment 219846 [details] looks like a great start.

Have you thought about how this could handle AbstractTaskListMigrator or AbstractRepositoryMigrator?  

Have you looked at how TasksUiExtensionReader registers these?  There are some additional things handled there such as checking for conflicts.
Comment 9 David Green CLA 2012-08-28 16:59:11 EDT
Created attachment 220421 [details]
mylyn/context/zip
Comment 10 Jaroslav Tulach CLA 2012-08-30 06:15:59 EDT
Created attachment 220504 [details]
Sample support for migrators

Thanks for looking at my previous patch. I've looked the code and it seems to me there can be at most one repository and one tasklist migrator per connector. If that is the case, it could be handled by a special property associated with the connector. 

I am attaching a sample patch to demonstrate what I mean.
Comment 11 Steffen Pingel CLA 2012-09-05 20:00:06 EDT
Thanks for the patches Jaroslav. I am a bit concerned that using declarative services for connectors would add complexity for connector implementors since Mylyn uses extension points for other extensibility hooks. I am wondering if it would make sense to copy the core portion of the extension point into o.e.m.tasks.core and move/reuse some of the extension point reading logic that is currently in the UI into core. The framework could attempt to read the core extension first and then fall back to legacy ui extension to retain backwards compatibility. What do you think?
Comment 12 Jaroslav Tulach CLA 2012-09-07 06:00:01 EDT
Compare 

(In reply to comment #4)
> I am not a big fan of adding redundant API of this sort since
> it tends to cause confusion for integrators but we could consider it was
> contributed. 

and

(In reply to comment #11)
> I am a bit concerned that using declarative
> services for connectors would add complexity for connector implementors
> since Mylyn uses extension points for other extensibility hooks. 

when we talked with David, we tried to avoid the problem stated in comment #4 by not duplicating the registration, but rather creating new, OSGi standards based way of registering the connector.

David mentioned that the issue of using declarative services was brought up before, just nobody had the need to really implement it. As using declarative services in NetBeans is simpler than extension registry, as OSGi folks tend to denounce extension registry and favor declarative services as a way to go, I wanted to use this opportunity to propose their use for connector registration as well.

Our general need is to have connectors registered in the *.core module, not in the *.ui. We don't care how that is done. But I admit I personally like declarative services more than extension registry.
Comment 13 Steffen Pingel CLA 2012-10-06 05:38:57 EDT
Sorry for the slow response. Declarative services would indeed be a reasonable approach for registering connectors and something we could consider in the future. At the moment I would like to keep using extension points to make it as simple as possible for connectors to move the extension to the core and avoid code duplication.

I have pushed a first pass here: https://git.eclipse.org/r/8070 . Turned out to be a larger refactoring. Could you check if that is along the lines what you need? The change only includes an update for the Bugzilla connector so we would need to still update extensions for other connectors.
Comment 14 Steffen Pingel CLA 2013-07-02 15:56:09 EDT
I have updated the review against the latest master. Since part of the code already moved to tasks.core as part of bug 408511 we are almost there. Sam, Tomek, can you take a look at the review at https://git.eclipse.org/r/#/c/8070/ and let me know what you think?
Comment 15 Steffen Pingel CLA 2013-07-12 08:32:11 EDT
I have updated the Gerrit review against the latest master. Jaroslav, Tomas, it'd great if you could take a look if that would satisfy your use case.
Comment 16 Tomas Stupka CLA 2013-07-17 06:53:38 EDT
(In reply to comment #15)
> I have updated the Gerrit review against the latest master. Jaroslav, Tomas,
> it'd great if you could take a look if that would satisfy your use case.
Thanks Steffen, we let you know in the next few days ...
Comment 17 Tomas Stupka CLA 2013-09-05 10:07:28 EDT
(In reply to Steffen Pingel from comment #15)
> I have updated the Gerrit review against the latest master. Jaroslav, Tomas,
> it'd great if you could take a look if that would satisfy your use case.
yes, it looks like what we were asking for. Thanks.

and sorry for the delay.
Comment 18 Steffen Pingel CLA 2013-09-26 18:18:32 EDT
I have submitted the change. We'll provide a snapshot build and update the API porting guide next week.