ADDED: Group/Ungroup function

This implements the group/ungroup functions to mark a set of EDA_ITEMs as a unit, allowing them to be moved and rotated as a unit

common/CMakeLists.txt

@@ -467,6 +467,7 @@ set( PCB_COMMON_SRCS
     ${CMAKE_SOURCE_DIR}/pcbnew/class_dimension.cpp
     ${CMAKE_SOURCE_DIR}/pcbnew/class_drawsegment.cpp
     ${CMAKE_SOURCE_DIR}/pcbnew/class_edge_mod.cpp
+    ${CMAKE_SOURCE_DIR}/pcbnew/class_group.cpp
     ${CMAKE_SOURCE_DIR}/pcbnew/class_marker_pcb.cpp
     ${CMAKE_SOURCE_DIR}/pcbnew/class_module.cpp
     ${CMAKE_SOURCE_DIR}/pcbnew/netinfo_item.cpp

common/base_struct.cpp

@@ -762,6 +762,7 @@ static struct EDA_ITEM_DESC
             .Map( PCB_ZONE_AREA_T,      _( "Zone" ) )
             .Map( PCB_ITEM_LIST_T,      _( "Item List" ) )
             .Map( PCB_NETINFO_T,        _( "Net Info" ) )
+            .Map( PCB_GROUP_T,          _( "Group" ) )
 
             .Map( SCH_MARKER_T,         _( "Schematic Marker" ) )
             .Map( SCH_JUNCTION_T,       _( "Junction" ) )

common/pcb.keywords

@@ -111,6 +111,7 @@ fp_text
 full
 general
 grid_origin
+group
 gr_arc
 gr_circle
 gr_curve
@@ -128,6 +129,7 @@ hatch_border_algorithm
 hatch_min_hole_area
 hide
 hole_to_hole_min
+id
 island
 island_removal_mode
 island_area_min
@@ -146,6 +148,7 @@ locked
 loss_tangent
 max_error
 material
+members
 micro
 mid
 min_thickness

include/class_group.h

@@ -0,0 +1,198 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2020 Joshua Redstone redstone at gmail.com
+ * Copyright (C) 1992-2020 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
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+
+/**
+ * @file class_group.h
+ * @brief Class to handle a set of BOARD_ITEMs.
+ * Group parent is always board, not logical parent group.
+ * Group is transparent container - e.g., its position is derived from the position
+ *  of its members.
+ * A selection containing a group implicitly contains its members. However other operations
+ * on sets of items, like committing, updating the view, etc the set is explicit.
+ */
+
+#ifndef CLASS_GROUP_H_
+#define CLASS_GROUP_H_
+
+#include <class_board_item.h>
+#include <common.h>
+#include <unordered_set>
+
+namespace KIGFX
+{
+class VIEW;
+}
+
+typedef std::unordered_set<BOARD_ITEM*> ITEM_SET;
+
+/**
+ * GROUP is a set of BOARD_ITEMs (i.e., without duplicates)
+ */
+class GROUP : public BOARD_ITEM
+{
+public:
+    GROUP( BOARD* parent );
+
+    static inline bool ClassOf( const EDA_ITEM* aItem )
+    {
+        return aItem && PCB_GROUP_T == aItem->Type();
+    }
+
+    wxString GetName() const
+    {
+        return m_name;
+    }
+
+    const ITEM_SET& GetItems() const
+    {
+        return m_items;
+    }
+
+    void SetName( wxString name )
+    {
+        m_name = name;
+    }
+
+    /**
+     * Adds item to group. Does not take ownership of item.
+     * @return true if item was added (false if item was already in set).
+     */
+    bool AddItem( BOARD_ITEM* item );
+
+    /**
+     * Removes item from group.
+     * @return true if item was removed (false if item was not in the group).
+     */
+    bool RemoveItem( const BOARD_ITEM* item );
+
+    wxString GetClass() const override
+    {
+        return wxT( "GROUP" );
+    }
+
+#if defined( DEBUG )
+    void Show( int nestLevel, std::ostream& os ) const override
+    {
+        ShowDummy( os );
+    }
+#endif
+
+    ///> @copydoc EDA_ITEM::GetPosition
+    wxPoint GetPosition() const override;
+
+    ///> @copydoc EDA_ITEM::SetPosition
+    void SetPosition( const wxPoint& ) override;
+
+    ///> @copydoc BOARD_ITEM::GetLayerSet
+    LSET GetLayerSet() const override;
+
+    ///> @copydoc BOARD_ITEM::SetLayer
+    void SetLayer( PCB_LAYER_ID aLayer ) override
+    {
+        wxFAIL_MSG( _( "groups don't support layer SetLayer" ) );
+    }
+
+    ///> @copydoc EDA_ITEM::Clone
+    EDA_ITEM* Clone() const override;
+
+    /*
+     * Clone() this and all descendents
+     */
+    GROUP* DeepClone() const;
+
+    /*
+     * Duplicate() this and all descendents
+     */
+    GROUP* DeepDuplicate() const;
+
+    ///> @copydoc BOARD_ITEM::SwapData
+    void SwapData( BOARD_ITEM* aImage ) override;
+
+    ///> @copydoc BOARD_ITEM::IsOnLayer
+    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;
+
+    ///> @copydoc EDA_ITEM::HitTest
+    bool HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy = 0 ) const override;
+
+    ///> @copydoc EDA_ITEM::GetBoundingBox
+    const EDA_RECT GetBoundingBox() const override;
+
+    ///> @copydoc EDA_ITEM::Visit
+    SEARCH_RESULT Visit( INSPECTOR inspector, void* testData, const KICAD_T scanTypes[] ) override;
+
+    ///> @copydoc VIEW_ITEM::ViewGetLayers
+    void ViewGetLayers( int aLayers[], int& aCount ) const override;
+
+    ///> @copydoc VIEW_ITEM::ViewGetLOD
+    unsigned int ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const override;
+
+    ///> @copydoc BOARD_ITEM::Move
+    void Move( const wxPoint& aMoveVector ) override;
+
+    ///> @copydoc BOARD_ITEM::Rotate
+    void Rotate( const wxPoint& aRotCentre, double aAngle ) override;
+
+    ///> @copydoc BOARD_ITEM::Flip
+    void Flip( const wxPoint& aCentre, bool aFlipLeftRight ) override;
+
+    ///> @copydoc EDA_ITEM::GetSelectMenuText
+    wxString GetSelectMenuText( EDA_UNITS aUnits ) const override;
+
+    ///> @copydoc EDA_ITEM::GetMenuImage
+    BITMAP_DEF GetMenuImage() const override;
+
+    ///> @copydoc EDA_ITEM::GetMsgPanelInfo
+    void GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList ) override;
+
+    /**
+     * Invokes a function on all members of the group.
+     * Note that this function should not add or remove items to the group
+     * @param aFunction is the function to be invoked.
+     */
+    void RunOnChildren( const std::function<void ( BOARD_ITEM* )>& aFunction );
+
+    /**
+     * Invokes a function on all descendents of the group.
+     * Note that this function should not add or remove items to the group or descendent 
+     * groups.
+     * @param aFunction is the function to be invoked.
+     */
+    void RunOnDescendants( const std::function<void( BOARD_ITEM* )>& aFunction );
+
+private:
+    // Members of the group
+    ITEM_SET m_items;
+
+    // Optional group name
+    wxString m_name;
+};
+
+#endif // CLASS_GROUP_H_

include/core/typeinfo.h

@@ -102,6 +102,7 @@ enum KICAD_T
     PCB_ZONE_AREA_T,        ///< class ZONE_CONTAINER, a zone area
     PCB_ITEM_LIST_T,        ///< class BOARD_ITEM_LIST, a list of board items
     PCB_NETINFO_T,          ///< class NETINFO_ITEM, a description of a net
+    PCB_GROUP_T,            ///< class GROUP, a set of BOARD_ITEMs
 
     PCB_LOCATE_STDVIA_T,
     PCB_LOCATE_UVIA_T,

pcbnew/board_commit.cpp

@@ -220,6 +220,7 @@ void BOARD_COMMIT::Push( const wxString& aMessage, bool aCreateUndoEntry, bool a
                 case PCB_DIMENSION_T:           // a dimension (graphic item)
                 case PCB_TARGET_T:              // a target (graphic item)
                 case PCB_MARKER_T:              // a marker used to show something
+                case PCB_GROUP_T:               // a group of items
                 case PCB_ZONE_AREA_T:
                     view->Remove( boardItem );
 
@@ -424,3 +425,9 @@ void BOARD_COMMIT::Revert()
 
     clear();
 }
+
+bool BOARD_COMMIT::HasRemoveEntry( EDA_ITEM* aItem )
+{
+    COMMIT::COMMIT_LINE* line = findEntry( aItem );
+    return line != nullptr && line->m_type == CHT_REMOVE;
+}

pcbnew/board_commit.h

@@ -51,6 +51,11 @@ public:
     COMMIT&      Stage(
                  const PICKED_ITEMS_LIST& aItems, UNDO_REDO_T aModFlag = UR_UNSPECIFIED ) override;
 
+    /*
+     * @return true iff the commit has an entry to remove aItem.
+     */
+    bool         HasRemoveEntry( EDA_ITEM* aItem );
+
 private:
     TOOL_MANAGER* m_toolMgr;
     bool m_editModules;

pcbnew/class_board.cpp

@@ -31,6 +31,7 @@
 #include <pcb_base_frame.h>
 #include <reporter.h>
 #include <ws_proxy_view_item.h>
+#include <board_commit.h>
 #include <class_board.h>
 #include <class_module.h>
 #include <class_track.h>
@@ -47,6 +48,7 @@
 #include <project/project_local_settings.h>
 #include <ratsnest/ratsnest_data.h>
 #include <ratsnest/ratsnest_viewitem.h>
+#include <tool/selection_conditions.h>
 
 /* This is an odd place for this, but CvPcb won't link if it is
  *  in class_board_item.cpp like I first tried it.
@@ -133,6 +135,8 @@ BOARD::~BOARD()
 
     delete m_CurrentZoneContour;
     m_CurrentZoneContour = NULL;
+
+    m_groups.clear();
 }
 
 
@@ -557,6 +561,11 @@ void BOARD::Add( BOARD_ITEM* aBoardItem, ADD_MODE aMode )
         m_markers.push_back( (MARKER_PCB*) aBoardItem );
         break;
 
+    // this one uses a vector
+    case PCB_GROUP_T:
+        m_groups.push_back( (GROUP*) aBoardItem );
+        break;
+
     // this one uses a vector
     case PCB_ZONE_AREA_T:
         m_ZoneDescriptorList.push_back( (ZONE_CONTAINER*) aBoardItem );
@@ -647,6 +656,12 @@ void BOARD::Remove( BOARD_ITEM* aBoardItem )
 
         break;
 
+    case PCB_GROUP_T:
+        m_groups.erase( std::remove_if( m_groups.begin(), m_groups.end(),
+                [aBoardItem]( BOARD_ITEM* aItem ){ return aItem == aBoardItem; } ) );
+
+        break;
+
     case PCB_ZONE_AREA_T:    // this one uses a vector
         // find the item in the vector, then delete then erase it.
         for( unsigned i = 0; i<m_ZoneDescriptorList.size(); ++i )
@@ -788,6 +803,10 @@ BOARD_ITEM* BOARD::GetItem( const KIID& aID )
         if( marker->m_Uuid == aID )
             return marker;
 
+    for( GROUP* group : m_groups )
+        if( group->m_Uuid == aID )
+            return group;
+
     if( m_Uuid == aID )
         return this;
 
@@ -823,6 +842,9 @@ void BOARD::FillItemMap( std::map<KIID, EDA_ITEM*>& aMap )
 
     for( MARKER_PCB* marker : m_markers )
         aMap[ marker->m_Uuid ] = marker;
+
+    for( GROUP* group : m_groups )
+        aMap[ group->m_Uuid ] = group;
 }
 
 
@@ -1109,6 +1131,11 @@ SEARCH_RESULT BOARD::Visit( INSPECTOR inspector, void* testData, const KICAD_T s
             ++p;
             break;
 
+        case PCB_GROUP_T:
+            result = IterateForward<GROUP*>( m_groups, inspector, testData, p );
+            ++p;
+            break;
+
         default:        // catch EOT or ANY OTHER type here and return.
             done = true;
             break;
@@ -1960,3 +1987,397 @@ void BOARD::HighLightON( bool aValue )
         InvokeListeners( &BOARD_LISTENER::OnBoardHighlightNetChanged, *this );
     }
 }
+
+GROUP* BOARD::TopLevelGroup( BOARD_ITEM* item, GROUP* scope  )
+{
+    GROUP* candidate = NULL;
+    bool foundParent;
+
+    do
+    {
+        foundParent = false;
+        for( GROUP* group : m_groups )
+        {
+            BOARD_ITEM* toFind = ( candidate == NULL ) ? item : candidate;
+            if( group->GetItems().find( toFind ) != group->GetItems().end() )
+            {
+                if( scope == group && candidate != NULL )
+                {
+                    wxCHECK( candidate->Type() == PCB_GROUP_T, NULL );
+                    return candidate;
+                }
+
+                candidate   = group;
+                foundParent = true;
+            }
+        }
+    } while( foundParent );
+
+    if( scope != NULL )
+    {
+        return NULL;
+    }
+
+    return candidate;
+}
+
+
+GROUP* BOARD::ParentGroup( BOARD_ITEM* item )
+{
+    for( GROUP* group : m_groups )
+    {
+        if( group->GetItems().find( item ) != group->GetItems().end() )
+            return group;
+    }
+    return NULL;
+}
+
+
+wxString BOARD::GroupsSanityCheck( bool repair )
+{
+    if( repair )
+    {
+        while( GroupsSanityCheckInternal( repair ) != wxEmptyString );
+        return wxEmptyString;
+    }
+    return GroupsSanityCheckInternal( repair );
+}
+
+
+wxString BOARD::GroupsSanityCheckInternal( bool repair )
+{
+    BOARD&                       board  = *this;
+    GROUPS&                      groups = board.Groups();
+    std::unordered_set<wxString> groupNames;
+    std::unordered_set<wxString> allMembers;
+
+    // To help with cycle detection, construct a mapping from
+    // each group to the at most single parent group it could belong to.
+    std::vector<int> parentGroupIdx( groups.size(), -1 );
+
+    for( size_t idx = 0; idx < groups.size(); idx++ )
+    {
+        GROUP& group    = *( groups[idx] );
+        BOARD_ITEM*  testItem = board.GetItem( group.m_Uuid );
+
+        if( testItem != groups[idx] )
+        {
+            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] );
+        }
+
+        // Non-blank group names must be unique
+        if( !group.GetName().empty() )
+        {
+            if( groupNames.find( group.GetName() ) != groupNames.end() )
+            {
+                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() )
+        {
+            BOARD_ITEM* item = board.GetItem( member->m_Uuid );
+
+            if( ( item == nullptr ) || ( item->Type() == NOT_USED ) )
+            {
+                if( repair )
+                    group.RemoveItem( member );
+                return wxString::Format( _( "Group %s contains deleted item %s" ),
+                                            group.m_Uuid.AsString(),
+                                            member->m_Uuid.AsString() );
+            }
+
+            if( item != member )
+            {
+                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(),
+                                          item,
+                                          item->GetSelectMenuText( EDA_UNITS::MILLIMETRES ),
+                                          member,
+                                          member->GetSelectMenuText( EDA_UNITS::MILLIMETRES )
+                    );
+            }
+
+            if( allMembers.find( member->m_Uuid.AsString() ) != allMembers.end() )
+            {
+                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" ) );
+
+            if( item->Type() == PCB_GROUP_T )
+            {
+                // Could speed up with a map structure if needed
+                size_t childIdx = std::distance(
+                        groups.begin(), std::find( groups.begin(), groups.end(), item ) );
+                // This check of childIdx should never fail, because if a group
+                // is not found in the groups list, then the board.GetItem()
+                // check above should have failed.
+                wxCHECK( childIdx >= 0 && childIdx < groups.size(),
+                         wxString::Format( _( "Group %s not found in groups list" ),
+                                           item->m_Uuid.AsString() ) );
+                wxCHECK( parentGroupIdx[childIdx] == -1,
+                         wxString::Format( _( "Duplicate group despite allMembers check previously: %s" ),
+                                           item->m_Uuid.AsString() ) );
+                parentGroupIdx[childIdx] = idx;
+            }
+        }
+
+        if( group.GetItems().size() == 0 )
+        {
+            if( repair )
+                board.Groups().erase( board.Groups().begin() + idx );
+            return wxString::Format( _( "Group must have at least one member: %s" ), group.m_Uuid.AsString() );
+        }
+    }
+
+    // Cycle detection
+    //
+    // Each group has at most one parent group.
+    // So we start at group 0 and traverse the parent chain, marking groups seen along the way.
+    // If we ever see a group that we've already marked, that's a cycle.
+    // If we reach the end of the chain, we know all groups in that chain are not part of any cycle.
+    //
+    // Algorithm below is linear in the # of groups because each group is visited only once.
+    // There may be extra time taken due to the container access calls and iterators.
+    //
+    // Groups we know are cycle free
+    std::unordered_set<int> knownCycleFreeGroups;
+    // Groups in the current chain we're exploring.
+    std::unordered_set<int> currentChainGroups;
+    // Groups we haven't checked yet.
+    std::unordered_set<int> toCheckGroups;
+
+    // Initialize set of groups to check that could participate in a cycle.
+    for( size_t idx = 0; idx < groups.size(); idx++ )
+    {
+        wxCHECK( toCheckGroups.insert( idx ).second == true, _( "Insert of ints failed" ) );
+    }
+
+    while( !toCheckGroups.empty() )
+    {
+        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() )
+            {
+                // 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 "";
+}
+
+
+BOARD::GroupLegalOpsField BOARD::GroupLegalOps( const PCBNEW_SELECTION& selection ) const
+{
+    GroupLegalOpsField legalOps = { false, false, false, false, false, false };
+
+    std::unordered_set<const BOARD_ITEM*> allMembers;
+    for( const GROUP* grp : m_groups )
+    {
+        for( const BOARD_ITEM* member : grp->GetItems() )
+        {
+            // Item can be member of at most one group.
+            wxCHECK( allMembers.insert( member ).second == true, legalOps );
+        }
+    }
+
+
+    bool hasGroup              = ( SELECTION_CONDITIONS::HasType( PCB_GROUP_T ) )( selection );
+    // All elements of selection are groups, and no element is a descendant group of any other.
+    bool onlyGroups            = ( SELECTION_CONDITIONS::OnlyType( PCB_GROUP_T ) )( selection );
+    // Any elements of the selections are already members of groups
+    bool anyGrouped            = false;
+    // Any elements of the selections, except the first group, are already members of groups.
+    bool anyGroupedExceptFirst = false;
+    // All elements of the selections are already members of groups
+    bool allGrouped            = true;
+    bool seenFirstGroup        = false;
+
+    if( onlyGroups )
+    {
+        // Check that no groups are descendant subgroups of another group in the selection
+        for( EDA_ITEM* item : selection )
+        {
+            const GROUP*                     group = static_cast<const GROUP*>( item );
+            std::unordered_set<const GROUP*> subgroupos;
+            std::queue<const GROUP*>         toCheck;
+            toCheck.push( group );
+
+            while( !toCheck.empty() )
+            {
+                const GROUP* candidate = toCheck.front();
+                toCheck.pop();
+
+                for( const BOARD_ITEM* aChild : candidate->GetItems() )
+                {
+                    if( aChild->Type() == PCB_GROUP_T )
+                    {
+                        const GROUP* childGroup = static_cast<const GROUP*>( aChild );
+                        subgroupos.insert( childGroup );
+                        toCheck.push( childGroup );
+                    }
+                }
+            }
+
+            for( EDA_ITEM* otherItem : selection )
+            {
+                if( otherItem != item
+                    && subgroupos.find( static_cast<GROUP*>( otherItem ) ) != subgroupos.end() )
+                {
+                    // otherItem is a descendant subgroup of item
+                    onlyGroups = false;
+                }
+            }
+        }
+    }
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item   = static_cast<BOARD_ITEM*>( item );
+        bool        isFirstGroup = !seenFirstGroup && board_item->Type() == PCB_GROUP_T;
+
+        if( isFirstGroup )
+        {
+            seenFirstGroup = true;
+        }
+
+        if( allMembers.find( board_item ) == allMembers.end() )
+        {
+            allGrouped = false;
+        }
+        else
+        {
+            anyGrouped = true;
+
+            if( !isFirstGroup )
+            {
+                anyGroupedExceptFirst = true;
+            }
+        }
+    }
+
+    legalOps.create      = !anyGrouped;
+    legalOps.merge       = hasGroup && !anyGroupedExceptFirst && ( selection.Size() > 1 );
+    legalOps.ungroup     = onlyGroups;
+    legalOps.removeItems = allGrouped;
+    legalOps.flatten     = onlyGroups;
+    legalOps.enter       = onlyGroups && selection.Size() == 1;
+    return legalOps;
+}
+
+void BOARD::GroupRemoveItems( const PCBNEW_SELECTION& selection, BOARD_COMMIT* commit )
+{
+    std::unordered_set<BOARD_ITEM*> emptyGroups;
+    std::unordered_set<GROUP*> emptyGroupParents;
+
+    // groups who have had children removed, either items or empty groups.
+    std::unordered_set<GROUP*> itemParents;
+    std::unordered_set<BOARD_ITEM*> itemsToRemove;
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
+        itemsToRemove.insert( board_item );
+    }
+
+    for( BOARD_ITEM* item : itemsToRemove )
+    {
+        GROUP* parentGroup = ParentGroup( item );
+        itemParents.insert( parentGroup );
+
+        while( parentGroup != nullptr )
+        {
+            // Test if removing this item would make parent empty
+            bool allRemoved = true;
+
+            for( BOARD_ITEM* grpItem : parentGroup->GetItems() )
+            {
+                if( ( itemsToRemove.find( grpItem ) == itemsToRemove.end() )
+                    && ( emptyGroups.find( grpItem ) == emptyGroups.end() ) )
+                    allRemoved = false;
+            }
+
+            if( allRemoved )
+            {
+                emptyGroups.insert( parentGroup );
+                parentGroup = ParentGroup( parentGroup );
+
+                if( parentGroup != nullptr )
+                    itemParents.insert( parentGroup );
+            }
+            else
+            {
+                break;
+            }
+        }
+    }
+
+    // Items themselves are removed outside the context of this function
+    // First let's check the parents of items that are no empty
+    for( GROUP* grp : itemParents )
+    {
+        if( emptyGroups.find( grp ) == emptyGroups.end() )
+        {
+            commit->Modify( grp );
+            ITEM_SET members = grp->GetItems();
+            bool removedSomething = false;
+
+            for( BOARD_ITEM* member : members )
+            {
+                if( ( itemsToRemove.find( member ) != itemsToRemove.end() )
+                    || ( emptyGroups.find( member ) != emptyGroups.end() ) )
+                {
+                    grp->RemoveItem( member );
+                    removedSomething = true;
+                }
+            }
+            wxCHECK_RET( removedSomething, _( "Item to be removed not found in it's parent group" ) );
+        }
+    }
+
+    for( BOARD_ITEM* grp : emptyGroups )
+    {
+        commit->Remove( grp );
+    }
+}
+

pcbnew/class_board.h

@@ -28,6 +28,7 @@
 #include <tuple>
 #include <board_design_settings.h>
 #include <board_item_container.h>
+#include <class_group.h>
 #include <class_module.h>
 #include <class_pad.h>
 #include <common.h> // PAGE_INFO
@@ -37,9 +38,11 @@
 #include <pcb_plot_params.h>
 #include <title_block.h>
 #include <zone_settings.h>
+#include <tools/pcbnew_selection.h>
 
 #include <memory>
 
+class BOARD_COMMIT;
 class PCB_BASE_FRAME;
 class PCB_EDIT_FRAME;
 class PICKED_ITEMS_LIST;
@@ -171,7 +174,8 @@ public:
 DECL_VEC_FOR_SWIG( MARKERS, MARKER_PCB* )
 DECL_VEC_FOR_SWIG( ZONE_CONTAINERS, ZONE_CONTAINER* )
 DECL_DEQ_FOR_SWIG( TRACKS, TRACK* )
-
+// Dequeue rather than Vector just so we can use moveUnflaggedItems in pcbnew_control.cpp
+DECL_DEQ_FOR_SWIG( GROUPS, GROUP* )
 
 /**
  * BOARD
@@ -197,6 +201,9 @@ private:
     /// TRACKS for traces on the board, owned by pointer.
     TRACKS                  m_tracks;
 
+    /// GROUPS for groups on the board, owned by pointer.
+    GROUPS                  m_groups;
+    
     /// edge zone descriptors, owned by pointer.
     ZONE_CONTAINERS         m_ZoneDescriptorList;
 
@@ -287,6 +294,19 @@ public:
         return m_markers;
     }
 
+    /**
+     * The groups must maintain the folowing invariants. These are checked by 
+     * GroupsSanityCheck():
+     *   - An item may appear in at most one group
+     *   - Each gruop must contain at least one item
+     *   - If a group specifies a name, it must be unique
+     *   - The graph of groups contianing subgroups must be acyclic.
+     */
+    GROUPS& Groups()
+    {
+        return m_groups;
+    }
+
     const std::vector<BOARD_CONNECTED_ITEM*> AllConnectedItems();
 
     /// zone contour currently in progress
@@ -343,6 +363,10 @@ public:
         m_modules.clear();
     }
 
+    /**
+     * @return null if aID is null. Returns an object of Type() == NOT_USED if
+     * the aID is not found.
+     */
     BOARD_ITEM* GetItem( const KIID& aID );
 
     void FillItemMap( std::map<KIID, EDA_ITEM*>& aMap );
@@ -1190,6 +1214,56 @@ public:
       * been modified in some way.
       */
     void OnItemChanged( BOARD_ITEM* aItem );
-};
 
+    /*
+     * Consistency check of internal m_groups structure.
+     * @param repair if true, modify groups structure until it passes the sanity check.
+     * @return empty string on success.  Or error description if there's a problem.
+     */
+    wxString GroupsSanityCheck( bool repair = false );
+
+    /*
+     * @param repair if true, make one modification to groups structure that brings it
+     *        closer to passing the sanity check.
+     * @return empty string on success.  Or error description if there's a problem.
+     */
+    wxString GroupsSanityCheckInternal( bool repair );
+
+    /*
+     * Searches for highest level group containing item.
+     * @param scope restricts the search to groups within the group scope.
+     * @return group containing item, if it exists, otherwise, NULL
+     */
+    GROUP* TopLevelGroup( BOARD_ITEM* item, GROUP* scope );
+
+
+    /*
+     * @return The group containing item as a child, or NULL if there is no
+     * such group.
+     */
+    GROUP* ParentGroup( BOARD_ITEM* item );
+
+    /*
+     * Given a selection of items, remove them from their groups and also
+     * recursively remove empty groups that result.
+     */
+    void GroupRemoveItems( const PCBNEW_SELECTION& selection, BOARD_COMMIT* commit );
+  
+
+    struct GroupLegalOpsField
+    {
+        bool create      : 1;
+        bool merge       : 1;
+        bool ungroup     : 1;
+        bool removeItems : 1;
+        bool flatten     : 1;
+        bool enter       : 1;
+    };
+
+    /*
+     * Check which selection tool group operations are legal given the selection.
+     * @return bit field of legal ops.
+     */
+    GroupLegalOpsField GroupLegalOps( const PCBNEW_SELECTION& selection ) const;
+};
 #endif      // CLASS_BOARD_H_

pcbnew/class_group.cpp

@@ -0,0 +1,309 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2020 Joshua Redstone redstone at gmail.com
+ * Copyright (C) 1992-2020 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
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+#include <bitmaps.h>
+#include <class_group.h>
+#include <confirm.h>
+#include <msgpanel.h>
+#include <view/view.h>
+
+GROUP::GROUP( BOARD* parent ) : BOARD_ITEM( (BOARD_ITEM*) parent, PCB_GROUP_T )
+{
+}
+
+
+bool GROUP::AddItem( BOARD_ITEM* item )
+{
+    return m_items.insert( item ).second;
+}
+
+bool GROUP::RemoveItem( const BOARD_ITEM* item )
+{
+    return m_items.erase( const_cast<BOARD_ITEM*>( item ) ) == 1;
+}
+
+wxPoint GROUP::GetPosition() const
+{
+    return GetBoundingBox().Centre();
+}
+
+void GROUP::SetPosition( const wxPoint& newpos )
+{
+    wxPoint delta = newpos - GetPosition();
+
+    for( auto member : m_items )
+    {
+        member->SetPosition( member->GetPosition() + delta );
+    }
+}
+
+EDA_ITEM* GROUP::Clone() const
+{
+    // Use copy constructor to get the same uuid and other fields
+    GROUP* newGroup = new GROUP( *this );
+    return newGroup;
+}
+
+GROUP* GROUP::DeepClone() const
+{
+    // Use copy constructor to get the same uuid and other fields
+    GROUP* newGroup = new GROUP( *this );
+    newGroup->m_items.clear();
+
+    for( auto member : m_items )
+    {
+        if( member->Type() == PCB_GROUP_T )
+        {
+            newGroup->AddItem( static_cast<GROUP*>( member )->DeepClone() );
+        }
+        else
+        {
+            newGroup->AddItem( static_cast<BOARD_ITEM*>( member->Clone() ) );
+        }
+    }
+
+    return newGroup;
+}
+
+
+GROUP* GROUP::DeepDuplicate() const
+{
+    GROUP* newGroup = static_cast<GROUP*>( this->Duplicate() );
+    newGroup->m_items.clear();
+
+    for( auto member : m_items )
+    {
+        if( member->Type() == PCB_GROUP_T )
+        {
+            newGroup->AddItem( static_cast<GROUP*>( member )->DeepDuplicate() );
+        }
+        else
+        {
+            newGroup->AddItem( static_cast<BOARD_ITEM*>( member->Duplicate() ) );
+        }
+    }
+
+    return newGroup;
+}
+
+
+void GROUP::SwapData( BOARD_ITEM* aImage )
+{
+    assert( aImage->Type() == PCB_GROUP_T );
+
+    std::swap( *( (GROUP*) this ), *( (GROUP*) aImage ) );
+}
+
+#if 0
+void GROUP::TransformShapeWithClearanceToPolygon( SHAPE_POLY_SET& aCornerBuffer,
+                                                  int aClearanceValue, int aError = ARC_LOW_DEF, bool ignoreLineWidth = false ) const
+{
+}
+const BOX2I GROUP::ViewBBox() const
+{
+    return GetBoundingBox();
+}
+
+#endif
+
+bool GROUP::HitTest( const wxPoint& aPosition, int aAccuracy ) const
+{
+    EDA_RECT rect = GetBoundingBox();
+    return rect.Inflate( aAccuracy ).Contains( aPosition );
+}
+
+bool GROUP::HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy ) const
+{
+    EDA_RECT arect = aRect;
+    arect.Inflate( aAccuracy );
+
+    EDA_RECT bbox = GetBoundingBox();
+
+    if( aContained )
+        return arect.Contains( bbox );
+    else
+    {
+        // If the rect does not intersect the bounding box, skip any tests
+        if( !aRect.Intersects( bbox ) )
+            return false;
+
+        for( BOARD_ITEM* member : m_items )
+        {
+            if( member->HitTest( arect, false, 0 ) )
+                return true;
+        }
+
+        // No items were hit
+        return false;
+    }
+}
+
+const EDA_RECT GROUP::GetBoundingBox() const
+{
+    EDA_RECT area;
+    bool     isFirst = true;
+
+    for( BOARD_ITEM* item : m_items )
+    {
+        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 GROUP::Visit( INSPECTOR inspector, void* testData, const KICAD_T scanTypes[] )
+{
+    for( const KICAD_T* stype = scanTypes; *stype != EOT; ++stype )
+    {
+        // If caller wants to inspect my type
+        if( *stype == Type() )
+        {
+            if( SEARCH_RESULT::QUIT == inspector( this, testData ) )
+                return SEARCH_RESULT::QUIT;
+        }
+    }
+
+    return SEARCH_RESULT::CONTINUE;
+}
+
+LSET GROUP::GetLayerSet() const
+{
+    LSET aSet;
+
+    for( BOARD_ITEM* item : m_items )
+    {
+        aSet |= item->GetLayerSet();
+    }
+    return aSet;
+}
+
+void GROUP::ViewGetLayers( int aLayers[], int& aCount ) const
+{
+    // What layer to put bounding box on?  change in class_group.cpp
+    std::unordered_set<int> layers = { LAYER_ANCHOR }; // for bounding box
+
+    for( BOARD_ITEM* item : m_items )
+    {
+        int member_layers[KIGFX::VIEW::VIEW_MAX_LAYERS], member_layers_count;
+        item->ViewGetLayers( member_layers, member_layers_count );
+
+        for( int i = 0; i < member_layers_count; i++ )
+            layers.insert( member_layers[i] );
+    }
+
+    aCount = layers.size();
+    int i  = 0;
+
+    for( int layer : layers )
+        aLayers[i++] = layer;
+}
+
+unsigned int GROUP::ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const
+{
+    if( aView->IsLayerVisible( LAYER_ANCHOR ) )
+        return 0;
+
+    return std::numeric_limits<unsigned int>::max();
+}
+
+void GROUP::Move( const wxPoint& aMoveVector )
+{
+    wxPoint newpos = GetPosition() + aMoveVector;
+    SetPosition( newpos );
+}
+
+void GROUP::Rotate( const wxPoint& aRotCentre, double aAngle )
+{
+    for( BOARD_ITEM* item : m_items )
+    {
+        item->Rotate( aRotCentre, aAngle );
+    }
+}
+
+void GROUP::Flip( const wxPoint& aCentre, bool aFlipLeftRight )
+{
+    for( BOARD_ITEM* item : m_items )
+    {
+        item->Flip( aCentre, aFlipLeftRight );
+    }
+}
+
+wxString GROUP::GetSelectMenuText( EDA_UNITS aUnits ) const
+{
+    if( m_name.empty() )
+    {
+        return wxString::Format( _( "Anonymous group %s with %ld members" ), m_Uuid.AsString(), m_items.size() );
+    }
+    return wxString::Format( _( "Group \"%s\" with %ld members" ), m_name, m_items.size() );
+}
+
+BITMAP_DEF GROUP::GetMenuImage() const
+{
+    return module_xpm;
+}
+
+void GROUP::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vector<MSG_PANEL_ITEM>& aList )
+{
+    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 );
+}
+
+void GROUP::RunOnChildren( const std::function<void( BOARD_ITEM* )>& aFunction )
+{
+    try
+    {
+        for( BOARD_ITEM* item : m_items )
+            aFunction( item );
+    }
+    catch( std::bad_function_call& )
+    {
+        DisplayError( NULL, wxT( "Error running GROUP::RunOnChildren" ) );
+    }
+}
+
+void GROUP::RunOnDescendants( const std::function<void( BOARD_ITEM* )>& aFunction )
+{
+    try
+    {
+        for( BOARD_ITEM* item : m_items )
+        {
+            aFunction( item );
+            if( item->Type() == PCB_GROUP_T )
+                static_cast<GROUP*>( item )->RunOnDescendants( aFunction );
+        }
+    }
+    catch( std::bad_function_call& )
+    {
+        DisplayError( NULL, wxT( "Error running GROUP::RunOnDescendants" ) );
+    }
+}

pcbnew/collectors.cpp

@@ -32,6 +32,7 @@
 #include <class_marker_pcb.h>
 #include <class_zone.h>
 #include <class_drawsegment.h>
+#include <class_group.h>
 #include <macros.h>
 #include <math/util.h>      // for KiROUND
 
@@ -56,6 +57,7 @@ const KICAD_T GENERAL_COLLECTOR::AllBoardItems[] = {
     PCB_PAD_T,                   // in modules
     PCB_MODULE_TEXT_T,           // in modules
     PCB_MODULE_T,                // in m_Modules
+    PCB_GROUP_T,                 // in m_Groups ?
     PCB_ZONE_AREA_T,             // in m_ZoneDescriptorList
     EOT
 };
@@ -71,6 +73,7 @@ const KICAD_T GENERAL_COLLECTOR::BoardLevelItems[] = {
     PCB_ARC_T,
     PCB_TRACE_T,
     PCB_MODULE_T,
+    PCB_GROUP_T,
     PCB_ZONE_AREA_T,
     EOT
 };
@@ -88,6 +91,7 @@ const KICAD_T GENERAL_COLLECTOR::AllButZones[] = {
     PCB_PAD_T,
     PCB_MODULE_TEXT_T,
     PCB_MODULE_T,
+    PCB_GROUP_T,
     PCB_ZONE_AREA_T,         // if it is visible on screen, it should be selectable
     EOT
 };
@@ -144,6 +148,7 @@ const KICAD_T GENERAL_COLLECTOR::Tracks[] = {
 
 const KICAD_T GENERAL_COLLECTOR::LockableItems[] = {
     PCB_MODULE_T,
+    PCB_GROUP_T,  // Can a group be locked?
     PCB_TRACE_T,
     PCB_ARC_T,
     PCB_VIA_T,
@@ -163,6 +168,7 @@ SEARCH_RESULT GENERAL_COLLECTOR::Inspect( EDA_ITEM* testItem, void* testData )
 {
     BOARD_ITEM*     item        = (BOARD_ITEM*) testItem;
     MODULE*         module      = nullptr;
+    GROUP*          group       = nullptr;
     D_PAD*          pad         = nullptr;
     bool            pad_through = false;
     VIA*            via         = nullptr;
@@ -349,6 +355,10 @@ SEARCH_RESULT GENERAL_COLLECTOR::Inspect( EDA_ITEM* testItem, void* testData )
         module = static_cast<MODULE*>( item );
         break;
 
+    case PCB_GROUP_T:
+        group = static_cast<GROUP*>( item );
+        break;
+
     case PCB_MARKER_T:
         marker = static_cast<MARKER_PCB*>( item );
         break;
@@ -395,6 +405,15 @@ SEARCH_RESULT GENERAL_COLLECTOR::Inspect( EDA_ITEM* testItem, void* testData )
         goto exit;
     }
 
+    if( group )
+    {
+        // Groups are not sensitive to the layer ... ?
+        if( group->HitTest( m_RefPos ) )
+            Append( item );
+
+        goto exit;
+    }
+
     if( via )
     {
         auto type = via->GetViaType();

pcbnew/kicad_clipboard.cpp

@@ -191,24 +191,40 @@ void CLIPBOARD_IO::SaveSelection( const PCBNEW_SELECTION& aSelected, bool isModE
                 zone->InitDataFromSrcInCopyCtor( *static_cast<ZONE_CONTAINER*>( item ) );
                 copy = zone;
             }
+            else if( item->Type() == PCB_GROUP_T )
+            {
+                copy = static_cast<GROUP*>( item )->DeepClone();
+            }
             else
             {
                 copy = static_cast<BOARD_ITEM*>( item->Clone() );
-
-                // locked means "locked in place"; copied items therefore can't be locked
-                if( MODULE* module = dyn_cast<MODULE*>( copy ) )
-                    module->SetLocked( false );
-                else if( TRACK* track = dyn_cast<TRACK*>( copy ) )
-                    track->SetLocked( false );
             }
 
+            auto prepItem = [&]( BOARD_ITEM* titem ) {
+                                // locked means "locked in place"; copied items therefore can't be locked
+                                if( MODULE* module = dyn_cast<MODULE*>( titem ) )
+                                    module->SetLocked( false );
+                                else if( TRACK* track = dyn_cast<TRACK*>( titem ) )
+                                    track->SetLocked( false );
+                            };
+
             if( copy )
             {
+                prepItem( copy );
+
                 // locate the reference point at (0, 0) in the copied items
                 copy->Move( (wxPoint) -refPoint );
 
                 Format( copy, 1 );
 
+                if( copy->Type() == PCB_GROUP_T )
+                {
+                    static_cast<GROUP*>( copy )->RunOnDescendants( prepItem );
+                    static_cast<GROUP*>( copy )->RunOnDescendants( [&]( BOARD_ITEM* titem ) {
+                                                                       Format( titem, 1 );
+                                                                   } );
+                }
+
                 delete copy;
             }
         }

pcbnew/kicad_plugin.cpp

@@ -40,6 +40,7 @@
 #include <class_drawsegment.h>
 #include <class_pcb_target.h>
 #include <class_edge_mod.h>
+#include <confirm.h>
 #include <zones.h>
 #include <kicad_plugin.h>
 #include <pcb_parser.h>
@@ -355,6 +356,21 @@ void PCB_IO::Save( const wxString& aFileName, BOARD* aBoard, const PROPERTIES* a
 {
     LOCALE_IO   toggle;     // toggles on, then off, the C locale.
 
+    wxString sanityResult = aBoard->GroupsSanityCheck();
+
+    if( sanityResult != wxEmptyString )
+    {
+        KIDIALOG dlg( nullptr, wxString::Format(
+             _( "Please report this bug.  Error validating group structure: %s"
+                "\n\nSave anyways?" ), sanityResult ),
+                      _( "Internal group data structure corrupt" ),
+                      wxOK | wxCANCEL | wxICON_ERROR );
+        dlg.SetOKLabel( _( "Save Anyway" ) );
+
+        if( dlg.ShowModal() == wxID_CANCEL )
+            return;
+    }
+
     init( aProperties );
 
     m_board = aBoard;       // after init()
@@ -439,6 +455,10 @@ void PCB_IO::Format( BOARD_ITEM* aItem, int aNestLevel ) const
         format( static_cast<TEXTE_MODULE*>( aItem ), aNestLevel );
         break;
 
+    case PCB_GROUP_T:
+        format( static_cast<GROUP*>( aItem ), aNestLevel );
+        break;
+
     case PCB_TRACE_T:
     case PCB_ARC_T:
     case PCB_VIA_T:
@@ -615,6 +635,8 @@ void PCB_IO::format( BOARD* aBoard, int aNestLevel ) const
             aBoard->Tracks().end() );
     std::set<BOARD_ITEM*, BOARD_ITEM::ptr_cmp> sorted_zones( aBoard->Zones().begin(),
             aBoard->Zones().end() );
+    std::set<BOARD_ITEM*, BOARD_ITEM::ptr_cmp> sorted_groups( aBoard->Groups().begin(),
+            aBoard->Groups().end() );
 
     formatHeader( aBoard, aNestLevel );
 
@@ -644,6 +666,10 @@ void PCB_IO::format( BOARD* aBoard, int aNestLevel ) const
     // Save the polygon (which are the newer technology) zones.
     for( auto zone : sorted_zones )
         Format( zone, aNestLevel );
+
+    // Save the groups
+    for( const auto group : sorted_groups )
+        Format( group, aNestLevel );
 }
 
 
@@ -1491,6 +1517,24 @@ void PCB_IO::format( TEXTE_PCB* aText, int aNestLevel ) const
 }
 
 
+void PCB_IO::format( GROUP* aGroup, int aNestLevel ) const
+{
+    m_out->Print( aNestLevel, "(group %s (id %s)\n", m_out->Quotew( aGroup->GetName() ).c_str(),
+            TO_UTF8( aGroup->m_Uuid.AsString() ) );
+    m_out->Print( aNestLevel + 2, "(members\n" );
+    std::set<BOARD_ITEM*, BOARD_ITEM::ptr_cmp> sorted_items( aGroup->GetItems().begin(),
+            aGroup->GetItems().end() );
+
+    for( const auto& item : sorted_items )
+    {
+        m_out->Print( aNestLevel + 4, "%s\n", TO_UTF8( item->m_Uuid.AsString() ) );
+    }
+
+    m_out->Print( 0, " )\n" );
+    m_out->Print( aNestLevel, ")\n" );
+}
+
+
 void PCB_IO::format( TEXTE_MODULE* aText, int aNestLevel ) const
 {
     std::string type;

pcbnew/kicad_plugin.h

@@ -41,6 +41,7 @@ class DRAWSEGMENT;
 class PCB_TARGET;
 class D_PAD;
 class TEXTE_MODULE;
+class GROUP;
 class TRACK;
 class ZONE_CONTAINER;
 class TEXTE_PCB;
@@ -75,7 +76,8 @@ class TEXTE_PCB;
 //#define SEXPR_BOARD_FILE_VERSION    20200724  // Add KIID to module components
 //#define SEXPR_BOARD_FILE_VERSION    20200807  // Add zone hatch advanced settings
 //#define SEXPR_BOARD_FILE_VERSION    20200808  // Add properties to modules
-#define SEXPR_BOARD_FILE_VERSION      20200809  // Add REMOVE_UNUSED_LAYERS option to vias and THT pads
+//#define SEXPR_BOARD_FILE_VERSION    20200809  // Add REMOVE_UNUSED_LAYERS option to vias and THT pads
+#define SEXPR_BOARD_FILE_VERSION      20200811  // Add groups
 
 #define CTL_STD_LAYER_NAMES         (1 << 0)    ///< Use English Standard layer names
 #define CTL_OMIT_NETS               (1 << 1)    ///< Omit pads net names (useless in library)
@@ -242,6 +244,8 @@ private:
 
     void format( EDGE_MODULE* aModuleDrawing, int aNestLevel = 0 ) const;
 
+    void format( GROUP* aGroup, int aNestLevel = 0 ) const;
+
     void format( DRAWSEGMENT* aSegment, int aNestLevel = 0 ) const;
 
     void format( PCB_TARGET* aTarget, int aNestLevel = 0 ) const;

pcbnew/pcb_base_frame.cpp

@@ -187,6 +187,13 @@ void PCB_BASE_FRAME::FocusOnItem( BOARD_ITEM* aItem )
                                                                  child->ClearBrightened();
                                                              });
         }
+        else if( lastItem->Type() == PCB_GROUP_T )
+        {
+            static_cast<GROUP*>( lastItem )->RunOnChildren( [&] ( BOARD_ITEM* child )
+            {
+                child->ClearBrightened();
+            });
+        }
 
         GetCanvas()->GetView()->Update( lastItem );
         lastBrightenedItemID = niluuid;
@@ -204,6 +211,13 @@ void PCB_BASE_FRAME::FocusOnItem( BOARD_ITEM* aItem )
                                                               child->SetBrightened();
                                                           });
         }
+        else if( aItem->Type() == PCB_GROUP_T )
+        {
+            static_cast<GROUP*>( aItem )->RunOnChildren( [&] ( BOARD_ITEM* child )
+            {
+                child->SetBrightened();
+            });
+        }
 
         GetCanvas()->GetView()->Update( aItem );
         lastBrightenedItemID = aItem->m_Uuid;

pcbnew/pcb_painter.cpp

@@ -25,6 +25,7 @@
 
 #include <class_board.h>
 #include <class_track.h>
+#include <class_group.h>
 #include <class_module.h>
 #include <class_pad.h>
 #include <class_drawsegment.h>
@@ -412,6 +413,10 @@ bool PCB_PAINTER::Draw( const VIEW_ITEM* aItem, int aLayer )
         draw( static_cast<const MODULE*>( item ), aLayer );
         break;
 
+    case PCB_GROUP_T:
+        draw( static_cast<const GROUP*>( item ), aLayer );
+        break;
+
     case PCB_ZONE_AREA_T:
         draw( static_cast<const ZONE_CONTAINER*>( item ), aLayer );
         break;
@@ -1151,6 +1156,27 @@ void PCB_PAINTER::draw( const MODULE* aModule, int aLayer )
 }
 
 
+void PCB_PAINTER::draw( const GROUP* aGroup, int aLayer )
+{
+    if( aLayer == LAYER_ANCHOR )
+    {
+        const COLOR4D color = m_pcbSettings.GetColor( aGroup, LAYER_ANCHOR );
+
+        EDA_RECT bbox = aGroup->GetBoundingBox();
+        m_gal->SetStrokeColor( color );
+        m_gal->SetLineWidth( m_pcbSettings.m_outlineWidth * 2.0f );
+        wxPoint pos = bbox.GetPosition();
+
+        m_gal->DrawLine( pos, pos + wxPoint( bbox.GetWidth(), 0 ) );
+        m_gal->DrawLine( pos + wxPoint( bbox.GetWidth(), 0 ),
+                         pos + wxPoint( bbox.GetWidth(), bbox.GetHeight() ) );
+        m_gal->DrawLine( pos + wxPoint( bbox.GetWidth(), bbox.GetHeight() ),
+                         pos + wxPoint( 0, bbox.GetHeight() ) );
+        m_gal->DrawLine( pos + wxPoint( 0, bbox.GetHeight() ), pos );
+    }
+}
+
+
 void PCB_PAINTER::draw( const ZONE_CONTAINER* aZone, int aLayer )
 {
     PCB_LAYER_ID layer = static_cast<PCB_LAYER_ID>( aLayer );

pcbnew/pcb_painter.h

@@ -42,6 +42,7 @@ class VIA;
 class TRACK;
 class D_PAD;
 class DRAWSEGMENT;
+class GROUP;
 class MODULE;
 class ZONE_CONTAINER;
 class TEXTE_PCB;
@@ -303,6 +304,7 @@ protected:
     void draw( const TEXTE_PCB* aText, int aLayer );
     void draw( const TEXTE_MODULE* aText, int aLayer );
     void draw( const MODULE* aModule, int aLayer );
+    void draw( const GROUP* aGroup, int aLayer );
     void draw( const ZONE_CONTAINER* aZone, int aLayer );
     void draw( const DIMENSION* aDimension, int aLayer );
     void draw( const PCB_TARGET* aTarget );

pcbnew/pcb_parser.cpp

@@ -38,6 +38,7 @@
 #include <class_dimension.h>
 #include <class_drawsegment.h>
 #include <class_edge_mod.h>
+#include <class_group.h>
 #include <class_pcb_target.h>
 #include <class_module.h>
 #include <netclass.h>
@@ -62,6 +63,8 @@ void PCB_PARSER::init()
     m_requiredVersion = 0;
     m_layerIndices.clear();
     m_layerMasks.clear();
+    m_groupInfos.clear();
+    m_resetKIIDMap.clear();
 
     // Add untranslated default (i.e. English) layernames.
     // Some may be overridden later if parsing a board rather than a footprint.
@@ -609,6 +612,10 @@ BOARD* PCB_PARSER::parseBOARD_unchecked()
             m_board->Add( parseARC(), ADD_MODE::APPEND );
             break;
 
+        case T_group:
+            parseGROUP();
+            break;
+
         case T_via:
             m_board->Add( parseVIA(), ADD_MODE::APPEND );
             break;
@@ -707,6 +714,81 @@ BOARD* PCB_PARSER::parseBOARD_unchecked()
         m_undefinedLayers.clear();
     }
 
+    // Now that we've parsed the other Uuids in the file we can resolve
+    // the uuids referrred to in the group declarations we saw.
+    //
+    // First add all group objects so subsequent GetItem() calls for nested
+    // groups work.
+
+    for( size_t idx = 0; idx < m_groupInfos.size(); idx++ )
+    {
+        auto&  aGrp  = m_groupInfos[idx];
+        GROUP* group = new GROUP( m_board );
+        group->SetName( aGrp.name );
+        const_cast<KIID&>( group->m_Uuid ) = aGrp.uuid;
+        m_board->Add( group );
+    }
+
+    wxString error;
+    for( size_t idx = 0; idx < m_groupInfos.size(); idx++ )
+    {
+        auto&  aGrp  = m_groupInfos[idx];
+        BOARD_ITEM* bItem = m_board->GetItem( aGrp.uuid );
+
+        if( bItem == nullptr || bItem->Type() != PCB_GROUP_T )
+        {
+            error = wxString::Format( _( "Group %s not found in board" ),
+                                      aGrp.uuid.AsString() );
+            continue;
+        }
+
+        GROUP* group = static_cast<GROUP*>( bItem );
+
+        for( const auto& aUuid : aGrp.memberUuids )
+        {
+            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
+            {
+                group->AddItem( item );
+            }
+        }
+    }
+
+    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 ),
+               _( "File data error" ), wxOK | wxCANCEL | wxICON_ERROR );
+        dlg.SetOKLabel( _( "Attempt repair" ) );
+
+        if( dlg.ShowModal() == wxID_CANCEL )
+            THROW_IO_ERROR( _( "File read cancelled" ) );
+
+        m_board->GroupsSanityCheck( true );
+    }
+
     return m_board;
 }
 
@@ -2165,7 +2247,7 @@ DRAWSEGMENT* PCB_PARSER::parseDRAWSEGMENT( bool aAllowCirclesZeroWidth )
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( segment->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( segment->m_Uuid ) = CurStrToKIID();
             break;
 
         case T_status:
@@ -2245,7 +2327,7 @@ TEXTE_PCB* PCB_PARSER::parseTEXTE_PCB()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( text->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( text->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -2297,7 +2379,7 @@ DIMENSION* PCB_PARSER::parseDIMENSION()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( dimension->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( dimension->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -2518,7 +2600,7 @@ MODULE* PCB_PARSER::parseMODULE_unchecked( wxArrayString* aInitialComments )
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( module->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( module->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -2822,7 +2904,7 @@ TEXTE_MODULE* PCB_PARSER::parseTEXTE_MODULE()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( text->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( text->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -3012,7 +3094,7 @@ EDGE_MODULE* PCB_PARSER::parseEDGE_MODULE()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( segment->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( segment->m_Uuid ) = CurStrToKIID();
             break;
 
         case T_status:
@@ -3490,7 +3572,7 @@ D_PAD* PCB_PARSER::parseD_PAD( MODULE* aParent )
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( pad->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( pad->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -3575,6 +3657,66 @@ bool PCB_PARSER::parseD_PAD_option( D_PAD* aPad )
 }
 
 
+// Example of group format:
+//     (group <(name “groupName”)> (id 12345679)
+//         (members id_1 id_2 … id_last )
+//     )
+void PCB_PARSER::parseGROUP()
+{
+    wxCHECK_RET( CurTok() == T_group,
+            wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + wxT( " as GROUP." ) );
+
+    wxPoint pt;
+    T       token;
+
+    m_groupInfos.push_back( GroupInfo() );
+    GroupInfo& groupInfo = m_groupInfos.back();
+
+    token = NextTok();
+
+    if( token != T_LEFT )
+    {
+        // Optional group name present.
+
+        if( !IsSymbol( token ) )
+            Expecting( DSN_SYMBOL );
+
+        groupInfo.name = FromUTF8();
+    }
+
+    NeedLEFT();
+    token = NextTok();
+
+    if( token != T_id )
+    {
+        Expecting( T_id );
+    }
+
+    NextTok();
+    groupInfo.uuid = CurStrToKIID();
+    NeedRIGHT();
+
+    NeedLEFT();
+    token = NextTok();
+
+    if( token != T_members )
+    {
+        Expecting( T_members );
+    }
+
+    while( ( token = NextTok() ) != T_RIGHT )
+    {
+        // This token is the Uuid of the item in the group.
+        // Since groups are serialized at the end of the file, the
+        // Uuid should already have been seen and exist in the board.
+        KIID        uuid( CurStr() );
+        groupInfo.memberUuids.push_back( uuid );
+    }
+
+    NeedRIGHT();
+}
+
+
 ARC* PCB_PARSER::parseARC()
 {
     wxCHECK_MSG( CurTok() == T_arc, NULL,
@@ -3629,7 +3771,7 @@ ARC* PCB_PARSER::parseARC()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( arc->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( arc->m_Uuid ) = CurStrToKIID();
             break;
 
         case T_status:
@@ -3696,7 +3838,7 @@ TRACK* PCB_PARSER::parseTRACK()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( track->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( track->m_Uuid ) = CurStrToKIID();
             break;
 
         case T_status:
@@ -3790,7 +3932,7 @@ VIA* PCB_PARSER::parseVIA()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( via->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( via->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -3879,7 +4021,7 @@ ZONE_CONTAINER* PCB_PARSER::parseZONE_CONTAINER( BOARD_ITEM_CONTAINER* aParent )
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( zone->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( zone->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -4420,7 +4562,7 @@ PCB_TARGET* PCB_PARSER::parsePCB_TARGET()
 
         case T_tstamp:
             NextTok();
-            const_cast<KIID&>( target->m_Uuid ) = m_resetKIIDs ? KIID() : KIID( CurStr() );
+            const_cast<KIID&>( target->m_Uuid ) = CurStrToKIID();
             NeedRIGHT();
             break;
 
@@ -4431,3 +4573,18 @@ PCB_TARGET* PCB_PARSER::parsePCB_TARGET()
 
     return target.release();
 }
+
+
+KIID PCB_PARSER::CurStrToKIID() {
+    KIID aid;
+    if( m_resetKIIDs )
+    {
+        aid = KIID();
+        m_resetKIIDMap.insert( std::make_pair( CurStr(), aid ) );
+    }
+    else
+    {
+        aid = KIID( CurStr() );
+    }
+    return aid;
+}

pcbnew/pcb_parser.h

@@ -53,6 +53,7 @@ class TEXTE_MODULE;
 class TEXTE_PCB;
 class TRACK;
 class MODULE;
+class GROUP;
 class PCB_TARGET;
 class VIA;
 class ZONE_CONTAINER;
@@ -70,6 +71,7 @@ class PCB_PARSER : public PCB_LEXER
 {
     typedef std::unordered_map< std::string, PCB_LAYER_ID > LAYER_ID_MAP;
     typedef std::unordered_map< std::string, LSET >         LSET_MAP;
+    typedef std::unordered_map< wxString, KIID >            KIID_MAP;
 
     BOARD*              m_board;
     LAYER_ID_MAP        m_layerIndices;     ///< map layer name to it's index
@@ -79,9 +81,23 @@ class PCB_PARSER : public PCB_LEXER
     bool                m_tooRecent;        ///< true if version parses as later than supported
     int                 m_requiredVersion;  ///< set to the KiCad format version this board requires
     bool                m_resetKIIDs;       ///< reading into an existing board; reset UUIDs
+    KIID_MAP            m_resetKIIDMap;     ///< if resetting UUIDs, record new ones to update groups with
 
     bool                m_showLegacyZoneWarning;
 
+    // Group membership info refers to other Uuids in the file.
+    // We don't want to rely on group declarations being last in the file, so
+    // we store info about the group declarations here during parsing and then resolve
+    // them into BOARD_ITEM* after we've parsed the rest of the file.
+    typedef struct
+    {
+        wxString          name;
+        KIID              uuid;
+        std::vector<KIID> memberUuids;
+    } GroupInfo;
+
+    std::vector<GroupInfo> m_groupInfos;
+
     ///> Converts net code using the mapping table if available,
     ///> otherwise returns unchanged net code if < 0 or if is is out of range
     inline int getNetCode( int aNetCode )
@@ -165,6 +181,7 @@ class PCB_PARSER : public PCB_LEXER
     PCB_TARGET*     parsePCB_TARGET();
     MARKER_PCB*     parseMARKER( BOARD_ITEM_CONTAINER* aParent );
     BOARD*          parseBOARD();
+    void            parseGROUP();
 
     /**
      * Function parseBOARD_unchecked
@@ -316,6 +333,11 @@ class PCB_PARSER : public PCB_LEXER
      */
     int parseVersion();
 
+    /*
+     * @return if m_resetKIIDs, returns new KIID(), otehrwise returns CurStr() as KIID.
+     */
+    KIID CurStrToKIID();
+
 public:
 
     PCB_PARSER( LINE_READER* aReader = NULL ) :

pcbnew/pcb_view.cpp

@@ -31,6 +31,7 @@ using namespace std::placeholders;
 #include <pcb_display_options.h>
 #include <pcb_painter.h>
 
+#include <class_group.h>
 #include <class_module.h>
 
 namespace KIGFX {

pcbnew/swig/board_item.i

@@ -50,6 +50,7 @@
 class TEXTE_PCB;
 class DIMENSION;
 class MODULE;
+class GROUP;
 class TEXTE_MODULE;
 class DRAWSEGMENT;
 class MARKER_PCB;
@@ -70,6 +71,7 @@ extern "C" {
 static TEXTE_PCB*        Cast_to_TEXTE_PCB( BOARD_ITEM* );
 static DIMENSION*        Cast_to_DIMENSION( BOARD_ITEM* );
 static MODULE*           Cast_to_MODULE( BOARD_ITEM* );
+static GROUP*            Cast_to_GROUP( BOARD_ITEM* );
 static TEXTE_MODULE*     Cast_to_TEXTE_MODULE( BOARD_ITEM* );
 static DRAWSEGMENT*      Cast_to_DRAWSEGMENT( BOARD_ITEM* );
 static MARKER_PCB*       Cast_to_MARKER_PCB( BOARD_ITEM* );
@@ -90,6 +92,7 @@ static PCB_TARGET*       Cast_to_PCB_TARGET( BOARD_ITEM* );
 static TEXTE_PCB*        Cast_to_TEXTE_PCB( BOARD_ITEM* );
 static DIMENSION*        Cast_to_DIMENSION( BOARD_ITEM* );
 static MODULE*           Cast_to_MODULE( BOARD_ITEM* );
+static GROUP*            Cast_to_GROUP( BOARD_ITEM* );
 static TEXTE_MODULE*     Cast_to_TEXTE_MODULE( BOARD_ITEM* );
 static DRAWSEGMENT*      Cast_to_DRAWSEGMENT( BOARD_ITEM* );
 static MARKER_PCB*       Cast_to_MARKER_PCB( BOARD_ITEM* );
@@ -122,6 +125,8 @@ static PCB_TARGET*       Cast_to_PCB_TARGET( BOARD_ITEM* );
             return Cast_to_EDGE_MODULE(self)
         elif ct=="MODULE":
             return Cast_to_MODULE(self)
+        elif ct=="GROUP":
+            return Cast_to_GROUP(self)
         elif ct=="PAD":
             return Cast_to_D_PAD(self)
         elif ct=="MTEXT":
@@ -165,6 +170,7 @@ static PCB_TARGET*       Cast_to_PCB_TARGET( BOARD_ITEM* );
 static TEXTE_PCB*        Cast_to_TEXTE_PCB( BOARD_ITEM* self )       {  return dynamic_cast<TEXTE_PCB*>(self);       }
 static DIMENSION*        Cast_to_DIMENSION( BOARD_ITEM* self )       {  return dynamic_cast<DIMENSION*>(self);       }
 static MODULE*           Cast_to_MODULE( BOARD_ITEM* self )          {  return dynamic_cast<MODULE*>(self);          }
+static GROUP*            Cast_to_GROUP( BOARD_ITEM* self )           {  return dynamic_cast<GROUP*>(self);           }
 static TEXTE_MODULE*     Cast_to_TEXTE_MODULE( BOARD_ITEM* self )    {  return dynamic_cast<TEXTE_MODULE*>(self);    }
 static DRAWSEGMENT*      Cast_to_DRAWSEGMENT( BOARD_ITEM* self )     {  return dynamic_cast<DRAWSEGMENT*>(self);     }
 static MARKER_PCB*       Cast_to_MARKER_PCB( BOARD_ITEM* self )      {  return dynamic_cast<MARKER_PCB*>(self);      }

pcbnew/tools/edit_tool.cpp

@@ -58,7 +58,7 @@ using namespace std::placeholders;
 #include <zone_filler.h>
 
 
-void EditToolSelectionFilter( GENERAL_COLLECTOR& aCollector, int aFlags )
+void EditToolSelectionFilter( GENERAL_COLLECTOR& aCollector, int aFlags, SELECTION_TOOL* selectionTool )
 {
     // Iterate from the back so we don't have to worry about removals.
     for( int i = aCollector.GetCount() - 1; i >= 0; --i )
@@ -110,6 +110,7 @@ void EditToolSelectionFilter( GENERAL_COLLECTOR& aCollector, int aFlags )
             aCollector.Remove( item );
         }
     }
+    selectionTool->FilterCollectorForGroups( aCollector );
 }
 
 
@@ -332,9 +333,9 @@ int EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, bool aPickReference )
     // Be sure that there is at least one item that we can modify. If nothing was selected before,
     // try looking for the stuff under mouse cursor (i.e. Kicad old-style hover selection)
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+              []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool );
             } );
 
     if( m_dragging || selection.Empty() )
@@ -346,9 +347,9 @@ int EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, bool aPickReference )
     // Now filter out locked pads.  We cannot do this in the first RequestSelection() as we need
     // the item_layers when a pad is the selection front (ie: will become curr_tiem).
     selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS, sTool );
             } );
 
     if( selection.Empty() )
@@ -428,6 +429,9 @@ int EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, bool aPickReference )
                 for( EDA_ITEM* item : sel_items )
                 {
                     // Don't double move footprint pads, fields, etc.
+                    //
+                    // For PCB_GROUP_T, we make sure the selection includes only the top level
+                    // group and not its descendants.
                     if( !item->GetParent() || !item->GetParent()->IsSelected() )
                         static_cast<BOARD_ITEM*>( item )->Move( movement );
                 }
@@ -463,10 +467,20 @@ int EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, bool aPickReference )
                     for( EDA_ITEM* item : selection )
                     {
                         // Don't double move footprint pads, fields, etc.
+                        //
+                        // For PCB_GROUP_T, the parent is the board.
                         if( item->GetParent() && item->GetParent()->IsSelected() )
                             continue;
 
                         m_commit->Modify( item );
+
+                        // If moving a group, record position of all the descendants for undo
+                        if( item->Type() == PCB_GROUP_T )
+                        {
+                            static_cast<GROUP*>( item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                                              m_commit->Modify( bItem );
+                                                                          });
+                        }
                     }
                 }
 
@@ -620,8 +634,8 @@ int EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, bool aPickReference )
 int EDIT_TOOL::ChangeTrackWidth( const TOOL_EVENT& aEvent )
 {
     const auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector ) {
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) {
-                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool );
             } );
 
     for( EDA_ITEM* item : selection )
@@ -676,9 +690,9 @@ int EDIT_TOOL::Properties( const TOOL_EVENT& aEvent )
 {
     PCB_BASE_EDIT_FRAME*    editFrame = getEditFrame<PCB_BASE_EDIT_FRAME>();
     const PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool );
             } );
 
     // Tracks & vias are treated in a special way:
@@ -730,9 +744,9 @@ int EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent )
     PCB_BASE_EDIT_FRAME* editFrame = getEditFrame<PCB_BASE_EDIT_FRAME>();
 
     auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             },
             nullptr, ! m_dragging );
 
@@ -750,8 +764,18 @@ int EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent )
     for( auto item : selection )
     {
         if( !item->IsNew() && !EditingModules() )
+        {
             m_commit->Modify( item );
 
+            // If rotating a group, record position of all the descendants for undo
+            if( item->Type() == PCB_GROUP_T )
+            {
+                static_cast<GROUP*>( item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                                  m_commit->Modify( bItem );
+                                                              });
+            }
+        }
+
         static_cast<BOARD_ITEM*>( item )->Rotate( refPt, rotateAngle );
     }
 
@@ -819,9 +843,9 @@ int EDIT_TOOL::Mirror( const TOOL_EVENT& aEvent )
     }
 
     auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             },
             nullptr, !m_dragging );
 
@@ -887,6 +911,7 @@ int EDIT_TOOL::Mirror( const TOOL_EVENT& aEvent )
 
         default:
             // it's likely the commit object is wrong if you get here
+            // Unsure if PCB_GROUP_T needs special attention here.
             assert( false );
             break;
         }
@@ -916,9 +941,9 @@ int EDIT_TOOL::Flip( const TOOL_EVENT& aEvent )
     }
 
     auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             },
             nullptr, !m_dragging );
 
@@ -946,6 +971,13 @@ int EDIT_TOOL::Flip( const TOOL_EVENT& aEvent )
         if( !item->IsNew() && !EditingModules() )
             m_commit->Modify( item );
 
+        if( item->Type() == PCB_GROUP_T )
+        {
+            static_cast<GROUP*>( item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                              m_commit->Modify( bItem );
+                                                          });
+        }
+
         static_cast<BOARD_ITEM*>( item )->Flip( modPoint, leftRight );
     }
 
@@ -994,9 +1026,9 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
     else
     {
         selectionCopy = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             } );
     }
 
@@ -1020,9 +1052,9 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
     {
         // Second RequestSelection removes locked items but keeps a copy of their pointers
         selectionCopy = m_selectionTool->RequestSelection(
-                []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+                []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
                 {
-                    EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED );
+                    EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED, sTool );
                 },
                 &lockedItems );
     }
@@ -1031,11 +1063,14 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
     // As we are about to remove items, they have to be removed from the selection first
     m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true );
 
+    PCBNEW_SELECTION removed;
+
     for( EDA_ITEM* item : selectionCopy )
     {
         if( m_editModules )
         {
             m_commit->Remove( item );
+            removed.Add( item );
             continue;
         }
 
@@ -1116,20 +1151,55 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent )
 
                 // Remove the entire zone otherwise
                 m_commit->Remove( item );
+                removed.Add( item );
+            }
+            break;
+
+        case PCB_GROUP_T:
+            {
+                m_commit->Remove( item );
+                removed.Add( item );
+
+                static_cast<GROUP*>( item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                                  m_commit->Remove( bItem );
+                                                              });
             }
             break;
 
         default:
             m_commit->Remove( item );
+            removed.Add( item );
             break;
         }
     }
 
+    // Figure out status of a group containing items to be removed.  if entered
+    // group is not set in the selection tool, then any groups to be removed are
+    // removed in their entirety and so no empty group could remain.  If entered
+    // group is set, then we could be removing all items of the entered group,
+    // in which case we need to remove the group itself.
+    GROUP* enteredGroup = m_selectionTool->GetEnteredGroup();
+
+    if( enteredGroup != nullptr )
+    {
+        board()->GroupRemoveItems( removed, m_commit.get() );
+
+        if( m_commit->HasRemoveEntry( enteredGroup ) )
+            m_selectionTool->exitGroup();
+    }
+
     if( isCut )
         m_commit->Push( _( "Cut" ) );
     else
         m_commit->Push( _( "Delete" ) );
 
+    if( enteredGroup != nullptr )
+    {
+        wxString check = board()->GroupsSanityCheck();
+        wxCHECK_MSG( check == wxEmptyString, 0,
+                     _( "Remove of items in entered group resulted in inconsistent state: " )+ check );
+    }
+
     if( !m_lockedSelected && !lockedItems.empty() )
     {
         ///> Popup nag for deleting locked items
@@ -1168,10 +1238,10 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
     }
 
     const auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
                 EditToolSelectionFilter( aCollector,
-                                      EXCLUDE_LOCKED | EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                                      EXCLUDE_LOCKED | EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             } );
 
     if( selection.Empty() )
@@ -1205,8 +1275,17 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent )
             BOARD_ITEM* item = static_cast<BOARD_ITEM*>( selItem );
 
             if( !item->IsNew() && !EditingModules() )
+            {
                 m_commit->Modify( item );
 
+                if( item->Type() == PCB_GROUP_T )
+                    {
+                        static_cast<GROUP*>( item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                                          m_commit->Modify( bItem );
+                                                                      });
+                    }
+            }
+            
             item->Move( translation );
 
             switch( rotationAnchor )
@@ -1256,9 +1335,9 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
 
     // Be sure that there is at least one item that we can modify
     const auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             } );
 
     if( selection.Empty() )
@@ -1273,14 +1352,13 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
     std::vector<BOARD_ITEM*> new_items;
     new_items.reserve( selection.Size() );
 
-    BOARD_ITEM* orig_item = nullptr;
-    BOARD_ITEM* dupe_item = nullptr;
 
     // Each selected item is duplicated and pushed to new_items list
     // Old selection is cleared, and new items are then selected.
     for( EDA_ITEM* item : selection )
     {
-        orig_item = static_cast<BOARD_ITEM*>( item );
+        BOARD_ITEM* dupe_item = nullptr;
+        BOARD_ITEM* orig_item = static_cast<BOARD_ITEM*>( item );
 
         if( m_editModules )
         {
@@ -1319,6 +1397,10 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
                 dupe_item = orig_item->Duplicate();
                 break;
 
+            case PCB_GROUP_T:
+                dupe_item = static_cast<GROUP*>( orig_item )->DeepDuplicate();
+                break;
+                
             default:
                 // Silently drop other items (such as footprint texts) from duplication
                 break;
@@ -1327,6 +1409,13 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent )
 
         if( dupe_item )
         {
+            if( dupe_item->Type() == PCB_GROUP_T )
+            {
+                static_cast<GROUP*>( dupe_item )->RunOnDescendants( [&]( BOARD_ITEM* bItem ) {
+                                                                        m_commit->Add( bItem );
+                                                                    });
+            }
+
             // Clear the selection flag here, otherwise the SELECTION_TOOL
             // will not properly select it later on
             dupe_item->ClearSelected();
@@ -1373,9 +1462,9 @@ int EDIT_TOOL::CreateArray( const TOOL_EVENT& aEvent )
     }
 
     const auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             } );
 
     if( selection.Empty() )
@@ -1390,7 +1479,7 @@ int EDIT_TOOL::CreateArray( const TOOL_EVENT& aEvent )
 }
 
 
-void EDIT_TOOL::PadFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector )
+void EDIT_TOOL::PadFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
 {
     for( int i = aCollector.GetCount() - 1; i >= 0; i-- )
     {
@@ -1402,7 +1491,7 @@ void EDIT_TOOL::PadFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector )
 }
 
 
-void EDIT_TOOL::FootprintFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector )
+void EDIT_TOOL::FootprintFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
 {
     for( int i = aCollector.GetCount() - 1; i >= 0; i-- )
     {
@@ -1517,8 +1606,8 @@ int EDIT_TOOL::copyToClipboard( const TOOL_EVENT& aEvent )
     Activate();
 
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector ) {
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) {
-                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool );
             } );
 
     if( selection.Empty() )

pcbnew/tools/edit_tool.h

@@ -55,7 +55,7 @@ namespace KIGFX {
 #define EXCLUDE_TRANSIENTS 0x0004
 #define INCLUDE_PADS_AND_MODULES 0x0008
 
-void EditToolSelectionFilter( GENERAL_COLLECTOR& aCollector, int aFlags );
+void EditToolSelectionFilter( GENERAL_COLLECTOR& aCollector, int aFlags, SELECTION_TOOL* sTool );
 
 /**
  * EDIT_TOOL
@@ -149,13 +149,13 @@ public:
      * Function FootprintFilter()
      * A selection filter which prunes the selection to contain only items of type PCB_MODULE_T
      */
-    static void FootprintFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector );
+    static void FootprintFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool );
 
     /**
      * Function PadFilter()
      * A selection filter which prunes the selection to contain only items of type PCB_PAD_T
      */
-    static void PadFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector );
+    static void PadFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool );
 
     ///> Sets up handlers for various events.
     void setTransitions() override;
@@ -190,6 +190,7 @@ private:
     bool pickReferencePoint( const wxString& aTooltip, const wxString& aSuccessMessage,
                              const wxString& aCanceledMessage, VECTOR2I& aReferencePoint );
 
+
 private:
     SELECTION_TOOL* m_selectionTool;   // Selection tool used for obtaining selected items
     bool            m_dragging;        // Indicates objects are being dragged right now

pcbnew/tools/global_edit_tool.cpp

@@ -201,9 +201,9 @@ int GLOBAL_EDIT_TOOL::RemoveUnusedPads( const TOOL_EVENT& aEvent )
 {
     PCB_EDIT_FRAME* editFrame = getEditFrame<PCB_EDIT_FRAME>();
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
             {
-                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS );
+                EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool );
             } );
     DIALOG_UNUSED_PAD_LAYERS dlg( editFrame, selection, *m_commit );
 

pcbnew/tools/pcb_actions.cpp

@@ -604,6 +604,36 @@ TOOL_ACTION PCB_ACTIONS::unlock( "pcbnew.EditorControl.unlock",
         _( "Unlock" ), "",
         unlocked_xpm );
 
+TOOL_ACTION PCB_ACTIONS::groupCreate( "pcbnew.EditorControl.groupCreate",
+        AS_GLOBAL, 0, "",
+        _( "Group" ), "",
+        locked_xpm );
+
+TOOL_ACTION PCB_ACTIONS::groupMerge( "pcbnew.EditorControl.groupMerge",
+        AS_GLOBAL, 0, "",
+        _( "Merge" ), "",
+        unlocked_xpm );
+
+TOOL_ACTION PCB_ACTIONS::groupUngroup( "pcbnew.EditorControl.groupUngroup",
+        AS_GLOBAL, 0, "",
+        _( "Ungroup" ), "",
+        unlocked_xpm );
+
+TOOL_ACTION PCB_ACTIONS::groupRemoveItems( "pcbnew.EditorControl.groupRemoveItems",
+        AS_GLOBAL, 0, "",
+        _( "Remove Items" ), "",
+        unlocked_xpm );
+
+TOOL_ACTION PCB_ACTIONS::groupFlatten( "pcbnew.EditorControl.groupFlatten",
+        AS_GLOBAL, 0, "",
+        _( "Flatten" ), "",
+        unlocked_xpm );
+
+TOOL_ACTION PCB_ACTIONS::groupEnter( "pcbnew.EditorControl.groupEnter",
+        AS_GLOBAL, 0, "",
+        _( "Enter" ), "",
+        unlocked_xpm );
+
 TOOL_ACTION PCB_ACTIONS::appendBoard( "pcbnew.EditorControl.appendBoard",
         AS_GLOBAL, 0, "",
         _( "Append Board..." ), "",

pcbnew/tools/pcb_actions.h

@@ -397,6 +397,14 @@ public:
     static TOOL_ACTION lock;
     static TOOL_ACTION unlock;
 
+    // Grouping
+    static TOOL_ACTION groupCreate;
+    static TOOL_ACTION groupMerge;
+    static TOOL_ACTION groupUngroup;
+    static TOOL_ACTION groupRemoveItems;
+    static TOOL_ACTION groupFlatten;
+    static TOOL_ACTION groupEnter;
+
     // Miscellaneous
     static TOOL_ACTION selectionTool;
     static TOOL_ACTION pickerTool;

pcbnew/tools/pcb_editor_control.cpp

@@ -31,6 +31,7 @@
 #include <bitmaps.h>
 #include <board_commit.h>
 #include <class_board.h>
+#include <class_group.h>
 #include <class_module.h>
 #include <class_pcb_target.h>
 #include <class_track.h>
@@ -146,6 +147,50 @@ public:
 };
 
 
+class GROUP_CONTEXT_MENU : public ACTION_MENU
+{
+public:
+    GROUP_CONTEXT_MENU( ) : ACTION_MENU( true )
+    {
+        SetIcon( locked_xpm ); // fixme
+        SetTitle( _( "Grouping" ) );
+
+        Add( PCB_ACTIONS::groupCreate );
+        Add( PCB_ACTIONS::groupUngroup );
+        Add( PCB_ACTIONS::groupMerge );
+        Add( PCB_ACTIONS::groupRemoveItems );
+        Add( PCB_ACTIONS::groupFlatten );
+        Add( PCB_ACTIONS::groupEnter );
+    }
+
+    ACTION_MENU* create() const override
+    {
+        return new GROUP_CONTEXT_MENU();
+    }
+
+private:
+    void update() override
+    {
+        SELECTION_TOOL* selTool = getToolManager()->GetTool<SELECTION_TOOL>();
+        BOARD* board = selTool->GetBoard();
+
+        const auto& selection = selTool->GetSelection();
+
+        wxString check = board->GroupsSanityCheck();
+        wxCHECK_RET( check == wxEmptyString, _( "Group is in inconsistent state: " ) + check );
+
+        BOARD::GroupLegalOpsField legalOps = board->GroupLegalOps( selection );
+
+        Enable( getMenuId( PCB_ACTIONS::groupCreate ), legalOps.create );
+        Enable( getMenuId( PCB_ACTIONS::groupMerge ), legalOps.merge );
+        Enable( getMenuId( PCB_ACTIONS::groupUngroup ), legalOps.ungroup );
+        Enable( getMenuId( PCB_ACTIONS::groupRemoveItems ), legalOps.removeItems );
+        Enable( getMenuId( PCB_ACTIONS::groupFlatten ), legalOps.flatten );
+        Enable( getMenuId( PCB_ACTIONS::groupEnter ), legalOps.enter );
+    }
+};
+
+
 PCB_EDITOR_CONTROL::PCB_EDITOR_CONTROL() :
     PCB_TOOL_BASE( "pcbnew.EditorControl" ),
     m_frame( nullptr )
@@ -206,6 +251,9 @@ bool PCB_EDITOR_CONTROL::Init()
     auto lockMenu = std::make_shared<LOCK_CONTEXT_MENU>();
     lockMenu->SetTool( this );
 
+    auto groupMenu = std::make_shared<GROUP_CONTEXT_MENU>();
+    groupMenu->SetTool( this );
+
     // Add the PCB control menus to relevant other tools
 
     SELECTION_TOOL* selTool = m_toolMgr->GetTool<SELECTION_TOOL>();
@@ -221,9 +269,11 @@ bool PCB_EDITOR_CONTROL::Init()
 
         toolMenu.AddSubMenu( zoneMenu );
         toolMenu.AddSubMenu( lockMenu );
+        toolMenu.AddSubMenu( groupMenu );
 
         menu.AddMenu( zoneMenu.get(), SELECTION_CONDITIONS::OnlyType( PCB_ZONE_AREA_T ), 200 );
         menu.AddMenu( lockMenu.get(), SELECTION_CONDITIONS::OnlyTypes( GENERAL_COLLECTOR::LockableItems ), 200 );
+        menu.AddMenu( groupMenu.get(), SELECTION_CONDITIONS::NotEmpty, 200 );
     }
 
     DRAWING_TOOL* drawingTool = m_toolMgr->GetTool<DRAWING_TOOL>();
@@ -970,6 +1020,268 @@ int PCB_EDITOR_CONTROL::modifyLockSelected( MODIFY_MODE aMode )
 }
 
 
+int PCB_EDITOR_CONTROL::GroupSelected( const TOOL_EVENT& aEvent )
+{
+    SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
+    const PCBNEW_SELECTION& selection = selTool->GetSelection();
+    BOARD*                  board     = getModel<BOARD>();
+    BOARD_COMMIT            commit( m_frame );
+
+    if( selection.Empty() )
+        m_toolMgr->RunAction( PCB_ACTIONS::selectionCursor, true );
+    // why don't we have to update the selection after selectionCursor action?
+
+    GROUP* group = new GROUP( board );
+
+    for( EDA_ITEM* item : selection )
+    {
+        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 );
+    commit.Push( _( "GroupCreate" ) );
+    wxString check = board->GroupsSanityCheck();
+    wxCHECK_MSG( check == wxEmptyString, 0, _( "Group create resulted in inconsistent state: " ) + check );
+
+    selTool->ClearSelection();
+    selTool->select( group );
+
+    // Should I call PostEvent and onModify() ?
+    m_toolMgr->PostEvent( EVENTS::SelectedItemsModified );
+    m_frame->OnModify();
+
+    return 0;
+}
+
+
+int PCB_EDITOR_CONTROL::GroupMergeSelected( const TOOL_EVENT& aEvent )
+{
+    SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
+    const PCBNEW_SELECTION& selection = selTool->GetSelection();
+    BOARD*                  board     = getModel<BOARD>();
+    BOARD_COMMIT            commit( m_frame );
+
+    if( selection.Empty() )
+        m_toolMgr->RunAction( PCB_ACTIONS::selectionCursor, true );
+    // why don't we have to update the selection after selectionCursor action?
+
+    GROUP* firstGroup = NULL;
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
+
+        if( firstGroup == NULL && board_item->Type() == PCB_GROUP_T )
+        {
+            firstGroup = static_cast<GROUP*>( board_item );
+            break;
+        }
+    }
+    // The group submenu update() call only enabled merge if there was a group
+    // in the selection.
+    wxCHECK_MSG( firstGroup != NULL, 0, _( "Group not found in selection though selection was checked" ) );
+
+    commit.Modify( firstGroup );
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
+
+        if( board_item != firstGroup )
+        {
+            // 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" ) );
+    wxString check = board->GroupsSanityCheck();
+    wxCHECK_MSG( check == wxEmptyString, 0, _( "Group merge resulted in inconsistent state: " ) + check );
+
+    selTool->ClearSelection();
+    selTool->select( firstGroup );
+
+    // Should I call PostEvent and onModify() ?
+    m_toolMgr->PostEvent( EVENTS::SelectedItemsModified );
+    m_frame->OnModify();
+
+    return 0;
+}
+
+
+int PCB_EDITOR_CONTROL::UngroupSelected( const TOOL_EVENT& aEvent )
+{
+    SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
+    const PCBNEW_SELECTION& selection = selTool->GetSelection();
+    BOARD*                  board     = getModel<BOARD>();
+    BOARD_COMMIT            commit( m_frame );
+    std::unordered_set<BOARD_ITEM*> ungroupedItems;
+
+    if( selection.Empty() )
+        m_toolMgr->RunAction( PCB_ACTIONS::selectionCursor, true );
+    // why don't we have to update the selection after selectionCursor action?
+
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
+
+        wxCHECK_MSG( board_item->Type() == PCB_GROUP_T, 0,
+                     _( "Selection for ungroup should only have groups in it - was checked." ) );
+
+        commit.Remove( board_item );
+
+        for( BOARD_ITEM* bItem : static_cast<GROUP*>( board_item )->GetItems() )
+        {
+            ungroupedItems.insert( bItem );
+        }
+    }
+
+    commit.Push( _( "GroupUngroup" ) );
+    wxString check = board->GroupsSanityCheck();
+    wxCHECK_MSG( check == wxEmptyString, 0, _( "Group merge resulted in inconsistent state: " ) + check );
+
+    selTool->ClearSelection();
+    for( BOARD_ITEM* item : ungroupedItems )
+    {
+        // commit.Remove() on the group recursively removed children from the view.
+        // Add them back to the view
+        //getView()->Add( item );
+
+        selTool->select( item );
+    }
+
+    // Should I call PostEvent and onModify() ?
+    m_toolMgr->PostEvent( EVENTS::SelectedItemsModified );
+    m_frame->OnModify();
+
+    return 0;
+}
+
+
+int PCB_EDITOR_CONTROL::GroupRemoveItemsSelected( const TOOL_EVENT& aEvent )
+{
+    SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
+    const PCBNEW_SELECTION& selection = selTool->GetSelection();
+    BOARD*                  board     = getModel<BOARD>();
+    BOARD_COMMIT            commit( m_frame );
+
+    if( selection.Empty() )
+        m_toolMgr->RunAction( PCB_ACTIONS::selectionCursor, true );
+    // why don't we have to update the selection after selectionCursor action?
+
+    board->GroupRemoveItems( selection, &commit );
+
+    commit.Push( _( "GroupRemoveItems" ) );
+    wxString check = board->GroupsSanityCheck();
+    wxCHECK_MSG( check == wxEmptyString, 0, _( "Group removeItems resulted in inconsistent state: " ) + check );
+
+    // Should I call PostEvent and onModify() ?
+    m_toolMgr->PostEvent( EVENTS::SelectedItemsModified );
+    m_frame->OnModify();
+
+    return 0;
+}
+
+
+int PCB_EDITOR_CONTROL::GroupFlattenSelected( const TOOL_EVENT& aEvent )
+{
+    SELECTION_TOOL*         selTool   = m_toolMgr->GetTool<SELECTION_TOOL>();
+    const PCBNEW_SELECTION& selection = selTool->GetSelection();
+    BOARD*                  board     = getModel<BOARD>();
+    BOARD_COMMIT            commit( m_frame );
+    const PCBNEW_SELECTION  origGroups = selTool->GetSelection();
+    // These items were moved up to the top-level group that need to be readded to
+    // the view.  That's becuase commit.Remove(group) recursively removed them from
+    // the view.
+    //std::unordered_set<BOARD_ITEM*> movedItems;
+
+    if( selection.Empty() )
+        m_toolMgr->RunAction( PCB_ACTIONS::selectionCursor, true );
+    // why don't we have to update the selection after selectionCursor action?
+
+    for( EDA_ITEM* item : selection )
+    {
+        BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( item );
+        wxCHECK_MSG( board_item->Type() == PCB_GROUP_T, 0,
+                     _( "Selection for ungroup should only have groups in it - was checked." ) );
+        std::queue<GROUP*> groupsToFlatten;
+        groupsToFlatten.push( static_cast<GROUP*>( board_item ) );
+        GROUP* topGroup = groupsToFlatten.front();
+        commit.Modify( topGroup );
+        std::unordered_set<BOARD_ITEM*> topSubgroupsToRemove;
+
+        while( !groupsToFlatten.empty() )
+        {
+            GROUP* grp = groupsToFlatten.front();
+            groupsToFlatten.pop();
+
+            for( BOARD_ITEM* grpItem : grp->GetItems() )
+            {
+                if( grpItem->Type() == PCB_GROUP_T )
+                {
+                    groupsToFlatten.push( static_cast<GROUP*>( grpItem ) );
+                    commit.Remove( grpItem );
+                    if( grp == topGroup )
+                        topSubgroupsToRemove.insert( grpItem );
+                }
+                else
+                {
+                    if( grp != topGroup )
+                    {
+                        wxCHECK( topGroup->AddItem( grpItem ), 0 );
+                        //movedItems.insert( grpItem );
+                    }
+                }
+            }
+        }
+
+        for( BOARD_ITEM* group : topSubgroupsToRemove )
+        {
+            topGroup->RemoveItem( group );
+        }
+    }
+
+    commit.Push( _( "GroupFlatten" ) );
+    wxString check = board->GroupsSanityCheck();
+    wxCHECK_MSG( check == wxEmptyString, 0, _( "Group flatten resulted in inconsistent state: " ) + check );
+
+    // Removing subgroups deselects the items in them. So reselect everything no that it's flattened.
+    selTool->ClearSelection();
+    for( EDA_ITEM* item : origGroups )
+        selTool->select( static_cast<BOARD_ITEM*>( item ) );
+
+    // Should I call PostEvent and onModify() ?
+    m_toolMgr->PostEvent( EVENTS::SelectedItemsModified );
+    m_frame->OnModify();
+
+    return 0;
+}
+
+
+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 );
+    BOARD_ITEM* board_item = static_cast<BOARD_ITEM*>( selection[0] );
+    wxCHECK( board_item->Type() == PCB_GROUP_T, 0 );
+
+    selTool->EnterGroup();
+
+    return 0;
+}
+
+
 int PCB_EDITOR_CONTROL::PlaceTarget( const TOOL_EVENT& aEvent )
 {
     KIGFX::VIEW* view = getView();
@@ -1336,6 +1648,12 @@ void PCB_EDITOR_CONTROL::setTransitions()
     Go( &PCB_EDITOR_CONTROL::ToggleLockSelected,     PCB_ACTIONS::toggleLock.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::LockSelected,           PCB_ACTIONS::lock.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::UpdatePCBFromSchematic, ACTIONS::updatePcbFromSchematic.MakeEvent() );
     Go( &PCB_EDITOR_CONTROL::UpdateSchematicFromPCB, ACTIONS::updateSchematicFromPcb.MakeEvent() );

pcbnew/tools/pcb_editor_control.h

@@ -110,6 +110,24 @@ public:
     ///> Unlocks selected items.
     int UnlockSelected( const TOOL_EVENT& aEvent );
 
+    ///> Groups selected items.
+    int GroupSelected( const TOOL_EVENT& aEvent );
+
+    ///> Merges selected items.
+    int GroupMergeSelected( const TOOL_EVENT& aEvent );
+
+    ///> Ungroups selected items.
+    int UngroupSelected( const TOOL_EVENT& aEvent );
+
+    ///> Remove selection from group.
+    int GroupRemoveItemsSelected( const TOOL_EVENT& aEvent );
+
+    ///> Collaps subgroups to single group.
+    int GroupFlattenSelected( const TOOL_EVENT& aEvent );
+
+    ///> Restrit seletion to only member of the group.
+    int GroupEnterSelected( 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

@@ -816,6 +816,13 @@ int PCBNEW_CONTROL::placeBoardItems( BOARD* aBoard, bool aAnchorAtOrigin  )
     moveUnflaggedItems( aBoard->Drawings(), items, isNew );
     moveUnflaggedItems( aBoard->Zones(), items, isNew );
 
+    // Subtlety: When selecting a group via the mouse,
+    // SELECTION_TOOL::highlightInternal runs, which does a SetSelected() on all
+    // descendants. In PCBNEW_CONTROL::placeBoardItems, below, we skip that and
+    // mark items non-recursively.  That works because the saving of the
+    // selection created aBoard that has the group and all descendents in it.
+    moveUnflaggedItems( aBoard->Groups(), items, isNew );
+
     return placeBoardItems( items, isNew, aAnchorAtOrigin );
 }
 
@@ -840,16 +847,49 @@ int PCBNEW_CONTROL::placeBoardItems( std::vector<BOARD_ITEM*>& aItems, bool aIsN
                 static_cast<MODULE*>( item )->SetPath( KIID_PATH() );
         }
 
-        item->SetSelected();
-        selection.Add( item );
-
         // Add or just select items for the move/place command
         if( aIsNew )
             editTool->GetCurrentCommit()->Add( item );
         else
             editTool->GetCurrentCommit()->Added( item );
+
+        // Matching the logic of SELECTION_TOOL::select for PCB_GROUP_T, there
+        // is a distinction between which items are SetSelected and which are in
+        // the selection object.  Top-level groups or items not in groups are
+        // added to the selection object (via selection.Add(), below), but all
+        // items have SetSelected called.  This is because much of the selection
+        // management logic (e.g. move) recursively acts on groups in the
+        // selection, so descendents of groups should not be in the selection
+        // object.
+        item->SetSelected();
     }
 
+    // Filter out from selection any items that are in groups that are also in the selection
+    // For PCB_GROUP_T, a selection including the group should not include its descendants.
+    std::unordered_set<GROUP*> groups;
+    for( BOARD_ITEM* item : aItems )
+    {
+        if( item->Type() == PCB_GROUP_T )
+            groups.insert( static_cast<GROUP*>( item ) );
+    }
+    for( BOARD_ITEM* item : aItems )
+    {
+        bool inGroup = false;
+        for( GROUP* grp : groups )
+        {
+            if( grp->GetItems().find( item ) != grp->GetItems().end() )
+            {
+                inGroup = true;
+                break;
+            }
+        }
+        if( !inGroup )
+        {
+            selection.Add( item );
+        }
+    }
+
+
     if( selection.Size() > 0 )
     {
         if( aAnchorAtOrigin )
@@ -892,6 +932,9 @@ int PCBNEW_CONTROL::AppendBoard( PLUGIN& pi, wxString& fileName )
     for( auto module : brd->Modules() )
         module->SetFlags( SKIP_STRUCT );
 
+    for( auto group : brd->Groups() )
+        group->SetFlags( SKIP_STRUCT );
+
     for( auto drawing : brd->Drawings() )
         drawing->SetFlags( SKIP_STRUCT );
 

pcbnew/tools/pcbnew_selection.cpp

@@ -93,6 +93,27 @@ const KIGFX::VIEW_GROUP::ITEMS PCBNEW_SELECTION::updateDrawList() const
 {
     std::vector<VIEW_ITEM*> items;
 
+    std::function<void ( EDA_ITEM* )> addItem;
+    addItem = [&]( EDA_ITEM* item ) {
+                       items.push_back( item );
+
+                       if( item->Type() == PCB_MODULE_T )
+                       {
+                           MODULE* module = static_cast<MODULE*>( item );
+                           module->RunOnChildren( [&] ( BOARD_ITEM* bitem ) { addItem( bitem ); } );
+                       }
+                       else if( item->Type() == PCB_GROUP_T )
+                       {
+                           GROUP* group = static_cast<GROUP*>( item );
+                           group->RunOnChildren( [&] ( BOARD_ITEM* bitem ) { addItem( bitem ); } );
+                       }
+                   };
+
+    for( auto item : m_items )
+    {
+        addItem( item );
+    }
+#if 0
     for( auto item : m_items )
     {
         items.push_back( item );
@@ -102,8 +123,13 @@ const KIGFX::VIEW_GROUP::ITEMS PCBNEW_SELECTION::updateDrawList() const
             MODULE* module = static_cast<MODULE*>( item );
             module->RunOnChildren( [&] ( BOARD_ITEM* bitem ) { items.push_back( bitem ); } );
         }
+        else if( item->Type() == PCB_GROUP_T )
+        {
+            GROUP* group = static_cast<GROUP*>( item );
+            group->RunOnChildren( [&] ( BOARD_ITEM* bitem ) { items.push_back( bitem ); } );
         }
-
+    }
+#endif
     return items;
 }
 

pcbnew/tools/placement_tool.cpp

@@ -143,16 +143,16 @@ size_t ALIGN_DISTRIBUTE_TOOL::GetSelections( ALIGNMENT_RECTS& aItems, ALIGNMENT_
 {
 
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
-            { EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS ); } );
+            { EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); } );
 
     if( selection.Size() <= 1 )
         return 0;
 
     std::vector<BOARD_ITEM*> lockedItems;
     selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
-            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED ); }, &lockedItems );
+            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED, sTool ); }, &lockedItems );
 
     aItems = GetBoundingBoxes( selection );
     aLocked = GetBoundingBoxes( lockedItems );
@@ -394,8 +394,8 @@ int ALIGN_DISTRIBUTE_TOOL::AlignCenterY( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::DistributeHorizontally( const TOOL_EVENT& aEvent )
 {
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
-            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS ); } );
+            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } );
 
     if( selection.Size() <= 1 )
         return 0;
@@ -496,8 +496,8 @@ void ALIGN_DISTRIBUTE_TOOL::doDistributeCentersHorizontally( ALIGNMENT_RECTS &it
 int ALIGN_DISTRIBUTE_TOOL::DistributeVertically( const TOOL_EVENT& aEvent )
 {
     PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
-            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS ); } );
+            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } );
 
     if( selection.Size() <= 1 )
         return 0;

pcbnew/tools/position_relative_tool.cpp

@@ -67,8 +67,8 @@ int POSITION_RELATIVE_TOOL::PositionRelative( const TOOL_EVENT& aEvent )
     PCB_BASE_FRAME*         editFrame = getEditFrame<PCB_BASE_FRAME>();
 
     const auto& selection = m_selectionTool->RequestSelection(
-            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector )
+            []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
-            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS ); } );
+            { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } );
 
     if( selection.Empty() )
         return 0;
@@ -140,9 +140,9 @@ int POSITION_RELATIVE_TOOL::SelectPositionRelativeItem( const TOOL_EVENT& aEvent
         {
             m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true );
             const PCBNEW_SELECTION& sel = m_selectionTool->RequestSelection(
-                    []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector ) {
+                    []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) {
                         EditToolSelectionFilter(
-                                aCollector, EXCLUDE_TRANSIENTS | INCLUDE_PADS_AND_MODULES );
+                                aCollector, EXCLUDE_TRANSIENTS | INCLUDE_PADS_AND_MODULES, sTool );
                     } );
 
             if( sel.Empty() )

pcbnew/tools/selection_tool.cpp

@@ -118,6 +118,7 @@ SELECTION_TOOL::SELECTION_TOOL() :
         m_multiple( false ),
         m_skip_heuristics( false ),
         m_locked( true ),
+        m_enteredGroup( NULL ),
         m_priv( std::make_unique<PRIV>() )
 {
     m_filter.lockedItems = true;
@@ -182,6 +183,9 @@ void SELECTION_TOOL::Reset( RESET_REASON aReason )
     m_frame = getEditFrame<PCB_BASE_FRAME>();
     m_locked = true;
 
+    if( m_enteredGroup != NULL )
+        exitGroup();
+
     if( aReason == TOOL_BASE::MODEL_RELOAD )
     {
         // Deselect any item being currently in edit, to avoid unexpected behavior
@@ -266,8 +270,15 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
             if( m_selection.Empty() )
                 selectPoint( evt->Position() );
 
+            if( m_selection.GetSize() == 1 && m_selection[0]->Type() == PCB_GROUP_T )
+            {
+                EnterGroup();
+            }
+            else
+            {
                 m_toolMgr->RunAction( PCB_ACTIONS::properties, true );
             }
+        }
 
         // drag with LMB? Select multiple objects (or at least draw a selection box) or drag them
         else if( evt->IsDrag( BUT_LEFT ) )
@@ -310,6 +321,9 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
         {
             m_frame->FocusOnItem( nullptr );
 
+            if( m_enteredGroup != NULL )
+                exitGroup();
+
             ClearSelection();
 
             if( evt->FirstResponder() == this )
@@ -329,6 +343,29 @@ int SELECTION_TOOL::Main( const TOOL_EVENT& aEvent )
 }
 
 
+void SELECTION_TOOL::EnterGroup()
+{
+    wxCHECK_RET( m_selection.GetSize() == 1 && m_selection[0]->Type() == PCB_GROUP_T,
+                 _( "EnterGroup called when selection is not a single group") );
+    GROUP* aGroup = static_cast<GROUP*>( m_selection[0] );
+
+    if( m_enteredGroup != NULL )
+    {
+        exitGroup();
+    }
+
+    ClearSelection();
+    m_enteredGroup = aGroup;
+    m_enteredGroup->RunOnChildren( [&]( BOARD_ITEM* titem ) { select( titem ); } );
+}
+
+
+void SELECTION_TOOL::exitGroup()
+{
+    m_enteredGroup = NULL;
+}
+
+
 PCBNEW_SELECTION& SELECTION_TOOL::GetSelection()
 {
     return m_selection;
@@ -367,7 +404,7 @@ PCBNEW_SELECTION& SELECTION_TOOL::RequestSelection( CLIENT_SELECTION_FILTER aCli
             itemDispositions[ item ] = BEFORE;
         }
 
-        aClientFilter( VECTOR2I(), collector );
+        aClientFilter( VECTOR2I(), collector, this );
 
         for( EDA_ITEM* item : collector )
         {
@@ -377,6 +414,11 @@ PCBNEW_SELECTION& SELECTION_TOOL::RequestSelection( CLIENT_SELECTION_FILTER aCli
                 itemDispositions[ item ] = AFTER;
         }
 
+        // Unhighlight the BEFORE items before highlighting the AFTER items.
+        // This is so that in the case of groups, if aClientFilter replaces a selection
+        // with the enclosing group, the unhighlight of the element doesn't undo the
+        // recursive highlighting of that elemetn by the group.
+
         for( std::pair<EDA_ITEM* const, DISPOSITION> itemDisposition : itemDispositions )
         {
             BOARD_ITEM* item = static_cast<BOARD_ITEM*>( itemDisposition.first );
@@ -389,7 +431,14 @@ PCBNEW_SELECTION& SELECTION_TOOL::RequestSelection( CLIENT_SELECTION_FILTER aCli
 
                 unhighlight( item, SELECTED, &m_selection );
             }
-            else if( disposition == AFTER )
+        }
+
+        for( std::pair<EDA_ITEM* const, DISPOSITION> itemDisposition : itemDispositions )
+        {
+            BOARD_ITEM* item = static_cast<BOARD_ITEM*>( itemDisposition.first );
+            DISPOSITION disposition = itemDisposition.second;
+
+            if( disposition == AFTER )
             {
                 highlight( item, SELECTED, &m_selection );
             }
@@ -441,6 +490,10 @@ bool SELECTION_TOOL::selectPoint( const VECTOR2I& aWhere, bool aOnDrag,
 
     guide.SetIgnoreZoneFills( displayOpts.m_DisplayZonesMode != 0 );
 
+    if( m_enteredGroup &&
+        !m_enteredGroup->GetBoundingBox().Contains( wxPoint( aWhere.x, aWhere.y ) ) )
+        exitGroup();
+
     collector.Collect( board(),
         m_editModules ? GENERAL_COLLECTOR::ModuleItems : GENERAL_COLLECTOR::AllBoardItems,
         wxPoint( aWhere.x, aWhere.y ), guide );
@@ -457,7 +510,7 @@ bool SELECTION_TOOL::selectPoint( const VECTOR2I& aWhere, bool aOnDrag,
     // Allow the client to do tool- or action-specific filtering to see if we
     // can get down to a single item
     if( aClientFilter )
-        aClientFilter( aWhere, collector );
+        aClientFilter( aWhere, collector, this );
 
     // Apply the stateful filter
     filterCollectedItems( collector );
@@ -809,7 +862,7 @@ void SELECTION_TOOL::UnbrightenItem( BOARD_ITEM* aItem )
 }
 
 
-void connectedItemFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector )
+void connectedItemFilter( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool )
 {
     // Narrow the collection down to a single BOARD_CONNECTED_ITEM for each represented net.
     // All other items types are removed.
@@ -1433,6 +1486,11 @@ bool SELECTION_TOOL::itemPassesFilter( BOARD_ITEM* aItem )
             return false;
     }
 
+    if( m_enteredGroup != NULL )
+    {
+        return m_enteredGroup->GetItems().find( aItem ) != m_enteredGroup->GetItems().end();
+    }
+
     return true;
 }
 
@@ -1878,6 +1936,20 @@ bool SELECTION_TOOL::Selectable( const BOARD_ITEM* aItem, bool checkVisibilityOn
         break;
     }
 
+    case PCB_GROUP_T:
+    {
+        GROUP* group = const_cast<GROUP*>( static_cast<const GROUP*>( aItem ) );
+
+        // Similar to logic for module, a group is selectable if any of its
+        // members are. (This recurses)
+        for( auto item : group->GetItems() )
+        {
+            if( Selectable( item, true ) )
+                return true;
+        }
+
+        return false;
+    }
 
     case PCB_MARKER_T:  // Always selectable
         return true;
@@ -1924,9 +1996,22 @@ void SELECTION_TOOL::unselect( BOARD_ITEM* aItem )
         m_locked = true;
 }
 
-
 void SELECTION_TOOL::highlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION* aGroup )
 {
+    highlightInternal( aItem, aMode, aGroup, false );
+
+    view()->Update( aItem );
+
+    // Many selections are very temporal and updating the display each time just
+    // creates noise.
+    if( aMode == BRIGHTENED )
+        getView()->MarkTargetDirty( KIGFX::TARGET_OVERLAY );
+}
+
+void SELECTION_TOOL::highlightInternal( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION* aGroup, bool isChild )
+{
+    wxLogTrace( "GRP", wxString::Format( _( "highlight() of %s %p" ),
+                         aItem->GetSelectMenuText( m_frame->GetUserUnits() ) ), aItem );
     if( aMode == SELECTED )
         aItem->SetSelected();
     else if( aMode == BRIGHTENED )
@@ -1937,6 +2022,7 @@ void SELECTION_TOOL::highlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION*
         // Hide the original item, so it is shown only on overlay
         view()->Hide( aItem, true );
 
+        if( !isChild || aMode == BRIGHTENED )
             aGroup->Add( aItem );
     }
 
@@ -1944,23 +2030,20 @@ void SELECTION_TOOL::highlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION*
     // highlight all the parts that make the module, not the module itself
     if( aItem->Type() == PCB_MODULE_T )
     {
-        static_cast<MODULE*>( aItem )->RunOnChildren(
+        static_cast<MODULE*>( aItem )->RunOnChildren( [&]( BOARD_ITEM* titem ) {
-                [&]( BOARD_ITEM* item )
+                                                          highlightInternal( titem, aMode, aGroup, true ); } );
-                {
-                    if( aMode == SELECTED )
-                        item->SetSelected();
-                    else if( aMode == BRIGHTENED )
-                    {
-                        item->SetBrightened();
-
-                        if( aGroup )
-                            aGroup->Add( item );
     }
-
+    else if( aItem->Type() == PCB_GROUP_T )
-                    if( aGroup )
+    {
-                        view()->Hide( item, true );
+        static_cast<GROUP*>( aItem )->RunOnChildren( [&]( BOARD_ITEM* titem ) {
-                });
+                                                         highlightInternal( titem, aMode, aGroup, true ); } );
     }
+}
+
+
+void SELECTION_TOOL::unhighlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION* aGroup )
+{
+    unhighlightInternal( aItem, aMode, aGroup, false );
 
     view()->Update( aItem );
 
@@ -1971,8 +2054,10 @@ void SELECTION_TOOL::highlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION*
 }
 
 
-void SELECTION_TOOL::unhighlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION* aGroup )
+void SELECTION_TOOL::unhighlightInternal( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION* aGroup, bool isChild )
 {
+    wxLogTrace( "GRP", wxString::Format( _( "unhighlight() of %s %p" ),
+                                  aItem->GetSelectMenuText( m_frame->GetUserUnits() ) ), aItem );
     if( aMode == SELECTED )
         aItem->ClearSelected();
     else if( aMode == BRIGHTENED )
@@ -1984,38 +2069,25 @@ void SELECTION_TOOL::unhighlight( BOARD_ITEM* aItem, int aMode, PCBNEW_SELECTION
 
         // Restore original item visibility
         view()->Hide( aItem, false );
+
+        // N.B. if we clear the selection flag for sub-elements, we need to also
+        // remove the element from the selection group (if it exists)
+        if( isChild )
+            view()->Update( aItem );
     }
 
     // Modules are treated in a special way - when they are highlighted, we have to
     // highlight all the parts that make the module, not the module itself
     if( aItem->Type() == PCB_MODULE_T )
     {
-        static_cast<MODULE*>( aItem )->RunOnChildren(
+        static_cast<MODULE*>( aItem )->RunOnChildren( [&]( BOARD_ITEM* titem ) {
-                [&]( BOARD_ITEM* item )
+                                                          unhighlightInternal( titem, aMode, aGroup, true ); } );
-                {
-                    if( aMode == SELECTED )
-                        item->ClearSelected();
-                    else if( aMode == BRIGHTENED )
-                        item->ClearBrightened();
-
-                    // N.B. if we clear the selection flag for sub-elements, we need to also
-                    // remove the element from the selection group (if it exists)
-                    if( aGroup )
-                    {
-                        aGroup->Remove( item );
-
-                        view()->Hide( item, false );
-                        view()->Update( item );
     }
-                });
+    else if( aItem->Type() == PCB_GROUP_T )
+    {
+        static_cast<GROUP*>( aItem )->RunOnChildren( [&]( BOARD_ITEM* titem ) {
+                                                         unhighlightInternal( titem, aMode, aGroup, true ); } );
     }
-
-    view()->Update( aItem );
-
-    // Many selections are very temporal and updating the display each time just
-    // creates noise.
-    if( aMode == BRIGHTENED )
-        getView()->MarkTargetDirty( KIGFX::TARGET_OVERLAY );
 }
 
 
@@ -2452,6 +2524,43 @@ void SELECTION_TOOL::GuessSelectionCandidates( GENERAL_COLLECTOR& aCollector,
             aCollector.Transfer( item );
         }
     }
+
+    FilterCollectorForGroups( aCollector );
+}
+
+
+void SELECTION_TOOL::FilterCollectorForGroups( GENERAL_COLLECTOR& aCollector ) const
+{
+    std::unordered_set<BOARD_ITEM*> toAdd;
+
+    // If any element is a member of a group, replace those elements with the top containing group.
+    for( int j = 0; j < aCollector.GetCount(); ++j )
+    {
+        GROUP* aTop = board()->TopLevelGroup( aCollector[j], m_enteredGroup );
+
+        if( aTop != NULL )
+        {
+            if( aTop != aCollector[j] )
+            {
+                toAdd.insert( aTop );
+                aCollector.Remove( aCollector[j] );
+            }
+        }
+        else if( m_enteredGroup != NULL &&
+                 m_enteredGroup->GetItems().find( aCollector[j] ) == m_enteredGroup->GetItems().end() )
+        {
+            // If a group is entered, no selections of objects not in the group.
+            aCollector.Remove( aCollector[j] );
+        }
+    }
+
+    for( BOARD_ITEM* item : toAdd )
+    {
+        if( !aCollector.HasItem( item ) )
+        {
+            aCollector.Append( item );
+        }
+    }
 }
 
 

pcbnew/tools/selection_tool.h

@@ -47,7 +47,7 @@ namespace KIGFX
 }
 
 
-typedef void (*CLIENT_SELECTION_FILTER)( const VECTOR2I&, GENERAL_COLLECTOR& );
+typedef void (*CLIENT_SELECTION_FILTER)( const VECTOR2I&, GENERAL_COLLECTOR&, SELECTION_TOOL* );
 
 
 /**
@@ -125,6 +125,14 @@ public:
     void BrightenItem( BOARD_ITEM* aItem );
     void UnbrightenItem( BOARD_ITEM* aItem );
 
+    /**
+     * Function select()
+     * Takes necessary action mark an item as selected.
+     *
+     * @param aItem is an item to be selected.
+     */
+    void select( BOARD_ITEM* aItem );
+
     /**
      * Function selectable()
      * Checks conditions for an item to be selected.
@@ -168,6 +176,17 @@ public:
     ///> Zooms the screen to center and fit the current selection.
     void zoomFitSelection();
 
+    BOARD* GetBoard()
+    {
+        return board();
+    }
+
+    void EnterGroup();
+    void exitGroup();
+    void FilterCollectorForGroups( GENERAL_COLLECTOR& aCollector ) const;
+
+    GROUP* GetEnteredGroup() { return m_enteredGroup; }
+    
 private:
     /**
      * Function selectPoint()
@@ -289,14 +308,6 @@ private:
      */
     BOARD_ITEM* pickSmallestComponent( GENERAL_COLLECTOR* aCollector );
 
-    /**
-     * Function select()
-     * Takes necessary action mark an item as selected.
-     *
-     * @param aItem is an item to be selected.
-     */
-    void select( BOARD_ITEM* aItem );
-
     /**
      * Function unselect()
      * Takes necessary action mark an item as unselected.
@@ -342,6 +353,12 @@ private:
     const GENERAL_COLLECTORS_GUIDE getCollectorsGuide() const;
 
 private:
+    void highlightInternal( BOARD_ITEM* aItem, int aHighlightMode, PCBNEW_SELECTION* aGroup,
+        bool isChild);
+
+    void unhighlightInternal( BOARD_ITEM* aItem, int aHighlightMode, PCBNEW_SELECTION* aGroup,
+        bool isChild);
+
     PCB_BASE_FRAME*  m_frame;     // Pointer to the parent frame
     PCBNEW_SELECTION m_selection; // Current state of selection
 
@@ -353,6 +370,7 @@ private:
     bool m_multiple;              // Multiple selection mode is active
     bool m_skip_heuristics;       // Heuristics are not allowed when choosing item under cursor
     bool m_locked;                // Other tools are not allowed to modify locked items
+    GROUP* m_enteredGroup;        // If non-null, selections are limited to members of this group
 
     /// Private state (opaque pointer/compilation firewall)
     class PRIV;

pcbnew/undo_redo.cpp

@@ -150,6 +150,13 @@ static bool TestForExistingItem( BOARD* aPcb, BOARD_ITEM* aItem )
             return true;
     }
 
+    // Append groups:
+    for( auto item : aPcb->Groups() )
+    {
+        if( aItem == static_cast<BOARD_ITEM*>( item ) )
+            return true;
+    }
+
     return false;
 }
 
@@ -436,6 +443,7 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList, bool
         switch( eda_item->Type() )
         {
         case PCB_MODULE_T:
+        case PCB_GROUP_T:
             deep_reBuild_ratsnest = true;   // Pointers on pads can be invalid
             KI_FALLTHROUGH;
 

qa/pcbnew/CMakeLists.txt

@@ -44,6 +44,8 @@ set( QA_PCBNEW_SRCS
 
     drc/test_drc_courtyard_invalid.cpp
     drc/test_drc_courtyard_overlap.cpp
+
+    group_saveload.cpp
 )
 
 add_executable( qa_pcbnew

qa/pcbnew/group_saveload.cpp

@@ -0,0 +1,227 @@
+/*
+ * This program source code file is part of KiCad, a free EDA CAD application.
+ *
+ * Copyright (C) 2020 Joshua Redstone redstone at gmail.com
+ * Copyright (C) 1992-2020 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
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you may find one here:
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+ * or you may search the http://www.gnu.org website for the version 2 license,
+ * or you may write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA
+ */
+
+
+#include <boost/filesystem.hpp>
+#include <class_board.h>
+#include <class_module.h>
+#include <class_pcb_text.h>
+#include <common.h>
+#include <pcbnew_utils/board_construction_utils.h>
+#include <pcbnew_utils/board_file_utils.h>
+#include <string>
+#include <unit_test_utils/unit_test_utils.h>
+
+BOOST_AUTO_TEST_SUITE( GroupSaveLoad )
+
+// The tests below set up a test case with a spec for the set of groups to create.
+// A group can contain members from this list of candidates.
+enum ItemType
+{
+    TEXT0,
+    TEXT1,
+    TEXT2,
+    TEXT3,
+    TEXT4,
+    TEXT5,
+    TEXT6,
+    TEXT7,
+    TEXT8,
+    REMOVED_TEXT, // not known to board
+    GROUP0,
+    GROUP1,
+    GROUP2,
+    NAME_GROUP3,
+    NAME_GROUP4,
+    NAME_GROUP3_DUP, // Group with name identical to NAME_GROUP3
+    NUM_ITEMS
+};
+
+/*
+ * Takes a vector of group specifications for groups to create.
+ * Each group is a vector of which ItemTypes to put in the group.
+ * The first group corresponds to GROUP0, the second to GROUP1, and os on.
+ */
+BOARD* createBoard( const std::vector<std::vector<ItemType>>& spec )
+{
+    BOARD*                  aBoard = new BOARD();
+    std::vector<GROUP*>     groups;
+    std::vector<TEXTE_PCB*> textItems;
+
+    // Create groups
+    for( int idx = 0; idx < 6; idx++ )
+    {
+        GROUP* gr = new GROUP( aBoard );
+        if( idx >= ( NAME_GROUP3 - GROUP0 ) )
+        {
+            wxString name = wxString::Format(
+                    _( "group-%d" ), ( idx == ( NAME_GROUP3_DUP - GROUP0 ) ) ? 3 : idx );
+            gr->SetName( name );
+            BOOST_CHECK_EQUAL( gr->GetName(), name );
+        }
+        groups.push_back( gr );
+    }
+
+    // Create text items and add to board.
+    for( int idx = 0; idx < 10; idx++ )
+    {
+        auto textItem = new TEXTE_PCB( aBoard );
+        textItem->SetText( wxString::Format( _( "some text-%d" ), idx ) );
+        if( idx < 9 ) // don't add REMOVED_TEXT
+        {
+            aBoard->Add( textItem );
+        }
+        textItems.push_back( textItem );
+    }
+
+    // Populate groups based on spec
+    for( int groupIdx = 0; groupIdx < spec.size(); groupIdx++ )
+    {
+        auto&  groupSpec = spec[groupIdx];
+        GROUP* group     = groups[groupIdx];
+        int    count     = 0;
+        for( ItemType item : groupSpec )
+        {
+            if( item <= REMOVED_TEXT )
+            {
+                group->AddItem( textItems[item] );
+                count++;
+            }
+            else // it's a group
+            {
+                group->AddItem( groups[item - GROUP0] );
+                count++;
+            }
+        }
+        BOOST_CHECK_EQUAL( group->GetItems().size(), count );
+        aBoard->Add( group );
+    }
+
+    BOOST_TEST_CHECKPOINT( "Returning fresh board" );
+    return aBoard;
+}
+
+// Check if two groups are identical by comparing the fields (by Uuid).
+void testGroupEqual( const GROUP& group1, const GROUP& group2 )
+{
+    BOOST_CHECK_EQUAL( group1.m_Uuid.AsString(), group2.m_Uuid.AsString() );
+    BOOST_CHECK_EQUAL( group1.GetName(), group2.GetName() );
+    auto items1 = group1.GetItems();
+    auto items2 = group2.GetItems();
+
+    // Test that the sets items1 and items2 are identical, by checking m_Uuid
+    BOOST_CHECK_EQUAL( items1.size(), items2.size() );
+    for( auto item1 : items1 )
+    {
+        auto item2 = std::find_if( items2.begin(), items2.end(),
+                [&]( auto elem ) { return elem->m_Uuid.AsString() == item1->m_Uuid.AsString(); } );
+        BOOST_CHECK( item2 != items2.end() );
+        // Could check other properties here...
+    }
+}
+
+// Check if two GROUPS are identical by comparing the groups in each of them.
+void testGroupsEqual( const GROUPS& groups1, const GROUPS& groups2 )
+{
+    BOOST_CHECK_EQUAL( groups1.size(), groups2.size() );
+    for( auto group1 : groups1 )
+    {
+        auto group2 = std::find_if( groups2.begin(), groups2.end(),
+                [&]( auto elem ) { return elem->m_Uuid.AsString() == group1->m_Uuid.AsString(); } );
+        BOOST_CHECK( group2 != groups2.end() );
+        testGroupEqual( *group1, **group2 );
+    }
+}
+
+/*
+ * Create board based on spec, save it to a file, load it, and make sure the
+ * groups in the resulting board are the same as the groups we started with.
+ */
+void testSaveLoad( const std::vector<std::vector<ItemType>>& spec )
+{
+    BOARD* aBoard1 = createBoard( spec );
+    auto path = boost::filesystem::temp_directory_path() / "group_saveload_tst.kicad_pcb";
+    ::KI_TEST::DumpBoardToFile( *aBoard1, path.string() );
+    auto aBoard2 = ::KI_TEST::ReadBoardFromFileOrStream( path.string() );
+    testGroupsEqual( aBoard1->Groups(), aBoard2->Groups() );
+}
+
+// Test saving & loading of a few configurations
+BOOST_AUTO_TEST_CASE( HealthyCases )
+{
+    //BOOST_TEST_CONTEXT( "happy" );
+
+    // Test board with no groups
+    testSaveLoad( {} );
+
+    // Single group
+    testSaveLoad( { { TEXT0 } } );
+    testSaveLoad( { { TEXT0, TEXT1 } } );
+
+    // Two groups
+    testSaveLoad( { { TEXT0, TEXT1 }, { TEXT2, TEXT3 } } );
+    testSaveLoad( { { TEXT0, TEXT1 }, { TEXT2, GROUP0 } } );
+
+    // Subgroups by no cycle
+    testSaveLoad( { { TEXT0, GROUP1 }, { TEXT2 }, { TEXT3, GROUP0 } } );
+    testSaveLoad( { { TEXT0 }, { TEXT2 }, { GROUP1, GROUP0 } } );
+    testSaveLoad( { { TEXT0 }, { TEXT1 }, { TEXT2, NAME_GROUP3 }, { TEXT3 } } );
+    testSaveLoad( { { TEXT0 }, { TEXT1 }, { TEXT2, NAME_GROUP3 }, { TEXT3, GROUP0 } } );
+    testSaveLoad( { { TEXT0 }, { TEXT1 }, { TEXT2 }, { TEXT3 }, { NAME_GROUP3, GROUP0 } } );
+}
+
+BOOST_AUTO_TEST_CASE( ErrorCases )
+{
+    // A cycle
+    BOARD* aBoard1 = createBoard( { { TEXT0, GROUP1 }, { TEXT2, GROUP0 } } );
+    BOOST_CHECK_EQUAL( aBoard1->GroupsSanityCheck(), "Cycle detected in group membership" );
+
+    // More complex cycle
+    aBoard1 = createBoard( { { TEXT0, GROUP1 }, { TEXT1 }, { TEXT2, NAME_GROUP4 },
+                             { TEXT3, GROUP2 }, { TEXT4, NAME_GROUP3 } } );
+    BOOST_CHECK_EQUAL( aBoard1->GroupsSanityCheck(), "Cycle detected in group membership" );
+
+    // Reference group not on board
+    aBoard1      = createBoard( { { TEXT0, GROUP1 } } );
+    wxString res = aBoard1->GroupsSanityCheck();
+    BOOST_CHECK_MESSAGE( res.find( "contains deleted item" ) != std::string::npos, res );
+
+    // Single empty group
+    aBoard1      = createBoard( { {} } );
+    res          = aBoard1->GroupsSanityCheck();
+    BOOST_CHECK_MESSAGE(
+            res.find( "Group must have at least one member" ) != std::string::npos, res );
+
+    // Duplicate group name
+    aBoard1 = createBoard( { { TEXT0 }, { TEXT1 }, { TEXT2 }, { TEXT3 }, { TEXT4 }, { TEXT5 } } );
+    res     = aBoard1->GroupsSanityCheck();
+    BOOST_CHECK_MESSAGE( res.find( "Two groups of identical name" ) != std::string::npos, res );
+
+    // Group references item that is not on board
+    aBoard1 = createBoard( { { REMOVED_TEXT } } );
+    res     = aBoard1->GroupsSanityCheck();
+    BOOST_CHECK_MESSAGE( res.find( "contains deleted item" ) != std::string::npos, res );
+}
+
+BOOST_AUTO_TEST_SUITE_END()