From 4f9ccec517819034d34d0b74a68659ccd6be272f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 23 Jun 2013 16:39:39 +0000 Subject: [PATCH] Add wxBookCtrlBase::DoSetSelectionAfterRemoval() and use it in wxSimplebook. This fixes the wrong handling of the selection in wxSimplebook when the currently selected page was deleted. Also extend the unit tests to check for this bug. Closes #15188. git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@74279 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- include/wx/bookctrl.h | 10 +++++++++- include/wx/simplebook.h | 11 +++++++++-- src/common/bookctrl.cpp | 20 ++++++++++++++++++++ src/generic/choicbkg.cpp | 17 +---------------- src/generic/listbkg.cpp | 17 +---------------- src/generic/toolbkg.cpp | 17 +---------------- tests/controls/bookctrlbasetest.cpp | 10 ++++++++-- 7 files changed, 49 insertions(+), 53 deletions(-) diff --git a/include/wx/bookctrl.h b/include/wx/bookctrl.h index d1065dbde5..324fd94ca1 100644 --- a/include/wx/bookctrl.h +++ b/include/wx/bookctrl.h @@ -256,6 +256,10 @@ protected: // false otherwise. bool DoSetSelectionAfterInsertion(size_t n, bool bSelect); + // Update the selection after removing the page at the given index, + // typically called from the derived class overridden DoRemovePage(). + void DoSetSelectionAfterRemoval(size_t n); + // set the selection to the given page, sending the events (which can // possibly prevent the page change from taking place) if SendEvent flag is // included @@ -292,7 +296,11 @@ protected: // having nodes without any associated page) virtual bool AllowNullPage() const { return false; } - // remove the page and return a pointer to it + // Remove the page and return a pointer to it. + // + // It also needs to update the current selection if necessary, i.e. if the + // page being removed comes before the selected one and the helper method + // DoSetSelectionAfterRemoval() can be used for this. virtual wxWindow *DoRemovePage(size_t page) = 0; // our best size is the size which fits all our pages diff --git a/include/wx/simplebook.h b/include/wx/simplebook.h index 9b6474b0c4..795391466c 100644 --- a/include/wx/simplebook.h +++ b/include/wx/simplebook.h @@ -156,8 +156,15 @@ protected: virtual wxWindow *DoRemovePage(size_t page) { - m_pageTexts.erase(m_pageTexts.begin() + page); - return wxBookCtrlBase::DoRemovePage(page); + wxWindow* const win = wxBookCtrlBase::DoRemovePage(page); + if ( win ) + { + m_pageTexts.erase(m_pageTexts.begin() + page); + + DoSetSelectionAfterRemoval(page); + } + + return win; } virtual void DoSize() diff --git a/src/common/bookctrl.cpp b/src/common/bookctrl.cpp index 9ec08bad1c..8d86c5343e 100644 --- a/src/common/bookctrl.cpp +++ b/src/common/bookctrl.cpp @@ -456,6 +456,26 @@ bool wxBookCtrlBase::DoSetSelectionAfterInsertion(size_t n, bool bSelect) return true; } +void wxBookCtrlBase::DoSetSelectionAfterRemoval(size_t n) +{ + if ( m_selection >= (int)n ) + { + // ensure that the selection is valid + int sel; + if ( GetPageCount() == 0 ) + sel = wxNOT_FOUND; + else + sel = m_selection ? m_selection - 1 : 0; + + // if deleting current page we shouldn't try to hide it + m_selection = m_selection == (int)n ? wxNOT_FOUND + : m_selection - 1; + + if ( sel != wxNOT_FOUND && sel != m_selection ) + SetSelection(sel); + } +} + int wxBookCtrlBase::DoSetSelection(size_t n, int flags) { wxCHECK_MSG( n < GetPageCount(), wxNOT_FOUND, diff --git a/src/generic/choicbkg.cpp b/src/generic/choicbkg.cpp index c8b2c5acf5..5c0ac7a514 100644 --- a/src/generic/choicbkg.cpp +++ b/src/generic/choicbkg.cpp @@ -209,22 +209,7 @@ wxWindow *wxChoicebook::DoRemovePage(size_t page) { GetChoiceCtrl()->Delete(page); - if ( m_selection >= (int)page ) - { - // ensure that the selection is valid - int sel; - if ( GetPageCount() == 0 ) - sel = wxNOT_FOUND; - else - sel = m_selection ? m_selection - 1 : 0; - - // if deleting current page we shouldn't try to hide it - m_selection = m_selection == (int)page ? wxNOT_FOUND - : m_selection - 1; - - if ( sel != wxNOT_FOUND && sel != m_selection ) - SetSelection(sel); - } + DoSetSelectionAfterRemoval(page); } return win; diff --git a/src/generic/listbkg.cpp b/src/generic/listbkg.cpp index ccdc712780..ba736367c7 100644 --- a/src/generic/listbkg.cpp +++ b/src/generic/listbkg.cpp @@ -348,28 +348,13 @@ wxListbook::InsertPage(size_t n, wxWindow *wxListbook::DoRemovePage(size_t page) { - const size_t page_count = GetPageCount(); wxWindow *win = wxBookCtrlBase::DoRemovePage(page); if ( win ) { GetListView()->DeleteItem(page); - if (m_selection >= (int)page) - { - // force new sel valid if possible - int sel = m_selection - 1; - if (page_count == 1) - sel = wxNOT_FOUND; - else if ((page_count == 2) || (sel == wxNOT_FOUND)) - sel = 0; - - // force sel invalid if deleting current page - don't try to hide it - m_selection = (m_selection == (int)page) ? wxNOT_FOUND : m_selection - 1; - - if ((sel != wxNOT_FOUND) && (sel != m_selection)) - SetSelection(sel); - } + DoSetSelectionAfterRemoval(page); GetListView()->Arrange(); UpdateSize(); diff --git a/src/generic/toolbkg.cpp b/src/generic/toolbkg.cpp index 1fe2ad781d..f324ea96db 100644 --- a/src/generic/toolbkg.cpp +++ b/src/generic/toolbkg.cpp @@ -333,28 +333,13 @@ bool wxToolbook::InsertPage(size_t n, wxWindow *wxToolbook::DoRemovePage(size_t page) { - const size_t page_count = GetPageCount(); wxWindow *win = wxBookCtrlBase::DoRemovePage(page); if ( win ) { GetToolBar()->DeleteTool(page + 1); - if (m_selection >= (int)page) - { - // force new sel valid if possible - int sel = m_selection - 1; - if (page_count == 1) - sel = wxNOT_FOUND; - else if ((page_count == 2) || (sel == wxNOT_FOUND)) - sel = 0; - - // force sel invalid if deleting current page - don't try to hide it - m_selection = (m_selection == (int)page) ? wxNOT_FOUND : m_selection - 1; - - if ((sel != wxNOT_FOUND) && (sel != m_selection)) - SetSelection(sel); - } + DoSetSelectionAfterRemoval(page); } return win; diff --git a/tests/controls/bookctrlbasetest.cpp b/tests/controls/bookctrlbasetest.cpp index c5065607e2..acd787f6fa 100644 --- a/tests/controls/bookctrlbasetest.cpp +++ b/tests/controls/bookctrlbasetest.cpp @@ -95,19 +95,25 @@ void BookCtrlBaseTestCase::PageManagement() CPPUNIT_ASSERT_EQUAL(0, base->GetSelection()); CPPUNIT_ASSERT_EQUAL(4, base->GetPageCount()); + // Change the selection to verify that deleting a page before the currently + // selected one correctly updates the selection. + base->SetSelection(2); + CPPUNIT_ASSERT_EQUAL(2, base->GetSelection()); + base->DeletePage(1); CPPUNIT_ASSERT_EQUAL(3, base->GetPageCount()); + CPPUNIT_ASSERT_EQUAL(1, base->GetSelection()); base->RemovePage(0); CPPUNIT_ASSERT_EQUAL(2, base->GetPageCount()); + CPPUNIT_ASSERT_EQUAL(0, base->GetSelection()); base->DeleteAllPages(); CPPUNIT_ASSERT_EQUAL(0, base->GetPageCount()); - - AddPanels(); + CPPUNIT_ASSERT_EQUAL(-1, base->GetSelection()); } void BookCtrlBaseTestCase::ChangeEvents()