Radio menu items should have at least one item checked.

This is to force all platforms to match the same behavior in GTK+.

diff --git a/atom/browser/api/atom_api_menu.cc b/atom/browser/api/atom_api_menu.cc
index 656205b..1971b3a 100644 (file)
--- a/atom/browser/api/atom_api_menu.cc
+++ b/atom/browser/api/atom_api_menu.cc
@@ -135,6 +135,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);
@@ -226,7 +232,7 @@ void Menu::BuildPrototype(v8::Isolate* isolate,
 #if defined(OS_WIN) || defined(TOOLKIT_GTK)
       .SetMethod("attachToWindow", &Menu::AttachToWindow)
 #endif
-      .SetMethod("popup", &Menu::Popup);
+      .SetMethod("_popup", &Menu::Popup);
 }
 
 }  // namespace api

diff --git a/atom/browser/api/atom_api_menu.h b/atom/browser/api/atom_api_menu.h
index c4bb8c2..a9e9e92 100644 (file)
--- a/atom/browser/api/atom_api_menu.h
+++ b/atom/browser/api/atom_api_menu.h
@@ -49,6 +49,7 @@ 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;
 

diff --git a/atom/browser/api/lib/menu-item.coffee b/atom/browser/api/lib/menu-item.coffee
index 834eddf..11d87f5 100644 (file)
--- a/atom/browser/api/lib/menu-item.coffee
+++ b/atom/browser/api/lib/menu-item.coffee
@@ -22,7 +22,7 @@ class MenuItem
     @checked = @checked ? false
     @submenu = @submenu ? null
 
-    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 = =>

diff --git a/atom/browser/api/lib/menu.coffee b/atom/browser/api/lib/menu.coffee
index 877a459..acc6324 100644 (file)
--- a/atom/browser/api/lib/menu.coffee
+++ b/atom/browser/api/lib/menu.coffee
@@ -26,11 +26,9 @@ generateGroupId = (items, pos) ->
 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
@@ -55,6 +53,14 @@ Menu::insert = (pos, item) ->
           if activeItem.type in ['checkbox', 'radio']
             activeItem.checked = !activeItem.checked
           activeItem.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

diff --git a/spec/api-menu-spec.coffee b/spec/api-menu-spec.coffee
index 545a85a..5ac7c14 100644 (file)
--- a/spec/api-menu-spec.coffee
+++ b/spec/api-menu-spec.coffee
@@ -43,6 +43,16 @@ describe 'menu module', ->
       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]