window - fix window state validation to actually apply properly and add more logging (microsoft#205677)

Author: bpasero

src/vs/platform/auxiliaryWindow/electron-main/auxiliaryWindowsMainService.ts

@@ -12,7 +12,7 @@ import { AuxiliaryWindow, IAuxiliaryWindow } from 'vs/platform/auxiliaryWindow/e
 import { IAuxiliaryWindowsMainService } from 'vs/platform/auxiliaryWindow/electron-main/auxiliaryWindows';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
 import { ILogService } from 'vs/platform/log/common/log';
-import { IWindowState } from 'vs/platform/window/electron-main/window';
+import { IWindowState, defaultAuxWindowState } from 'vs/platform/window/electron-main/window';
 import { WindowStateValidator, defaultBrowserWindowOptions, getLastFocused } from 'vs/platform/windows/electron-main/windows';
 
 export class AuxiliaryWindowsMainService extends Disposable implements IAuxiliaryWindowsMainService {
@@ -79,7 +79,7 @@ export class AuxiliaryWindowsMainService extends Disposable implements IAuxiliar
 		});
 	}
 
-	private validateWindowState(details: HandlerDetails): IWindowState | undefined {
+	private validateWindowState(details: HandlerDetails): IWindowState {
 		const windowState: IWindowState = {};
 
 		const features = details.features.split(','); // for example: popup=yes,left=270,top=14.5,width=800,height=600
@@ -101,7 +101,11 @@ export class AuxiliaryWindowsMainService extends Disposable implements IAuxiliar
 			}
 		}
 
-		return WindowStateValidator.validateWindowState(this.logService, windowState);
+		const state = WindowStateValidator.validateWindowState(this.logService, windowState) ?? defaultAuxWindowState();
+
+		this.logService.trace('[aux window] using window state', state);
+
+		return state;
 	}
 
 	registerWindow(webContents: WebContents): void {

src/vs/platform/window/electron-main/window.ts

@@ -3,7 +3,7 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { BrowserWindow, Rectangle } from 'electron';
+import { BrowserWindow, Rectangle, screen } from 'electron';
 import { CancellationToken } from 'vs/base/common/cancellation';
 import { Event } from 'vs/base/common/event';
 import { IDisposable } from 'vs/base/common/lifecycle';
@@ -145,6 +145,30 @@ export const defaultWindowState = function (mode = WindowMode.Normal): IWindowSt
 	};
 };
 
+export const defaultAuxWindowState = function (): IWindowState {
+
+	// Auxiliary windows are being created from a `window.open` call
+	// that sets `windowFeatures` that encode the desired size and
+	// position of the new window (`top`, `left`).
+	// In order to truly override this to a good default window state
+	// we need to set not only width and height but also x and y to
+	// a good location on the primary display.
+
+	const width = 800;
+	const height = 600;
+	const workArea = screen.getPrimaryDisplay().workArea;
+	const x = Math.max(workArea.x + (workArea.width / 2) - (width / 2), 0);
+	const y = Math.max(workArea.y + (workArea.height / 2) - (height / 2), 0);
+
+	return {
+		x,
+		y,
+		width,
+		height,
+		mode: WindowMode.Normal
+	};
+};
+
 export const enum WindowMode {
 	Maximized,
 	Normal,

src/vs/platform/windows/electron-main/windowImpl.ts

@@ -570,7 +570,6 @@ export class CodeWindow extends BaseWindow implements ICodeWindow {
 				}
 			});
 
-
 			// Create the browser window
 			mark('code/willCreateCodeBrowserWindow');
 			this._win = new BrowserWindow(options);

src/vs/platform/windows/electron-main/windows.ts

@@ -115,7 +115,7 @@ export interface IOpenConfiguration extends IBaseOpenConfiguration {
 
 export interface IOpenEmptyConfiguration extends IBaseOpenConfiguration { }
 
-export function defaultBrowserWindowOptions(accessor: ServicesAccessor, windowState?: IWindowState, overrides?: BrowserWindowConstructorOptions): BrowserWindowConstructorOptions & { experimentalDarkMode: boolean } {
+export function defaultBrowserWindowOptions(accessor: ServicesAccessor, windowState: IWindowState, overrides?: BrowserWindowConstructorOptions): BrowserWindowConstructorOptions & { experimentalDarkMode: boolean } {
 	const themeMainService = accessor.get(IThemeMainService);
 	const productService = accessor.get(IProductService);
 	const configurationService = accessor.get(IConfigurationService);
@@ -129,10 +129,14 @@ export function defaultBrowserWindowOptions(accessor: ServicesAccessor, windowSt
 		minHeight: WindowMinimumSize.HEIGHT,
 		title: productService.nameLong,
 		...overrides,
+		x: windowState.x,
+		y: windowState.y,
+		width: windowState.width,
+		height: windowState.height,
 		webPreferences: {
 			enableWebSQL: false,
 			spellcheck: false,
-			zoomFactor: zoomLevelToZoomFactor(windowState?.zoomLevel ?? windowSettings?.zoomLevel),
+			zoomFactor: zoomLevelToZoomFactor(windowState.zoomLevel ?? windowSettings?.zoomLevel),
 			autoplayPolicy: 'user-gesture-required',
 			// Enable experimental css highlight api https://chromestatus.com/feature/5436441440026624
 			// Refs https://github.com/microsoft/vscode/issues/140098
@@ -143,13 +147,6 @@ export function defaultBrowserWindowOptions(accessor: ServicesAccessor, windowSt
 		experimentalDarkMode: true
 	};
 
-	if (windowState) {
-		options.x = windowState.x;
-		options.y = windowState.y;
-		options.width = windowState.width;
-		options.height = windowState.height;
-	}
-
 	if (isLinux) {
 		options.icon = join(environmentMainService.appRoot, 'resources/linux/code.png'); // always on Linux
 	} else if (isWindows && !environmentMainService.isBuilt) {
@@ -246,8 +243,9 @@ export namespace WindowStateValidator {
 		//          some pixels (128) visible on the screen for the user to drag it back.
 		if (displays.length === 1) {
 			const displayWorkingArea = getWorkingArea(displays[0]);
+			logService.trace('window#validateWindowState: single monitor working area', displayWorkingArea);
+
 			if (displayWorkingArea) {
-				logService.trace('window#validateWindowState: 1 monitor working area', displayWorkingArea);
 
 				function ensureStateInDisplayWorkingArea(): void {
 					if (!state || typeof state.x !== 'number' || typeof state.y !== 'number' || !displayWorkingArea) {
@@ -320,10 +318,13 @@ export namespace WindowStateValidator {
 		try {
 			display = screen.getDisplayMatching({ x: state.x, y: state.y, width: state.width, height: state.height });
 			displayWorkingArea = getWorkingArea(display);
+
+			logService.trace('window#validateWindowState: multi-monitor working area', displayWorkingArea);
 		} catch (error) {
 			// Electron has weird conditions under which it throws errors
 			// e.g. https://github.com/microsoft/vscode/issues/100334 when
 			// large numbers are passed in
+			logService.error('window#validateWindowState: error finding display for window state', error);
 		}
 
 		if (
@@ -334,11 +335,11 @@ export namespace WindowStateValidator {
 			state.x < displayWorkingArea.x + displayWorkingArea.width &&	// prevent window from falling out of the screen to the right
 			state.y < displayWorkingArea.y + displayWorkingArea.height		// prevent window from falling out of the screen to the bottom
 		) {
-			logService.trace('window#validateWindowState: multi-monitor working area', displayWorkingArea);
-
 			return state;
 		}
 
+		logService.trace('window#validateWindowState: state is outside of the multi-monitor working area');
+
 		return undefined;
 	}