-
Notifications
You must be signed in to change notification settings - Fork 174
Moving PublishedGradleVersionsWrapper to a declarative OSGi service #1152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" name="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper"> | ||
| <property name="service.ranking" type="Integer" value="1"/> | ||
| <service> | ||
| <provide interface="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper"/> | ||
| </service> | ||
| <implementation class="org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersionsWrapper"/> | ||
| </scr:component> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,58 +18,60 @@ | |
| import org.eclipse.core.runtime.IStatus; | ||
| import org.eclipse.core.runtime.Status; | ||
| import org.eclipse.core.runtime.jobs.Job; | ||
|
|
||
| import org.osgi.service.component.annotations.Component; | ||
| import org.eclipse.buildship.core.internal.CorePlugin; | ||
| import org.eclipse.buildship.core.internal.util.gradle.PublishedGradleVersions.LookupStrategy; | ||
|
|
||
| /** | ||
| * Wraps the {@link PublishedGradleVersions} functionality in a background job that handles all | ||
| * exceptions gracefully. If an exception occurs while calling the underlying | ||
| * {@link PublishedGradleVersions} instance, default version information is provided. This handles, | ||
| * for example, those scenarios where the versions cannot be retrieved because the user is behind a | ||
| * proxy or offline. | ||
| * Wraps the {@link PublishedGradleVersions} functionality in a background job | ||
| * that handles all exceptions gracefully. If an exception occurs while calling | ||
| * the underlying {@link PublishedGradleVersions} instance, default version | ||
| * 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you really need a non default service rank here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you look at the activator it also sets this ranking so I just replicated it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 final class PublishedGradleVersionsWrapper { | ||
|
|
||
| private final AtomicReference<PublishedGradleVersions> publishedGradleVersions; | ||
| private final AtomicReference<PublishedGradleVersions> publishedGradleVersions; | ||
|
|
||
| public PublishedGradleVersionsWrapper() { | ||
| this.publishedGradleVersions = new AtomicReference<PublishedGradleVersions>(); | ||
| new LoadVersionsJob().schedule(); | ||
| } | ||
| public PublishedGradleVersionsWrapper() { | ||
| this.publishedGradleVersions = new AtomicReference<PublishedGradleVersions>(); | ||
| new LoadVersionsJob().schedule(); | ||
| } | ||
|
|
||
| public List<GradleVersion> getVersions() { | ||
| PublishedGradleVersions versions = this.publishedGradleVersions.get(); | ||
| return versions != null ? versions.getVersions() : ImmutableList.of(GradleVersion.current()); | ||
| } | ||
| public List<GradleVersion> getVersions() { | ||
| PublishedGradleVersions versions = this.publishedGradleVersions.get(); | ||
| return versions != null ? versions.getVersions() : ImmutableList.of(GradleVersion.current()); | ||
| } | ||
|
|
||
| /** | ||
| * Loads the published Gradle versions in the background. | ||
| * @author Stefan Oehme | ||
| */ | ||
| private final class LoadVersionsJob extends Job { | ||
| /** | ||
| * Loads the published Gradle versions in the background. | ||
| * | ||
| * @author Stefan Oehme | ||
| */ | ||
| private final class LoadVersionsJob extends Job { | ||
|
|
||
| public LoadVersionsJob() { | ||
| super("Loading available Gradle versions"); | ||
| setSystem(true); | ||
| } | ||
| public LoadVersionsJob() { | ||
| super("Loading available Gradle versions"); | ||
| setSystem(true); | ||
| } | ||
|
|
||
| @Override | ||
| protected IStatus run(IProgressMonitor monitor) { | ||
| try { | ||
| PublishedGradleVersions versions = PublishedGradleVersions.create(LookupStrategy.REMOTE_IF_NOT_CACHED); | ||
| PublishedGradleVersionsWrapper.this.publishedGradleVersions.set(versions); | ||
| } catch (RuntimeException e) { | ||
| CorePlugin.logger().warn("Could not load Gradle version information", e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| @Override | ||
| protected IStatus run(IProgressMonitor monitor) { | ||
| try { | ||
| PublishedGradleVersions versions = PublishedGradleVersions.create(LookupStrategy.REMOTE_IF_NOT_CACHED); | ||
| PublishedGradleVersionsWrapper.this.publishedGradleVersions.set(versions); | ||
| } catch (RuntimeException e) { | ||
| CorePlugin.logger().warn("Could not load Gradle version information", e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
|
|
||
| @Override | ||
| protected void canceling() { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| @Override | ||
| protected void canceling() { | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to check if the plugin !=null