Merge pull request #354 from atom/automatic-group-id

Improve radio and checkbox menu items support

atom/browser/api/atom_api_menu.cc

@@ -44,7 +44,9 @@ v8::Handle<v8::Value> CallDelegate(v8::Handle<v8::Value> default_value,
 
 }  // namespace
 
-Menu::Menu() : model_(new ui::SimpleMenuModel(this)) {
+Menu::Menu()
+    : model_(new ui::SimpleMenuModel(this)),
+      parent_(NULL) {
 }
 
 Menu::~Menu() {
@@ -135,6 +137,12 @@ void Menu::ExecuteCommand(int command_id, int event_flags) {
                command_id);
 }
 
+void Menu::MenuWillShow(ui::SimpleMenuModel* source) {
+  v8::Locker locker(node_isolate);
+  v8::HandleScope handle_scope(node_isolate);
+  CallDelegate(v8::False(), GetWrapper(node_isolate), "menuWillShow", -1);
+}
+
 void Menu::InsertItemAt(
     int index, int command_id, const base::string16& label) {
   model_->InsertItemAt(index, command_id, label);
@@ -161,6 +169,7 @@ void Menu::InsertSubMenuAt(int index,
                            int command_id,
                            const base::string16& label,
                            Menu* menu) {
+  menu->parent_ = this;
   model_->InsertSubMenuAt(index, command_id, label, menu->model_.get());
 }
 
@@ -224,9 +233,12 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("isEnabledAt", &Menu::IsEnabledAt)
       .SetMethod("isVisibleAt", &Menu::IsVisibleAt)
 #if defined(OS_WIN) || defined(TOOLKIT_GTK)
-      .SetMethod("attachToWindow", &Menu::AttachToWindow)
+      .SetMethod("_attachToWindow", &Menu::AttachToWindow)
 #endif
-      .SetMethod("popup", &Menu::Popup);
+#if defined(OS_WIN)
+      .SetMethod("_updateStates", &Menu::UpdateStates)
+#endif
+      .SetMethod("_popup", &Menu::Popup);
 }
 
 }  // namespace api

atom/browser/api/atom_api_menu.h

@@ -49,10 +49,12 @@ class Menu : public mate::Wrappable,
   virtual string16 GetLabelForCommandId(int command_id) const OVERRIDE;
   virtual string16 GetSublabelForCommandId(int command_id) const OVERRIDE;
   virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE;
+  virtual void MenuWillShow(ui::SimpleMenuModel* source) OVERRIDE;
 
   virtual void Popup(Window* window) = 0;
 
   scoped_ptr<ui::SimpleMenuModel> model_;
+  Menu* parent_;
 
  private:
   void InsertItemAt(int index, int command_id, const base::string16& label);
@@ -79,8 +81,12 @@ class Menu : public mate::Wrappable,
   bool IsEnabledAt(int index) const;
   bool IsVisibleAt(int index) const;
 
+#if defined(OS_WIN)
+  virtual void UpdateStates() = 0;
+#endif
+
 #if defined(OS_WIN) || defined(TOOLKIT_GTK)
-  void AttachToWindow(Window* window);
+  virtual void AttachToWindow(Window* window) = 0;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(Menu);

atom/browser/api/atom_api_menu_gtk.cc

@@ -37,7 +37,7 @@ void MenuGtk::Popup(Window* window) {
   menu_gtk_->PopupAsContext(point, triggering_event_time);
 }
 
-void Menu::AttachToWindow(Window* window) {
+void MenuGtk::AttachToWindow(Window* window) {
   static_cast<NativeWindowGtk*>(window->window())->SetMenu(model_.get());
 }
 

atom/browser/api/atom_api_menu_gtk.h

@@ -19,6 +19,7 @@ class MenuGtk : public Menu,
 
  protected:
   virtual void Popup(Window* window) OVERRIDE;
+  virtual void AttachToWindow(Window* window) OVERRIDE;
 
  private:
   scoped_ptr<::MenuGtk> menu_gtk_;

atom/browser/api/atom_api_menu_win.cc

@@ -15,17 +15,28 @@ namespace atom {
 
 namespace api {
 
-MenuWin::MenuWin() {
+MenuWin::MenuWin() : menu_(NULL) {
 }
 
 void MenuWin::Popup(Window* window) {
   gfx::Point cursor = gfx::Screen::GetNativeScreen()->GetCursorScreenPoint();
-  menu_.reset(new atom::Menu2(model_.get()));
+  popup_menu_.reset(new atom::Menu2(model_.get()));
+  menu_ = popup_menu_.get();
   menu_->RunContextMenuAt(cursor);
 }
 
-void Menu::AttachToWindow(Window* window) {
-  static_cast<NativeWindowWin*>(window->window())->SetMenu(model_.get());
+void MenuWin::UpdateStates() {
+  MenuWin* top = this;
+  while (top->parent_)
+    top = static_cast<MenuWin*>(top->parent_);
+  if (top->menu_)
+    top->menu_->UpdateStates();
+}
+
+void MenuWin::AttachToWindow(Window* window) {
+  NativeWindowWin* nw = static_cast<NativeWindowWin*>(window->window());
+  nw->SetMenu(model_.get());
+  menu_ = nw->menu();
 }
 
 // static

atom/browser/api/atom_api_menu_win.h

@@ -19,9 +19,12 @@ class MenuWin : public Menu {
 
  protected:
   virtual void Popup(Window* window) OVERRIDE;
+  virtual void UpdateStates() OVERRIDE;
+  virtual void AttachToWindow(Window* window) OVERRIDE;
 
  private:
-  scoped_ptr<atom::Menu2> menu_;
+  atom::Menu2* menu_;  // Weak ref, could be window menu or popup menu.
+  scoped_ptr<atom::Menu2> popup_menu_;
 
   DISALLOW_COPY_AND_ASSIGN(MenuWin);
 };

atom/browser/api/lib/menu-item.coffee

@@ -1,4 +1,5 @@
 BrowserWindow = require 'browser-window'
+v8Util = process.atomBinding 'v8_util'
 
 nextCommandId = 0
 
@@ -8,28 +9,50 @@ class MenuItem
   constructor: (options) ->
     Menu = require 'menu'
 
-    {click, @selector, @type, @label, @sublabel, @accelerator, @enabled, @visible, @checked, @groupId, @submenu} = options
+    {click, @selector, @type, @label, @sublabel, @accelerator, @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
 
-    @type = @type ? 'normal'
-    @label = @label ? ''
-    @sublabel = @sublabel ? ''
-    @accelerator = @accelerator ? null
-    @enabled = @enabled ? true
-    @visible = @visible ? true
-    @checked = @checked ? false
-    @groupId = @groupId ? null
-    @submenu = @submenu ? null
+    @overrideReadOnlyProperty 'type', 'normal'
+    @overrideReadOnlyProperty 'accelerator'
+    @overrideReadOnlyProperty 'submenu'
+    @overrideProperty 'label', ''
+    @overrideProperty 'sublabel', ''
+    @overrideProperty 'enabled', true
+    @overrideProperty 'visible', true
+    @overrideProperty 'checked', false
 
-    throw new Error('Unknown menu type') if MenuItem.types.indexOf(@type) is -1
+    throw new Error("Unknown menu type #{@type}") if MenuItem.types.indexOf(@type) is -1
 
     @commandId = ++nextCommandId
     @click = =>
+      # Manually flip the checked flags when clicked.
+      @checked = !@checked if @type in ['checkbox', 'radio']
+
       if typeof click is 'function'
         click this, BrowserWindow.getFocusedWindow()
       else if typeof @selector is 'string'
         Menu.sendActionToFirstResponder @selector
 
+  overrideProperty: (name, defaultValue=null) ->
+    this[name] ?= defaultValue
+
+    # Update states when property is changed on Windows.
+    return unless process.platform is 'win32'
+    v8Util.setHiddenValue this, name, this[name]
+    Object.defineProperty this, name,
+      enumerable: true
+      get: => v8Util.getHiddenValue this, name
+      set: (val) =>
+        v8Util.setHiddenValue this, name, val
+        @menu?._updateStates()
+
+  overrideReadOnlyProperty: (name, defaultValue=null) ->
+    this[name] ?= defaultValue
+    Object.defineProperty this, name,
+      enumerable: true
+      writable: false
+      value: this[name]
+
 module.exports = MenuItem

atom/browser/api/lib/menu.coffee

@@ -1,17 +1,34 @@
 BrowserWindow = require 'browser-window'
 EventEmitter = require('events').EventEmitter
 MenuItem = require 'menu-item'
+v8Util = process.atomBinding 'v8_util'
 
 bindings = process.atomBinding 'menu'
 
+# Automatically generated radio menu item's group id.
+nextGroupId = 0
+
+# Search between seperators to find a radio menu item and return its group id,
+# otherwise generate a group id.
+generateGroupId = (items, pos) ->
+  if pos > 0
+    for i in [pos - 1..0]
+      item = items[i]
+      return item.groupId if item.type is 'radio'
+      break if item.type is 'separator'
+  else if pos < items.length
+    for i in [pos..items.length - 1]
+      item = items[i]
+      return item.groupId if item.type is 'radio'
+      break if item.type is 'separator'
+  ++nextGroupId
+
 Menu = bindings.Menu
 Menu::__proto__ = EventEmitter.prototype
 
-popup = Menu::popup
 Menu::popup = (window) ->
   throw new TypeError('Invalid window') unless window?.constructor is BrowserWindow
-
-  popup.call this, window
+  @_popup window
 
 Menu::append = (item) ->
   @insert @getItemCount(), item
@@ -19,15 +36,7 @@ Menu::append = (item) ->
 Menu::insert = (pos, item) ->
   throw new TypeError('Invalid item') unless item?.constructor is MenuItem
 
-  switch item.type
-    when 'normal' then @insertItem pos, item.commandId, item.label
-    when 'checkbox' then @insertCheckItem pos, item.commandId, item.label
-    when 'radio' then @insertRadioItem pos, item.commandId, item.label, item.groupId
-    when 'separator' then @insertSeparator pos
-    when 'submenu' then @insertSubMenu pos, item.commandId, item.label, item.submenu
-
-  @setSublabel pos, item.sublabel if item.sublabel?
-
+  # Create delegate.
   unless @delegate?
     @commandsMap = {}
     @groupsMap = {}
@@ -37,23 +46,59 @@ Menu::insert = (pos, item) ->
       isCommandIdEnabled: (commandId) => @commandsMap[commandId]?.enabled
       isCommandIdVisible: (commandId) => @commandsMap[commandId]?.visible
       getAcceleratorForCommandId: (commandId) => @commandsMap[commandId]?.accelerator
-      executeCommand: (commandId) =>
-        activeItem = @commandsMap[commandId]
-        if activeItem?
-          switch activeItem.type
-            when 'checkbox'
-              activeItem.checked = !activeItem.checked
-            when 'radio'
-              for item in @groupsMap[activeItem.groupId]
-                item.checked = false
-              activeItem.checked = true
-          activeItem.click()
+      executeCommand: (commandId) => @commandsMap[commandId]?.click()
+      menuWillShow: =>
+        # Make sure radio groups have at least one menu item seleted.
+        for id, group of @groupsMap
+          checked = false
+          for radioItem in group when radioItem.checked
+            checked = true
+            break
+          v8Util.setHiddenValue group[0], 'checked', true unless checked
+
+  switch item.type
+    when 'normal' then @insertItem pos, item.commandId, item.label
+    when 'checkbox' then @insertCheckItem pos, item.commandId, item.label
+    when 'separator' then @insertSeparator pos
+    when 'submenu' then @insertSubMenu pos, item.commandId, item.label, item.submenu
+    when 'radio'
+      # Grouping radio menu items.
+      item.overrideReadOnlyProperty 'groupId', generateGroupId(@items, pos)
+      @groupsMap[item.groupId] ?= []
+      @groupsMap[item.groupId].push item
+
+      # Setting a radio menu item should flip other items in the group.
+      v8Util.setHiddenValue item, 'checked', item.checked
+      Object.defineProperty item, 'checked',
+        enumerable: true
+        get: -> v8Util.getHiddenValue item, 'checked'
+        set: (val) =>
+          for otherItem in @groupsMap[item.groupId] when otherItem isnt item
+            v8Util.setHiddenValue otherItem, 'checked', false
+          v8Util.setHiddenValue item, 'checked', true
+
+          # Update states when clicked on Windows.
+          @_updateStates() if process.platform is 'win32'
+
+      @insertRadioItem pos, item.commandId, item.label, item.groupId
+
+  @setSublabel pos, item.sublabel if item.sublabel?
+
+  # Make menu accessable to items.
+  item.overrideReadOnlyProperty 'menu', this
+
+  # Remember the items.
   @items.splice pos, 0, item
   @commandsMap[item.commandId] = item
 
-  if item.groupId?
-    @groupsMap[item.groupId] ?= []
-    @groupsMap[item.groupId].push item
+Menu::attachToWindow = (window) ->
+  @_callMenuWillShow() if process.platform is 'win32'
+  @_attachToWindow window
+
+# Force menuWillShow to be called
+Menu::_callMenuWillShow = ->
+  @delegate?.menuWillShow()
+  item.submenu._callMenuWillShow() for item in @items when item.submenu?
 
 applicationMenu = null
 Menu.setApplicationMenu = (menu) ->
@@ -61,6 +106,7 @@ Menu.setApplicationMenu = (menu) ->
   applicationMenu = menu  # Keep a reference.
 
   if process.platform is 'darwin'
+    menu._callMenuWillShow()
     bindings.setApplicationMenu menu
   else
     windows = BrowserWindow.getAllWindows()

atom/browser/native_window_win.cc

@@ -415,6 +415,7 @@ void NativeWindowWin::OnMenuCommand(int position, HMENU menu) {
 
 void NativeWindowWin::SetMenu(ui::MenuModel* menu_model) {
   menu_.reset(new atom::Menu2(menu_model, true));
+  menu_->UpdateStates();
   ::SetMenu(GetNativeWindow(), menu_->GetNativeMenu());
   RegisterAccelerators();
 

atom/browser/native_window_win.h

@@ -81,6 +81,7 @@ class NativeWindowWin : public NativeWindow,
   void SetMenu(ui::MenuModel* menu_model);
 
   views::Widget* window() const { return window_.get(); }
+  atom::Menu2* menu() const { return menu_.get(); }
   SkRegion* draggable_region() { return draggable_region_.get(); }
 
  protected:

docs/api/menu-item.md

@@ -17,7 +17,6 @@
   * `enabled` Boolean
   * `visible` Boolean
   * `checked` Boolean
-  * `groupId` Integer - Should be specified for `radio` type menu item
   * `submenu` Menu - Should be specified for `submenu` type menu item, when
      it's specified the `type: 'submenu'` can be omitted for the menu item
 

spec/api-menu-spec.coffee

@@ -35,3 +35,57 @@ describe 'menu module', ->
           done()
       ]
       menu.delegate.executeCommand menu.items[0].commandId
+
+  describe 'MenuItem with checked property', ->
+    it 'clicking an checkbox item should flip the checked property', ->
+      menu = Menu.buildFromTemplate [ label: 'text', type: 'checkbox' ]
+      assert.equal menu.items[0].checked, false
+      menu.delegate.executeCommand menu.items[0].commandId
+      assert.equal menu.items[0].checked, true
+
+    it 'clicking an radio item should always make checked property true', ->
+      menu = Menu.buildFromTemplate [ label: 'text', type: 'radio' ]
+      menu.delegate.executeCommand menu.items[0].commandId
+      assert.equal menu.items[0].checked, true
+      menu.delegate.executeCommand menu.items[0].commandId
+      assert.equal menu.items[0].checked, true
+
+    it 'at least have one item checked in each group', ->
+      template = []
+      template.push label: "#{i}", type: 'radio' for i in [0..10]
+      template.push type: 'separator'
+      template.push label: "#{i}", type: 'radio' for i in [12..20]
+      menu = Menu.buildFromTemplate template
+      menu.delegate.menuWillShow()
+      assert.equal menu.items[0].checked, true
+      assert.equal menu.items[12].checked, true
+
+    it 'should assign groupId automatically', ->
+      template = []
+      template.push label: "#{i}", type: 'radio' for i in [0..10]
+      template.push type: 'separator'
+      template.push label: "#{i}", type: 'radio' for i in [12..20]
+      menu = Menu.buildFromTemplate template
+      groupId = menu.items[0].groupId
+      assert.equal menu.items[i].groupId, groupId for i in [0..10]
+      assert.equal menu.items[i].groupId, groupId + 1 for i in [12..20]
+
+    it "setting 'checked' should flip other items' 'checked' property", ->
+      template = []
+      template.push label: "#{i}", type: 'radio' for i in [0..10]
+      template.push type: 'separator'
+      template.push label: "#{i}", type: 'radio' for i in [12..20]
+      menu = Menu.buildFromTemplate template
+      assert.equal menu.items[i].checked, false for i in [0..10]
+      menu.items[0].checked = true
+      assert.equal menu.items[0].checked, true
+      assert.equal menu.items[i].checked, false for i in [1..10]
+      menu.items[10].checked = true
+      assert.equal menu.items[10].checked, true
+      assert.equal menu.items[i].checked, false for i in [0..9]
+      assert.equal menu.items[i].checked, false for i in [12..20]
+      menu.items[12].checked = true
+      assert.equal menu.items[10].checked, true
+      assert.equal menu.items[i].checked, false for i in [0..9]
+      assert.equal menu.items[12].checked, true
+      assert.equal menu.items[i].checked, false for i in [13..20]

spec/static/main.js

@@ -32,10 +32,13 @@ ipc.on('echo', function(event, msg) {
   event.returnValue = msg;
 });
 
-process.on('uncaughtException', function(error) {
-  console.log(error);
-  window.openDevTools();
-});
+if (process.argv[1] == '--ci') {
+  process.removeAllListeners('uncaughtException');
+  process.on('uncaughtException', function(error) {
+    console.error(error, error.stack);
+    process.exit(1);
+  });
+}
 
 app.on('window-all-closed', function() {
   app.quit();
@@ -124,10 +127,6 @@ app.on('ready', function() {
         },
       ]
     },
-    {
-      label: 'Help',
-      submenu: [],
-    }
   ];
 
   var menu = Menu.buildFromTemplate(template);