From e1a702a205a145ff791a8f8097d33e18c86646de Mon Sep 17 00:00:00 2001 From: Stefan Ziegler Date: Wed, 5 Dec 2018 13:14:01 +0100 Subject: [PATCH] Fix removing and inserting pages in wxToolbook Removing a page from wxToolbook could result in crashes due to dereferencing the now invalid page index. Fix this by not assuming that the page index is the same as its tool ID, but adding functions to explicitly map one to the other. Also fix inserting pages into wxToolbook, which worked as appending them before. Closes https://github.com/wxWidgets/wxWidgets/pull/1042 Closes #18275. --- docs/changes.txt | 1 + include/wx/toolbook.h | 8 +++++ src/generic/toolbkg.cpp | 77 ++++++++++++++++++++++++++++++++--------- 3 files changed, 69 insertions(+), 17 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index 3cf2901ca5..57a5769bcc 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -142,6 +142,7 @@ All (GUI): - Add wxGraphicsContext::GetWindow() and implement wxGraphicsContext::GetDPI(). - Add wxToolbook::EnablePage() (Stefan Ziegler). - Adapt AUI colours to system colour changes (Daniel Kulp). +- Fix removing and inserting pages in wxToolbook (Stefan Ziegler). wxGTK: diff --git a/include/wx/toolbook.h b/include/wx/toolbook.h index ee3e018fe0..fd868226cb 100644 --- a/include/wx/toolbook.h +++ b/include/wx/toolbook.h @@ -123,6 +123,14 @@ private: // common part of all constructors void Init(); + // returns the tool identifier for the specified page + int PageToToolId(size_t page) const; + + // returns the page index for the specified tool ID or + // wxNOT_FOUND if there is no page with that tool ID + int ToolIdToPage(int toolId) const; + + wxDECLARE_EVENT_TABLE(); wxDECLARE_DYNAMIC_CLASS_NO_COPY(wxToolbook); }; diff --git a/src/generic/toolbkg.cpp b/src/generic/toolbkg.cpp index a7ad0601fb..38be3659a5 100644 --- a/src/generic/toolbkg.cpp +++ b/src/generic/toolbkg.cpp @@ -42,7 +42,7 @@ wxDEFINE_EVENT( wxEVT_TOOLBOOK_PAGE_CHANGED, wxBookCtrlEvent ); wxBEGIN_EVENT_TABLE(wxToolbook, wxBookCtrlBase) EVT_SIZE(wxToolbook::OnSize) - EVT_TOOL_RANGE(1, 50, wxToolbook::OnToolSelected) + EVT_TOOL(wxID_ANY, wxToolbook::OnToolSelected) EVT_IDLE(wxToolbook::OnIdle) wxEND_EVENT_TABLE() @@ -134,8 +134,8 @@ void wxToolbook::OnSize(wxSizeEvent& event) bool wxToolbook::SetPageText(size_t n, const wxString& strText) { - // Assume tool ids start from 1 - wxToolBarToolBase* tool = GetToolBar()->FindById(n + 1); + int toolId = PageToToolId(n); + wxToolBarToolBase* tool = GetToolBar()->FindById(toolId); if (tool) { tool->SetLabel(strText); @@ -147,7 +147,8 @@ bool wxToolbook::SetPageText(size_t n, const wxString& strText) wxString wxToolbook::GetPageText(size_t n) const { - wxToolBarToolBase* tool = GetToolBar()->FindById(n + 1); + int toolId = PageToToolId(n); + wxToolBarToolBase* tool = GetToolBar()->FindById(toolId); if (tool) return tool->GetLabel(); else @@ -167,9 +168,10 @@ bool wxToolbook::SetPageImage(size_t n, int imageId) if (!GetImageList()) return false; + int toolId = PageToToolId(n); wxBitmap bmp = GetImageList()->GetBitmap(imageId); - GetToolBar()->SetToolNormalBitmap(n + 1, bmp); - GetToolBar()->SetToolDisabledBitmap(n + 1, bmp.ConvertToDisabled()); + GetToolBar()->SetToolNormalBitmap(toolId, bmp); + GetToolBar()->SetToolDisabledBitmap(toolId, bmp.ConvertToDisabled()); return true; } @@ -199,7 +201,8 @@ void wxToolbook::MakeChangedEvent(wxBookCtrlEvent &event) void wxToolbook::UpdateSelectedPage(size_t newsel) { - GetToolBar()->ToggleTool(newsel + 1, true); + int toolId = PageToToolId(newsel); + GetToolBar()->ToggleTool(toolId, true); } // Not part of the wxBookctrl API, but must be called in OnIdle or @@ -302,16 +305,30 @@ bool wxToolbook::InsertPage(size_t n, m_maxBitmapSize.x = wxMax(bitmap.GetWidth(), m_maxBitmapSize.x); m_maxBitmapSize.y = wxMax(bitmap.GetHeight(), m_maxBitmapSize.y); + int toolId = page->GetId(); GetToolBar()->SetToolBitmapSize(m_maxBitmapSize); - GetToolBar()->AddRadioTool(n + 1, text, bitmap, bitmap.ConvertToDisabled(), text); + GetToolBar()->InsertTool(n, toolId, text, bitmap, bitmap.ConvertToDisabled(), wxITEM_RADIO); - if (bSelect) + // fix current selection + if (m_selection == wxNOT_FOUND) { - GetToolBar()->ToggleTool(n, true); + DoShowPage(page, true); m_selection = n; } + else if ((size_t) m_selection >= n) + { + DoShowPage(page, false); + m_selection++; + } else - page->Hide(); + { + DoShowPage(page, false); + } + + if ( bSelect ) + { + SetSelection(n); + } InvalidateBestSize(); return true; @@ -319,11 +336,12 @@ bool wxToolbook::InsertPage(size_t n, wxWindow *wxToolbook::DoRemovePage(size_t page) { + int toolId = PageToToolId(page); wxWindow *win = wxBookCtrlBase::DoRemovePage(page); if ( win ) { - GetToolBar()->DeleteTool(page + 1); + GetToolBar()->DeleteTool(toolId); DoSetSelectionAfterRemoval(page); } @@ -340,7 +358,8 @@ bool wxToolbook::DeleteAllPages() bool wxToolbook::EnablePage(size_t page, bool enable) { - GetToolBar()->EnableTool(page + 1, enable); + int toolId = PageToToolId(page); + GetToolBar()->EnableTool(toolId, enable); if (!enable && GetSelection() == (int)page) { AdvanceSelection(); @@ -358,15 +377,39 @@ bool wxToolbook::EnablePage(wxWindow *page, bool enable) return EnablePage(pageIndex, enable); } +int wxToolbook::PageToToolId(size_t page) const +{ + wxCHECK_MSG(page < GetPageCount(), wxID_NONE, "Invalid page number"); + return GetPage(page)->GetId(); +} + +int wxToolbook::ToolIdToPage(int toolId) const +{ + for (size_t i = 0; i < m_pages.size(); i++) + { + if (m_pages[i]->GetId() == toolId) + { + return (int) i; + } + } + return wxNOT_FOUND; +} + // ---------------------------------------------------------------------------- // wxToolbook events // ---------------------------------------------------------------------------- void wxToolbook::OnToolSelected(wxCommandEvent& event) { - const int selNew = event.GetId() - 1; + // find page for the tool + int page = ToolIdToPage(event.GetId()); + if (page == wxNOT_FOUND) + { + // this happens only of page id has changed afterwards + return; + } - if ( selNew == m_selection ) + if (page == m_selection ) { // this event can only come from our own Select(m_selection) below // which we call when the page change is vetoed, so we should simply @@ -374,10 +417,10 @@ void wxToolbook::OnToolSelected(wxCommandEvent& event) return; } - SetSelection(selNew); + SetSelection(page); // change wasn't allowed, return to previous state - if (m_selection != selNew) + if (m_selection != page) { GetToolBar()->ToggleTool(m_selection, false); }