Fix #114432: Multiple save dialogs appearing on Windows if Ctrl+S is pressed multiple times #114450

CameronIrvine · 2021-01-16T03:00:38Z

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).

bpasero · 2021-01-18T10:11:48Z

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

alexr00 · 2021-01-18T10:21:35Z

The simple file dialog won't have this problem. I can't actually repro the original issue on Windows with the native dialog.

CameronIrvine · 2021-01-18T13:21:15Z

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 dialog.showSaveDialog but I could be wrong.

bpasero

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

ghost · 2021-01-19T10:21:47Z

All CLA requirements met.

CameronIrvine · 2021-01-19T10:29:46Z

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).

bpasero · 2021-01-19T14:36:24Z

@CameronIrvine I pushed 0ba4635 to your branch and now I am asking you for a review + testing. Summary:

change method to acquireFileDialogLock and simplify by returning a IDisposable for releasing
make sure to preserve the previous semantic of waiting on any queued message box via joinMessageBoxQueue
most importantly: throw an Error when a file dialog is already opened instead of returning a bogus result

bpasero · 2021-01-19T15:33:59Z

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 Ctrl+S multiple times).

alexr00 · 2021-01-19T15:46:16Z

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.

bpasero · 2021-01-19T16:06:35Z

Yeah it is a tradeoff, I am open for better ideas.

CameronIrvine · 2021-01-20T08:03:20Z

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.

bpasero · 2021-01-20T08:19:13Z

Yeah agree, I would go with queueing by options for now. We have a hash utility in base that could be used I think.

CameronIrvine · 2021-01-22T10:33:56Z

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.

src/vs/platform/dialogs/electron-main/dialogMainService.ts

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

…alue instead of any type for getWindowDialogQueue

bpasero

Thanks, almost there...

bpasero · 2021-02-01T07:24:02Z

src/vs/platform/dialogs/electron-main/dialogMainService.ts

+	private readonly windowDialogQueues = new Map<number, Queue<any>>();
+	private readonly noWindowDialogueQueue = new Queue<any>();


Can we still get rid of these 2 any usages somehow?

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.

src/vs/platform/dialogs/electron-main/dialogMainService.ts

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

bpasero · 2021-02-01T13:41:40Z

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>

…trl+S is pressed multiple times (microsoft#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>

@bpasero

* 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>

CameronIrvine added 2 commits January 16, 2021 13:07

fix multiple save dialogs appearing on Windows when spamming Ctrl+S

436d6bf

Merge branch 'master' into multiple-save-dialogs

00e4d94

bpasero self-assigned this Jan 18, 2021

bpasero added the info-needed Issue requires more information from poster label Jan 18, 2021

bpasero added this to the On Deck milestone Jan 18, 2021

bpasero requested changes Jan 18, 2021

View reviewed changes

bpasero mentioned this pull request Jan 18, 2021

Add queuing to dialog service #70979

Closed

remove old fix and instead keep track of windows with open dialogs in…

4cf0bae

… the dialogMainService

CameronIrvine requested a review from bpasero January 19, 2021 10:43

CameronIrvine and others added 3 commits January 19, 2021 21:49

keep initialisation of activeWindowDialogs in constructor

0124bbb

remove unused variable

30b0b78

some changes

0ba4635

queue dialogs based on hash of options

67a9db4

bpasero removed the info-needed Issue requires more information from poster label Jan 22, 2021

bpasero modified the milestones: On Deck, February 2021 Jan 22, 2021

simplify structure, fix comment typo

2089a8a

bpasero requested changes Jan 30, 2021

View reviewed changes

CameronIrvine and others added 4 commits January 30, 2021 20:34

Apply suggestions from code review

81922a7

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

remove unnecessary async/await for aquireFileDialogLock method

1e9e036

don't acquire file dialog lock for message boxes

7106e19

use MessageBoxReturnValue | SaveDialogReturnValue | OpenDialogReturnV…

ae5d9e4

…alue instead of any type for getWindowDialogQueue

CameronIrvine requested a review from bpasero January 30, 2021 09:50

bpasero requested changes Feb 1, 2021

View reviewed changes

Apply suggestions from code review

cc8806a

Co-authored-by: Benjamin Pasero <benjamin.pasero@gmail.com>

bpasero merged commit 38ca469 into microsoft:master Feb 1, 2021

github-actions bot locked and limited conversation to collaborators Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

CameronIrvine commented Jan 16, 2021

bpasero commented Jan 18, 2021

alexr00 commented Jan 18, 2021 •

edited

Loading

CameronIrvine commented Jan 18, 2021

bpasero left a comment

ghost commented Jan 19, 2021 •

edited by ghost

Loading

CameronIrvine commented Jan 19, 2021

bpasero commented Jan 19, 2021

bpasero commented Jan 19, 2021

alexr00 commented Jan 19, 2021

bpasero commented Jan 19, 2021

CameronIrvine commented Jan 20, 2021

bpasero commented Jan 20, 2021

CameronIrvine commented Jan 22, 2021

bpasero left a comment

bpasero Feb 1, 2021

CameronIrvine Feb 1, 2021

bpasero commented Feb 1, 2021

	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;
	}

		private readonly windowDialogQueues = new Map<number, Queue<any>>();
		private readonly noWindowDialogueQueue = new Queue<any>();

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

CameronIrvine commented Jan 16, 2021

bpasero commented Jan 18, 2021

alexr00 commented Jan 18, 2021 • edited Loading

CameronIrvine commented Jan 18, 2021

bpasero left a comment

Choose a reason for hiding this comment

ghost commented Jan 19, 2021 • edited by ghost Loading

CameronIrvine commented Jan 19, 2021

bpasero commented Jan 19, 2021

bpasero commented Jan 19, 2021

alexr00 commented Jan 19, 2021

bpasero commented Jan 19, 2021

CameronIrvine commented Jan 20, 2021

bpasero commented Jan 20, 2021

CameronIrvine commented Jan 22, 2021

bpasero left a comment

Choose a reason for hiding this comment

bpasero Feb 1, 2021

Choose a reason for hiding this comment

CameronIrvine Feb 1, 2021

Choose a reason for hiding this comment

bpasero commented Feb 1, 2021

alexr00 commented Jan 18, 2021 •

edited

Loading

ghost commented Jan 19, 2021 •

edited by ghost

Loading