Fix insertion of radio menu items in wxOSX wxMenu.

Deal correctly with updating the indices when a radio item is inserted into an
existing radio group (which wasn't done previously and resulted in a unit test
failure in MenuTestCase::RadioItems()) and also with inserting the normal
items before an existing radio group as the stored indices were not updated
correctly.

The code is still ugly and it probably wouldn't be a bad idea to reuse
wxMenuRadioItemsData used in wxMSW for similar purposes, but at least the unit
tests pass now.

git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@74548 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

diff --git a/docs/changes.txt b/docs/changes.txt
index 0e824dccee666c6a775c92d44d08ffb742c3e3fd..0eebedff240ff1f2c8b147d227a3acc025173d4d 100644 (file)
--- a/docs/changes.txt
+++ b/docs/changes.txt
@@ -726,6 +726,7 @@ wxOSX/Cocoa:
 - Implement image support in wxNotebook (Malcolm MacLeod).
 - Add support for button mnemonics (joostn).
 - Implemented wxTextCtrl::SetDefaultStyle().
+- Fix insertion and removal of radio items in wxMenu.
 
 
 2.9.4: (released 2012-07-09)

diff --git a/src/osx/menu_osx.cpp b/src/osx/menu_osx.cpp
index e7199755d3d48398878c39c16de7822e118f506a..02be1abc4309e426cfbabbb13572e21405184e85 100644 (file)
--- a/src/osx/menu_osx.cpp
+++ b/src/osx/menu_osx.cpp
@@ -105,6 +105,8 @@ bool wxMenu::DoInsertOrAppend(wxMenuItem *item, size_t pos)
     wxASSERT_MSG( item != NULL, wxT("can't append NULL item to the menu") );
     GetPeer()->InsertOrAppend( item, pos );
 
+    bool check = false;
+
     if ( item->IsSeparator() )
     {
         // nothing to do here
@@ -119,69 +121,131 @@ bool wxMenu::DoInsertOrAppend(wxMenuItem *item, size_t pos)
 
             pSubMenu->DoRearrange();
         }
-        else
+        else if ( item->IsRadio() )
         {
-            if ( item->GetId() == idMenuTitle )
-                item->GetMenu()->Enable( idMenuTitle, false );
-        }
-    }
+            // If a previous or next item is a radio button, add this radio
+            // button to the existing radio group. Otherwise start a new one
+            // for it.
+            wxMenuItemList& items = GetMenuItems();
 
-    // if we're already attached to the menubar, we must update it
-    if ( IsAttached() && GetMenuBar()->IsAttached() )
-        GetMenuBar()->Refresh();
+            size_t const
+                posItem = pos == (size_t)-1 ? items.GetCount() - 1 : pos;
 
-    return true ;
-}
+            wxMenuItemList::compatibility_iterator node = items.Item(posItem);
+            wxCHECK_MSG( node, false, wxS("New item must have been inserted") );
 
-wxMenuItem* wxMenu::DoAppend(wxMenuItem *item)
-{
-    wxCHECK_MSG( item, NULL, wxT("NULL item in wxMenu::DoAppend") );
+            bool foundGroup = false;
+            if ( node->GetPrevious() )
+            {
+                wxMenuItem* const prev = node->GetPrevious()->GetData();
 
-    bool check = false;
+                if ( prev->IsRadio() )
+                {
+                    // This item is in the same group as the preceding one so
+                    // we should use the same starting item, but getting it is
+                    // a bit difficult as we can't query the start radio group
+                    // item for it.
+                    const int groupStart = prev->IsRadioGroupStart()
+                                            ? posItem - 1
+                                            : prev->GetRadioGroupStart();
+                    item->SetRadioGroupStart(groupStart);
+
+                    // We must also account for the new item by incrementing
+                    // the index of the last item in this group.
+                    wxMenuItem* const first = items.Item(groupStart)->GetData();
+                    first->SetRadioGroupEnd(first->GetRadioGroupEnd() + 1);
+
+                    foundGroup = true;
+                }
+            }
 
-    if ( item->IsRadio() )
-    {
-        int count = GetMenuItemCount();
+            if ( !foundGroup && node->GetNext() )
+            {
+                wxMenuItem* const next = node->GetNext()->GetData();
 
-        if ( !count || !(*GetMenuItems().rbegin())->IsRadio() )
-        {
-            // start a new radio group
-            item->SetAsRadioGroupStart();
-            item->SetRadioGroupEnd(count);
+                if ( next->IsRadio() )
+                {
+                    // This item is the new starting item of this group as the
+                    // previous item is not a radio item.
+                    wxASSERT_MSG( next->IsRadioGroupStart(),
+                                  wxS("Where is the start of this group?") );
 
-            // ensure that we have a checked item in the radio group
-            check = true;
-        }
-        else // extend the current radio group
-        {
-            // we need to update its end item
-            wxMenuItem* const last = *GetMenuItems().rbegin();
-            const int groupStart = last->IsRadioGroupStart()
-                                    ? count
-                                    : last->GetRadioGroupStart();
+                    // The index of the last item of the radio group must be
+                    // incremented to account for the new item.
+                    item->SetAsRadioGroupStart();
+                    item->SetRadioGroupEnd(next->GetRadioGroupEnd() + 1);
 
-            item->SetRadioGroupStart(groupStart);
-            wxMenuItemList::compatibility_iterator node = GetMenuItems().Item(groupStart);
+                    // And the previous start item is not one any longer.
+                    next->SetAsRadioGroupStart(false);
+
+                    foundGroup = true;
+                }
+            }
 
-            if ( node )
+            if ( !foundGroup )
             {
-                node->GetData()->SetRadioGroupEnd(count);
+                // start a new radio group
+                item->SetAsRadioGroupStart();
+                item->SetRadioGroupEnd(posItem);
+
+                // ensure that we have a checked item in the radio group
+                check = true;
             }
-            else
+        }
+        else
+        {
+            if ( item->GetId() == idMenuTitle )
+                item->GetMenu()->Enable( idMenuTitle, false );
+        }
+    }
+
+    // We also need to update the indices of radio group start and end we store
+    // in any existing radio items after this item.
+    if ( pos < GetMenuItemCount() - 1 ) // takes into account pos == -1 case
+    {
+        for ( wxMenuItemList::compatibility_iterator
+                node = GetMenuItems().Item(pos + 1);
+                node;
+                node = node->GetNext() )
+        {
+            wxMenuItem* const item = node->GetData();
+            if ( item->IsRadio() )
             {
-                wxFAIL_MSG( wxT("where is the radio group start item?") );
+                if ( item->IsRadioGroupStart() )
+                {
+                    // If the starting item is after the just inserted one,
+                    // then the end one must be after it too and needs to be
+                    // updated.
+                    item->SetRadioGroupEnd(item->GetRadioGroupEnd() + 1);
+                }
+                else // Not the first radio group item.
+                {
+                    // We need to update the start item index only if it is
+                    // after the just inserted item.
+                    const int groupStart = item->GetRadioGroupStart();
+                    if ( (size_t)groupStart > pos )
+                        item->SetRadioGroupStart(groupStart + 1);
+                }
             }
         }
     }
 
-    if ( !wxMenuBase::DoAppend(item) || !DoInsertOrAppend(item) )
-        return NULL;
+    // if we're already attached to the menubar, we must update it
+    if ( IsAttached() && GetMenuBar()->IsAttached() )
+        GetMenuBar()->Refresh();
 
     if ( check )
-        // check the item initially
         item->Check(true);
 
-    return item;
+    return true ;
+}
+
+wxMenuItem* wxMenu::DoAppend(wxMenuItem *item)
+{
+    if (wxMenuBase::DoAppend(item) && DoInsertOrAppend(item) )
+        return item;
+
+    return NULL;
 }
 
 wxMenuItem* wxMenu::DoInsert(size_t pos, wxMenuItem *item)