Skip to content

fix(menu): remove accelerators for linux COMPASS-8494 #6485

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Nov 18, 2024
Merged

fix(menu): remove accelerators for linux COMPASS-8494 #6485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ae347bb
remove accelerators for linux menu
mabaasit Nov 13, 2024
51c3de3
fix tests
mabaasit Nov 13, 2024
da546b7
add todo ticket
mabaasit Nov 13, 2024
2ef8483
add test
mabaasit Nov 13, 2024
2cafca0
filter more accelerators for linux
mabaasit Nov 13, 2024
796da35
compass menu accelerator assertions
mabaasit Nov 14, 2024
17a8807
skip tests on linux
mabaasit Nov 14, 2024
a212ad4
Merge branch 'main' into COMPASS-8494-disable-menu-accelerators
mabaasit Nov 14, 2024
383ff25
use actual menu for tests
mabaasit Nov 15, 2024
2a9b3f7
Merge branch 'COMPASS-8494-disable-menu-accelerators' of https://gith…
mabaasit Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 102 additions & 4 deletions packages/compass/src/main/menu.spec.ts
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test, similar to the one that was added in the backport PR you opened, that checks that the menus we build are working on the platforms we're running these tests at?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in 2ef8483

Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -457,7 +458,8 @@ describe('CompassMenu', function () {
]);
});

['linux', 'win32'].forEach((platform) => {
// 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 () {
sinon.stub(process, 'platform').value(platform);
Expand Down Expand Up @@ -588,6 +590,87 @@ describe('CompassMenu', function () {
});
});

// TODO(COMPASS-8505): 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('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();
}
});

it('should generate a menu template without collection submenu if `showCollection` is `false`', function () {
expect(
CompassMenu.getTemplate(0).find((item) => item.label === '&Collection')
Expand All @@ -612,7 +695,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',
},
{
Expand Down Expand Up @@ -646,7 +734,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',
},
{
Expand All @@ -670,7 +763,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',
});
});
Expand Down
60 changes: 45 additions & 15 deletions packages/compass/src/main/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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'],
Expand All @@ -50,7 +57,7 @@ function quitItem(
void compassApp.preferences.savePreferences({
enableShowDialogOnQuit: false,
});
app.quit();
electronApp.quit();
}
});
},
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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'],
});
},
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -513,6 +520,24 @@ class WindowMenuState {
updateManagerState: UpdateManagerState = 'idle';
}

function removeAcceleratorFromMenu(
menu?: MenuItemConstructorOptions[]
): MenuItemConstructorOptions[] {
if (!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
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -686,7 +711,12 @@ class CompassMenu {
if (process.platform === 'darwin') {
return darwinMenu(menuState, this.app);
}
return nonDarwinMenu(menuState, this.app);
const menu = nonDarwinMenu(menuState, this.app);
// TODO(COMPASS-8505): Remove this check once accelerator issue is resolve for linux.
if (os.platform() === 'linux') {
return removeAcceleratorFromMenu(menu);
}
return menu;
}

private static refreshMenu = () => {
Expand Down
Loading