Remove undo-of-ungroup hack.

The hack assumed that the parent group would be the first
deleted item of type group in the undo list.  While this
will be true when undoing a user ungroup command, it will
not be when undoing an ungroup side-effect, such as when a
member of a group is deleted during UpdateFromPCB.

Fixes https://gitlab.com/kicad/code/kicad/-/issues/16384

include/undo_redo_container.h

@@ -102,8 +102,8 @@ public:
     BASE_SCREEN* GetScreen() const { return m_screen; }
 
 private:
-    EDA_ITEM_FLAGS m_pickerFlags;      /* a copy of m_flags member. useful in mode/drag
+    EDA_ITEM_FLAGS m_pickerFlags;      /* A copy of m_flags member. Currently used only to flag
-                                        * undo/redo commands */
+                                        * transient items. */
     UNDO_REDO      m_undoRedoStatus;   /* type of operation to undo/redo for this item */
     EDA_ITEM*      m_pickedItem;       /* Pointer on the schematic or board item that is concerned
                                         * (picked), or in undo redo commands, the copy of an

pcbnew/board_commit.cpp

@@ -382,10 +382,17 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags )
         }
 
         case CHT_UNGROUP:
-            boardItem->SetParentGroup( nullptr );
-
             if( !( aCommitFlags & SKIP_UNDO ) )
-                undoList.PushItem( ITEM_PICKER( nullptr, boardItem, UNDO_REDO::UNGROUP ) );
+            {
+                ITEM_PICKER itemWrapper( nullptr, boardItem, UNDO_REDO::UNGROUP );
+
+                if( PCB_GROUP* group = boardItem->GetParentGroup() )
+                    itemWrapper.SetLink( group->Clone() );
+
+                undoList.PushItem( itemWrapper );
+            }
+
+            boardItem->SetParentGroup( nullptr );
 
             break;
 

pcbnew/generators/pcb_tuning_pattern.cpp

@@ -979,13 +979,10 @@ void PCB_TUNING_PATTERN::Remove( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_COM
     // Ungroup first so that undo works
     if( !GetItems().empty() )
     {
-        PCB_GENERATOR*    group = this;
+        PCB_GENERATOR*  group = this;
-        PICKED_ITEMS_LIST ungroupList;
 
         for( BOARD_ITEM* member : group->GetItems() )
-            ungroupList.PushItem( ITEM_PICKER( nullptr, member, UNDO_REDO::UNGROUP ) );
+            aCommit->Stage( member, CHT_UNGROUP );
-
-        aCommit->Stage( ungroupList );
 
         group->GetItems().clear();
     }
@@ -1305,7 +1302,6 @@ void PCB_TUNING_PATTERN::EditPush( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_C
     KIGFX::VIEW*      view = aTool->GetManager()->GetView();
     PNS::ROUTER*      router = aTool->Router();
     SHAPE_LINE_CHAIN  bounds = getRectShape();
-    PICKED_ITEMS_LIST groupUndoList;
     int               epsilon = aBoard->GetDesignSettings().GetDRCEpsilon();
 
     if( router->RoutingInProgress() )
@@ -1341,11 +1337,10 @@ void PCB_TUNING_PATTERN::EditPush( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_C
                     && bounds.PointInside( track->GetEnd(), epsilon ) )
                 {
                     AddItem( item );
-                    groupUndoList.PushItem( ITEM_PICKER( nullptr, item, UNDO_REDO::REGROUP ) );
+                    aCommit->Stage( item, CHT_GROUP );
                 }
             }
 
-            aCommit->Stage( groupUndoList );
             aCommit->Add( item );
         }
     }

pcbnew/netlist_reader/board_netlist_updater.cpp

@@ -1245,6 +1245,9 @@ bool BOARD_NETLIST_UPDATER::UpdateNetlist( NETLIST& aNetlist )
             }
             else
             {
+                if( footprint->GetParentGroup() )
+                    m_commit.Stage( footprint, CHT_UNGROUP );
+
                 m_commit.Remove( footprint );
                 msg.Printf( _( "Removed unused footprint %s." ), footprint->GetReference() );
             }

pcbnew/tools/group_tool.cpp

@@ -269,16 +269,12 @@ int GROUP_TOOL::Group( const TOOL_EVENT& aEvent )
 
     commit.Add( group );
 
-    PICKED_ITEMS_LIST groupList;
-
     for( EDA_ITEM* eda_item : selection )
     {
         if( BOARD_ITEM* item = dynamic_cast<BOARD_ITEM*>( eda_item ) )
-            groupList.PushItem( ITEM_PICKER( nullptr, item, UNDO_REDO::REGROUP ) );
+            commit.Stage( item, CHT_GROUP );
     }
 
-    commit.Stage( groupList );
-
     commit.Push( _( "Group Items" ) );
 
     selTool->ClearSelection();
@@ -309,16 +305,12 @@ int GROUP_TOOL::Ungroup( const TOOL_EVENT& aEvent )
 
         if( group )
         {
-            PICKED_ITEMS_LIST ungroupList;
-
             for( BOARD_ITEM* member : group->GetItems() )
             {
-                ungroupList.PushItem( ITEM_PICKER( nullptr, member, UNDO_REDO::UNGROUP ) );
+                commit.Stage( member, CHT_UNGROUP );
                 toSelect.push_back( member );
             }
 
-            commit.Stage( ungroupList );
-
             group->GetItems().clear();
             group->SetSelected();
             commit.Remove( group );

pcbnew/undo_redo.cpp

@@ -273,15 +273,13 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList )
     auto view = GetCanvas()->GetView();
     auto connectivity = GetBoard()->GetConnectivity();
 
-    PCB_GROUP* addedGroup = nullptr;
-
     GetBoard()->IncrementTimeStamp();   // clear caches
 
     // Undo in the reverse order of list creation: (this can allow stacked changes
     // like the same item can be changes and deleted in the same complex command
 
     // Restore changes in reverse order
-    for( int ii = aList->GetCount() - 1; ii >= 0 ; ii-- )
+    for( int ii = (int) aList->GetCount() - 1; ii >= 0 ; ii-- )
     {
         EDA_ITEM* eda_item = aList->GetPickedItem( (unsigned) ii );
 
@@ -414,26 +412,38 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList )
             if( eda_item->Type() != PCB_NETINFO_T )
                 view->Add( eda_item );
 
-            if( PCB_GROUP* group = dynamic_cast<PCB_GROUP*>( eda_item ) )
-                addedGroup = group;
-
             break;
 
-        case UNDO_REDO::REGROUP:
+        case UNDO_REDO::REGROUP:    /* grouped items are ungrouped */
             aList->SetPickedItemStatus( UNDO_REDO::UNGROUP, ii );
 
             if( BOARD_ITEM* boardItem = dynamic_cast<BOARD_ITEM*>( eda_item ) )
+            {
+                if( PCB_GROUP* group = boardItem->GetParentGroup() )
+                {
+                    if( !aList->GetPickedItemLink( ii ) )
+                        aList->SetPickedItemLink( group->Clone(), ii );
+                }
+
                 boardItem->SetParentGroup( nullptr );
+            }
 
             break;
 
-        case UNDO_REDO::UNGROUP:
+        case UNDO_REDO::UNGROUP:    /* ungrouped items are re-added to their previuos groups */
             aList->SetPickedItemStatus( UNDO_REDO::REGROUP, ii );
 
             if( BOARD_ITEM* boardItem = dynamic_cast<BOARD_ITEM*>( eda_item ) )
             {
-                if( addedGroup )
+                PCB_GROUP* group = nullptr;
-                    addedGroup->AddItem( boardItem );
+
+                // The link is just a clone of the original parent group; we need to look up
+                // the UUID in the document to find the real parent.
+                if( EDA_ITEM* link = aList->GetPickedItemLink( ii ) )
+                    group = dynamic_cast<PCB_GROUP*>( GetBoard()->GetItem( link->m_Uuid ) );
+
+                if( group )
+                    group->AddItem( boardItem );
             }
 
             break;