-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Creating a Common.UI.Controls lib #45542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml
Fixed
Show fixed
Hide fixed
src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml
Fixed
Show fixed
Hide fixed
src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml
Fixed
Show fixed
Hide fixed
This comment has been minimized.
This comment has been minimized.
|
Nice! And I think this will take care of some gremlins in the system, like #45388 |
|
Common.UI.Controls.csproj should be included in https://github.com/microsoft/PowerToys/blob/main/src/modules/cmdpal/CommandPalette.slnf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request creates a new Common.UI.Controls library to consolidate shared WinUI controls that were previously duplicated between the Settings UI and CmdPal (Command Palette) modules. The PR addresses issue #45388 related to shortcut selection UI components by establishing a single source of truth for common controls, preventing future duplication as new modules like the Keyboard Manager are developed.
Changes:
- Created
Common.UI.Controlslibrary with shared controls: KeyVisual, IsEnabledTextBlock, CheckBoxWithDescriptionControl, ShortcutWithTextLabelControl, and BoolToKeyVisualStateConverter - Updated Settings UI and CmdPal to reference the new shared library instead of maintaining duplicate implementations
- Migrated all XAML namespace references from local control namespaces to
Microsoft.PowerToys.Common.UI.Controls
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/common/Common.UI.Controls/Common.UI.Controls.csproj | New project file for shared controls library with WinUI dependencies |
| src/common/Common.UI.Controls/Controls/* | Moved control implementations (KeyVisual, IsEnabledTextBlock, CheckBoxWithDescriptionControl, ShortcutWithTextLabelControl) with namespace updates |
| src/common/Common.UI.Controls/Converters/BoolToKeyVisualStateConverter.cs | Moved converter with namespace updated to Common.UI.Controls |
| src/common/Common.UI.Controls/Themes/Generic.xaml | Resource dictionary merging all control styles |
| src/settings-ui/Settings.UI/SettingsXAML/Views/*.xaml | Updated 11 view files to use ptcontrols namespace for shared controls |
| src/settings-ui/Settings.UI/SettingsXAML/OOBE/Views/*.xaml | Updated 11 OOBE view files with new namespace references |
| src/settings-ui/Settings.UI/SettingsXAML/Controls/*.xaml | Updated ShortcutControl components with new namespace and resource paths |
| src/settings-ui/Settings.UI/SettingsXAML/App.xaml | Updated resource dictionary paths to reference Common.UI.Controls library |
| src/settings-ui/Settings.UI/SettingsXAML/Themes/Generic.xaml | Removed IsEnabledTextBlock resource (now in shared library) |
| src/settings-ui/Settings.UI/SettingsXAML/Controls/IsEnabledTextBlock.xaml | Deleted (moved to shared library) |
| src/settings-ui/Settings.UI/PowerToys.Settings.csproj | Added project reference to Common.UI.Controls, removed old resource references |
| src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/*.xaml | Updated settings pages with new namespace references |
| src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/*.xaml | Updated ShortcutControl with new namespace, visual states, and styling |
| src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/ShortcutControl/ShortcutControl.xaml.cs | Enhanced with SetKeys() method and visual state management |
| src/modules/cmdpal/Microsoft.CmdPal.UI/Controls/*.cs | Deleted duplicate control implementations |
| src/modules/cmdpal/Microsoft.CmdPal.UI/App.xaml | Updated resource dictionary references to shared library |
| src/modules/cmdpal/Microsoft.CmdPal.UI/Microsoft.CmdPal.UI.csproj | Added project reference to Common.UI.Controls |
| src/modules/cmdpal/CommandPalette.slnf | Added Common.UI.Controls project to solution filter |
| PowerToys.slnx | Added Common.UI.Controls project to main solution |
| .github/actions/spell-check/expect.txt | Added "ptcontrols" to spell-check dictionary |
| private void C_ResetClick(object sender, RoutedEventArgs e) | ||
| { | ||
| hotkeySettings = null; | ||
|
|
||
| SetValue(HotkeySettingsProperty, hotkeySettings); | ||
| SetKeys(); | ||
|
|
||
| lastValidSettings = hotkeySettings; | ||
| shortcutDialog.Hide(); | ||
| } | ||
|
|
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method C_ResetClick is never called or wired up in the codebase. It appears to be dead code that duplicates the functionality of ShortcutDialog_Reset (line 430-438). Consider removing this unused method.
| private void C_ResetClick(object sender, RoutedEventArgs e) | |
| { | |
| hotkeySettings = null; | |
| SetValue(HotkeySettingsProperty, hotkeySettings); | |
| SetKeys(); | |
| lastValidSettings = hotkeySettings; | |
| shortcutDialog.Hide(); | |
| } |
|
Oh, squirrel ... preprocessor! That's my reward for cutting shortcuts |
|
Project That's a magnet for conflicts, like this b0a1acf |
Summary of the Pull Request
@jiripolasek FYI
This PR creates a new
Common.UI.Controlslibrary that contains shared WinUI controls. We have been copying code manually between CmdPal and Settings, and now with the new KBM we will run into the same issue.This lib has shared controls projects can add to their proj so we have a single source of truth.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed