Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Keyboard Manager Editor UI to support manual key selection/editing via dropdowns and introduces a new “Disable key or shortcut” action, while also making the editor more resilient when the native KBM wrapper DLL isn’t available.
Changes:
- Added a new “Disable key or shortcut” action type and UI section for displaying disabled mappings.
- Introduced key dropdown UI (with new interop API) to manually adjust recorded trigger/action keys.
- Hardened startup/keyboard-hook paths to tolerate missing native KBM DLL (standalone/read-only scenarios).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Styles/Colors.xaml | Minor cleanup in resource dictionary. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Styles/Button.xaml | Adds DropDownButton styles used by key-selection UI. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Strings/en-US/Resources.resw | Adds localized strings for the new Disable action and labels. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Settings/SettingsManager.cs | Makes native service optional; supports standalone/read-only mode. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/MainPage.xaml.cs | Adds disabled mappings list handling + save/toggle logic for Disable action. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Pages/MainPage.xaml | Adds a new “Disabled” section to the consolidated mappings view. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Interop/KeyboardMappingService.cs | Adds managed wrapper to fetch key lists from native code. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Interop/KeyboardManagerInterop.cs | Adds P/Invoke + struct for native key list retrieval. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Interop/KeyNameEntry.cs | New record type for key list entries. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Helpers/KeyboardHookHelper.cs | Makes hook/service initialization more resilient when native components are missing. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/UnifiedMappingControl.xaml.cs | Adds dropdown event handling and new Disable action support in the unified editor. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/UnifiedMappingControl.xaml | Replaces KeyVisual with KeyDropDownButton and adds Disable action UI. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/KeyDropDownButton.xaml.cs | New control backing logic: key list flyout + caching + selection events. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/KeyDropDownButton.xaml | New dropdown-based key visual UI. |
| src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/KeyChangedEventArgs.cs | New event args type for dropdown key changes. |
| UnifiedMappingControl.SetActionType(UnifiedMappingControl.ActionType.Disable); | ||
| UnifiedMappingControl.SetAppSpecific(!disabledMapping.IsAllApps, disabledMapping.AppName); | ||
| RemappingDialog.Title = "Edit remapping"; | ||
| await ShowRemappingDialog(); |
There was a problem hiding this comment.
RemappingDialog.Title is being set with a hard-coded, non-localizable string. Since the dialog already has x:Uid, the title should come from Resources.resw (or a ResourceLoader) so it can be localized like other UI text.
| private bool SaveDisableMapping(List<string> triggerKeys) | ||
| { | ||
| // VK_DISABLED = 0x100 (256) — target "256" tells the engine to suppress the key | ||
| const string vkDisabledCode = "256"; | ||
| bool isAppSpecific = UnifiedMappingControl.GetIsAppSpecific(); | ||
| string appName = UnifiedMappingControl.GetAppName(); | ||
|
|
||
| string originalKeysString = string.Join( | ||
| ";", | ||
| triggerKeys.Select(k => _mappingService!.GetKeyCodeFromName(k).ToString(System.Globalization.CultureInfo.InvariantCulture))); | ||
|
|
There was a problem hiding this comment.
SaveDisableMapping does not validate the trigger keys (e.g., modifier-only shortcuts, illegal shortcuts like Win+L, or duplicates) before saving. Because Disable is a new ActionType and ValidateMapping currently doesn’t cover it, this path can persist invalid/unsupported mappings (including 0 key codes if a dropdown provides a “None” option). Consider adding a dedicated validation step for Disable (reuse the same original-keys checks as ValidateKeyMapping without requiring remapped keys).
There was a problem hiding this comment.
SaveDisableMapping does not validate the trigger keys (e.g., modifier-only shortcuts, illegal shortcuts like Win+L, or duplicates) before saving. [...]
I would argue that modifier-only shortcuts should also be customizable and deactivatable.
While these could lead to unintended side effects and shortcuts like Win+L are even more critical, these are also changes that could arise in legitimate use cases.
With modifier keys, I could imagine that entire key combinations might be swapped for ergonomic reasons. Other special/protected keys might've already been duplicated on customized keyboards.
There even seem to be hardening projects for kiosk mode that, for example, remove shortcuts and keys (such as the Win key).
Such changes (e.g.,
deactivating a modifier key) should already be possible with other programs. Win+L (and even the entire locking part) can be bypassed at the user level with a registry tweak like DisableLockWorkstation.
I therefore suggest not introducing these validations and restrictions and, if they already exist, removing them.
But that would likely be a different/new issue, so for now I’ll just ask you to not implement this suggestion by Copilot.
———
Regardless of the result,
thanks a lot for this fantastic PR!
It really does cover all the features.
| private void EnableDisabledMapping(Remapping remapping) | ||
| { | ||
| string originalKeysString = string.Join( | ||
| ";", | ||
| remapping.Shortcut.Select(k => _mappingService!.GetKeyCodeFromName(k).ToString(System.Globalization.CultureInfo.InvariantCulture))); | ||
|
|
||
| if (remapping.Shortcut.Count == 1) | ||
| { | ||
| int originalKey = _mappingService!.GetKeyCodeFromName(remapping.Shortcut[0]); | ||
| if (originalKey != 0) | ||
| { | ||
| _mappingService.AddSingleKeyMapping(originalKey, 0x100); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| _mappingService!.AddShortcutMapping( | ||
| originalKeysString, | ||
| "256", | ||
| !remapping.IsAllApps ? remapping.AppName : string.Empty); | ||
| } |
There was a problem hiding this comment.
The VK_DISABLED sentinel is hard-coded in multiple forms ("256" and 0x100) across SaveDisableMapping/LoadRemappings/EnableDisabledMapping. This duplication makes it easy for the behavior to drift; please centralize it into a single constant (string + int as needed) and reuse it everywhere you detect or apply a disabled mapping.
| dropDown.KeyChanged += TriggerKeyDropDown_KeyChanged; | ||
| dropDown.Unloaded += (s, _) => | ||
| { | ||
| if (s is KeyDropDownButton dd) | ||
| { | ||
| dd.KeyChanged -= TriggerKeyDropDown_KeyChanged; | ||
| } | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| private void ActionKeyDropDown_Loaded(object sender, RoutedEventArgs e) | ||
| { | ||
| if (sender is KeyDropDownButton dropDown) | ||
| { | ||
| dropDown.KeyChanged += ActionKeyDropDown_KeyChanged; | ||
| dropDown.Unloaded += (s, _) => | ||
| { | ||
| if (s is KeyDropDownButton dd) | ||
| { | ||
| dd.KeyChanged -= ActionKeyDropDown_KeyChanged; | ||
| } | ||
| }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
TriggerKeyDropDown_Loaded / ActionKeyDropDown_Loaded subscribe to KeyChanged on every Loaded and also attach a new anonymous Unloaded handler each time. In WinUI, Loaded/Unloaded can fire repeatedly for recycled visuals, which can lead to duplicate subscriptions and accumulating Unloaded handlers. Prefer subscribing once (e.g., inside KeyDropDownButton) or detach-before-attach and use a named Unloaded handler you can add/remove.
| dropDown.KeyChanged += TriggerKeyDropDown_KeyChanged; | |
| dropDown.Unloaded += (s, _) => | |
| { | |
| if (s is KeyDropDownButton dd) | |
| { | |
| dd.KeyChanged -= TriggerKeyDropDown_KeyChanged; | |
| } | |
| }; | |
| } | |
| } | |
| private void ActionKeyDropDown_Loaded(object sender, RoutedEventArgs e) | |
| { | |
| if (sender is KeyDropDownButton dropDown) | |
| { | |
| dropDown.KeyChanged += ActionKeyDropDown_KeyChanged; | |
| dropDown.Unloaded += (s, _) => | |
| { | |
| if (s is KeyDropDownButton dd) | |
| { | |
| dd.KeyChanged -= ActionKeyDropDown_KeyChanged; | |
| } | |
| }; | |
| } | |
| } | |
| // Ensure we do not accumulate multiple subscriptions when Loaded fires repeatedly. | |
| dropDown.KeyChanged -= TriggerKeyDropDown_KeyChanged; | |
| dropDown.KeyChanged += TriggerKeyDropDown_KeyChanged; | |
| // Use a named Unloaded handler so we can detach it and avoid accumulating handlers. | |
| dropDown.Unloaded -= TriggerKeyDropDown_Unloaded; | |
| dropDown.Unloaded += TriggerKeyDropDown_Unloaded; | |
| } | |
| } | |
| private void TriggerKeyDropDown_Unloaded(object sender, RoutedEventArgs e) | |
| { | |
| if (sender is KeyDropDownButton dropDown) | |
| { | |
| dropDown.KeyChanged -= TriggerKeyDropDown_KeyChanged; | |
| dropDown.Unloaded -= TriggerKeyDropDown_Unloaded; | |
| } | |
| } | |
| private void ActionKeyDropDown_Loaded(object sender, RoutedEventArgs e) | |
| { | |
| if (sender is KeyDropDownButton dropDown) | |
| { | |
| // Ensure we do not accumulate multiple subscriptions when Loaded fires repeatedly. | |
| dropDown.KeyChanged -= ActionKeyDropDown_KeyChanged; | |
| dropDown.KeyChanged += ActionKeyDropDown_KeyChanged; | |
| // Use a named Unloaded handler so we can detach it and avoid accumulating handlers. | |
| dropDown.Unloaded -= ActionKeyDropDown_Unloaded; | |
| dropDown.Unloaded += ActionKeyDropDown_Unloaded; | |
| } | |
| } | |
| private void ActionKeyDropDown_Unloaded(object sender, RoutedEventArgs e) | |
| { | |
| if (sender is KeyDropDownButton dropDown) | |
| { | |
| dropDown.KeyChanged -= ActionKeyDropDown_KeyChanged; | |
| dropDown.Unloaded -= ActionKeyDropDown_Unloaded; | |
| } | |
| } |
| private void TriggerKeyDropDown_KeyChanged(object? sender, KeyChangedEventArgs e) | ||
| { | ||
| if (sender is KeyDropDownButton dropDown) | ||
| { | ||
| int index = GetDropDownIndex(TriggerKeys, dropDown); | ||
| if (index >= 0 && index < _triggerKeys.Count) | ||
| { | ||
| _triggerKeys[index] = e.NewKeyName; | ||
| HandleAutoGrowShrink(_triggerKeys, index, e.NewKeyCode); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void ActionKeyDropDown_KeyChanged(object? sender, KeyChangedEventArgs e) | ||
| { | ||
| if (sender is KeyDropDownButton dropDown) | ||
| { | ||
| int index = GetDropDownIndex(ActionKeys, dropDown); | ||
| if (index >= 0 && index < _actionKeys.Count) | ||
| { | ||
| _actionKeys[index] = e.NewKeyName; | ||
| HandleAutoGrowShrink(_actionKeys, index, e.NewKeyCode); | ||
| } |
There was a problem hiding this comment.
KeyChanged handlers write the selected display name directly into _triggerKeys/_actionKeys without handling the special "None" entry (KeyCode=0) returned by LayoutMap when isShortcut=true. Since GetKeyCodeFromName("None") resolves to 0, this can later serialize invalid mappings ("0" in the key list) and bypass validation for Disable. Handle NewKeyCode==0 explicitly (e.g., treat as clearing/removing that slot), and consider updating app-specific enablement after trigger keys change.
src/modules/keyboardmanager/KeyboardManagerEditorUI/Controls/UnifiedMappingControl.xaml.cs
Outdated
Show resolved
Hide resolved
| var service = new KeyboardMappingService(); | ||
| cached = service.GetKeyboardKeysList(isShortcut); | ||
| service.Dispose(); |
There was a problem hiding this comment.
GetKeyList manually calls service.Dispose() only on the success path. If GetKeyboardKeysList throws after the service is constructed, the service won’t be disposed. Prefer a using/try-finally pattern so the native handle is reliably released even when exceptions occur.
| var service = new KeyboardMappingService(); | |
| cached = service.GetKeyboardKeysList(isShortcut); | |
| service.Dispose(); | |
| using (var service = new KeyboardMappingService()) | |
| { | |
| cached = service.GetKeyboardKeysList(isShortcut); | |
| } |
| <value>Disable key or shortcut</value> | ||
| </data> | ||
| <data name="DisableDescription.Text" xml:space="preserve"> | ||
| <value>This shortcut will be disabled.</value> |
There was a problem hiding this comment.
DisableDescription.Text says "This shortcut will be disabled.", but the feature also supports disabling a single key. Consider changing the string to cover both cases (e.g., "This key or shortcut will be disabled.") so the UI remains accurate.
| <value>This shortcut will be disabled.</value> | |
| <value>This key or shortcut will be disabled.</value> |
| if (e.ClickedItem is KeyNameEntry entry) | ||
| { | ||
| string oldKeyName = KeyName; | ||
| KeyName = entry.DisplayName; |
There was a problem hiding this comment.
KeyName is data-bound from the parent ItemsControl (e.g., KeyName="{Binding}" in UnifiedMappingControl). Setting KeyName directly here can break/override the binding and lead to the visual not tracking later source updates. Prefer not setting KeyName in the control and instead only raise KeyChanged (let the parent update the bound collection to drive the UI), including for reverting invalid selections.
| KeyName = entry.DisplayName; |
| public static readonly DependencyProperty IsShortcutProperty = | ||
| DependencyProperty.Register( | ||
| nameof(IsShortcut), | ||
| typeof(bool), | ||
| typeof(KeyDropDownButton), | ||
| new PropertyMetadata(true)); | ||
|
|
There was a problem hiding this comment.
IsShortcut defaults to true, which means the native key list includes a synthetic "None" entry with keycode 0 (see LayoutMap::GetKeyNameList for shortcut lists). If the user selects that entry, the UI will store/display "None" and later save paths can end up emitting keycode 0 in OriginalKeys (especially for multi-key mappings), producing an invalid mapping. Consider filtering out the 0/"None" entry for this UI, or treating selection of keycode 0 as clearing the slot (set empty string + trigger shrink logic) rather than a real key name.
| private static string? ValidateDropDownSelection(ObservableCollection<string> keys, int changedIndex, int newKeyCode, string newKeyName) | ||
| { | ||
| const int maxShortcutSize = 5; | ||
|
|
||
| // KeyType: 0=Win, 1=Ctrl, 2=Alt, 3=Shift, 4=Action | ||
| int newKeyType = KeyboardManagerInterop.GetKeyType(newKeyCode); | ||
| bool isModifier = newKeyType < 4; | ||
|
|
||
| // Rule: action key at position 0 in multi-key shortcut (shortcut must start with modifier) | ||
| if (!isModifier && changedIndex == 0 && keys.Count > 1) | ||
| { | ||
| return ResourceHelper.GetString("Warning_ShortcutStartWithModifier"); | ||
| } | ||
|
|
||
| // Rule: no repeated modifier types | ||
| if (isModifier) | ||
| { | ||
| for (int i = 0; i < keys.Count; i++) | ||
| { | ||
| if (i == changedIndex) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| int existingKeyCode = KeyboardManagerInterop.GetKeyCodeFromName(keys[i]); | ||
| int existingKeyType = KeyboardManagerInterop.GetKeyType(existingKeyCode); | ||
|
|
||
| if (existingKeyType == newKeyType) | ||
| { | ||
| return ResourceHelper.GetString("Warning_RepeatedModifier"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ValidateDropDownSelection iterates over keys and calls KeyboardManagerInterop.GetKeyCodeFromName(keys[i]) even when entries are placeholders (string.Empty) added by HandleAutoGrowShrink. This can incorrectly trigger repeated-modifier warnings (and treats empty as keycode 0). Consider filtering to non-empty entries when validating (and use the count of non-empty keys for rules like "shortcut must start with a modifier").
| string? validationError = ValidateDropDownSelection(_actionKeys, index, e.NewKeyCode, e.NewKeyName); | ||
| if (validationError != null) | ||
| { | ||
| dropDown.KeyName = e.OldKeyName; | ||
| ShowNotificationTip(validationError); | ||
| return; |
There was a problem hiding this comment.
Same binding concern in ActionKeyDropDown_KeyChanged: reverting by setting dropDown.KeyName = e.OldKeyName can override/break the KeyName="{Binding}" expression. Consider keeping the underlying _actionKeys[index] unchanged on validation failure and drive the UI back to the old value through the collection/binding instead.
| int index = actionType switch | ||
| { | ||
| ActionType.Text => 1, | ||
| ActionType.OpenUrl => 2, | ||
| ActionType.OpenApp => 3, | ||
| ActionType.MouseClick => 4, | ||
| ActionType.Disable => 4, | ||
| ActionType.MouseClick => 5, | ||
| _ => 0, | ||
| }; |
There was a problem hiding this comment.
SetActionType relies on hard-coded ComboBox indices. This is brittle (and currently ActionType.MouseClick => 5 doesn't correspond to any visible ComboBoxItem since MouseClick is commented out in XAML). Consider selecting the item by matching the ComboBoxItem.Tag instead of by index, so adding/removing items in XAML can't desync the mapping.
| // Process all shortcut mappings (RunProgram, OpenUri, RemapShortcut, RemapText) | ||
| foreach (ShortcutKeyMapping mapping in _mappingService.GetShortcutMappings()) | ||
| foreach (ShortcutKeyMapping mapping in _mappingService!.GetShortcutMappings()) | ||
| { | ||
| AddShortcutMapping(settings, mapping); | ||
| } | ||
|
|
||
| // Process single key to key mappings | ||
| foreach (var mapping in _mappingService.GetSingleKeyMappings()) | ||
| foreach (var mapping in _mappingService!.GetSingleKeyMappings()) | ||
| { |
There was a problem hiding this comment.
CreateSettingsFromKeyboardManagerService mixes _mappingService! and _mappingService access. Since _mappingService is now nullable, the later foreach (var mapping in _mappingService.GetKeyToTextMappings()) can raise nullable warnings (and potentially fail builds if warnings are treated as errors). Suggest using _mappingService! consistently in this method, or refactor to pass a non-null service instance into the method.
| Logger.LogError("Failed to initialize KeyboardMappingService in MainPage page: " + ex.Message); | ||
| } | ||
|
|
||
| LoadAllMappings(); |
There was a problem hiding this comment.
LoadAllMappings() now runs even if KeyboardMappingService fails to initialize (so _mappingService stays null). In that state, the UI will still populate from JSON, but interactive actions like toggling active state or deleting will early-return (and toggles can visually change without persisting). Consider explicitly switching the page into a read-only mode when _mappingService is null (e.g., disable toggle switches / delete buttons / save actions, or keep LoadAllMappings() inside the successful init path if standalone mode isn't intended for end users).
| LoadAllMappings(); | |
| if (_mappingService != null) | |
| { | |
| LoadAllMappings(); | |
| } | |
| else | |
| { | |
| MappingState = "Error"; | |
| } |
| IsOn="{x:Bind IsActive}" | ||
| Style="{StaticResource RightAlignedCompactToggleSwitchStyle}" | ||
| Toggled="ToggleSwitch_Toggled" /> | ||
| <Button | ||
| VerticalAlignment="Center" |
There was a problem hiding this comment.
The Disabled section’s ToggleSwitch is always enabled, but the toggle handler is a no-op when the mapping service isn't available. This can leave the UI showing a changed active state that wasn't applied. Bind IsEnabled (and ideally the delete button too) to a service-availability flag (e.g., SettingsManager.IsNativeServiceAvailable exposed via the page/viewmodel) so the section is clearly read-only when native interop isn't present.
| IsOn="{x:Bind IsActive}" | |
| Style="{StaticResource RightAlignedCompactToggleSwitchStyle}" | |
| Toggled="ToggleSwitch_Toggled" /> | |
| <Button | |
| VerticalAlignment="Center" | |
| IsOn="{x:Bind IsActive}" | |
| IsEnabled="{x:Bind IsNativeServiceAvailable, Mode=OneWay}" | |
| Style="{StaticResource RightAlignedCompactToggleSwitchStyle}" | |
| Toggled="ToggleSwitch_Toggled" /> | |
| <Button | |
| VerticalAlignment="Center" | |
| IsEnabled="{x:Bind IsNativeServiceAvailable, Mode=OneWay}" |
| string? validationError = ValidateDropDownSelection(_triggerKeys, index, e.NewKeyCode, e.NewKeyName); | ||
| if (validationError != null) | ||
| { | ||
| // Revert the selection | ||
| dropDown.KeyName = e.OldKeyName; | ||
| ShowNotificationTip(validationError); | ||
| return; |
There was a problem hiding this comment.
On invalid dropdown selection you revert by setting dropDown.KeyName = e.OldKeyName, but KeyName is bound (KeyName="{Binding}") so directly setting it can override/break the binding and may not reliably revert the displayed value. Prefer reverting via the bound source (leave _triggerKeys[index] unchanged and refresh the dropdown selection from that), rather than writing to the bound DP.
niels9001
added
the
Product-Keyboard Shortcut Manager
Summary of the Pull Request
This PR introduces the following changes:
GIF:
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed