Bug 517068 - key binding conflicts check for enabled commands

Invoking a keybinding wasn't taking into account whether command is
executable or not. This can be annoying in case of multiple commands
bound to the same binding: user got asked to choose which command to
invoke, but some of those commands were not executable and resulted in
an error message to user.
With a filter to only use executable commands, in most cases, we
shouldn't even see a pop-up as commands on a same shortcut are usually
mutually exclusive, and in case the pop-up shows up, it would only
propose commands that can run properly.

Change-Id: Ic62cd279652fd8ed42c58b66347e3dafea3861e4
Signed-off-by: Mickael Istria <mistria@redhat.com>

5 files changed, 215 insertions, 30 deletions

diff --git a/bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java b/bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
index 7ec51a55316..2dc4465325e 100644
--- a/bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
+++ b/bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java
@@ -1,19 +1,22 @@
 /*******************************************************************************
- * Copyright (c) 2009, 2015 IBM Corporation and others.
+ * Copyright (c) 2009, 2017 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- *     IBM Corporation - initial API and implementation
+ *  IBM Corporation - initial API and implementation
+ *  Mickael Istria (Red Hat Inc.) - [517068] conflicts consider enabled commands
  ******************************************************************************/
 
 package org.eclipse.e4.ui.bindings.keys;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
+import java.util.stream.Collectors;
 import javax.inject.Inject;
 import org.eclipse.core.commands.Command;
 import org.eclipse.core.commands.ExecutionException;
@@ -264,8 +267,7 @@ public class KeyBindingDispatcher {
 		final EHandlerService handlerService = getHandlerService();
 		final Command command = parameterizedCommand.getCommand();
 
-		final IEclipseContext staticContext = EclipseContextFactory.create("keys-staticContext"); //$NON-NLS-1$
-		staticContext.set(Event.class, trigger);
+		final IEclipseContext staticContext = createContext(trigger);
 
 		final boolean commandDefined = command.isDefined();
 		// boolean commandEnabled;
@@ -303,6 +305,12 @@ public class KeyBindingDispatcher {
 		return (commandDefined && commandHandled);
 	}
 
+	private IEclipseContext createContext(final Event trigger) {
+		final IEclipseContext staticContext = EclipseContextFactory.create("keys-staticContext"); //$NON-NLS-1$
+		staticContext.set(Event.class, trigger);
+		return staticContext;
+	}
+
 	/**
 	 * <p>
 	 * Launches the command matching a the typed key. This filter an incoming
@@ -406,19 +414,6 @@ public class KeyBindingDispatcher {
 	}
 
 	/**
-	 * Determines whether the key sequence is a perfect match for any command. If there is a match,
-	 * then the corresponding command identifier is returned.
-	 *
-	 * @param keySequence
-	 *            The key sequence to check for a match; must never be <code>null</code>.
-	 * @return The perfectly matching command; <code>null</code> if no command matches.
-	 */
-	private ParameterizedCommand getPerfectMatch(KeySequence keySequence) {
-		Binding perfectMatch = getBindingService().getPerfectMatch(keySequence);
-		return perfectMatch == null ? null : perfectMatch.getParameterizedCommand();
-	}
-
-	/**
 	 * Changes the key binding state to the given value. This should be an incremental change, but
 	 * there are no checks to guarantee this is so. It also sets up a <code>Shell</code> to be
 	 * displayed after one second has elapsed. This shell will show the user the possible
@@ -473,14 +468,38 @@ public class KeyBindingDispatcher {
 	}
 
 	/**
-	 * Determines whether the key sequence perfectly matches on of the active key bindings.
+	 * Determines whether the key sequence perfectly matches on of the active key
+	 * bindings.
 	 *
 	 * @param keySequence
-	 *            The key sequence to check for a perfect match; must never be <code>null</code>.
-	 * @return <code>true</code> if there is a perfect match; <code>false</code> otherwise.
+	 *            The key sequence to check for a perfect match; must never be
+	 *            <code>null</code>.
+	 * @param context
+	 * @return <code>true</code> if there is a perfect match; <code>false</code>
+	 *         otherwise.
+	 */
+	private boolean isUniqueMatch(KeySequence keySequence, IEclipseContext context) {
+		return getBindingService().isPerfectMatch(keySequence)
+				|| getExecutableMatches(keySequence, context).size() == 1;
+	}
+
+	/**
+	 * @param keySequence
+	 * @param context2
+	 * @return
 	 */
-	private boolean isPerfectMatch(KeySequence keySequence) {
-		return getBindingService().isPerfectMatch(keySequence);
+	private Collection<Binding> getExecutableMatches(KeySequence keySequence, IEclipseContext context2) {
+		Binding binding = getBindingService().getPerfectMatch(keySequence);
+		if (binding != null) {
+			return Collections.singleton(binding);
+		}
+		Collection<Binding> conflicts = getBindingService().getConflictsFor(keySequence);
+		if (conflicts != null) {
+			return conflicts.stream()
+					.filter(match -> getHandlerService().canExecute(match.getParameterizedCommand(), context))
+					.collect(Collectors.toList());
+		}
+		return Collections.emptySet();
 	}
 
 	/**
@@ -492,6 +511,7 @@ public class KeyBindingDispatcher {
 		KeySequence errorSequence = null;
 		Collection<Binding> errorMatch = null;
 
+		IEclipseContext createContext = createContext(event);
 		KeySequence sequenceBeforeKeyStroke = state;
 		for (KeyStroke keyStroke : potentialKeyStrokes) {
 			KeySequence sequenceAfterKeyStroke = KeySequence.getInstance(sequenceBeforeKeyStroke,
@@ -500,8 +520,9 @@ public class KeyBindingDispatcher {
 				incrementState(sequenceAfterKeyStroke);
 				return true;
 
-			} else if (isPerfectMatch(sequenceAfterKeyStroke)) {
-				final ParameterizedCommand cmd = getPerfectMatch(sequenceAfterKeyStroke);
+			} else if (isUniqueMatch(sequenceAfterKeyStroke, createContext)) {
+				final ParameterizedCommand cmd = getExecutableMatches(sequenceAfterKeyStroke, context).iterator().next()
+						.getParameterizedCommand();
 				try {
 					return executeCommand(cmd, event) || !sequenceBeforeKeyStroke.isEmpty();
 				} catch (final CommandException e) {
@@ -518,11 +539,9 @@ public class KeyBindingDispatcher {
 				return false;
 
 			} else {
-				Collection<Binding> matches = getBindingService().getConflictsFor(
-						sequenceAfterKeyStroke);
-				if (matches != null && !matches.isEmpty()) {
+				Collection<Binding> errorMatches = getExecutableMatches(sequenceAfterKeyStroke, context);
+				if (errorMatches != null && !errorMatches.isEmpty()) {
 					errorSequence = sequenceAfterKeyStroke;
-					errorMatch = matches;
 				}
 			}
 		}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/commands/CheckInvokedHandler.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/commands/CheckInvokedHandler.java
new file mode 100644
index 00000000000..364d5f23434
--- /dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/commands/CheckInvokedHandler.java
@@ -0,0 +1,26 @@
+/*******************************************************************************
+ * Copyright (c) 2017, Red Hat Inc. and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Mickael Istria (Red Hat Inc.)
+ ******************************************************************************/
+package org.eclipse.ui.tests.commands;
+
+import org.eclipse.core.commands.AbstractHandler;
+import org.eclipse.core.commands.ExecutionEvent;
+
+public class CheckInvokedHandler extends AbstractHandler {
+
+	public static boolean invoked = false;
+
+	@Override
+	public Object execute(ExecutionEvent event) {
+		invoked = true;
+		return Boolean.TRUE;
+	}
+
+}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/DispatcherTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/DispatcherTest.java
new file mode 100644
index 00000000000..2630263fe07
--- /dev/null
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/DispatcherTest.java
@@ -0,0 +1,93 @@
+/*******************************************************************************
+ * Copyright (c) 2017, Red Hat Inc. and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Mickael Istria (Red Hat Inc.)
+ ******************************************************************************/
+package org.eclipse.ui.tests.keys;
+
+import java.io.ByteArrayInputStream;
+import java.util.Arrays;
+
+import org.eclipse.core.resources.IFile;
+import org.eclipse.core.resources.IProject;
+import org.eclipse.core.resources.ResourcesPlugin;
+import org.eclipse.core.runtime.CoreException;
+import org.eclipse.core.runtime.NullProgressMonitor;
+import org.eclipse.e4.ui.bindings.keys.KeyBindingDispatcher;
+import org.eclipse.jface.bindings.keys.KeyStroke;
+import org.eclipse.swt.SWT;
+import org.eclipse.swt.widgets.Display;
+import org.eclipse.ui.IEditorPart;
+import org.eclipse.ui.ide.IDE;
+import org.eclipse.ui.tests.commands.CheckInvokedHandler;
+import org.eclipse.ui.tests.harness.util.UITestCase;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class DispatcherTest extends UITestCase {
+
+	private KeyBindingDispatcher dispatcher;
+	private IProject p;
+
+	@Override
+	public void doSetUp() throws CoreException {
+		this.dispatcher = getWorkbench().getService(KeyBindingDispatcher.class);
+		CheckInvokedHandler.invoked = false;
+		p = ResourcesPlugin.getWorkspace().getRoot().getProject(getClass().getName() + System.currentTimeMillis());
+		p.create(new NullProgressMonitor());
+		p.open(new NullProgressMonitor());
+	}
+
+	@Override
+	protected void doTearDown() throws Exception {
+		p.delete(true, new NullProgressMonitor());
+	}
+
+	public DispatcherTest() {
+		super(DispatcherTest.class.getSimpleName());
+	}
+
+	@Test
+	public void testDispatcherMultipleDisabledCommands() throws Exception {
+		IFile file = p.getFile("test.whatever");
+		try (ByteArrayInputStream stream = new ByteArrayInputStream("hello".getBytes())) {
+			file.create(stream, true, new NullProgressMonitor());
+		}
+		IEditorPart part = IDE.openEditor(getWorkbench().getActiveWorkbenchWindow().getActivePage(), file);
+		try {
+			int shellCount = Display.getCurrent().getShells().length;
+			dispatcher.press(Arrays.asList(new KeyStroke[] { KeyStroke.getInstance(SWT.CTRL, 'O') }), null);
+			Assert.assertFalse("No handler should have been invoked", CheckInvokedHandler.invoked);
+			Assert.assertEquals("No Shell should have been added", shellCount, Display.getCurrent().getShells().length);
+		} finally {
+			if (part != null) {
+				part.getEditorSite().getPage().closeEditor(part, false);
+			}
+		}
+	}
+
+	@Test
+	public void testDispatcherMultipleCommandsOnlyOneEnabled() throws Exception {
+		IFile file = p.getFile("test.content-type2");
+		try (ByteArrayInputStream stream = new ByteArrayInputStream("hello".getBytes())) {
+			file.create(stream, true, new NullProgressMonitor());
+		}
+		IEditorPart part = IDE.openEditor(getWorkbench().getActiveWorkbenchWindow().getActivePage(), file);
+		try {
+			int shellCount = Display.getCurrent().getShells().length;
+			dispatcher.press(Arrays.asList(new KeyStroke[] { KeyStroke.getInstance(SWT.CTRL, 'O') }), null);
+			Assert.assertTrue("Handler should have been invoked", CheckInvokedHandler.invoked);
+			Assert.assertEquals("No Shell should have been added", shellCount, Display.getCurrent().getShells().length);
+		} finally {
+			if (part != null) {
+				part.getEditorSite().getPage().closeEditor(part, false);
+			}
+		}
+	}
+
+}
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeysTestSuite.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeysTestSuite.java
index fdaad0daac6..60e391f2767 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeysTestSuite.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeysTestSuite.java
@@ -20,6 +20,7 @@ import org.junit.runners.Suite;
 @Suite.SuiteClasses({
 	BindingInteractionsTest.class,
 	BindingManagerTest.class,
+		DispatcherTest.class,
 	BindingPersistenceTest.class,
 	// TODO This no longer works due to focus issues related to key bindings
 	//Bug36420Test.class,
@@ -45,7 +46,7 @@ import org.junit.runners.Suite;
 	 */
 	//Bug53489Test.class,
 	Bug189167Test.class,
-	KeysPreferenceModelTest.class
+		KeysPreferenceModelTest.class
  })
 public class KeysTestSuite {
 
diff --git a/tests/org.eclipse.ui.tests/plugin.xml b/tests/org.eclipse.ui.tests/plugin.xml
index be54cdd855c..834d9f350cf 100644
--- a/tests/org.eclipse.ui.tests/plugin.xml
+++ b/tests/org.eclipse.ui.tests/plugin.xml
@@ -4440,5 +4440,51 @@
          </activeWhen>
       </projectConfigurator>
    </extension>
-   
+
+	<!-- Bug 517068 -->
+	<extension point="org.eclipse.ui.commands">
+		<command
+			name="testCommand1"
+			description="testCommand1"
+			id="org.eclipse.ui.tests.testCommand1"/>
+		<command
+			name="testCommand2"
+			description="testCommand2"
+			id="org.eclipse.ui.tests.testCommand2"/>
+	</extension>
+	<extension point="org.eclipse.ui.bindings">
+		<key
+			commandId="org.eclipse.ui.tests.testCommand1"
+			schemeId="org.eclipse.ui.defaultAcceleratorConfiguration"
+			sequence="Ctrl+O" />
+		<key
+			commandId="org.eclipse.ui.tests.testCommand2"
+			schemeId="org.eclipse.ui.defaultAcceleratorConfiguration"
+			sequence="Ctrl+O" />
+	</extension>
+	<extension point="org.eclipse.ui.handlers">
+		<handler
+			class="org.eclipse.ui.tests.commands.CheckInvokedHandler"
+			commandId="org.eclipse.ui.tests.testCommand1">
+			<activeWhen>
+				<with variable="activeEditorInput">
+					<adapt type="org.eclipse.core.resources.IFile">
+						<test property="org.eclipse.core.resources.contentTypeId" value="org.eclipse.ui.tests.content-type1" />
+					</adapt>
+				</with>
+			</activeWhen>
+		</handler>
+		<handler
+			class="org.eclipse.ui.tests.commands.CheckInvokedHandler"
+			commandId="org.eclipse.ui.tests.testCommand2">
+			<activeWhen>
+				<with variable="activeEditorInput">
+					<adapt type="org.eclipse.core.resources.IFile">
+						<test property="org.eclipse.core.resources.contentTypeId" value="org.eclipse.ui.tests.content-type2" />
+    				</adapt>
+				</with>
+			</activeWhen>
+		</handler>
+	</extension>
+
 </plugin>