From 665b5a76a88b0db8ee04103ce1005e9cd239c5e4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 17:51:27 +0100 Subject: [PATCH 1/8] Remove unnecessary wxNotebook::DoSetSelection() from wxQt Having this method didn't really help, it's simpler and more straightforward to implement ChangeSelection() as a signal-blocking wrapper around SetSelection() instead. No real changes in behaviour, this is a pure refactoring. --- include/wx/qt/notebook.h | 5 ++--- src/qt/notebook.cpp | 24 ++++++++++++++---------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/wx/qt/notebook.h b/include/wx/qt/notebook.h index 288d0d9a7f..0dd81175c1 100644 --- a/include/wx/qt/notebook.h +++ b/include/wx/qt/notebook.h @@ -42,14 +42,13 @@ public: virtual wxSize CalcSizeFromPage(const wxSize& sizePage) const; - int SetSelection(size_t nPage) { return DoSetSelection(nPage, SetSelection_SendEvent); } - int ChangeSelection(size_t nPage) { return DoSetSelection(nPage); } + int SetSelection(size_t nPage); + int ChangeSelection(size_t nPage); virtual QWidget *GetHandle() const; protected: virtual wxWindow *DoRemovePage(size_t page); - int DoSetSelection(size_t nPage, int flags = 0); private: QTabWidget *m_qtTabWidget; diff --git a/src/qt/notebook.cpp b/src/qt/notebook.cpp index 27d4ce7387..029716ada7 100644 --- a/src/qt/notebook.cpp +++ b/src/qt/notebook.cpp @@ -169,27 +169,31 @@ wxSize wxNotebook::CalcSizeFromPage(const wxSize& sizePage) const return sizePage; } -int wxNotebook::DoSetSelection(size_t page, int flags) +int wxNotebook::SetSelection(size_t page) { wxCHECK_MSG(page < GetPageCount(), wxNOT_FOUND, "invalid notebook index"); int selOld = GetSelection(); - // do not fire signals for certain methods (i.e. ChangeSelection - if ( !(flags & SetSelection_SendEvent) ) - { - m_qtTabWidget->blockSignals(true); - } // change the QTabWidget selected page: m_selection = page; m_qtTabWidget->setCurrentIndex( page ); - if ( !(flags & SetSelection_SendEvent) ) - { - m_qtTabWidget->blockSignals(false); - } + return selOld; } +int wxNotebook::ChangeSelection(size_t nPage) +{ + // ChangeSelection() is not supposed to generate events, unlike + // SetSelection(). + m_qtTabWidget->blockSignals(true); + + const int selOld = SetSelection(nPage); + + m_qtTabWidget->blockSignals(false); + + return selOld; +} wxWindow *wxNotebook::DoRemovePage(size_t page) { From 6fdcfd819937d44e7d8a541c9401cd4554e5a311 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 18:21:32 +0100 Subject: [PATCH 2/8] Add unit test for no events when adding first wxNotebook page Adding the first page shouldn't generate any events even if the selection does change (from -1 to 0) internally. --- tests/controls/notebooktest.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/controls/notebooktest.cpp b/tests/controls/notebooktest.cpp index 1812d08a97..4309a46ee5 100644 --- a/tests/controls/notebooktest.cpp +++ b/tests/controls/notebooktest.cpp @@ -20,7 +20,9 @@ #endif // WX_PRECOMP #include "wx/notebook.h" + #include "bookctrlbasetest.h" +#include "testableframe.h" class NotebookTestCase : public BookCtrlBaseTestCase, public CppUnit::TestCase { @@ -118,4 +120,25 @@ void NotebookTestCase::NoEventsOnDestruction() CHECK( m_numPageChanges == 1 ); } +TEST_CASE("wxNotebook::NoEventsForFirstPage", "[wxNotebook][event]") +{ + wxNotebook* const + notebook = new wxNotebook(wxTheApp->GetTopWindow(), wxID_ANY, + wxDefaultPosition, wxSize(400, 200)); + + CHECK( notebook->GetSelection() == wxNOT_FOUND ); + + EventCounter countPageChanging(notebook, wxEVT_NOTEBOOK_PAGE_CHANGING); + EventCounter countPageChanged(notebook, wxEVT_NOTEBOOK_PAGE_CHANGED); + + notebook->AddPage(new wxPanel(notebook), "First page"); + + // The selection should have been changed. + CHECK( notebook->GetSelection() == 0 ); + + // But no events should have been generated. + CHECK( countPageChanging.GetCount() == 0 ); + CHECK( countPageChanged.GetCount() == 0 ); +} + #endif //wxUSE_NOTEBOOK From da96ab179fa8b6b572b392f3b89e0c7d8467f588 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 18:29:31 +0100 Subject: [PATCH 3/8] Fix updating selection in wxNotebook::InsertPage() in wxQt Just use DoSetSelectionAfterInsertion() provided by the base class, which does the right thing already, including both setting m_selection and sending (or not sending) the corresponding events. --- src/qt/notebook.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/qt/notebook.cpp b/src/qt/notebook.cpp index 029716ada7..f5c4c9ea60 100644 --- a/src/qt/notebook.cpp +++ b/src/qt/notebook.cpp @@ -154,12 +154,8 @@ bool wxNotebook::InsertPage(size_t n, wxWindow *page, const wxString& text, // reenable firing qt signals as internal wx initialization was completed m_qtTabWidget->blockSignals(false); - m_selection = m_qtTabWidget->currentIndex(); - if (bSelect && GetPageCount() > 1) - { - SetSelection( n ); - } + DoSetSelectionAfterInsertion(n, bSelect); return true; } From 359b23a58b3ac2d0137b756989ebebbf9aef4b4f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 18:51:02 +0100 Subject: [PATCH 4/8] Avoid events on first page insertion in wxGTK wxNotebook Make wxGTK consistent with wxMSW and wxOSX and also make it pass the unit test added in the previous commit by suppressing events generated by gtk_notebook_insert_page() when adding the first page (but not any subsequent ones). --- src/gtk/notebook.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/gtk/notebook.cpp b/src/gtk/notebook.cpp index 71bd4d8ec2..4e86999f34 100644 --- a/src/gtk/notebook.cpp +++ b/src/gtk/notebook.cpp @@ -464,7 +464,12 @@ bool wxNotebook::InsertPage( size_t position, pageData->m_label, false, false, m_padding); gtk_widget_show_all(pageData->m_box); + + // Inserting the page may generate selection changing events that are not + // expected here: we will send them ourselves below if necessary. + g_signal_handlers_block_by_func(m_widget, (void*)switch_page, this); gtk_notebook_insert_page(notebook, win->m_widget, pageData->m_box, position); + g_signal_handlers_unblock_by_func(m_widget, (void*)switch_page, this); /* apply current style */ #ifdef __WXGTK3__ From 7cd6a27e1615eea9806b07e5076e75aee23dde90 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 18:56:03 +0100 Subject: [PATCH 5/8] Clarify when wxBookCtrl::AddPage() sends page changing events Notably document that it does not do it when adding the first page. --- interface/wx/bookctrl.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/interface/wx/bookctrl.h b/interface/wx/bookctrl.h index 7789496765..dc775d00a6 100644 --- a/interface/wx/bookctrl.h +++ b/interface/wx/bookctrl.h @@ -249,7 +249,10 @@ public: The page must have the book control itself as the parent and must not have been added to this control previously. - The call to this function may generate the page changing events. + The call to this function will generate the page changing and page + changed events if @a select is true, but not when inserting the very + first page (as there is no previous page selection to switch from in + this case and so it wouldn't make sense to e.g. veto such event). @param page Specifies the new page. From 7d36b02bcc190a309d013f107701b16c6aca8828 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 18:59:34 +0100 Subject: [PATCH 6/8] Reuse base class helper in wxGTK wxNotebook::InsertPage() Call DoSetSelectionAfterInsertion() instead of partially duplicating it. This ensures that m_selection is correct after inserting a new page, which wasn't the case before. Surprisingly, this didn't actually matter because wxGTK implementation doesn't really use the base class m_selection neither and just queries GTK+ widget directly for its selection in GetSelection() instead, but it was at the very least confusing. --- src/gtk/notebook.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/gtk/notebook.cpp b/src/gtk/notebook.cpp index 4e86999f34..a47d136a1e 100644 --- a/src/gtk/notebook.cpp +++ b/src/gtk/notebook.cpp @@ -483,10 +483,7 @@ bool wxNotebook::InsertPage( size_t position, } #endif - if (select && GetPageCount() > 1) - { - SetSelection( position ); - } + DoSetSelectionAfterInsertion(position, select); InvalidateBestSize(); return true; From c22e81c7a1ab18b9c5109e299855bc92f7041957 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 19:01:17 +0100 Subject: [PATCH 7/8] Extend unit test for events generated by wxNotebook::AddPage() In addition to checking that adding the first page doesn't generate any events, check that adding another page later does generate them. --- tests/controls/notebooktest.cpp | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/tests/controls/notebooktest.cpp b/tests/controls/notebooktest.cpp index 4309a46ee5..60c9cdd31b 100644 --- a/tests/controls/notebooktest.cpp +++ b/tests/controls/notebooktest.cpp @@ -120,7 +120,7 @@ void NotebookTestCase::NoEventsOnDestruction() CHECK( m_numPageChanges == 1 ); } -TEST_CASE("wxNotebook::NoEventsForFirstPage", "[wxNotebook][event]") +TEST_CASE("wxNotebook::AddPageEvents", "[wxNotebook][AddPage][event]") { wxNotebook* const notebook = new wxNotebook(wxTheApp->GetTopWindow(), wxID_ANY, @@ -131,7 +131,8 @@ TEST_CASE("wxNotebook::NoEventsForFirstPage", "[wxNotebook][event]") EventCounter countPageChanging(notebook, wxEVT_NOTEBOOK_PAGE_CHANGING); EventCounter countPageChanged(notebook, wxEVT_NOTEBOOK_PAGE_CHANGED); - notebook->AddPage(new wxPanel(notebook), "First page"); + // Add the first page, it is special. + notebook->AddPage(new wxPanel(notebook), "Initial page"); // The selection should have been changed. CHECK( notebook->GetSelection() == 0 ); @@ -139,6 +140,28 @@ TEST_CASE("wxNotebook::NoEventsForFirstPage", "[wxNotebook][event]") // But no events should have been generated. CHECK( countPageChanging.GetCount() == 0 ); CHECK( countPageChanged.GetCount() == 0 ); + + + // Add another page without selecting it. + notebook->AddPage(new wxPanel(notebook), "Unselected page"); + + // Selection shouldn't have changed. + CHECK( notebook->GetSelection() == 0 ); + + // And no events should have been generated, of course. + CHECK( countPageChanging.GetCount() == 0 ); + CHECK( countPageChanged.GetCount() == 0 ); + + + // Finally add another page and do select it. + notebook->AddPage(new wxPanel(notebook), "Selected page", true); + + // It should have become selected. + CHECK( notebook->GetSelection() == 2 ); + + // And events for the selection change should have been generated. + CHECK( countPageChanging.GetCount() == 1 ); + CHECK( countPageChanged.GetCount() == 1 ); } #endif //wxUSE_NOTEBOOK From ce1b72a4d2869e7cd7fdcbe3e9e0698135d53fa8 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Jan 2019 19:05:01 +0100 Subject: [PATCH 8/8] Document wxNotebook::AddPage() events change in wxGTK This hopefully shouldn't affect many people, but still document this backwards-incompatible change. --- docs/changes.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changes.txt b/docs/changes.txt index 4a41c873e9..3a288f2ca3 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -62,6 +62,9 @@ Changes in behaviour not resulting in compilation errors - Generic wxDataViewCtrl now always resizes its last column to fill all the available space, as the GTK+ version always did. +- wxGTK wxNotebook::AddPage() doesn't generate any events any more for the + first page being added, for consistency with the other ports. + Changes in behaviour which may result in build errors -----------------------------------------------------