From 3b22d9a56bd5b3f3365fda346bb8486ad79fb428 Mon Sep 17 00:00:00 2001 From: Robin Dunn Date: Wed, 24 Jan 2018 17:59:25 +0100 Subject: [PATCH 1/5] Test wxTreeListCtrl::DeleteAllItems() in the sample Add a menu command to invoke this method for testing. --- samples/treelist/treelist.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/samples/treelist/treelist.cpp b/samples/treelist/treelist.cpp index 2bdc74507a..0614f0ef84 100644 --- a/samples/treelist/treelist.cpp +++ b/samples/treelist/treelist.cpp @@ -65,6 +65,8 @@ enum Id_CheckboxesUser3State, Id_Checkboxes_End, + Id_DeleteAllItems, + Id_DumpSelection, Id_Check_HTMLDocs, Id_Uncheck_HTMLDocs, @@ -187,6 +189,8 @@ private: void OnAbout(wxCommandEvent& event); void OnExit(wxCommandEvent& event); + void OnDeleteAllItems(wxCommandEvent& event); + void OnSelectionChanged(wxTreeListEvent& event); void OnItemExpanding(wxTreeListEvent& event); void OnItemExpanded(wxTreeListEvent& event); @@ -272,6 +276,8 @@ wxBEGIN_EVENT_TABLE(MyFrame, wxFrame) EVT_MENU(wxID_ABOUT, MyFrame::OnAbout) EVT_MENU(wxID_EXIT, MyFrame::OnExit) + EVT_MENU(Id_DeleteAllItems, MyFrame::OnDeleteAllItems) + EVT_TREELIST_SELECTION_CHANGED(wxID_ANY, MyFrame::OnSelectionChanged) EVT_TREELIST_ITEM_EXPANDING(wxID_ANY, MyFrame::OnItemExpanding) EVT_TREELIST_ITEM_EXPANDED(wxID_ANY, MyFrame::OnItemExpanded) @@ -314,6 +320,8 @@ MyFrame::MyFrame() treeOper->Append(Id_Indet_HTMLDocs, "Make Doc/HTML &indeterminate\tCtrl-I"); treeOper->Append(Id_Select_HTMLDocs, "&Select Doc/HTML item\tCtrl-S"); + treeOper->Append(Id_DeleteAllItems, "DeleteAllItems"); + wxMenu* helpMenu = new wxMenu; helpMenu->Append(wxID_ABOUT); @@ -577,6 +585,11 @@ void MyFrame::OnExit(wxCommandEvent& WXUNUSED(event)) Close(true); } +void MyFrame::OnDeleteAllItems(wxCommandEvent& WXUNUSED(event)) +{ + m_treelist->DeleteAllItems(); +} + wxString MyFrame::DumpItem(wxTreeListItem item) const { return item.IsOk() ? m_treelist->GetItemText(item) : wxString("NONE"); From 0cea0a67f1c8d66a4abd916b9ffb9a11bd0feb30 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Jan 2018 18:18:22 +0100 Subject: [PATCH 2/5] Use RAII SelectionEventsSuppressor in wxGTK wxDataViewCtrl code This is simpler and safer than always remembering to call GtkDisableSelectionEvents() and GtkEnableSelectionEvents() in pairs. No real changes. --- include/wx/gtk/dataview.h | 19 +++++++++++++++++++ src/gtk/dataview.cpp | 20 +++++--------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/include/wx/gtk/dataview.h b/include/wx/gtk/dataview.h index a6d1d45c3b..059f1dc367 100644 --- a/include/wx/gtk/dataview.h +++ b/include/wx/gtk/dataview.h @@ -188,6 +188,25 @@ public: int GTKGetUniformRowHeight() const { return m_uniformRowHeight; } + // Simple RAII helper for disabling selection events during its lifetime. + class SelectionEventsSuppressor + { + public: + explicit SelectionEventsSuppressor(wxDataViewCtrl* ctrl) + : m_ctrl(ctrl) + { + m_ctrl->GtkDisableSelectionEvents(); + } + + ~SelectionEventsSuppressor() + { + m_ctrl->GtkEnableSelectionEvents(); + } + + private: + wxDataViewCtrl* const m_ctrl; + }; + protected: virtual void DoSetExpanderColumn() wxOVERRIDE; virtual void DoSetIndent() wxOVERRIDE; diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp index d78d075559..3ca8b55e60 100644 --- a/src/gtk/dataview.cpp +++ b/src/gtk/dataview.cpp @@ -5068,7 +5068,7 @@ void wxDataViewCtrl::SetSelections( const wxDataViewItemArray & sel ) { wxCHECK_RET( m_internal, "model must be associated before calling SetSelections" ); - GtkDisableSelectionEvents(); + SelectionEventsSuppressor noSelection(this); GtkTreeSelection *selection = gtk_tree_view_get_selection( GTK_TREE_VIEW(m_treeview) ); @@ -5093,8 +5093,6 @@ void wxDataViewCtrl::SetSelections( const wxDataViewItemArray & sel ) iter.user_data = (gpointer) item.GetID(); gtk_tree_selection_select_iter( selection, &iter ); } - - GtkEnableSelectionEvents(); } void wxDataViewCtrl::Select( const wxDataViewItem & item ) @@ -5103,7 +5101,7 @@ void wxDataViewCtrl::Select( const wxDataViewItem & item ) ExpandAncestors(item); - GtkDisableSelectionEvents(); + SelectionEventsSuppressor noSelection(this); GtkTreeSelection *selection = gtk_tree_view_get_selection( GTK_TREE_VIEW(m_treeview) ); @@ -5111,15 +5109,13 @@ void wxDataViewCtrl::Select( const wxDataViewItem & item ) iter.stamp = m_internal->GetGtkModel()->stamp; iter.user_data = (gpointer) item.GetID(); gtk_tree_selection_select_iter( selection, &iter ); - - GtkEnableSelectionEvents(); } void wxDataViewCtrl::Unselect( const wxDataViewItem & item ) { wxCHECK_RET( m_internal, "model must be associated before calling Unselect" ); - GtkDisableSelectionEvents(); + SelectionEventsSuppressor noSelection(this); GtkTreeSelection *selection = gtk_tree_view_get_selection( GTK_TREE_VIEW(m_treeview) ); @@ -5127,8 +5123,6 @@ void wxDataViewCtrl::Unselect( const wxDataViewItem & item ) iter.stamp = m_internal->GetGtkModel()->stamp; iter.user_data = (gpointer) item.GetID(); gtk_tree_selection_unselect_iter( selection, &iter ); - - GtkEnableSelectionEvents(); } bool wxDataViewCtrl::IsSelected( const wxDataViewItem & item ) const @@ -5146,24 +5140,20 @@ bool wxDataViewCtrl::IsSelected( const wxDataViewItem & item ) const void wxDataViewCtrl::SelectAll() { - GtkDisableSelectionEvents(); + SelectionEventsSuppressor noSelection(this); GtkTreeSelection *selection = gtk_tree_view_get_selection( GTK_TREE_VIEW(m_treeview) ); gtk_tree_selection_select_all( selection ); - - GtkEnableSelectionEvents(); } void wxDataViewCtrl::UnselectAll() { - GtkDisableSelectionEvents(); + SelectionEventsSuppressor noSelection(this); GtkTreeSelection *selection = gtk_tree_view_get_selection( GTK_TREE_VIEW(m_treeview) ); gtk_tree_selection_unselect_all( selection ); - - GtkEnableSelectionEvents(); } void wxDataViewCtrl::EnsureVisible(const wxDataViewItem& item, From 27f705686de5dd83c8cd2df68e3ff070d13021da Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Jan 2018 22:05:14 +0100 Subject: [PATCH 3/5] Avoid using the already deleted model items when clearing it There is an impedance mismatch between wxDataViewCtrl API, which allows deleting all items of the model at once, and GtkTreeView, which only allows deleting them one by one, so bad things happen when we start deleting the items from the GTK+ model after already having deleted them from the wxDataViewModel, due to dereferencing the already freed pointers. Work around this by explicitly marking the model as being "temporarily unsafe to use" by setting the stamp, uniquely identifying it, to 0 (and ensuring that it's never 0 during the normal operation), and checking for it in all functions that are called by GTK+ from inside gtk_tree_model_row_deleted() that we call from Cleared(). The fix is not ideal, as the list of these functions was determined empirically and could change in the future GTK+ versions, and also ugly, but there doesn't seem to be any other way around this and at least now Valgrind doesn't report invalid memory reads after calling wxTreeListCtrl::DeleteAllItems() as it did before. --- src/gtk/dataview.cpp | 43 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp index 3ca8b55e60..ee482cd68c 100644 --- a/src/gtk/dataview.cpp +++ b/src/gtk/dataview.cpp @@ -680,7 +680,13 @@ wxgtk_tree_model_init(GTypeInstance* instance, void*) { GtkWxTreeModel* tree_model = GTK_WX_TREE_MODEL(instance); tree_model->internal = NULL; - tree_model->stamp = g_random_int(); + + // 0 is handled specially in wxGtkTreeCellDataFunc, so don't use it as the + // stamp. + do + { + tree_model->stamp = g_random_int(); + } while ( tree_model->stamp == 0 ); } } // extern "C" @@ -746,9 +752,18 @@ static GtkTreePath * wxgtk_tree_model_get_path (GtkTreeModel *tree_model, GtkTreeIter *iter) { - GtkWxTreeModel *wxtree_model = (GtkWxTreeModel *) tree_model; - g_return_val_if_fail (GTK_IS_WX_TREE_MODEL (wxtree_model), NULL); - g_return_val_if_fail (iter->stamp == GTK_WX_TREE_MODEL (wxtree_model)->stamp, NULL); + g_return_val_if_fail (GTK_IS_WX_TREE_MODEL (tree_model), NULL); + + GtkWxTreeModel *wxtree_model = GTK_WX_TREE_MODEL (tree_model); + if ( wxtree_model->stamp == 0 ) + { + // The model is temporarily invalid and can't be used, see Cleared(), + // but we need to return some valid path from here -- just return an + // empty one. + return gtk_tree_path_new(); + } + + g_return_val_if_fail (iter->stamp == wxtree_model->stamp, NULL); return wxtree_model->internal->get_path( iter ); } @@ -1891,12 +1906,25 @@ bool wxGtkDataViewModelNotifier::Cleared() GtkTreePath *path = gtk_tree_path_new_first(); // points to root + // It is important to avoid selection changed events being generated from + // here as they would reference the already deleted model items, which + // would result in crashes in any code attempting to handle these events. + wxDataViewCtrl::SelectionEventsSuppressor noSelection(m_internal->GetOwner()); + + // We also need to prevent wxGtkTreeCellDataFunc from using the model items + // not existing any longer, so change the model stamp to indicate that it + // temporarily can't be used. + const gint stampOrig = wxgtk_model->stamp; + wxgtk_model->stamp = 0; + int i; for (i = 0; i < count; i++) gtk_tree_model_row_deleted( GTK_TREE_MODEL(wxgtk_model), path ); gtk_tree_path_free( path ); + wxgtk_model->stamp = stampOrig; + m_internal->Cleared(); return true; @@ -3059,6 +3087,13 @@ static void wxGtkTreeCellDataFunc( GtkTreeViewColumn *WXUNUSED(column), g_return_if_fail (GTK_IS_WX_TREE_MODEL (model)); GtkWxTreeModel *tree_model = (GtkWxTreeModel *) model; + if ( !tree_model->stamp ) + { + // The model is temporarily invalid and can't be used, see the code in + // wxGtkDataViewModelNotifier::Cleared(). + return; + } + wxDataViewRenderer *cell = (wxDataViewRenderer*) data; wxDataViewItem item( (void*) iter->user_data ); From e1d8137e70ab7697a27b9439476e83ebd9cad1d0 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Jan 2018 23:08:15 +0100 Subject: [PATCH 4/5] Use wxGtkTreePath instead of calling gtk_tree_path_free() Use RAII helper class instead of freeing the object manually. Also add a couple of "const"s and other minor style fixes. No real changes. --- src/gtk/dataview.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/gtk/dataview.cpp b/src/gtk/dataview.cpp index ee482cd68c..91b7621fde 100644 --- a/src/gtk/dataview.cpp +++ b/src/gtk/dataview.cpp @@ -1902,10 +1902,6 @@ bool wxGtkDataViewModelNotifier::Cleared() // has been deleted so call row_deleted() for every // child of root... - int count = m_internal->iter_n_children( NULL ); // number of children of root - - GtkTreePath *path = gtk_tree_path_new_first(); // points to root - // It is important to avoid selection changed events being generated from // here as they would reference the already deleted model items, which // would result in crashes in any code attempting to handle these events. @@ -1917,11 +1913,12 @@ bool wxGtkDataViewModelNotifier::Cleared() const gint stampOrig = wxgtk_model->stamp; wxgtk_model->stamp = 0; - int i; - for (i = 0; i < count; i++) - gtk_tree_model_row_deleted( GTK_TREE_MODEL(wxgtk_model), path ); - - gtk_tree_path_free( path ); + { + wxGtkTreePath path(gtk_tree_path_new_first()); // points to root + const int count = m_internal->iter_n_children( NULL ); // number of children of root + for (int i = 0; i < count; i++) + gtk_tree_model_row_deleted( GTK_TREE_MODEL(wxgtk_model), path ); + } wxgtk_model->stamp = stampOrig; From ad03c8475d05f7f63a1a9082658100f78b93bb8f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 24 Jan 2018 18:00:29 +0100 Subject: [PATCH 5/5] Revert "Fix crash when deleting all wxTreeListCtrl items with wxGTK3" This reverts commit 1dd102d741dc9858df2b7e933761ed7cc7a51093 as it introduced a crash in the same method when using generic wxDataViewCtrl implementation, e.g. under MSW, and is not necessary to avoid the crash with wxGTK3 any longer after the few previous commits. --- src/generic/treelist.cpp | 8 ++------ tests/controls/treelistctrltest.cpp | 9 --------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/src/generic/treelist.cpp b/src/generic/treelist.cpp index 3c6f26d955..6c599100ae 100644 --- a/src/generic/treelist.cpp +++ b/src/generic/treelist.cpp @@ -572,16 +572,12 @@ void wxTreeListModel::DeleteItem(Node* item) void wxTreeListModel::DeleteAllItems() { - // Note that this must be called before actually deleting the items as - // clearing GTK+ wxDataViewCtrl results in SELECTION_CHANGED events being - // sent and these events contain pointers to the model items, so they must - // still be valid. - Cleared(); - while ( m_root->GetChild() ) { m_root->DeleteChild(); } + + Cleared(); } const wxString& wxTreeListModel::GetItemText(Node* item, unsigned col) const diff --git a/tests/controls/treelistctrltest.cpp b/tests/controls/treelistctrltest.cpp index 02164ab3a1..d4a3ac96d6 100644 --- a/tests/controls/treelistctrltest.cpp +++ b/tests/controls/treelistctrltest.cpp @@ -39,7 +39,6 @@ private: CPPUNIT_TEST( Traversal ); CPPUNIT_TEST( ItemText ); CPPUNIT_TEST( ItemCheck ); - CPPUNIT_TEST( DeleteAll ); CPPUNIT_TEST_SUITE_END(); // Create the control with the given style. @@ -56,7 +55,6 @@ private: void Traversal(); void ItemText(); void ItemCheck(); - void DeleteAll(); // The control itself. @@ -233,11 +231,4 @@ void TreeListCtrlTestCase::ItemCheck() m_treelist->GetCheckedState(m_code) ); } -void TreeListCtrlTestCase::DeleteAll() -{ - m_treelist->DeleteAllItems(); - - CHECK( !m_treelist->GetFirstChild(m_treelist->GetRootItem()).IsOk() ); -} - #endif // wxUSE_TREELISTCTRL