]> git.saurik.com Git - wxWidgets.git/commitdiff
Rewrote wxMSW radio menu items code to support not only appending them.
authorVadim Zeitlin <vadim@wxwidgets.org>
Tue, 10 May 2011 08:50:38 +0000 (08:50 +0000)
committerVadim Zeitlin <vadim@wxwidgets.org>
Tue, 10 May 2011 08:50:38 +0000 (08:50 +0000)
Previously the radio menu items could only be appended to a menu in wxMSW,
inserting them (either in an existing radio group or to start a new one) not
only didn't work but could even result in crashes because invalid iterators in
the menu items list could be used.

Fix this by storing the ranges of all radio groups in wxMenu itself instead of
storing the information about the radio group an item belongs to in the item
itself and by updating this data whenever a new radio item is inserted. Also
get rid of the notion of "current radio group" in wxMenu which doesn't really
make any sense.

Finally add a unit test checking that inserting radio items works as expected.

Closes #13200.

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

include/wx/msw/menu.h
include/wx/msw/menuitem.h
src/msw/menu.cpp
src/msw/menuitem.cpp
tests/menu/menu.cpp

index d80e85cca199ec093f25bb51f3ff6d478404dc58..4c5e95f5ad2fece2044ea313e3c04e5f1adb351f 100644 (file)
@@ -25,6 +25,7 @@ class WXDLLIMPEXP_FWD_CORE wxFrame;
 class WXDLLIMPEXP_FWD_CORE wxToolBar;
 #endif
 
+class wxMenuRadioItemsData;
 
 // Not using a combined wxToolBar/wxMenuBar? then use
 // a commandbar in WinCE .NET to implement the
@@ -63,13 +64,16 @@ public:
     // implementation only from now on
     // -------------------------------
 
-    virtual void Attach(wxMenuBarBase *menubar);
-
     bool MSWCommand(WXUINT param, WXWORD id);
 
     // get the native menu handle
     WXHMENU GetHMenu() const { return m_hMenu; }
 
+    // Return the start and end position of the radio group to which the item
+    // at the given position belongs. Returns false if there is no radio group
+    // containing this position.
+    bool MSWGetRadioGroupRange(int pos, int *start, int *end) const;
+
 #if wxUSE_ACCEL
     // called by wxMenuBar to build its accel table from the accels of all menus
     bool HasAccels() const { return !m_accels.empty(); }
@@ -122,15 +126,17 @@ private:
     // common part of Append/Insert (behaves as Append is pos == (size_t)-1)
     bool DoInsertOrAppend(wxMenuItem *item, size_t pos = (size_t)-1);
 
-    // terminate the current radio group, if any
-    void EndRadioGroup();
+
+    // This variable contains the description of the radio item groups and
+    // allows to find whether an item at the given position is part of the
+    // group and also where its group starts and ends.
+    //
+    // It is initially NULL and only allocated if we have any radio items.
+    wxMenuRadioItemsData *m_radioData;
 
     // if true, insert a breal before appending the next item
     bool m_doBreak;
 
-    // the position of the first item in the current radio group or -1
-    int m_startRadioGroup;
-
     // the menu handle of this menu
     WXHMENU m_hMenu;
 
index 21be80c38f3e81fbcfc3395b3f48ae3f5a7690c6..1fe684971bf3ef757afaa007fa77fb3554003a43 100644 (file)
@@ -61,11 +61,6 @@ public:
     // Win32 API
     WXWPARAM GetMSWId() const;
 
-    // mark item as belonging to the given radio group
-    void SetAsRadioGroupStart();
-    void SetRadioGroupStart(int start);
-    void SetRadioGroupEnd(int end);
-
 #if WXWIN_COMPATIBILITY_2_8
     // compatibility only, don't use in new code
     wxDEPRECATED(
@@ -130,18 +125,6 @@ private:
     // common part of all ctors
     void Init();
 
-    // the positions of the first and last items of the radio group this item
-    // belongs to or -1: start is the radio group start and is valid for all
-    // but first radio group items (m_isRadioGroupStart == false), end is valid
-    // only for the first one
-    union
-    {
-        int start;
-        int end;
-    } m_radioGroup;
-
-    // does this item start a radio group?
-    bool m_isRadioGroupStart;
 
 #if wxUSE_OWNER_DRAWN
     // item bitmaps
index 3901d885fe561e4803d2dcbded431f3e40318c87..c1accd90e2f30272b0801a7bd762f2fcfbc1f1c2 100644 (file)
@@ -41,6 +41,7 @@
 #endif
 
 #include "wx/scopedarray.h"
+#include "wx/vector.h"
 
 #include "wx/msw/private.h"
 #include "wx/msw/wrapcctl.h" // include <commctrl.h> "properly"
 static const int idMenuTitle = wxID_NONE;
 
 // ----------------------------------------------------------------------------
-// private functions
+// private helper classes and functions
 // ----------------------------------------------------------------------------
 
+// Contains the data about the radio items groups in the given menu.
+class wxMenuRadioItemsData
+{
+public:
+    wxMenuRadioItemsData() { }
+
+    // Default copy ctor, assignment operator and dtor are all ok.
+
+    // Find the start and end of the group containing the given position or
+    // return false if it's not inside any range.
+    bool GetGroupRange(int pos, int *start, int *end) const
+    {
+        // We use a simple linear search here because there are not that many
+        // items in a menu and hence even fewer radio items ranges anyhow, so
+        // normally there is no need to do anything fancy (like keeping the
+        // array sorted and using binary search).
+        for ( Ranges::const_iterator it = m_ranges.begin();
+              it != m_ranges.end();
+              ++it )
+        {
+            const Range& r = *it;
+
+            if ( r.start <= pos && pos <= r.end )
+            {
+                if ( start )
+                    *start = r.start;
+                if ( end )
+                    *end = r.end;
+
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    // Take into account the new radio item about to be added at the given
+    // position.
+    //
+    // Returns true if this item starts a new radio group, false if it extends
+    // an existing one.
+    bool UpdateOnInsert(int pos)
+    {
+        bool inExistingGroup = false;
+
+        for ( Ranges::iterator it = m_ranges.begin();
+              it != m_ranges.end();
+              ++it )
+        {
+            Range& r = *it;
+
+            if ( pos < r.start )
+            {
+                // Item is inserted before this range, update its indices.
+                r.start++;
+                r.end++;
+            }
+            else if ( pos <= r.end + 1 )
+            {
+                // Item is inserted in the middle of this range or immediately
+                // after it in which case it extends this range so make it span
+                // one more item in any case.
+                r.end++;
+
+                inExistingGroup = true;
+            }
+            //else: Item is inserted after this range, nothing to do for it.
+        }
+
+        if ( inExistingGroup )
+            return false;
+
+        // Make a new range for the group this item will belong to.
+        Range r;
+        r.start = pos;
+        r.end = pos;
+        m_ranges.push_back(r);
+
+        return true;
+    }
+
+private:
+    // Contains the inclusive positions of the range start and end.
+    struct Range
+    {
+        int start;
+        int end;
+    };
+
+    typedef wxVector<Range> Ranges;
+    Ranges m_ranges;
+};
+
 namespace
 {
 
@@ -168,8 +262,8 @@ inline bool IsGreaterThanStdSize(const wxBitmap& bmp)
 // Construct a menu with optional title (then use append)
 void wxMenu::Init()
 {
+    m_radioData = NULL;
     m_doBreak = false;
-    m_startRadioGroup = -1;
 
 #if wxUSE_OWNER_DRAWN
     m_ownerDrawn = false;
@@ -211,6 +305,8 @@ wxMenu::~wxMenu()
     // delete accels
     WX_CLEAR_ARRAY(m_accels);
 #endif // wxUSE_ACCEL
+
+    delete m_radioData;
 }
 
 void wxMenu::Break()
@@ -219,13 +315,6 @@ void wxMenu::Break()
     m_doBreak = true;
 }
 
-void wxMenu::Attach(wxMenuBarBase *menubar)
-{
-    wxMenuBase::Attach(menubar);
-
-    EndRadioGroup();
-}
-
 #if wxUSE_ACCEL
 
 int wxMenu::FindAccel(int id) const
@@ -348,6 +437,11 @@ HBITMAP GetHBitmapForMenu(wxMenuItem *pItem, bool checked = true)
 
 } // anonymous namespace
 
+bool wxMenu::MSWGetRadioGroupRange(int pos, int *start, int *end) const
+{
+    return m_radioData && m_radioData->GetGroupRange(pos, start, end);
+}
+
 // append a new item or submenu to the menu
 bool wxMenu::DoInsertOrAppend(wxMenuItem *pItem, size_t pos)
 {
@@ -397,6 +491,21 @@ bool wxMenu::DoInsertOrAppend(wxMenuItem *pItem, size_t pos)
         pos = GetMenuItemCount() - 1;
     }
 
+    // Update radio groups data if we're inserting a new radio item.
+    //
+    // NB: If we supported inserting non-radio items in the middle of existing
+    //     radio groups to break them into two subgroups, we'd need to update
+    //     m_radioData in this case too but currently this is not supported.
+    bool checkInitially = false;
+    if ( pItem->GetKind() == wxITEM_RADIO )
+    {
+        if ( !m_radioData )
+            m_radioData = new wxMenuRadioItemsData;
+
+        if ( m_radioData->UpdateOnInsert(pos) )
+            checkInitially = true;
+    }
+
     // adjust position to account for the title of a popup menu, if any
     if ( !GetMenuBar() && !m_title.empty() )
         pos += 2; // for the title itself and its separator
@@ -600,6 +709,10 @@ bool wxMenu::DoInsertOrAppend(wxMenuItem *pItem, size_t pos)
     }
 
 
+    // Check the item if it should be initially checked.
+    if ( checkInitially )
+        pItem->Check(true);
+
     // if we just appended the title, highlight it
     if ( id == (UINT_PTR)idMenuTitle )
     {
@@ -616,67 +729,9 @@ bool wxMenu::DoInsertOrAppend(wxMenuItem *pItem, size_t pos)
     return true;
 }
 
-void wxMenu::EndRadioGroup()
-{
-    // we're not inside a radio group any longer
-    m_startRadioGroup = -1;
-}
-
 wxMenuItem* wxMenu::DoAppend(wxMenuItem *item)
 {
-    wxCHECK_MSG( item, NULL, wxT("NULL item in wxMenu::DoAppend") );
-
-    bool check = false;
-
-    if ( item->GetKind() == wxITEM_RADIO )
-    {
-        int count = GetMenuItemCount();
-
-        if ( m_startRadioGroup == -1 )
-        {
-            // start a new radio group
-            m_startRadioGroup = count;
-
-            // for now it has just one element
-            item->SetAsRadioGroupStart();
-            item->SetRadioGroupEnd(m_startRadioGroup);
-
-            // 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
-            item->SetRadioGroupStart(m_startRadioGroup);
-            wxMenuItemList::compatibility_iterator node = GetMenuItems().Item(m_startRadioGroup);
-
-            if ( node )
-            {
-                node->GetData()->SetRadioGroupEnd(count);
-            }
-            else
-            {
-                wxFAIL_MSG( wxT("where is the radio group start item?") );
-            }
-        }
-    }
-    else // not a radio item
-    {
-        EndRadioGroup();
-    }
-
-    if ( !wxMenuBase::DoAppend(item) || !DoInsertOrAppend(item) )
-    {
-        return NULL;
-    }
-
-    if ( check )
-    {
-        // check the item initially
-        item->Check(true);
-    }
-
-    return item;
+    return wxMenuBase::DoAppend(item) && DoInsertOrAppend(item) ? item : NULL;
 }
 
 wxMenuItem* wxMenu::DoInsert(size_t pos, wxMenuItem *item)
index f4f61e1321fe59836f4469f7e82c903816457148..94d963d4ac6474871def7f4f4b04f8274a677221 100644 (file)
@@ -494,9 +494,6 @@ wxMenuItem::wxMenuItem(wxMenu *parentMenu,
 
 void wxMenuItem::Init()
 {
-    m_radioGroup.start = -1;
-    m_isRadioGroupStart = false;
-
 #if  wxUSE_OWNER_DRAWN
 
     // when the color is not valid, wxOwnerDraw takes the default ones.
@@ -557,30 +554,6 @@ bool wxMenuItem::IsChecked() const
     return (flag & MF_CHECKED) != 0;
 }
 
-// radio group stuff
-// -----------------
-
-void wxMenuItem::SetAsRadioGroupStart()
-{
-    m_isRadioGroupStart = true;
-}
-
-void wxMenuItem::SetRadioGroupStart(int start)
-{
-    wxASSERT_MSG( !m_isRadioGroupStart,
-                  wxT("should only be called for the next radio items") );
-
-    m_radioGroup.start = start;
-}
-
-void wxMenuItem::SetRadioGroupEnd(int end)
-{
-    wxASSERT_MSG( m_isRadioGroupStart,
-                  wxT("should only be called for the first radio item") );
-
-    m_radioGroup.end = end;
-}
-
 // change item state
 // -----------------
 
@@ -634,17 +607,10 @@ void wxMenuItem::Check(bool check)
             int start,
                 end;
 
-            if ( m_isRadioGroupStart )
-            {
-                // we already have all information we need
-                start = pos;
-                end = m_radioGroup.end;
-            }
-            else // next radio group item
+            if ( !m_parentMenu->MSWGetRadioGroupRange(pos, &start, &end) )
             {
-                // get the radio group end from the start item
-                start = m_radioGroup.start;
-                end = items.Item(start)->GetData()->m_radioGroup.end;
+                wxFAIL_MSG( wxT("Menu radio item not part of radio group?") );
+                return;
             }
 
 #ifdef __WIN32__
index 6d8fd784de33f4dd8cfeee2bcc7232583af8833c..3424af6b50482a726ad1eb1e818140b5329916da 100644 (file)
@@ -84,6 +84,7 @@ private:
         CPPUNIT_TEST( FindInMenu );
         CPPUNIT_TEST( Count );
         CPPUNIT_TEST( Labels );
+        CPPUNIT_TEST( RadioItems );
     CPPUNIT_TEST_SUITE_END();
 
     void CreateFrame();
@@ -92,6 +93,7 @@ private:
     void FindInMenu();
     void Count();
     void Labels();
+    void RadioItems();
 
     wxFrame* m_frame;
 
@@ -304,3 +306,56 @@ void MenuTestCase::Labels()
     CPPUNIT_ASSERT_EQUAL( "Foo", itemFoo->GetItemLabelText() );
     CPPUNIT_ASSERT_EQUAL( "Foo", wxMenuItem::GetLabelText("&Foo\tCtrl-F") );
 }
+
+void MenuTestCase::RadioItems()
+{
+    wxMenuBar * const bar = m_frame->GetMenuBar();
+    wxMenu * const menu = new wxMenu;
+    bar->Append(menu, "&Radio");
+
+    // Adding consecutive radio items creates a radio group.
+    menu->AppendRadioItem(MenuTestCase_First, "Radio 0");
+    menu->AppendRadioItem(MenuTestCase_First + 1, "Radio 1");
+
+    // First item of a radio group is checked by default.
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First) );
+
+    // Checking the second one make the first one unchecked however.
+    menu->Check(MenuTestCase_First + 1, true);
+    CPPUNIT_ASSERT( !menu->IsChecked(MenuTestCase_First) );
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First + 1) );
+
+    // Adding more radio items after a separator creates another radio group...
+    menu->AppendSeparator();
+    menu->AppendRadioItem(MenuTestCase_First + 2, "Radio 2");
+    menu->AppendRadioItem(MenuTestCase_First + 3, "Radio 3");
+    menu->AppendRadioItem(MenuTestCase_First + 4, "Radio 4");
+
+    // ... which is independent from the first one.
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First + 2) );
+
+    menu->Check(MenuTestCase_First + 3, true);
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First + 3) );
+    CPPUNIT_ASSERT( !menu->IsChecked(MenuTestCase_First + 2) );
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First + 1) );
+
+
+    // Insert an item in the middle of an existing radio group.
+    menu->InsertRadioItem(4, MenuTestCase_First + 5, "Radio 5");
+    CPPUNIT_ASSERT( menu->IsChecked(MenuTestCase_First + 3) );
+
+    menu->Check( MenuTestCase_First + 5, true );
+    CPPUNIT_ASSERT( !menu->IsChecked(MenuTestCase_First + 3) );
+
+
+    // Prepend a couple of items before the first group.
+    menu->PrependRadioItem(MenuTestCase_First + 6, "Radio 6");
+    menu->PrependRadioItem(MenuTestCase_First + 7, "Radio 7");
+    menu->Check(MenuTestCase_First + 7, true);
+    CPPUNIT_ASSERT( !menu->IsChecked(MenuTestCase_First + 1) );
+
+
+    // Check that the last radio group still works as expected.
+    menu->Check(MenuTestCase_First + 4, true);
+    CPPUNIT_ASSERT( !menu->IsChecked(MenuTestCase_First + 5) );
+}