Look up the displaced action by command id in KeyBindingService - #4243
Merged
vogella merged 1 commit intoAug 31, 2026
Merged
Conversation
Contributor
vogella
force-pushed
the
keybindingservice-registeraction-lookup
branch
from
August 30, 2026 15:53
e1201e2 to
47f2246
Compare
There was a problem hiding this comment.
Pull request overview
Optimizes key-binding action registration by replacing linear scans with command/action lookup maps and adds regression coverage.
Changes:
- Adds bidirectional command-to-action bookkeeping.
- Updates registration, unregistration, and disposal logic.
- Adds and registers
KeyBindingServicebehavior tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
KeyBindingService.java |
Implements constant-time displaced-action lookup. |
KeyBindingServiceTest.java |
Tests registration and displacement behavior. |
KeysTestSuite.java |
Adds the new tests to the suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 so the lookup is a single map access. Removal takes the id from the handler activation, which was created for exactly that id, so no inverse map is needed. 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 map is 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. Two further tests cover the changed definition id case and are the only ones that fail 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. Assisted-by: multiple AI agents and layers of automated tooling 🤖
vogella
force-pushed
the
keybindingservice-registeraction-lookup
branch
from
August 30, 2026 18:24
47f2246 to
bfa4da8
Compare
vogella
added
plan
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
KeyBindingService.registerActionscanned every already registered action to find the one currently holding the command id it is about to take over.AbstractTextEditorregisters dozens of actions per editor viasetAction, so registering n actions costs O(n^2) comparisons on every editor open. A map from command id to action turns that into a single lookup; removal takes the id from the handler activation, so no inverse map is needed. As a side effect the scan no longer removes entries from thekeySetit is iterating, which previously only avoided aConcurrentModificationExceptionbecause of thebreakimmediately after it.The map is keyed by the command id an action was registered under rather than by its current definition id. Those differ only when
getActionDefinitionIdchanges after registration, and in that case eviction now matches the id the handler activation actually uses, rather than one the activation was never created for.The PR also adds
KeyBindingServiceTest. Registering an action for an already bound command id displaces the previous action, and the displaced one has to be deactivated rather than shadowed so it does not resurface when the replacement is unregistered. None of that had any coverage. Two further tests pin down the changed definition id case and are the only ones that fail against the previous implementation.To be upfront about the performance side: this is not a measurable win. Timed with
OpenCloseEditorTestover three alternating runs per variant, the difference stays below the run to run spread of the baseline itself. The value here is the removed quadratic loop, the removed iteration hazard, and the new test coverage.