Skip to content

Commit

Permalink
Fix #114432: Multiple save dialogs appearing on Windows if Ctrl+S is …
Browse files Browse the repository at this point in the history 
…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>
  • Loading branch information
@CameronIrvine @bpasero
3 people authored Feb 1, 2021
1 parent 178e703 commit 38ca469
Showing 1 changed file with 104 additions and 43 deletions.
147 changes: 104 additions & 43 deletions src/vs/platform/dialogs/electron-main/dialogMainService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { withNullAsUndefined } from 'vs/base/common/types';
import { localize } from 'vs/nls';
import { WORKSPACE_FILTER } from 'vs/platform/workspaces/common/workspaces';
import { mnemonicButtonLabel } from 'vs/base/common/labels';
import { Disposable, dispose, IDisposable, toDisposable } from 'vs/base/common/lifecycle';
import { hash } from 'vs/base/common/hash';

export const IDialogMainService = createDecorator<IDialogMainService>('dialogMainService');

Expand Down Expand Up @@ -48,14 +50,13 @@ export class DialogMainService implements IDialogMainService {

private static readonly workingDirPickerStorageKey = 'pickerWorkingDir';

private readonly mapWindowToDialogQueue: Map<number, Queue<void>>;
private readonly noWindowDialogQueue: Queue<void>;
private readonly windowDialogLocks = new Map<number, Set<number>>();
private readonly windowDialogQueues = new Map<number, Queue<any>>();
private readonly noWindowDialogueQueue = new Queue<any>();

constructor(
@IStateService private readonly stateService: IStateService
) {
this.mapWindowToDialogQueue = new Map<number, Queue<void>>();
this.noWindowDialogQueue = new Queue<void>();
}

pickFileFolder(options: INativeOpenDialogOptions, window?: BrowserWindow): Promise<string[] | undefined> {
Expand Down Expand Up @@ -123,22 +124,25 @@ export class DialogMainService implements IDialogMainService {
return;
}

private getDialogQueue(window?: BrowserWindow): Queue<any> {
if (!window) {
return this.noWindowDialogQueue;
}
private getWindowDialogQueue<T extends MessageBoxReturnValue | SaveDialogReturnValue | OpenDialogReturnValue>(window?: BrowserWindow): Queue<T> {

let windowDialogQueue = this.mapWindowToDialogQueue.get(window.id);
if (!windowDialogQueue) {
windowDialogQueue = new Queue<any>();
this.mapWindowToDialogQueue.set(window.id, windowDialogQueue);
}
// Queue message box requests per window so that one can show
// after the other.
if (window) {
let windowDialogQueue = this.windowDialogQueues.get(window.id);
if (!windowDialogQueue) {
windowDialogQueue = new Queue<T>();
this.windowDialogQueues.set(window.id, windowDialogQueue);
}

return windowDialogQueue;
return windowDialogQueue;
} else {
return this.noWindowDialogueQueue;
}
}

showMessageBox(options: MessageBoxOptions, window?: BrowserWindow): Promise<MessageBoxReturnValue> {
return this.getDialogQueue(window).queue(async () => {
return this.getWindowDialogQueue<MessageBoxReturnValue>(window).queue(async () => {
if (window) {
return dialog.showMessageBox(window, options);
}
Expand All @@ -147,7 +151,7 @@ export class DialogMainService implements IDialogMainService {
});
}

showSaveDialog(options: SaveDialogOptions, window?: BrowserWindow): Promise<SaveDialogReturnValue> {
async showSaveDialog(options: SaveDialogOptions, window?: BrowserWindow): Promise<SaveDialogReturnValue> {

function normalizePath(path: string | undefined): string | undefined {
if (path && isMacintosh) {
Expand All @@ -157,21 +161,31 @@ export class DialogMainService implements IDialogMainService {
return path;
}

return this.getDialogQueue(window).queue(async () => {
let result: SaveDialogReturnValue;
if (window) {
result = await dialog.showSaveDialog(window, options);
} else {
result = await dialog.showSaveDialog(options);
}
// prevent duplicates of the same dialog queueing at the same time
const fileDialogLock = this.acquireFileDialogLock(options, window);
if (!fileDialogLock) {
throw new Error('A file save dialog is already showing for the window with the same configuration');
}

result.filePath = normalizePath(result.filePath);
try {
return await this.getWindowDialogQueue<SaveDialogReturnValue>(window).queue(async () => {
let result: SaveDialogReturnValue;
if (window) {
result = await dialog.showSaveDialog(window, options);
} else {
result = await dialog.showSaveDialog(options);
}

return result;
});
result.filePath = normalizePath(result.filePath);

return result;
});
} finally {
dispose(fileDialogLock);
}
}

showOpenDialog(options: OpenDialogOptions, window?: BrowserWindow): Promise<OpenDialogReturnValue> {
async showOpenDialog(options: OpenDialogOptions, window?: BrowserWindow): Promise<OpenDialogReturnValue> {

function normalizePaths(paths: string[]): string[] {
if (paths && paths.length > 0 && isMacintosh) {
Expand All @@ -181,27 +195,74 @@ export class DialogMainService implements IDialogMainService {
return paths;
}

return this.getDialogQueue(window).queue(async () => {
// Ensure the path exists (if provided)
if (options.defaultPath) {
const pathExists = await exists(options.defaultPath);
if (!pathExists) {
options.defaultPath = undefined;
}
}

// prevent duplicates of the same dialog queueing at the same time
const fileDialogLock = this.acquireFileDialogLock(options, window);
if (!fileDialogLock) {
throw new Error('A file open dialog is already showing for the window with the same configuration');
}

// Ensure the path exists (if provided)
if (options.defaultPath) {
const pathExists = await exists(options.defaultPath);
if (!pathExists) {
options.defaultPath = undefined;
try {
return await this.getWindowDialogQueue<OpenDialogReturnValue>(window).queue(async () => {
let result: OpenDialogReturnValue;
if (window) {
result = await dialog.showOpenDialog(window, options);
} else {
result = await dialog.showOpenDialog(options);
}
}

// Show dialog
let result: OpenDialogReturnValue;
if (window) {
result = await dialog.showOpenDialog(window, options);
} else {
result = await dialog.showOpenDialog(options);
}
result.filePaths = normalizePaths(result.filePaths);

return result;
});
} finally {
dispose(fileDialogLock);
}
}

private acquireFileDialogLock(options: SaveDialogOptions | OpenDialogOptions, window?: BrowserWindow): IDisposable | undefined {

// if no window is provided, allow as many dialogs as
// needed since we consider them not modal per window
if (!window) {
return Disposable.None;
}

// if a window is provided, only allow a single dialog
// at the same time because dialogs are modal and we
// do not want to open one dialog after the other
// (https://github.com/microsoft/vscode/issues/114432)
let windowDialogLocks = this.windowDialogLocks.get(window.id);

// figure out if a dialog with these options is already
// showing by hashing the options
const optionsHash = hash(options);
if (windowDialogLocks?.has(optionsHash)) {
return undefined;
}

result.filePaths = normalizePaths(result.filePaths);
if (!windowDialogLocks) {
windowDialogLocks = new Set();
this.windowDialogLocks.set(window.id, windowDialogLocks);
}

windowDialogLocks.add(optionsHash);

return result;
return toDisposable(() => {
const windowDialogLocks = this.windowDialogLocks.get(window.id);
windowDialogLocks?.delete(optionsHash);

// if the window has no more dialog locks, delete it from the set of locks
if (windowDialogLocks?.size === 0) {
this.windowDialogLocks.delete(window.id);
}
});
}
}

0 comments on commit 38ca469

Please sign in to comment.