From 6efdb3b337990cac9766a69475a287c8c92449bf Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 19 Oct 2020 01:24:55 +0200 Subject: [PATCH 1/3] Put generic wxDataViewCtrl helpers in an anonymous namespace No real changes, just don't pollute the global namespace unnecessarily. --- src/generic/datavgen.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index fb83cd49cb..d259e93509 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -2993,6 +2993,10 @@ void wxDataViewHeaderWindow::FinishEditing() //----------------------------------------------------------------------------- // Helper class for do operation on the tree node //----------------------------------------------------------------------------- + +namespace +{ + class DoJob { public: @@ -3040,6 +3044,8 @@ bool Walker( wxDataViewTreeNode * node, DoJob & func ) return false; } +} // anonymous namespace + bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxDataViewItem & item) { if (IsVirtualList()) From c24dddc4628fe5d4d2420c763e601ea77a7a3faf Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 19 Oct 2020 01:25:44 +0200 Subject: [PATCH 2/3] Return empty rectangle from GetItemRect() for collapsed items GetItemByRow() returned a valid row even for an item which was collapsed which is clearly inappropriate for its use in GetItemRect(), which is supposed return an invalid rectangle if the item is not visible. It also might be inappropriate in other cases, but this is not totally clear and it seems like it is supposed to return a valid row even for collapsed items at least sometimes, so just make its behaviour conditional by adding a new flags parameter to GetItemRect() and to Walker() helper used by it itself, so that it could skip over collapsed items. Update the test to show that it succeeds now even when the item is present in the tree, as it only passed before because the item had never been expanded at all, and so wasn't really present in the tree structure and the updated test would have failed without the changes to the code in this commit. --- src/generic/datavgen.cpp | 25 ++++++++++++++++++------- tests/controls/dataviewctrltest.cpp | 6 ++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index d259e93509..25a28ae657 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -74,6 +74,13 @@ static const int PADDING_RIGHTLEFT = 3; namespace { +// Flags for Walker() function defined below. +enum WalkFlags +{ + Walk_All, // Visit all items. + Walk_ExpandedOnly // Visit only expanded items. +}; + // The column is either the index of the column to be used for sorting or one // of the special values in this enum: enum @@ -854,7 +861,8 @@ public: // Some useful functions for row and item mapping wxDataViewItem GetItemByRow( unsigned int row ) const; - int GetRowByItem( const wxDataViewItem & item ) const; + int GetRowByItem( const wxDataViewItem & item, + WalkFlags flags = Walk_All ) const; wxDataViewTreeNode * GetTreeNodeByRow( unsigned int row ) const; // We did not need this temporarily @@ -3014,7 +3022,8 @@ public: virtual int operator() ( wxDataViewTreeNode * node ) = 0; }; -bool Walker( wxDataViewTreeNode * node, DoJob & func ) +bool +Walker(wxDataViewTreeNode * node, DoJob & func, WalkFlags flags = Walk_All) { wxCHECK_MSG( node, false, "can't walk NULL node" ); @@ -3028,7 +3037,7 @@ bool Walker( wxDataViewTreeNode * node, DoJob & func ) break; } - if ( node->HasChildren() ) + if ( node->HasChildren() && (flags != Walk_ExpandedOnly || node->IsOpen()) ) { const wxDataViewTreeNodes& nodes = node->GetChildNodes(); @@ -3036,7 +3045,7 @@ bool Walker( wxDataViewTreeNode * node, DoJob & func ) i != nodes.end(); ++i ) { - if ( Walker(*i, func) ) + if ( Walker(*i, func, flags) ) return true; } } @@ -4143,7 +4152,7 @@ wxRect wxDataViewMainWindow::GetItemRect( const wxDataViewItem & item, xpos = 0; } - const int row = GetRowByItem(item); + const int row = GetRowByItem(item, Walk_ExpandedOnly); if ( row == -1 ) { // This means the row is currently not visible at all. @@ -4244,7 +4253,9 @@ private: }; -int wxDataViewMainWindow::GetRowByItem(const wxDataViewItem & item) const +int +wxDataViewMainWindow::GetRowByItem(const wxDataViewItem & item, + WalkFlags flags) const { const wxDataViewModel * model = GetModel(); if( model == NULL ) @@ -4274,7 +4285,7 @@ int wxDataViewMainWindow::GetRowByItem(const wxDataViewItem & item) const // the parent chain was created by adding the deepest parent first. // so if we want to start at the root node, we have to iterate backwards through the vector ItemToRowJob job( item, parentChain.rbegin() ); - if ( !Walker( m_root, job ) ) + if ( !Walker( m_root, job, flags ) ) return -1; return job.GetResult(); diff --git a/tests/controls/dataviewctrltest.cpp b/tests/controls/dataviewctrltest.cpp index c88eac22ca..2f88a0aa17 100644 --- a/tests/controls/dataviewctrltest.cpp +++ b/tests/controls/dataviewctrltest.cpp @@ -297,6 +297,12 @@ TEST_CASE_METHOD(SingleSelectDataViewCtrlTestCase, CHECK( rect1.y < rect2.y ); } + // This forces generic implementation to add m_grandchild to the tree, as + // it does it only on demand. We want the item to really be there to check + // that GetItemRect() returns an empty rectangle for collapsed items. + m_dvc->Expand(m_child1); + m_dvc->Collapse(m_child1); + const wxRect rectNotShown = m_dvc->GetItemRect(m_grandchild); CHECK( rectNotShown == wxRect() ); From 931b47234213e2f5dfa387506c77ced3864324e1 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 19 Oct 2020 01:32:49 +0200 Subject: [PATCH 3/3] Add items to generic wxDataViewCtrl immediately in ItemAdded() This reverts the changes of 4dc78a33e0 (Fix adding items to a never opened branch of generic wxDataViewCtrl, 2018-01-17) which was supposed to fix a problem which doesn't seem to be reproducible any longer (and, unfortunately, wasn't recorded in the commit message back then), but introduced another problem instead: postponing adding the node to the tree didn't work correctly in case of depth-first model traversal in the user code calling ItemAdded() for all the items, as in this case the next call to ItemAdded() for a child of the item that wasn't added to the tree because its parent hadn't been opened yet, would result in adding this item and all its siblings to the tree when FindNode(parent) would be called. And this, in turn, would result in the siblings of this item being added to the tree twice. Adding a test reproducing this problem is difficult, as we don't have any way to check the internal state of wxDataViewMainWindow from the outside, so this commit still doesn't do this, unfortunately. Closes #18405. --- src/generic/datavgen.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 25a28ae657..2dfd7a953c 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -3075,14 +3075,6 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData parentNode->SetHasChildren(true); - // If the parent node isn't and hadn't been opened yet, we don't have - // anything to do here, all the items will be added to it when it's - // opened for the first time. - if ( !parentNode->IsOpen() && parentNode->GetChildNodes().empty() ) - { - return true; - } - wxDataViewTreeNode *itemNode = new wxDataViewTreeNode(parentNode, item); itemNode->SetHasChildren(GetModel()->IsContainer(item));