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 5 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
110 changes: 105 additions & 5 deletions packages/compass/src/main/menu.spec.ts
View file
Open in desktop
Copy link
Collaborator

@gribnoysup gribnoysup Nov 13, 2024

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

@mabaasit mabaasit Nov 13, 2024

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
@@ -1,8 +1,9 @@
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';
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,89 @@ 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', async () => {
const menu = Menu.buildFromTemplate([
{
label: 'Test Super',
accelerator: 'Super+Ctrl+T',
},
{
label: 'Test Meta',
accelerator: 'Meta+Ctrl+T',
},
]);
Copy link
Collaborator

@gribnoysup gribnoysup Nov 13, 2024

Choose a reason for hiding this comment

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

Can we use our template here so that we're checking that our template works on all platforms? 🙂 (this will probably fail now because this includes accelerator and will run on linux)

Copy link
Collaborator Author

@mabaasit mabaasit Nov 14, 2024

Choose a reason for hiding this comment

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

i added more checks and also skipped the tests for linux

Copy link
Collaborator

@gribnoysup gribnoysup Nov 14, 2024

Choose a reason for hiding this comment

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

I think there is some kind of misunderstanding here: the fix you're making is programmatically changing the template generated by our code in a way that should make it usable on linux, I want us to have a test that checks that the generated template actually works on linux (not tangentially checking it through something arbitrary like whether or not acceletator key is present), more specifically: I want this test to build our template and then run it through the logic in this test that would trigger the break if we're not doing it correctly and it should run on linux too as we're expecting the introduced change to make it work on linux. Does this make sense?

Copy link
Collaborator Author

@mabaasit mabaasit Nov 15, 2024

Choose a reason for hiding this comment

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

Yeah i see you point and there was misunderstanding from my side. I am testing the actual menu now 383ff25. I am not setting the menu on the app, directly trying to show it which if crashes would fail the test .

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')
Expand All @@ -612,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',
},
{
Expand Down Expand Up @@ -646,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',
},
{
Expand All @@ -670,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',
});
});
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 (!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
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