-
Notifications
You must be signed in to change notification settings - Fork 7.7k
[Settings] Implement singleton pattern for ShortcutConflictWindow #42440
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
- Added static fields and methods in App.xaml.cs to manage ShortcutConflictWindow singleton - Updated ShortcutConflictWindow to clear singleton reference on close - Modified all three locations that create ShortcutConflictWindow to check for existing instance - If window already exists, activate it instead of creating a new one Co-authored-by: davidegiacometti <[email protected]>
davidegiacometti
left a comment
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.
Code LGTM! 🚀
I pushed some minor changes and also ported 2 small features from the OOBE window:
- React to theme change
- Close PT Settings process when Shortcuts Conflicts is the last window being closed
Tested Shortcuts Conflicts window opening from:
- Dashboard
- OOBE window
- Shortcut control
src/settings-ui/Settings.UI/SettingsXAML/Controls/Dashboard/ShortcutConflictControl.xaml.cs
Fixed
Show fixed
Hide fixed
src/settings-ui/Settings.UI/SettingsXAML/Controls/ShortcutControl/ShortcutControl.xaml.cs
Fixed
Show fixed
Hide fixed
src/settings-ui/Settings.UI/SettingsXAML/OOBE/Views/OobeOverview.xaml.cs
Fixed
Show fixed
Hide fixed
This comment has been minimized.
This comment has been minimized.
|
Followed the same pattern as #44721 after it's merged, so we got unified oobe, scoobe, and shortcutconflict secondary layer window management |
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
Implements single-instance behavior for the Settings “Shortcut conflicts” window so repeated entry points activate the existing window instead of opening duplicates, aligning the UX with other secondary windows (e.g., OOBE/SCOOBE). It also adds disposal/guard logic around the Dashboard view model lifecycle.
Changes:
- Added singleton-style management for
ShortcutConflictWindowinApp, and updated callers to use it. - Updated
ShortcutConflictWindowto integrate with theme changes and to clear singleton state on close. - Added disposal handling and
_isDisposedguards inDashboardViewModel, and disposes the VM whenDashboardPageunloads.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs | Adds _isDisposed guarding and updates Dispose() behavior. |
| src/settings-ui/Settings.UI/SettingsXAML/Views/DashboardPage.xaml.cs | Disposes the dashboard VM on Unloaded. |
| src/settings-ui/Settings.UI/SettingsXAML/MainWindow.xaml.cs | Uses the renamed App.IsSecondaryWindowOpen() check. |
| src/settings-ui/Settings.UI/SettingsXAML/Controls/ShortcutControl/ShortcutControl.xaml.cs | Routes “Learn more” to the singleton conflict window opener. |
| src/settings-ui/Settings.UI/SettingsXAML/Controls/Dashboard/ShortcutConflictWindow.xaml.cs | Hooks theme changes and notifies App on close for singleton cleanup. |
| src/settings-ui/Settings.UI/SettingsXAML/Controls/Dashboard/ShortcutConflictControl.xaml.cs | Routes dashboard conflict button to the singleton conflict window opener. |
| src/settings-ui/Settings.UI/SettingsXAML/App.xaml.cs | Adds tracking/opening logic for the singleton ShortcutConflictWindow and expands “secondary window” detection. |
Comments suppressed due to low confidence (1)
src/settings-ui/Settings.UI/ViewModels/DashboardViewModel.cs:855
- DashboardViewModel.Dispose() now gets called (via DashboardPage.Unloaded), but it only unsubscribes its own SettingsChanged handler. The contained QuickAccessViewModel also subscribes to _settingsRepository.SettingsChanged and enabled-module notifications and currently has no Dispose/unsubscribe path, so disposing the dashboard won’t actually stop those subscriptions/work. Either avoid disposing the DashboardViewModel on navigation, or add cleanup for QuickAccessViewModel (e.g., implement IDisposable there and call it here).
public override void Dispose()
{
if (_isDisposed)
{
return;
}
_isDisposed = true;
base.Dispose();
if (_settingsRepository != null)
{
_settingsRepository.SettingsChanged -= OnSettingsChanged;
}
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Summary
Fixes issue where multiple ShortcutConflictWindow instances could be opened simultaneously. The window now follows the same singleton pattern as OobeWindow - only one instance can exist at a time, and attempting to open another brings the existing window to the foreground.
Changes
Implemented singleton management for
ShortcutConflictWindowfollowing the established pattern used byOobeWindow:App.xaml.cs
GetShortcutConflictWindow(),SetShortcutConflictWindow(), andClearShortcutConflictWindow()methods for lifecycle managementShortcutConflictWindow.xaml.cs
WindowEx_Closedevent handler to callApp.ClearShortcutConflictWindow()to properly clean up the singleton reference when the window is closedUpdated all three entry points that create ShortcutConflictWindow:
Each location now checks if a window already exists using
App.GetShortcutConflictWindow():App.SetShortcutConflictWindow()Activate()to bring it to the foregroundTesting
The fix ensures that:
Fixes #[issue_number]
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
i1qvsblobprodcus353.vsblob.vsassets.iodotnet build PowerToys.Settings.csproj --configuration Release(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
Fixes #42437
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.