Bug 355057

Comment 1 Steffen Pingel 2011-08-18 07:44:49 EDT

If I remove the <enabledWhen/> expression it works when I invoke the command from the search box. Seems strange though since I would expect the expression to evaluate to true.

      <handler
            class="org.eclipse.mylyn.internal.discovery.ui.commands.ShowConnectorDiscoveryWizardCommandHandler"
            commandId="org.eclipse.mylyn.discovery.ui.discoveryWizardCommand">
         <enabledWhen>
     		<test
     			property="org.eclipse.core.runtime.isBundleInstalled" args="org.eclipse.equinox.p2.repository"/>
         </enabledWhen>
      </handler>

Comment 2 Remy Suen 2011-08-18 07:58:42 EDT

Steffen, what does your plugin.xml look like? We have a report of enabledWhen not working properly yesterday, see bug 354967.

Comment 3 Steffen Pingel 2011-08-18 08:06:57 EDT

Created attachment 201712 [details]
org.eclipse.mylyn.commons/org.eclipse.mylyn.discovery.ui/plugin.xml

Comment 4 Remy Suen 2011-08-18 08:09:55 EDT

Whoops, think I'm barking up the wrong tree.

So you say this wizard is invoked via a button and possibly related to bug 321263. So the button actually calls Java APIs to run the wizard and the button click is not a result of a tool item (defined in some plugin.xml) being clicked, correct?

Comment 5 Steffen Pingel 2011-08-18 08:18:52 EDT

The button programmatically invokes the command. The weird thing is though that even if I invoke the command directly it doesn't work which makes me believe that the cause of the problem is actually related to the command enablement.

Comment 6 Remy Suen 2011-08-18 09:22:46 EDT

(In reply to comment #5)
> The button programmatically invokes the command.

What does the Java code look like?

Comment 7 Steffen Pingel 2011-08-18 10:15:32 EDT

Stepping through the code the CanExecute check in HandlerServiceImpl.executeHandler(ParameterizedCommand command, IEclipseContext staticContext) returns false and hence the command does not execute.

Invocation code:

pre.. 

		final Command discoveryWizardCommand = TasksUiInternal.getConfiguredDiscoveryWizardCommand();
		if (discoveryWizardCommand != null && discoveryWizardCommand.isEnabled()) {
...
				public void widgetSelected(SelectionEvent event) {
					IHandlerService handlerService = (IHandlerService) PlatformUI.getWorkbench().getService(
							IHandlerService.class);
					try {
						handlerService.executeCommand(discoveryWizardCommand.getId(), null);
					} catch (Exception e) {
...
					}
				}


	public static Command getConfiguredDiscoveryWizardCommand() {
		ICommandService service = (ICommandService) PlatformUI.getWorkbench().getService(ICommandService.class);
		final Command discoveryWizardCommand = service.getCommand("org.eclipse.mylyn.tasks.ui.discoveryWizardCommand"); //$NON-NLS-1$
		if (discoveryWizardCommand != null) {
			IHandlerService handlerService = (IHandlerService) PlatformUI.getWorkbench().getService(
					IHandlerService.class);
			EvaluationContext evaluationContext = createDiscoveryWizardEvaluationContext(handlerService);
			// update enabled state in case something has changed (ProxyHandler caches state)
			discoveryWizardCommand.setEnabled(evaluationContext);
		}
		return discoveryWizardCommand;
	}

	public static EvaluationContext createDiscoveryWizardEvaluationContext(IHandlerService handlerService) {
		EvaluationContext evaluationContext = new EvaluationContext(handlerService.getCurrentState(), Platform.class);
		// must specify this variable otherwise the PlatformPropertyTester won't work
		evaluationContext.addVariable("platform", Platform.class); //$NON-NLS-1$
		return evaluationContext;
	}

Comment 8 Remy Suen 2011-08-18 10:32:39 EDT

I have reproduced the problem locally on a similar snippet of code and xml, thank you Steffen for your information.

org.eclipse.core.runtime.CoreException: No property tester contributes a property org.eclipse.core.runtime.isBundleInstalled to type class java.util.Collections$EmptyList
	at org.eclipse.core.internal.expressions.TypeExtensionManager.getProperty(TypeExtensionManager.java:123)
	at org.eclipse.core.internal.expressions.TestExpression.evaluate(TestExpression.java:96)
	at org.eclipse.ui.internal.services.EvaluationReference.evaluate(EvaluationReference.java:93)
	at org.eclipse.ui.internal.services.EvaluationReference.evaluate(EvaluationReference.java:130)
	at org.eclipse.ui.internal.services.EvaluationReference.changed(EvaluationReference.java:124)
	at org.eclipse.e4.core.internal.contexts.TrackableComputationExt.update(TrackableComputationExt.java:94)
	at org.eclipse.e4.core.internal.contexts.EclipseContext.runAndTrack(EclipseContext.java:316)
	at org.eclipse.ui.internal.services.EvaluationService.addEvaluationReference(EvaluationService.java:199)
	at org.eclipse.ui.internal.services.EvaluationService.addEvaluationListener(EvaluationService.java:184)
	at org.eclipse.ui.internal.handlers.HandlerProxy.registerEnablement(HandlerProxy.java:213)
	at org.eclipse.ui.internal.handlers.HandlerProxy.<init>(HandlerProxy.java:192)
	at org.eclipse.ui.internal.handlers.LegacyHandlerService.readHandlers(LegacyHandlerService.java:621)
	at org.eclipse.ui.internal.handlers.LegacyHandlerService.readRegistry(LegacyHandlerService.java:558)

Comment 9 Remy Suen 2011-08-18 10:57:25 EDT

We added some code for this when fixing bug 334257. I have confirmed that explicitly defining the variable will workaround this problem.

That is:
<enabledWhen>
  <with variable="org.eclipse.core.runtime.Platform">
    <test
        property="org.eclipse.core.runtime.isBundleInstalled"
        args="org.eclipse.equinox.p2.repository"/>
  </with>
</enabledWhen>

Will dig around some more to see how the "simple" version is implemented in 3.x.

Comment 10 Remy Suen 2011-08-18 11:28:17 EDT

(In reply to comment #7)
>         // must specify this variable otherwise the PlatformPropertyTester
> won't work
>         evaluationContext.addVariable("platform", Platform.class);
> //$NON-NLS-1$

I didn't see this code since I only read the first part of comment 7. So it seems explicitly changing your plugin.xml like the snippet I provided in comment 9 will make the button work in addition to Mylyn not having to have this piece of code.

I'll have to investigate further to determine the legitimacy of this piece of code to get that corresponding snippet of xml to work.

Comment 11 Remy Suen 2011-09-13 15:00:19 EDT

Steffen, do you know why the code was explicitly added in comment 7 instead of just using the XML described in comment 9.

Comment 12 Steffen Pingel 2011-09-14 13:17:24 EDT

Not entirely sure why we update the command enablement state programmatically, sounds like cached state would sometimes cause the check to fail even though discovery (p2) was available.

Comment 13 Remy Suen 2012-02-17 08:29:54 EST

(In reply to comment #9)
> <enabledWhen>
>   <with variable="org.eclipse.core.runtime.Platform">
>     <test
>         property="org.eclipse.core.runtime.isBundleInstalled"
>         args="org.eclipse.equinox.p2.repository"/>
>   </with>
> </enabledWhen>

Please change your plugin.xml to match the code above. Paul has told me that this is a requirement.

Comment 14 Steffen Pingel 2012-02-17 10:22:22 EST

Thanks a lot! I have made this change and will trigger a new build to test the changes.

Comment 15 Steffen Pingel 2012-02-17 11:47:49 EST

I had to make an additional change to the code. Here is what we used to call which ends up invoking MakeHandlersGo.setEnabled() which is noop:

  discoveryWizardCommand.setEnabled(evaluationContext);
  
I changed the code to do this:

  discoveryWizardCommand.isEnabled()
  
That triggers HandlerProxy.setEnabled() which then does the right thing. 

With the change to the extension declaration and the code the command enablement check now works as expected.