More tightening of group parent lifecycles.

Fixes https://gitlab.com/kicad/code/kicad/issues/12908

common/commit.cpp

@@ -48,14 +48,14 @@ COMMIT::~COMMIT()
 COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType )
 {
     // CHT_MODIFY and CHT_DONE are not compatible
-    assert( ( aChangeType & ( CHT_MODIFY | CHT_DONE ) ) != ( CHT_MODIFY | CHT_DONE ) );
+    wxASSERT( ( aChangeType & ( CHT_MODIFY | CHT_DONE ) ) != ( CHT_MODIFY | CHT_DONE ) );
 
     int flag = aChangeType & CHT_FLAGS;
 
     switch( aChangeType & CHT_TYPE )
     {
     case CHT_ADD:
-        assert( m_changedItems.find( aItem ) == m_changedItems.end() );
+        wxASSERT( m_changedItems.find( aItem ) == m_changedItems.end() );
         makeEntry( aItem, CHT_ADD | flag );
         return *this;
 
@@ -66,14 +66,9 @@ COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType )
     case CHT_MODIFY:
     {
         EDA_ITEM* parent = parentObject( aItem );
-        EDA_ITEM* clone = nullptr;
+        EDA_ITEM* clone = makeImage( parent );
 
-        assert( parent );
+        wxASSERT( clone );
-
-        if( parent )
-            clone = parent->Clone();
-
-        assert( clone );
 
         if( clone )
             return createModified( parent, clone, flag );
@@ -82,7 +77,7 @@ COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType )
     }
 
     default:
-        assert( false );
+        wxFAIL;
     }
 
     return *this;
@@ -92,9 +87,7 @@ COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType )
 COMMIT& COMMIT::Stage( std::vector<EDA_ITEM*>& container, CHANGE_TYPE aChangeType )
 {
     for( EDA_ITEM* item : container )
-    {
         Stage( item, aChangeType );
-    }
 
     return *this;
 }
@@ -106,12 +99,11 @@ COMMIT& COMMIT::Stage( const PICKED_ITEMS_LIST& aItems, UNDO_REDO aModFlag )
     {
         UNDO_REDO change_type = aItems.GetPickedItemStatus( i );
         EDA_ITEM* item = aItems.GetPickedItem( i );
-        EDA_ITEM* copy = nullptr;
 
         if( change_type == UNDO_REDO::UNSPECIFIED )
             change_type = aModFlag;
 
-        if( ( copy = aItems.GetPickedItemLink( i ) ) )
+        if( EDA_ITEM* copy = aItems.GetPickedItemLink( i ) )
         {
             assert( change_type == UNDO_REDO::CHANGED );
 

common/undo_redo_container.cpp

@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2018 jp.charras at wanadoo.fr
  * Copyright (C) 2011 Wayne Stambaugh <stambaughw@gmail.com>
- * Copyright (C) 2021 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 2021-2022 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -114,9 +114,8 @@ void PICKED_ITEMS_LIST::ClearItemsList()
 }
 
 
-void PICKED_ITEMS_LIST::ClearListAndDeleteItems()
+void PICKED_ITEMS_LIST::ClearListAndDeleteItems( std::function<void(EDA_ITEM*)> aItemDeleter )
 {
-    // Delete items is they are not flagged NEWITEM, or if this is a block operation
     while( GetCount() > 0 )
     {
         ITEM_PICKER wrapper = PopItem();
@@ -126,17 +125,17 @@ void PICKED_ITEMS_LIST::ClearListAndDeleteItems()
 
         // The Link is an undo construct; it is always owned by the undo/redo container
         if( wrapper.GetLink() )
-            delete wrapper.GetLink();
+            aItemDeleter( wrapper.GetLink() );
 
         if( wrapper.GetFlags() & UR_TRANSIENT )
         {
-            delete wrapper.GetItem();
+            aItemDeleter( wrapper.GetItem() );
         }
         else if( wrapper.GetStatus() == UNDO_REDO::DELETED )
         {
             // This should really be replaced with UR_TRANSIENT, but currently many clients
             // (eeschema in particular) abuse this to achieve non-undo-related deletions.
-            delete wrapper.GetItem();
+            aItemDeleter( wrapper.GetItem() );
         }
     }
 }

eeschema/schematic_undo_redo.cpp

@@ -393,7 +393,10 @@ void SCH_EDIT_FRAME::RollbackSchematicFromUndo()
     if( undo )
     {
         PutDataInPreviousState( undo );
-        undo->ClearListAndDeleteItems();
+        undo->ClearListAndDeleteItems( []( EDA_ITEM* aItem )
+                                       {
+                                           delete aItem;
+                                       } );
         delete undo;
 
         SetSheetNumberAndCount();
@@ -417,7 +420,10 @@ void SCH_EDIT_FRAME::ClearUndoORRedoList( UNDO_REDO_LIST whichList, int aItemCou
 
     for( PICKED_ITEMS_LIST* command : list.m_CommandsList )
     {
-        command->ClearListAndDeleteItems();
+        command->ClearListAndDeleteItems( []( EDA_ITEM* aItem )
+                                          {
+                                              delete aItem;
+                                          } );
         delete command;
     }
 

eeschema/symbol_editor/symbol_edit_frame.cpp

@@ -1377,7 +1377,10 @@ void SYMBOL_EDIT_FRAME::ClearUndoORRedoList( UNDO_REDO_LIST whichList, int aItem
 
     for( PICKED_ITEMS_LIST* command : list.m_CommandsList )
     {
-        command->ClearListAndDeleteItems();
+        command->ClearListAndDeleteItems( []( EDA_ITEM* aItem )
+                                          {
+                                              delete aItem;
+                                          } );
         delete command;
     }
 

gerbview/dialogs/dialog_layers_select_to_pcb.cpp

@@ -6,7 +6,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 1992-2021 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2022 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -229,7 +229,7 @@ void LAYERS_MAP_DIALOG::initDialog()
                 int currLayer = gerber2KicadMapping[ii];
 
                 // Default to "Do Not Export" for unselected or undefined layer
-                if( ( currLayer == UNSELECTED_LAYER ) )
+                if( currLayer == UNSELECTED_LAYER )
                 {
                     m_layersList[ii]->SetLabel( _( "Do not export" ) );
                     m_layersList[ii]->SetForegroundColour( *wxBLUE );

include/commit.h

@@ -172,6 +172,8 @@ protected:
 
     virtual EDA_ITEM* parentObject( EDA_ITEM* aItem ) const = 0;
 
+    virtual EDA_ITEM* makeImage( EDA_ITEM* aItem ) const = 0;
+
     CHANGE_TYPE convert( UNDO_REDO aType ) const;
 
     std::set<EDA_ITEM*> m_changedItems;

include/undo_redo_container.h

@@ -170,7 +170,7 @@ public:
      * Delete the list of pickers AND the data pointed by #m_PickedItem or #m_PickedItemLink
      * according to the type of undo/redo command recorded.
      */
-    void ClearListAndDeleteItems();
+    void ClearListAndDeleteItems( std::function<void(EDA_ITEM*)> aItemDeleter );
 
     /**
      * @return The count of pickers stored in this list.

pagelayout_editor/pl_editor_frame.cpp

@@ -925,7 +925,10 @@ void PL_EDITOR_FRAME::ClearUndoORRedoList( UNDO_REDO_LIST whichList, int aItemCo
         PICKED_ITEMS_LIST* curr_cmd = list.m_CommandsList[0];
         list.m_CommandsList.erase( list.m_CommandsList.begin() );
 
-        curr_cmd->ClearListAndDeleteItems();
+        curr_cmd->ClearListAndDeleteItems( []( EDA_ITEM* aItem )
+                                           {
+                                               delete aItem;
+                                           } );
         delete curr_cmd;    // Delete command
     }
 }

pcbnew/board_commit.cpp

@@ -218,7 +218,7 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags )
                 if( !ent.m_copy )
                 {
                     wxASSERT( changeType != CHT_MODIFY );     // too late to make a copy..
-                    ent.m_copy = ent.m_item->Clone();
+                    ent.m_copy = makeImage( ent.m_item );
                 }
 
                 wxASSERT( ent.m_item->Type() == PCB_FOOTPRINT_T );
@@ -603,6 +603,15 @@ EDA_ITEM* BOARD_COMMIT::parentObject( EDA_ITEM* aItem ) const
 }
 
 
+EDA_ITEM* BOARD_COMMIT::makeImage( EDA_ITEM* aItem ) const
+{
+    BOARD_ITEM* clone = static_cast<BOARD_ITEM*>( aItem->Clone() );
+
+    clone->SetParentGroup( nullptr );
+    return clone;
+}
+
+
 void BOARD_COMMIT::Revert()
 {
     PICKED_ITEMS_LIST                  undoList;
@@ -651,6 +660,16 @@ void BOARD_COMMIT::Revert()
 
             item->SwapItemData( copy );
 
+            if( item->Type() == PCB_GROUP_T )
+            {
+                PCB_GROUP* group = static_cast<PCB_GROUP*>( item );
+
+                group->RunOnChildren( [&]( BOARD_ITEM* child )
+                                      {
+                                          child->SetParentGroup( group );
+                                      } );
+            }
+
             view->Add( item );
             connectivity->Add( item );
             board->OnItemChanged( item );

pcbnew/board_commit.h

@@ -73,7 +73,9 @@ public:
     void SetResolveNetConflicts( bool aResolve = true ) { m_resolveNetConflicts = aResolve; }
 
 private:
-    virtual EDA_ITEM* parentObject( EDA_ITEM* aItem ) const override;
+    EDA_ITEM* parentObject( EDA_ITEM* aItem ) const override;
+
+    EDA_ITEM* makeImage( EDA_ITEM* aItem ) const override;
 
     void dirtyIntersectingZones( BOARD_ITEM* item );
 

pcbnew/netlist_reader/board_netlist_updater.cpp

@@ -258,8 +258,14 @@ bool BOARD_NETLIST_UPDATER::updateFootprintParameters( FOOTPRINT* aPcbFootprint,
     wxString msg;
 
     // Create a copy only if the footprint has not been added during this update
-    FOOTPRINT* copy = m_commit.GetStatus( aPcbFootprint ) ? nullptr
+    FOOTPRINT* copy = nullptr;
-                                                          : (FOOTPRINT*) aPcbFootprint->Clone();
+
+    if( m_commit.GetStatus( aPcbFootprint ) )
+    {
+        copy = static_cast<FOOTPRINT*>( aPcbFootprint->Clone() );
+        copy->SetParentGroup( nullptr );
+    }
+
     bool       changed = false;
 
     // Test for reference designator field change.
@@ -437,14 +443,9 @@ bool BOARD_NETLIST_UPDATER::updateFootprintParameters( FOOTPRINT* aPcbFootprint,
     }
 
     if( changed && copy )
-    {
         m_commit.Modified( aPcbFootprint, copy );
-    }
     else if( copy )
-    {
-        copy->SetParentGroup( nullptr );
         delete copy;
-    }
 
     return true;
 }
@@ -456,8 +457,15 @@ bool BOARD_NETLIST_UPDATER::updateComponentPadConnections( FOOTPRINT* aFootprint
     wxString msg;
 
     // Create a copy only if the footprint has not been added during this update
-    FOOTPRINT* copy = m_commit.GetStatus( aFootprint ) ? nullptr : (FOOTPRINT*) aFootprint->Clone();
+    FOOTPRINT* copy = nullptr;
-    bool       changed = false;
+
+    if( m_commit.GetStatus( aFootprint ) )
+    {
+        copy = static_cast<FOOTPRINT*>( aFootprint->Clone() );
+        copy->SetParentGroup( nullptr );
+    }
+
+    bool changed = false;
 
     // At this point, the component footprint is updated.  Now update the nets.
     for( PAD* pad : aFootprint->Pads() )
@@ -626,14 +634,9 @@ bool BOARD_NETLIST_UPDATER::updateComponentPadConnections( FOOTPRINT* aFootprint
     }
 
     if( changed && copy )
-    {
         m_commit.Modified( aFootprint, copy );
-    }
     else if( copy )
-    {
-        copy->SetParentGroup( nullptr );
         delete copy;
-    }
 
     return true;
 }

pcbnew/undo_redo.cpp

@@ -35,10 +35,8 @@ using namespace std::placeholders;
 #include <pcb_target.h>
 #include <footprint.h>
 #include <pad.h>
-#include <pcb_dimension.h>
 #include <origin_viewitem.h>
 #include <connectivity/connectivity_data.h>
-#include <pcbnew_settings.h>
 #include <tool/tool_manager.h>
 #include <tool/actions.h>
 #include <tools/pcb_selection_tool.h>
@@ -192,6 +190,7 @@ void PCB_BASE_EDIT_FRAME::saveCopyInUndoList( PICKED_ITEMS_LIST* commandToUndo,
                 FOOTPRINT* orig = static_cast<FOOTPRINT*>( item );
                 FOOTPRINT* clone = new FOOTPRINT( *orig );
                 clone->SetParent( GetBoard() );
+                clone->SetParentGroup( nullptr );
 
                 // Clear current flags (which can be temporary set by a current edit command)
                 for( BOARD_ITEM* child : clone->GraphicalItems() )
@@ -240,7 +239,12 @@ void PCB_BASE_EDIT_FRAME::saveCopyInUndoList( PICKED_ITEMS_LIST* commandToUndo,
 
             // If we don't yet have a copy in the link, set one up
             if( !commandToUndo->GetPickedItemLink( ii ) )
-                commandToUndo->SetPickedItemLink( item->Clone(), ii );
+            {
+                BOARD_ITEM* clone = static_cast<BOARD_ITEM*>( item->Clone() );
+                clone->SetParentGroup( nullptr );
+
+                commandToUndo->SetPickedItemLink( clone, ii );
+            }
 
             break;
 
@@ -580,16 +584,11 @@ void PCB_BASE_EDIT_FRAME::ClearUndoORRedoList( UNDO_REDO_LIST whichList, int aIt
 
 void PCB_BASE_EDIT_FRAME::ClearListAndDeleteItems( PICKED_ITEMS_LIST* aList )
 {
-    for( size_t ii = 0; ii < aList->GetCount();  ++ii )
+    aList->ClearListAndDeleteItems( []( EDA_ITEM* item )
-    {
+                                    {
-        if( BOARD_ITEM* item = static_cast<BOARD_ITEM*>( aList->GetPickedItem( ii ) ) )
+                                        static_cast<BOARD_ITEM*>( item )->SetParentGroup( nullptr );
-            item->SetParentGroup( nullptr );
+                                        delete item;
-
+                                    } );
-        if( BOARD_ITEM* link = static_cast<BOARD_ITEM*>( aList->GetPickedItemLink( ii ) ) )
-            link->SetParentGroup( nullptr );
-    }
-
-    aList->ClearListAndDeleteItems();
 }
 
 

pcbnew/zones_functions_for_undo_redo.cpp

@@ -139,6 +139,8 @@ void SaveCopyOfZones( PICKED_ITEMS_LIST& aPickList, BOARD* aPcb )
     {
         ZONE* zoneDup = new ZONE( *zone );
         zoneDup->SetParent( aPcb );
+        zoneDup->SetParentGroup( nullptr );
+
         ITEM_PICKER picker( nullptr, zone, UNDO_REDO::CHANGED );
         picker.SetLink( zoneDup );
         aPickList.PushItem( picker );
@@ -217,7 +219,7 @@ void UpdateCopyOfZonesList( PICKED_ITEMS_LIST& aPickList, PICKED_ITEMS_LIST& aAu
                     wxASSERT_MSG( zcopy != nullptr,
                                   wxT( "UpdateCopyOfZonesList() error: link = NULL" ) );
 
-                    *ref = *zcopy;
+                    ref->SwapItemData( zcopy );
 
                     // the copy was deleted; the link does not exists now.
                     aPickList.SetPickedItemLink( nullptr, kk );