Remove non-const access to board-owned items

(with a few things const-casted for now)

pcbnew/board.cpp

@@ -1086,6 +1086,90 @@ void BOARD::Remove( BOARD_ITEM* aBoardItem, REMOVE_MODE aRemoveMode )
 }
 
 
+void BOARD::RemoveAll( std::initializer_list<KICAD_T> aTypes )
+{
+    std::vector<BOARD_ITEM*> removed;
+
+    if( aTypes.size() == 1 && *aTypes.begin() == TYPE_NOT_INIT )
+    {
+        aTypes = {
+            PCB_NETINFO_T, PCB_MARKER_T, PCB_GROUP_T, PCB_ZONE_T, PCB_GENERATOR_T, PCB_FOOTPRINT_T,
+            PCB_TRACE_T, PCB_SHAPE_T
+        };
+    }
+
+    for( const KICAD_T& type : aTypes )
+    {
+        switch( type )
+        {
+        case PCB_NETINFO_T:
+            std::copy( m_NetInfo.begin(), m_NetInfo.end(), std::back_inserter( removed ) );
+            m_NetInfo.clear();
+            break;
+
+        case PCB_MARKER_T:
+            std::copy( m_markers.begin(), m_markers.end(), std::back_inserter( removed ) );
+            m_markers.clear();
+            break;
+
+        case PCB_GROUP_T:
+            std::copy( m_groups.begin(), m_groups.end(), std::back_inserter( removed ) );
+            m_groups.clear();
+            break;
+
+        case PCB_ZONE_T:
+            std::copy( m_zones.begin(), m_zones.end(), std::back_inserter( removed ) );
+            m_zones.clear();
+            break;
+
+        case PCB_GENERATOR_T:
+            std::copy( m_generators.begin(), m_generators.end(), std::back_inserter( removed ) );
+            m_generators.clear();
+            break;
+
+        case PCB_FOOTPRINT_T:
+            std::copy( m_footprints.begin(), m_footprints.end(), std::back_inserter( removed ) );
+            m_footprints.clear();
+            break;
+
+        case PCB_TRACE_T:
+            std::copy( m_tracks.begin(), m_tracks.end(), std::back_inserter( removed ) );
+            m_tracks.clear();
+            break;
+
+        case PCB_ARC_T:
+        case PCB_VIA_T:
+            wxFAIL_MSG( wxT( "Use PCB_TRACE_T to remove all tracks, arcs, and vias" ) );
+            break;
+
+        case PCB_SHAPE_T:
+            std::copy( m_drawings.begin(), m_drawings.end(), std::back_inserter( removed ) );
+            m_drawings.clear();
+            break;
+
+        case PCB_DIM_ALIGNED_T:
+        case PCB_DIM_CENTER_T:
+        case PCB_DIM_RADIAL_T:
+        case PCB_DIM_ORTHOGONAL_T:
+        case PCB_DIM_LEADER_T:
+        case PCB_REFERENCE_IMAGE_T:
+        case PCB_FIELD_T:
+        case PCB_TEXT_T:
+        case PCB_TEXTBOX_T:
+        case PCB_TABLE_T:
+        case PCB_TARGET_T:
+            wxFAIL_MSG( wxT( "Use PCB_SHAPE_T to remove all graphics and text" ) );
+            break;
+
+        default:
+            wxFAIL_MSG( wxT( "BOARD::RemoveAll() needs more ::Type() support" ) );
+        }
+    }
+
+    FinalizeBulkRemove( removed );
+}
+
+
 wxString BOARD::GetItemDescription( UNITS_PROVIDER* aUnitsProvider ) const
 {
     return wxString::Format( _( "PCB" ) );

pcbnew/board.h

@@ -312,22 +312,16 @@ public:
 
     const wxString &GetFileName() const { return m_fileName; }
 
-    TRACKS& Tracks() { return m_tracks; }
     const TRACKS& Tracks() const { return m_tracks; }
 
-    FOOTPRINTS& Footprints() { return m_footprints; }
     const FOOTPRINTS& Footprints() const { return m_footprints; }
 
-    DRAWINGS& Drawings() { return m_drawings; }
     const DRAWINGS& Drawings() const { return m_drawings; }
 
-    ZONES& Zones() { return m_zones; }
     const ZONES& Zones() const { return m_zones; }
 
-    GENERATORS&       Generators() { return m_generators; }
     const GENERATORS& Generators() const { return m_generators; }
 
-    MARKERS& Markers() { return m_markers; }
     const MARKERS& Markers() const { return m_markers; }
 
     const BOARD_ITEM_SET GetItemSet();
@@ -340,7 +334,6 @@ public:
      *   - If a group specifies a name, it must be unique
      *   - The graph of groups containing subgroups must be cyclic.
      */
-    GROUPS& Groups() { return m_groups; }
     const GROUPS& Groups() const { return m_groups; }
 
     const std::vector<BOARD_CONNECTED_ITEM*> AllConnectedItems();
@@ -389,6 +382,16 @@ public:
     ///< @copydoc BOARD_ITEM_CONTAINER::Remove()
     void Remove( BOARD_ITEM* aBoardItem, REMOVE_MODE aMode = REMOVE_MODE::NORMAL ) override;
 
+    /**
+     * An efficient way to remove all items of a certain type from the board.
+     * Because of how items are stored, this method has some limitations in order to preserve
+     * performance: tracks, vias, and arcs are all removed together by PCB_TRACE_T, and all graphics
+     * and text object types are removed together by PCB_SHAPE_T.  If you need something more
+     * granular than that, use BOARD::Remove.
+     * @param aTypes is a list of one or more types to remove, or leave default to remove all
+     */
+    void RemoveAll( std::initializer_list<KICAD_T> aTypes = { TYPE_NOT_INIT } );
+
     /**
      * Must be used if Add() is used using a BULK_x ADD_MODE to generate a change event for
      * listeners.
@@ -1277,6 +1280,8 @@ private:
     int                 m_timeStamp;                // actually a modification counter
 
     wxString            m_fileName;
+
+    // These containers only have const accessors and must only be modified by Add()/Remove()
     MARKERS             m_markers;
     DRAWINGS            m_drawings;
     FOOTPRINTS          m_footprints;

pcbnew/dialogs/dialog_drc.cpp

@@ -848,21 +848,25 @@ void DIALOG_DRC::OnDRCItemRClick( wxDataViewEvent& aEvent )
         m_ignoredList->InsertItem( m_ignoredList->GetItemCount(),
                                    wxT( " • " ) + rcItem->GetErrorText() );
 
-        std::vector<PCB_MARKER*>& markers = m_frame->GetBoard()->Markers();
+        BOARD* board = m_frame->GetBoard();
+        const std::vector<PCB_MARKER*>& markers = board->Markers();
 
-        for( unsigned i = 0; i < markers.size(); )
+        std::vector<BOARD_ITEM*> toRemove;
+
+        for( PCB_MARKER* marker : board->Markers() )
         {
-            if( markers[i]->GetRCItem()->GetErrorCode() == rcItem->GetErrorCode() )
+            if( marker->GetRCItem()->GetErrorCode() == rcItem->GetErrorCode() )
             {
-                m_frame->GetCanvas()->GetView()->Remove( markers.at( i ) );
+                m_frame->GetCanvas()->GetView()->Remove( marker );
-                markers.erase( markers.begin() + i );
+                toRemove.emplace_back( marker );
-            }
-            else
-            {
-                ++i;
             }
         }
 
+        for( BOARD_ITEM* marker : toRemove )
+            board->Remove( marker, REMOVE_MODE::BULK );
+
+        board->FinalizeBulkRemove( toRemove );
+
         if( rcItem->GetErrorCode() == DRCE_UNCONNECTED_ITEMS )
             m_frame->GetCanvas()->RedrawRatsnest();
 

pcbnew/graphics_cleaner.cpp

@@ -36,7 +36,7 @@
 #include <tool/tool_manager.h>
 #include <tools/pad_tool.h>
 
-GRAPHICS_CLEANER::GRAPHICS_CLEANER( DRAWINGS& aDrawings, FOOTPRINT* aParentFootprint,
+GRAPHICS_CLEANER::GRAPHICS_CLEANER( const DRAWINGS& aDrawings, FOOTPRINT* aParentFootprint,
                                     BOARD_COMMIT& aCommit, TOOL_MANAGER* aToolMgr ) :
         m_drawings( aDrawings ),
         m_parentFootprint( aParentFootprint ),
@@ -410,4 +410,4 @@ void GRAPHICS_CLEANER::mergePads()
             m_itemsList->push_back( item );
         }
     }
-}
+}

pcbnew/graphics_cleaner.h

@@ -36,7 +36,7 @@ class TOOL_MANAGER;
 class GRAPHICS_CLEANER
 {
 public:
-    GRAPHICS_CLEANER( DRAWINGS& aDrawings, FOOTPRINT* aParentFootprint, BOARD_COMMIT& aCommit,
+    GRAPHICS_CLEANER( const DRAWINGS& aDrawings, FOOTPRINT* aParentFootprint, BOARD_COMMIT& aCommit,
                       TOOL_MANAGER* aToolManager );
 
     /**
@@ -60,7 +60,7 @@ private:
     void mergePads();
 
 private:
-    DRAWINGS&     m_drawings;
+    const DRAWINGS& m_drawings;
     FOOTPRINT*    m_parentFootprint;  // nullptr if not in Footprint Editor
     BOARD_COMMIT& m_commit;
     TOOL_MANAGER* m_toolMgr;

pcbnew/pcb_io/fabmaster/import_fabmaster.cpp

@@ -3075,7 +3075,9 @@ bool FABMASTER::loadGraphics( BOARD* aBoard )
 
 bool FABMASTER::orderZones( BOARD* aBoard )
 {
-    std::sort( aBoard->Zones().begin(), aBoard->Zones().end(),
+    std::vector<ZONE*> sortedZones;
+    std::copy( aBoard->Zones().begin(), aBoard->Zones().end(), std::back_inserter( sortedZones ) );
+    std::sort( sortedZones.begin(), sortedZones.end(),
             [&]( const ZONE* a, const ZONE* b )
             {
                 if( a->GetLayer() == b->GetLayer() )
@@ -3087,7 +3089,7 @@ bool FABMASTER::orderZones( BOARD* aBoard )
     PCB_LAYER_ID layer = UNDEFINED_LAYER;
     unsigned int priority = 0;
 
-    for( ZONE* zone : aBoard->Zones() )
+    for( ZONE* zone : sortedZones )
     {
         /// Rule areas do not have priorities
         if( zone->GetIsRuleArea() )

pcbnew/specctra_import_export/specctra_import.cpp

@@ -341,12 +341,11 @@ void SPECCTRA_DB::FromSESSION( BOARD* aBoard )
 
     // delete the old tracks and vias but save locked tracks/vias; they will be re-added later
     std::vector<PCB_TRACK*> locked;
+    TRACKS tracks = aBoard->Tracks();
+    aBoard->RemoveAll( { PCB_TRACE_T } );
 
-    while( !aBoard->Tracks().empty() )
+    for( PCB_TRACK* track : tracks )
     {
-        PCB_TRACK* track = aBoard->Tracks().back();
-        aBoard->Tracks().pop_back();
-
         if( track->IsLocked() )
         {
             locked.push_back( track );
@@ -366,7 +365,7 @@ void SPECCTRA_DB::FromSESSION( BOARD* aBoard )
 
     // Add locked tracks: because they are exported as Fix tracks, they are not
     // in .ses file.
-    for( PCB_TRACK* track: locked )
+    for( PCB_TRACK* track : locked )
         aBoard->Add( track );
 
     if( m_session->placement )

pcbnew/tools/pcb_control.cpp

@@ -973,11 +973,12 @@ int PCB_CONTROL::Paste( const TOOL_EVENT& aEvent )
                     pastedItems.push_back( group );
                 }
 
-                clipBoard->Groups().clear();
+                clipBoard->RemoveAll( { PCB_GROUP_T } );
 
                 for( FOOTPRINT* clipFootprint : clipBoard->Footprints() )
                     pasteFootprintItemsToFootprintEditor( clipFootprint, board(), pastedItems );
 
+
                 for( BOARD_ITEM* clipDrawItem : clipBoard->Drawings() )
                 {
                     switch( clipDrawItem->Type() )
@@ -1002,7 +1003,7 @@ int PCB_CONTROL::Paste( const TOOL_EVENT& aEvent )
                     }
                 }
 
-                clipBoard->Drawings().clear();
+                clipBoard->RemoveAll( { PCB_SHAPE_T } );
 
                 clipBoard->Visit(
                         [&]( EDA_ITEM* item, void* testData )
@@ -1106,7 +1107,7 @@ int PCB_CONTROL::AppendBoardFromFile( const TOOL_EVENT& aEvent )
 
 
 template<typename T>
-static void moveUnflaggedItems( std::deque<T>& aList, std::vector<BOARD_ITEM*>& aTarget,
+static void moveUnflaggedItems( const std::deque<T>& aList, std::vector<BOARD_ITEM*>& aTarget,
                                 bool aIsNew )
 {
     std::copy_if( aList.begin(), aList.end(), std::back_inserter( aTarget ),
@@ -1119,54 +1120,26 @@ static void moveUnflaggedItems( std::deque<T>& aList, std::vector<BOARD_ITEM*>&
 
                 return doCopy;
             } );
-
-    if( aIsNew )
-        aList.clear();
 }
 
 
-static void moveUnflaggedItems( ZONES& aList, std::vector<BOARD_ITEM*>& aTarget, bool aIsNew )
+template<typename T>
+static void moveUnflaggedItems( const std::vector<T>& aList, std::vector<BOARD_ITEM*>& aTarget,
+                                bool aIsNew )
 {
-    if( aList.size() == 0 )
+    std::copy_if( aList.begin(), aList.end(), std::back_inserter( aTarget ),
-        return;
+            [aIsNew]( T aItem )
-
-    auto obj = aList.front();
-    int idx = 0;
-
-    if( aIsNew )
-    {
-        obj = aList.back();
-        aList.pop_back();
-    }
-
-    for( ; obj ; )
-    {
-        if( obj->HasFlag( SKIP_STRUCT ) )
-            obj->ClearFlags( SKIP_STRUCT );
-        else
-            aTarget.push_back( obj );
-
-        if( aIsNew )
-        {
-            if( aList.size() )
             {
-                obj = aList.back();
+                bool doCopy = ( aItem->GetFlags() & SKIP_STRUCT ) == 0;
-                aList.pop_back();
+
-            }
+                aItem->ClearFlags( SKIP_STRUCT );
-            else
+                aItem->SetFlags( aIsNew ? IS_NEW : 0 );
-            {
+
-                obj = nullptr;
+                return doCopy;
-            }
+            } );
-        }
-        else
-        {
-            obj = idx < int(aList.size()-1) ? aList[++idx] : nullptr;
-        }
-    }
 }
 
 
-
 bool PCB_CONTROL::placeBoardItems( BOARD_COMMIT* aCommit, BOARD* aBoard, bool aAnchorAtOrigin,
                                    bool aReannotateDuplicates )
 {
@@ -1188,6 +1161,17 @@ bool PCB_CONTROL::placeBoardItems( BOARD_COMMIT* aCommit, BOARD* aBoard, bool aA
 
     moveUnflaggedItems( aBoard->Generators(), items, isNew );
 
+    if( isNew )
+        aBoard->RemoveAll();
+
+    // Reparent before calling pruneItemLayers, as SetLayer can have a dependence on the
+    // item's parent board being set correctly.
+    if( isNew )
+    {
+        for( BOARD_ITEM* item : items )
+            item->SetParent( board() );
+    }
+
     pruneItemLayers( items );
 
     return placeBoardItems( aCommit, items, isNew, aAnchorAtOrigin, aReannotateDuplicates );

pcbnew/zone_filler.cpp

@@ -87,7 +87,7 @@ void ZONE_FILLER::SetProgressReporter( PROGRESS_REPORTER* aReporter )
  *
  * Caller is also responsible for re-building connectivity afterwards.
  */
-bool ZONE_FILLER::Fill( std::vector<ZONE*>& aZones, bool aCheck, wxWindow* aParent )
+bool ZONE_FILLER::Fill( const std::vector<ZONE*>& aZones, bool aCheck, wxWindow* aParent )
 {
     std::lock_guard<KISPINLOCK> lock( m_board->GetConnectivity()->GetLock() );
 

pcbnew/zone_filler.h

@@ -55,7 +55,7 @@ public:
      *
      * Caller is also responsible for re-building connectivity afterwards.
      */
-    bool Fill( std::vector<ZONE*>& aZones, bool aCheck = false, wxWindow* aParent = nullptr );
+    bool Fill( const std::vector<ZONE*>& aZones, bool aCheck = false, wxWindow* aParent = nullptr );
 
     bool IsDebug() const { return m_debugZoneFiller; }
 

pcbnew/zone_manager/dialog_zone_manager.cpp

@@ -402,12 +402,18 @@ void DIALOG_ZONE_MANAGER::OnButtonApplyClick( wxCommandEvent& aEvent )
     m_filler = std::make_unique<ZONE_FILLER>( board, commit.get() );
     auto reporter = std::make_unique<WX_PROGRESS_REPORTER>( this, _( "Fill All Zones" ), 5 );
     m_filler->SetProgressReporter( reporter.get() );
-    board->Zones() = m_zonesContainer->GetClonedZoneList();
+
+    // TODO: replace these const_cast calls with a different solution that avoids mutating the
+    // container of the board.  This is relatively safe as-is because the original zones list is
+    // swapped back in below, but still should be changed to avoid invalidating the board state
+    // in case this code is refactored to be a non-modal dialog in the future.
+    const_cast<ZONES&>( board->Zones() ) = m_zonesContainer->GetClonedZoneList();
+
     //NOTE - Nether revert nor commit is needed here , cause the cloned zones are not owned by
     //       the pcb frame.
     m_zoneFillComplete = m_filler->Fill( board->Zones() );
     board->BuildConnectivity();
-    board->Zones() = m_zonesContainer->GetOriginalZoneList();
+    const_cast<ZONES&>( board->Zones() ) = m_zonesContainer->GetOriginalZoneList();
 
     if( auto gal = m_zoneViewer->GetZoneGAL() )
     {