Fixes #4924. Setting Shortcut.Key to Key.Empty now removes the key binding#4925
Merged
Fixes #4924. Setting Shortcut.Key to Key.Empty now removes the key binding#4925
Conversation
…nding - Remove the `if (!Key.IsValid) return` early exit from UpdateKeyBindings, which prevented cleanup when Key was set to Key.Empty (IsValid is false for Empty). - Guard only the *add* path with `if (Key != Key.Empty)` so the old binding is always removed but Key.Empty is never registered as a new binding. - Add two unit tests that fail without the fix and pass with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1a3ccda to
7c96c5d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #4924 by ensuring that setting Shortcut.Key = Key.Empty actually removes any previously-registered key binding rather than leaving the old binding active.
Changes:
- Updated
Shortcut.UpdateKeyBindingsto always remove the old binding and to skip adding a new binding whenKeyisKey.Empty. - Added unit tests covering both view-scoped and application-scoped bindings being removed when
Keyis set toKey.Empty.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Terminal.Gui/Views/Shortcut.cs |
Adjusts key-binding update logic to allow unbinding via Key.Empty. |
Tests/UnitTestsParallelizable/Views/ShortcutTests.KeyDown.cs |
Adds regression tests for unbinding behavior; minor cleanup to event handlers/loops. |
BDisp
approved these changes
Apr 11, 2026
Collaborator
BDisp
left a comment
There was a problem hiding this comment.
Great. Now assigning a command to a key is always guaranteed, as long as the key is not empty.
This was referenced May 1, 2026
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.
Fixes #4924
Problem
Shortcut.UpdateKeyBindingshad an early-return guard:Key.IsValidisfalseforKey.Empty, so settingshortcut.Key = Key.Emptywas a no-op — the previously-registered binding was never removed. The old key kept firing the action.Fix
if (!Key.IsValid) returnguard entirely.if (Key != Key.Empty), so:Key.Emptyis never registered as a new binding.Changes
Terminal.Gui/Views/Shortcut.csKey.Emptyguards on add pathsTests/…/ShortcutTests.KeyDown.csExamples/UICatalog/Scenarios/Keys.csStatusBarwithDim.Fill(statusBar)for list viewsTerminal.Gui/Drivers/AnsiDriver/AnsiInput.csKeyboardFlagsas a public propertyTerminal.Gui/Drivers/AnsiDriver/AnsiComponentFactory.csTerminal.Gui/Drivers/DriverImpl.csKittyKeyboardProtocolpublicTerminal.Gui/Drivers/IDriver.csTests/…/InitTests.csTests/…/AllDriverTests.csTests
Two new tests in
ShortcutTests.KeyDown.cs:Setting_Key_To_Empty_Removes_KeyBinding— view-scoped bindingSetting_Key_To_Empty_Removes_AppLevel_KeyBinding— app-scoped (BindKeyToApplication = true)Both fail on the unfixed code and pass with the fix.