From 9c9fdb256971002c083a7f698c8de7f7c6a66b7b Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Tue, 4 Aug 2020 01:41:56 +0100 Subject: [PATCH] More performant (and more correct) deletion of DRC markers. Fixes https://gitlab.com/kicad/code/kicad/issues/5057 --- common/rc_item.cpp | 64 ++++++++++++++++++--------------- common/rc_item.h | 9 ++++- eeschema/dialogs/dialog_erc.cpp | 2 +- eeschema/erc_settings.cpp | 12 +++++++ eeschema/erc_settings.h | 2 ++ pcbnew/class_board.cpp | 22 ++++++++++++ pcbnew/class_board.h | 2 ++ pcbnew/cleanup_item.h | 11 ++++++ pcbnew/dialogs/dialog_drc.cpp | 2 +- pcbnew/drc/drc_provider.h | 21 +++++++++++ 10 files changed, 116 insertions(+), 31 deletions(-) diff --git a/common/rc_item.cpp b/common/rc_item.cpp index 9eaa781dc6..5ac85b83bf 100644 --- a/common/rc_item.cpp +++ b/common/rc_item.cpp @@ -387,17 +387,16 @@ void RC_TREE_MODEL::ValueChanged( RC_TREE_NODE* aNode ) void RC_TREE_MODEL::DeleteCurrentItem( bool aDeep ) { - DeleteItems( true, false, false, aDeep ); + DeleteItems( true, false, aDeep ); } -void RC_TREE_MODEL::DeleteItems( bool aCurrent, bool aWarningsAndErrors, bool aExclusions, - bool aDeep ) +void RC_TREE_MODEL::DeleteItems( bool aCurrentOnly, bool aIncludeExclusions, bool aDeep ) { RC_TREE_NODE* current_node = ToNode( m_view->GetCurrentItem() ); const RC_ITEM* current_item = current_node ? current_node->m_RcItem : nullptr; - if( !current_item && !aWarningsAndErrors && !aExclusions ) + if( aCurrentOnly && !current_item ) { wxBell(); return; @@ -407,33 +406,42 @@ void RC_TREE_MODEL::DeleteItems( bool aCurrent, bool aWarningsAndErrors, bool aE { RC_ITEM* rcItem = m_rcItemsProvider->GetItem( i ); MARKER_BASE* marker = rcItem->GetParent(); + bool excluded = marker ? marker->IsExcluded() : false; - if( ( aCurrent && rcItem == current_item ) - || ( aWarningsAndErrors && marker && !marker->IsExcluded() ) - || ( aExclusions && marker && marker->IsExcluded() ) ) + if( aCurrentOnly && rcItem != current_item ) + continue; + + if( excluded && !aIncludeExclusions ) + continue; + + wxDataViewItem markerItem = ToItem( m_tree[i] ); + wxDataViewItemArray childItems; + wxDataViewItem parentItem = ToItem( m_tree[i]->m_Parent ); + + for( RC_TREE_NODE* child : m_tree[i]->m_Children ) { - wxDataViewItem markerItem = ToItem( m_tree[i] ); - wxDataViewItemArray childItems; - wxDataViewItem parentItem = ToItem( m_tree[i]->m_Parent ); - - for( RC_TREE_NODE* child : m_tree[i]->m_Children ) - { - childItems.push_back( ToItem( child ) ); - delete child; - } - - m_tree[i]->m_Children.clear(); - ItemsDeleted( markerItem, childItems ); - - delete m_tree[i]; - m_tree.erase( m_tree.begin() + i ); - ItemDeleted( parentItem, markerItem ); - - m_rcItemsProvider->DeleteItem( i, aDeep ); - - if( !aWarningsAndErrors && !aExclusions ) - break; + childItems.push_back( ToItem( child ) ); + delete child; } + + m_tree[i]->m_Children.clear(); + ItemsDeleted( markerItem, childItems ); + + delete m_tree[i]; + m_tree.erase( m_tree.begin() + i ); + ItemDeleted( parentItem, markerItem ); + + // Only deep delete the current item here; others will be done through the + // DeleteAllItems() call below, which is more efficient. + m_rcItemsProvider->DeleteItem( i, aDeep && aCurrentOnly ); + + if( aCurrentOnly ) + break; + } + + if( !aCurrentOnly ) + { + m_rcItemsProvider->DeleteAllItems( aIncludeExclusions, aDeep ); } } diff --git a/common/rc_item.h b/common/rc_item.h index eab8cdefa0..dcd1f6bd6b 100644 --- a/common/rc_item.h +++ b/common/rc_item.h @@ -59,6 +59,8 @@ public: */ virtual void DeleteItem( int aIndex, bool aDeep ) = 0; + virtual void DeleteAllItems( bool aIncludeExclusions, bool aDeep ) = 0; + virtual ~RC_ITEMS_PROVIDER() { } }; @@ -272,7 +274,12 @@ public: void ValueChanged( RC_TREE_NODE* aNode ); void DeleteCurrentItem( bool aDeep ); - void DeleteItems( bool aCurrent, bool aWarningsAndErrors, bool aExclusions, bool aDeep ); + + /** + * Deletes the current item or all items. If all, \a aIncludeExclusions determines + * whether or not exclusions are also deleted. + */ + void DeleteItems( bool aCurrentOnly, bool aIncludeExclusions, bool aDeep ); private: void rebuildModel( RC_ITEMS_PROVIDER* aProvider, int aSeverities ); diff --git a/eeschema/dialogs/dialog_erc.cpp b/eeschema/dialogs/dialog_erc.cpp index 7924cf6ee3..e373859b0f 100644 --- a/eeschema/dialogs/dialog_erc.cpp +++ b/eeschema/dialogs/dialog_erc.cpp @@ -582,7 +582,7 @@ void DIALOG_ERC::deleteAllMarkers( bool aIncludeExclusions ) // Clear current selection list to avoid selection of deleted items m_parent->GetToolManager()->RunAction( EE_ACTIONS::clearSelection, true ); - m_markerTreeModel->DeleteItems( false, true, aIncludeExclusions, true ); + m_markerTreeModel->DeleteItems( false, true, aIncludeExclusions ); } diff --git a/eeschema/erc_settings.cpp b/eeschema/erc_settings.cpp index 829a8fb3e1..fa2c47e0cf 100644 --- a/eeschema/erc_settings.cpp +++ b/eeschema/erc_settings.cpp @@ -315,3 +315,15 @@ void SHEETLIST_ERC_ITEMS_PROVIDER::DeleteItem( int aIndex, bool aDeep ) screens.DeleteMarker( marker ); } } + + +void SHEETLIST_ERC_ITEMS_PROVIDER::DeleteAllItems( bool aIncludeExclusions, bool aDeep ) +{ + // Filtered list was already handled through DeleteItem() by the tree control + + if( aDeep ) + { + SCH_SCREENS screens( m_schematic->Root() ); + screens.DeleteAllMarkers( MARKER_BASE::MARKER_ERC, aIncludeExclusions ); + } +} diff --git a/eeschema/erc_settings.h b/eeschema/erc_settings.h index 17243a2a85..95c58fee22 100644 --- a/eeschema/erc_settings.h +++ b/eeschema/erc_settings.h @@ -190,6 +190,8 @@ public: ERC_ITEM* GetItem( int aIndex ) override; void DeleteItem( int aIndex, bool aDeep ) override; + + void DeleteAllItems( bool aIncludeExclusions, bool aDeep ) override; }; diff --git a/pcbnew/class_board.cpp b/pcbnew/class_board.cpp index 37704d9cd2..8fe71eee08 100644 --- a/pcbnew/class_board.cpp +++ b/pcbnew/class_board.cpp @@ -715,6 +715,28 @@ void BOARD::DeleteMARKERs() } +void BOARD::DeleteMARKERs( bool aWarningsAndErrors, bool aExclusions ) +{ + // Deleting lots of items from a vector can be very slow. Copy remaining items instead. + MARKERS remaining; + + for( MARKER_PCB* marker : m_markers ) + { + if( ( marker->IsExcluded() && aExclusions ) + || ( !marker->IsExcluded() && aWarningsAndErrors ) ) + { + delete marker; + } + else + { + remaining.push_back( marker ); + } + } + + m_markers = remaining; +} + + void BOARD::DeleteZONEOutlines() { // the vector does not know how to delete the ZONE Outlines, it holds pointers diff --git a/pcbnew/class_board.h b/pcbnew/class_board.h index d04efef384..e7f080eff6 100644 --- a/pcbnew/class_board.h +++ b/pcbnew/class_board.h @@ -367,6 +367,8 @@ public: */ void DeleteMARKERs(); + void DeleteMARKERs( bool aWarningsAndErrors, bool aExclusions ); + /** * Function DeleteZONEOutlines * deletes ALL zone outlines from the board. diff --git a/pcbnew/cleanup_item.h b/pcbnew/cleanup_item.h index e03655969f..8befcaa0d1 100644 --- a/pcbnew/cleanup_item.h +++ b/pcbnew/cleanup_item.h @@ -106,6 +106,17 @@ public: m_sourceVector->erase( m_sourceVector->begin() + aIndex ); } } + + void DeleteAllItems( bool aIncludeExclusions, bool aDeep ) override + { + if( aDeep ) + { + for( CLEANUP_ITEM* item : *m_sourceVector ) + delete item; + + m_sourceVector->clear(); + } + } }; diff --git a/pcbnew/dialogs/dialog_drc.cpp b/pcbnew/dialogs/dialog_drc.cpp index 099c39eba6..9e74b8e4f5 100644 --- a/pcbnew/dialogs/dialog_drc.cpp +++ b/pcbnew/dialogs/dialog_drc.cpp @@ -513,7 +513,7 @@ void DIALOG_DRC::deleteAllMarkers( bool aIncludeExclusions ) // Clear current selection list to avoid selection of deleted items m_brdEditor->GetToolManager()->RunAction( PCB_ACTIONS::selectionClear, true ); - m_markerTreeModel->DeleteItems( false, true, aIncludeExclusions, true ); + m_markerTreeModel->DeleteItems( false, true, aIncludeExclusions ); } diff --git a/pcbnew/drc/drc_provider.h b/pcbnew/drc/drc_provider.h index 4b14cdddeb..8721363595 100644 --- a/pcbnew/drc/drc_provider.h +++ b/pcbnew/drc/drc_provider.h @@ -159,6 +159,14 @@ public: if( aDeep ) m_board->Delete( marker ); } + + void DeleteAllItems( bool aIncludeExclusions, bool aDeep ) override + { + // Filtered list was already handled through DeleteItem() by the tree control + + if( aDeep ) + m_board->DeleteMARKERs( true, aIncludeExclusions ); + } }; @@ -246,6 +254,19 @@ public: } } } + + void DeleteAllItems( bool aIncludeExclusions, bool aDeep ) override + { + if( aDeep ) + { + for( DRC_ITEM* item : *m_sourceVector ) + delete item; + + m_sourceVector->clear(); + } + + m_filteredVector.clear(); // no ownership of DRC_ITEM pointers + } };