From b28a241dbf90e63fffef0837b7ccb766d53056fb Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 13 Feb 2015 11:47:22 +0800 Subject: [PATCH 1/3] Simplify the code to call delegate method --- atom/browser/api/atom_api_menu.cc | 127 ++++++------------------------ atom/browser/api/atom_api_menu.h | 17 ++-- vendor/native_mate | 2 +- 3 files changed, 35 insertions(+), 111 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 6c3bd89c2836..97049093c01b 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -7,6 +7,7 @@ #include "atom/browser/native_window.h" #include "atom/common/native_mate_converters/accelerator_converter.h" #include "atom/common/native_mate_converters/string16_converter.h" +#include "native_mate/callback.h" #include "native_mate/constructor.h" #include "native_mate/dictionary.h" #include "native_mate/object_template_builder.h" @@ -17,30 +18,6 @@ namespace atom { namespace api { -namespace { - -// Call method of delegate object. -v8::Handle CallDelegate(v8::Isolate* isolate, - v8::Handle default_value, - v8::Handle menu, - const char* method, - int command_id) { - v8::Handle delegate = menu->Get( - MATE_STRING_NEW(isolate, "delegate")); - if (!delegate->IsObject()) - return default_value; - - v8::Handle function = v8::Handle::Cast( - delegate->ToObject()->Get(MATE_STRING_NEW(isolate, method))); - if (!function->IsFunction()) - return default_value; - - v8::Handle argv = MATE_INTEGER_NEW(isolate, command_id); - return function->Call(isolate->GetCurrentContext()->Global(), 1, &argv); -} - -} // namespace - Menu::Menu() : model_(new ui::SimpleMenuModel(this)), parent_(NULL) { @@ -49,37 +26,30 @@ Menu::Menu() Menu::~Menu() { } +void Menu::AfterInit(v8::Isolate* isolate) { + mate::Dictionary wrappable(isolate, GetWrapper(isolate)); + mate::Dictionary delegate; + if (!wrappable.Get("delegate", &delegate)) + return; + + delegate.Get("isCommandIdChecked", &is_checked_); + delegate.Get("isCommandIdEnabled", &is_enabled_); + delegate.Get("isCommandIdVisible", &is_visible_); + delegate.Get("getAcceleratorForCommandId", &get_accelerator_); + delegate.Get("executeCommand", &execute_command_); + delegate.Get("menuWillShow", &menu_will_show_); +} + bool Menu::IsCommandIdChecked(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - return CallDelegate(isolate, - MATE_FALSE(isolate), - const_cast(this)->GetWrapper(isolate), - "isCommandIdChecked", - command_id)->BooleanValue(); + return is_checked_.Run(command_id); } bool Menu::IsCommandIdEnabled(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - return CallDelegate(isolate, - MATE_TRUE(isolate), - const_cast(this)->GetWrapper(isolate), - "isCommandIdEnabled", - command_id)->BooleanValue(); + return is_enabled_.Run(command_id); } bool Menu::IsCommandIdVisible(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - return CallDelegate(isolate, - MATE_TRUE(isolate), - const_cast(this)->GetWrapper(isolate), - "isCommandIdVisible", - command_id)->BooleanValue(); + return is_visible_.Run(command_id); } bool Menu::GetAcceleratorForCommandId(int command_id, @@ -87,69 +57,16 @@ bool Menu::GetAcceleratorForCommandId(int command_id, v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::Locker locker(isolate); v8::HandleScope handle_scope(isolate); - v8::Handle shortcut = CallDelegate(isolate, - MATE_UNDEFINED(isolate), - GetWrapper(isolate), - "getAcceleratorForCommandId", - command_id); - return mate::ConvertFromV8(isolate, shortcut, accelerator); -} - -bool Menu::IsItemForCommandIdDynamic(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - return CallDelegate(isolate, - MATE_FALSE(isolate), - const_cast(this)->GetWrapper(isolate), - "isItemForCommandIdDynamic", - command_id)->BooleanValue(); -} - -base::string16 Menu::GetLabelForCommandId(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - v8::Handle result = CallDelegate( - isolate, - MATE_FALSE(isolate), - const_cast(this)->GetWrapper(isolate), - "getLabelForCommandId", - command_id); - base::string16 label; - mate::ConvertFromV8(isolate, result, &label); - return label; -} - -base::string16 Menu::GetSublabelForCommandId(int command_id) const { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - v8::Handle result = CallDelegate( - isolate, - MATE_FALSE(isolate), - const_cast(this)->GetWrapper(isolate), - "getSubLabelForCommandId", - command_id); - base::string16 label; - mate::ConvertFromV8(isolate, result, &label); - return label; + v8::Handle val = get_accelerator_.Run(command_id); + return mate::ConvertFromV8(isolate, val, accelerator); } void Menu::ExecuteCommand(int command_id, int event_flags) { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - CallDelegate(isolate, MATE_FALSE(isolate), GetWrapper(isolate), - "executeCommand", command_id); + execute_command_.Run(command_id); } void Menu::MenuWillShow(ui::SimpleMenuModel* source) { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::Locker locker(isolate); - v8::HandleScope handle_scope(isolate); - CallDelegate(isolate, MATE_FALSE(isolate), GetWrapper(isolate), - "menuWillShow", -1); + menu_will_show_.Run(); } void Menu::AttachToWindow(Window* window) { diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 7b8e68d01a1a..0c8f28ec7723 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -8,6 +8,7 @@ #include #include "atom/browser/api/atom_api_window.h" +#include "base/callback.h" #include "base/memory/scoped_ptr.h" #include "ui/base/models/simple_menu_model.h" #include "native_mate/wrappable.h" @@ -16,8 +17,6 @@ namespace atom { namespace api { -class MenuMac; - class Menu : public mate::Wrappable, public ui::SimpleMenuModel::Delegate { public: @@ -40,6 +39,9 @@ class Menu : public mate::Wrappable, Menu(); virtual ~Menu(); + // mate::Wrappable: + void AfterInit(v8::Isolate* isolate) override; + // ui::SimpleMenuModel::Delegate implementations: bool IsCommandIdChecked(int command_id) const override; bool IsCommandIdEnabled(int command_id) const override; @@ -47,9 +49,6 @@ class Menu : public mate::Wrappable, bool GetAcceleratorForCommandId( int command_id, ui::Accelerator* accelerator) override; - bool IsItemForCommandIdDynamic(int command_id) const override; - base::string16 GetLabelForCommandId(int command_id) const override; - base::string16 GetSublabelForCommandId(int command_id) const override; void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; @@ -85,6 +84,14 @@ class Menu : public mate::Wrappable, bool IsEnabledAt(int index) const; bool IsVisibleAt(int index) const; + // Stored delegate methods. + base::Callback is_checked_; + base::Callback is_enabled_; + base::Callback is_visible_; + base::Callback(int)> get_accelerator_; + base::Callback execute_command_; + base::Callback menu_will_show_; + DISALLOW_COPY_AND_ASSIGN(Menu); }; diff --git a/vendor/native_mate b/vendor/native_mate index 8d537ee2b6da..d0db7bfb586a 160000 --- a/vendor/native_mate +++ b/vendor/native_mate @@ -1 +1 @@ -Subproject commit 8d537ee2b6da29c1aa38928590d4c56700e1c69b +Subproject commit d0db7bfb586afe9f491bd4cb368353d2660ecfe1 From e81baf759ae875ca5c1148dd3c8f69d7705ce339 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 13 Feb 2015 12:11:50 +0800 Subject: [PATCH 2/3] Enable setting icon of menu item --- atom/browser/api/atom_api_menu.cc | 6 ++++++ atom/browser/api/atom_api_menu.h | 6 +++--- atom/browser/api/lib/menu-item.coffee | 3 ++- atom/browser/api/lib/menu.coffee | 2 ++ 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc index 97049093c01b..010d0414d9a9 100644 --- a/atom/browser/api/atom_api_menu.cc +++ b/atom/browser/api/atom_api_menu.cc @@ -6,6 +6,7 @@ #include "atom/browser/native_window.h" #include "atom/common/native_mate_converters/accelerator_converter.h" +#include "atom/common/native_mate_converters/image_converter.h" #include "atom/common/native_mate_converters/string16_converter.h" #include "native_mate/callback.h" #include "native_mate/constructor.h" @@ -103,6 +104,10 @@ void Menu::InsertSubMenuAt(int index, model_->InsertSubMenuAt(index, command_id, label, menu->model_.get()); } +void Menu::SetIcon(int index, const gfx::Image& image) { + model_->SetIcon(index, image); +} + void Menu::SetSublabel(int index, const base::string16& sublabel) { model_->SetSublabel(index, sublabel); } @@ -152,6 +157,7 @@ void Menu::BuildPrototype(v8::Isolate* isolate, .SetMethod("insertRadioItem", &Menu::InsertRadioItemAt) .SetMethod("insertSeparator", &Menu::InsertSeparatorAt) .SetMethod("insertSubMenu", &Menu::InsertSubMenuAt) + .SetMethod("setIcon", &Menu::SetIcon) .SetMethod("setSublabel", &Menu::SetSublabel) .SetMethod("clear", &Menu::Clear) .SetMethod("getIndexOfCommandId", &Menu::GetIndexOfCommandId) diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h index 0c8f28ec7723..830c5d0c6465 100644 --- a/atom/browser/api/atom_api_menu.h +++ b/atom/browser/api/atom_api_menu.h @@ -46,9 +46,8 @@ class Menu : public mate::Wrappable, bool IsCommandIdChecked(int command_id) const override; bool IsCommandIdEnabled(int command_id) const override; bool IsCommandIdVisible(int command_id) const override; - bool GetAcceleratorForCommandId( - int command_id, - ui::Accelerator* accelerator) override; + bool GetAcceleratorForCommandId(int command_id, + ui::Accelerator* accelerator) override; void ExecuteCommand(int command_id, int event_flags) override; void MenuWillShow(ui::SimpleMenuModel* source) override; @@ -73,6 +72,7 @@ class Menu : public mate::Wrappable, int command_id, const base::string16& label, Menu* menu); + void SetIcon(int index, const gfx::Image& image); void SetSublabel(int index, const base::string16& sublabel); void Clear(); int GetIndexOfCommandId(int command_id); diff --git a/atom/browser/api/lib/menu-item.coffee b/atom/browser/api/lib/menu-item.coffee index 30560e4b7ebd..d8e896f8ffa1 100644 --- a/atom/browser/api/lib/menu-item.coffee +++ b/atom/browser/api/lib/menu-item.coffee @@ -9,13 +9,14 @@ class MenuItem constructor: (options) -> Menu = require 'menu' - {click, @selector, @type, @label, @sublabel, @accelerator, @enabled, @visible, @checked, @submenu} = options + {click, @selector, @type, @label, @sublabel, @accelerator, @icon, @enabled, @visible, @checked, @submenu} = options @type = 'submenu' if not @type? and @submenu? throw new Error('Invalid submenu') if @type is 'submenu' and @submenu?.constructor isnt Menu @overrideReadOnlyProperty 'type', 'normal' @overrideReadOnlyProperty 'accelerator' + @overrideReadOnlyProperty 'icon' @overrideReadOnlyProperty 'submenu' @overrideProperty 'label', '' @overrideProperty 'sublabel', '' diff --git a/atom/browser/api/lib/menu.coffee b/atom/browser/api/lib/menu.coffee index bde4e91522dc..b9ee44e23f5d 100644 --- a/atom/browser/api/lib/menu.coffee +++ b/atom/browser/api/lib/menu.coffee @@ -35,6 +35,7 @@ Menu::_init = -> isCommandIdEnabled: (commandId) => @commandsMap[commandId]?.enabled isCommandIdVisible: (commandId) => @commandsMap[commandId]?.visible getAcceleratorForCommandId: (commandId) => @commandsMap[commandId]?.accelerator + getIconForCommandId: (commandId) => @commandsMap[commandId]?.icon executeCommand: (commandId) => @commandsMap[commandId]?.click() menuWillShow: => # Make sure radio groups have at least one menu item seleted. @@ -82,6 +83,7 @@ Menu::insert = (pos, item) -> @insertRadioItem pos, item.commandId, item.label, item.groupId @setSublabel pos, item.sublabel if item.sublabel? + @setIcon pos, item.icon if item.icon? # Make menu accessable to items. item.overrideReadOnlyProperty 'menu', this From ac6c2ce69af30af7561b51039e6cdc99ebf4d0b6 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Fri, 13 Feb 2015 12:12:40 +0800 Subject: [PATCH 3/3] docs: "icon" attribute --- docs/api/menu-item.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/api/menu-item.md b/docs/api/menu-item.md index f1a351211478..ca4cb6b3c586 100644 --- a/docs/api/menu-item.md +++ b/docs/api/menu-item.md @@ -13,6 +13,7 @@ * `label` String * `sublabel` String * `accelerator` [Accelerator](accelerator.md) + * `icon` [NativeImage](native-image.md) * `enabled` Boolean * `visible` Boolean * `checked` Boolean