-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Fix Window Walker 'Close Window' to respect stay open setting #44040
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
Fix Window Walker 'Close Window' to respect stay open setting #44040
Conversation
|
@microsoft-github-policy-service agree |
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 PR fixes a settings inconsistency where the "Stay open after closing windows and killing processes" setting (OpenAfterKillAndClose) was not being respected by the "Close Window" command in Window Walker. The fix aligns CloseWindowCommand with EndTaskCommand's behavior by checking the setting and returning the appropriate CommandResult.
Key Changes:
- Added SettingsManager.Instance check to CloseWindowCommand.Invoke() to respect the OpenAfterKillAndClose setting
- Added necessary using directive for Microsoft.CmdPal.Ext.WindowWalker.Helpers
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
🟠 I think this could use a bit more polish. It fixes the immediate issue, but doesn’t refresh the list, so closing the window leaves the list unchanged, which can be confusing. |
|
@jiripolasek Thanks for the feedback! You're right that the list doesn't refresh after closing the window. I see a few approaches to address this:
Could you suggest which approach would be preferred for this extension, or if there's an existing pattern in Command Palette extensions that handles this scenario? I want to make sure the solution aligns with the codebase conventions. Looking at |
derp I didn't read the thread. JIri's got a good point
27b3622 to
01f6aa8
Compare
|
Hmmm. The event seems like it's the most correct, but that's kind of a lot of plumbing, based on the way the WindowWalkerListPage is separated from the ResultHelper from the ContextMenuHelper. Honestly, the easiest solution would be to add a weak reference messenger (need to add a dependency to WW project), send a "RefreshWindows" message from the close command, and handle that up on the list page. |
|
@zadjii-msft Thanks for the guidance! I've implemented the WeakReferenceMessenger approach. Changes:
This should now refresh the window list after closing a window or killing a process when the "Stay open" setting is enabled. Please re-test when you get a chance! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@ThanhNguyxn It’s not compiling: |
|
@jiripolasek Fixed! Added the missing Please trigger CI again when you have a chance. 🙏 |
41b7e91 to
661c0e4
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
661c0e4 to
5ab8506
Compare
The 'Stay open after closing windows and killing processes' setting was only working for 'End Task' command but not for 'Close Window' command. This change updates CloseWindowCommand.Invoke() to check the OpenAfterKillAndClose setting and return CommandResult.KeepOpen() or CommandResult.Dismiss() accordingly, matching the behavior of EndTaskCommand. Fixes microsoft#43256
Implements WeakReferenceMessenger pattern as suggested by @zadjii-msft: - Add CommunityToolkit.Mvvm dependency to WindowWalker extension - Create RefreshWindowsMessage for communication between commands and list page - CloseWindowCommand now sends RefreshWindowsMessage when KeepOpen - EndTaskCommand now sends RefreshWindowsMessage when KeepOpen - WindowWalkerListPage subscribes to message and refreshes window list - Small delay (100ms) added to allow Windows time to close window
5ab8506 to
18eec1e
Compare
| public void Receive(RefreshWindowsMessage message) | ||
| { | ||
| // Small delay to allow Windows to actually close the window | ||
| System.Threading.Tasks.Task.Delay(100).ContinueWith(_ => |
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.
[minor nit] it would probably make more sense to delay sending of the message (since the caller actions are what requires the delay). But let's keep this.
|
Hi @ThanhNguyxn, it’s best to avoid force-pushing changes to a branch under review—especially without a reason. Doing so resets the checks and forces maintainers to review it again, pushing it to the back of the queue. Before marking a PR as ready for review or submitting further changes, please make sure all unit tests pass and that you’ve manually tested the changes and their impact on the application. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
18eec1e LGTM |
|
@ThanhNguyxn, thanks for attempting to contribute, but after multiple attempts, we can't get your code to compile. In this instance, you also force pushed an unrelated change. That's considered very bad form. Good luck in the future. Cheers! |
Summary of the Pull Request
Fixes the "Stay open after closing windows and killing processes" setting not working for the "Close Window" command in Window Walker (Command Palette).
PR Checklist
Problem
The "Stay open after closing windows and killing processes" setting (
OpenAfterKillAndClose) was being respected by the "End Task" command but not by the "Close Window" command.Solution
Updated
CloseWindowCommand.Invoke()to check theSettingsManager.Instance.OpenAfterKillAndClosesetting and returnCommandResult.KeepOpen()orCommandResult.Dismiss()accordingly, matching the behavior ofEndTaskCommand.Validation Steps Performed