From f451ce6416b5075e515d409b7c40e5f597f5bfcf Mon Sep 17 00:00:00 2001 From: "trop[bot]" Date: Mon, 26 Nov 2018 13:15:47 -0800 Subject: [PATCH] feat: add registerAccelerator flag to allow menu items to optionally skip accelerator registration (backport: 4-0-x) (#15840) * feat: add registerAccelerator flag to allow menu items to skip registration * docs: add docs for registerAccelerator * docs: re-add accidentally removed line --- atom/browser/api/atom_api_menu.cc | 8 ++++++++ atom/browser/api/atom_api_menu.h | 2 ++ atom/browser/ui/accelerator_util.cc | 8 +++++--- atom/browser/ui/atom_menu_model.cc | 8 ++++++++ atom/browser/ui/atom_menu_model.h | 4 ++++ docs/api/menu-item.md | 2 ++ lib/browser/api/menu-item-roles.js | 16 ++++++++++++---- lib/browser/api/menu-item.js | 1 + lib/browser/api/menu.js | 1 + 9 files changed, 43 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index c378e17f87297..9817fc834d411 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -41,6 +41,8 @@ void Menu::AfterInit(v8::Isolate* isolate) { delegate.Get("isCommandIdEnabled", &is_enabled_); delegate.Get("isCommandIdVisible", &is_visible_); delegate.Get("getAcceleratorForCommandId", &get_accelerator_); + delegate.Get("shouldRegisterAcceleratorForCommandId", + &should_register_accelerator_); delegate.Get("executeCommand", &execute_command_); delegate.Get("menuWillShow", &menu_will_show_); } @@ -74,6 +76,12 @@ bool Menu::GetAcceleratorForCommandIdWithParams( return mate::ConvertFromV8(isolate(), val, accelerator); } +bool Menu::ShouldRegisterAcceleratorForCommandId(int command_id) const { + v8::Locker locker(isolate()); + v8::HandleScope handle_scope(isolate()); + return should_register_accelerator_.Run(GetWrapper(), command_id); +} + void Menu::ExecuteCommand(int command_id, int flags) { v8::Locker locker(isolate()); v8::HandleScope handle_scope(isolate()); diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 499263bf492b7..c6887ad22b5b5 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -51,6 +51,7 @@ class Menu : public mate::TrackableObject, int command_id, bool use_default_accelerator, ui::Accelerator* accelerator) const override; + bool ShouldRegisterAcceleratorForCommandId(int command_id) const override; void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; @@ -102,6 +103,7 @@ class Menu : public mate::TrackableObject, base::Callback, int)> is_visible_; base::Callback(v8::Local, int, bool)> get_accelerator_; + base::Callback, int)> should_register_accelerator_; base::Callback, v8::Local, int)> execute_command_; base::Callback)> menu_will_show_; diff --git a/atom/browser/ui/accelerator_util.cc b/atom/browser/ui/accelerator_util.cc index c0d14c01e8485..21d08dfba141e 100644 --- a/atom/browser/ui/accelerator_util.cc +++ b/atom/browser/ui/accelerator_util.cc @@ -77,9 +77,11 @@ void GenerateAcceleratorTable(AcceleratorTable* table, GenerateAcceleratorTable(table, submodel); } else { ui::Accelerator accelerator; - if (model->GetAcceleratorAtWithParams(i, true, &accelerator)) { - MenuItem item = {i, model}; - (*table)[accelerator] = item; + if (model->ShouldRegisterAcceleratorAt(i)) { + if (model->GetAcceleratorAtWithParams(i, true, &accelerator)) { + MenuItem item = {i, model}; + (*table)[accelerator] = item; + } } } } diff --git a/atom/browser/ui/atom_menu_model.cc b/atom/browser/ui/atom_menu_model.cc index eef1c2b995750..d50b85acb7db7 100644 --- a/atom/browser/ui/atom_menu_model.cc +++ b/atom/browser/ui/atom_menu_model.cc @@ -43,6 +43,14 @@ bool AtomMenuModel::GetAcceleratorAtWithParams( return false; } +bool AtomMenuModel::ShouldRegisterAcceleratorAt(int index) const { + if (delegate_) { + return delegate_->ShouldRegisterAcceleratorForCommandId( + GetCommandIdAt(index)); + } + return true; +} + void AtomMenuModel::MenuWillClose() { ui::SimpleMenuModel::MenuWillClose(); for (Observer& observer : observers_) { diff --git a/atom/browser/ui/atom_menu_model.h b/atom/browser/ui/atom_menu_model.h index efdd8ec9d8834..6b3e2e57a373b 100644 --- a/atom/browser/ui/atom_menu_model.h +++ b/atom/browser/ui/atom_menu_model.h @@ -23,6 +23,9 @@ class AtomMenuModel : public ui::SimpleMenuModel { bool use_default_accelerator, ui::Accelerator* accelerator) const = 0; + virtual bool ShouldRegisterAcceleratorForCommandId( + int command_id) const = 0; + private: // ui::SimpleMenuModel::Delegate: bool GetAcceleratorForCommandId( @@ -52,6 +55,7 @@ class AtomMenuModel : public ui::SimpleMenuModel { bool GetAcceleratorAtWithParams(int index, bool use_default_accelerator, ui::Accelerator* accelerator) const; + bool ShouldRegisterAcceleratorAt(int index) const; // ui::SimpleMenuModel: void MenuWillClose() override; diff --git a/docs/api/menu-item.md b/docs/api/menu-item.md index ad2a0a3b048ab..ead1abd9b1159 100644 --- a/docs/api/menu-item.md +++ b/docs/api/menu-item.md @@ -27,6 +27,8 @@ See [`Menu`](menu.md) for examples. * `visible` Boolean (optional) - If false, the menu item will be entirely hidden. * `checked` Boolean (optional) - Should only be specified for `checkbox` or `radio` type menu items. + * `registerAccelerator` Boolean (optional) - If false, the accelerator won't be registered + with the system, but it will still be displayed. Defaults to true. * `submenu` (MenuItemConstructorOptions[] | [Menu](menu.md)) (optional) - Should be specified for `submenu` type menu items. If `submenu` is specified, the `type: 'submenu'` can be omitted. If the value is not a [`Menu`](menu.md) then it will be automatically converted to one using diff --git a/lib/browser/api/menu-item-roles.js b/lib/browser/api/menu-item-roles.js index 4055d2fcbef32..61c6226f630fa 100644 --- a/lib/browser/api/menu-item-roles.js +++ b/lib/browser/api/menu-item-roles.js @@ -16,12 +16,14 @@ const roles = { copy: { label: 'Copy', accelerator: 'CommandOrControl+C', - webContentsMethod: 'copy' + webContentsMethod: 'copy', + registerAccelerator: false }, cut: { label: 'Cut', accelerator: 'CommandOrControl+X', - webContentsMethod: 'cut' + webContentsMethod: 'cut', + registerAccelerator: false }, delete: { label: 'Delete', @@ -59,12 +61,14 @@ const roles = { paste: { label: 'Paste', accelerator: 'CommandOrControl+V', - webContentsMethod: 'paste' + webContentsMethod: 'paste', + registerAccelerator: false }, pasteandmatchstyle: { label: 'Paste and Match Style', accelerator: 'Shift+CommandOrControl+V', - webContentsMethod: 'pasteAndMatchStyle' + webContentsMethod: 'pasteAndMatchStyle', + registerAccelerator: false }, quit: { get label () { @@ -243,6 +247,10 @@ exports.getDefaultAccelerator = (role) => { if (roles.hasOwnProperty(role)) return roles[role].accelerator } +exports.shouldRegisterAccelerator = (role) => { + return roles.hasOwnProperty(role) ? roles[role].registerAccelerator : true +} + exports.getDefaultSubmenu = (role) => { if (!roles.hasOwnProperty(role)) return diff --git a/lib/browser/api/menu-item.js b/lib/browser/api/menu-item.js index 699a63e1da38a..33388cbb6d408 100644 --- a/lib/browser/api/menu-item.js +++ b/lib/browser/api/menu-item.js @@ -36,6 +36,7 @@ const MenuItem = function (options) { this.overrideProperty('enabled', true) this.overrideProperty('visible', true) this.overrideProperty('checked', false) + this.overrideProperty('registerAccelerator', roles.shouldRegisterAccelerator(this.role)) if (!MenuItem.types.includes(this.type)) { throw new Error(`Unknown menu item type: ${this.type}`) diff --git a/lib/browser/api/menu.js b/lib/browser/api/menu.js index 691b624f936ed..14fcaba0a2dd7 100644 --- a/lib/browser/api/menu.js +++ b/lib/browser/api/menu.js @@ -24,6 +24,7 @@ const delegate = { if (command.accelerator != null) return command.accelerator if (useDefaultAccelerator) return command.getDefaultRoleAccelerator() }, + shouldRegisterAcceleratorForCommandId: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].registerAccelerator : undefined, executeCommand: (menu, event, id) => { const command = menu.commandsMap[id] if (!command) return