[Keyboard Manager] Toggle module hotkey/shortcut#42472
[Keyboard Manager] Toggle module hotkey/shortcut#42472niels9001 merged 14 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
…/PowerToys into keyboard-manager-toggle-shortcut
|
Thanks for posting this as a PR. I was going to install your release, but I'll wait for this to be merged to prod. |
|
@weikequ we want to get this in for the next release. Would you mind resolving the remaining conflicts so that we can get this reviewed and merged in :)? |
Awesome to hear. Just pushed a fix to resolve the merge conflicts. Let me know if there's anything else you need on my end. Thanks! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR adds a keyboard shortcut feature to toggle the Keyboard Manager module on and off, addressing issue #4879. The implementation follows the pattern used in the Color Picker module, with a default hotkey of Win+Shift+K.
Key changes:
- Adds toggle hotkey infrastructure to Keyboard Manager with configurable shortcut
- Extends KeyboardManagerViewModel to inherit from PageViewModelBase for hotkey support
- Implements hotkey parsing and toggle functionality in the C++ module DLL
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/settings-ui/Settings.UI/ViewModels/KeyboardManagerViewModel.cs | Refactors ViewModel to extend PageViewModelBase and implements GetAllHotkeySettings() method to expose the toggle shortcut configuration |
| src/settings-ui/Settings.UI/Strings/en-us/Resources.resw | Adds localized strings for the toggle shortcut UI header and description |
| src/settings-ui/Settings.UI/SettingsXAML/Views/KeyboardManagerPage.xaml | Adds ShortcutControl UI element for configuring the toggle shortcut |
| src/settings-ui/Settings.UI/SerializationContext/SourceGenerationContextContext.cs | Registers KeyboardManagerSettings for JSON serialization context |
| src/settings-ui/Settings.UI.Library/KeyboardManagerSettings.cs | Implements GetAllHotkeyAccessors() method to provide access to the toggle shortcut setting |
| src/settings-ui/Settings.UI.Library/KeyboardManagerProperties.cs | Adds ToggleShortcut property with default value (Win+Shift+K) |
| src/modules/keyboardmanager/dll/dllmain.cpp | Implements hotkey parsing, storage, and toggle handler that enables/disables the module with toast notifications |
| src/modules/keyboardmanager/dll/KeyboardManager.vcxproj | Adds project reference to the notifications library for toast support |
| notifications::show_toast(L"Keyboard Manager Disabled", L""); | ||
| } | ||
| else | ||
| { | ||
| enable(); | ||
| notifications::show_toast(L"Keyboard Manager Enabled", L""); |
There was a problem hiding this comment.
The notification strings "Keyboard Manager Disabled" and "Keyboard Manager Enabled" are hardcoded and not localized. These should use GET_RESOURCE_STRING with appropriate resource IDs defined in the Resources.resx file to support internationalization, similar to how app_name uses GET_RESOURCE_STRING(IDS_KEYBOARDMANAGER) on line 51.
| notifications::show_toast(L"Keyboard Manager Disabled", L""); | |
| } | |
| else | |
| { | |
| enable(); | |
| notifications::show_toast(L"Keyboard Manager Enabled", L""); | |
| notifications::show_toast(GET_RESOURCE_STRING(IDS_KEYBOARDMANAGER_DISABLED), L""); | |
| } | |
| else | |
| { | |
| enable(); | |
| notifications::show_toast(GET_RESOURCE_STRING(IDS_KEYBOARDMANAGER_ENABLED), L""); |
src/settings-ui/Settings.UI/SettingsXAML/Views/KeyboardManagerPage.xaml
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@weikequ and @niels9001 , should we consider another existing pattern in PowerToys?
|
|
Hi @weikequ @niels9001, |
Nope. The module itself is problematic when it is on in certain scenarios. If you're suggesting an additional setting, great, but this is a much-needed feature. |
I do like this as an option, but I think it should also be added as an extra setting on top of the proposed one here by @weikequ. |
Thanks @mike-mgt for the comments. So it behaves like this: When the module is disabled in Settings, the shortcut does nothing: This way, our implementation stays consistent with others. |
| // Process the hotkey event | ||
| virtual bool on_hotkey(size_t /*hotkeyId*/) override | ||
| { | ||
| if (m_enabled) |
There was a problem hiding this comment.
We currently only use notifications to communicate updates and other 'critical' things as we know users don't like to get too many notifications.
Since enabling/disabling KMB is a deliberate action by the user, notifications seem excessive.
There was a problem hiding this comment.
When I was testing it, it was really helpful to know when the action was successful, hence the notification. Having the user press a hotkey and receive no feedback is unintuitive imo , as it makes them question if anything happened. I still prefer somehow letting the user know when it has succeeded, do you have any other thoughts on how we might be able to do it without using the notification system?
There was a problem hiding this comment.
Thanks for sprinkling some good US @weikequ. Being notified of a successful activation is essential. I use SoundSwitch and I love seeing the notification that clearly states whether the input has switched or there's been an error.
There was a problem hiding this comment.
Great work! See comment about notifications.
And to @lei9444 's comment; we should follow the activation behavior that other utilities are using so we are in line. This would also set us up for success on the longer term as we are planning a UI revamp of the KBM editor :)!
I think it would be hard to capture all scenarios/apps where this would help the user, and using a specific keyboard press helps solidify user intent on performing this action. Though for my own purposes, this does work as well (maybe a blacklist to select from a list of processes?). The other option is to have no shortcut by default, that way, it is opt-in only for those who need it. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| <value>Toggle shortcut</value> | ||
| </data> | ||
| <data name="KeyboardManager_Toggle_Shortcut.Description" xml:space="preserve"> | ||
| <value>Use a shortcut to toggle this module on or off (note that the Settings UI will not update)</value> |
There was a problem hiding this comment.
The description text mentions that "the Settings UI will not update" when the hotkey is pressed. This is a user-facing limitation that should ideally be addressed. Consider implementing a mechanism to notify the Settings UI of the state change, similar to how GeneralSettings.Enabled is updated in other toggle scenarios. This could involve sending an IPC message back to the Settings UI or implementing a polling/refresh mechanism.
| <value>Use a shortcut to toggle this module on or off (note that the Settings UI will not update)</value> | |
| <value>Use a shortcut to toggle this module on or off from anywhere</value> |
| // Process the hotkey event | ||
| virtual bool on_hotkey(size_t /*hotkeyId*/) override | ||
| { | ||
| if (!m_enabled) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| constexpr ULONGLONG hotkeyToggleDebounceMs = 500; | ||
| const auto now = GetTickCount64(); | ||
| if (now - m_lastHotkeyToggleTime < hotkeyToggleDebounceMs) | ||
| { | ||
| return true; | ||
| } | ||
| m_lastHotkeyToggleTime = now; | ||
|
|
||
| refresh_process_state(); | ||
| if (m_active) | ||
| { | ||
| stop_engine(); | ||
| } | ||
| else | ||
| { | ||
| start_engine(); | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
When toggling the engine via hotkey, no telemetry is sent, unlike the enable/disable methods which call Trace::EnableKeyboardManager. Consider whether telemetry should be tracked for hotkey toggles to understand user behavior, or document why it's intentionally omitted (e.g., to avoid spam from frequent toggling).
| public HotkeySettings ToggleShortcut | ||
| { | ||
| get => Settings.Properties.ToggleShortcut; | ||
| set | ||
| { | ||
| if (Settings.Properties.ToggleShortcut != value) | ||
| { | ||
| Settings.Properties.ToggleShortcut = value ?? Settings.Properties.DefaultToggleShortcut; | ||
| OnPropertyChanged(nameof(ToggleShortcut)); | ||
| NotifySettingsChanged(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding unit tests for the new toggle shortcut functionality, similar to the tests in ViewModelTests/ColorPicker.cs that verify the ActivationShortcut property. Tests should verify that the ToggleShortcut property is correctly initialized, persisted, and updated. This aligns with the PR checklist item for tests that is currently unchecked.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Addressed the comments:
|





Summary of the Pull Request
Adds a keyboard shortcut to be able to toggle the Keyboard Manager module on and off.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Modeled the changes and addition of a global shortcut using the Color Picker module.
Notes:
KeyboardManagerSettingsand the associated .json settings file. I don't think there's anything else in this module that uses this.winkey + shift + kKeyboardManagerViewModelto extendPageViewModelBaseas opposed toObservableto get it to work. But I will say that there were some things in here that I didn't fully dig into, so let me know if there's any potential things I'm missing.Validation Steps Performed
winkey + shift + k)Media
Screen.Recording.2025-10-21.173753.mp4