Fix commands being executed twice due to stale isEnabled() check #4080

In commit 35f5f09, the commandHandled evaluation in
KeyBindingDispatcher.executeCommand() was changed to include
command.isEnabled() in addition to handler.isHandled(). This causes
commands to be executed twice (e.g. Paste on a Combo widget):

- command.isEnabled() reads the handler's enabled state without first
  calling setEnabled(evaluationContext), so it returns stale state that
  does not reflect the currently focused widget context.
- Inside Command.executeWithChecks(), the framework calls
  setEnabled(event.getApplicationContext()) immediately before the same
  isEnabled() check, giving a fresh result. The two checks are not
  equivalent and can disagree for IHandler2 implementations.
- When command.isEnabled() incorrectly returns false, commandHandled is
  false, executeCommand() returns false, and the key event is not
  consumed (event.doit remains true) even though the command is actually
  executed. The OS then processes the keystroke natively, causing a
  second execution of the same action.

Fix 1: Revert determination of successful command handling to use only
handler.isHandled(), as it was before commit 35f5f09.

Fix 2: Change handleCommandExecution() to return a boolean (false if a
CommandException was recorded after execution) and use that return value
to update commandHandled in the synchronous non-Browser path. This
restores the original behaviour where a handler that claims isHandled()
but fails during execution (e.g. NotEnabledException after a fresh
setEnabled() evaluation) does not eat the key event. The async Browser
path is unchanged by design.

Fixes #4080

Author: HeikoKlare

bundles/org.eclipse.e4.ui.bindings/src/org/eclipse/e4/ui/bindings/keys/KeyBindingDispatcher.java

@@ -304,7 +304,7 @@ public final boolean executeCommand(final ParameterizedCommand parameterizedComm
 		Object obj = HandlerServiceImpl.lookUpHandler(context, command.getId());
 		if (obj != null) {
 			if (obj instanceof IHandler handler) {
-				commandHandled = command.isEnabled() && handler.isHandled();
+				commandHandled = handler.isHandled();
 			} else {
 				commandHandled = true;
 			}
@@ -319,12 +319,12 @@ public final boolean executeCommand(final ParameterizedCommand parameterizedComm
 			getDisplay()
 					.asyncExec(() -> handleCommandExecution(parameterizedCommand, handlerService, trigger, obj));
 		} else {
-			handleCommandExecution(parameterizedCommand, handlerService, trigger, obj);
+			commandHandled &= handleCommandExecution(parameterizedCommand, handlerService, trigger, obj);
 		}
 		return (commandDefined && commandHandled);
 	}
 
-	private void handleCommandExecution(final ParameterizedCommand parameterizedCommand,
+	private boolean handleCommandExecution(final ParameterizedCommand parameterizedCommand,
 			final EHandlerService handlerService, final Event trigger, Object handler) {
 		final IEclipseContext staticContext = createContext(trigger);
 		try {
@@ -360,6 +360,7 @@ private void handleCommandExecution(final ParameterizedCommand parameterizedComm
 						}
 					}
 				}
+				return false;
 			}
 		} finally {
 			staticContext.dispose();
@@ -371,6 +372,7 @@ private void handleCommandExecution(final ParameterizedCommand parameterizedComm
 		if (keyAssistDialog != null) {
 			keyAssistDialog.clearRememberedState();
 		}
+		return true;
 	}
 
 	private boolean isTracingEnabled() {

tests/org.eclipse.e4.ui.bindings.tests/src/org/eclipse/e4/ui/bindings/tests/KeyDispatcherTest.java

@@ -16,6 +16,8 @@
 package org.eclipse.e4.ui.bindings.tests;
 
 import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.*;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
@@ -24,7 +26,11 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import org.eclipse.core.commands.AbstractHandler;
 import org.eclipse.core.commands.Category;
+import org.eclipse.core.commands.Command;
+import org.eclipse.core.commands.ExecutionEvent;
+import org.eclipse.core.commands.ExecutionException;
 import org.eclipse.core.commands.ParameterizedCommand;
 import org.eclipse.core.commands.contexts.Context;
 import org.eclipse.core.commands.contexts.ContextManager;
@@ -197,6 +203,57 @@ public void testExecuteOneCommand() {
 		assertTrue(handler.q2);
 	}
 
+	/**
+	 * Regression test for https://github.com/eclipse-platform/eclipse.platform.ui/issues/4080
+	 * <p>
+	 * A handler's enabled state may be stale from a previous UI-refresh cycle and return
+	 * {@code false} even though the handler is capable of handling the command and would execute
+	 * it successfully. {@link KeyBindingDispatcher#executeCommand} must return {@code true}
+	 * whenever the handler is capable of handling the command, so that the caller consumes the key
+	 * event and prevents the OS from also processing it natively — which would cause the command to
+	 * execute twice.
+	 */
+	@Test
+	public void testExecuteCommandReturnsTrueWhenHandlerEnabledStateIsStale() throws Exception {
+		// Spy on an AbstractHandler (IHandler2) whose enabled state starts stale (false) but
+		// refreshes to true when Command.executeWithChecks() calls setEnabled(appContext).
+		AbstractHandler staleHandler = spy(new AbstractHandler() {
+			{
+				setBaseEnabled(false); // stale: disabled without an execution context
+			}
+
+			@Override
+			public void setEnabled(Object evaluationContext) {
+				setBaseEnabled(evaluationContext != null);
+			}
+
+			@Override
+			public Object execute(ExecutionEvent event) throws ExecutionException {
+				return null;
+			}
+		});
+
+		EHandlerService hs = workbenchContext.get(EHandlerService.class);
+		hs.activateHandler(TEST_ID1, staleHandler);
+
+		ECommandService cs = workbenchContext.get(ECommandService.class);
+		ParameterizedCommand paramCmd = cs.createCommand(TEST_ID1, null);
+		Command command = paramCmd.getCommand();
+		command.setHandler(staleHandler);
+
+		assertFalse(command.isEnabled(), "Precondition: command.isEnabled() must be false (stale state)");
+
+		Event event = new Event();
+		event.type = SWT.KeyDown;
+		event.stateMask = SWT.CTRL;
+		event.keyCode = 'A';
+
+		boolean result = dispatcher.executeCommand(paramCmd, event);
+
+		assertTrue(result, "executeCommand() must return true when the handler is capable of execution");
+		verify(staleHandler, times(1)).execute(any(ExecutionEvent.class));
+	}
+
 	@Test
 	public void testExecuteMultiStrokeBinding() {
 		assertFalse(twoStrokeHandler.q2);