From 6fcbde54843ea070cff3de884ccabd9668e4b2af Mon Sep 17 00:00:00 2001 From: Aryan gupta Date: Thu, 6 Aug 2026 15:14:00 +0530 Subject: [PATCH] [Keyboard Manager] Fix shortcut modifier display order in the new editor (#49707) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request Fix shortcut key display order in the new Keyboard Manager editor (C# WinUI). When recording a shortcut, modifier keys are now always displayed in the standard canonical order (Win → Ctrl → Alt → Shift → Action key), regardless of the order the user physically pressed them. This matches the existing behavior of the old C++ editor's `GetKeyVector` function. ## PR Checklist - [x] Closes: #48943 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments **Bug:** In the new C# KBM editor (`KeyboardManagerEditorUI`), the `GetFormattedKeyList()` method in `KeyboardHookHelper.cs` displayed modifier keys in the order the user pressed them rather than the standard display order. For example, pressing Shift before Win would show `Shift + Win + S` instead of `Win + Shift + S`. **Root cause:** The `modifierKeys` list was populated by iterating `_keyPressOrder` (which preserves temporal press order), and was then rendered directly without sorting. **Fix:** Added a sort step before the display loop that sorts modifier keys using the existing `KeyboardManagerInterop.GetKeyType()` P/Invoke, which returns the `KeyType` enum value (Win=0, Ctrl=1, Alt=2, Shift=3). This enforces the canonical order **Win → Ctrl → Alt → Shift → Action key**, matching the old C++ `EditorHelpers::GetKeyVector()` behavior. **Scope:** Single-line change in `KeyboardHookHelper.GetFormattedKeyList()`. This is a display-only fix — it does not affect the internal key tracking (`_keyPressOrder`), save/load logic, or hook behavior. **Changed file:** - `src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs` ## Validation Steps Performed 1. Open the new Keyboard Manager editor 2. Click the shortcut trigger button to start recording 3. Press modifier keys in non-standard order (e.g., press Shift first, then Win, then S) 4. **Before fix:** UI shows `Shift + Win + S` 5. **After fix:** UI shows `Win + Shift + S` (correct canonical order) 6. Verified standard-order input (e.g., Win → Shift → S) still displays correctly 7. Verified single modifier + action key shortcuts (e.g., Ctrl+C) display correctly 8. Verified all four modifiers (Win+Ctrl+Alt+Shift+Key) display in correct order regardless of press sequence 9. Verified saving and loading remappings is unaffected by the display change ```` --- PowerToys.slnx | 4 + .../KeyboardManagerEditorUI.UnitTests.csproj | 24 +++ .../RemappingHelperTests.cs | 182 ++++++++++++++++++ .../Helpers/KeyboardHookHelper.cs | 7 +- .../Helpers/RemappingHelper.cs | 29 ++- 5 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/KeyboardManagerEditorUI.UnitTests.csproj create mode 100644 src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/RemappingHelperTests.cs diff --git a/PowerToys.slnx b/PowerToys.slnx index f0ac6c974e..645a61167d 100644 --- a/PowerToys.slnx +++ b/PowerToys.slnx @@ -532,6 +532,10 @@ + + + + diff --git a/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/KeyboardManagerEditorUI.UnitTests.csproj b/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/KeyboardManagerEditorUI.UnitTests.csproj new file mode 100644 index 0000000000..d7b146c41d --- /dev/null +++ b/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/KeyboardManagerEditorUI.UnitTests.csproj @@ -0,0 +1,24 @@ + + + + + + true + win-x64 + win-arm64 + false + false + false + $(SolutionDir)$(Platform)\$(Configuration)\tests\KeyboardManagerEditorUI.UnitTests\ + Exe + + + + + + + + + + + diff --git a/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/RemappingHelperTests.cs b/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/RemappingHelperTests.cs new file mode 100644 index 0000000000..fe85be7d8a --- /dev/null +++ b/src/modules/keyboardmanager/KeyboardManagerEditorUI.UnitTests/RemappingHelperTests.cs @@ -0,0 +1,182 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using KeyboardManagerEditorUI.Helpers; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Windows.System; + +namespace KeyboardManagerEditorUI.UnitTests +{ + [TestClass] + public class RemappingHelperTests + { + // Test that modifier keys pressed in reverse order (Shift, Alt, Ctrl, Win) + // are sorted into the standard display order (Win, Ctrl, Alt, Shift) + [TestMethod] + public void SortModifierKeys_ShouldSortInCanonicalOrder_WhenPressedInReverseOrder() + { + // Arrange - keys in reverse of standard order + var keys = new List + { + VirtualKey.Shift, + VirtualKey.Menu, // Alt + VirtualKey.Control, + VirtualKey.LeftWindows, + }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert - should be Win, Ctrl, Alt, Shift + Assert.AreEqual(VirtualKey.LeftWindows, keys[0]); + Assert.AreEqual(VirtualKey.Control, keys[1]); + Assert.AreEqual(VirtualKey.Menu, keys[2]); + Assert.AreEqual(VirtualKey.Shift, keys[3]); + } + + // Test the specific bug scenario: Win+Shift+S where Shift was pressed before Win + [TestMethod] + public void SortModifierKeys_ShouldShowWinBeforeShift_WhenShiftPressedFirst() + { + // Arrange - Shift pressed before Win (the reported bug scenario) + var keys = new List + { + VirtualKey.Shift, + VirtualKey.LeftWindows, + }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert - Win should come before Shift + Assert.AreEqual(VirtualKey.LeftWindows, keys[0]); + Assert.AreEqual(VirtualKey.Shift, keys[1]); + } + + // Test that keys already in the correct order remain unchanged + [TestMethod] + public void SortModifierKeys_ShouldPreserveOrder_WhenAlreadyInCanonicalOrder() + { + // Arrange - already in correct order + var keys = new List + { + VirtualKey.LeftWindows, + VirtualKey.Control, + VirtualKey.Menu, + VirtualKey.Shift, + }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert - order should be unchanged + Assert.AreEqual(VirtualKey.LeftWindows, keys[0]); + Assert.AreEqual(VirtualKey.Control, keys[1]); + Assert.AreEqual(VirtualKey.Menu, keys[2]); + Assert.AreEqual(VirtualKey.Shift, keys[3]); + } + + // Test that left/right variants of the same modifier maintain their relative + // position but are grouped correctly relative to other modifier types + [TestMethod] + public void SortModifierKeys_ShouldSortCorrectly_WithLeftRightVariants() + { + // Arrange - right shift before left control + var keys = new List + { + VirtualKey.RightShift, + VirtualKey.LeftControl, + }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert - Ctrl should come before Shift + Assert.AreEqual(VirtualKey.LeftControl, keys[0]); + Assert.AreEqual(VirtualKey.RightShift, keys[1]); + } + + // Test with a single modifier key (should not throw) + [TestMethod] + public void SortModifierKeys_ShouldHandleSingleModifier() + { + // Arrange + var keys = new List { VirtualKey.Control }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert + Assert.AreEqual(1, keys.Count); + Assert.AreEqual(VirtualKey.Control, keys[0]); + } + + // Test with an empty list (should not throw) + [TestMethod] + public void SortModifierKeys_ShouldHandleEmptyList() + { + // Arrange + var keys = new List(); + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert + Assert.AreEqual(0, keys.Count); + } + + // Test GetModifierSortOrder returns correct values + [TestMethod] + public void GetModifierSortOrder_ShouldReturnCorrectOrder_ForAllModifierTypes() + { + // Win keys should return 0 + Assert.AreEqual(0, RemappingHelper.GetModifierSortOrder(VirtualKey.LeftWindows)); + Assert.AreEqual(0, RemappingHelper.GetModifierSortOrder(VirtualKey.RightWindows)); + + // Ctrl keys should return 1 + Assert.AreEqual(1, RemappingHelper.GetModifierSortOrder(VirtualKey.Control)); + Assert.AreEqual(1, RemappingHelper.GetModifierSortOrder(VirtualKey.LeftControl)); + Assert.AreEqual(1, RemappingHelper.GetModifierSortOrder(VirtualKey.RightControl)); + + // Alt keys should return 2 + Assert.AreEqual(2, RemappingHelper.GetModifierSortOrder(VirtualKey.Menu)); + Assert.AreEqual(2, RemappingHelper.GetModifierSortOrder(VirtualKey.LeftMenu)); + Assert.AreEqual(2, RemappingHelper.GetModifierSortOrder(VirtualKey.RightMenu)); + + // Shift keys should return 3 + Assert.AreEqual(3, RemappingHelper.GetModifierSortOrder(VirtualKey.Shift)); + Assert.AreEqual(3, RemappingHelper.GetModifierSortOrder(VirtualKey.LeftShift)); + Assert.AreEqual(3, RemappingHelper.GetModifierSortOrder(VirtualKey.RightShift)); + } + + // Test that non-modifier keys get the highest sort order (4) + [TestMethod] + public void GetModifierSortOrder_ShouldReturnFour_ForNonModifierKeys() + { + Assert.AreEqual(4, RemappingHelper.GetModifierSortOrder(VirtualKey.A)); + Assert.AreEqual(4, RemappingHelper.GetModifierSortOrder(VirtualKey.Space)); + Assert.AreEqual(4, RemappingHelper.GetModifierSortOrder(VirtualKey.Enter)); + } + + // Test that two modifiers pressed out of order (Alt then Ctrl) get corrected + [TestMethod] + public void SortModifierKeys_ShouldSortCorrectly_WhenAltPressedBeforeCtrl() + { + // Arrange + var keys = new List + { + VirtualKey.Menu, // Alt + VirtualKey.Control, + }; + + // Act + RemappingHelper.SortModifierKeys(keys); + + // Assert - Ctrl should come before Alt + Assert.AreEqual(VirtualKey.Control, keys[0]); + Assert.AreEqual(VirtualKey.Menu, keys[1]); + } + } +} diff --git a/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs b/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs index 6f71058111..224e0024b3 100644 --- a/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs +++ b/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation +// Copyright (c) Microsoft Corporation // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. @@ -212,6 +212,11 @@ namespace KeyboardManagerEditorUI.Helpers } } + // Sort modifier keys by the standard display order: Win → Ctrl → Alt → Shift + // This matches the old C++ GetKeyVector behavior where keys are always shown in + // canonical order regardless of the order the user pressed them. + RemappingHelper.SortModifierKeys(modifierKeys); + foreach (var key in modifierKeys) { keyList.Add(_mappingService.GetKeyDisplayName((int)key)); diff --git a/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/RemappingHelper.cs b/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/RemappingHelper.cs index 48c32d853c..515b3a19e4 100644 --- a/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/RemappingHelper.cs +++ b/src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/RemappingHelper.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation +// Copyright (c) Microsoft Corporation // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. @@ -176,5 +176,32 @@ namespace KeyboardManagerEditorUI.Helpers || key == VirtualKey.LeftWindows || key == VirtualKey.RightWindows; } + + /// + /// Returns the sort order for a modifier key in the standard display order: + /// Win(0) → Ctrl(1) → Alt(2) → Shift(3). + /// Non-modifier (action) keys return 4. + /// This matches the old C++ GetKeyVector behavior. + /// + public static int GetModifierSortOrder(VirtualKey key) + { + return key switch + { + VirtualKey.LeftWindows or VirtualKey.RightWindows => 0, + VirtualKey.Control or VirtualKey.LeftControl or VirtualKey.RightControl => 1, + VirtualKey.Menu or VirtualKey.LeftMenu or VirtualKey.RightMenu => 2, + VirtualKey.Shift or VirtualKey.LeftShift or VirtualKey.RightShift => 3, + _ => 4, + }; + } + + /// + /// Sorts a list of modifier keys in the standard display order: + /// Win → Ctrl → Alt → Shift. + /// + public static void SortModifierKeys(List modifierKeys) + { + modifierKeys.Sort((a, b) => GetModifierSortOrder(a).CompareTo(GetModifierSortOrder(b))); + } } }