Moving PublishedGradleVersionsWrapper to a declarative OSGi service#1152
Moving PublishedGradleVersionsWrapper to a declarative OSGi service#1152vogella wants to merge 1 commit into
Conversation
| * information is provided. This handles, for example, those scenarios where the | ||
| * versions cannot be retrieved because the user is behind a proxy or offline. | ||
| */ | ||
| @Component(property = { "service.ranking:Integer=1" }, service = PublishedGradleVersionsWrapper.class) |
There was a problem hiding this comment.
Do you really need a non default service rank here?
There was a problem hiding this comment.
Do you really need a non default service rank here?
If you look at the activator it also sets this ranking so I just replicated it.
There was a problem hiding this comment.
This is a final class, it is very unlikley that there are more than one service and even if they seem to be totaly eqivialent so this is most probably obsolete.
Service ranking is only important if there are multiple service of the same type and you need to have some kind of ordering of them.
There was a problem hiding this comment.
This is a final class, it is very unlikley that there are more than one service and even if they seem to be totaly eqivialent so this is most probably obsolete.
Service ranking is only important if there are multiple service of the same type and you need to have some kind of ordering of them.
Agree, but changing logic AND implementation is IMHO a bad way. So leaving the priority in should allow @donat to review this easier. And next step could be to remove with a follow-up commit.
|
|
||
| public static PublishedGradleVersionsWrapper publishedGradleVersions() { | ||
| return (PublishedGradleVersionsWrapper) getInstance().publishedGradleVersionsServiceTracker.getService(); | ||
| BundleContext bundleContext = FrameworkUtil.getBundle(CorePlugin.class).getBundleContext(); |
There was a problem hiding this comment.
You should really use a servicetracker here see for example here how to handle such legacy cases:
https://github.com/eclipse-m2e/m2e-core/blob/25d6833f9cf78e0c423155c9d8381f0db8eaa9d0/org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/MavenPluginActivator.java#L114
There was a problem hiding this comment.
How is the tracker closed in your example?
There was a problem hiding this comment.
All services are automatically freed when the bundle stops, you can of course clear them manually if you like also see for a more complete example:
https://github.com/eclipse-platform/eclipse.platform.runtime/pull/28/files
In the current code you do not check for null and you increment the use-count on each call.
There was a problem hiding this comment.
Your decisions of course, I usually try to avoid migrating useless/outdated stuff as one has to test this twice then or no one feels to change this later on.
There was a problem hiding this comment.
Thanks, I update the PR with your suggestion.
With this change the PublishedGradleVersionsWrapper is published as declarative OSGi service instead registering it via the activator. For eclipse-buildship#1151
| } | ||
|
|
||
| private static <T> T getService(Class<T> service) { | ||
| BundleContext context = plugin.getBundle().getBundleContext(); |
There was a problem hiding this comment.
you might want to check if the plugin !=null
donat
force-pushed
the
master
branch
5 times, most recently
from
7e55112 to
7040f04
Compare
donat
force-pushed
the
master
branch
19 times, most recently
from
4c6e8d6 to
adde486
Compare
donat
force-pushed
the
master
branch
4 times, most recently
from
ffec47b to
03dc167
Compare
2 participants
With this change the PublishedGradleVersionsWrapper is published as
declarative OSGi service instead registering it via the activator.
For #1151
Fixes #?
Context