From 89511b42686033f8cb7e8a110580847bc2470b08 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 10 May 2011 08:50:38 +0000 Subject: [PATCH] Rewrote wxMSW radio menu items code to support not only appending them. 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 | 20 ++-- include/wx/msw/menuitem.h | 17 ---- src/msw/menu.cpp | 191 ++++++++++++++++++++++++-------------- src/msw/menuitem.cpp | 40 +------- tests/menu/menu.cpp | 55 +++++++++++ 5 files changed, 194 insertions(+), 129 deletions(-) diff --git a/include/wx/msw/menu.h b/include/wx/msw/menu.h index d80e85cca1..4c5e95f5ad 100644 --- a/include/wx/msw/menu.h +++ b/include/wx/msw/menu.h @@ -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; diff --git a/include/wx/msw/menuitem.h b/include/wx/msw/menuitem.h index 21be80c38f..1fe684971b 100644 --- a/include/wx/msw/menuitem.h +++ b/include/wx/msw/menuitem.h @@ -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 diff --git a/src/msw/menu.cpp b/src/msw/menu.cpp index 3901d885fe..c1accd90e2 100644 --- a/src/msw/menu.cpp +++ b/src/msw/menu.cpp @@ -41,6 +41,7 @@ #endif #include "wx/scopedarray.h" +#include "wx/vector.h" #include "wx/msw/private.h" #include "wx/msw/wrapcctl.h" // include "properly" @@ -85,9 +86,102 @@ 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 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) diff --git a/src/msw/menuitem.cpp b/src/msw/menuitem.cpp index f4f61e1321..94d963d4ac 100644 --- a/src/msw/menuitem.cpp +++ b/src/msw/menuitem.cpp @@ -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__ diff --git a/tests/menu/menu.cpp b/tests/menu/menu.cpp index 6d8fd784de..3424af6b50 100644 --- a/tests/menu/menu.cpp +++ b/tests/menu/menu.cpp @@ -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) ); +} -- 2.47.2