mirror of
https://github.com/openswarm-ai/openswarm.git
synced 2026-08-26 06:22:22 +02:00
[eric] electron: declare the Edit accelerators so Cmd+C works in every window, not just the first (ENG-289)
This commit is contained in:
@@ -0,0 +1,81 @@
|
||||
// Cmd+C works in every window, not just the first (ENG-289).
|
||||
//
|
||||
// The app never called setApplicationMenu, so macOS was left with Electron's implicit default and
|
||||
// the Edit accelerators were whatever that happened to bind. The menu is process-global while its
|
||||
// items act on whichever webContents holds focus, so "which window" is decided at keypress time by
|
||||
// role dispatch, not by whatever window the menu was built next to. Declaring the menu ourselves
|
||||
// makes that dispatch explicit and identical in the first window and the tenth.
|
||||
//
|
||||
// ROLES ONLY, deliberately. A hand-written copy handler has to answer "copy from where", and the
|
||||
// answer is a focused-webContents lookup that is wrong for a <webview> guest exactly when the user
|
||||
// is inside one. Chromium already resolves roles against the focused contents, including guests,
|
||||
// so the correct implementation is the one where we write no logic at all.
|
||||
//
|
||||
// macOS only: on Windows and Linux setting a menu paints a visible menu bar this app has never had.
|
||||
|
||||
const APP_NAME = 'OpenSwarm';
|
||||
|
||||
/** The template, separated from the Electron call so it can be asserted without an app instance. */
|
||||
function applicationMenuTemplate() {
|
||||
return [
|
||||
{
|
||||
label: APP_NAME,
|
||||
submenu: [
|
||||
{ role: 'about' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'services' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'hide' },
|
||||
{ role: 'hideOthers' },
|
||||
{ role: 'unhide' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'quit' },
|
||||
],
|
||||
},
|
||||
{
|
||||
label: 'Edit',
|
||||
submenu: [
|
||||
{ role: 'undo' },
|
||||
{ role: 'redo' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'cut' },
|
||||
{ role: 'copy' },
|
||||
{ role: 'paste' },
|
||||
{ role: 'pasteAndMatchStyle' },
|
||||
{ role: 'delete' },
|
||||
{ role: 'selectAll' },
|
||||
],
|
||||
},
|
||||
{
|
||||
label: 'View',
|
||||
submenu: [
|
||||
{ role: 'reload' },
|
||||
{ role: 'forceReload' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'resetZoom' },
|
||||
{ role: 'zoomIn' },
|
||||
{ role: 'zoomOut' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'togglefullscreen' },
|
||||
],
|
||||
},
|
||||
{
|
||||
label: 'Window',
|
||||
submenu: [
|
||||
{ role: 'minimize' },
|
||||
{ role: 'zoom' },
|
||||
{ type: 'separator' },
|
||||
{ role: 'front' },
|
||||
],
|
||||
},
|
||||
];
|
||||
}
|
||||
|
||||
function installApplicationMenu(electron) {
|
||||
if (process.platform !== 'darwin') return false;
|
||||
const { Menu } = electron;
|
||||
Menu.setApplicationMenu(Menu.buildFromTemplate(applicationMenuTemplate()));
|
||||
return true;
|
||||
}
|
||||
|
||||
module.exports = { applicationMenuTemplate, installApplicationMenu, APP_NAME };
|
||||
@@ -0,0 +1,65 @@
|
||||
// Run: node --test electron/applicationMenu.test.js
|
||||
//
|
||||
// ENG-289: "open a second window, select text, Cmd+C, nothing is copied; the same steps in the
|
||||
// first window work." The app never declared an application menu, so the Edit accelerators were
|
||||
// whatever Electron's implicit default bound. The menu is process-global and its items act on the
|
||||
// FOCUSED webContents, so declaring it ourselves is what makes the second window behave like the
|
||||
// first.
|
||||
//
|
||||
// These assert the two properties that decide whether the bug can come back: the Edit accelerators
|
||||
// exist at all, and they are ROLES (Chromium resolves those against the focused contents, including
|
||||
// a <webview> guest) rather than hand-written handlers that have to guess at a target.
|
||||
const test = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
|
||||
const { applicationMenuTemplate, installApplicationMenu } = require('./applicationMenu');
|
||||
|
||||
function editSubmenu() {
|
||||
const edit = applicationMenuTemplate().find((m) => m.label === 'Edit');
|
||||
assert.ok(edit, 'there must be an Edit menu; it is what binds Cmd+C on macOS');
|
||||
return edit.submenu;
|
||||
}
|
||||
|
||||
test('the clipboard accelerators are all present', () => {
|
||||
const roles = editSubmenu().map((i) => i.role).filter(Boolean);
|
||||
for (const needed of ['copy', 'cut', 'paste', 'selectAll', 'undo', 'redo']) {
|
||||
assert.ok(roles.includes(needed), `Edit menu is missing the ${needed} role`);
|
||||
}
|
||||
});
|
||||
|
||||
test('clipboard items are roles, never hand-written click handlers', () => {
|
||||
// A click handler would have to resolve "copy from where" itself, and that lookup is wrong for a
|
||||
// webview guest exactly when the user is typing inside one.
|
||||
for (const item of editSubmenu()) {
|
||||
if (item.type === 'separator') continue;
|
||||
assert.ok(item.role, `Edit item ${JSON.stringify(item)} must be a role`);
|
||||
assert.equal(item.click, undefined, `Edit item ${item.role} must not carry a click handler`);
|
||||
}
|
||||
});
|
||||
|
||||
test('no item pins itself to a specific window', () => {
|
||||
const json = JSON.stringify(applicationMenuTemplate());
|
||||
assert.ok(!json.includes('mainWindow'), 'a menu bound to one window is exactly the reported bug');
|
||||
assert.ok(!json.includes('webContents'), 'roles resolve the target themselves; do not capture one');
|
||||
});
|
||||
|
||||
test('the menu is installed on macOS and skipped elsewhere', () => {
|
||||
const calls = [];
|
||||
const fake = { Menu: { setApplicationMenu: (m) => calls.push(m), buildFromTemplate: (t) => t } };
|
||||
const real = process.platform;
|
||||
try {
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true });
|
||||
assert.equal(installApplicationMenu(fake), true);
|
||||
assert.equal(calls.length, 1, 'macOS must get an explicit menu');
|
||||
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true });
|
||||
assert.equal(installApplicationMenu(fake), false, 'Windows would paint a menu bar this app never had');
|
||||
assert.equal(calls.length, 1);
|
||||
} finally {
|
||||
Object.defineProperty(process, 'platform', { value: real, configurable: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('main.js actually installs it', () => {
|
||||
const src = require('fs').readFileSync(require('path').join(__dirname, 'main.js'), 'utf8');
|
||||
assert.match(src, /installApplicationMenu/, 'a menu module nothing calls fixes nothing');
|
||||
});
|
||||
@@ -2267,6 +2267,9 @@ ipcMain.handle('haptic:perform', (event, pattern) => {
|
||||
});
|
||||
|
||||
app.whenReady().then(async () => {
|
||||
// Declare the Edit accelerators instead of inheriting whatever the implicit default bound, so
|
||||
// Cmd+C acts on the focused window rather than only the first one (ENG-289). macOS only.
|
||||
try { require('./applicationMenu').installApplicationMenu(require('electron')); } catch (_) {}
|
||||
// We made it here, so any prior update swap finished. Drop a stale updating.lock
|
||||
// (the watchdog never deletes it) so a real crash later isn't silently swallowed.
|
||||
try { fs.unlinkSync(CRASH_WATCHDOG_UPDATING_LOCK); } catch (_) {}
|
||||
|
||||
Reference in New Issue
Block a user