First round of cleanup for the PCB groups

ADDED: Leave group action

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

include/class_board_item.h

@@ -32,6 +32,7 @@
 
 
 #include <base_struct.h>
+#include <common.h>
 #include <convert_to_biu.h>
 #include <gr_basic.h>
 #include <layers_id_colors_and_visibility.h>
@@ -88,13 +89,37 @@ class BOARD_ITEM : public EDA_ITEM
 {
 protected:
     PCB_LAYER_ID    m_Layer;
+    KIID            m_groupUuid;
 
 public:
-    BOARD_ITEM( BOARD_ITEM* aParent, KICAD_T idtype ) :
+    BOARD_ITEM( BOARD_ITEM* aParent, KICAD_T idtype )
-        EDA_ITEM( aParent, idtype ), m_Layer( F_Cu )
+        : EDA_ITEM( aParent, idtype ),
+          m_Layer( F_Cu ),
+          m_groupUuid( 0 )
     {
     }
 
+    /**
+     * Set the group that this item belongs to.
+     *
+     * @param aGroup is the group this belongs to
+     */
+    void SetGroup( const KIID& aGroup ) { m_groupUuid = aGroup; }
+
+    /**
+     * Get the group this item belongs to.
+     *
+     * @return the group this item is in
+     */
+    const KIID& GetGroup() const { return m_groupUuid; }
+
+    /**
+     * Test if this item is inside a group.
+     *
+     * @return true if inside a group
+     */
+    bool IsInGroup() { return m_groupUuid != niluuid; }
+
     // Do not create a copy constructor & operator=.
     // The ones generated by the compiler are adequate.
     int GetX() const
@@ -159,13 +184,13 @@ public:
     /**
      * Function GetEffectiveShape
      * Some pad shapes can be complex (rounded/chamfered rectangle), even without considering
-     * custom shapes.  This routine returns a COMPOUND shape (set of simple shapes which make 
+     * custom shapes.  This routine returns a COMPOUND shape (set of simple shapes which make
      * up the pad fod use with routing, collision determiniation, etc).
      *
      * Note that this list can contain a SHAPE_SIMPLE (a simple single-outline non-intersecting
      * polygon), but should never contain a SHAPE_POLY_SET (a complex polygon consisting of
      * multiple outlines and/or holes).
-     * 
+     *
      * @param aLayer in case of items spanning multiple layers, only the shapes belonging to aLayer
      *               will be returned. Pass UNDEFINED_LAYER to return shapes for all layers.
      */

include/class_pcb_group.h

@@ -53,7 +53,7 @@ typedef std::unordered_set<BOARD_ITEM*> ITEM_SET;
 class PCB_GROUP : public BOARD_ITEM
 {
 public:
-    PCB_GROUP( BOARD* parent );
+    PCB_GROUP( BOARD* aParent );
 
     static inline bool ClassOf( const EDA_ITEM* aItem )
     {
@@ -70,22 +70,24 @@ public:
         return m_items;
     }
 
-    void SetName( wxString name )
+    void SetName( wxString aName )
     {
-        m_name = name;
+        m_name = aName;
     }
 
     /**
      * Adds item to group. Does not take ownership of item.
-     * @return true if item was added (false if item was already in set).
+     *
+     * @return true if item was added (false if item belongs to a different group).
      */
-    bool AddItem( BOARD_ITEM* item );
+    bool AddItem( BOARD_ITEM* aItem );
 
     /**
      * Removes item from group.
+     *
      * @return true if item was removed (false if item was not in the group).
      */
-    bool RemoveItem( const BOARD_ITEM* item );
+    bool RemoveItem( BOARD_ITEM* aItem );
 
     wxString GetClass() const override
     {
@@ -103,7 +105,7 @@ public:
     wxPoint GetPosition() const override;
 
     ///> @copydoc EDA_ITEM::SetPosition
-    void SetPosition( const wxPoint& ) override;
+    void SetPosition( const wxPoint& aNewpos ) override;
 
     ///> @copydoc BOARD_ITEM::GetLayerSet
     LSET GetLayerSet() const override;
@@ -131,11 +133,7 @@ public:
     void SwapData( BOARD_ITEM* aImage ) override;
 
     ///> @copydoc BOARD_ITEM::IsOnLayer
-    bool IsOnLayer( PCB_LAYER_ID aLayer ) const override
+    bool IsOnLayer( PCB_LAYER_ID aLayer ) const override;
-    {
-        wxFAIL_MSG( "groups don't support layer IsOnLayer" );
-        return false;
-    }
 
     ///> @copydoc EDA_ITEM::HitTest
     bool HitTest( const wxPoint& aPosition, int aAccuracy = 0 ) const override;
@@ -147,7 +145,7 @@ public:
     const EDA_RECT GetBoundingBox() const override;
 
     ///> @copydoc EDA_ITEM::Visit
-    SEARCH_RESULT Visit( INSPECTOR inspector, void* testData, const KICAD_T scanTypes[] ) override;
+    SEARCH_RESULT Visit( INSPECTOR aInspector, void* aTestData, const KICAD_T aScanTypes[] ) override;
 
     ///> @copydoc VIEW_ITEM::ViewGetLayers
     void ViewGetLayers( int aLayers[], int& aCount ) const override;

pcbnew/class_board.cpp

@@ -1943,6 +1943,7 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
         {
             if( repair )
                 board.Groups().erase( board.Groups().begin() + idx );
+
             return  wxString::Format( _( "Group Uuid %s maps to 2 different BOARD_ITEMS: %p and %p" ),
                                       group.m_Uuid.AsString(),
                                       testItem, groups[idx] );
@@ -1955,13 +1956,15 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
             {
                 if( repair )
                     group.SetName( group.GetName() + "-" + group.m_Uuid.AsString() );
+
                 return wxString::Format( _( "Two groups of identical name: %s" ), group.GetName() );
             }
+
             wxCHECK( groupNames.insert( group.GetName() ).second == true,
                      "Insert failed of new group" );
         }
 
-        for( const BOARD_ITEM* member : group.GetItems() )
+        for( BOARD_ITEM* member : group.GetItems() )
         {
             BOARD_ITEM* item = board.GetItem( member->m_Uuid );
 
@@ -1969,6 +1972,7 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
             {
                 if( repair )
                     group.RemoveItem( member );
+
                 return wxString::Format( _( "Group %s contains deleted item %s" ),
                                             group.m_Uuid.AsString(),
                                             member->m_Uuid.AsString() );
@@ -1978,6 +1982,7 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
             {
                 if( repair )
                     group.RemoveItem( member );
+
                 return wxString::Format( _( "Uuid %s maps to 2 different BOARD_ITEMS: %s %p %s and %p %s" ),
                                           member->m_Uuid.AsString(),
                                           item->m_Uuid.AsString(),
@@ -1992,11 +1997,13 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
             {
                 if( repair )
                     group.RemoveItem( member );
+
                 return wxString::Format(
                         _( "BOARD_ITEM %s appears multiple times in groups (either in the "
                            "same group or in multiple groups) " ),
                         item->m_Uuid.AsString() );
             }
+
             wxCHECK( allMembers.insert( member->m_Uuid.AsString() ).second == true,
                      "Insert failed of new member" );
 
@@ -2022,6 +2029,7 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
         {
             if( repair )
                 board.Groups().erase( board.Groups().begin() + idx );
+
             return wxString::Format( _( "Group must have at least one member: %s" ), group.m_Uuid.AsString() );
         }
     }
@@ -2053,12 +2061,14 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
     {
         currentChainGroups.clear();
         int currIdx = *toCheckGroups.begin();
+
         while( true )
         {
             if( currentChainGroups.find( currIdx ) != currentChainGroups.end() )
             {
                 if( repair )
                     board.Groups().erase( board.Groups().begin() + currIdx );
+
                 return "Cycle detected in group membership";
             }
             else if( knownCycleFreeGroups.find( currIdx ) != knownCycleFreeGroups.end() )
@@ -2066,21 +2076,25 @@ wxString BOARD::GroupsSanityCheckInternal( bool repair )
                 // Parent is a group we know does not lead to a cycle
                 break;
             }
+
             wxCHECK( currentChainGroups.insert( currIdx ).second == true,
                      "Insert of new group to check failed" );
             // We haven't visited currIdx yet, so it must be in toCheckGroups
             wxCHECK( toCheckGroups.erase( currIdx ) == 1,
                      "Erase of idx for group just checked failed" );
             currIdx = parentGroupIdx[currIdx];
+
             if( currIdx == -1 )
             {
                 // end of chain and no cycles found in this chain
                 break;
             }
         }
+
         // No cycles found in chain, so add it to set of groups we know don't participate in a cycle.
         knownCycleFreeGroups.insert( currentChainGroups.begin(), currentChainGroups.end() );
     }
+
     // Success
     return "";
 }

pcbnew/class_pcb_group.cpp

@@ -22,25 +22,43 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
  */
 #include <bitmaps.h>
+#include <class_board.h>
+#include <class_board_item.h>
 #include <class_pcb_group.h>
 #include <confirm.h>
 #include <msgpanel.h>
 #include <view/view.h>
 
-PCB_GROUP::PCB_GROUP( BOARD* parent ) : BOARD_ITEM( (BOARD_ITEM*) parent, PCB_GROUP_T )
+PCB_GROUP::PCB_GROUP( BOARD*aParent ) : BOARD_ITEM( aParent, PCB_GROUP_T )
 {
 }
 
 
-bool PCB_GROUP::AddItem( BOARD_ITEM* item )
+bool PCB_GROUP::AddItem( BOARD_ITEM* aItem )
 {
-    return m_items.insert( item ).second;
+    // Items can only be in one group at a time
+    if( aItem->IsInGroup() )
+        return false;
+
+    m_items.insert( aItem );
+    aItem->SetGroup( m_Uuid );
+    return true;
 }
 
 
-bool PCB_GROUP::RemoveItem( const BOARD_ITEM* item )
+bool PCB_GROUP::RemoveItem( BOARD_ITEM* aItem )
 {
-    return m_items.erase( const_cast<BOARD_ITEM*>( item ) ) == 1;
+    if( !aItem->IsInGroup() )
+        return false;
+
+    // Only clear the item's group field if it was inside this group
+    if( m_items.erase( aItem ) == 1 )
+    {
+        aItem->SetGroup( niluuid );
+        return true;
+    }
+
+    return false;
 }
 
 
@@ -50,9 +68,9 @@ wxPoint PCB_GROUP::GetPosition() const
 }
 
 
-void PCB_GROUP::SetPosition( const wxPoint& newpos )
+void PCB_GROUP::SetPosition( const wxPoint& aNewpos )
 {
-    wxPoint delta = newpos - GetPosition();
+    wxPoint delta = aNewpos - GetPosition();
 
     Move( delta );
 }
@@ -72,16 +90,12 @@ PCB_GROUP* PCB_GROUP::DeepClone() const
     PCB_GROUP* newGroup = new PCB_GROUP( *this );
     newGroup->m_items.clear();
 
-    for( auto member : m_items )
+    for( BOARD_ITEM* member : m_items )
     {
         if( member->Type() == PCB_GROUP_T )
-        {
             newGroup->AddItem( static_cast<PCB_GROUP*>( member )->DeepClone() );
-        }
         else
-        {
             newGroup->AddItem( static_cast<BOARD_ITEM*>( member->Clone() ) );
-        }
     }
 
     return newGroup;
@@ -93,16 +107,12 @@ PCB_GROUP* PCB_GROUP::DeepDuplicate() const
     PCB_GROUP* newGroup = static_cast<PCB_GROUP*>( this->Duplicate() );
     newGroup->m_items.clear();
 
-    for( auto member : m_items )
+    for( BOARD_ITEM* member : m_items )
     {
         if( member->Type() == PCB_GROUP_T )
-        {
             newGroup->AddItem( static_cast<PCB_GROUP*>( member )->DeepDuplicate() );
-        }
         else
-        {
             newGroup->AddItem( static_cast<BOARD_ITEM*>( member->Duplicate() ) );
-        }
     }
 
     return newGroup;
@@ -132,7 +142,9 @@ bool PCB_GROUP::HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy )
     EDA_RECT bbox = GetBoundingBox();
 
     if( aContained )
+    {
         return arect.Contains( bbox );
+    }
     else
     {
         // If the rect does not intersect the bounding box, skip any tests
@@ -144,44 +156,34 @@ bool PCB_GROUP::HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy )
             if( member->HitTest( arect, false, 0 ) )
                 return true;
         }
-
-        // No items were hit
-        return false;
     }
+
+    // No items were hit
+    return false;
 }
 
 
 const EDA_RECT PCB_GROUP::GetBoundingBox() const
 {
     EDA_RECT area;
-    bool     isFirst = true;
 
     for( BOARD_ITEM* item : m_items )
-    {
+        area.Merge( item->GetBoundingBox() );
-        if( isFirst )
+
-        {
-            area    = item->GetBoundingBox();
-            isFirst = false;
-        }
-        else
-        {
-            area.Merge( item->GetBoundingBox() );
-        }
-    }
     area.Inflate( Millimeter2iu( 0.25 ) ); // Give a min size to the area
 
     return area;
 }
 
 
-SEARCH_RESULT PCB_GROUP::Visit( INSPECTOR inspector, void* testData, const KICAD_T scanTypes[] )
+SEARCH_RESULT PCB_GROUP::Visit( INSPECTOR aInspector, void* aTestData, const KICAD_T aScanTypes[] )
 {
-    for( const KICAD_T* stype = scanTypes; *stype != EOT; ++stype )
+    for( const KICAD_T* stype = aScanTypes; *stype != EOT; ++stype )
     {
         // If caller wants to inspect my type
         if( *stype == Type() )
         {
-            if( SEARCH_RESULT::QUIT == inspector( this, testData ) )
+            if( SEARCH_RESULT::QUIT == aInspector( this, aTestData ) )
                 return SEARCH_RESULT::QUIT;
         }
     }
@@ -195,13 +197,25 @@ LSET PCB_GROUP::GetLayerSet() const
     LSET aSet;
 
     for( BOARD_ITEM* item : m_items )
-    {
         aSet |= item->GetLayerSet();
-    }
+
     return aSet;
 }
 
 
+bool PCB_GROUP::IsOnLayer( PCB_LAYER_ID aLayer ) const
+{
+    // A group is on a layer if any item is on the layer
+    for( BOARD_ITEM* item : m_items )
+    {
+        if( item->IsOnLayer( aLayer ) )
+            return true;
+    }
+
+    return false;
+}
+
+
 void PCB_GROUP::ViewGetLayers( int aLayers[], int& aCount ) const
 {
     // What layer to put bounding box on?  change in class_pcb_group.cpp
@@ -235,28 +249,22 @@ unsigned int PCB_GROUP::ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const
 
 void PCB_GROUP::Move( const wxPoint& aMoveVector )
 {
-    for( auto member : m_items )
+    for( BOARD_ITEM* member : m_items )
-    {
         member->Move( aMoveVector );
-    }
 }
 
 
 void PCB_GROUP::Rotate( const wxPoint& aRotCentre, double aAngle )
 {
     for( BOARD_ITEM* item : m_items )
-    {
         item->Rotate( aRotCentre, aAngle );
-    }
 }
 
 
 void PCB_GROUP::Flip( const wxPoint& aCentre, bool aFlipLeftRight )
 {
     for( BOARD_ITEM* item : m_items )
-    {
         item->Flip( aCentre, aFlipLeftRight );
-    }
 }
 
 
@@ -264,10 +272,11 @@ wxString PCB_GROUP::GetSelectMenuText( EDA_UNITS aUnits ) const
 {
     if( m_name.empty() )
     {
-        return wxString::Format( _( "Anonymous group %s with %ld members" ),
+        return wxString::Format( _( "Anonymous group %s with %zu members" ),
                                  m_Uuid.AsString(), m_items.size() );
     }
-    return wxString::Format( _( "Group \"%s\" with %ld members" ), m_name, m_items.size() );
+
+    return wxString::Format( _( "Group \"%s\" with %zu members" ), m_name, m_items.size() );
 }
 
 
@@ -281,7 +290,7 @@ void PCB_GROUP::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_I
 {
     aList.emplace_back( _( "Group" ), m_name.empty() ? _( "Anonymous" ) :
                         wxString::Format( "\"%s\"", m_name ), DARKCYAN );
-    aList.emplace_back( _( "Members" ), wxString::Format( "%ld", m_items.size() ), BROWN );
+    aList.emplace_back( _( "Members" ), wxString::Format( "%zu", m_items.size() ), BROWN );
 }
 
 
@@ -294,7 +303,7 @@ void PCB_GROUP::RunOnChildren( const std::function<void( BOARD_ITEM* )>& aFuncti
     }
     catch( std::bad_function_call& )
     {
-        wxFAIL_MSG( "Error running PCB_GROUP::RunOnChildren" );
+        wxFAIL_MSG( wxT( "Error calling function in PCB_GROUP::RunOnChildren" ) );
     }
 }
 
@@ -306,12 +315,13 @@ void PCB_GROUP::RunOnDescendants( const std::function<void( BOARD_ITEM* )>& aFun
         for( BOARD_ITEM* item : m_items )
         {
             aFunction( item );
+
             if( item->Type() == PCB_GROUP_T )
                 static_cast<PCB_GROUP*>( item )->RunOnDescendants( aFunction );
         }
     }
     catch( std::bad_function_call& )
     {
-        wxFAIL_MSG( "Error running PCB_GROUP::RunOnDescendants" );
+        wxFAIL_MSG( wxT( "Error calling function in PCB_GROUP::RunOnDescendants" ) );
     }
 }

pcbnew/pcb_parser.cpp

@@ -737,6 +737,7 @@ BOARD* PCB_PARSER::parseBOARD_unchecked()
     }
 
     wxString error;
+
     for( size_t idx = 0; idx < m_groupInfos.size(); idx++ )
     {
         auto&  aGrp  = m_groupInfos[idx];
@@ -753,26 +754,33 @@ BOARD* PCB_PARSER::parseBOARD_unchecked()
 
         for( const auto& aUuid : aGrp.memberUuids )
         {
-            KIID        tUuid = aUuid;
+            KIID tUuid = aUuid;
+
             if( m_resetKIIDs )
             {
                 if( m_resetKIIDMap.find( aUuid.AsString() ) == m_resetKIIDMap.end() )
                 {
                     if( error == wxEmptyString )
+                    {
                         error = wxString::Format( _( "Group %s references missing item %s" ),
                                                   aGrp.uuid.AsString(), aUuid.AsString() );
+                    }
                 }
                 else
                 {
                     tUuid = m_resetKIIDMap[ aUuid.AsString() ];
                 }
             }
+
             BOARD_ITEM* item = m_board->GetItem( tUuid );
+
             if( ( item == nullptr ) || ( item->Type() == NOT_USED ) )
             {
                 if( error == wxEmptyString )
+                {
                     error = wxString::Format( _( "Group %s references missing item %s" ),
                                               aGrp.uuid.AsString(), tUuid.AsString() );
+                }
             }
             else
             {
@@ -782,12 +790,13 @@ BOARD* PCB_PARSER::parseBOARD_unchecked()
     }
 
     wxString sanityResult = m_board->GroupsSanityCheck();
+
     if( error != wxEmptyString || sanityResult != wxEmptyString )
     {
         wxString errMsg = ( error != wxEmptyString ) ? error : sanityResult;
         KIDIALOG dlg( nullptr, wxString::Format(
-               _( "Error in group structure in file: %s\n\nAttempt repair?" ), errMsg ),
+                      _( "Error in group structure in file: %s\n\nAttempt repair?" ), errMsg ),
-               _( "File data error" ), wxOK | wxCANCEL | wxICON_ERROR );
+                      _( "File data error" ), wxOK | wxCANCEL | wxICON_ERROR );
         dlg.SetOKLabel( _( "Attempt repair" ) );
 
         if( dlg.ShowModal() == wxID_CANCEL )

pcbnew/tools/edit_tool.cpp

@@ -1214,7 +1214,7 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
         board()->GroupRemoveItems( removed, m_commit.get() );
 
         if( m_commit->HasRemoveEntry( enteredGroup ) )
-            m_selectionTool->exitGroup();
+            m_selectionTool->ExitGroup();
     }
 
     if( isCut )

pcbnew/tools/pcb_actions.cpp

@@ -629,7 +629,7 @@ TOOL_ACTION PCB_ACTIONS::unlock( "pcbnew.EditorControl.unlock",
 
 TOOL_ACTION PCB_ACTIONS::groupCreate( "pcbnew.EditorControl.groupCreate",
         AS_GLOBAL, 0, "",
-        _( "Group" ), "",
+        _( "Group" ), _( "Add the selected items to a new group" ),
         locked_xpm );
 
 TOOL_ACTION PCB_ACTIONS::groupMerge( "pcbnew.EditorControl.groupMerge",
@@ -644,19 +644,24 @@ TOOL_ACTION PCB_ACTIONS::groupUngroup( "pcbnew.EditorControl.groupUngroup",
 
 TOOL_ACTION PCB_ACTIONS::groupRemoveItems( "pcbnew.EditorControl.groupRemoveItems",
         AS_GLOBAL, 0, "",
-        _( "Remove Items" ), "",
+        _( "Remove Items" ), _( "Remove items from group" ),
         unlocked_xpm );
 
 TOOL_ACTION PCB_ACTIONS::groupFlatten( "pcbnew.EditorControl.groupFlatten",
         AS_GLOBAL, 0, "",
-        _( "Flatten" ), "",
+        _( "Flatten Group" ), "",
         unlocked_xpm );
 
 TOOL_ACTION PCB_ACTIONS::groupEnter( "pcbnew.EditorControl.groupEnter",
         AS_GLOBAL, 0, "",
-        _( "Enter" ), "",
+        _( "Enter Group" ), _( "Enter the group to edit items" ),
         unlocked_xpm );
 
+TOOL_ACTION PCB_ACTIONS::groupLeave( "pcbnew.EditorControl.groupLeave",
+        AS_GLOBAL, 0, "",
+        _( "Leave Group" ), _( "Leave the current group" ),
+        cancel_xpm );
+
 TOOL_ACTION PCB_ACTIONS::appendBoard( "pcbnew.EditorControl.appendBoard",
         AS_GLOBAL, 0, "",
         _( "Append Board..." ), "",

pcbnew/tools/pcb_actions.h

@@ -403,6 +403,7 @@ public:
     static TOOL_ACTION groupRemoveItems;
     static TOOL_ACTION groupFlatten;
     static TOOL_ACTION groupEnter;
+    static TOOL_ACTION groupLeave;
 
     // Miscellaneous
     static TOOL_ACTION selectionTool;

pcbnew/tools/pcb_editor_control.cpp

@@ -28,6 +28,7 @@
 #include "pcbnew_picker_tool.h"
 #include "selection_tool.h"
 #include "edit_tool.h"
+#include "tool/tool_event.h"
 #include <bitmaps.h>
 #include <board_commit.h>
 #include <class_board.h>
@@ -1019,14 +1020,7 @@ int PCB_EDITOR_CONTROL::GroupSelected( const TOOL_EVENT& aEvent )
     PCB_GROUP* group = new PCB_GROUP( board );
 
     for( EDA_ITEM* item : selection )
-    {
+        group->AddItem( static_cast<BOARD_ITEM*>( item ) );
-        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
-
-        // Should be impossible to generate a selection with duplicates
-        wxCHECK_MSG( group->AddItem( board_item ), 0,
-                     wxString::Format( _( "Item %s appears in selection multiple times" ),
-                                       board_item->m_Uuid.AsString() ) );
-    }
 
 
     commit.Add( group );
@@ -1079,12 +1073,7 @@ int PCB_EDITOR_CONTROL::GroupMergeSelected( const TOOL_EVENT& aEvent )
         BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
 
         if( board_item != firstGroup )
-        {
+            firstGroup->AddItem( board_item );
-            // Should be impossible to generate a selection with duplicates
-            wxCHECK_MSG( firstGroup->AddItem( board_item ), 0,
-                         wxString::Format( _( "Item %s is already in group for merge"),
-                                           board_item->m_Uuid.AsString() ) );
-        }
     }
 
     commit.Push( "GroupMerge" );
@@ -1257,12 +1246,16 @@ int PCB_EDITOR_CONTROL::GroupEnterSelected( const TOOL_EVENT& aEvent )
     SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
     const PCBNEW_SELECTION& selection = selTool->GetSelection();
 
-    wxCHECK( selection.GetSize() == 1, 0 );
+    if( selection.GetSize() == 1 && selection[0]->Type() == PCB_GROUP_T )
-    BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( selection[0] );
+        selTool->EnterGroup();
-    wxCHECK( board_item->Type() == PCB_GROUP_T, 0 );
 
-    selTool->EnterGroup();
+    return 0;
+}
 
+
+int PCB_EDITOR_CONTROL::GroupLeave( const TOOL_EVENT& aEvent )
+{
+    m_toolMgr->GetTool<SELECTION_TOOL>()->ExitGroup( true /* Select the group */ );
     return 0;
 }
 
@@ -1631,24 +1624,25 @@ void PCB_EDITOR_CONTROL::setTransitions()
     Go( &PCB_EDITOR_CONTROL::EditFpInFpEditor,         PCB_ACTIONS::editFpInFpEditor.MakeEvent() );
 
     // Other
-    Go( &PCB_EDITOR_CONTROL::ToggleLockSelected,     PCB_ACTIONS::toggleLock.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::ToggleLockSelected,       PCB_ACTIONS::toggleLock.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::LockSelected,           PCB_ACTIONS::lock.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::LockSelected,             PCB_ACTIONS::lock.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::UnlockSelected,         PCB_ACTIONS::unlock.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::UnlockSelected,           PCB_ACTIONS::unlock.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::GroupSelected,            PCB_ACTIONS::groupCreate.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::GroupMergeSelected,       PCB_ACTIONS::groupMerge.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::UngroupSelected,          PCB_ACTIONS::groupUngroup.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::GroupRemoveItemsSelected, PCB_ACTIONS::groupRemoveItems.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::GroupFlattenSelected,     PCB_ACTIONS::groupFlatten.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::GroupEnterSelected,       PCB_ACTIONS::groupEnter.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::GroupLeave,               PCB_ACTIONS::groupLeave.MakeEvent() );
 
-    Go( &PCB_EDITOR_CONTROL::UpdatePCBFromSchematic, ACTIONS::updatePcbFromSchematic.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::UpdatePCBFromSchematic,   ACTIONS::updatePcbFromSchematic.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::UpdateSchematicFromPCB, ACTIONS::updateSchematicFromPcb.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::UpdateSchematicFromPCB,   ACTIONS::updateSchematicFromPcb.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::ShowEeschema,           PCB_ACTIONS::showEeschema.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::ShowEeschema,             PCB_ACTIONS::showEeschema.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::ToggleLayersManager,    PCB_ACTIONS::showLayersManager.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::ToggleLayersManager,      PCB_ACTIONS::showLayersManager.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::ToggleMicrowaveToolbar, PCB_ACTIONS::showMicrowaveToolbar.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::ToggleMicrowaveToolbar,   PCB_ACTIONS::showMicrowaveToolbar.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::TogglePythonConsole,    PCB_ACTIONS::showPythonConsole.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::TogglePythonConsole,      PCB_ACTIONS::showPythonConsole.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::FlipPcbView,            PCB_ACTIONS::flipBoard.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::FlipPcbView,              PCB_ACTIONS::flipBoard.MakeEvent() );
-    Go( &PCB_EDITOR_CONTROL::RepairBoard,            PCB_ACTIONS::repairBoard.MakeEvent() );
+    Go( &PCB_EDITOR_CONTROL::RepairBoard,              PCB_ACTIONS::repairBoard.MakeEvent() );
 }
 
 

pcbnew/tools/pcb_editor_control.h

@@ -58,7 +58,7 @@ public:
     int SaveCopyAs( const TOOL_EVENT& aEvent );
     int PageSettings( const TOOL_EVENT& aEvent );
     int Plot( const TOOL_EVENT& aEvent );
-    
+
     int BoardSetup( const TOOL_EVENT& aEvent );
     int ImportNetlist( const TOOL_EVENT& aEvent );
     int ImportSpecctraSession( const TOOL_EVENT& aEvent );
@@ -125,9 +125,12 @@ public:
     ///> Collaps subgroups to single group.
     int GroupFlattenSelected( const TOOL_EVENT& aEvent );
 
-    ///> Restrit seletion to only member of the group.
+    ///> Restrict seletion to only member of the group.
     int GroupEnterSelected( const TOOL_EVENT& aEvent );
 
+    ///> Leave the current group (deselect its members and select the group as a whole)
+    int GroupLeave( const TOOL_EVENT& aEvent );
+
     ///> Runs the drill origin tool for setting the origin for drill and pick-and-place files.
     int DrillOrigin( const TOOL_EVENT& aEvent );
 

pcbnew/tools/pcbnew_control.cpp

@@ -843,7 +843,17 @@ int PCBNEW_CONTROL::placeBoardItems( std::vector<BOARD_ITEM*>& aItems, bool aIsN
             const_cast<KIID&>( item->m_Uuid ) = KIID();
 
             if( item->Type() == PCB_MODULE_T )
+            {
                 static_cast<MODULE*>( item )->SetPath( KIID_PATH() );
+            }
+            else if( item->Type() == PCB_GROUP_T )
+            {
+                // If pasting a group, its immediate children must be updated to have its new KIID
+                static_cast<PCB_GROUP*>( item )->RunOnChildren( [item] ( BOARD_ITEM* aBrdItem )
+                                                                {
+                                                                    aBrdItem->SetGroup( item->m_Uuid );
+                                                                } );
+            }
         }
 
         // Add or just select items for the move/place command

pcbnew/tools/selection_tool.cpp

@@ -149,11 +149,18 @@ bool SELECTION_TOOL::Init()
                 return !frame->ToolStackIsEmpty();
             };
 
+    auto inGroupCondition =
+            [this] ( const SELECTION& )
+            {
+                return m_enteredGroup != nullptr;
+            };
+
     menu.AddMenu( selectMenu.get(), SELECTION_CONDITIONS::NotEmpty );
     menu.AddSeparator( 1000 );
 
     // "Cancel" goes at the top of the context menu when a tool is active
     menu.AddItem( ACTIONS::cancelInteractive, activeToolCondition, 1 );
+    menu.AddItem( PCB_ACTIONS::groupLeave,    inGroupCondition,    1);
     menu.AddSeparator( 1 );
 
     if( frame )
@@ -169,7 +176,7 @@ void SELECTION_TOOL::Reset( RESET_REASON aReason )
     m_locked = true;
 
     if( m_enteredGroup != NULL )
-        exitGroup();
+        ExitGroup();
 
     if( aReason == TOOL_BASE::MODEL_RELOAD )
     {
@@ -309,7 +316,7 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             m_frame->FocusOnItem( nullptr );
 
             if( m_enteredGroup != NULL )
-                exitGroup();
+                ExitGroup();
 
             ClearSelection();
 
@@ -332,9 +339,7 @@ void SELECTION_TOOL::EnterGroup()
     PCB_GROUP* aGroup = static_cast<PCB_GROUP*>( m_selection[0] );
 
     if( m_enteredGroup != NULL )
-    {
+        ExitGroup();
-        exitGroup();
-    }
 
     ClearSelection();
     m_enteredGroup = aGroup;
@@ -342,8 +347,17 @@ void SELECTION_TOOL::EnterGroup()
 }
 
 
-void SELECTION_TOOL::exitGroup()
+void SELECTION_TOOL::ExitGroup( bool aSelectGroup )
 {
+    // Only continue if there is a group entered
+    if( m_enteredGroup == nullptr )
+        return;
+
+    ClearSelection();
+
+    if( aSelectGroup )
+        select( m_enteredGroup );
+
     m_enteredGroup = NULL;
 }
 
@@ -354,9 +368,6 @@ PCBNEW_SELECTION& SELECTION_TOOL::GetSelection()
 }
 
 
-
-
-
 PCBNEW_SELECTION& SELECTION_TOOL::RequestSelection( CLIENT_SELECTION_FILTER aClientFilter,
                                                     std::vector<BOARD_ITEM*>* aFiltered,
                                                     bool aConfirmLockedItems )
@@ -479,7 +490,9 @@ bool SELECTION_TOOL::selectPoint( const VECTOR2I& aWhere, bool aOnDrag,
 
     if( m_enteredGroup &&
         !m_enteredGroup->GetBoundingBox().Contains( wxPoint( aWhere.x, aWhere.y ) ) )
-        exitGroup();
+    {
+            ExitGroup();
+    }
 
     collector.Collect( board(),
         m_editModules ? GENERAL_COLLECTOR::ModuleItems : GENERAL_COLLECTOR::AllBoardItems,

pcbnew/tools/selection_tool.h

@@ -185,12 +185,20 @@ public:
     }
 
     void EnterGroup();
-    void exitGroup();
+
+    /**
+     * Leave the currently entered group.
+     *
+     * @param aSelectGroup when true will select the group after leaving
+     */
+    void ExitGroup( bool aSelectGroup = false );
+
     void FilterCollectorForGroups( GENERAL_COLLECTOR& aCollector ) const;
 
     PCB_GROUP* GetEnteredGroup() { return m_enteredGroup; }
 
 private:
+
     /**
      * Function selectPoint()
      * Selects an item pointed by the parameter aWhere. If there is more than one item at that