From 51ed404ca5ba76097e9f3dfc6f407456859dd7db Mon Sep 17 00:00:00 2001 From: Erik Sperling Johansen Date: Thu, 23 Mar 2017 23:48:09 +0800 Subject: [PATCH 01/10] Optimize sorting items in wxDataViewCtrl Limit node sorting to a minimum, by making all sorts happen on demand and utilizing already sorted child lists to do quicker insertions. --- src/generic/datavgen.cpp | 382 +++++++++++++++++++++++++++++---------- 1 file changed, 291 insertions(+), 91 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 9cc10f4e0e..f62622acaa 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -91,9 +91,6 @@ static const int EXPANDER_OFFSET = 1; // of the special values in this enum: enum { - // Sort when we're thawed later. - SortColumn_OnThaw = -3, - // Don't sort at all. SortColumn_None = -2, @@ -541,6 +538,8 @@ public: { m_branchData->open = !m_branchData->open; ChangeSubTreeCount(+sum); + // Sort the children if needed + Resort(); } } @@ -588,6 +587,7 @@ public: } void Resort(); + void MoveUpdatedChild(wxDataViewTreeNode * childNode); private: wxDataViewMainWindow * const m_window; @@ -602,7 +602,9 @@ private: { BranchNodeData() : open(false), - subTreeCount(0) + subTreeCount(0), + sortColumn(SortColumn_None), + sortAscending(false) { } @@ -613,6 +615,12 @@ private: // Is the branch node currently open (expanded)? bool open; + // By which column are the children currently sorted + int sortColumn; + + // Are the children currently sorted ascending or descending + bool sortAscending; + // Total count of expanded (i.e. visible with the help of some // scrolling) items in the subtree, but excluding this node. I.e. it is // 0 for leaves and is the number of rows the subtree occupies for @@ -660,19 +668,6 @@ public: UpdateDisplay(); } - // Override the base class method to resort if needed, i.e. if - // SortPrepare() was called -- and ignored -- while we were frozen. - virtual void DoThaw() wxOVERRIDE - { - if ( m_sortColumn == SortColumn_OnThaw ) - { - Resort(); - m_sortColumn = SortColumn_None; - } - - wxWindow::DoThaw(); - } - void SortPrepare() { wxDataViewModel* model = GetModel(); @@ -682,11 +677,7 @@ public: { if (model->HasDefaultCompare()) { - // See below for the explanation of IsFrozen() test. - if ( IsFrozen() ) - m_sortColumn = SortColumn_OnThaw; - else - m_sortColumn = SortColumn_Default; + m_sortColumn = SortColumn_Default; } else m_sortColumn = SortColumn_None; @@ -695,15 +686,6 @@ public: return; } - // Avoid sorting while the window is frozen, this allows to quickly add - // many items without resorting after each addition and only resort - // them all at once when the window is finally thawed, see above. - if ( IsFrozen() ) - { - m_sortColumn = SortColumn_OnThaw; - return; - } - m_sortColumn = col->GetModelColumn(); m_sortAscending = col->IsSortOrderAscending(); } @@ -1637,39 +1619,207 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) if (!m_branchData) m_branchData = new BranchNodeData; - m_branchData->children.Insert(node, index); + g_column = m_window->GetSortColumn(); + g_model = m_window->GetModel(); + g_asending = m_window->IsAscendingSort(); - // TODO: insert into sorted array directly in O(log n) instead of resorting in O(n log n) - if ((g_column = m_window->GetSortColumn()) >= -1) + // Flag indicating whether we should retain existing sorted list when + // inserting the child node. + bool insertSorted = false; + + if ( g_column == SortColumn_None ) { - g_model = m_window->GetModel(); - g_asending = m_window->IsAscendingSort(); + // We should insert assuming an unsorted list. This will cause the + // child list to lose the current sort order, if any. + m_branchData->sortColumn = SortColumn_None; + m_branchData->sortAscending = true; + } + else if ( m_branchData->open ) + { + // The child list will always be correctly sorted for open nodes. + // The sole exception is the root node, on the first child + // addition. + if ( ( m_parent == NULL ) && m_branchData->children.IsEmpty()) + { + m_branchData->sortColumn = g_column; + m_branchData->sortAscending = g_asending; + } + else + { + wxASSERT(m_branchData->sortColumn == g_column); + wxASSERT(m_branchData->sortAscending == g_asending); + } - m_branchData->children.Sort(&wxGenericTreeModelNodeCmp); + // We retain the sorted child list + insertSorted = true; + } + else if ( m_branchData->children.IsEmpty() ) + { + // We're inserting the first child of a closed node. We can choose + // whether to consider this empty child list sorted or unsorted. + // By choosing unsorted, we postpone comparisons until the parent + // node is opened in the view, which may be never. + m_branchData->sortColumn = SortColumn_None; + m_branchData->sortAscending = true; + } + else if ( (m_branchData->sortColumn == g_column) && (m_branchData->sortAscending == g_asending) ) + { + // The children are already sorted by the correct criteria. This + // means the node has at some point in time been open. Even though + // the node is closed now, we insert sorted to avoid a later resort. + insertSorted = true; + } + else + { + // The children aren't sorted by the correct criteria, so we just + // insert unsorted. + m_branchData->sortColumn = SortColumn_None; + m_branchData->sortAscending = true; + } + + + if ( insertSorted ) + { + if ( m_branchData->children.IsEmpty() ) + { + m_branchData->children.Insert(node, 0); + } + else + { + // Insert directly into sorted array + int lo = 0, hi = m_branchData->children.size(); + while ( lo < hi ) + { + int mid = lo + (hi - lo) / 2; + int r = wxGenericTreeModelNodeCmp(&node, &m_branchData->children[mid]); + if ( r < 0 ) + hi = mid; + else if ( r > 0 ) + lo = mid + 1; + else + lo = hi = mid; + } + m_branchData->children.Insert(node, lo); + + } + } + else + { + m_branchData->children.Insert(node, index); } } + void wxDataViewTreeNode::Resort() { if (!m_branchData) return; - if ((g_column = m_window->GetSortColumn()) >= -1) - { - wxDataViewTreeNodes& nodes = m_branchData->children; + // No reason to sort a closed node. + if ( !m_branchData->open ) + return; + g_column = m_window->GetSortColumn(); + if ( g_column != SortColumn_None ) + { g_model = m_window->GetModel(); g_asending = m_window->IsAscendingSort(); - nodes.Sort(&wxGenericTreeModelNodeCmp); - int len = nodes.GetCount(); - for (int i = 0; i < len; i++) + wxDataViewTreeNodes& nodes = m_branchData->children; + + // Only sort the children if they aren't already sorted by the wanted criteria + if ( (g_column != m_branchData->sortColumn) || (g_asending != m_branchData->sortAscending) ) { - if (nodes[i]->HasChildren()) + nodes.Sort(&wxGenericTreeModelNodeCmp); + m_branchData->sortColumn = g_column; + m_branchData->sortAscending = g_asending; + } + + // There may be open child nodes that also need a resort. + int len = nodes.GetCount(); + for ( int i = 0; i < len; i++ ) + { + if ( nodes[i]->HasChildren() ) nodes[i]->Resort(); } } } + +void wxDataViewTreeNode::MoveUpdatedChild(wxDataViewTreeNode * childNode) +{ + // The childNode has changed, and may need to be moved to another location + // in the sorted child list. + + if ( !m_branchData ) + return; + if ( !m_branchData->open ) + return; + if ( m_branchData->sortColumn == SortColumn_None ) + return; + + g_column = m_window->GetSortColumn(); + g_model = m_window->GetModel(); + g_asending = m_window->IsAscendingSort(); + + wxASSERT(g_column == m_branchData->sortColumn); + wxASSERT(g_asending == m_branchData->sortAscending); + + wxDataViewTreeNodes& nodes = m_branchData->children; + + // First find the node in the current child list + int hi = nodes.size(); + int oldLocation = -1; + for ( int index = 0; index < hi; ++index ) + { + if ( nodes[index] == childNode ) + { + oldLocation = index; + break; + } + } + wxASSERT(oldLocation >= 0); + + if ( oldLocation < 0 ) + return; + + // Check if we actually need to move the node. + bool locationChanged = false; + + if ( oldLocation > 0 ) + { + if (wxGenericTreeModelNodeCmp(&nodes[oldLocation - 1], &childNode) > 0 ) + locationChanged = true; + } + if ( !locationChanged && (oldLocation + 1 < static_cast(nodes.size())) ) + { + if ( wxGenericTreeModelNodeCmp(&childNode, &nodes[oldLocation + 1]) > 0 ) + locationChanged = true; + } + + + if ( locationChanged ) + { + // Remove and reinsert the node in the child list + nodes.RemoveAt(oldLocation); + hi = nodes.size(); + int lo = 0; + while ( lo < hi ) + { + int mid = lo + (hi - lo) / 2; + int r = wxGenericTreeModelNodeCmp(&childNode, &m_branchData->children[mid]); + if ( r < 0 ) + hi = mid; + else if ( r > 0 ) + lo = mid + 1; + else + lo = hi = mid; + } + nodes.Insert(childNode, lo); + } + +} + + //----------------------------------------------------------------------------- // wxDataViewMainWindow //----------------------------------------------------------------------------- @@ -2536,60 +2686,70 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData if ( !parentNode ) return false; - wxDataViewItemArray modelSiblings; - GetModel()->GetChildren(parent, modelSiblings); - const int modelSiblingsSize = modelSiblings.size(); - - int posInModel = modelSiblings.Index(item, /*fromEnd=*/true); - wxCHECK_MSG( posInModel != wxNOT_FOUND, false, "adding non-existent item?" ); - wxDataViewTreeNode *itemNode = new wxDataViewTreeNode(this, parentNode, item); itemNode->SetHasChildren(GetModel()->IsContainer(item)); - parentNode->SetHasChildren(true); - const wxDataViewTreeNodes& nodeSiblings = parentNode->GetChildNodes(); - const int nodeSiblingsSize = nodeSiblings.size(); - - int nodePos = 0; - - if ( posInModel == modelSiblingsSize - 1 ) + if ( m_sortColumn == SortColumn_None ) { - nodePos = nodeSiblingsSize; - } - else if ( modelSiblingsSize == nodeSiblingsSize + 1 ) - { - // This is the simple case when our node tree already matches the - // model and only this one item is missing. - nodePos = posInModel; + // There's no sorting, so we need to select an insertion position + + wxDataViewItemArray modelSiblings; + GetModel()->GetChildren(parent, modelSiblings); + const int modelSiblingsSize = modelSiblings.size(); + + int posInModel = modelSiblings.Index(item, /*fromEnd=*/true); + wxCHECK_MSG(posInModel != wxNOT_FOUND, false, "adding non-existent item?"); + + + const wxDataViewTreeNodes& nodeSiblings = parentNode->GetChildNodes(); + const int nodeSiblingsSize = nodeSiblings.size(); + + int nodePos = 0; + + if ( posInModel == modelSiblingsSize - 1 ) + { + nodePos = nodeSiblingsSize; + } + else if ( modelSiblingsSize == nodeSiblingsSize + 1 ) + { + // This is the simple case when our node tree already matches the + // model and only this one item is missing. + nodePos = posInModel; + } + else + { + // It's possible that a larger discrepancy between the model and + // our realization exists. This can happen e.g. when adding a bunch + // of items to the model and then calling ItemsAdded() just once + // afterwards. In this case, we must find the right position by + // looking at sibling items. + + // append to the end if we won't find a better position: + nodePos = nodeSiblingsSize; + + for ( int nextItemPos = posInModel + 1; + nextItemPos < modelSiblingsSize; + nextItemPos++ ) + { + int nextNodePos = parentNode->FindChildByItem(modelSiblings[nextItemPos]); + if ( nextNodePos != wxNOT_FOUND ) + { + nodePos = nextNodePos; + break; + } + } + } + parentNode->ChangeSubTreeCount(+1); + parentNode->InsertChild(itemNode, nodePos); } else { - // It's possible that a larger discrepancy between the model and - // our realization exists. This can happen e.g. when adding a bunch - // of items to the model and then calling ItemsAdded() just once - // afterwards. In this case, we must find the right position by - // looking at sibling items. - - // append to the end if we won't find a better position: - nodePos = nodeSiblingsSize; - - for ( int nextItemPos = posInModel + 1; - nextItemPos < modelSiblingsSize; - nextItemPos++ ) - { - int nextNodePos = parentNode->FindChildByItem(modelSiblings[nextItemPos]); - if ( nextNodePos != wxNOT_FOUND ) - { - nodePos = nextNodePos; - break; - } - } + // Node list is or will be sorted, so InsertChild do not need insertion position + parentNode->ChangeSubTreeCount(+1); + parentNode->InsertChild(itemNode, 0); } - parentNode->ChangeSubTreeCount(+1); - parentNode->InsertChild(itemNode, nodePos); - InvalidateCount(); } @@ -2716,7 +2876,16 @@ bool wxDataViewMainWindow::ItemDeleted(const wxDataViewItem& parent, bool wxDataViewMainWindow::ItemChanged(const wxDataViewItem & item) { SortPrepare(); - GetModel()->Resort(); + + // Instead of resorting the parent, just move this specific child node. + wxDataViewItem parentItem = GetModel()->GetParent(item); + wxDataViewTreeNode * parentNode = FindNode(parentItem); + if ( parentNode ) + { + wxDataViewTreeNode * node = FindNode(item); + wxASSERT(node); + parentNode->MoveUpdatedChild(node); + } GetOwner()->InvalidateColBestWidths(); @@ -2724,6 +2893,8 @@ bool wxDataViewMainWindow::ItemChanged(const wxDataViewItem & item) wxDataViewEvent le(wxEVT_DATAVIEW_ITEM_VALUE_CHANGED, m_owner, item); m_owner->ProcessWindowEvent(le); + // Make sure the change is actually shown right away + UpdateDisplay(); return true; } @@ -2743,7 +2914,30 @@ bool wxDataViewMainWindow::ValueChanged( const wxDataViewItem & item, unsigned i return true; */ SortPrepare(); - GetModel()->Resort(); + + // Instead of resorting the parent, just move this specific child node. + + // It is possible to skip the call to MoveUpdatedChild if the + // modified column is not the sort column, but in real world + // applications it's fully possible and likely that custom + // compare uses not only the selected model column but also + // falls back to other values for comparison. To ensure + // consistency it is better to treat a value change as if it + // was an item change. + + // Alternatively, one could require the user to use ItemChanged + // rather than ValueChanged when the changed value may affect + // sorting, and add a check for model_column vs sort column + // here. + + wxDataViewItem parentItem = GetModel()->GetParent(item); + wxDataViewTreeNode * parentNode = FindNode(parentItem); + if ( parentNode ) + { + wxDataViewTreeNode * node = FindNode(item); + wxASSERT(node); + parentNode->MoveUpdatedChild(node); + } GetOwner()->InvalidateColBestWidth(view_column); @@ -2752,6 +2946,9 @@ bool wxDataViewMainWindow::ValueChanged( const wxDataViewItem & item, unsigned i wxDataViewEvent le(wxEVT_DATAVIEW_ITEM_VALUE_CHANGED, m_owner, column, item); m_owner->ProcessWindowEvent(le); + // Make sure the change is actually shown right away + UpdateDisplay(); + return true; } @@ -3390,12 +3587,15 @@ void wxDataViewMainWindow::Expand( unsigned int row ) return; } + + // ToggleOpen needs sorting to be prepared, in case the child list + // isn't sorted correctly. + SortPrepare(); node->ToggleOpen(); // build the children of current node if( node->GetChildNodes().empty() ) { - SortPrepare(); ::BuildTreeHelper(this, GetModel(), node->GetItem(), node); } From eef994016a204df7d136ea4f5e9c43f444529871 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 16 Dec 2017 17:30:11 +0100 Subject: [PATCH 02/10] Reorder BranchNodeData struct members to make it more compact By simply putting two bool fields consecutively to each other, we reduce the number of bytes of padding used from 6 to 2 in a typical 32 bit build, saving 4 (or 8, in 64 bits) bytes per branch item. --- src/generic/datavgen.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index f62622acaa..b4a993f888 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -615,12 +615,12 @@ private: // Is the branch node currently open (expanded)? bool open; - // By which column are the children currently sorted - int sortColumn; - // Are the children currently sorted ascending or descending bool sortAscending; + // By which column are the children currently sorted + int sortColumn; + // Total count of expanded (i.e. visible with the help of some // scrolling) items in the subtree, but excluding this node. I.e. it is // 0 for leaves and is the number of rows the subtree occupies for From 8d9f0e6fef89100f66888ff7c09591e178504331 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sat, 16 Dec 2017 17:32:53 +0100 Subject: [PATCH 03/10] Initialize BranchNodeData members in declaration order Doing it out of order is harmless here, but would result gcc -Wreorder warnings. --- src/generic/datavgen.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index b4a993f888..aaa097ca68 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -602,9 +602,9 @@ private: { BranchNodeData() : open(false), - subTreeCount(0), + sortAscending(false), sortColumn(SortColumn_None), - sortAscending(false) + subTreeCount(0) { } From 0268c062c0d14d9028908e5d9b10f45ee26d29ae Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 17:23:42 +0100 Subject: [PATCH 04/10] Minor improvements to wxDataViewCtrl sorting optimization commit Simplify some code and make it more clear, notably by changing conditionals to be easier to follow. Also avoid repeating the same functions calls (like SortPrepare() or UpdateDisplay()) by folding them into the functions that actually need them to be done. --- src/generic/datavgen.cpp | 229 +++++++++++++++++++-------------------- 1 file changed, 111 insertions(+), 118 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index aaa097ca68..5408794848 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -587,9 +587,23 @@ public: } void Resort(); - void MoveUpdatedChild(wxDataViewTreeNode * childNode); + + // Should be called after changing the item value to update its position in + // the control if necessary. + void PutInSortOrder() + { + if ( m_parent ) + m_parent->PutChildInSortOrder(this); + } private: + // Called by the child after it has been updated to put it in the right + // place among its siblings, depending on the sort order. + // + // The argument must be non-null, but is passed as a pointer as it's + // inserted into m_branchData->children. + void PutChildInSortOrder(wxDataViewTreeNode* childNode); + wxDataViewMainWindow * const m_window; wxDataViewTreeNode *m_parent; @@ -673,21 +687,20 @@ public: wxDataViewModel* model = GetModel(); wxDataViewColumn* col = GetOwner()->GetSortingColumn(); - if( !col ) + if ( col ) + { + m_sortColumn = col->GetModelColumn(); + m_sortAscending = col->IsSortOrderAscending(); + } + else { if (model->HasDefaultCompare()) - { m_sortColumn = SortColumn_Default; - } else m_sortColumn = SortColumn_None; m_sortAscending = true; - return; } - - m_sortColumn = col->GetModelColumn(); - m_sortAscending = col->IsSortOrderAscending(); } void SetOwner( wxDataViewCtrl* owner ) { m_owner = owner; } @@ -1619,6 +1632,8 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) if (!m_branchData) m_branchData = new BranchNodeData; + m_window->SortPrepare(); + g_column = m_window->GetSortColumn(); g_model = m_window->GetModel(); g_asending = m_window->IsAscendingSort(); @@ -1636,21 +1651,23 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) } else if ( m_branchData->open ) { - // The child list will always be correctly sorted for open nodes. - // The sole exception is the root node, on the first child - // addition. - if ( ( m_parent == NULL ) && m_branchData->children.IsEmpty()) + // For open branches, children should be already sorted, with one + // possible exception that we check for here: + if ( m_branchData->sortColumn != g_column || + m_branchData->sortAscending != g_asending ) { + // This can happen for the root node, on the first child addition, + // in which case the children are nevertheless sorted (because + // there is only one of them), but we need to set the correct sort + // order. + wxASSERT_MSG( !m_parent && m_branchData->children.IsEmpty(), + "Logic error in wxDVC sorting code" ); + m_branchData->sortColumn = g_column; m_branchData->sortAscending = g_asending; } - else - { - wxASSERT(m_branchData->sortColumn == g_column); - wxASSERT(m_branchData->sortAscending == g_asending); - } - // We retain the sorted child list + // We can use fast insertion. insertSorted = true; } else if ( m_branchData->children.IsEmpty() ) @@ -1662,11 +1679,13 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) m_branchData->sortColumn = SortColumn_None; m_branchData->sortAscending = true; } - else if ( (m_branchData->sortColumn == g_column) && (m_branchData->sortAscending == g_asending) ) + else if ( (m_branchData->sortColumn == g_column) && + (m_branchData->sortAscending == g_asending) ) { - // The children are already sorted by the correct criteria. This - // means the node has at some point in time been open. Even though - // the node is closed now, we insert sorted to avoid a later resort. + // The children are already sorted by the correct criteria (because + // the node must have been opened in the same time in the past). Even + // though it is closed now, we still insert in sort order to avoid a + // later resort. insertSorted = true; } else @@ -1680,28 +1699,20 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) if ( insertSorted ) { - if ( m_branchData->children.IsEmpty() ) + // Use binary search to find the correct position to insert at. + int lo = 0, hi = m_branchData->children.size(); + while ( lo < hi ) { - m_branchData->children.Insert(node, 0); - } - else - { - // Insert directly into sorted array - int lo = 0, hi = m_branchData->children.size(); - while ( lo < hi ) - { - int mid = lo + (hi - lo) / 2; - int r = wxGenericTreeModelNodeCmp(&node, &m_branchData->children[mid]); - if ( r < 0 ) - hi = mid; - else if ( r > 0 ) - lo = mid + 1; - else - lo = hi = mid; - } - m_branchData->children.Insert(node, lo); - + int mid = lo + (hi - lo) / 2; + int r = wxGenericTreeModelNodeCmp(&node, &m_branchData->children[mid]); + if ( r < 0 ) + hi = mid; + else if ( r > 0 ) + lo = mid + 1; + else + lo = hi = mid; } + m_branchData->children.Insert(node, lo); } else { @@ -1726,8 +1737,10 @@ void wxDataViewTreeNode::Resort() g_asending = m_window->IsAscendingSort(); wxDataViewTreeNodes& nodes = m_branchData->children; - // Only sort the children if they aren't already sorted by the wanted criteria - if ( (g_column != m_branchData->sortColumn) || (g_asending != m_branchData->sortAscending) ) + // Only sort the children if they aren't already sorted by the wanted + // criteria. + if ( (g_column != m_branchData->sortColumn) || + (g_asending != m_branchData->sortAscending) ) { nodes.Sort(&wxGenericTreeModelNodeCmp); m_branchData->sortColumn = g_column; @@ -1745,7 +1758,7 @@ void wxDataViewTreeNode::Resort() } -void wxDataViewTreeNode::MoveUpdatedChild(wxDataViewTreeNode * childNode) +void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) { // The childNode has changed, and may need to be moved to another location // in the sorted child list. @@ -1757,6 +1770,15 @@ void wxDataViewTreeNode::MoveUpdatedChild(wxDataViewTreeNode * childNode) if ( m_branchData->sortColumn == SortColumn_None ) return; + wxDataViewTreeNodes& nodes = m_branchData->children; + + // This is more than an optimization, the code below assumes that 1 is a + // valid index. + if ( nodes.size() == 1 ) + return; + + m_window->SortPrepare(); + g_column = m_window->GetSortColumn(); g_model = m_window->GetModel(); g_asending = m_window->IsAscendingSort(); @@ -1764,11 +1786,9 @@ void wxDataViewTreeNode::MoveUpdatedChild(wxDataViewTreeNode * childNode) wxASSERT(g_column == m_branchData->sortColumn); wxASSERT(g_asending == m_branchData->sortAscending); - wxDataViewTreeNodes& nodes = m_branchData->children; - // First find the node in the current child list int hi = nodes.size(); - int oldLocation = -1; + int oldLocation = wxNOT_FOUND; for ( int index = 0; index < hi; ++index ) { if ( nodes[index] == childNode ) @@ -1777,46 +1797,47 @@ void wxDataViewTreeNode::MoveUpdatedChild(wxDataViewTreeNode * childNode) break; } } - wxASSERT(oldLocation >= 0); - - if ( oldLocation < 0 ) - return; + wxCHECK_RET( oldLocation >= 0, "not our child?" ); // Check if we actually need to move the node. bool locationChanged = false; - if ( oldLocation > 0 ) + if ( oldLocation == 0 ) { - if (wxGenericTreeModelNodeCmp(&nodes[oldLocation - 1], &childNode) > 0 ) + // Compare with the next item (as we return early in the case of only a + // single child, we know that there is one) to check if the item is now + // out of order. + if ( wxGenericTreeModelNodeCmp(&childNode, &nodes[1]) > 0 ) locationChanged = true; } - if ( !locationChanged && (oldLocation + 1 < static_cast(nodes.size())) ) + else // Compare with the previous item. { - if ( wxGenericTreeModelNodeCmp(&childNode, &nodes[oldLocation + 1]) > 0 ) + if ( wxGenericTreeModelNodeCmp(&nodes[oldLocation - 1], &childNode) > 0 ) locationChanged = true; } + if ( !locationChanged ) + return; - if ( locationChanged ) + // Remove and reinsert the node in the child list + nodes.RemoveAt(oldLocation); + hi = nodes.size(); + int lo = 0; + while ( lo < hi ) { - // Remove and reinsert the node in the child list - nodes.RemoveAt(oldLocation); - hi = nodes.size(); - int lo = 0; - while ( lo < hi ) - { - int mid = lo + (hi - lo) / 2; - int r = wxGenericTreeModelNodeCmp(&childNode, &m_branchData->children[mid]); - if ( r < 0 ) - hi = mid; - else if ( r > 0 ) - lo = mid + 1; - else - lo = hi = mid; - } - nodes.Insert(childNode, lo); + int mid = lo + (hi - lo) / 2; + int r = wxGenericTreeModelNodeCmp(&childNode, &m_branchData->children[mid]); + if ( r < 0 ) + hi = mid; + else if ( r > 0 ) + lo = mid + 1; + else + lo = hi = mid; } + nodes.Insert(childNode, lo); + // Make sure the change is actually shown right away + m_window->UpdateDisplay(); } @@ -2679,8 +2700,6 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData } else { - SortPrepare(); - wxDataViewTreeNode *parentNode = FindNode(parent); if ( !parentNode ) @@ -2875,17 +2894,10 @@ bool wxDataViewMainWindow::ItemDeleted(const wxDataViewItem& parent, bool wxDataViewMainWindow::ItemChanged(const wxDataViewItem & item) { - SortPrepare(); - - // Instead of resorting the parent, just move this specific child node. - wxDataViewItem parentItem = GetModel()->GetParent(item); - wxDataViewTreeNode * parentNode = FindNode(parentItem); - if ( parentNode ) - { - wxDataViewTreeNode * node = FindNode(item); - wxASSERT(node); - parentNode->MoveUpdatedChild(node); - } + // Move this node to its new correct place after it was updated. + wxDataViewTreeNode* const node = FindNode(item); + wxCHECK_MSG( node, false, "invalid item" ); + node->PutInSortOrder(); GetOwner()->InvalidateColBestWidths(); @@ -2893,8 +2905,6 @@ bool wxDataViewMainWindow::ItemChanged(const wxDataViewItem & item) wxDataViewEvent le(wxEVT_DATAVIEW_ITEM_VALUE_CHANGED, m_owner, item); m_owner->ProcessWindowEvent(le); - // Make sure the change is actually shown right away - UpdateDisplay(); return true; } @@ -2913,31 +2923,21 @@ bool wxDataViewMainWindow::ValueChanged( const wxDataViewItem & item, unsigned i return true; */ - SortPrepare(); - // Instead of resorting the parent, just move this specific child node. + // In principle, we could skip the call to PutInSortOrder() if the modified + // column is not the sort column, but in real-world applications it's fully + // possible and likely that custom compare uses not only the selected model + // column but also falls back to other values for comparison. To ensure + // consistency it is better to treat a value change as if it was an item + // change. + // + // Alternatively, one could require the user to use ItemChanged() rather + // than ValueChanged() when the changed value may affect sorting, and add a + // check for model_column vs sort column here. - // It is possible to skip the call to MoveUpdatedChild if the - // modified column is not the sort column, but in real world - // applications it's fully possible and likely that custom - // compare uses not only the selected model column but also - // falls back to other values for comparison. To ensure - // consistency it is better to treat a value change as if it - // was an item change. - - // Alternatively, one could require the user to use ItemChanged - // rather than ValueChanged when the changed value may affect - // sorting, and add a check for model_column vs sort column - // here. - - wxDataViewItem parentItem = GetModel()->GetParent(item); - wxDataViewTreeNode * parentNode = FindNode(parentItem); - if ( parentNode ) - { - wxDataViewTreeNode * node = FindNode(item); - wxASSERT(node); - parentNode->MoveUpdatedChild(node); - } + wxDataViewTreeNode* const node = FindNode(item); + wxCHECK_MSG( node, false, "invalid item" ); + node->PutInSortOrder(); GetOwner()->InvalidateColBestWidth(view_column); @@ -2946,9 +2946,6 @@ bool wxDataViewMainWindow::ValueChanged( const wxDataViewItem & item, unsigned i wxDataViewEvent le(wxEVT_DATAVIEW_ITEM_VALUE_CHANGED, m_owner, column, item); m_owner->ProcessWindowEvent(le); - // Make sure the change is actually shown right away - UpdateDisplay(); - return true; } @@ -3587,10 +3584,6 @@ void wxDataViewMainWindow::Expand( unsigned int row ) return; } - - // ToggleOpen needs sorting to be prepared, in case the child list - // isn't sorted correctly. - SortPrepare(); node->ToggleOpen(); // build the children of current node From 259a3578244864976cbb90ceb6317913dbdc883f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 17:31:09 +0100 Subject: [PATCH 05/10] Use index, not value, when removing items from wxDataViewTreeNode This avoids another (linear) search among the item siblings, as we can use the index we've just found instead directly. --- src/generic/datavgen.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 5408794848..0da29eba68 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -471,10 +471,10 @@ public: void InsertChild(wxDataViewTreeNode *node, unsigned index); - void RemoveChild(wxDataViewTreeNode *node) + void RemoveChild(unsigned index) { wxCHECK_RET( m_branchData != NULL, "leaf node doesn't have children" ); - m_branchData->children.Remove(node); + m_branchData->children.RemoveAt(index); } // returns position of child node for given item in children list or wxNOT_FOUND @@ -2835,7 +2835,7 @@ bool wxDataViewMainWindow::ItemDeleted(const wxDataViewItem& parent, // Delete the item from wxDataViewTreeNode representation: const int itemsDeleted = 1 + itemNode->GetSubTreeCount(); - parentNode->RemoveChild(itemNode); + parentNode->RemoveChild(itemPosInNode); delete itemNode; parentNode->ChangeSubTreeCount(-itemsDeleted); From a7c68663fd25d10cd152f8460d8c6e14089f19cd Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 17:43:11 +0100 Subject: [PATCH 06/10] Replace macro-based items array with wxVector in wxDataViewCtrl No real changes, just try to minimize the use of the old macro-based containers as there is really no reason to do it here. --- src/generic/datavgen.cpp | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 0da29eba68..55de3a6eb2 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -35,6 +35,7 @@ #include "wx/msgdlg.h" #include "wx/dcscreen.h" #include "wx/frame.h" + #include "wx/vector.h" #endif #include "wx/stockitem.h" @@ -422,7 +423,8 @@ public: class wxDataViewMainWindow; class wxDataViewTreeNode; -WX_DEFINE_ARRAY( wxDataViewTreeNode *, wxDataViewTreeNodes ); + +typedef wxVector wxDataViewTreeNodes; class wxDataViewTreeNode { @@ -474,7 +476,7 @@ public: void RemoveChild(unsigned index) { wxCHECK_RET( m_branchData != NULL, "leaf node doesn't have children" ); - m_branchData->children.RemoveAt(index); + m_branchData->RemoveChild(index); } // returns position of child node for given item in children list or wxNOT_FOUND @@ -525,7 +527,7 @@ public: int sum = 0; const wxDataViewTreeNodes& nodes = m_branchData->children; - const int len = nodes.GetCount(); + const int len = nodes.size(); for ( int i = 0;i < len; i ++) sum += 1 + nodes[i]->GetSubTreeCount(); @@ -622,6 +624,16 @@ private: { } + void InsertChild(wxDataViewTreeNode* node, unsigned index) + { + children.insert(children.begin() + index, node); + } + + void RemoveChild(unsigned index) + { + children.erase(children.begin() + index); + } + // Child nodes. Note that this may be empty even if m_hasChildren in // case this branch of the tree wasn't expanded and realized yet. wxDataViewTreeNodes children; @@ -1660,7 +1672,7 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) // in which case the children are nevertheless sorted (because // there is only one of them), but we need to set the correct sort // order. - wxASSERT_MSG( !m_parent && m_branchData->children.IsEmpty(), + wxASSERT_MSG( !m_parent && m_branchData->children.empty(), "Logic error in wxDVC sorting code" ); m_branchData->sortColumn = g_column; @@ -1670,7 +1682,7 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) // We can use fast insertion. insertSorted = true; } - else if ( m_branchData->children.IsEmpty() ) + else if ( m_branchData->children.empty() ) { // We're inserting the first child of a closed node. We can choose // whether to consider this empty child list sorted or unsorted. @@ -1712,11 +1724,11 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) else lo = hi = mid; } - m_branchData->children.Insert(node, lo); + m_branchData->InsertChild(node, lo); } else { - m_branchData->children.Insert(node, index); + m_branchData->InsertChild(node, index); } } @@ -1742,13 +1754,15 @@ void wxDataViewTreeNode::Resort() if ( (g_column != m_branchData->sortColumn) || (g_asending != m_branchData->sortAscending) ) { - nodes.Sort(&wxGenericTreeModelNodeCmp); + qsort(&m_branchData->children[0], m_branchData->children.size(), + sizeof(m_branchData->children[0]), + (CMPFUNC)&wxGenericTreeModelNodeCmp); m_branchData->sortColumn = g_column; m_branchData->sortAscending = g_asending; } // There may be open child nodes that also need a resort. - int len = nodes.GetCount(); + int len = nodes.size(); for ( int i = 0; i < len; i++ ) { if ( nodes[i]->HasChildren() ) @@ -1820,7 +1834,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) return; // Remove and reinsert the node in the child list - nodes.RemoveAt(oldLocation); + m_branchData->RemoveChild(oldLocation); hi = nodes.size(); int lo = 0; while ( lo < hi ) @@ -1834,7 +1848,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) else lo = hi = mid; } - nodes.Insert(childNode, lo); + m_branchData->InsertChild(childNode, lo); // Make sure the change is actually shown right away m_window->UpdateDisplay(); @@ -3700,7 +3714,7 @@ wxDataViewTreeNode * wxDataViewMainWindow::FindNode( const wxDataViewItem & item const wxDataViewTreeNodes& nodes = node->GetChildNodes(); bool found = false; - for (unsigned i = 0; i < nodes.GetCount(); ++i) + for (unsigned i = 0; i < nodes.size(); ++i) { wxDataViewTreeNode* currentNode = nodes[i]; if (currentNode->GetItem() == parentChain[iter]) From 5365f3563e2800aac27fc47551da399cd47f30ea Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 18:40:14 +0100 Subject: [PATCH 07/10] Get rid of global sort-related variables in wxDataViewCtrl Having to set up global variables before (re)sorting the items was too ugly and became even more so after the latest changes optimizing item sorting as sorting is done in more places now. Improve the code by introducing a SortOrder class instead of dealing with the sort column and sort order direction separately and, especially, by using std::sort() (which should be fine to use here, considering that it's used since quite some time in wxArrayString implementation) with a comparator object instead of qsort(), which doesn't allow passing any data to the sort callback otherwise than via the global variables. No changes in behaviour. --- src/generic/datavgen.cpp | 208 +++++++++++++++++++++------------------ 1 file changed, 113 insertions(+), 95 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 55de3a6eb2..cb77a75e71 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -67,7 +67,7 @@ class wxDataViewHeaderWindow; class wxDataViewCtrl; //----------------------------------------------------------------------------- -// classes +// constants //----------------------------------------------------------------------------- static const int SCROLL_UNIT_X = 15; @@ -84,9 +84,8 @@ static const int EXPANDER_OFFSET = 4; static const int EXPANDER_OFFSET = 1; #endif -// Below is the compare stuff. -// For the generic implementation, both the leaf nodes and the nodes are sorted for -// fast search when needed +namespace +{ // The column is either the index of the column to be used for sorting or one // of the special values in this enum: @@ -99,13 +98,46 @@ enum SortColumn_Default = -1 }; +// A class storing the definition of sort order used, as a column index and +// sort direction by this column. +// +// Notice that the sort order may be invalid, meaning that items shouldn't be +// sorted. +class SortOrder +{ +public: + explicit SortOrder(int column = SortColumn_None, bool ascending = true) + : m_column(column), + m_ascending(ascending) + { + } + + // Default copy ctor, assignment operator and dtor are all OK. + + bool IsNone() const { return m_column == SortColumn_None; } + + int GetColumn() const { return m_column; } + bool IsAscending() const { return m_ascending; } + + bool operator==(const SortOrder& other) const + { + return m_column == other.m_column && m_ascending == other.m_ascending; + } + + bool operator!=(const SortOrder& other) const + { + return !(*this == other); + } + +private: + int m_column; + bool m_ascending; +}; + // ---------------------------------------------------------------------------- // helper functions // ---------------------------------------------------------------------------- -namespace -{ - // Return the expander column or, if it is not set, the first column and also // set it as the expander one for the future. wxDataViewColumn* GetExpanderColumnOrFirstOne(wxDataViewCtrl* dataview) @@ -618,8 +650,6 @@ private: { BranchNodeData() : open(false), - sortAscending(false), - sortColumn(SortColumn_None), subTreeCount(0) { } @@ -638,15 +668,12 @@ private: // case this branch of the tree wasn't expanded and realized yet. wxDataViewTreeNodes children; + // Order in which children are sorted (possibly none). + SortOrder sortOrder; + // Is the branch node currently open (expanded)? bool open; - // Are the children currently sorted ascending or descending - bool sortAscending; - - // By which column are the children currently sorted - int sortColumn; - // Total count of expanded (i.e. visible with the help of some // scrolling) items in the subtree, but excluding this node. I.e. it is // 0 for leaves and is the number of rows the subtree occupies for @@ -688,30 +715,25 @@ public: { if (!IsVirtualList()) { - SortPrepare(); m_root->Resort(); } UpdateDisplay(); } - void SortPrepare() + SortOrder GetSortOrder() const { - wxDataViewModel* model = GetModel(); - - wxDataViewColumn* col = GetOwner()->GetSortingColumn(); + wxDataViewColumn* const col = GetOwner()->GetSortingColumn(); if ( col ) { - m_sortColumn = col->GetModelColumn(); - m_sortAscending = col->IsSortOrderAscending(); + return SortOrder(col->GetModelColumn(), + col->IsSortOrderAscending()); } else { - if (model->HasDefaultCompare()) - m_sortColumn = SortColumn_Default; + if (GetModel()->HasDefaultCompare()) + return SortOrder(SortColumn_Default); else - m_sortColumn = SortColumn_None; - - m_sortAscending = true; + return SortOrder(); } } @@ -852,9 +874,6 @@ public: return FindColumnForEditing(item, wxDATAVIEW_CELL_EDITABLE) != NULL; } - int GetSortColumn() const { return m_sortColumn; } - bool IsAscendingSort() const { return m_sortAscending; } - private: void InvalidateCount() { m_count = -1; } void UpdateCount(int count) @@ -923,10 +942,6 @@ private: // This is the tree node under the cursor wxDataViewTreeNode * m_underMouse; - // sorted column + extra flags - int m_sortColumn; - bool m_sortAscending; - // The control used for editing or NULL. wxWeakRef m_editorCtrl; @@ -1624,49 +1639,68 @@ void wxDataViewRenameTimer::Notify() // wxDataViewTreeNode // ---------------------------------------------------------------------------- -// Used by wxGenericTreeModelNodeCmp sort callback only. -// -// This is not thread safe but it should be fine if all events are handled in -// one thread. -static wxDataViewModel* g_model; -static int g_column; -static bool g_asending; - -int LINKAGEMODE wxGenericTreeModelNodeCmp(wxDataViewTreeNode ** node1, - wxDataViewTreeNode ** node2) +namespace { - return g_model->Compare((*node1)->GetItem(), (*node2)->GetItem(), g_column, g_asending); -} +// Comparator used for sorting the tree nodes using the model-defined sort +// order and also for performing binary search in our own code. +class wxGenericTreeModelNodeCmp +{ +public: + wxGenericTreeModelNodeCmp(wxDataViewMainWindow* window, + const SortOrder& sortOrder) + : m_model(window->GetModel()), + m_sortOrder(sortOrder) + { + wxASSERT_MSG( !m_sortOrder.IsNone(), "should have sort order" ); + } + + // Return negative, zero or positive value depending on whether the first + // item is less than, equal to or greater than the second one. + int Compare(wxDataViewTreeNode* first, wxDataViewTreeNode* second) const + { + return m_model->Compare(first->GetItem(), second->GetItem(), + m_sortOrder.GetColumn(), + m_sortOrder.IsAscending()); + } + + // Return true if the items are in order, i.e. the first item is less than + // or equal (because it's useless to exchange them in this case) than the + // second one. This is used by std::sort(). + bool operator()(wxDataViewTreeNode* first, wxDataViewTreeNode* second) const + { + return Compare(first, second) <= 0; + } + +private: + wxDataViewModel* const m_model; + const SortOrder m_sortOrder; +}; + +} // anonymous namespace void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) { if (!m_branchData) m_branchData = new BranchNodeData; - m_window->SortPrepare(); - - g_column = m_window->GetSortColumn(); - g_model = m_window->GetModel(); - g_asending = m_window->IsAscendingSort(); + const SortOrder sortOrder = m_window->GetSortOrder(); // Flag indicating whether we should retain existing sorted list when // inserting the child node. bool insertSorted = false; - if ( g_column == SortColumn_None ) + if ( sortOrder.IsNone() ) { // We should insert assuming an unsorted list. This will cause the // child list to lose the current sort order, if any. - m_branchData->sortColumn = SortColumn_None; - m_branchData->sortAscending = true; + m_branchData->sortOrder = SortOrder(); } else if ( m_branchData->open ) { // For open branches, children should be already sorted, with one // possible exception that we check for here: - if ( m_branchData->sortColumn != g_column || - m_branchData->sortAscending != g_asending ) + if ( m_branchData->sortOrder != sortOrder ) { // This can happen for the root node, on the first child addition, // in which case the children are nevertheless sorted (because @@ -1675,8 +1709,7 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) wxASSERT_MSG( !m_parent && m_branchData->children.empty(), "Logic error in wxDVC sorting code" ); - m_branchData->sortColumn = g_column; - m_branchData->sortAscending = g_asending; + m_branchData->sortOrder = sortOrder; } // We can use fast insertion. @@ -1688,11 +1721,9 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) // whether to consider this empty child list sorted or unsorted. // By choosing unsorted, we postpone comparisons until the parent // node is opened in the view, which may be never. - m_branchData->sortColumn = SortColumn_None; - m_branchData->sortAscending = true; + m_branchData->sortOrder = SortOrder(); } - else if ( (m_branchData->sortColumn == g_column) && - (m_branchData->sortAscending == g_asending) ) + else if ( m_branchData->sortOrder == sortOrder ) { // The children are already sorted by the correct criteria (because // the node must have been opened in the same time in the past). Even @@ -1704,19 +1735,19 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) { // The children aren't sorted by the correct criteria, so we just // insert unsorted. - m_branchData->sortColumn = SortColumn_None; - m_branchData->sortAscending = true; + m_branchData->sortOrder = SortOrder(); } if ( insertSorted ) { // Use binary search to find the correct position to insert at. + wxGenericTreeModelNodeCmp cmp(m_window, sortOrder); int lo = 0, hi = m_branchData->children.size(); while ( lo < hi ) { int mid = lo + (hi - lo) / 2; - int r = wxGenericTreeModelNodeCmp(&node, &m_branchData->children[mid]); + int r = cmp.Compare(node, m_branchData->children[mid]); if ( r < 0 ) hi = mid; else if ( r > 0 ) @@ -1742,23 +1773,20 @@ void wxDataViewTreeNode::Resort() if ( !m_branchData->open ) return; - g_column = m_window->GetSortColumn(); - if ( g_column != SortColumn_None ) + const SortOrder sortOrder = m_window->GetSortOrder(); + if ( !sortOrder.IsNone() ) { - g_model = m_window->GetModel(); - g_asending = m_window->IsAscendingSort(); wxDataViewTreeNodes& nodes = m_branchData->children; // Only sort the children if they aren't already sorted by the wanted // criteria. - if ( (g_column != m_branchData->sortColumn) || - (g_asending != m_branchData->sortAscending) ) + if ( m_branchData->sortOrder != sortOrder ) { - qsort(&m_branchData->children[0], m_branchData->children.size(), - sizeof(m_branchData->children[0]), - (CMPFUNC)&wxGenericTreeModelNodeCmp); - m_branchData->sortColumn = g_column; - m_branchData->sortAscending = g_asending; + std::sort(m_branchData->children.begin(), + m_branchData->children.end(), + wxGenericTreeModelNodeCmp(m_window, sortOrder)); + + m_branchData->sortOrder = sortOrder; } // There may be open child nodes that also need a resort. @@ -1781,7 +1809,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) return; if ( !m_branchData->open ) return; - if ( m_branchData->sortColumn == SortColumn_None ) + if ( m_branchData->sortOrder.IsNone() ) return; wxDataViewTreeNodes& nodes = m_branchData->children; @@ -1791,14 +1819,8 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) if ( nodes.size() == 1 ) return; - m_window->SortPrepare(); - - g_column = m_window->GetSortColumn(); - g_model = m_window->GetModel(); - g_asending = m_window->IsAscendingSort(); - - wxASSERT(g_column == m_branchData->sortColumn); - wxASSERT(g_asending == m_branchData->sortAscending); + // We should already be sorted in the right order. + wxASSERT(m_branchData->sortOrder == m_window->GetSortOrder()); // First find the node in the current child list int hi = nodes.size(); @@ -1813,6 +1835,8 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) } wxCHECK_RET( oldLocation >= 0, "not our child?" ); + wxGenericTreeModelNodeCmp cmp(m_window, m_branchData->sortOrder); + // Check if we actually need to move the node. bool locationChanged = false; @@ -1821,12 +1845,12 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) // Compare with the next item (as we return early in the case of only a // single child, we know that there is one) to check if the item is now // out of order. - if ( wxGenericTreeModelNodeCmp(&childNode, &nodes[1]) > 0 ) + if ( !cmp(childNode, nodes[1]) ) locationChanged = true; } else // Compare with the previous item. { - if ( wxGenericTreeModelNodeCmp(&nodes[oldLocation - 1], &childNode) > 0 ) + if ( !cmp(nodes[oldLocation - 1], childNode) ) locationChanged = true; } @@ -1840,7 +1864,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) while ( lo < hi ) { int mid = lo + (hi - lo) / 2; - int r = wxGenericTreeModelNodeCmp(&childNode, &m_branchData->children[mid]); + int r = cmp.Compare(childNode, m_branchData->children[mid]); if ( r < 0 ) hi = mid; else if ( r > 0 ) @@ -1943,9 +1967,6 @@ wxDataViewMainWindow::wxDataViewMainWindow( wxDataViewCtrl *parent, wxWindowID i m_count = -1; m_underMouse = NULL; - m_sortColumn = SortColumn_None; - m_sortAscending = true; - UpdateDisplay(); } @@ -2723,7 +2744,7 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData itemNode->SetHasChildren(GetModel()->IsContainer(item)); parentNode->SetHasChildren(true); - if ( m_sortColumn == SortColumn_None ) + if ( GetSortOrder().IsNone() ) { // There's no sorting, so we need to select an insertion position @@ -2971,7 +2992,6 @@ bool wxDataViewMainWindow::Cleared() if (GetModel()) { - SortPrepare(); BuildTree( GetModel() ); } else @@ -3707,7 +3727,6 @@ wxDataViewTreeNode * wxDataViewMainWindow::FindNode( const wxDataViewItem & item // Even though the item is a container, it doesn't have any // child nodes in the control's representation yet. We have // to realize its subtree now. - SortPrepare(); ::BuildTreeHelper(this, model, node->GetItem(), node); } @@ -3955,7 +3974,6 @@ void wxDataViewMainWindow::BuildTree(wxDataViewModel * model) // First we define a invalid item to fetch the top-level elements wxDataViewItem item; - SortPrepare(); BuildTreeHelper(this, model, item, m_root); InvalidateCount(); } From 3f41e84830e522e2d5340f2d109c953359dd8a8b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 18:59:36 +0100 Subject: [PATCH 08/10] Remove unused wxDataViewSelection dynamic array macro This should have been done in 36a5983f64c0f9adfdc67b5c11fe44671cea1ab5 but was forgotten and the unused array remained in the code. --- src/generic/datavgen.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index cb77a75e71..9c8736cb78 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -690,8 +690,6 @@ private: // wxDataViewMainWindow //----------------------------------------------------------------------------- -WX_DEFINE_SORTED_ARRAY_SIZE_T(unsigned int, wxDataViewSelection); - class wxDataViewMainWindow: public wxWindow { public: From 1bfe42b0ce02ab63cdc53e012cb6c553a261861e Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 19:09:04 +0100 Subject: [PATCH 09/10] Simplify sort order handling for first child item Test whether this is the first child before testing whether the branch is open as this allows to avoid the special case of inserting the first child under the root node and simplifies the assert condition to a simple check that the sort order is already what we expect. --- src/generic/datavgen.cpp | 42 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 9c8736cb78..3750116e64 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -1694,33 +1694,33 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) // child list to lose the current sort order, if any. m_branchData->sortOrder = SortOrder(); } - else if ( m_branchData->open ) + else if ( m_branchData->children.empty() ) { - // For open branches, children should be already sorted, with one - // possible exception that we check for here: - if ( m_branchData->sortOrder != sortOrder ) + if ( m_branchData->open ) { - // This can happen for the root node, on the first child addition, - // in which case the children are nevertheless sorted (because - // there is only one of them), but we need to set the correct sort - // order. - wxASSERT_MSG( !m_parent && m_branchData->children.empty(), - "Logic error in wxDVC sorting code" ); - + // We don't need to search for the right place to insert the first + // item (there is only one), but we do need to remember the sort + // order to use for the subsequent ones. m_branchData->sortOrder = sortOrder; } + else + { + // We're inserting the first child of a closed node. We can choose + // whether to consider this empty child list sorted or unsorted. + // By choosing unsorted, we postpone comparisons until the parent + // node is opened in the view, which may be never. + m_branchData->sortOrder = SortOrder(); + } + } + else if ( m_branchData->open ) + { + // For open branches, children should be already sorted. + wxASSERT_MSG( m_branchData->sortOrder == sortOrder, + wxS("Logic error in wxDVC sorting code") ); // We can use fast insertion. insertSorted = true; } - else if ( m_branchData->children.empty() ) - { - // We're inserting the first child of a closed node. We can choose - // whether to consider this empty child list sorted or unsorted. - // By choosing unsorted, we postpone comparisons until the parent - // node is opened in the view, which may be never. - m_branchData->sortOrder = SortOrder(); - } else if ( m_branchData->sortOrder == sortOrder ) { // The children are already sorted by the correct criteria (because @@ -1731,8 +1731,8 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) } else { - // The children aren't sorted by the correct criteria, so we just - // insert unsorted. + // The children of this closed node aren't sorted by the correct + // criteria, so we just insert unsorted. m_branchData->sortOrder = SortOrder(); } From 1dbe3dafe4b3a2fcc6134ed24abf21d7faa3851f Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 17 Dec 2017 19:16:34 +0100 Subject: [PATCH 10/10] Stop storing window pointer in all wxDataViewTreeNode objects This didn't make any sense, all these objects reachable from the given root node are used by the same window, so storing the same pointer in all of them just wasted memory unnecessarily. Avoid this by passing wxDataViewMainWindow pointer explicitly to those methods of wxDataViewTreeNode that need it. No changes in behaviour, this is just a (memory use) optimization. --- src/generic/datavgen.cpp | 79 ++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/src/generic/datavgen.cpp b/src/generic/datavgen.cpp index 3750116e64..82885fdc41 100644 --- a/src/generic/datavgen.cpp +++ b/src/generic/datavgen.cpp @@ -461,11 +461,8 @@ typedef wxVector wxDataViewTreeNodes; class wxDataViewTreeNode { public: - wxDataViewTreeNode(wxDataViewMainWindow *window, - wxDataViewTreeNode *parent, - const wxDataViewItem& item) - : m_window(window), - m_parent(parent), + wxDataViewTreeNode(wxDataViewTreeNode *parent, const wxDataViewItem& item) + : m_parent(parent), m_item(item), m_branchData(NULL) { @@ -487,9 +484,9 @@ public: } } - static wxDataViewTreeNode* CreateRootNode(wxDataViewMainWindow *w) + static wxDataViewTreeNode* CreateRootNode() { - wxDataViewTreeNode *n = new wxDataViewTreeNode(w, NULL, wxDataViewItem()); + wxDataViewTreeNode *n = new wxDataViewTreeNode(NULL, wxDataViewItem()); n->m_branchData = new BranchNodeData; n->m_branchData->open = true; return n; @@ -503,7 +500,8 @@ public: return m_branchData->children; } - void InsertChild(wxDataViewTreeNode *node, unsigned index); + void InsertChild(wxDataViewMainWindow* window, + wxDataViewTreeNode *node, unsigned index); void RemoveChild(unsigned index) { @@ -547,7 +545,7 @@ public: return m_branchData && m_branchData->open; } - void ToggleOpen() + void ToggleOpen(wxDataViewMainWindow* window) { // We do not allow the (invisible) root node to be collapsed because // there is no way to expand it again. @@ -573,7 +571,7 @@ public: m_branchData->open = !m_branchData->open; ChangeSubTreeCount(+sum); // Sort the children if needed - Resort(); + Resort(window); } } @@ -620,14 +618,14 @@ public: m_parent->ChangeSubTreeCount(num); } - void Resort(); + void Resort(wxDataViewMainWindow* window); // Should be called after changing the item value to update its position in // the control if necessary. - void PutInSortOrder() + void PutInSortOrder(wxDataViewMainWindow* window) { if ( m_parent ) - m_parent->PutChildInSortOrder(this); + m_parent->PutChildInSortOrder(window, this); } private: @@ -636,9 +634,9 @@ private: // // The argument must be non-null, but is passed as a pointer as it's // inserted into m_branchData->children. - void PutChildInSortOrder(wxDataViewTreeNode* childNode); + void PutChildInSortOrder(wxDataViewMainWindow* window, + wxDataViewTreeNode* childNode); - wxDataViewMainWindow * const m_window; wxDataViewTreeNode *m_parent; // Corresponding model item. @@ -713,7 +711,7 @@ public: { if (!IsVirtualList()) { - m_root->Resort(); + m_root->Resort(this); } UpdateDisplay(); } @@ -1677,12 +1675,13 @@ private: } // anonymous namespace -void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) +void wxDataViewTreeNode::InsertChild(wxDataViewMainWindow* window, + wxDataViewTreeNode *node, unsigned index) { if (!m_branchData) m_branchData = new BranchNodeData; - const SortOrder sortOrder = m_window->GetSortOrder(); + const SortOrder sortOrder = window->GetSortOrder(); // Flag indicating whether we should retain existing sorted list when // inserting the child node. @@ -1740,7 +1739,7 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) if ( insertSorted ) { // Use binary search to find the correct position to insert at. - wxGenericTreeModelNodeCmp cmp(m_window, sortOrder); + wxGenericTreeModelNodeCmp cmp(window, sortOrder); int lo = 0, hi = m_branchData->children.size(); while ( lo < hi ) { @@ -1762,7 +1761,7 @@ void wxDataViewTreeNode::InsertChild(wxDataViewTreeNode *node, unsigned index) } -void wxDataViewTreeNode::Resort() +void wxDataViewTreeNode::Resort(wxDataViewMainWindow* window) { if (!m_branchData) return; @@ -1771,7 +1770,7 @@ void wxDataViewTreeNode::Resort() if ( !m_branchData->open ) return; - const SortOrder sortOrder = m_window->GetSortOrder(); + const SortOrder sortOrder = window->GetSortOrder(); if ( !sortOrder.IsNone() ) { wxDataViewTreeNodes& nodes = m_branchData->children; @@ -1782,7 +1781,7 @@ void wxDataViewTreeNode::Resort() { std::sort(m_branchData->children.begin(), m_branchData->children.end(), - wxGenericTreeModelNodeCmp(m_window, sortOrder)); + wxGenericTreeModelNodeCmp(window, sortOrder)); m_branchData->sortOrder = sortOrder; } @@ -1792,13 +1791,15 @@ void wxDataViewTreeNode::Resort() for ( int i = 0; i < len; i++ ) { if ( nodes[i]->HasChildren() ) - nodes[i]->Resort(); + nodes[i]->Resort(window); } } } -void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) +void +wxDataViewTreeNode::PutChildInSortOrder(wxDataViewMainWindow* window, + wxDataViewTreeNode* childNode) { // The childNode has changed, and may need to be moved to another location // in the sorted child list. @@ -1818,7 +1819,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) return; // We should already be sorted in the right order. - wxASSERT(m_branchData->sortOrder == m_window->GetSortOrder()); + wxASSERT(m_branchData->sortOrder == window->GetSortOrder()); // First find the node in the current child list int hi = nodes.size(); @@ -1833,7 +1834,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) } wxCHECK_RET( oldLocation >= 0, "not our child?" ); - wxGenericTreeModelNodeCmp cmp(m_window, m_branchData->sortOrder); + wxGenericTreeModelNodeCmp cmp(window, m_branchData->sortOrder); // Check if we actually need to move the node. bool locationChanged = false; @@ -1873,7 +1874,7 @@ void wxDataViewTreeNode::PutChildInSortOrder(wxDataViewTreeNode* childNode) m_branchData->InsertChild(childNode, lo); // Make sure the change is actually shown right away - m_window->UpdateDisplay(); + window->UpdateDisplay(); } @@ -1959,7 +1960,7 @@ wxDataViewMainWindow::wxDataViewMainWindow( wxDataViewCtrl *parent, wxWindowID i // TODO: maybe there is something system colour to use m_penExpander = wxPen(wxColour(0,0,0)); - m_root = wxDataViewTreeNode::CreateRootNode(this); + m_root = wxDataViewTreeNode::CreateRootNode(); // Make m_count = -1 will cause the class recaculate the real displaying number of rows. m_count = -1; @@ -2738,7 +2739,7 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData if ( !parentNode ) return false; - wxDataViewTreeNode *itemNode = new wxDataViewTreeNode(this, parentNode, item); + wxDataViewTreeNode *itemNode = new wxDataViewTreeNode(parentNode, item); itemNode->SetHasChildren(GetModel()->IsContainer(item)); parentNode->SetHasChildren(true); @@ -2793,13 +2794,13 @@ bool wxDataViewMainWindow::ItemAdded(const wxDataViewItem & parent, const wxData } } parentNode->ChangeSubTreeCount(+1); - parentNode->InsertChild(itemNode, nodePos); + parentNode->InsertChild(this, itemNode, nodePos); } else { // Node list is or will be sorted, so InsertChild do not need insertion position parentNode->ChangeSubTreeCount(+1); - parentNode->InsertChild(itemNode, 0); + parentNode->InsertChild(this, itemNode, 0); } InvalidateCount(); @@ -2886,7 +2887,7 @@ bool wxDataViewMainWindow::ItemDeleted(const wxDataViewItem& parent, // If it's still a container, make sure we show "+" icon for it // and not "-" one as there is nothing to collapse any more. if ( parentNode->IsOpen() ) - parentNode->ToggleOpen(); + parentNode->ToggleOpen(this); } } @@ -2930,7 +2931,7 @@ bool wxDataViewMainWindow::ItemChanged(const wxDataViewItem & item) // Move this node to its new correct place after it was updated. wxDataViewTreeNode* const node = FindNode(item); wxCHECK_MSG( node, false, "invalid item" ); - node->PutInSortOrder(); + node->PutInSortOrder(this); GetOwner()->InvalidateColBestWidths(); @@ -2970,7 +2971,7 @@ bool wxDataViewMainWindow::ValueChanged( const wxDataViewItem & item, unsigned i wxDataViewTreeNode* const node = FindNode(item); wxCHECK_MSG( node, false, "invalid item" ); - node->PutInSortOrder(); + node->PutInSortOrder(this); GetOwner()->InvalidateColBestWidth(view_column); @@ -3616,7 +3617,7 @@ void wxDataViewMainWindow::Expand( unsigned int row ) return; } - node->ToggleOpen(); + node->ToggleOpen(this); // build the children of current node if( node->GetChildNodes().empty() ) @@ -3672,7 +3673,7 @@ void wxDataViewMainWindow::Collapse(unsigned int row) SendSelectionChangedEvent(GetItemByRow(row)); } - node->ToggleOpen(); + node->ToggleOpen(this); // Adjust the current row if necessary. if ( m_currentRow > row ) @@ -3946,12 +3947,12 @@ static void BuildTreeHelper( wxDataViewMainWindow *window, const wxDataViewModel for ( unsigned int index = 0; index < num; index++ ) { - wxDataViewTreeNode *n = new wxDataViewTreeNode(window, node, children[index]); + wxDataViewTreeNode *n = new wxDataViewTreeNode(node, children[index]); if( model->IsContainer(children[index]) ) n->SetHasChildren( true ); - node->InsertChild(n, index); + node->InsertChild(window, n, index); } wxASSERT( node->IsOpen() ); @@ -3968,7 +3969,7 @@ void wxDataViewMainWindow::BuildTree(wxDataViewModel * model) return; } - m_root = wxDataViewTreeNode::CreateRootNode(this); + m_root = wxDataViewTreeNode::CreateRootNode(); // First we define a invalid item to fetch the top-level elements wxDataViewItem item;