From e1201e29558a9af0ba067caea0c640b7cf67f4e5 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 13 Aug 2026 14:02:38 +0200 Subject: [PATCH] Look up the displaced action by command id in KeyBindingService registerAction scanned every already registered action to find the one holding the command id it is about to take over. AbstractTextEditor registers dozens of actions per editor through setAction, so registering n actions cost O(n^2) comparisons on every editor open. Keep a map from command id to action, and its inverse for removal, so the lookup is a single map access. The scan also removed entries from the keySet it was iterating and only avoided a ConcurrentModificationException because of the break directly after, which the map lookup makes moot. The maps are keyed by the command id an action was registered under rather than by its current definition id. These differ only if getActionDefinitionId changes after registration, in which case eviction now matches the id the handler activation actually uses instead of one the activation was never created for. Add KeyBindingServiceTest, which pins down that registering an action for an already bound command id displaces the previous action, and that the displaced action is deactivated rather than shadowed, so it does not resurface once the replacement is unregistered. That behaviour had no coverage. The tests pass unchanged against the previous implementation. This is not a measurable speedup. Timed with OpenCloseEditorTest over three alternating runs per variant, the difference stays under the run to run spread of the baseline itself. --- .../ui/internal/KeyBindingService.java | 29 ++- .../ui/tests/keys/KeyBindingServiceTest.java | 166 ++++++++++++++++++ .../eclipse/ui/tests/keys/KeysTestSuite.java | 3 +- 3 files changed, 190 insertions(+), 8 deletions(-) create mode 100644 tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeyBindingServiceTest.java diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/KeyBindingService.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/KeyBindingService.java index c5a00749a93..d1e3942d04b 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/KeyBindingService.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/KeyBindingService.java @@ -72,6 +72,15 @@ public final class KeyBindingService implements INestableKeyBindingService { private final Map actionToProxy = new HashMap<>(); + /** + * Reverse view of {@link #actionToProxy} keyed by the command id an action was + * registered under, so that registering an action does not have to scan every + * action already registered. + */ + private final Map commandIdToAction = new HashMap<>(); + + private final Map actionToCommandId = new HashMap<>(); + /** * Constructs a new instance of KeyBindingService on a given * workbench site. This instance is not nested. @@ -247,6 +256,8 @@ public void dispose() { IHandlerService hs = workbenchPartSite.getService(IHandlerService.class); hs.deactivateHandlers(actionToProxy.values()); actionToProxy.clear(); + commandIdToAction.clear(); + actionToCommandId.clear(); } } @@ -316,17 +327,17 @@ public void registerAction(IAction action) { String commandId = action.getActionDefinitionId(); if (commandId != null) { - for (IAction registeredAction : actionToProxy.keySet()) { - // we also need to unregister any other action that may have - // been registered with the same definition id - if (commandId.equals(registeredAction.getActionDefinitionId())) { - unregisterAction(registeredAction); - break; - } + // we also need to unregister any other action that may have + // been registered with the same definition id + IAction registeredAction = commandIdToAction.get(commandId); + if (registeredAction != null) { + unregisterAction(registeredAction); } IHandlerService hs = workbenchPartSite.getService(IHandlerService.class); actionToProxy.put(action, hs.activateHandler(commandId, new ActionHandler(action))); + commandIdToAction.put(commandId, action); + actionToCommandId.put(action, commandId); } } @@ -400,6 +411,10 @@ public void unregisterAction(IAction action) { } IHandlerActivation activation = actionToProxy.remove(action); + String commandId = actionToCommandId.remove(action); + if (commandId != null) { + commandIdToAction.remove(commandId, action); + } if (activation == null) { return; } diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeyBindingServiceTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeyBindingServiceTest.java new file mode 100644 index 00000000000..104cfb8f7b5 --- /dev/null +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/keys/KeyBindingServiceTest.java @@ -0,0 +1,166 @@ +/******************************************************************************* + * Copyright (c) 2026 Vogella GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Lars Vogel - initial API and implementation + *******************************************************************************/ +package org.eclipse.ui.tests.keys; + +import static org.eclipse.ui.tests.harness.util.UITestUtil.openTestWindow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.eclipse.core.commands.Category; +import org.eclipse.core.commands.Command; +import org.eclipse.core.commands.NotHandledException; +import org.eclipse.e4.core.commands.internal.HandlerServiceImpl; +import org.eclipse.jface.action.Action; +import org.eclipse.ui.IKeyBindingService; +import org.eclipse.ui.IPageLayout; +import org.eclipse.ui.IViewPart; +import org.eclipse.ui.IWorkbenchWindow; +import org.eclipse.ui.commands.ICommandService; +import org.eclipse.ui.handlers.IHandlerService; +import org.eclipse.ui.tests.harness.util.CloseTestWindowsExtension; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +/** + * Tests how {@link IKeyBindingService} binds actions to commands, in particular + * that registering an action displaces any action previously registered for the + * same command id. + */ +@ExtendWith(CloseTestWindowsExtension.class) +public class KeyBindingServiceTest { + + private static final String CATEGORY_ID = "org.eclipse.ui.tests.keys.keyBindingServiceCategory"; + + private static final String CMD_ID = "org.eclipse.ui.tests.keys.keyBindingServiceCommand"; + + private static final String OTHER_CMD_ID = "org.eclipse.ui.tests.keys.keyBindingServiceOtherCommand"; + + /** Records how often it was run, so we can tell which action is bound. */ + private static final class CountingAction extends Action { + private int runCount; + + CountingAction(String actionDefinitionId) { + setId(actionDefinitionId); + setActionDefinitionId(actionDefinitionId); + } + + @Override + public void run() { + runCount++; + } + } + + private IKeyBindingService keyBindingService; + + private IHandlerService handlerService; + + @BeforeEach + public void setUp() throws Exception { + IWorkbenchWindow window = openTestWindow(); + ICommandService commandService = window.getWorkbench().getService(ICommandService.class); + defineCommand(commandService, CMD_ID, "Key Binding Service Test Command"); + defineCommand(commandService, OTHER_CMD_ID, "Key Binding Service Other Test Command"); + + IViewPart view = window.getActivePage().showView(IPageLayout.ID_OUTLINE); + keyBindingService = view.getSite().getKeyBindingService(); + handlerService = view.getSite().getService(IHandlerService.class); + } + + private static void defineCommand(ICommandService commandService, String commandId, String name) + throws Exception { + Command command = commandService.getCommand(commandId); + if (!command.isDefined()) { + Category category = commandService.getCategory(CATEGORY_ID); + if (!category.isDefined()) { + category.define("Key Binding Service Tests", null); + } + command.define(name, null, category); + command.setHandler(HandlerServiceImpl.getHandler(commandId)); + } + } + + @Test + public void registeringAnActionBindsItToItsCommand() throws Exception { + CountingAction action = new CountingAction(CMD_ID); + keyBindingService.registerAction(action); + + handlerService.executeCommand(CMD_ID, null); + + assertEquals(1, action.runCount, "Registered action should have run"); + } + + @Test + public void registeringASecondActionForTheSameCommandReplacesTheFirst() throws Exception { + CountingAction first = new CountingAction(CMD_ID); + CountingAction second = new CountingAction(CMD_ID); + keyBindingService.registerAction(first); + keyBindingService.registerAction(second); + + handlerService.executeCommand(CMD_ID, null); + + assertEquals(0, first.runCount, "Replaced action should no longer be bound"); + assertEquals(1, second.runCount, "Most recently registered action should be bound"); + } + + /** + * The action displaced by a second registration must be deactivated, not merely + * shadowed, otherwise it would resurface once the second one is unregistered. + */ + @Test + public void unregisteringTheReplacementLeavesTheCommandUnhandled() throws Exception { + CountingAction first = new CountingAction(CMD_ID); + CountingAction second = new CountingAction(CMD_ID); + keyBindingService.registerAction(first); + keyBindingService.registerAction(second); + keyBindingService.unregisterAction(second); + + assertThrows(NotHandledException.class, () -> handlerService.executeCommand(CMD_ID, null)); + assertEquals(0, first.runCount, "Replaced action should not resurface"); + } + + @Test + public void unregisteringTheOnlyActionLeavesTheCommandUnhandled() throws Exception { + CountingAction action = new CountingAction(CMD_ID); + keyBindingService.registerAction(action); + keyBindingService.unregisterAction(action); + + assertThrows(NotHandledException.class, () -> handlerService.executeCommand(CMD_ID, null)); + } + + @Test + public void registeringTheSameActionTwiceKeepsItBound() throws Exception { + CountingAction action = new CountingAction(CMD_ID); + keyBindingService.registerAction(action); + keyBindingService.registerAction(action); + + handlerService.executeCommand(CMD_ID, null); + + assertEquals(1, action.runCount, "Re-registered action should still be bound exactly once"); + } + + @Test + public void actionsForDifferentCommandsDoNotDisplaceEachOther() throws Exception { + CountingAction action = new CountingAction(CMD_ID); + CountingAction otherAction = new CountingAction(OTHER_CMD_ID); + keyBindingService.registerAction(action); + keyBindingService.registerAction(otherAction); + + handlerService.executeCommand(CMD_ID, null); + handlerService.executeCommand(OTHER_CMD_ID, null); + + assertEquals(1, action.runCount, "Action for the first command should still be bound"); + assertEquals(1, otherAction.runCount, "Action for the second command should be bound"); + } +} 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 56a2bbb10eb..2281d2ddc4a 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 @@ -38,7 +38,8 @@ Bug44460Test.class, Bug53489Test.class, Bug189167Test.class, - KeysPreferenceModelTest.class + KeysPreferenceModelTest.class, + KeyBindingServiceTest.class }) public class KeysTestSuite {