From ae347bb5de02b03fc2b73a812ea29f9d772ba8ff Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 13 Nov 2024 15:13:24 +0100 Subject: [PATCH 1/8] remove accelerators for linux menu --- packages/compass/src/main/menu.ts | 61 +++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/packages/compass/src/main/menu.ts b/packages/compass/src/main/menu.ts index d960af8b94e..6c81788b8a2 100644 --- a/packages/compass/src/main/menu.ts +++ b/packages/compass/src/main/menu.ts @@ -1,7 +1,14 @@ import type { MenuItemConstructorOptions } from 'electron'; -import { BrowserWindow, Menu, app, dialog, shell } from 'electron'; +import { + BrowserWindow, + Menu, + app as electronApp, + dialog, + shell, +} from 'electron'; import { ipcMain } from 'hadron-ipc'; import fs from 'fs'; +import os from 'os'; import path from 'path'; import createDebug from 'debug'; import type { THEMES } from 'compass-preferences-model'; @@ -34,11 +41,11 @@ function quitItem( accelerator: 'CmdOrCtrl+Q', click() { !compassApp.preferences.getPreferences().enableShowDialogOnQuit - ? app.quit() + ? electronApp.quit() : void dialog .showMessageBox({ type: 'warning', - title: `Quit ${app.getName()}`, + title: `Quit ${electronApp.getName()}`, icon: COMPASS_ICON, message: 'Are you sure you want to quit?', buttons: ['Quit', 'Cancel'], @@ -50,7 +57,7 @@ function quitItem( void compassApp.preferences.savePreferences({ enableShowDialogOnQuit: false, }); - app.quit(); + electronApp.quit(); } }); }, @@ -96,10 +103,10 @@ function darwinCompassSubMenu( compassApp: typeof CompassApplication ): MenuItemConstructorOptions { return { - label: app.getName(), + label: electronApp.getName(), submenu: [ { - label: `About ${app.getName()}`, + label: `About ${electronApp.getName()}`, role: 'about', }, updateSubmenu(windowState, compassApp), @@ -239,14 +246,14 @@ function editSubMenu(): MenuItemConstructorOptions { function nonDarwinAboutItem(): MenuItemConstructorOptions { return { - label: `&About ${app.getName()}`, + label: `&About ${electronApp.getName()}`, click() { void dialog.showMessageBox({ type: 'info', - title: 'About ' + app.getName(), + title: 'About ' + electronApp.getName(), icon: COMPASS_ICON, - message: app.getName(), - detail: 'Version ' + app.getVersion(), + message: electronApp.getName(), + detail: 'Version ' + electronApp.getVersion(), buttons: ['OK'], }); }, @@ -255,7 +262,7 @@ function nonDarwinAboutItem(): MenuItemConstructorOptions { function helpWindowItem(): MenuItemConstructorOptions { return { - label: `&Online ${app.getName()} Help`, + label: `&Online ${electronApp.getName()} Help`, accelerator: 'F1', click() { void shell.openExternal(COMPASS_HELP); @@ -299,7 +306,7 @@ function license(): MenuItemConstructorOptions { label: '&License', click() { void import('../../LICENSE').then(({ default: LICENSE }) => { - const licenseTemp = path.join(app.getPath('temp'), 'License'); + const licenseTemp = path.join(electronApp.getPath('temp'), 'License'); fs.writeFile(licenseTemp, LICENSE, (err) => { if (!err) { void shell.openPath(licenseTemp); @@ -513,6 +520,24 @@ class WindowMenuState { updateManagerState: UpdateManagerState = 'idle'; } +function removeAcceleratorFromMenu( + menu?: MenuItemConstructorOptions[] +): MenuItemConstructorOptions[] { + if (!menu || !Array.isArray(menu)) { + return []; + } + // eslint-disable-next-line @typescript-eslint/no-unused-vars + return menu.map(({ accelerator, ...item }) => { + if (!item.submenu || !Array.isArray(item.submenu)) { + return item; + } + return { + ...item, + submenu: removeAcceleratorFromMenu(item.submenu), + }; + }); +} + class CompassMenu { private constructor() { // marking constructor as private to disallow usage @@ -618,9 +643,9 @@ class CompassMenu { } private static async setupDockMenu() { - await app.whenReady(); + await electronApp.whenReady(); if (process.platform === 'darwin') { - app.dock.setMenu( + electronApp.dock.setMenu( Menu.buildFromTemplate([ { label: 'New Window', @@ -686,7 +711,13 @@ class CompassMenu { if (process.platform === 'darwin') { return darwinMenu(menuState, this.app); } - return nonDarwinMenu(menuState, this.app); + const menu = nonDarwinMenu(menuState, this.app); + // Currently on Linux, the menu accelerators crash the app and while + // fix is out, we are removing the accelerators from the all menu items. + if (os.platform() === 'linux') { + return removeAcceleratorFromMenu(menu); + } + return menu; } private static refreshMenu = () => { From 51c3de3be24e28a98949b2c8a8a1888a8f34f35c Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 13 Nov 2024 15:26:00 +0100 Subject: [PATCH 2/8] fix tests --- packages/compass/src/main/menu.spec.ts | 67 +++++++++++++++++++++++++- packages/compass/src/main/menu.ts | 3 +- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index 28b9555ed8c..d365f367170 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -457,7 +457,8 @@ describe('CompassMenu', function () { ]); }); - ['linux', 'win32'].forEach((platform) => { + // TODO(COMPASS-XXXX): Add `linux` back to this list + ['win32'].forEach((platform) => { // TODO(COMPASS-7906): remove it.skip(`[single-connection] should generate a menu template for ${platform}`, function () { sinon.stub(process, 'platform').value(platform); @@ -588,6 +589,70 @@ describe('CompassMenu', function () { }); }); + // TODO(COMPASS-XXXX): Remove this test + it('should generate a menu template for linux', async function () { + await App.preferences.savePreferences({ + enableMultipleConnectionSystem: true, + }); + sinon.stub(process, 'platform').value('linux'); + + expect(serializable(CompassMenu.getTemplate(0))).to.deep.equal([ + { + label: '&Connections', + submenu: [ + { label: '&Import Saved Connections' }, + { label: '&Export Saved Connections' }, + { type: 'separator' }, + { label: 'E&xit' }, + ], + }, + { + label: 'Edit', + submenu: [ + { label: 'Undo', role: 'undo' }, + { label: 'Redo', role: 'redo' }, + { type: 'separator' }, + { label: 'Cut', role: 'cut' }, + { label: 'Copy', role: 'copy' }, + { label: 'Paste', role: 'paste' }, + { + label: 'Select All', + role: 'selectAll', + }, + { type: 'separator' }, + { label: 'Find' }, + { type: 'separator' }, + { label: '&Settings' }, + ], + }, + { + label: '&View', + submenu: [ + { label: '&Reload' }, + { label: '&Reload Data' }, + { type: 'separator' }, + { label: 'Actual Size' }, + { label: 'Zoom In' }, + { label: 'Zoom Out' }, + ], + }, + { + label: '&Help', + submenu: [ + { label: `&Online ${app.getName()} Help` }, + { label: '&License' }, + { label: `&View Source Code on GitHub` }, + { label: `&Suggest a Feature` }, + { label: `&Report a Bug` }, + { label: '&Open Log File' }, + { type: 'separator' }, + { label: `&About ${app.getName()}` }, + { label: 'Check for updates…' }, + ], + }, + ]); + }); + it('should generate a menu template without collection submenu if `showCollection` is `false`', function () { expect( CompassMenu.getTemplate(0).find((item) => item.label === '&Collection') diff --git a/packages/compass/src/main/menu.ts b/packages/compass/src/main/menu.ts index 6c81788b8a2..5f812955171 100644 --- a/packages/compass/src/main/menu.ts +++ b/packages/compass/src/main/menu.ts @@ -712,8 +712,7 @@ class CompassMenu { return darwinMenu(menuState, this.app); } const menu = nonDarwinMenu(menuState, this.app); - // Currently on Linux, the menu accelerators crash the app and while - // fix is out, we are removing the accelerators from the all menu items. + // TODO(COMPASS-XXXX): Remove this check once accelerator issue is resolve for linux. if (os.platform() === 'linux') { return removeAcceleratorFromMenu(menu); } From da546b7b2b197e19534dbfaab3ecf992bc50d221 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 13 Nov 2024 15:32:02 +0100 Subject: [PATCH 3/8] add todo ticket --- packages/compass/src/main/menu.spec.ts | 4 ++-- packages/compass/src/main/menu.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index d365f367170..49383c8ee88 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -457,7 +457,7 @@ describe('CompassMenu', function () { ]); }); - // TODO(COMPASS-XXXX): Add `linux` back to this list + // TODO(COMPASS-8505): Add `linux` back to this list ['win32'].forEach((platform) => { // TODO(COMPASS-7906): remove it.skip(`[single-connection] should generate a menu template for ${platform}`, function () { @@ -589,7 +589,7 @@ describe('CompassMenu', function () { }); }); - // TODO(COMPASS-XXXX): Remove this test + // TODO(COMPASS-8505): Remove this test it('should generate a menu template for linux', async function () { await App.preferences.savePreferences({ enableMultipleConnectionSystem: true, diff --git a/packages/compass/src/main/menu.ts b/packages/compass/src/main/menu.ts index 5f812955171..e4df8d6d050 100644 --- a/packages/compass/src/main/menu.ts +++ b/packages/compass/src/main/menu.ts @@ -712,7 +712,7 @@ class CompassMenu { return darwinMenu(menuState, this.app); } const menu = nonDarwinMenu(menuState, this.app); - // TODO(COMPASS-XXXX): Remove this check once accelerator issue is resolve for linux. + // TODO(COMPASS-8505): Remove this check once accelerator issue is resolve for linux. if (os.platform() === 'linux') { return removeAcceleratorFromMenu(menu); } From 2ef8483ccdacd2137a6e7059bcc3738b2aea500e Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 13 Nov 2024 16:08:08 +0100 Subject: [PATCH 4/8] add test --- packages/compass/src/main/menu.spec.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index 49383c8ee88..a9d56ba0743 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -1,4 +1,4 @@ -import EventEmitter from 'events'; +import EventEmitter, { once } from 'events'; import type { MenuItemConstructorOptions } from 'electron'; import { BrowserWindow, ipcMain, Menu, app, dialog } from 'electron'; import { expect } from 'chai'; @@ -653,6 +653,25 @@ describe('CompassMenu', function () { ]); }); + it('does not crash when rendering menu item with an accelerator', async () => { + const menu = Menu.buildFromTemplate([ + { + label: 'Test Super', + accelerator: 'Super+Ctrl+T', + }, + { + label: 'Test Meta', + accelerator: 'Meta+Ctrl+T', + }, + ]); + const menuWillClose = once(menu, 'menu-will-close'); + menu.popup({ + window: new BrowserWindow({ show: false, width: 100, height: 100 }), + }); + menu.closePopup(); + await menuWillClose; + }); + it('should generate a menu template without collection submenu if `showCollection` is `false`', function () { expect( CompassMenu.getTemplate(0).find((item) => item.label === '&Collection') From 2cafca012d591076f970fde83d50024542d19d96 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Wed, 13 Nov 2024 16:13:04 +0100 Subject: [PATCH 5/8] filter more accelerators for linux --- packages/compass/src/main/menu.spec.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index a9d56ba0743..f6b28c61345 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -3,6 +3,7 @@ import type { MenuItemConstructorOptions } from 'electron'; import { BrowserWindow, ipcMain, Menu, app, dialog } from 'electron'; import { expect } from 'chai'; import sinon from 'sinon'; +import os from 'os'; import { createSandboxFromDefaultPreferences } from 'compass-preferences-model'; import type { CompassApplication } from './application'; @@ -696,7 +697,12 @@ describe('CompassMenu', function () { label: '&Collection', submenu: [ { - accelerator: 'Alt+CmdOrCtrl+S', + // TODO(COMPASS-8505): Add `accelerator` back to this + ...(os.platform() === 'linux' + ? {} + : { + accelerator: 'Alt+CmdOrCtrl+S', + }), label: '&Share Schema as JSON', }, { @@ -730,7 +736,12 @@ describe('CompassMenu', function () { label: '&Collection', submenu: [ { - accelerator: 'Alt+CmdOrCtrl+S', + // TODO(COMPASS-8505): Add `accelerator` back to this + ...(os.platform() === 'linux' + ? {} + : { + accelerator: 'Alt+CmdOrCtrl+S', + }), label: '&Share Schema as JSON', }, { @@ -754,7 +765,12 @@ describe('CompassMenu', function () { expect( menu.find((item: any) => item.label === '&Toggle DevTools') ).to.deep.eq({ - accelerator: 'Alt+CmdOrCtrl+I', + // TODO(COMPASS-8505): Add `accelerator` back to this + ...(os.platform() === 'linux' + ? {} + : { + accelerator: 'Alt+CmdOrCtrl+I', + }), label: '&Toggle DevTools', }); }); From 796da35e34e12c1e770ae734e8242d561184412f Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Thu, 14 Nov 2024 09:39:37 +0100 Subject: [PATCH 6/8] compass menu accelerator assertions --- packages/compass/src/main/menu.spec.ts | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index f6b28c61345..0612a1fbd54 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -664,10 +664,30 @@ describe('CompassMenu', function () { label: 'Test Meta', accelerator: 'Meta+Ctrl+T', }, + { + label: 'Test CmdOrCtrl', + accelerator: 'CmdOrCtrl+Q', + }, + { + label: 'Test Command', + accelerator: 'Command+H', + }, + { + label: 'Test Command+Shift', + accelerator: 'Command+Shift+H', + }, + { + label: 'Test Atl+CmdOrCtrl', + accelerator: 'Alt+CmdOrCtrl+S', + }, + { + label: 'Test Shift+CmdOrCtrl', + accelerator: 'Shift+CmdOrCtrl+S', + }, ]); const menuWillClose = once(menu, 'menu-will-close'); menu.popup({ - window: new BrowserWindow({ show: false, width: 100, height: 100 }), + window: new BrowserWindow({ show: false }), }); menu.closePopup(); await menuWillClose; From 17a8807eb3eb625b794acdb9cd92139b9269f1c3 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Thu, 14 Nov 2024 09:47:51 +0100 Subject: [PATCH 7/8] skip tests on linux --- packages/compass/src/main/menu.spec.ts | 81 ++++++++++++++------------ 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index 0612a1fbd54..fc17ef928c0 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -654,44 +654,49 @@ describe('CompassMenu', function () { ]); }); - it('does not crash when rendering menu item with an accelerator', async () => { - const menu = Menu.buildFromTemplate([ - { - label: 'Test Super', - accelerator: 'Super+Ctrl+T', - }, - { - label: 'Test Meta', - accelerator: 'Meta+Ctrl+T', - }, - { - label: 'Test CmdOrCtrl', - accelerator: 'CmdOrCtrl+Q', - }, - { - label: 'Test Command', - accelerator: 'Command+H', - }, - { - label: 'Test Command+Shift', - accelerator: 'Command+Shift+H', - }, - { - label: 'Test Atl+CmdOrCtrl', - accelerator: 'Alt+CmdOrCtrl+S', - }, - { - label: 'Test Shift+CmdOrCtrl', - accelerator: 'Shift+CmdOrCtrl+S', - }, - ]); - const menuWillClose = once(menu, 'menu-will-close'); - menu.popup({ - window: new BrowserWindow({ show: false }), - }); - menu.closePopup(); - await menuWillClose; - }); + // TODO(COMPASS-8505): Remove skipping on linux + const testIt = os.platform() === 'linux' ? it.skip : it; + testIt( + 'does not crash when rendering menu item with an accelerator', + async () => { + const menu = Menu.buildFromTemplate([ + { + label: 'Test Super', + accelerator: 'Super+Ctrl+T', + }, + { + label: 'Test Meta', + accelerator: 'Meta+Ctrl+T', + }, + { + label: 'Test CmdOrCtrl', + accelerator: 'CmdOrCtrl+Q', + }, + { + label: 'Test Command', + accelerator: 'Command+H', + }, + { + label: 'Test Command+Shift', + accelerator: 'Command+Shift+H', + }, + { + label: 'Test Atl+CmdOrCtrl', + accelerator: 'Alt+CmdOrCtrl+S', + }, + { + label: 'Test Shift+CmdOrCtrl', + accelerator: 'Shift+CmdOrCtrl+S', + }, + ]); + const menuWillClose = once(menu, 'menu-will-close'); + menu.popup({ + window: new BrowserWindow({ show: false }), + }); + menu.closePopup(); + await menuWillClose; + } + ); it('should generate a menu template without collection submenu if `showCollection` is `false`', function () { expect( From 383ff25ba249f8b2213a1285feee89c4bda44f60 Mon Sep 17 00:00:00 2001 From: Basit Chonka Date: Fri, 15 Nov 2024 02:12:05 +0100 Subject: [PATCH 8/8] use actual menu for tests --- packages/compass/src/main/menu.spec.ts | 57 +++++++------------------- packages/compass/src/main/menu.ts | 2 +- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/packages/compass/src/main/menu.spec.ts b/packages/compass/src/main/menu.spec.ts index fc17ef928c0..fc374b5f8e3 100644 --- a/packages/compass/src/main/menu.spec.ts +++ b/packages/compass/src/main/menu.spec.ts @@ -1,4 +1,4 @@ -import EventEmitter, { once } from 'events'; +import EventEmitter from 'events'; import type { MenuItemConstructorOptions } from 'electron'; import { BrowserWindow, ipcMain, Menu, app, dialog } from 'electron'; import { expect } from 'chai'; @@ -654,49 +654,22 @@ describe('CompassMenu', function () { ]); }); - // TODO(COMPASS-8505): Remove skipping on linux - const testIt = os.platform() === 'linux' ? it.skip : it; - testIt( - 'does not crash when rendering menu item with an accelerator', - async () => { - const menu = Menu.buildFromTemplate([ - { - label: 'Test Super', - accelerator: 'Super+Ctrl+T', - }, - { - label: 'Test Meta', - accelerator: 'Meta+Ctrl+T', - }, - { - label: 'Test CmdOrCtrl', - accelerator: 'CmdOrCtrl+Q', - }, - { - label: 'Test Command', - accelerator: 'Command+H', - }, - { - label: 'Test Command+Shift', - accelerator: 'Command+Shift+H', - }, - { - label: 'Test Atl+CmdOrCtrl', - accelerator: 'Alt+CmdOrCtrl+S', - }, - { - label: 'Test Shift+CmdOrCtrl', - accelerator: 'Shift+CmdOrCtrl+S', - }, - ]); - const menuWillClose = once(menu, 'menu-will-close'); - menu.popup({ - window: new BrowserWindow({ show: false }), - }); + it('does not crash when rendering menu item with an accelerator', () => { + const window = new BrowserWindow({ show: false }); + const template = CompassMenu.getTemplate(window.id); + + // As the root menu items do not have accelerators, we test + // against each item's submenu. + for (const item of template) { + // for TS. compass menu has submenus + if (!Array.isArray(item.submenu)) { + continue; + } + const menu = Menu.buildFromTemplate(item.submenu); + menu.popup({ window }); menu.closePopup(); - await menuWillClose; } - ); + }); it('should generate a menu template without collection submenu if `showCollection` is `false`', function () { expect( diff --git a/packages/compass/src/main/menu.ts b/packages/compass/src/main/menu.ts index e4df8d6d050..5c4b4ebd5f1 100644 --- a/packages/compass/src/main/menu.ts +++ b/packages/compass/src/main/menu.ts @@ -523,7 +523,7 @@ class WindowMenuState { function removeAcceleratorFromMenu( menu?: MenuItemConstructorOptions[] ): MenuItemConstructorOptions[] { - if (!menu || !Array.isArray(menu)) { + if (!Array.isArray(menu)) { return []; } // eslint-disable-next-line @typescript-eslint/no-unused-vars