Skip to content
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

Add pub support #107

Merged
merged 19 commits into from Jul 31, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Add "get dependencies" handler and command

Signed-off-by: Jonas Hungershausen <jonas.hungershausen@vogella.com>
  • Loading branch information...
jonas-jonas committed Jul 24, 2019
commit 370dccd9bcb4f9787ee3a5c36ea4b1a92d814430
@@ -25,10 +25,7 @@ Require-Bundle: org.eclipse.ui.genericeditor;bundle-version="1.0.0",
org.eclipse.tm4e.ui,
com.google.guava;bundle-version="21.0.0",
org.eclipse.lsp4j.jsonrpc
Import-Package: org.eclipse.e4.core.contexts;version="1.7.0",
org.eclipse.e4.core.di;version="1.7.0",
org.eclipse.e4.core.di.annotations;version="1.6.0",
org.osgi.service.component.annotations;version="1.3.0",
Import-Package: org.osgi.service.component.annotations;version="1.3.0",
org.slf4j,
org.slf4j.spi
Service-Component: OSGI-INF/org.eclipse.dartboard.ListenerService.xml
@@ -71,6 +71,28 @@
</visibleWhen>
</command>
</menuContribution>
<menuContribution
locationURI="popup:org.eclipse.ui.popup.any?after=additions">
<menu
id="org.eclipse.dartboard.pubmenuentry"
label="Pub">
<command
commandId="org.eclipse.dartboard.commands.pubget"
style="push">
</command>
<visibleWhen>
<iterate>
<adapt
type="org.eclipse.core.resources.IResource">
<test
forcePluginActivation="true"
property="org.eclipse.dartboard.isDartProject">
</test>
</adapt>
</iterate>
</visibleWhen>
</menu>
</menuContribution>
</extension>
<extension point="org.eclipse.debug.ui.launchShortcuts">
<shortcut class="org.eclipse.dartboard.launch.LaunchShortcut" id="org.eclipse.dartboard.run_as_shortcut" label="Run as Dart Program" icon="icons/dart.png" modes="run">
@@ -191,4 +213,27 @@
scopeName="source.dart">
</snippet>
</extension>
<extension
point="org.eclipse.ui.commands">
<command
description="Get the current package&apos;s dependencies."
id="org.eclipse.dartboard.commands.pubget"
name="Get dependencies">
</command>
</extension>
<extension
point="org.eclipse.ui.handlers">
<handler
class="org.eclipse.dartboard.pub.PubGetHandler"
commandId="org.eclipse.dartboard.commands.pubget">
</handler>
</extension>
<extension
point="org.eclipse.ui.commandImages">
<image
commandId="org.eclipse.dartboard.commands.pubget"
disabledIcon="platform:/plugin/org.eclipse.pde.ui/icons/dlcl16/refresh.png"
icon="platform:/plugin/org.eclipse.pde.ui/icons/elcl16/refresh.png">
</image>
</extension>
</plugin>
@@ -13,26 +13,19 @@
*******************************************************************************/
package org.eclipse.dartboard;

import javax.inject.Inject;

import org.eclipse.core.resources.IWorkspace;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.dartboard.pub.PubspecChangeListener;
import org.eclipse.e4.core.contexts.ContextInjectionFactory;
import org.eclipse.e4.core.contexts.IEclipseContext;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;

@Component(immediate = true)
public class ListenerService {

@Inject
IEclipseContext context;

@Activate
public void registerListeners() {
IWorkspace workspace = ResourcesPlugin.getWorkspace();
workspace.addResourceChangeListener(ContextInjectionFactory.make(PubspecChangeListener.class, context));
workspace.addResourceChangeListener(new PubspecChangeListener());
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

Don't need to create an object for IWorkspace. We can call directly ResourcesPlugin.getWorkspace().addResourceChangeListener(new PubspecChangeListener());

This comment has been minimized.

Copy link
@jonas-jonas

jonas-jonas Jul 30, 2019

Author Member

Not now, but maybe later. There is no harm in having it this way.

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

There is no harm. except creating an unnecessary object reference :)

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

Don't we need to remove the listener?. I am not sure.

This comment has been minimized.

Copy link
@jonas-jonas

jonas-jonas Jul 30, 2019

Author Member

Yes, ideally we would remove the listener when the user unchecks the checkbox on the preference page. But as the ListenerService is not available in the class in the current design, the listener stays registered as is and the necessary checks are done whenever it fires.

And it should stay registered until the workspace is closed.

}

}
@@ -0,0 +1,36 @@
package org.eclipse.dartboard.pub;

import org.eclipse.core.commands.AbstractHandler;
import org.eclipse.core.commands.ExecutionEvent;
import org.eclipse.core.commands.ExecutionException;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.Adapters;
import org.eclipse.core.runtime.preferences.InstanceScope;
import org.eclipse.dartboard.Constants;
import org.eclipse.jface.viewers.ISelection;
import org.eclipse.jface.viewers.IStructuredSelection;
import org.eclipse.ui.handlers.HandlerUtil;
import org.eclipse.ui.preferences.ScopedPreferenceStore;

public class PubGetHandler extends AbstractHandler {

private PubService pub;

private ScopedPreferenceStore preferences;

public PubGetHandler() {
pub = PubService.getInstance();
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

Same here also. You can directly call the static method instead of creating an object.

This comment has been minimized.

Copy link
@jonas-jonas

jonas-jonas Jul 30, 2019

Author Member

PubService is supposed to be a Singleton. And since it has some Data stored, it would not be ideal to make it all static.

Ideally I would like to provide it as an OSGi service and have it injected into the classes I need it. This however is not possible for pages (see DartPreferencePage) that are defined via extension points. At least I could not figure out how to do so.

Once we have a working OSGi service structure for this, the refactoring would be minimal, as we can just add a @Inject to the field instead of changing all method calls to be non-static. That's why I used the Singlet pattern for now.

preferences = new ScopedPreferenceStore(InstanceScope.INSTANCE, Constants.PLUGIN_ID);
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

I think it's not the right place to create the object. Is the preference store object is not available at this point? If not, we need to create in startup class and need to access like activator

  public IPreferenceStore getPreferenceStore() {
  	if (preferenceStore == null) {
  		preferenceStore = new ScopedPreferenceStore(InstanceScope.INSTANCE, Constants.PLUGIN_ID);

  	}
  	return preferenceStore;
  }

This comment has been minimized.

Copy link
@jonas-jonas

jonas-jonas Jul 30, 2019

Author Member

I'm not sure what the benefit of a globally accessible constant/variable would be. Again, it would be nice to have it as an @Inject'able service, but I have not figured out how to do this. I will have to wait for @vogella for advice on this.

Also it is done this way all over the codebase right now and would require a major refactor that is IMO out of scope of this PR.

}

@Override
public Object execute(ExecutionEvent event) throws ExecutionException {
ISelection sel = HandlerUtil.getCurrentSelection(event);
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

We can use directly org.eclipse.ui.handlers.HandlerUtil.getCurrentStructuredSelection(ExecutionEvent). Instead of typecasting.

if (sel instanceof IStructuredSelection) {
IResource res = Adapters.adapt(((IStructuredSelection) sel).getFirstElement(), IResource.class);
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

Do we really need to call the adapt method? I think we can write like this

		IStructuredSelection structuredSelection = HandlerUtil.getCurrentStructuredSelection(event);
		if (structuredSelection.getFirstElement() instanceof IResource) {
                         // Do it
		}

pub.get(res.getProject(), preferences.getBoolean(Constants.PREFERENCES_OFFLINE_PUB));
}
return null;
}

}
@@ -36,7 +36,6 @@
import org.eclipse.dartboard.Messages;
import org.eclipse.dartboard.util.DartUtil;
import org.eclipse.dartboard.util.StatusUtil;
import org.eclipse.e4.core.di.annotations.Creatable;
import org.eclipse.osgi.util.NLS;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -51,12 +50,15 @@
* @see PubspecChangeListener
*
*/
@Creatable
@Singleton
public class PubService {

private static final Logger LOG = LoggerFactory.getLogger(PubService.class);

private static PubService instance;

private PubService() {
}

/**
* A {@link Map} holding an {@link IProject} and a {@link Job} as ideally there
* should only be one {@code pub get} job running per project.
@@ -153,4 +155,19 @@ public void done(IJobChangeEvent event) {
pubSync.schedule(1000);
pubGetJobs.put(project, pubSync);
}

/**
* Returns an instance of {@link PubService}
*
* If {@link #instance} is null, a new instance is created.
*
* @return The instance of {@link PubService} defined in {@link #instance}
*/
public static PubService getInstance() {
// TODO: Turn this class into a @Inject'able service?
if (instance == null) {
instance = new PubService();
}
return instance;
}
}
@@ -28,6 +28,7 @@

public PubspecChangeListener() {
preferences = new ScopedPreferenceStore(InstanceScope.INSTANCE, Constants.PLUGIN_ID);
This conversation was marked as resolved by jonas-jonas

This comment has been minimized.

Copy link
@lak-proddev

lak-proddev Jul 30, 2019

Contributor

It's a duplicate object for the preference store. We need to access the same object in a whole plugin

This comment has been minimized.

Copy link
@jonas-jonas

jonas-jonas Jul 30, 2019

Author Member

It's a duplicate object for the preference store. We need to access the same object in a whole plugin

That's not true. If I understand this https://www.vogella.com/tutorials/EclipsePreferences/article.html correctly, a new object should be created every time it is needed. But again, OSGi service would be nice here.

pub = PubService.getInstance();
}

@Override
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.