-
Notifications
You must be signed in to change notification settings - Fork 29k
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 #114432: Multiple save dialogs appearing on Windows if Ctrl+S is pressed multiple times #114450
Fix #114432: Multiple save dialogs appearing on Windows if Ctrl+S is pressed multiple times #114450
Conversation
Does this only apply for the save dialog or others as well, such as the open one? I think a fix should cover all cases if possible. //cc @alexr00 |
The simple file dialog won't have this problem. I can't actually repro the original issue on Windows with the native dialog. |
Doesn't seem to affect anything other than the native save dialogs for Windows. I've been able to repro this by repeatedly pressing Ctrl+S (holding it down doesn't seem to do anything). It looks like it could be an issue with Electron's |
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.
I was able to reproduce this even for the "Open" dialog but it is harder. Looking at the code, we already seem to be queueing up requests for dialogs per window:
vscode/src/vs/platform/dialogs/electron-main/dialogMainService.ts
Lines 126 to 138 in 878bf13
private getDialogQueue(window?: BrowserWindow): Queue<any> { | |
if (!window) { | |
return this.noWindowDialogQueue; | |
} | |
let windowDialogQueue = this.mapWindowToDialogQueue.get(window.id); | |
if (!windowDialogQueue) { | |
windowDialogQueue = new Queue<any>(); | |
this.mapWindowToDialogQueue.set(window.id, windowDialogQueue); | |
} | |
return windowDialogQueue; | |
} |
I cannot recall why this queue was added but now that I think about it, it would imho make more sense to not queue requests but rather ignore them per window when a dialog is already showing. I would try with that approach instead.
… the dialogMainService
Thanks @bpasero for pointing me in the right direction. I've updated my solution with changes now focused on the dialogMainService. With this change only 1 instance of either the open file dialog or save file dialog can exist for a given window. I've left the queueing stuff there for the message box dialogs though as I think there may still be a use for it (can be easily changed if this isn't the case). |
@CameronIrvine I pushed 0ba4635 to your branch and now I am asking you for a review + testing. Summary:
|
Now that I think more about it: should we only block dialogs that have the same options? Imho it would still be valid to queue save/open dialog that have different properties, because the only thing we really want to block is the same dialog opening again (e.g. when you hit |
It is possible that a dialog could have the same options but become from a different source, so I'm not sure that gating the queueing on whether the options are the same will be helpful. |
Yeah it is a tradeoff, I am open for better ideas. |
Those changes look good to me @bpasero, definitely a lot nicer returning errors rather than falsified dialog results. I've manually tested them and can confirm they're behaving as expected. As for queueing I'm not sure if there's a better alternative to using options. One other way I thought of is if we sent a source ID string along with each dialog request but that might not be ideal and could probably get messy. |
Yeah agree, I would go with queueing by options for now. We have a |
I've made some changes based on your suggestions @bpasero. Dialog requests for each window are now queued if their options don't match the hash of a currently queued dialog's options. |
Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
…alue instead of any type for getWindowDialogQueue
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.
Thanks, almost there...
private readonly windowDialogQueues = new Map<number, Queue<any>>(); | ||
private readonly noWindowDialogueQueue = new Queue<any>(); |
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.
Can we still get rid of these 2 any
usages somehow?
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.
Not quite sure of the best approach for this. I've attempted to use MessageBoxReturnValue | SaveDialogReturnValue | OpenDialogReturnValue
for the queues but have had some trouble with getting them to work in getWindowDialogQueue
.
I don't have a lot of experience with TS generics so I might be missing something really obvious here.
Open to any suggestions.
Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
Ok I will go ahead and merge this, I can look into the typing issue. Thanks a lot! |
…pressed multiple times (#114450) * fix multiple save dialogs appearing on Windows when spamming Ctrl+S * remove old fix and instead keep track of windows with open dialogs in the dialogMainService * keep initialisation of activeWindowDialogs in constructor * remove unused variable * some changes * queue dialogs based on hash of options * simplify structure, fix comment typo * Apply suggestions from code review Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com> * remove unnecessary async/await for aquireFileDialogLock method * don't acquire file dialog lock for message boxes * use MessageBoxReturnValue | SaveDialogReturnValue | OpenDialogReturnValue instead of any type for getWindowDialogQueue * Apply suggestions from code review Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com> Co-authored-by: Benjamin Pasero <benjpas@microsoft.com> Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
* draft trusted workspace service / model * renaming * add request model and action * err fix * add request handlers with mock actions * some quick fixes * adding badge icon to activity bar gear * Add Statusbar item to indicate trust * Cleanup code * Add background color * Use theme color for the status background color * adding basic editing experience * observe trust with startup tasks * Extension enablement * Add capability to provide a custom message * Remove old actions * explorer: if you can not undo, pass undo to editor fixes #111630 * Remove plug icon from ports view Part of microsoft/vscode-internalbacklog#1689 * Fixed compilation error * Handle extension uninstall * Handle extension install * Ability to prompt when state is untrusted * Do not change state is the modal dialog is dismissed or the Cancel button is pressed * Refactored enablement code * Prompt when installing from VSIX * Prompt when installing from the Gallery * Move file into the browser folder * fixes and polish * restructure workspace contributions * restructure actions and use confirmations * Initial draft of the proposed APIs * Added stubs for the proposed api * Trusted Workspace proposed API * Fix a regression introduced by merge * status bar indicator improvements * remove helper command as we now have hooks * verbose messaging for the immediate request * add indication to global activity icon of pending request * try personal title * Add configuration setting * Add additional extension actions * Fix contributions * Removed context key that is not needed * Fixed issue with the dialog * Reduce arbitrary event limiter from 16ms down to 4.16666 (support for monitors up-to 240hz) #107016 * Fixes #115221: update emoji tests * Give a higher priority to language configuration set via API call (#114684) * debug console menu action polish * Avoid the CSS general sibling combinator ~ for perf reasons * more notebook todos * Use label as tooltip fallback properly Part of #115337 * Fixes microsoft/monaco-editor#2329: Move `registerThemingParticipant` call to `/editor/` * Fix port label not always getting set Part of microsoft/vscode-remote-release#4364 * simplify map creation, fyi @bpasero * Fix #114432: Multiple save dialogs appearing on Windows if Ctrl+S is pressed multiple times (#114450) * fix multiple save dialogs appearing on Windows when spamming Ctrl+S * remove old fix and instead keep track of windows with open dialogs in the dialogMainService * keep initialisation of activeWindowDialogs in constructor * remove unused variable * some changes * queue dialogs based on hash of options * simplify structure, fix comment typo * Apply suggestions from code review Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com> * remove unnecessary async/await for aquireFileDialogLock method * don't acquire file dialog lock for message boxes * use MessageBoxReturnValue | SaveDialogReturnValue | OpenDialogReturnValue instead of any type for getWindowDialogQueue * Apply suggestions from code review Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com> Co-authored-by: Benjamin Pasero <benjpas@microsoft.com> Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com> * 💄 dialog main service locks * debt - adopt some ? operator * Better hiding of custom hover in icon label * Limit to 8ms (120fps) * more API todos for notebooks * 💄 * Update grammars * chore - group notebook specific api proposals together * added unreleased fixes to endgame notebook * Add changes back to the modal dialog * Add back the workspace trust proposed APIs * Adjust dialog buttons * Standardize on WorkspaceTrust name across interfaces, classes, variables * Renamed some of the missing keys * Add TestWorkspaceTrust stub and fix failing tests * Add requiresWorkspaceTrust property to fix test failure * remove notebook change Co-authored-by: Ladislau Szomoru <lszomoru@microsoft.com> Co-authored-by: isidor <inikolic@microsoft.com> Co-authored-by: Alex Ross <alros@microsoft.com> Co-authored-by: TacticalDan <gorksorf@gmail.com> Co-authored-by: Alexandru Dima <alexdima@microsoft.com> Co-authored-by: Johannes Rieken <johannes.rieken@gmail.com> Co-authored-by: Cameron <cameron532@gmail.com> Co-authored-by: Benjamin Pasero <benjpas@microsoft.com> Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>
This PR fixes #114432
I've added a basic condition check to the pickFileToSave method inside the FileDialogService.
This ensures that the file save dialog will only appear if there isn't one already open and will prevent multiple dialogs appearing if Ctrl+S is spammed a few times on Windows.
This is my first contribution here and I'm still very new to this codebase, so please let me know if there is a more elegant solution (or if Electron provides this kind of functionality somewhere).