diff --git a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp index f2cd2f69cb..ec0afd8411 100644 --- a/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp +++ b/3d-viewer/3d_canvas/create_3Dgraphic_brd_items.cpp @@ -380,9 +380,6 @@ void BOARD_ADAPTER::createPadWithMargin( const PAD* aPad, CONTAINER_2D_BASE* aCo dummy.SetSize( aLayer, VECTOR2I( dummySize.x, dummySize.y ) ); dummy.TransformShapeToPolygon( poly, aLayer, 0, maxError, ERROR_INSIDE ); clearance = { 0, 0 }; - - // Remove group membership from dummy item before deleting - dummy.SetParentGroup( nullptr ); } else if( aPad->GetShape( aLayer ) == PAD_SHAPE::CUSTOM ) { diff --git a/3d-viewer/3d_canvas/create_layer_items.cpp b/3d-viewer/3d_canvas/create_layer_items.cpp index 0c9d33b619..2ed71ff2a8 100644 --- a/3d-viewer/3d_canvas/create_layer_items.cpp +++ b/3d-viewer/3d_canvas/create_layer_items.cpp @@ -1179,10 +1179,10 @@ void BOARD_ADAPTER::createLayers( REPORTER* aStatusReporter ) // ADD PLATED COPPER ConvertPolygonToTriangles( *m_frontPlatedCopperPolys, *m_platedPadsFront, m_biuTo3Dunits, - *m_board->GetItem( niluuid ) ); + *DELETED_BOARD_ITEM::GetInstance() ); ConvertPolygonToTriangles( *m_backPlatedCopperPolys, *m_platedPadsBack, m_biuTo3Dunits, - *m_board->GetItem( niluuid ) ); + *DELETED_BOARD_ITEM::GetInstance() ); m_platedPadsFront->BuildBVH(); m_platedPadsBack->BuildBVH(); diff --git a/common/commit.cpp b/common/commit.cpp index e0f01bd6b4..0253e4a948 100644 --- a/common/commit.cpp +++ b/common/commit.cpp @@ -39,28 +39,12 @@ COMMIT::COMMIT() COMMIT::~COMMIT() { for( COMMIT_LINE& ent : m_changes ) - { - if( ent.m_copy ) - { - // If we're deleting changes, we have a commit that is being abandoned, - // and the copies of items with group memberships can have their group membership - // cleared as long as we make sure that group doesn't hold a reference to the copy. - if( ent.m_copy->GetParentGroup() ) - { - // This is a copy, so it should not be in the group. - // (RemoveItem() returns false when it doesn't find the item), - // but if it is we need to remove it to prevent a crash. - wxASSERT( ent.m_copy->GetParentGroup()->RemoveItem( ent.m_copy ) == false ); - ent.m_copy->SetParentGroup( nullptr ); - } - - delete ent.m_copy; - } - } + delete ent.m_copy; } -COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aScreen ) +COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aScreen, + RECURSE_MODE aRecurse ) { // CHT_MODIFY and CHT_DONE are not compatible wxASSERT( ( aChangeType & ( CHT_MODIFY | CHT_DONE ) ) != ( CHT_MODIFY | CHT_DONE ) ); @@ -71,32 +55,25 @@ COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aS { case CHT_ADD: makeEntry( aItem, CHT_ADD | flag, nullptr, aScreen ); - return *this; + break; case CHT_REMOVE: wxASSERT( m_deletedItems.find( aItem ) == m_deletedItems.end() ); m_deletedItems.insert( aItem ); - makeEntry( aItem, CHT_REMOVE | flag, nullptr, aScreen ); - return *this; + makeEntry( aItem, CHT_REMOVE | flag, makeImage( aItem ), aScreen ); + + if( EDA_GROUP* parentGroup = aItem->GetParentGroup() ) + Modify( parentGroup->AsEdaItem(), aScreen ); + + break; case CHT_MODIFY: { EDA_ITEM* parent = parentObject( aItem ); - EDA_ITEM* clone = makeImage( parent ); - - wxASSERT( clone ); - - if( clone ) - return createModified( parent, clone, flag, aScreen ); - + createModified( parent, makeImage( parent ), flag, aScreen ); break; } - case CHT_GROUP: - case CHT_UNGROUP: - makeEntry( aItem, aChangeType, nullptr, aScreen ); - return *this; - default: wxFAIL; } @@ -106,7 +83,7 @@ COMMIT& COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aS COMMIT& COMMIT::Stage( std::vector &container, CHANGE_TYPE aChangeType, - BASE_SCREEN *aScreen ) + BASE_SCREEN *aScreen ) { for( EDA_ITEM* item : container ) Stage( item, aChangeType, aScreen); @@ -157,7 +134,6 @@ COMMIT& COMMIT::createModified( EDA_ITEM* aItem, EDA_ITEM* aCopy, int aExtraFlag if( m_changedItems.find( parent ) != m_changedItems.end() ) { - aCopy->SetParentGroup( nullptr ); delete aCopy; return *this; // item has been already modified once } @@ -170,9 +146,6 @@ COMMIT& COMMIT::createModified( EDA_ITEM* aItem, EDA_ITEM* aCopy, int aExtraFlag void COMMIT::makeEntry( EDA_ITEM* aItem, CHANGE_TYPE aType, EDA_ITEM* aCopy, BASE_SCREEN *aScreen ) { - // Expect an item copy if it is going to be modified - wxASSERT( !!aCopy == ( ( aType & CHT_TYPE ) == CHT_MODIFY ) ); - COMMIT_LINE ent; ent.m_item = aItem; @@ -206,8 +179,6 @@ CHANGE_TYPE COMMIT::convert( UNDO_REDO aType ) const { case UNDO_REDO::NEWITEM: return CHT_ADD; case UNDO_REDO::DELETED: return CHT_REMOVE; - case UNDO_REDO::REGROUP: return CHT_GROUP; - case UNDO_REDO::UNGROUP: return CHT_UNGROUP; case UNDO_REDO::CHANGED: return CHT_MODIFY; default: wxASSERT( false ); return CHT_MODIFY; } @@ -220,8 +191,6 @@ UNDO_REDO COMMIT::convert( CHANGE_TYPE aType ) const { case CHT_ADD: return UNDO_REDO::NEWITEM; case CHT_REMOVE: return UNDO_REDO::DELETED; - case CHT_GROUP: return UNDO_REDO::REGROUP; - case CHT_UNGROUP: return UNDO_REDO::UNGROUP; case CHT_MODIFY: return UNDO_REDO::CHANGED; default: wxASSERT( false ); return UNDO_REDO::CHANGED; } diff --git a/common/dialogs/dialog_group_properties.cpp b/common/dialogs/dialog_group_properties.cpp index b52826cfa4..846d64e03b 100644 --- a/common/dialogs/dialog_group_properties.cpp +++ b/common/dialogs/dialog_group_properties.cpp @@ -83,7 +83,7 @@ bool DIALOG_GROUP_PROPERTIES::TransferDataToWindow() bool DIALOG_GROUP_PROPERTIES::TransferDataFromWindow() { - m_commit->Modify( m_group->AsEdaItem(), m_frame->GetScreen() ); + m_commit->Modify( m_group->AsEdaItem(), m_frame->GetScreen(), RECURSE_MODE::RECURSE ); for( size_t ii = 0; ii < m_membersList->GetCount(); ++ii ) { @@ -117,7 +117,9 @@ bool DIALOG_GROUP_PROPERTIES::TransferDataFromWindow() m_group->SetDesignBlockLibId( libId ); } else + { m_group->SetDesignBlockLibId( LIB_ID() ); + } m_toolMgr->RunAction( ACTIONS::selectionClear ); m_group->RemoveAll(); diff --git a/common/eda_group.cpp b/common/eda_group.cpp index de953d3b49..d4c704fe1a 100644 --- a/common/eda_group.cpp +++ b/common/eda_group.cpp @@ -1,6 +1,68 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The 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 -EDA_GROUP::~EDA_GROUP() + +void EDA_GROUP::AddItem( EDA_ITEM* aItem ) { + wxCHECK_RET( aItem, wxT( "Nullptr added to group." ) ); + + // Items can only be in one group at a time + if( EDA_GROUP* parentGroup = aItem->GetParentGroup() ) + parentGroup->RemoveItem( aItem ); + + m_items.insert( aItem ); + aItem->SetParentGroup( this ); } + + +void EDA_GROUP::RemoveItem( EDA_ITEM* aItem ) +{ + wxCHECK_RET( aItem, wxT( "Nullptr removed from group." ) ); + + if( m_items.erase( aItem ) == 1 ) + aItem->SetParentGroup( nullptr ); +} + + +void EDA_GROUP::RemoveAll() +{ + for( EDA_ITEM* item : m_items ) + item->SetParentGroup( nullptr ); + + m_items.clear(); +} + + +KIID_VECT_LIST EDA_GROUP::GetGroupMemberIds() const +{ + KIID_VECT_LIST members; + + for( EDA_ITEM* item : m_items ) + members.push_back( item->m_Uuid ); + + return members; +} + + diff --git a/common/eda_group.h b/common/eda_group.h index fabceba65a..f51689b701 100644 --- a/common/eda_group.h +++ b/common/eda_group.h @@ -46,53 +46,36 @@ class EDA_GROUP { public: virtual EDA_ITEM* AsEdaItem() = 0; - virtual ~EDA_GROUP(); + virtual ~EDA_GROUP() = default; wxString GetName() const { return m_name; } void SetName( const wxString& aName ) { m_name = aName; } std::unordered_set& GetItems() { return m_items; } - const std::unordered_set& GetItems() const { return m_items; } /** * Add item to group. Does not take ownership of item. - * - * @return true if item was added (false if item belongs to a different group). */ - virtual bool AddItem( EDA_ITEM* aItem ) = 0; + void AddItem( EDA_ITEM* aItem ); /** * Remove item from group. - * - * @return true if item was removed (false if item was not in the group). */ - virtual bool RemoveItem( EDA_ITEM* aItem ) = 0; + void RemoveItem( EDA_ITEM* aItem ); + void RemoveAll(); - virtual void RemoveAll() = 0; - - /* - * Clone() this and all descendants - */ - virtual EDA_GROUP* DeepClone() const = 0; - - /* - * Duplicate() this and all descendants - */ - virtual EDA_GROUP* DeepDuplicate() const = 0; + KIID_VECT_LIST GetGroupMemberIds() const; bool HasDesignBlockLink() const { return m_designBlockLibId.IsValid(); } void SetDesignBlockLibId( const LIB_ID& aLibId ) { m_designBlockLibId = aLibId; } - const LIB_ID& GetDesignBlockLibId() const { return m_designBlockLibId; } protected: - std::unordered_set m_items; // Members of the group - wxString m_name; // Optional group name - - // Optional link to a design block - LIB_ID m_designBlockLibId; + std::unordered_set m_items; // Members of the group + wxString m_name; // Optional group name + LIB_ID m_designBlockLibId; // Optional link to a design block }; #endif // CLASS_PCB_GROUP_H_ diff --git a/common/eda_item.cpp b/common/eda_item.cpp index cbbab533d6..ec2cd39ff7 100644 --- a/common/eda_item.cpp +++ b/common/eda_item.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -71,6 +72,31 @@ EDA_ITEM::EDA_ITEM( const EDA_ITEM& base ) : } +EDA_ITEM* EDA_ITEM::findParent( KICAD_T aType ) const +{ + EDA_ITEM* parent = GetParent(); + + while( parent ) + { + if( parent->Type() == aType ) + return parent; + else + parent = parent->GetParent(); + } + + return nullptr; +} + + +KIID EDA_ITEM::GetParentGroupId() const +{ + if( EDA_GROUP* group = GetParentGroup() ) + return group->AsEdaItem()->m_Uuid; + else + return niluuid; +} + + void EDA_ITEM::SetModified() { SetFlags( IS_CHANGED ); diff --git a/common/rc_item.cpp b/common/rc_item.cpp index 9238077fa2..f8840e30e2 100644 --- a/common/rc_item.cpp +++ b/common/rc_item.cpp @@ -402,9 +402,8 @@ unsigned int RC_TREE_MODEL::GetChildren( wxDataViewItem const& aItem, } -void RC_TREE_MODEL::GetValue( wxVariant& aVariant, - wxDataViewItem const& aItem, - unsigned int aCol ) const +void RC_TREE_MODEL::GetValue( wxVariant& aVariant, wxDataViewItem const& aItem, + unsigned int aCol ) const { const RC_TREE_NODE* node = ToNode( aItem ); const std::shared_ptr rcItem = node->m_RcItem; @@ -443,20 +442,20 @@ void RC_TREE_MODEL::GetValue( wxVariant& aVariant, if( marker && marker->GetMarkerType() == MARKER_BASE::MARKER_DRAWING_SHEET ) msg = _( "Drawing Sheet" ); else - item = m_editFrame->GetItem( rcItem->GetMainItemID() ); + item = m_editFrame->ResolveItem( rcItem->GetMainItemID() ); break; case RC_TREE_NODE::AUX_ITEM: - item = m_editFrame->GetItem( rcItem->GetAuxItemID() ); + item = m_editFrame->ResolveItem( rcItem->GetAuxItemID() ); break; case RC_TREE_NODE::AUX_ITEM2: - item = m_editFrame->GetItem( rcItem->GetAuxItem2ID() ); + item = m_editFrame->ResolveItem( rcItem->GetAuxItem2ID() ); break; case RC_TREE_NODE::AUX_ITEM3: - item = m_editFrame->GetItem( rcItem->GetAuxItem3ID() ); + item = m_editFrame->ResolveItem( rcItem->GetAuxItem3ID() ); break; case RC_TREE_NODE::COMMENT: diff --git a/common/tool/group_tool.cpp b/common/tool/group_tool.cpp index 790214f03d..b3ea28492d 100644 --- a/common/tool/group_tool.cpp +++ b/common/tool/group_tool.cpp @@ -164,18 +164,18 @@ int GROUP_TOOL::Ungroup( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : selCopy ) { - EDA_GROUP* group = dynamic_cast( item ); - - if( group ) + if( EDA_GROUP* group = dynamic_cast( item ) ) { + group->AsEdaItem()->SetSelected(); + m_commit->Remove( group->AsEdaItem(), m_frame->GetScreen() ); + for( EDA_ITEM* member : group->GetItems() ) { - m_commit->Stage( member, CHT_UNGROUP, m_frame->GetScreen() ); + m_commit->Modify( member, m_frame->GetScreen() ); toSelect.push_back( member ); } - group->AsEdaItem()->SetSelected(); - m_commit->Remove( group->AsEdaItem(), m_frame->GetScreen() ); + group->RemoveAll(); } } @@ -208,7 +208,9 @@ int GROUP_TOOL::AddToGroup( const TOOL_EVENT& aEvent ) group = item; } else if( !item->GetParentGroup() ) + { toAdd.push_back( item ); + } } if( !group || toAdd.empty() ) @@ -219,7 +221,18 @@ int GROUP_TOOL::AddToGroup( const TOOL_EVENT& aEvent ) m_commit->Modify( group, m_frame->GetScreen() ); for( EDA_ITEM* item : toAdd ) - m_commit->Stage( item, CHT_GROUP, m_frame->GetScreen() ); + { + EDA_GROUP* existingGroup = item->GetParentGroup(); + KIID existingGroupId = existingGroup ? existingGroup->AsEdaItem()->m_Uuid : niluuid; + + if( existingGroupId != group->m_Uuid ) + { + m_commit->Modify( item, m_frame->GetScreen() ); + + if( existingGroup ) + m_commit->Modify( existingGroup->AsEdaItem(), m_frame->GetScreen() ); + } + } m_commit->Push( _( "Add Items to Group" ) ); @@ -240,8 +253,12 @@ int GROUP_TOOL::RemoveFromGroup( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : selection ) { - if( item->GetParentGroup() ) - m_commit->Stage( item, CHT_UNGROUP, m_frame->GetScreen() ); + if( EDA_GROUP* group = item->GetParentGroup() ) + { + m_commit->Modify( group->AsEdaItem(), m_frame->GetScreen() ); + m_commit->Modify( item, m_frame->GetScreen() ); + group->RemoveItem( item ); + } } m_commit->Push( _( "Remove Group Items" ) ); diff --git a/common/undo_redo_container.cpp b/common/undo_redo_container.cpp index 1b761019d4..78f1a74bf5 100644 --- a/common/undo_redo_container.cpp +++ b/common/undo_redo_container.cpp @@ -24,6 +24,7 @@ */ #include +#include #include @@ -47,13 +48,6 @@ ITEM_PICKER::ITEM_PICKER( BASE_SCREEN* aScreen, EDA_ITEM* aItem, UNDO_REDO aUndo } -void ITEM_PICKER::SetItem( EDA_ITEM* aItem ) -{ - m_pickedItem = aItem; - m_pickedItemType = aItem ? aItem->Type() : TYPE_NOT_INIT; -} - - PICKED_ITEMS_LIST::PICKED_ITEMS_LIST() { } @@ -64,6 +58,29 @@ PICKED_ITEMS_LIST::~PICKED_ITEMS_LIST() } +void ITEM_PICKER::SetItem( EDA_ITEM* aItem ) +{ + m_pickedItem = aItem; + m_pickedItemType = aItem ? aItem->Type() : TYPE_NOT_INIT; + + if( EDA_GROUP* group = dynamic_cast( aItem ) ) + m_groupMembers = group->GetGroupMemberIds(); + + m_groupId = aItem->GetParentGroupId(); +} + + +void ITEM_PICKER::SetLink( EDA_ITEM* aItem ) +{ + m_link = aItem; + + if( EDA_GROUP* group = dynamic_cast( aItem ) ) + m_groupMembers = group->GetGroupMemberIds(); + + m_groupId = aItem->GetParentGroupId(); +} + + void PICKED_ITEMS_LIST::PushItem( const ITEM_PICKER& aItem ) { m_ItemsList.push_back( aItem ); @@ -96,22 +113,6 @@ bool PICKED_ITEMS_LIST::ContainsItem( const EDA_ITEM* aItem ) const } -bool PICKED_ITEMS_LIST::ContainsItemType( KICAD_T aItemType ) const -{ - for( const ITEM_PICKER& picker : m_ItemsList ) - { - const EDA_ITEM* item = picker.GetItem(); - - wxCHECK2( item, continue ); - - if( item->Type() == aItemType ) - return true; - } - - return false; -} - - int PICKED_ITEMS_LIST::FindItem( const EDA_ITEM* aItem ) const { for( size_t i = 0; i < m_ItemsList.size(); i++ ) @@ -157,14 +158,15 @@ void PICKED_ITEMS_LIST::ClearListAndDeleteItems( std::function } -ITEM_PICKER PICKED_ITEMS_LIST::GetItemWrapper( unsigned int aIdx ) const +const ITEM_PICKER& PICKED_ITEMS_LIST::GetItemWrapper( unsigned int aIdx ) const { - ITEM_PICKER picker; + return m_ItemsList.at( aIdx ); +} - if( aIdx < m_ItemsList.size() ) - picker = m_ItemsList[aIdx]; - return picker; +ITEM_PICKER& PICKED_ITEMS_LIST::GetItemWrapper( unsigned int aIdx ) +{ + return m_ItemsList.at( aIdx ); } @@ -213,27 +215,6 @@ EDA_ITEM_FLAGS PICKED_ITEMS_LIST::GetPickerFlags( unsigned aIdx ) const } -KIID PICKED_ITEMS_LIST::GetPickedItemGroupId( unsigned aIdx ) const -{ - if( aIdx < m_ItemsList.size() ) - return m_ItemsList[aIdx].GetGroupId(); - - return KIID(); -} - - -bool PICKED_ITEMS_LIST::SetPickedItemGroupId( KIID aGroupId, unsigned aIdx ) -{ - if( aIdx < m_ItemsList.size() ) - { - m_ItemsList[aIdx].SetGroupId( aGroupId ); - return true; - } - - return false; -} - - bool PICKED_ITEMS_LIST::SetPickedItem( EDA_ITEM* aItem, unsigned aIdx ) { if( aIdx < m_ItemsList.size() ) @@ -258,19 +239,6 @@ bool PICKED_ITEMS_LIST::SetPickedItemLink( EDA_ITEM* aLink, unsigned aIdx ) } -bool PICKED_ITEMS_LIST::SetPickedItem( EDA_ITEM* aItem, UNDO_REDO aStatus, unsigned aIdx ) -{ - if( aIdx < m_ItemsList.size() ) - { - m_ItemsList[aIdx].SetItem( aItem ); - m_ItemsList[aIdx].SetStatus( aStatus ); - return true; - } - - return false; -} - - bool PICKED_ITEMS_LIST::SetPickedItemStatus( UNDO_REDO aStatus, unsigned aIdx ) { if( aIdx < m_ItemsList.size() ) diff --git a/cvpcb/display_footprints_frame.cpp b/cvpcb/display_footprints_frame.cpp index b123696e40..de47f0e435 100644 --- a/cvpcb/display_footprints_frame.cpp +++ b/cvpcb/display_footprints_frame.cpp @@ -308,10 +308,8 @@ FOOTPRINT* DISPLAY_FOOTPRINTS_FRAME::GetFootprint( const wxString& aFootprintNam try { - const FOOTPRINT* fp = fpTable->GetEnumeratedFootprint( libNickname, fpName ); - - if( fp ) - footprint = static_cast( fp->Duplicate() ); + if( const FOOTPRINT* fp = fpTable->GetEnumeratedFootprint( libNickname, fpName ) ) + footprint = static_cast( fp->Duplicate( IGNORE_PARENT_GROUP ) ); } catch( const IO_ERROR& ioe ) { diff --git a/eeschema/bus-wire-junction.cpp b/eeschema/bus-wire-junction.cpp index 26ed3a9aa9..395c3f7d8c 100644 --- a/eeschema/bus-wire-junction.cpp +++ b/eeschema/bus-wire-junction.cpp @@ -302,7 +302,7 @@ void SCH_EDIT_FRAME::BreakSegment( SCH_COMMIT* aCommit, SCH_LINE* aSegment, cons // Save the copy of aSegment before breaking it aCommit->Modify( aSegment, aScreen ); - SCH_LINE* newSegment = aSegment->BreakAt( aPoint ); + SCH_LINE* newSegment = aSegment->BreakAt( aCommit, aPoint ); aSegment->SetFlags( IS_CHANGED | IS_BROKEN ); newSegment->SetFlags( IS_NEW | IS_BROKEN ); diff --git a/eeschema/cross-probing.cpp b/eeschema/cross-probing.cpp index b635b1f201..cc58f07e7f 100644 --- a/eeschema/cross-probing.cpp +++ b/eeschema/cross-probing.cpp @@ -964,7 +964,7 @@ void SCH_EDIT_FRAME::KiwayMailIn( KIWAY_EXPRESS& mail ) KIID uuid( payload ); SCH_SHEET_PATH path; - if( SCH_ITEM* item = m_schematic->GetItem( uuid, &path ) ) + if( SCH_ITEM* item = m_schematic->ResolveItem( uuid, &path, true ) ) { if( item->Type() == SCH_SHEET_T ) payload = static_cast( item )->GetShownName( false ); diff --git a/eeschema/dialogs/dialog_erc.cpp b/eeschema/dialogs/dialog_erc.cpp index 7507dddd57..861d0657c4 100644 --- a/eeschema/dialogs/dialog_erc.cpp +++ b/eeschema/dialogs/dialog_erc.cpp @@ -530,7 +530,7 @@ void DIALOG_ERC::OnERCItemSelected( wxDataViewEvent& aEvent ) { const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); SCH_SHEET_PATH sheet; - SCH_ITEM* item = m_parent->Schematic().GetItem( itemID, &sheet ); + SCH_ITEM* item = m_parent->Schematic().ResolveItem( itemID, &sheet, true ); if( m_centerMarkerOnIdle ) { @@ -544,8 +544,8 @@ void DIALOG_ERC::OnERCItemSelected( wxDataViewEvent& aEvent ) if( node ) { // Determine the owning sheet for sheet-specific items - std::shared_ptr ercItem = - std::static_pointer_cast( node->m_RcItem ); + std::shared_ptr ercItem = std::static_pointer_cast( node->m_RcItem ); + switch( node->m_Type ) { case RC_TREE_NODE::MARKER: @@ -569,8 +569,7 @@ void DIALOG_ERC::OnERCItemSelected( wxDataViewEvent& aEvent ) if( !sheet.empty() && sheet != m_parent->GetCurrentSheet() ) { - m_parent->GetToolManager()->RunAction( SCH_ACTIONS::changeSheet, - &sheet ); + m_parent->GetToolManager()->RunAction( SCH_ACTIONS::changeSheet, &sheet ); m_parent->RedrawScreen( m_parent->GetScreen()->m_ScrollCenter, false ); } diff --git a/eeschema/erc/erc_item.cpp b/eeschema/erc/erc_item.cpp index f5905d90c6..23cccc7a79 100644 --- a/eeschema/erc/erc_item.cpp +++ b/eeschema/erc/erc_item.cpp @@ -425,7 +425,7 @@ void ERC_TREE_MODEL::GetValue( wxVariant& aVariant, wxDataViewItem const& aItem, } else { - msg = getItemDesc( schEditFrame->GetItem( ercItem->GetMainItemID() ), + msg = getItemDesc( schEditFrame->ResolveItem( ercItem->GetMainItemID() ), ercItem->MainItemHasSheetPath() ? ercItem->GetMainItemSheetPath() : schEditFrame->GetCurrentSheet() ); } @@ -433,18 +433,18 @@ void ERC_TREE_MODEL::GetValue( wxVariant& aVariant, wxDataViewItem const& aItem, break; case RC_TREE_NODE::AUX_ITEM: - msg = getItemDesc( schEditFrame->GetItem( ercItem->GetAuxItemID() ), + msg = getItemDesc( schEditFrame->ResolveItem( ercItem->GetAuxItemID() ), ercItem->AuxItemHasSheetPath() ? ercItem->GetAuxItemSheetPath() : schEditFrame->GetCurrentSheet() ); break; case RC_TREE_NODE::AUX_ITEM2: - msg = getItemDesc( schEditFrame->GetItem( ercItem->GetAuxItem2ID() ), + msg = getItemDesc( schEditFrame->ResolveItem( ercItem->GetAuxItem2ID() ), schEditFrame->GetCurrentSheet() ); break; case RC_TREE_NODE::AUX_ITEM3: - msg = getItemDesc( schEditFrame->GetItem( ercItem->GetAuxItem3ID() ), + msg = getItemDesc( schEditFrame->ResolveItem( ercItem->GetAuxItem3ID() ), schEditFrame->GetCurrentSheet() ); break; diff --git a/eeschema/lib_symbol.cpp b/eeschema/lib_symbol.cpp index 535dd30e38..e48f304ca6 100644 --- a/eeschema/lib_symbol.cpp +++ b/eeschema/lib_symbol.cpp @@ -1357,7 +1357,7 @@ void LIB_SYMBOL::SetUnitCount( int aCount, bool aDuplicateDrawItems ) for( int j = prevCount + 1; j <= aCount; j++ ) { - SCH_ITEM* newItem = item.Duplicate(); + SCH_ITEM* newItem = item.Duplicate( IGNORE_PARENT_GROUP ); newItem->m_unit = j; tmp.push_back( newItem ); } @@ -1397,7 +1397,7 @@ void LIB_SYMBOL::SetHasAlternateBodyStyle( bool aHasAlternate, bool aDuplicatePi { if( item.m_bodyStyle == 1 ) { - SCH_ITEM* newItem = item.Duplicate(); + SCH_ITEM* newItem = item.Duplicate( IGNORE_PARENT_GROUP ); newItem->m_bodyStyle = 2; tmp.push_back( newItem ); } diff --git a/eeschema/sch_bitmap.cpp b/eeschema/sch_bitmap.cpp index 8a8b1e0035..4e1e67e48c 100644 --- a/eeschema/sch_bitmap.cpp +++ b/eeschema/sch_bitmap.cpp @@ -85,8 +85,6 @@ EDA_ITEM* SCH_BITMAP::Clone() const void SCH_BITMAP::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( aItem->Type() == SCH_BITMAP_T, wxString::Format( wxT( "SCH_BITMAP object cannot swap data with %s object." ), aItem->GetClass() ) ); diff --git a/eeschema/sch_bus_entry.cpp b/eeschema/sch_bus_entry.cpp index 0e15e68255..29457eb3f9 100644 --- a/eeschema/sch_bus_entry.cpp +++ b/eeschema/sch_bus_entry.cpp @@ -136,8 +136,6 @@ VECTOR2I SCH_BUS_ENTRY_BASE::GetEnd() const void SCH_BUS_ENTRY_BASE::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - SCH_BUS_ENTRY_BASE* item = dynamic_cast( aItem ); wxCHECK_RET( item, wxT( "Cannot swap bus entry data with invalid item." ) ); diff --git a/eeschema/sch_commit.cpp b/eeschema/sch_commit.cpp index 33b0b5fdc0..dd33e52abe 100644 --- a/eeschema/sch_commit.cpp +++ b/eeschema/sch_commit.cpp @@ -67,7 +67,8 @@ SCH_COMMIT::~SCH_COMMIT() } -COMMIT& SCH_COMMIT::Stage( EDA_ITEM *aItem, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen ) +COMMIT& SCH_COMMIT::Stage( EDA_ITEM *aItem, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen, + RECURSE_MODE aRecurse ) { wxCHECK( aItem, *this ); @@ -82,18 +83,12 @@ COMMIT& SCH_COMMIT::Stage( EDA_ITEM *aItem, CHANGE_TYPE aChangeType, BASE_SCREEN aChangeType = CHT_MODIFY; } - // Many operations (move, rotate, etc.) are applied directly to a group's children, so they - // must be staged as well. - if( aChangeType == CHT_MODIFY ) + if( aRecurse == RECURSE_MODE::RECURSE ) { if( SCH_GROUP* group = dynamic_cast( aItem ) ) { - group->RunOnChildren( - [&]( EDA_ITEM* child ) - { - Stage( child, aChangeType, aScreen ); - }, - RECURSE_MODE::NO_RECURSE ); + for( EDA_ITEM* member : group->GetItems() ) + Stage( member, aChangeType, aScreen, aRecurse ); } } @@ -198,12 +193,12 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) SCH_EDIT_FRAME* frame = static_cast( m_toolMgr->GetToolHolder() ); SCH_SELECTION_TOOL* selTool = m_toolMgr->GetTool(); + SCH_GROUP* enteredGroup = selTool ? selTool->GetEnteredGroup() : nullptr; bool itemsDeselected = false; bool selectedModified = false; bool dirtyConnectivity = false; bool refreshHierarchy = false; SCH_CLEANUP_FLAGS connectivityCleanUp = NO_CLEANUP; - SCH_GROUP* addedGroup = nullptr; if( Empty() ) return; @@ -215,6 +210,37 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) std::vector bulkRemovedItems; std::vector itemsChanged; + auto updateConnectivityFlag = + [&]( SCH_ITEM* schItem ) + { + if( schItem->IsConnectable() || ( schItem->Type() == SCH_RULE_AREA_T ) ) + { + dirtyConnectivity = true; + + // Do a local clean up if there are any connectable objects in the commit. + if( connectivityCleanUp == NO_CLEANUP ) + connectivityCleanUp = LOCAL_CLEANUP; + + // Do a full rebuild of the connectivity if there is a sheet in the commit. + if( schItem->Type() == SCH_SHEET_T ) + connectivityCleanUp = GLOBAL_CLEANUP; + } + }; + + // We don't know that anything will be added to the entered group, but it does no harm to + // add it to the commit anyway. + if( enteredGroup ) + Modify( enteredGroup ); + + for( COMMIT_LINE& ent : m_changes ) + { + SCH_ITEM* schItem = dynamic_cast( ent.m_item ); + int changeType = ent.m_type & CHT_TYPE; + + if( changeType == CHT_REMOVE && schItem->GetParentGroup() ) + Modify( schItem->GetParentGroup()->AsEdaItem() ); + } + for( COMMIT_LINE& ent : m_changes ) { int changeType = ent.m_type & CHT_TYPE; @@ -242,33 +268,14 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) RECURSE_MODE::NO_RECURSE ); } - auto updateConnectivityFlag = [&]() - { - if( schItem->IsConnectable() || ( schItem->Type() == SCH_RULE_AREA_T ) ) - { - dirtyConnectivity = true; - - // Do a local clean up if there are any connectable objects in the commit. - if( connectivityCleanUp == NO_CLEANUP ) - connectivityCleanUp = LOCAL_CLEANUP; - - // Do a full rebuild of the connectivity if there is a sheet in the commit. - if( schItem->Type() == SCH_SHEET_T ) - connectivityCleanUp = GLOBAL_CLEANUP; - } - }; - switch( changeType ) { case CHT_ADD: { - if( selTool && selTool->GetEnteredGroup() && !schItem->GetParentGroup() - && SCH_GROUP::IsGroupableType( schItem->Type() ) ) - { + if( enteredGroup && schItem->IsGroupableType() && !schItem->GetParentGroup() ) selTool->GetEnteredGroup()->AddItem( schItem ); - } - updateConnectivityFlag(); + updateConnectivityFlag( schItem ); if( !( aCommitFlags & SKIP_UNDO ) ) undoList.PushItem( ITEM_PICKER( screen, schItem, UNDO_REDO::NEWITEM ) ); @@ -282,12 +289,6 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) view->Add( schItem ); } - if( schItem->Type() == SCH_GROUP_T ) - { - wxASSERT_MSG( !addedGroup, "Multiple groups in a single commit. This is not supported." ); - addedGroup = static_cast( schItem ); - } - if( frame ) frame->UpdateItem( schItem, true, true ); @@ -301,10 +302,15 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) case CHT_REMOVE: { - updateConnectivityFlag(); + updateConnectivityFlag( schItem ); if( !( aCommitFlags & SKIP_UNDO ) ) - undoList.PushItem( ITEM_PICKER( screen, schItem, UNDO_REDO::DELETED ) ); + { + ITEM_PICKER itemWrapper( screen, schItem, UNDO_REDO::DELETED ); + itemWrapper.SetLink( ent.m_copy ); + ent.m_copy = nullptr; // We've transferred ownership to the undo list + undoList.PushItem( itemWrapper ); + } if( schItem->IsSelected() ) { @@ -320,6 +326,9 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) break; } + if( EDA_GROUP* group = schItem->GetParentGroup() ) + group->RemoveItem( schItem ); + if( !( changeFlags & CHT_DONE ) ) { screen->Remove( schItem ); @@ -331,104 +340,61 @@ void SCH_COMMIT::pushSchEdit( const wxString& aMessage, int aCommitFlags ) if( frame ) frame->UpdateItem( schItem, true, true ); - bulkRemovedItems.push_back( schItem ); - if( schItem->Type() == SCH_SHEET_T ) refreshHierarchy = true; + bulkRemovedItems.push_back( schItem ); break; } case CHT_MODIFY: { + const SCH_ITEM* itemCopy = static_cast( ent.m_copy ); + SCH_SHEET_PATH currentSheet; + + if( frame ) + currentSheet = frame->GetCurrentSheet(); + + if( itemCopy->HasConnectivityChanges( schItem, ¤tSheet ) + || ( itemCopy->Type() == SCH_RULE_AREA_T ) ) + { + updateConnectivityFlag( schItem ); + } + if( !( aCommitFlags & SKIP_UNDO ) ) { ITEM_PICKER itemWrapper( screen, schItem, UNDO_REDO::CHANGED ); - wxASSERT( ent.m_copy ); itemWrapper.SetLink( ent.m_copy ); - - const SCH_ITEM* itemCopy = static_cast( ent.m_copy ); - - wxCHECK2( itemCopy, continue ); - - SCH_SHEET_PATH currentSheet; - - if( frame ) - currentSheet = frame->GetCurrentSheet(); - - if( itemCopy->HasConnectivityChanges( schItem, ¤tSheet ) - || ( itemCopy->Type() == SCH_RULE_AREA_T ) ) - { - updateConnectivityFlag(); - } - - - if( schItem->Type() == SCH_SHEET_T ) - { - const SCH_SHEET* modifiedSheet = static_cast( schItem ); - const SCH_SHEET* originalSheet = static_cast( itemCopy ); - wxCHECK2( modifiedSheet && originalSheet, continue ); - - if( originalSheet->HasPageNumberChanges( *modifiedSheet ) ) - refreshHierarchy = true; - } - - undoList.PushItem( itemWrapper ); ent.m_copy = nullptr; // We've transferred ownership to the undo list + undoList.PushItem( itemWrapper ); } - if( schItem->Type() == SCH_GROUP_T ) + if( schItem->Type() == SCH_SHEET_T ) { - wxASSERT_MSG( !addedGroup, "Multiple groups in a single commit. This is not supported." ); - addedGroup = static_cast( schItem ); + const SCH_SHEET* modifiedSheet = static_cast( schItem ); + const SCH_SHEET* originalSheet = static_cast( itemCopy ); + wxCHECK2( modifiedSheet && originalSheet, continue ); + + if( originalSheet->HasPageNumberChanges( *modifiedSheet ) ) + refreshHierarchy = true; } if( frame ) frame->UpdateItem( schItem, false, true ); itemsChanged.push_back( schItem ); - - if( ent.m_copy ) - { - // if no undo entry is needed, the copy would create a memory leak - delete ent.m_copy; - ent.m_copy = nullptr; - } - break; } - case CHT_UNGROUP: - if( EDA_GROUP* group = schItem->GetParentGroup() ) - { - if( !( aCommitFlags & SKIP_UNDO ) ) - { - ITEM_PICKER itemWrapper( nullptr, schItem, UNDO_REDO::UNGROUP ); - itemWrapper.SetGroupId( group->AsEdaItem()->m_Uuid ); - undoList.PushItem( itemWrapper ); - } - - group->RemoveItem( schItem ); - } - - break; - - case CHT_GROUP: - if( addedGroup ) - { - addedGroup->AddItem( schItem ); - - if( !( aCommitFlags & SKIP_UNDO ) ) - undoList.PushItem( ITEM_PICKER( nullptr, schItem, UNDO_REDO::REGROUP ) ); - } - - break; - default: wxASSERT( false ); break; } + // Delete any copies we still have ownership of + delete ent.m_copy; + ent.m_copy = nullptr; + // Clear all flags but SELECTED and others used to move and rotate commands, // after edition (selected items must keep their selection flag). const int selected_mask = ( SELECTED | STARTPOINT | ENDPOINT ); diff --git a/eeschema/sch_commit.h b/eeschema/sch_commit.h index edf66200b1..daafb19bba 100644 --- a/eeschema/sch_commit.h +++ b/eeschema/sch_commit.h @@ -55,8 +55,8 @@ public: int aCommitFlags = 0 ) override; virtual void Revert() override; - COMMIT& Stage( EDA_ITEM *aItem, CHANGE_TYPE aChangeType, - BASE_SCREEN *aScreen = nullptr ) override; + COMMIT& Stage( EDA_ITEM *aItem, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen = nullptr, + RECURSE_MODE aRecurse = RECURSE_MODE::NO_RECURSE ) override; COMMIT& Stage( std::vector &container, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen = nullptr ) override; COMMIT& Stage( const PICKED_ITEMS_LIST &aItems, UNDO_REDO aModFlag = UNDO_REDO::UNSPECIFIED, diff --git a/eeschema/sch_design_block_utils.cpp b/eeschema/sch_design_block_utils.cpp index 7b2ce75a7f..1159c3c26d 100644 --- a/eeschema/sch_design_block_utils.cpp +++ b/eeschema/sch_design_block_utils.cpp @@ -392,10 +392,14 @@ bool SCH_EDIT_FRAME::SaveSelectionToDesignBlock( const LIB_ID& aLibId ) { group = static_cast( item ); - selection.Remove( item ); + selection.Remove( group ); // Don't recurse; if we have a group of groups the user probably intends the inner groups to be saved - group->RunOnChildren( [&]( EDA_ITEM* aItem ) { selection.Add( aItem ); }, RECURSE_MODE::NO_RECURSE ); + group->RunOnChildren( [&]( EDA_ITEM* aItem ) + { + selection.Add( aItem ); + }, + RECURSE_MODE::NO_RECURSE ); } } @@ -420,15 +424,15 @@ bool SCH_EDIT_FRAME::SaveSelectionToDesignBlock( const LIB_ID& aLibId ) SCH_SCREEN* tempScreen = new SCH_SCREEN( m_schematic ); auto cloneAndAdd = - [&] ( EDA_ITEM* aItem ) - { - if( !aItem->IsSCH_ITEM() ) - return static_cast( nullptr ); + [&] ( EDA_ITEM* aItem ) -> SCH_ITEM* + { + if( !aItem->IsSCH_ITEM() ) + return nullptr; - SCH_ITEM* copy = static_cast( aItem->Clone() ); - tempScreen->Append( static_cast( copy ) ); - return copy; - }; + SCH_ITEM* copy = static_cast( aItem->Clone() ); + tempScreen->Append( static_cast( copy ) ); + return copy; + }; // Copy the selected items to the temporary board for( EDA_ITEM* item : selection ) diff --git a/eeschema/sch_edit_frame.cpp b/eeschema/sch_edit_frame.cpp index e54fa2040e..7fc2890b52 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -856,7 +856,7 @@ void SCH_EDIT_FRAME::AddCopyForRepeatItem( const SCH_ITEM* aItem ) if( aItem ) { - std::unique_ptr repeatItem( static_cast( aItem->Duplicate() ) ); + std::unique_ptr repeatItem( static_cast( aItem->Duplicate( IGNORE_PARENT_GROUP ) ) ); // Clone() preserves the flags & parent, we want 'em cleared. repeatItem->ClearFlags(); @@ -867,9 +867,9 @@ void SCH_EDIT_FRAME::AddCopyForRepeatItem( const SCH_ITEM* aItem ) } -EDA_ITEM* SCH_EDIT_FRAME::GetItem( const KIID& aId ) const +EDA_ITEM* SCH_EDIT_FRAME::ResolveItem( const KIID& aId, bool aAllowNullptrReturn ) const { - return Schematic().GetItem( aId ); + return Schematic().ResolveItem( aId, nullptr, aAllowNullptrReturn ); } @@ -2319,8 +2319,7 @@ void SCH_EDIT_FRAME::FocusOnItem( EDA_ITEM* aItem ) static KIID lastBrightenedItemID( niluuid ); - SCH_SHEET_PATH dummy; - SCH_ITEM* lastItem = Schematic().GetItem( lastBrightenedItemID, &dummy ); + SCH_ITEM* lastItem = Schematic().ResolveItem( lastBrightenedItemID, nullptr, true ); if( lastItem && lastItem != aItem ) { @@ -2377,7 +2376,7 @@ void SCH_EDIT_FRAME::SaveSymbolToSchematic( const LIB_SYMBOL& aSymbol, { SCH_SHEET_PATH principalPath; SCH_SHEET_LIST sheets = Schematic().Hierarchy(); - SCH_ITEM* item = sheets.GetItem( aSchematicSymbolUUID, &principalPath ); + SCH_ITEM* item = sheets.ResolveItem( aSchematicSymbolUUID, &principalPath, true ); SCH_SYMBOL* principalSymbol = dynamic_cast( item ); SCH_COMMIT commit( m_toolManager ); diff --git a/eeschema/sch_edit_frame.h b/eeschema/sch_edit_frame.h index b7e4c53be8..79760b6c3d 100644 --- a/eeschema/sch_edit_frame.h +++ b/eeschema/sch_edit_frame.h @@ -743,7 +743,7 @@ public: m_items_to_repeat.clear(); } - EDA_ITEM* GetItem( const KIID& aId ) const override; + EDA_ITEM* ResolveItem( const KIID& aId, bool aAllowNullptrReturn = false ) const override; /** * Perform an undo of the last edit **without** logging a corresponding redo. diff --git a/eeschema/sch_field.cpp b/eeschema/sch_field.cpp index 1997e708a4..3a71c75e2b 100644 --- a/eeschema/sch_field.cpp +++ b/eeschema/sch_field.cpp @@ -407,8 +407,6 @@ void SCH_FIELD::swapData( SCH_ITEM* aItem ) { wxCHECK_RET( aItem && aItem->Type() == SCH_FIELD_T, wxT( "Cannot swap with invalid item." ) ); - SCH_ITEM::SwapFlags( aItem ); - SCH_FIELD* item = static_cast( aItem ); std::swap( m_layer, item->m_layer ); diff --git a/eeschema/sch_group.cpp b/eeschema/sch_group.cpp index a902e8ca4c..ecebe95b7c 100644 --- a/eeschema/sch_group.cpp +++ b/eeschema/sch_group.cpp @@ -47,91 +47,14 @@ SCH_GROUP::SCH_GROUP( SCH_SCREEN* aParent ) : SCH_ITEM( aParent, SCH_GROUP_T ) { } -bool SCH_GROUP::IsGroupableType( KICAD_T aType ) -{ - switch( aType ) - { - case SCH_SYMBOL_T: - case SCH_PIN_T: - case SCH_SHAPE_T: - case SCH_BITMAP_T: - case SCH_FIELD_T: - case SCH_TEXT_T: - case SCH_TEXTBOX_T: - case SCH_TABLE_T: - case SCH_GROUP_T: - case SCH_LINE_T: - case SCH_JUNCTION_T: - case SCH_NO_CONNECT_T: - case SCH_BUS_WIRE_ENTRY_T: - case SCH_BUS_BUS_ENTRY_T: - case SCH_LABEL_T: - case SCH_GLOBAL_LABEL_T: - case SCH_HIER_LABEL_T: - case SCH_RULE_AREA_T: - case SCH_DIRECTIVE_LABEL_T: - case SCH_SHEET_PIN_T: - case SCH_SHEET_T: - return true; - default: - return false; - } -} - - -bool SCH_GROUP::AddItem( EDA_ITEM* aItem ) -{ - wxCHECK_MSG( aItem, false, wxT( "Nullptr added to group." ) ); - - wxCHECK_MSG( IsGroupableType( aItem->Type() ), false, - wxT( "Invalid item type added to group: " ) + aItem->GetTypeDesc() ); - - // Items can only be in one group at a time - if( aItem->GetParentGroup() ) - aItem->GetParentGroup()->RemoveItem( aItem ); - - m_items.insert( aItem ); - aItem->SetParentGroup( this ); - return true; -} - - -bool SCH_GROUP::RemoveItem( EDA_ITEM* aItem ) -{ - wxCHECK_MSG( aItem, false, wxT( "Nullptr removed from group." ) ); - - // Only clear the item's group field if it was inside this group - if( m_items.erase( aItem ) == 1 ) - { - aItem->SetParentGroup( nullptr ); - return true; - } - - return false; -} - - -void SCH_GROUP::RemoveAll() -{ - for( EDA_ITEM* item : m_items ) - item->SetParentGroup( nullptr ); - - m_items.clear(); -} - - std::unordered_set SCH_GROUP::GetSchItems() const { std::unordered_set items; for( EDA_ITEM* item : m_items ) { - SCH_ITEM* sch_item = dynamic_cast( item ); - - if( sch_item ) - { - items.insert( sch_item ); - } + if( item->IsSCH_ITEM() ) + items.insert( static_cast( item ) ); } return items; @@ -230,17 +153,17 @@ SCH_GROUP* SCH_GROUP::DeepClone() const } -SCH_GROUP* SCH_GROUP::DeepDuplicate() const +SCH_GROUP* SCH_GROUP::DeepDuplicate( bool addToParentGroup, SCH_COMMIT* aCommit ) const { - SCH_GROUP* newGroup = static_cast( Duplicate() ); + SCH_GROUP* newGroup = static_cast( Duplicate( addToParentGroup, aCommit ) ); newGroup->m_items.clear(); for( EDA_ITEM* member : m_items ) { if( member->Type() == SCH_GROUP_T ) - newGroup->AddItem( static_cast( member )->DeepDuplicate() ); + newGroup->AddItem( static_cast( member )->DeepDuplicate( IGNORE_PARENT_GROUP ) ); else - newGroup->AddItem( static_cast( member )->Duplicate() ); + newGroup->AddItem( static_cast( member )->Duplicate( IGNORE_PARENT_GROUP ) ); } return newGroup; @@ -252,26 +175,9 @@ void SCH_GROUP::swapData( SCH_ITEM* aImage ) assert( aImage->Type() == SCH_GROUP_T ); SCH_GROUP* image = static_cast( aImage ); - std::swap( *this, *image ); - - // A group doesn't own its children (they're owned by the schematic), so undo doesn't do a - // deep clone when making an image. However, it's still safest to update the parentGroup - // pointers of the group's children -- we just have to be careful to do it in the right - // order in case any of the children are shared (ie: image first, "this" second so that - // any shared children end up with "this"). - image->RunOnChildren( - [&]( SCH_ITEM* child ) - { - child->SetParentGroup( image ); - }, - RECURSE_MODE::NO_RECURSE ); - - RunOnChildren( - [&]( SCH_ITEM* child ) - { - child->SetParentGroup( this ); - }, - RECURSE_MODE::NO_RECURSE ); + std::swap( m_items, image->m_items ); + std::swap( m_name, image->m_name ); + std::swap( m_designBlockLibId, image->m_designBlockLibId ); } diff --git a/eeschema/sch_group.h b/eeschema/sch_group.h index 2d8bba11e2..376e965d3d 100644 --- a/eeschema/sch_group.h +++ b/eeschema/sch_group.h @@ -63,22 +63,6 @@ public: wxString GetClass() const override { return wxT( "SCH_GROUP" ); } - /** - * Add item to group. Does not take ownership of item. - * - * @return true if item was added (false if item belongs to a different group). - */ - bool AddItem( EDA_ITEM* aItem ) override; - - /** - * Remove item from group. - * - * @return true if item was removed (false if item was not in the group). - */ - bool RemoveItem( EDA_ITEM* aItem ) override; - - void RemoveAll() override; - std::unordered_set GetSchItems() const; /* @@ -111,14 +95,17 @@ public: EDA_ITEM* Clone() const override; /* - * Clone() this and all descendants + * Clone this and all descendants */ - SCH_GROUP* DeepClone() const override; + SCH_GROUP* DeepClone() const; /* - * Duplicate() this and all descendants + * Duplicate this and all descendants + * + * @param addToParentGroup if the original is part of a group then the new member will also + * be added to said group */ - SCH_GROUP* DeepDuplicate() const override; + SCH_GROUP* DeepDuplicate( bool addToParentGroup, SCH_COMMIT* aCommit = nullptr ) const; /// @copydoc EDA_ITEM::HitTest bool HitTest( const VECTOR2I& aPosition, int aAccuracy = 0 ) const override; @@ -166,13 +153,6 @@ public: ///< @copydoc SCH_ITEM::RunOnChildren void RunOnChildren( const std::function& aFunction, RECURSE_MODE aMode ) override; - /** - * Check if the proposed type can be added to a group - * @param aType KICAD_T type to check - * @return true if the type can belong to a group, false otherwise - */ - static bool IsGroupableType( KICAD_T aType ); - /// @copydoc SCH_ITEM::swapData void swapData( SCH_ITEM* aImage ) override; }; diff --git a/eeschema/sch_io/cadstar/cadstar_sch_archive_loader.cpp b/eeschema/sch_io/cadstar/cadstar_sch_archive_loader.cpp index 137467247c..c6e8b2c99e 100644 --- a/eeschema/sch_io/cadstar/cadstar_sch_archive_loader.cpp +++ b/eeschema/sch_io/cadstar/cadstar_sch_archive_loader.cpp @@ -1728,7 +1728,7 @@ const LIB_SYMBOL* CADSTAR_SCH_ARCHIVE_LOADER::loadSymdef( const SYMDEF_ID& aSymd RotatePoint( linePos, libtext->GetTextPos(), -libtext->GetTextAngle() ); - SCH_TEXT* textLine = static_cast( libtext->Duplicate() ); + SCH_TEXT* textLine = static_cast( libtext->Duplicate( IGNORE_PARENT_GROUP ) ); textLine->SetText( strings[ii] ); textLine->SetHorizJustify( GR_TEXT_H_ALIGN_LEFT ); textLine->SetVertJustify( GR_TEXT_V_ALIGN_BOTTOM ); @@ -2655,8 +2655,8 @@ void CADSTAR_SCH_ARCHIVE_LOADER::loadItemOntoKiCadSheet( const LAYER_ID& aCadsta for( std::pair sheetPair : Sheets.SheetNames ) { LAYER_ID sheetID = sheetPair.first; - duplicateItem = aItem->Duplicate(); - m_sheetMap.at( sheetID )->GetScreen()->Append( aItem->Duplicate() ); + duplicateItem = aItem->Duplicate( IGNORE_PARENT_GROUP ); + m_sheetMap.at( sheetID )->GetScreen()->Append( aItem->Duplicate( IGNORE_PARENT_GROUP ) ); } //Get rid of the extra copy: diff --git a/eeschema/sch_io/eagle/sch_io_eagle.cpp b/eeschema/sch_io/eagle/sch_io_eagle.cpp index 67a602051d..fe929666c2 100644 --- a/eeschema/sch_io/eagle/sch_io_eagle.cpp +++ b/eeschema/sch_io/eagle/sch_io_eagle.cpp @@ -785,7 +785,7 @@ void SCH_IO_EAGLE::loadSchematic( const ESCHEMATIC& aSchematic ) // Instantiate the missing symbol unit int unit = unitEntry.first; const wxString reference = origSymbol->GetField( FIELD_T::REFERENCE )->GetText(); - std::unique_ptr symbol( (SCH_SYMBOL*) origSymbol->Duplicate() ); + std::unique_ptr symbol( (SCH_SYMBOL*) origSymbol->Duplicate( IGNORE_PARENT_GROUP ) ); symbol->SetUnitSelection( &sheetpath, unit ); symbol->SetUnit( unit ); diff --git a/eeschema/sch_io/http_lib/sch_io_http_lib.cpp b/eeschema/sch_io/http_lib/sch_io_http_lib.cpp index d9933b5332..063ee14851 100644 --- a/eeschema/sch_io/http_lib/sch_io_http_lib.cpp +++ b/eeschema/sch_io/http_lib/sch_io_http_lib.cpp @@ -397,16 +397,14 @@ LIB_SYMBOL* SCH_IO_HTTP_LIB::loadSymbolFromPart( const wxString& aSymbo } else if( !symbolId.IsValid() ) { - wxLogTrace( traceHTTPLib, - wxT( "loadSymbolFromPart: source symbol id '%s' is invalid, " - "will create empty symbol" ), + wxLogTrace( traceHTTPLib, wxT( "loadSymbolFromPart: source symbol id '%s' is invalid, " + "will create empty symbol" ), symbolIdStr ); } else { - wxLogTrace( traceHTTPLib, - wxT( "loadSymbolFromPart: source symbol '%s' not found, " - "will create empty symbol" ), + wxLogTrace( traceHTTPLib, wxT( "loadSymbolFromPart: source symbol '%s' not found, " + "will create empty symbol" ), symbolIdStr ); } } diff --git a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_parser.cpp b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_parser.cpp index 9c00b04812..d18720eb21 100644 --- a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_parser.cpp +++ b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr_parser.cpp @@ -3088,9 +3088,9 @@ SCH_SYMBOL* SCH_IO_KICAD_SEXPR_PARSER::parseSchematicSymbol() { if( static_cast( name.size() ) > bad_pos ) { - wxString msg = wxString::Format( - _( "Symbol %s contains invalid character '%c'" ), name, - name[bad_pos] ); + wxString msg = wxString::Format( _( "Symbol %s contains invalid character '%c'" ), + name, + name[bad_pos] ); THROW_PARSE_ERROR( msg, CurSource(), CurLine(), CurLineNumber(), CurOffset() ); } @@ -4865,6 +4865,7 @@ void SCH_IO_KICAD_SEXPR_PARSER::parseGroup() if( !IsSymbol( token ) && token != T_NUMBER ) Expecting( "symbol|number" ); + LIB_ID libId; wxString name = FromUTF8(); // Some symbol LIB_IDs have the '/' character escaped which can break // symbol links. The '/' character is no longer an illegal LIB_ID character so @@ -4952,15 +4953,8 @@ void SCH_IO_KICAD_SEXPR_PARSER::resolveGroups( SCH_SCREEN* aParent ) { for( const KIID& aUuid : groupInfo.memberUuids ) { - SCH_ITEM* gItem = getItem( aUuid ); - - if( !gItem || gItem->Type() == NOT_USED ) - { - // This is the deleted item singleton, which means we didn't find the uuid. - continue; - } - - group->AddItem( gItem ); + if( SCH_ITEM* gItem = getItem( aUuid ) ) + group->AddItem( gItem ); } } } diff --git a/eeschema/sch_item.cpp b/eeschema/sch_item.cpp index 5e79e10939..80ec69eaf0 100644 --- a/eeschema/sch_item.cpp +++ b/eeschema/sch_item.cpp @@ -129,16 +129,45 @@ SCH_ITEM::~SCH_ITEM() } -SCH_ITEM* SCH_ITEM::Duplicate( bool doClone ) const +bool SCH_ITEM::IsGroupableType() const +{ + switch( Type() ) + { + case SCH_SYMBOL_T: + case SCH_PIN_T: + case SCH_SHAPE_T: + case SCH_BITMAP_T: + case SCH_FIELD_T: + case SCH_TEXT_T: + case SCH_TEXTBOX_T: + case SCH_TABLE_T: + case SCH_GROUP_T: + case SCH_LINE_T: + case SCH_JUNCTION_T: + case SCH_NO_CONNECT_T: + case SCH_BUS_WIRE_ENTRY_T: + case SCH_BUS_BUS_ENTRY_T: + case SCH_LABEL_T: + case SCH_GLOBAL_LABEL_T: + case SCH_HIER_LABEL_T: + case SCH_RULE_AREA_T: + case SCH_DIRECTIVE_LABEL_T: + case SCH_SHEET_PIN_T: + case SCH_SHEET_T: + return true; + default: + return false; + } +} + + +SCH_ITEM* SCH_ITEM::Duplicate( bool addToParentGroup, SCH_COMMIT* aCommit, bool doClone ) const { SCH_ITEM* newItem = (SCH_ITEM*) Clone(); if( !doClone ) const_cast( newItem->m_Uuid ) = KIID(); - if( newItem->GetParentGroup() ) - newItem->GetParentGroup()->AddItem( newItem ); - newItem->ClearFlags( SELECTED | BRIGHTENED ); newItem->RunOnChildren( @@ -148,6 +177,17 @@ SCH_ITEM* SCH_ITEM::Duplicate( bool doClone ) const }, RECURSE_MODE::NO_RECURSE ); + if( addToParentGroup ) + { + wxCHECK_MSG( aCommit, newItem, "Must supply a commit to update parent group" ); + + if( newItem->GetParentGroup() ) + { + aCommit->Modify( newItem->GetParentGroup()->AsEdaItem() ); + newItem->GetParentGroup()->AddItem( newItem ); + } + } + return newItem; } @@ -176,33 +216,17 @@ void SCH_ITEM::SetBodyStyleProp( int aBodyStyle ) SCHEMATIC* SCH_ITEM::Schematic() const { - EDA_ITEM* parent = GetParent(); - - while( parent ) - { - if( parent->Type() == SCHEMATIC_T ) - return static_cast( parent ); - else - parent = parent->GetParent(); - } - - return nullptr; + return static_cast( findParent( SCHEMATIC_T ) ); } const SYMBOL* SCH_ITEM::GetParentSymbol() const { - const EDA_ITEM* parent = GetParent(); + if( SYMBOL* sch_symbol = static_cast( findParent( SCH_SYMBOL_T ) ) ) + return sch_symbol; - while( parent ) - { - if( parent->Type() == SCH_SYMBOL_T ) - return static_cast( parent ); - else if( parent->Type() == LIB_SYMBOL_T ) - return static_cast( parent ); - else - parent = parent->GetParent(); - } + if( SYMBOL* lib_symbol = static_cast( findParent( LIB_SYMBOL_T ) ) ) + return lib_symbol; return nullptr; } @@ -210,17 +234,11 @@ const SYMBOL* SCH_ITEM::GetParentSymbol() const SYMBOL* SCH_ITEM::GetParentSymbol() { - EDA_ITEM* parent = GetParent(); + if( SYMBOL* sch_symbol = static_cast( findParent( SCH_SYMBOL_T ) ) ) + return sch_symbol; - while( parent ) - { - if( parent->Type() == SCH_SYMBOL_T ) - return static_cast( parent ); - else if( parent->Type() == LIB_SYMBOL_T ) - return static_cast( parent ); - else - parent = parent->GetParent(); - } + if( SYMBOL* lib_symbol = static_cast( findParent( LIB_SYMBOL_T ) ) ) + return lib_symbol; return nullptr; } @@ -381,16 +399,13 @@ void SCH_ITEM::SwapItemData( SCH_ITEM* aImage ) if( aImage == nullptr ) return; - EDA_ITEM* parent = GetParent(); - EDA_GROUP* group = GetParentGroup(); + EDA_ITEM* parent = GetParent(); - SetParentGroup( nullptr ); - aImage->SetParentGroup( nullptr ); + SwapFlags( aImage ); + std::swap( m_group, aImage->m_group ); swapData( aImage ); - // Restore pointers to be sure they are not broken SetParent( parent ); - SetParentGroup( group ); } diff --git a/eeschema/sch_item.h b/eeschema/sch_item.h index 185bc4a035..43375dde3f 100644 --- a/eeschema/sch_item.h +++ b/eeschema/sch_item.h @@ -42,6 +42,7 @@ class CONNECTION_GRAPH; class SCH_CONNECTION; class SCH_SHEET_PATH; class SCHEMATIC; +class SCH_COMMIT; class SYMBOL; class LINE_READER; class SCH_EDIT_FRAME; @@ -202,6 +203,8 @@ public: return false; } + bool IsGroupableType() const; + /** * Swap data between \a aItem and \a aImage. * @@ -224,10 +227,13 @@ public: * * The new object is not put in draw list (not linked). * - * @param doClone (default = false) indicates unique values (such as timestamp and - * sheet name) should be duplicated. Use only for undo/redo operations. + * @param addToParentGroup Indicates whether or not the new item is added to the group + * (if any) containing the old item. If true, aCommit must be + * provided. + * @param aDoClone (default = false) indicates unique values (such as timestamp and + * sheet name) should be duplicated. Use only for undo/redo operations. */ - SCH_ITEM* Duplicate( bool doClone = false ) const; + SCH_ITEM* Duplicate( bool addToParentGroup, SCH_COMMIT* aCommit = nullptr, bool doClone = false ) const; static wxString GetUnitDescription( int aUnit ); static wxString GetBodyStyleDescription( int aBodyStyle ); diff --git a/eeschema/sch_junction.cpp b/eeschema/sch_junction.cpp index 56e741fd57..a94a913add 100644 --- a/eeschema/sch_junction.cpp +++ b/eeschema/sch_junction.cpp @@ -60,8 +60,6 @@ EDA_ITEM* SCH_JUNCTION::Clone() const void SCH_JUNCTION::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( ( aItem != nullptr ) && ( aItem->Type() == SCH_JUNCTION_T ), wxT( "Cannot swap junction data with invalid item." ) ); diff --git a/eeschema/sch_line.cpp b/eeschema/sch_line.cpp index d30a1fcdb8..3937ec120d 100644 --- a/eeschema/sch_line.cpp +++ b/eeschema/sch_line.cpp @@ -543,9 +543,9 @@ SCH_LINE* SCH_LINE::MergeOverlap( SCH_SCREEN* aScreen, SCH_LINE* aLine, bool aCh } -SCH_LINE* SCH_LINE::BreakAt( const VECTOR2I& aPoint ) +SCH_LINE* SCH_LINE::BreakAt( SCH_COMMIT* aCommit, const VECTOR2I& aPoint ) { - SCH_LINE* newSegment = static_cast( Duplicate() ); + SCH_LINE* newSegment = static_cast( Duplicate( aCommit ) ); newSegment->SetStartPoint( aPoint ); newSegment->SetConnectivityDirty( true ); @@ -804,8 +804,6 @@ bool SCH_LINE::HitTest( const BOX2I& aRect, bool aContained, int aAccuracy ) con void SCH_LINE::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - SCH_LINE* item = (SCH_LINE*) aItem; std::swap( m_layer, item->m_layer ); diff --git a/eeschema/sch_line.h b/eeschema/sch_line.h index 7efae8f7a1..fe5c910a4d 100644 --- a/eeschema/sch_line.h +++ b/eeschema/sch_line.h @@ -256,7 +256,7 @@ public: * @param aPoint Point at which to break the segment * @return The newly created segment. */ - SCH_LINE* BreakAt( const VECTOR2I& aPoint ); + SCH_LINE* BreakAt( SCH_COMMIT* aCommit, const VECTOR2I& aPoint ); bool IsParallel( const SCH_LINE* aLine ) const; diff --git a/eeschema/sch_marker.cpp b/eeschema/sch_marker.cpp index 5dce094ec8..ba89122597 100644 --- a/eeschema/sch_marker.cpp +++ b/eeschema/sch_marker.cpp @@ -70,8 +70,6 @@ EDA_ITEM* SCH_MARKER::Clone() const void SCH_MARKER::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - SCH_MARKER* item = static_cast( aItem ); std::swap( m_isLegacyMarker, item->m_isLegacyMarker ); @@ -84,6 +82,7 @@ void SCH_MARKER::swapData( SCH_ITEM* aItem ) std::swap( m_scalingFactor, item->m_scalingFactor ); std::swap( m_shapeBoundingBox, item->m_shapeBoundingBox ); + // TODO: isn't this going to swap all the stuff above a second time? std::swap( *((SCH_MARKER*) this), *((SCH_MARKER*) aItem ) ); } @@ -106,7 +105,7 @@ wxString SCH_MARKER::SerializeToString() const || m_rcItem->GetErrorCode() == ERCE_GENERIC_ERROR || m_rcItem->GetErrorCode() == ERCE_UNRESOLVED_VARIABLE ) { - SCH_ITEM* sch_item = Schematic()->GetItem( erc->GetMainItemID() ); + SCH_ITEM* sch_item = Schematic()->ResolveItem( erc->GetMainItemID() ); SCH_ITEM* parent = static_cast( sch_item->GetParent() ); EDA_TEXT* text_item = dynamic_cast( sch_item ); @@ -163,7 +162,7 @@ SCH_MARKER* SCH_MARKER::DeserializeFromString( const SCH_SHEET_LIST& aSheetList, if( !props[4].IsEmpty() ) { KIID uuid = niluuid; - SCH_ITEM* parent = aSheetList.GetItem( KIID( props[3] ) ); + SCH_ITEM* parent = aSheetList.ResolveItem( KIID( props[3] ) ); // Check fields and pins for a match parent->RunOnChildren( @@ -386,10 +385,10 @@ void SCH_MARKER::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vectorGetMainItemID() != niluuid ) - mainItem = aFrame->GetItem( m_rcItem->GetMainItemID() ); + mainItem = aFrame->ResolveItem( m_rcItem->GetMainItemID() ); if( m_rcItem->GetAuxItemID() != niluuid ) - auxItem = aFrame->GetItem( m_rcItem->GetAuxItemID() ); + auxItem = aFrame->ResolveItem( m_rcItem->GetAuxItemID() ); if( mainItem ) mainText = mainItem->GetItemDescription( aFrame, true ); diff --git a/eeschema/sch_no_connect.cpp b/eeschema/sch_no_connect.cpp index ba748a7974..8921063e34 100644 --- a/eeschema/sch_no_connect.cpp +++ b/eeschema/sch_no_connect.cpp @@ -57,8 +57,6 @@ EDA_ITEM* SCH_NO_CONNECT::Clone() const void SCH_NO_CONNECT::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( ( aItem != nullptr ) && ( aItem->Type() == SCH_NO_CONNECT_T ), wxT( "Cannot swap no connect data with invalid item." ) ); diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index 93f60ae21d..d106d122ce 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -306,13 +306,6 @@ void SCH_SCREEN::FreeDrawList() std::copy_if( m_rtree.begin(), m_rtree.end(), std::back_inserter( delete_list ), []( SCH_ITEM* aItem ) { - // Untangle group parents before doing any deleting - if( aItem->Type() == SCH_GROUP_T ) - { - for( EDA_ITEM* item : static_cast(aItem)->GetItems() ) - item->SetParentGroup( nullptr ); - } - return ( aItem->Type() != SCH_SHEET_PIN_T && aItem->Type() != SCH_FIELD_T ); } ); diff --git a/eeschema/sch_shape.cpp b/eeschema/sch_shape.cpp index 5ada2e9af6..ba3a492f95 100644 --- a/eeschema/sch_shape.cpp +++ b/eeschema/sch_shape.cpp @@ -59,8 +59,6 @@ int SCH_SHAPE::GetArcToSegMaxErrorIU( bool aHighDefinition ) const void SCH_SHAPE::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - SCH_SHAPE* shape = static_cast( aItem ); EDA_SHAPE::SwapShape( shape ); diff --git a/eeschema/sch_sheet.cpp b/eeschema/sch_sheet.cpp index bf0bc76cdd..516d4ed877 100644 --- a/eeschema/sch_sheet.cpp +++ b/eeschema/sch_sheet.cpp @@ -327,8 +327,6 @@ bool SCH_SHEET::ResolveTextVar( const SCH_SHEET_PATH* aPath, wxString* token, in void SCH_SHEET::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( aItem->Type() == SCH_SHEET_T, wxString::Format( wxT( "SCH_SHEET object cannot swap data with %s object." ), aItem->GetClass() ) ); diff --git a/eeschema/sch_sheet_path.cpp b/eeschema/sch_sheet_path.cpp index d3f063921e..b5903dc1c6 100644 --- a/eeschema/sch_sheet_path.cpp +++ b/eeschema/sch_sheet_path.cpp @@ -978,11 +978,11 @@ void SCH_SHEET_LIST::ClearModifyStatus() } -SCH_ITEM* SCH_SHEET_LIST::GetItem( const KIID& aID, SCH_SHEET_PATH* aPathOut ) const +SCH_ITEM* SCH_SHEET_LIST::ResolveItem( const KIID& aID, SCH_SHEET_PATH* aPathOut, bool aAllowNullptrReturn ) const { for( const SCH_SHEET_PATH& sheet : *this ) { - SCH_ITEM* item = sheet.GetItem( aID ); + SCH_ITEM* item = sheet.ResolveItem( aID ); if( item ) { @@ -994,11 +994,14 @@ SCH_ITEM* SCH_SHEET_LIST::GetItem( const KIID& aID, SCH_SHEET_PATH* aPathOut ) c } // Not found; weak reference has been deleted. - return DELETED_SHEET_ITEM::GetInstance(); + if( aAllowNullptrReturn ) + return nullptr; + else + return DELETED_SHEET_ITEM::GetInstance(); } -SCH_ITEM* SCH_SHEET_PATH::GetItem( const KIID& aID ) const +SCH_ITEM* SCH_SHEET_PATH::ResolveItem( const KIID& aID ) const { for( SCH_ITEM* aItem : LastScreen()->Items() ) { diff --git a/eeschema/sch_sheet_path.h b/eeschema/sch_sheet_path.h index 568727ffd9..da234f8835 100644 --- a/eeschema/sch_sheet_path.h +++ b/eeschema/sch_sheet_path.h @@ -270,7 +270,7 @@ public: /** * Fetch a SCH_ITEM by ID. */ - SCH_ITEM* GetItem( const KIID& aID ) const; + SCH_ITEM* ResolveItem( const KIID& aID ) const; /** * Return the path of time stamps which do not changes even when editing sheet parameters. @@ -487,7 +487,8 @@ public: * * Also returns the sheet the item was found on in \a aPathOut. */ - SCH_ITEM* GetItem( const KIID& aID, SCH_SHEET_PATH* aPathOut = nullptr ) const; + SCH_ITEM* ResolveItem( const KIID& aID, SCH_SHEET_PATH* aPathOut = nullptr, + bool aAllowNullptrReturn = false ) const; /** * Fill an item cache for temporary use when many items need to be fetched. diff --git a/eeschema/sch_symbol.cpp b/eeschema/sch_symbol.cpp index cb6dd4187b..7864a01028 100644 --- a/eeschema/sch_symbol.cpp +++ b/eeschema/sch_symbol.cpp @@ -1202,8 +1202,6 @@ std::vector SCH_SYMBOL::GetPins() const void SCH_SYMBOL::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( aItem != nullptr && aItem->Type() == SCH_SYMBOL_T, wxT( "Cannot swap data with invalid symbol." ) ); diff --git a/eeschema/sch_table.cpp b/eeschema/sch_table.cpp index 76f420b5bc..d249541f6e 100644 --- a/eeschema/sch_table.cpp +++ b/eeschema/sch_table.cpp @@ -78,8 +78,6 @@ SCH_TABLE::~SCH_TABLE() void SCH_TABLE::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - wxCHECK_RET( aItem != nullptr && aItem->Type() == SCH_TABLE_T, wxT( "Cannot swap data with invalid table." ) ); diff --git a/eeschema/sch_text.cpp b/eeschema/sch_text.cpp index fa574df0a5..0baa11aeb6 100644 --- a/eeschema/sch_text.cpp +++ b/eeschema/sch_text.cpp @@ -230,8 +230,6 @@ void SCH_TEXT::MirrorSpinStyle( bool aLeftRight ) void SCH_TEXT::swapData( SCH_ITEM* aItem ) { - SCH_ITEM::SwapFlags( aItem ); - SCH_TEXT* item = static_cast( aItem ); std::swap( m_layer, item->m_layer ); diff --git a/eeschema/schematic.cpp b/eeschema/schematic.cpp index c23d647250..936fd5f111 100644 --- a/eeschema/schematic.cpp +++ b/eeschema/schematic.cpp @@ -440,7 +440,7 @@ bool SCHEMATIC::ResolveCrossReference( wxString* token, int aDepth ) const KIID_PATH path( ref ); KIID uuid = path.back(); SCH_SHEET_PATH sheetPath; - SCH_ITEM* refItem = GetItem( KIID( uuid ), &sheetPath ); + SCH_ITEM* refItem = ResolveItem( KIID( uuid ), &sheetPath, true ); if( path.size() > 1 ) { @@ -602,7 +602,7 @@ wxString SCHEMATIC::ConvertKIIDsToRefs( const wxString& aSource ) const KIID_PATH path( ref ); KIID uuid = path.back(); SCH_SHEET_PATH sheetPath; - SCH_ITEM* refItem = GetItem( uuid, &sheetPath ); + SCH_ITEM* refItem = ResolveItem( uuid, &sheetPath, true ); if( path.size() > 1 ) { @@ -768,7 +768,7 @@ void SCHEMATIC::FixupJunctions() // Breakup wires for( SCH_LINE* wire : screen->GetBusesAndWires( point, true ) ) { - SCH_LINE* newSegment = wire->BreakAt( point ); + SCH_LINE* newSegment = wire->BreakAt( nullptr, point ); screen->Append( newSegment ); } } @@ -858,7 +858,7 @@ void SCHEMATIC::ResolveERCExclusionsPostUpdate() for( SCH_MARKER* marker : ResolveERCExclusions() ) { SCH_SHEET_PATH errorPath; - ignore_unused( sheetList.GetItem( marker->GetRCItem()->GetMainItemID(), &errorPath ) ); + ignore_unused( sheetList.ResolveItem( marker->GetRCItem()->GetMainItemID(), &errorPath ) ); if( errorPath.LastScreen() ) errorPath.LastScreen()->Append( marker ); diff --git a/eeschema/schematic.h b/eeschema/schematic.h index 5904e4d666..55bbd818fd 100644 --- a/eeschema/schematic.h +++ b/eeschema/schematic.h @@ -109,9 +109,10 @@ public: void RefreshHierarchy(); - SCH_ITEM* GetItem( const KIID& aID, SCH_SHEET_PATH* aPathOut = nullptr ) const + SCH_ITEM* ResolveItem( const KIID& aID, SCH_SHEET_PATH* aPathOut = nullptr, + bool aAllowNullptrReturn = false ) const { - return BuildUnorderedSheetList().GetItem( aID, aPathOut ); + return BuildUnorderedSheetList().ResolveItem( aID, aPathOut, aAllowNullptrReturn ); } SCH_SHEET& Root() const diff --git a/eeschema/schematic_undo_redo.cpp b/eeschema/schematic_undo_redo.cpp index cf1bc4c113..816e75caa8 100644 --- a/eeschema/schematic_undo_redo.cpp +++ b/eeschema/schematic_undo_redo.cpp @@ -143,7 +143,7 @@ void SCH_EDIT_FRAME::SaveCopyInUndoList( SCH_SCREEN* aScreen, SCH_ITEM* aItem, switch( aCommandType ) { case UNDO_REDO::CHANGED: /* Create a copy of item */ - itemWrapper.SetLink( aItem->Duplicate( true ) ); + itemWrapper.SetLink( aItem->Duplicate( IGNORE_PARENT_GROUP, nullptr, true ) ); commandToUndo->PushItem( itemWrapper ); break; @@ -246,7 +246,7 @@ void SCH_EDIT_FRAME::SaveCopyInUndoList( const PICKED_ITEMS_LIST& aItemsList, * If this link is not null, the copy is already done */ if( commandToUndo->GetPickedItemLink( ii ) == nullptr ) - commandToUndo->SetPickedItemLink( sch_item->Duplicate( true ), ii ); + commandToUndo->SetPickedItemLink( sch_item->Duplicate( IGNORE_PARENT_GROUP, nullptr, true ), ii ); wxASSERT( commandToUndo->GetPickedItemLink( ii ) ); break; @@ -255,8 +255,6 @@ void SCH_EDIT_FRAME::SaveCopyInUndoList( const PICKED_ITEMS_LIST& aItemsList, case UNDO_REDO::DELETED: case UNDO_REDO::PAGESETTINGS: case UNDO_REDO::REPEAT_ITEM: - case UNDO_REDO::REGROUP: - case UNDO_REDO::UNGROUP: break; default: @@ -390,26 +388,6 @@ void SCH_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) bulkAddedItems.emplace_back( schItem ); } - else if( status == UNDO_REDO::REGROUP ) /* grouped items are ungrouped */ - { - aList->SetPickedItemStatus( UNDO_REDO::UNGROUP, ii ); - - if( EDA_GROUP* group = eda_item->GetParentGroup() ) - { - aList->SetPickedItemGroupId( group->AsEdaItem()->m_Uuid, ii ); - - group->RemoveItem( eda_item ); - } - } - else if( status == UNDO_REDO::UNGROUP ) /* ungrouped items are re-added to their previuos groups */ - { - aList->SetPickedItemStatus( UNDO_REDO::REGROUP, ii ); - - EDA_GROUP* group = dynamic_cast( Schematic().GetItem( aList->GetPickedItemGroupId( ii ) ) ); - - if( group ) - group->AddItem( eda_item ); - } else if( status == UNDO_REDO::PAGESETTINGS ) { if( GetCurrentSheet() != undoSheet ) @@ -450,8 +428,6 @@ void SCH_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) wxCHECK2( itemCopy, continue ); - EDA_GROUP* parentGroup = schItem->GetParentGroup(); - if( schItem->HasConnectivityChanges( itemCopy, &GetCurrentSheet() ) ) propagateConnectivityDamage( schItem ); @@ -485,24 +461,8 @@ void SCH_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) } } - if( parentGroup ) - parentGroup->RemoveItem( schItem ); - schItem->SwapItemData( itemCopy ); - if( SCH_GROUP* group = dynamic_cast( schItem ) ) - { - group->RunOnChildren( [&]( SCH_ITEM* child ) - { - child->SetParentGroup( group ); - }, - RECURSE_MODE::NO_RECURSE ); - } - - if( parentGroup ) - parentGroup->AddItem( schItem ); - - bulkChangedItems.emplace_back( schItem ); // Special cases for items which have instance data @@ -544,6 +504,32 @@ void SCH_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) } } + // We have now swapped all the group parent and group member pointers. But it is a + // risky proposition to bet on the pointers being invariant, so validate them all. + for( int ii = 0; ii < (int) aList->GetCount(); ++ii ) + { + ITEM_PICKER& wrapper = aList->GetItemWrapper( ii ); + + SCH_ITEM* parentGroup = Schematic().ResolveItem( wrapper.GetGroupId(), nullptr, true ); + wrapper.GetItem()->SetParentGroup( dynamic_cast( parentGroup ) ); + + if( EDA_GROUP* group = dynamic_cast( wrapper.GetItem() ) ) + { + // Items list may contain dodgy pointers, so don't use RemoveAll() + group->GetItems().clear(); + + for( const KIID& member : wrapper.GetGroupMembers() ) + { + if( SCH_ITEM* memberItem = Schematic().ResolveItem( member, nullptr, true ) ) + group->AddItem( memberItem ); + } + } + + // And prepare for a redo by updating group info based on current image + if( EDA_ITEM* item = wrapper.GetLink() ) + wrapper.SetLink( item ); + } + GetCanvas()->GetView()->ClearHiddenFlags(); // Notify our listeners diff --git a/eeschema/sim/simulator_frame_ui.cpp b/eeschema/sim/simulator_frame_ui.cpp index b3eb8bad92..8338090c74 100644 --- a/eeschema/sim/simulator_frame_ui.cpp +++ b/eeschema/sim/simulator_frame_ui.cpp @@ -1653,7 +1653,7 @@ void SIMULATOR_FRAME_UI::AddTuner( const SCH_SHEET_PATH& aSheetPath, SCH_SYMBOL* void SIMULATOR_FRAME_UI::UpdateTunerValue( const SCH_SHEET_PATH& aSheetPath, const KIID& aSymbol, const wxString& aRef, const wxString& aValue ) { - SCH_ITEM* item = aSheetPath.GetItem( aSymbol ); + SCH_ITEM* item = aSheetPath.ResolveItem( aSymbol ); SCH_SYMBOL* symbol = dynamic_cast( item ); if( !symbol ) @@ -2156,7 +2156,7 @@ void SIMULATOR_FRAME_UI::applyTuners() SCH_SHEET_PATH sheetPath; wxString ref = tuner->GetSymbolRef(); KIID symbolId = tuner->GetSymbol( &sheetPath ); - SCH_ITEM* schItem = sheetPath.GetItem( symbolId ); + SCH_ITEM* schItem = sheetPath.ResolveItem( symbolId ); SCH_SYMBOL* symbol = dynamic_cast( schItem ); if( !symbol ) diff --git a/eeschema/symbol.cpp b/eeschema/symbol.cpp index 4b0e7bbd8c..7e5518ddcc 100644 --- a/eeschema/symbol.cpp +++ b/eeschema/symbol.cpp @@ -52,14 +52,3 @@ std::vector SYMBOL::ViewGetLayers() const LAYER_FIELDS, LAYER_DEVICE_BACKGROUND, LAYER_SHAPES_BACKGROUND, LAYER_NOTES_BACKGROUND, LAYER_SELECTION_SHADOWS }; } - - -SYMBOL::~SYMBOL() -{ - // Untangle group parents before doing any deleting - RunOnChildren( []( EDA_ITEM* item ) - { - if( item->Type() == SCH_GROUP_T) - item->SetParentGroup( nullptr ); - }, RECURSE_MODE::RECURSE ); -} diff --git a/eeschema/symbol.h b/eeschema/symbol.h index 45273b532b..43fcd399d3 100644 --- a/eeschema/symbol.h +++ b/eeschema/symbol.h @@ -104,7 +104,7 @@ public: return *this; }; - ~SYMBOL() override; + ~SYMBOL() override = default; virtual const LIB_ID& GetLibId() const = 0; virtual wxString GetDescription() const = 0; diff --git a/eeschema/symbol_editor/symbol_editor_undo_redo.cpp b/eeschema/symbol_editor/symbol_editor_undo_redo.cpp index ffbba04494..3ea20179a5 100644 --- a/eeschema/symbol_editor/symbol_editor_undo_redo.cpp +++ b/eeschema/symbol_editor/symbol_editor_undo_redo.cpp @@ -37,8 +37,6 @@ void SYMBOL_EDIT_FRAME::PushSymbolToUndoList( const wxString& aDescription, LIB_ if( !aSymbolCopy ) return; - auto* drawingTool = GetToolManager()->GetTool(); - PICKED_ITEMS_LIST* lastcmd = new PICKED_ITEMS_LIST(); // Clear current flags (which can be temporary set by a current edit command). @@ -47,7 +45,6 @@ void SYMBOL_EDIT_FRAME::PushSymbolToUndoList( const wxString& aDescription, LIB_ aSymbolCopy->SetFlags( UR_TRANSIENT ); ITEM_PICKER wrapper( GetScreen(), aSymbolCopy, aUndoType ); - wrapper.SetGroupId( drawingTool->GetLastPin() ); lastcmd->PushItem( wrapper ); lastcmd->SetDescription( aDescription ); PushCommandToUndoList( lastcmd ); @@ -91,7 +88,6 @@ void SYMBOL_EDIT_FRAME::GetSymbolFromRedoList() oldSymbol->SetFlags( UR_TRANSIENT ); ITEM_PICKER undoWrapper( GetScreen(), oldSymbol, undoRedoType ); - undoWrapper.SetGroupId( drawingTool->GetLastPin() ); undoCommand->SetDescription( description ); undoCommand->PushItem( undoWrapper ); PushCommandToUndoList( undoCommand ); @@ -147,7 +143,6 @@ void SYMBOL_EDIT_FRAME::GetSymbolFromUndoList() oldSymbol->SetFlags( UR_TRANSIENT ); ITEM_PICKER redoWrapper( GetScreen(), oldSymbol, undoRedoType ); - redoWrapper.SetGroupId( drawingTool->GetLastPin() ); redoCommand->PushItem( redoWrapper ); redoCommand->SetDescription( description ); PushCommandToRedoList( redoCommand ); diff --git a/eeschema/tools/sch_drawing_tools.cpp b/eeschema/tools/sch_drawing_tools.cpp index 3f3b0003c9..b30beb22f5 100644 --- a/eeschema/tools/sch_drawing_tools.cpp +++ b/eeschema/tools/sch_drawing_tools.cpp @@ -487,7 +487,7 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) // We are either stepping to the next unit or next symbol if( m_frame->eeconfig()->m_SymChooserPanel.keep_symbol || new_unit > 1 ) { - nextSymbol = static_cast( symbol->Duplicate() ); + nextSymbol = static_cast( symbol->Duplicate( IGNORE_PARENT_GROUP ) ); nextSymbol->SetUnit( new_unit ); nextSymbol->SetUnitSelection( new_unit ); @@ -757,7 +757,9 @@ int SCH_DRAWING_TOOLS::ImportSheet( const TOOL_EVENT& aEvent ) newItems.emplace_back( item ); } else + { item->ClearFlags( SKIP_STRUCT ); + } } if( placeAsGroup ) @@ -766,7 +768,9 @@ int SCH_DRAWING_TOOLS::ImportSheet( const TOOL_EVENT& aEvent ) selectionTool->AddItemToSel( group ); } else + { selectionTool->AddItemsToSel( &newItems, true ); + } cursorPos = grid.Align( controls->GetMousePosition(), grid.GetSelectionGrid( selectionTool->GetSelection() ) ); diff --git a/eeschema/tools/sch_edit_tool.cpp b/eeschema/tools/sch_edit_tool.cpp index a42e352d4c..1fa875fd39 100644 --- a/eeschema/tools/sch_edit_tool.cpp +++ b/eeschema/tools/sch_edit_tool.cpp @@ -824,7 +824,7 @@ int SCH_EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent ) rotPoint = m_frame->GetNearestHalfGridPosition( head->GetBoundingBox().GetCenter() ); if( !moving ) - commit->Modify( head, m_frame->GetScreen() ); + commit->Modify( head, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); switch( head->Type() ) { @@ -989,7 +989,7 @@ int SCH_EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent ) continue; if( !moving ) - commit->Modify( item, m_frame->GetScreen() ); + commit->Modify( item, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); if( item->Type() == SCH_LINE_T ) { @@ -1092,7 +1092,7 @@ int SCH_EDIT_TOOL::Mirror( const TOOL_EVENT& aEvent ) if( selection.GetSize() == 1 ) { if( !moving ) - commit->Modify( item, m_frame->GetScreen() ); + commit->Modify( item, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); switch( item->Type() ) { @@ -1193,7 +1193,7 @@ int SCH_EDIT_TOOL::Mirror( const TOOL_EVENT& aEvent ) item = static_cast( edaItem ); if( !moving ) - commit->Modify( item, m_frame->GetScreen() ); + commit->Modify( item, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); if( item->Type() == SCH_SHEET_PIN_T ) { @@ -1511,12 +1511,13 @@ int SCH_EDIT_TOOL::RepeatDrawItem( const TOOL_EVENT& aEvent ) m_toolMgr->RunAction( ACTIONS::selectionClear ); - SCH_COMMIT commit( m_toolMgr ); - SCH_SELECTION newItems; + SCH_SELECTION_TOOL* selectionTool = m_toolMgr->GetTool(); + SCH_COMMIT commit( m_toolMgr ); + SCH_SELECTION newItems; for( const std::unique_ptr& item : sourceItems ) { - SCH_ITEM* newItem = item->Duplicate(); + SCH_ITEM* newItem = item->Duplicate( IGNORE_PARENT_GROUP ); SETTINGS_MANAGER& mgr = Pgm().GetSettingsManager(); EESCHEMA_SETTINGS* cfg = mgr.GetAppSettings( "eeschema" ); bool restore_state = false; @@ -1525,6 +1526,15 @@ int SCH_EDIT_TOOL::RepeatDrawItem( const TOOL_EVENT& aEvent ) // a list of items to repeat must be attached to this current screen newItem->SetParent( m_frame->GetScreen() ); + if( SCH_GROUP* enteredGroup = selectionTool->GetEnteredGroup() ) + { + if( newItem->IsGroupableType() ) + { + commit.Modify( enteredGroup ); + enteredGroup->AddItem( newItem ); + } + } + if( SCH_LABEL_BASE* label = dynamic_cast( newItem ) ) { // If incrementing tries to go below zero, tell user why the value is repeated @@ -1716,14 +1726,7 @@ int SCH_EDIT_TOOL::DoDelete( const TOOL_EVENT& aEvent ) } else if( sch_item->Type() == SCH_GROUP_T ) { - // Groups need to have all children ungrouped, then deleted - sch_item->RunOnChildren( - [&]( SCH_ITEM* aChild ) - { - commit.Stage( aChild, CHT_UNGROUP, m_frame->GetScreen() ); - }, - RECURSE_MODE::RECURSE ); - + // Groups need to delete their children sch_item->RunOnChildren( [&]( SCH_ITEM* aChild ) { diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index 395870ffb3..cfcbb95759 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -2338,14 +2338,15 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent ) SCH_ITEM* item = nullptr; group->RunOnChildren( - [&]( SCH_ITEM* schItem ) - { - if( !found && schItem->Type() != SCH_GROUP_T ) + [&]( SCH_ITEM* schItem ) { - item = schItem; - found = true; - } - }, RECURSE_MODE::RECURSE ); + if( !found && schItem->Type() != SCH_GROUP_T ) + { + item = schItem; + found = true; + } + }, + RECURSE_MODE::RECURSE ); if( found ) selection.SetReferencePoint( item->GetPosition() ); @@ -2900,12 +2901,15 @@ int SCH_EDITOR_CONTROL::RepairSchematic( const TOOL_EVENT& aEvent ) { processItem( item ); - item->RunOnChildren( - [&]( SCH_ITEM* aChild ) - { - processItem( item ); - }, - RECURSE_MODE::NO_RECURSE ); + if( item->Type() != SCH_GROUP_T ) + { + item->RunOnChildren( + [&]( SCH_ITEM* aChild ) + { + processItem( item ); + }, + RECURSE_MODE::NO_RECURSE ); + } } } diff --git a/eeschema/tools/sch_find_replace_tool.cpp b/eeschema/tools/sch_find_replace_tool.cpp index d66d396153..05aded802e 100644 --- a/eeschema/tools/sch_find_replace_tool.cpp +++ b/eeschema/tools/sch_find_replace_tool.cpp @@ -69,16 +69,7 @@ int SCH_FIND_REPLACE_TOOL::UpdateFind( const TOOL_EVENT& aEvent ) [&]() { for( SCH_ITEM* item : m_frame->GetScreen()->Items() ) - { visit( item, &m_frame->GetCurrentSheet() ); - - item->RunOnChildren( - [&]( SCH_ITEM* aChild ) - { - visit( aChild, &m_frame->GetCurrentSheet() ); - }, - RECURSE_MODE::NO_RECURSE ); - } }; if( aEvent.IsAction( &ACTIONS::find ) || aEvent.IsAction( &ACTIONS::findAndReplace ) diff --git a/eeschema/tools/sch_group_tool.cpp b/eeschema/tools/sch_group_tool.cpp index d40314b643..faeed3398b 100644 --- a/eeschema/tools/sch_group_tool.cpp +++ b/eeschema/tools/sch_group_tool.cpp @@ -133,10 +133,12 @@ int SCH_GROUP_TOOL::Group( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : selection.GetItems() ) { - if( !SCH_GROUP::IsGroupableType( item->Type() ) ) + SCH_ITEM* schItem = static_cast( item ); + + if( !schItem->IsGroupableType() ) selection.Remove( item ); - if( isSymbolEditor && static_cast( item )->GetParentSymbol() ) + if( isSymbolEditor && schItem->GetParentSymbol() ) selection.Remove( item ); } @@ -151,11 +153,13 @@ int SCH_GROUP_TOOL::Group( const TOOL_EVENT& aEvent ) else group->SetParent( screen ); - m_commit->Add( group, screen ); - for( EDA_ITEM* eda_item : selection ) - m_commit->Stage( eda_item, CHT_GROUP, screen ); + { + m_commit->Modify( eda_item, screen ); + group->AddItem( eda_item ); + } + m_commit->Add( group, screen ); m_commit->Push( _( "Group Items" ) ); m_toolMgr->RunAction( ACTIONS::selectionClear ); diff --git a/eeschema/tools/sch_inspection_tool.cpp b/eeschema/tools/sch_inspection_tool.cpp index 90adfec668..1941518544 100644 --- a/eeschema/tools/sch_inspection_tool.cpp +++ b/eeschema/tools/sch_inspection_tool.cpp @@ -250,7 +250,7 @@ void SCH_INSPECTION_TOOL::InspectERCError( const std::shared_ptr& aERCI wxCHECK( frame, /* void */ ); - EDA_ITEM* a = frame->GetItem( aERCItem->GetMainItemID() ); + EDA_ITEM* a = frame->ResolveItem( aERCItem->GetMainItemID() ); if( aERCItem->GetErrorCode() == ERCE_BUS_TO_NET_CONFLICT ) { diff --git a/eeschema/tools/sch_line_wire_bus_tool.cpp b/eeschema/tools/sch_line_wire_bus_tool.cpp index 0baeacb78a..15f12ba43d 100644 --- a/eeschema/tools/sch_line_wire_bus_tool.cpp +++ b/eeschema/tools/sch_line_wire_bus_tool.cpp @@ -291,6 +291,7 @@ int SCH_LINE_WIRE_BUS_TOOL::DrawSegments( const TOOL_EVENT& aEvent ) REENTRANCY_GUARD guard( &m_inDrawingTool ); const DRAW_SEGMENT_EVENT_PARAMS* params = aEvent.Parameter(); + SCH_COMMIT commit( m_toolMgr ); m_frame->PushTool( aEvent ); m_toolMgr->RunAction( ACTIONS::selectionClear ); @@ -304,10 +305,10 @@ int SCH_LINE_WIRE_BUS_TOOL::DrawSegments( const TOOL_EVENT& aEvent ) grid.SetUseGrid( getView()->GetGAL()->GetGridSnapping() && !aEvent.DisableGridSnapping() ); VECTOR2D cursorPos = grid.BestSnapAnchor( aEvent.Position(), gridType, nullptr ); - startSegments( params->layer, cursorPos, params->sourceSegment ); + startSegments( commit, params->layer, cursorPos, params->sourceSegment ); } - return doDrawSegments( aEvent, params->layer, params->quitOnDraw ); + return doDrawSegments( aEvent, commit, params->layer, params->quitOnDraw ); } @@ -318,9 +319,10 @@ int SCH_LINE_WIRE_BUS_TOOL::UnfoldBus( const TOOL_EVENT& aEvent ) REENTRANCY_GUARD guard( &m_inDrawingTool ); - wxString* netPtr = aEvent.Parameter(); - wxString net; - SCH_LINE* segment = nullptr; + SCH_COMMIT commit( m_toolMgr ); + wxString* netPtr = aEvent.Parameter(); + wxString net; + SCH_LINE* segment = nullptr; m_frame->PushTool( aEvent ); Activate(); @@ -366,12 +368,12 @@ int SCH_LINE_WIRE_BUS_TOOL::UnfoldBus( const TOOL_EVENT& aEvent ) // Break a wire for the given net out of the bus if( !net.IsEmpty() ) - segment = doUnfoldBus( net ); + segment = doUnfoldBus( commit, net ); // If we have an unfolded wire to draw, then draw it if( segment ) { - return doDrawSegments( aEvent, LAYER_WIRE, false ); + return doDrawSegments( aEvent, commit, LAYER_WIRE, false ); } else { @@ -388,7 +390,7 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::getBusForUnfolding() } -SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::doUnfoldBus( const wxString& aNet, +SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::doUnfoldBus( SCH_COMMIT& aCommit, const wxString& aNet, const std::optional& aPos ) { SCHEMATIC_SETTINGS& cfg = getModel()->Settings(); @@ -398,9 +400,8 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::doUnfoldBus( const wxString& aN if ( bus == nullptr ) { - wxASSERT_MSG( false, - wxString::Format( "Couldn't find the originating bus line (but had a net: %s )", - aNet ) ); + wxASSERT_MSG( false, wxString::Format( "Couldn't find the originating bus line (but had a net: %s )", + aNet ) ); return nullptr; } @@ -441,7 +442,7 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::doUnfoldBus( const wxString& aN m_busUnfold.entry->SetEndDangling( false ); m_busUnfold.label->SetIsDangling( false ); - return startSegments( LAYER_WIRE, m_busUnfold.entry->GetEnd() ); + return startSegments( aCommit, LAYER_WIRE, m_busUnfold.entry->GetEnd() ); } @@ -601,7 +602,8 @@ void SCH_LINE_WIRE_BUS_TOOL::computeBreakPoint( const std::pairGetScreen(); SCH_LINE* segment = nullptr; @@ -717,7 +719,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, segment->SetEndPoint( cursorPos ); // Create a new segment, and chain it after the current segment. - segment = static_cast( segment->Duplicate() ); + segment = static_cast( segment->Duplicate( true, &aCommit ) ); segment->SetFlags( IS_NEW | IS_MOVING ); segment->SetStartPoint( cursorPos ); m_wires.push_back( segment ); @@ -778,9 +780,11 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, { if( segment || m_busUnfold.in_progress ) { - finishSegments(); + finishSegments( aCommit ); segment = nullptr; + aCommit.Push( _( "Draw Wires" ) ); + if( aQuitOnDraw ) { m_frame->PopTool( aTool ); @@ -807,7 +811,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, if( !segment ) { - segment = startSegments( aType, VECTOR2D( cursorPos ) ); + segment = startSegments( aCommit, aType, VECTOR2D( cursorPos ) ); } // Create a new segment if we're out of previously-created ones else if( !segment->IsNull() @@ -817,9 +821,11 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, // wire or bus. if( screen->IsTerminalPoint( cursorPos, segment->GetLayer() ) ) { - finishSegments(); + finishSegments( aCommit ); segment = nullptr; + aCommit.Push( _( "Draw Wires" ) ); + if( aQuitOnDraw ) { m_frame->PopTool( aTool ); @@ -841,7 +847,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, for( int i = 0; i < placedSegments; i++ ) { // Create a new segment, and chain it after the current segment. - segment = static_cast( segment->Duplicate() ); + segment = static_cast( segment->Duplicate( true, &aCommit ) ); segment->SetFlags( IS_NEW | IS_MOVING ); segment->SetStartPoint( cursorPos ); m_wires.push_back( segment ); @@ -859,9 +865,11 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, currentMode, posture ); } - finishSegments(); + finishSegments( aCommit ); segment = nullptr; + aCommit.Push( _( "Draw Wires" ) ); + if( aQuitOnDraw ) { m_frame->PopTool( aTool ); @@ -1031,7 +1039,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, aType = LAYER_WIRE; wxString net = *evt->Parameter(); - segment = doUnfoldBus( net, contextMenuPos ); + segment = doUnfoldBus( aCommit, net, contextMenuPos ); } } //------------------------------------------------------------------------ @@ -1073,7 +1081,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const TOOL_EVENT& aTool, int aType, } -SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::startSegments( int aType, const VECTOR2D& aPos, +SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::startSegments( SCH_COMMIT& aCommit, int aType, const VECTOR2D& aPos, SCH_LINE* aSegment ) { if( !aSegment ) @@ -1093,7 +1101,7 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::startSegments( int aType, const VECTOR2D& aPos } else { - aSegment = static_cast( aSegment->Duplicate() ); + aSegment = static_cast( aSegment->Duplicate( true, &aCommit ) ); aSegment->SetStartPoint( aPos ); } @@ -1107,7 +1115,7 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::startSegments( int aType, const VECTOR2D& aPos // horizontal and vertical lines only switch is on. if( m_frame->eeconfig()->m_Drawing.line_mode ) { - aSegment = static_cast( aSegment->Duplicate() ); + aSegment = static_cast( aSegment->Duplicate( true, &aCommit ) ); aSegment->SetFlags( IS_NEW | IS_MOVING ); m_wires.push_back( aSegment ); @@ -1165,7 +1173,7 @@ void SCH_LINE_WIRE_BUS_TOOL::simplifyWireList() } -void SCH_LINE_WIRE_BUS_TOOL::finishSegments() +void SCH_LINE_WIRE_BUS_TOOL::finishSegments( SCH_COMMIT& aCommit ) { // Clear selection when done so that a new wire can be started. // NOTE: this must be done before simplifyWireList is called or we might end up with @@ -1173,7 +1181,6 @@ void SCH_LINE_WIRE_BUS_TOOL::finishSegments() m_toolMgr->RunAction( ACTIONS::selectionClear ); SCH_SCREEN* screen = m_frame->GetScreen(); - SCH_COMMIT commit( m_toolMgr ); // Remove segments backtracking over others simplifyWireList(); @@ -1198,17 +1205,17 @@ void SCH_LINE_WIRE_BUS_TOOL::finishSegments() new_ends.push_back( pt ); } - commit.Added( wire, screen ); + aCommit.Added( wire, screen ); } if( m_busUnfold.in_progress && m_busUnfold.label_placed ) { wxASSERT( m_busUnfold.entry && m_busUnfold.label ); - commit.Added( m_busUnfold.entry, screen ); + aCommit.Added( m_busUnfold.entry, screen ); m_frame->SaveCopyForRepeatItem( m_busUnfold.entry ); - commit.Added( m_busUnfold.label, screen ); + aCommit.Added( m_busUnfold.label, screen ); m_frame->AddCopyForRepeatItem( m_busUnfold.label ); m_busUnfold.label->ClearEditFlags(); } @@ -1239,7 +1246,7 @@ void SCH_LINE_WIRE_BUS_TOOL::finishSegments() getViewControls()->SetAutoPan( false ); // Correct and remove segments that need to be merged. - m_frame->SchematicCleanUp( &commit ); + m_frame->SchematicCleanUp( &aCommit ); std::vector symbols; @@ -1256,14 +1263,14 @@ void SCH_LINE_WIRE_BUS_TOOL::finishSegments() for( auto pt = pts.begin(); pt != pts.end(); pt++ ) { for( auto secondPt = pt + 1; secondPt != pts.end(); secondPt++ ) - m_frame->TrimWire( &commit, *pt, *secondPt ); + m_frame->TrimWire( &aCommit, *pt, *secondPt ); } } for( const VECTOR2I& pt : new_ends ) { if( m_frame->GetScreen()->IsExplicitJunctionNeeded( pt ) ) - m_frame->AddJunction( &commit, m_frame->GetScreen(), pt ); + m_frame->AddJunction( &aCommit, m_frame->GetScreen(), pt ); } if( m_busUnfold.in_progress ) @@ -1271,8 +1278,6 @@ void SCH_LINE_WIRE_BUS_TOOL::finishSegments() for( SCH_ITEM* item : m_frame->GetScreen()->Items() ) item->ClearEditFlags(); - - commit.Push( _( "Draw Wires" ) ); } diff --git a/eeschema/tools/sch_line_wire_bus_tool.h b/eeschema/tools/sch_line_wire_bus_tool.h index 1b19533f48..c64d79a8e8 100644 --- a/eeschema/tools/sch_line_wire_bus_tool.h +++ b/eeschema/tools/sch_line_wire_bus_tool.h @@ -96,9 +96,11 @@ public: int TrimOverLappingWires( SCH_COMMIT* aCommit, SCH_SELECTION* aSelection ); private: - int doDrawSegments( const TOOL_EVENT& aTool, int aType, bool aQuitOnDraw ); + int doDrawSegments( const TOOL_EVENT& aTool, SCH_COMMIT& aCommit, int aType, + bool aQuitOnDraw ); - SCH_LINE* startSegments( int aType, const VECTOR2D& aPos, SCH_LINE* aSegment = nullptr ); + SCH_LINE* startSegments( SCH_COMMIT& aCommit, int aType, const VECTOR2D& aPos, + SCH_LINE* aSegment = nullptr ); /** * Choose a bus to unfold based on the current tool selection. @@ -112,10 +114,10 @@ private: * @param aPos The position to unfold the bus from, which will be the cursor if * not provided, and will then be snapped to the selected bus segment. */ - SCH_LINE* doUnfoldBus( const wxString& aNet, + SCH_LINE* doUnfoldBus( SCH_COMMIT& aCommit, const wxString& aNet, const std::optional& aPos = std::nullopt ); - void finishSegments(); + void finishSegments( SCH_COMMIT& aCommit ); /** * Iterate over the wire list and removes the null segments and diff --git a/eeschema/tools/sch_move_tool.cpp b/eeschema/tools/sch_move_tool.cpp index ce48871300..4d903c72ee 100644 --- a/eeschema/tools/sch_move_tool.cpp +++ b/eeschema/tools/sch_move_tool.cpp @@ -625,36 +625,48 @@ bool SCH_MOVE_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, SCH_COMMIT* aComm for( EDA_ITEM* item : selection ) { - if( item->IsNew() ) + SCH_ITEM* schItem = static_cast( item ); + + if( schItem->IsNew() ) { // Item was added to commit in a previous command + + // While SCH_COMMIT::Push() will add any new items to the entered group, + // we need to do it earlier so that the previews while moving are correct. + if( SCH_GROUP* enteredGroup = m_selectionTool->GetEnteredGroup() ) + { + if( schItem->IsGroupableType() && !schItem->GetParentGroup() ) + { + aCommit->Modify( enteredGroup ); + enteredGroup->AddItem( schItem ); + } + } } - else if( item->GetParent() && item->GetParent()->IsSelected() ) + else if( schItem->GetParent() && schItem->GetParent()->IsSelected() ) { // Item will be (or has been) added to commit by parent } else { - aCommit->Modify( item, m_frame->GetScreen() ); + aCommit->Modify( schItem, m_frame->GetScreen() ); } - item->SetFlags( IS_MOVING ); + schItem->SetFlags( IS_MOVING ); - if( SCH_SHAPE* shape = dynamic_cast( item ) ) + if( SCH_SHAPE* shape = dynamic_cast( schItem ) ) { shape->SetHatchingDirty(); shape->UpdateHatching(); } - static_cast( item )->RunOnChildren( + schItem->RunOnChildren( [&]( SCH_ITEM* schItem ) { item->SetFlags( IS_MOVING ); }, RECURSE_MODE::RECURSE ); - if( SCH_ITEM* schItem = dynamic_cast( item ) ) - schItem->SetStoredPos( schItem->GetPosition() ); + schItem->SetStoredPos( schItem->GetPosition() ); } // Set up the starting position and move/drag offset @@ -1670,7 +1682,7 @@ int SCH_MOVE_TOOL::AlignToGrid( const TOOL_EVENT& aEvent ) auto doMoveItem = [&]( EDA_ITEM* item, const VECTOR2I& delta ) { - commit.Modify( item, m_frame->GetScreen() ); + commit.Modify( item, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); // Ensure only one end is moved when calling moveItem // i.e. we are in drag mode diff --git a/eeschema/tools/sch_selection_tool.cpp b/eeschema/tools/sch_selection_tool.cpp index 9b33a64049..31a4d0ff5d 100644 --- a/eeschema/tools/sch_selection_tool.cpp +++ b/eeschema/tools/sch_selection_tool.cpp @@ -1076,7 +1076,7 @@ int SCH_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent ) if( lastRolloverItem != niluuid && lastRolloverItem != rolloverItem ) { - EDA_ITEM* item = m_frame->GetItem( lastRolloverItem ); + EDA_ITEM* item = m_frame->ResolveItem( lastRolloverItem ); if( item->IsRollover() ) { @@ -1091,7 +1091,7 @@ int SCH_SELECTION_TOOL::Main( const TOOL_EVENT& aEvent ) if( rolloverItem != niluuid ) { - EDA_ITEM* item = m_frame->GetItem( rolloverItem ); + EDA_ITEM* item = m_frame->ResolveItem( rolloverItem ); if( !item->IsRollover() ) { @@ -1164,6 +1164,7 @@ void SCH_SELECTION_TOOL::EnterGroup() { if( aChild->Type() == SCH_LINE_T ) aChild->SetFlags( STARTPOINT | ENDPOINT ); + select( aChild ); }, RECURSE_MODE::NO_RECURSE ); @@ -1897,13 +1898,12 @@ SCH_SELECTION& SCH_SELECTION_TOOL::RequestSelection( const std::vector& if( item->Type() == SCH_GROUP_T ) { - static_cast(item) - ->RunOnChildren( [&]( SCH_ITEM* aChild ) - { - if( aChild->IsType( aScanTypes ) ) - selectedChildren.insert( aChild ); - }, - RECURSE_MODE::RECURSE ); + static_cast(item)->RunOnChildren( [&]( SCH_ITEM* aChild ) + { + if( aChild->IsType( aScanTypes ) ) + selectedChildren.insert( aChild ); + }, + RECURSE_MODE::RECURSE ); unselect( item ); anyUnselected = true; } diff --git a/eeschema/tools/symbol_editor_drawing_tools.cpp b/eeschema/tools/symbol_editor_drawing_tools.cpp index 6e0bc29e33..f00db08568 100644 --- a/eeschema/tools/symbol_editor_drawing_tools.cpp +++ b/eeschema/tools/symbol_editor_drawing_tools.cpp @@ -290,7 +290,7 @@ int SYMBOL_EDITOR_DRAWING_TOOLS::TwoClickPlace( const TOOL_EVENT& aEvent ) switch( item->Type() ) { case SCH_PIN_T: - pinTool->PlacePin( static_cast( item ) ); + pinTool->PlacePin( &commit, static_cast( item ) ); item->ClearEditFlags(); commit.Push( _( "Place Pin" ) ); break; diff --git a/eeschema/tools/symbol_editor_edit_tool.cpp b/eeschema/tools/symbol_editor_edit_tool.cpp index 6d1fb52afa..6f10ac8679 100644 --- a/eeschema/tools/symbol_editor_edit_tool.cpp +++ b/eeschema/tools/symbol_editor_edit_tool.cpp @@ -183,7 +183,7 @@ int SYMBOL_EDITOR_EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent ) commit = &localCommit; if( !item->IsMoving() ) - commit->Modify( m_frame->GetCurSymbol(), m_frame->GetScreen() ); + commit->Modify( m_frame->GetCurSymbol(), m_frame->GetScreen(), RECURSE_MODE::RECURSE ); if( selection.GetSize() == 1 ) rotPoint = item->GetPosition(); @@ -1015,7 +1015,7 @@ int SYMBOL_EDITOR_EDIT_TOOL::Paste( const TOOL_EVENT& aEvent ) if( item.Type() == SCH_FIELD_T ) continue; - SCH_ITEM* newItem = item.Duplicate(); + SCH_ITEM* newItem = item.Duplicate( true, &commit ); newItem->SetParent( symbol ); newItem->SetFlags( IS_NEW | IS_PASTED | SELECTED ); @@ -1093,7 +1093,7 @@ int SYMBOL_EDITOR_EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : oldItems ) { SCH_ITEM* oldItem = static_cast( item ); - SCH_ITEM* newItem = oldItem->Duplicate(); + SCH_ITEM* newItem = oldItem->Duplicate( true, &commit ); if( newItem->Type() == SCH_PIN_T ) { diff --git a/eeschema/tools/symbol_editor_move_tool.cpp b/eeschema/tools/symbol_editor_move_tool.cpp index 1f81f7da22..8a885c0eb6 100644 --- a/eeschema/tools/symbol_editor_move_tool.cpp +++ b/eeschema/tools/symbol_editor_move_tool.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -232,8 +233,21 @@ bool SYMBOL_EDITOR_MOVE_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, SCH_COM // Drag items to the current cursor position for( EDA_ITEM* item : selection ) { - moveItem( item, delta ); - updateItem( item, false ); + SCH_ITEM* schItem = static_cast( item ); + + moveItem( schItem, delta ); + updateItem( schItem, false ); + + // While SCH_COMMIT::Push() will add any new items to the entered group, + // we need to do it earlier so that the previews while moving are correct. + if( SCH_GROUP* enteredGroup = m_selectionTool->GetEnteredGroup() ) + { + if( schItem->IsGroupableType() && !item->GetParentGroup() ) + { + aCommit->Modify( enteredGroup ); + enteredGroup->AddItem( schItem ); + } + } } m_anchorPos = m_cursor; @@ -329,7 +343,7 @@ bool SYMBOL_EDITOR_MOVE_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, SCH_COM { SCH_PIN* curr_pin = static_cast( selection.Front() ); - if( pinTool->PlacePin( curr_pin ) ) + if( pinTool->PlacePin( aCommit, curr_pin ) ) { // PlacePin() clears the current selection, which we don't want. Not only // is it a poor user experience, but it also prevents us from doing the @@ -383,21 +397,17 @@ int SYMBOL_EDITOR_MOVE_TOOL::AlignElements( const TOOL_EVENT& aEvent ) SCH_SELECTION& selection = m_selectionTool->RequestSelection(); SCH_COMMIT commit( m_toolMgr ); - auto doMoveItem = - [&]( EDA_ITEM* item, const VECTOR2I& delta ) - { - commit.Modify( item, m_frame->GetScreen() ); - static_cast( item )->Move( delta ); - updateItem( item, true ); - }; - for( EDA_ITEM* item : selection ) { VECTOR2I newPos = grid.AlignGrid( item->GetPosition(), grid.GetItemGrid( item ) ); VECTOR2I delta = newPos - item->GetPosition(); if( delta != VECTOR2I( 0, 0 ) ) - doMoveItem( item, delta ); + { + commit.Modify( item, m_frame->GetScreen(), RECURSE_MODE::RECURSE ); + static_cast( item )->Move( delta ); + updateItem( item, true ); + }; if( SCH_PIN* pin = dynamic_cast( item ) ) { diff --git a/eeschema/tools/symbol_editor_pin_tool.cpp b/eeschema/tools/symbol_editor_pin_tool.cpp index 81fdce77de..6d02a1e4bf 100644 --- a/eeschema/tools/symbol_editor_pin_tool.cpp +++ b/eeschema/tools/symbol_editor_pin_tool.cpp @@ -24,8 +24,6 @@ #include "symbol_editor_pin_tool.h" -#include -#include #include #include #include @@ -217,7 +215,7 @@ bool SYMBOL_EDITOR_PIN_TOOL::EditPinProperties( SCH_PIN* aPin, bool aFocusPinNum } -bool SYMBOL_EDITOR_PIN_TOOL::PlacePin( SCH_PIN* aPin ) +bool SYMBOL_EDITOR_PIN_TOOL::PlacePin( SCH_COMMIT* aCommit, SCH_PIN* aPin ) { LIB_SYMBOL* symbol = m_frame->GetCurSymbol(); bool ask_for_pin = true; // Test for another pin in same position in other units @@ -266,7 +264,7 @@ bool SYMBOL_EDITOR_PIN_TOOL::PlacePin( SCH_PIN* aPin ) g_LastPinShape = aPin->GetShape(); if( m_frame->SynchronizePins() ) - CreateImagePins( aPin ); + CreateImagePins( aCommit, aPin ); symbol->AddDrawItem( aPin ); aPin->ClearFlags( IS_NEW ); @@ -325,7 +323,7 @@ SCH_PIN* SYMBOL_EDITOR_PIN_TOOL::CreatePin( const VECTOR2I& aPosition, LIB_SYMBO } -void SYMBOL_EDITOR_PIN_TOOL::CreateImagePins( SCH_PIN* aPin ) +void SYMBOL_EDITOR_PIN_TOOL::CreateImagePins( SCH_COMMIT* aCommit, SCH_PIN* aPin ) { int ii; SCH_PIN* newPin; @@ -348,7 +346,9 @@ void SYMBOL_EDITOR_PIN_TOOL::CreateImagePins( SCH_PIN* aPin ) if( ii == aPin->GetUnit() ) continue; - newPin = static_cast( aPin->Duplicate() ); + // Already called Modify() on parent symbol; no need for Modify() calls on individual items + SCH_COMMIT dummy( m_toolMgr ); + newPin = static_cast( aPin->Duplicate( true, &dummy ) ); // To avoid mistakes, gives this pin a new pin number because // it does no have the save pin number as the master pin @@ -366,8 +366,7 @@ void SYMBOL_EDITOR_PIN_TOOL::CreateImagePins( SCH_PIN* aPin ) } catch( const boost::bad_pointer& e ) { - wxFAIL_MSG( wxString::Format( wxT( "Boost pointer exception occurred: %s" ), - e.what() )); + wxFAIL_MSG( wxString::Format( wxT( "Boost pointer exception occurred: %s" ), e.what() )); delete newPin; return; } @@ -423,7 +422,7 @@ SCH_PIN* SYMBOL_EDITOR_PIN_TOOL::RepeatPin( const SCH_PIN* aSourcePin ) commit.Modify( symbol ); - SCH_PIN* pin = static_cast( aSourcePin->Duplicate() ); + SCH_PIN* pin = static_cast( aSourcePin->Duplicate( true, &commit ) ); VECTOR2I step; pin->ClearFlags(); @@ -454,7 +453,7 @@ SCH_PIN* SYMBOL_EDITOR_PIN_TOOL::RepeatPin( const SCH_PIN* aSourcePin ) if( m_frame->SynchronizePins() ) pin->SetFlags( IS_LINKED ); - if( PlacePin( pin ) ) + if( PlacePin( &commit, pin ) ) { commit.Push( _( "Repeat Pin" ) ); return pin; diff --git a/eeschema/tools/symbol_editor_pin_tool.h b/eeschema/tools/symbol_editor_pin_tool.h index ef2f03fecc..c8a567d8c5 100644 --- a/eeschema/tools/symbol_editor_pin_tool.h +++ b/eeschema/tools/symbol_editor_pin_tool.h @@ -44,8 +44,8 @@ public: SCH_PIN* CreatePin( const VECTOR2I& aPosition, LIB_SYMBOL* aSymbol ); SCH_PIN* RepeatPin( const SCH_PIN* aSourcePin ); - bool PlacePin( SCH_PIN* aPin ); - void CreateImagePins( SCH_PIN* aPin ); + bool PlacePin( SCH_COMMIT* aCommit, SCH_PIN* aPin ); + void CreateImagePins( SCH_COMMIT* aCommit, SCH_PIN* aPin ); bool EditPinProperties( SCH_PIN* aPin, bool aFocusPinNumber ); int PushPinProperties( const TOOL_EVENT& aEvent ); diff --git a/include/board_item.h b/include/board_item.h index 72693fa66d..375eb625bf 100644 --- a/include/board_item.h +++ b/include/board_item.h @@ -37,6 +37,7 @@ class BOARD; class BOARD_DESIGN_SETTINGS; class BOARD_ITEM_CONTAINER; +class BOARD_COMMIT; class SHAPE_POLY_SET; class SHAPE_SEGMENT; class PCB_BASE_FRAME; @@ -87,6 +88,8 @@ public: virtual void CopyFrom( const BOARD_ITEM* aOther ); + bool IsGroupableType() const; + // Do not create a copy constructor & operator=. // The ones generated by the compiler are adequate. int GetX() const @@ -284,8 +287,11 @@ public: /** * Create a copy of this #BOARD_ITEM. + * + * @param addToParentGroup Indicates whether or not the new item is added to the group + * containing the old item. If true, aCommit must be provided. */ - virtual BOARD_ITEM* Duplicate() const; + virtual BOARD_ITEM* Duplicate( bool addToParentGroup, BOARD_COMMIT* aCommit = nullptr ) const; /** * Swap data between \a aItem and \a aImage. diff --git a/include/commit.h b/include/commit.h index 74bd0afffa..39d23b1100 100644 --- a/include/commit.h +++ b/include/commit.h @@ -33,8 +33,9 @@ #include #include #include +#include -class EDA_ITEM; +class EDA_GROUP; class BASE_SCREEN; ///< Types of changes @@ -42,9 +43,7 @@ enum CHANGE_TYPE { CHT_ADD = 1, CHT_REMOVE = 2, CHT_MODIFY = 4, - CHT_GROUP = 8, - CHT_UNGROUP = 16, - CHT_TYPE = CHT_ADD | CHT_REMOVE | CHT_MODIFY | CHT_GROUP | CHT_UNGROUP, + CHT_TYPE = CHT_ADD | CHT_REMOVE | CHT_MODIFY, CHT_DONE = 32, ///< Flag to indicate the change is already applied CHT_FLAGS = CHT_DONE @@ -105,9 +104,10 @@ public: * * @note Must be called before modification is performed. */ - COMMIT& Modify( EDA_ITEM* aItem, BASE_SCREEN *aScreen = nullptr ) + COMMIT& Modify( EDA_ITEM* aItem, BASE_SCREEN *aScreen = nullptr, + RECURSE_MODE aRecurse = RECURSE_MODE::NO_RECURSE ) { - return Stage( aItem, CHT_MODIFY, aScreen ); + return Stage( aItem, CHT_MODIFY, aScreen, aRecurse ); } /** @@ -131,8 +131,8 @@ public: } /// Add a change of the item aItem of type aChangeType to the change list. - virtual COMMIT& Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, - BASE_SCREEN *aScreen = nullptr ); + virtual COMMIT& Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen = nullptr, + RECURSE_MODE aRecurse = RECURSE_MODE::NO_RECURSE ); virtual COMMIT& Stage( std::vector& container, CHANGE_TYPE aChangeType, BASE_SCREEN *aScreen = nullptr ); diff --git a/include/eda_draw_frame.h b/include/eda_draw_frame.h index 011c1ef1f7..15428c4fc8 100644 --- a/include/eda_draw_frame.h +++ b/include/eda_draw_frame.h @@ -431,7 +431,10 @@ public: /** * Fetch an item by KIID. Frame-type-specific implementation. */ - virtual EDA_ITEM* GetItem( const KIID& aId ) const { return nullptr; } + virtual EDA_ITEM* ResolveItem( const KIID& aId, bool aAllowNullptrReturn = false ) const + { + return nullptr; + } /** * Use to start up the GAL drawing canvas. diff --git a/include/eda_item.h b/include/eda_item.h index f4e73388a7..37dbd288ad 100644 --- a/include/eda_item.h +++ b/include/eda_item.h @@ -51,6 +51,7 @@ enum RECURSE_MODE NO_RECURSE, }; +#define IGNORE_PARENT_GROUP false /** * Additional flag values wxFindReplaceData::m_Flags @@ -95,7 +96,7 @@ typedef const INSPECTOR_FUNC& INSPECTOR; class EDA_ITEM : public KIGFX::VIEW_ITEM, public SERIALIZABLE { public: - virtual ~EDA_ITEM() { wxASSERT( m_group == nullptr ); }; + virtual ~EDA_ITEM() = default; /** * Returns the type of object. @@ -113,6 +114,8 @@ public: virtual void SetParentGroup( EDA_GROUP* aGroup ) { m_group = aGroup; } virtual EDA_GROUP* GetParentGroup() const { return m_group; } + KIID GetParentGroupId() const; + virtual bool IsLocked() const { return false; } virtual void SetLocked( bool aLocked ) {} @@ -498,6 +501,8 @@ protected: */ bool Matches( const wxString& aText, const EDA_SEARCH_DATA& aSearchData ) const; + EDA_ITEM* findParent( KICAD_T aType ) const; + public: const KIID m_Uuid; @@ -511,8 +516,8 @@ private: protected: EDA_ITEM_FLAGS m_flags; - EDA_ITEM* m_parent; ///< Linked list: Link (parent struct). - EDA_GROUP* m_group; ///< The group this item belongs to + EDA_ITEM* m_parent; ///< Owner. + EDA_GROUP* m_group; ///< The group this item belongs to, if any. No ownership implied. bool m_forceVisible; bool m_isRollover; }; diff --git a/include/pcb_base_frame.h b/include/pcb_base_frame.h index 0de78b52ac..9740e3eeff 100644 --- a/include/pcb_base_frame.h +++ b/include/pcb_base_frame.h @@ -213,7 +213,7 @@ public: */ virtual BOARD_ITEM_CONTAINER* GetModel() const = 0; - EDA_ITEM* GetItem( const KIID& aId ) const override; + EDA_ITEM* ResolveItem( const KIID& aId, bool aAllowNullptrReturn = false ) const override; void FocusOnItem( EDA_ITEM* aItem ) override; void FocusOnItem( BOARD_ITEM* aItem, PCB_LAYER_ID aLayer = UNDEFINED_LAYER ); diff --git a/include/undo_redo_container.h b/include/undo_redo_container.h index d7e7ea8510..2560df281a 100644 --- a/include/undo_redo_container.h +++ b/include/undo_redo_container.h @@ -68,9 +68,6 @@ enum class UNDO_REDO { DRILLORIGIN, // origin changed (like CHANGED, contains the origin and a copy) GRIDORIGIN, // origin changed (like CHANGED, contains the origin and a copy) PAGESETTINGS, // page settings or title block changes - REGROUP, // new group of items created (NB: can't use GROUP because of collision - // with a header on msys2) - UNGROUP, // existing group destroyed (items not destroyed) REPEAT_ITEM // storage entry for the editor's global repeatItems list }; @@ -78,32 +75,24 @@ enum class UNDO_REDO { class ITEM_PICKER { public: -// ITEM_PICKER( EDA_ITEM* aItem = NULL, UNDO_REDO aStatus = UNSPECIFIED ); ITEM_PICKER(); ITEM_PICKER( BASE_SCREEN* aScreen, EDA_ITEM* aItem, UNDO_REDO aStatus = UNDO_REDO::UNSPECIFIED ); EDA_ITEM* GetItem() const { return m_pickedItem; } - void SetItem( EDA_ITEM* aItem ); - KICAD_T GetItemType() const { return m_pickedItemType; } - void SetStatus( UNDO_REDO aStatus ) { m_undoRedoStatus = aStatus; } - UNDO_REDO GetStatus() const { return m_undoRedoStatus; } void SetFlags( EDA_ITEM_FLAGS aFlags ) { m_pickerFlags = aFlags; } - EDA_ITEM_FLAGS GetFlags() const { return m_pickerFlags; } - void SetLink( EDA_ITEM* aItem ) { m_link = aItem; } - + void SetLink( EDA_ITEM* aItem ); EDA_ITEM* GetLink() const { return m_link; } KIID GetGroupId() const { return m_groupId; } - - void SetGroupId( KIID aId ) { m_groupId = aId; } + KIID_VECT_LIST GetGroupMembers() const { return m_groupMembers; } BASE_SCREEN* GetScreen() const { return m_screen; } @@ -122,8 +111,8 @@ private: * duplicate) m_Item points the duplicate (i.e the old * copy of an active item) and m_Link points the active * item in schematic */ - KIID m_groupId; /* Id of the group of items in case this is a - * group/ungroup command */ + KIID m_groupId; /* Id of the parent group */ + KIID_VECT_LIST m_groupMembers; /* Ids of the members of a group */ BASE_SCREEN* m_screen; /* For new and deleted items the screen the item should * be added to/removed from. */ @@ -159,14 +148,6 @@ public: */ bool ContainsItem( const EDA_ITEM* aItem ) const; - /** - * Check the undo/redo list for any #EDA_ITEM of type \a aItemType. - * - * @param aItemType is an #EDA_ITEM type from the list of #KICAD_T types. - * @return true if an item of \a aItemType is found in the pick list. - */ - bool ContainsItemType( KICAD_T aItemType ) const; - /** * @return Index of the searched item. If the item is not stored in the list, negative value * is returned. @@ -206,7 +187,8 @@ public: * @param aIdx Index of the picker in the picked list if this picker does not exist, * a picker is returned, with its members set to 0 or NULL. */ - ITEM_PICKER GetItemWrapper( unsigned int aIdx ) const; + const ITEM_PICKER& GetItemWrapper( unsigned int aIdx ) const; + ITEM_PICKER& GetItemWrapper( unsigned int aIdx ); /** * @return A pointer to the picked item. @@ -233,12 +215,6 @@ public: */ UNDO_REDO GetPickedItemStatus( unsigned int aIdx ) const; - /** - * @return The group id of the picked item, or null KIID if does not exist. - * @param aIdx Index of the picked item in the picked list. - */ - KIID GetPickedItemGroupId( unsigned int aIdx ) const; - /** * Return the value of the picker flag. * @@ -254,14 +230,6 @@ public: */ bool SetPickedItem( EDA_ITEM* aItem, unsigned aIdx ); - /** - * @param aItem A pointer to the item to pick. - * @param aStatus The type of undo/redo operation associated to the item to pick. - * @param aIdx Index of the picker in the picked list. - * @return True if the picker exists or false if does not exist. - */ - bool SetPickedItem( EDA_ITEM* aItem, UNDO_REDO aStatus, unsigned aIdx ); - /** * Set the link associated to a given picked item. * @@ -271,15 +239,6 @@ public: */ bool SetPickedItemLink( EDA_ITEM* aLink, unsigned aIdx ); - /** - * Set the group id associated to a given picked item. - * - * @param aId is the group id to associate to the picked item. - * @param aIdx is index of the picker in the picked list. - * @return true if the picker exists, or false if does not exist. - */ - bool SetPickedItemGroupId( KIID aId, unsigned aIdx ); - /** * Set the type of undo/redo operation for a given picked item. * diff --git a/pcbnew/api/api_handler_pcb.cpp b/pcbnew/api/api_handler_pcb.cpp index 332d777195..9d35c56b96 100644 --- a/pcbnew/api/api_handler_pcb.cpp +++ b/pcbnew/api/api_handler_pcb.cpp @@ -270,9 +270,9 @@ std::unique_ptr API_HANDLER_PCB::createCommit() std::optional API_HANDLER_PCB::getItemById( const KIID& aId ) const { - BOARD_ITEM* item = frame()->GetBoard()->GetItem( aId ); + BOARD_ITEM* item = frame()->GetBoard()->ResolveItem( aId, true ); - if( item == DELETED_BOARD_ITEM::GetInstance() ) + if( !item ) return std::nullopt; return item; @@ -649,7 +649,7 @@ void API_HANDLER_PCB::deleteItemsInternal( std::map& a for( std::pair pair : aItemsToDelete ) { - if( BOARD_ITEM* item = board->GetItem( pair.first ) ) + if( BOARD_ITEM* item = board->ResolveItem( pair.first, true ) ) { validatedItems.push_back( item ); aItemsToDelete[pair.first] = ItemDeletionStatus::IDS_OK; diff --git a/pcbnew/board.cpp b/pcbnew/board.cpp index 54a0a3d125..1e329d75ec 100644 --- a/pcbnew/board.cpp +++ b/pcbnew/board.cpp @@ -147,19 +147,6 @@ BOARD::BOARD() : BOARD::~BOARD() { - // Untangle group parents before doing any deleting - for( PCB_GROUP* group : m_groups ) - { - for( EDA_ITEM* item : group->GetItems() ) - item->SetParentGroup( nullptr ); - } - - for( PCB_GENERATOR* generator : m_generators ) - { - for( EDA_ITEM* item : generator->GetItems() ) - item->SetParentGroup( nullptr ); - } - m_itemByIdCache.clear(); // Clean up the owned elements @@ -390,7 +377,7 @@ std::vector BOARD::ResolveDRCExclusions( bool aCreateMarkers ) // Check to see if items still exist for( const KIID& guid : marker->GetRCItem()->GetIDs() ) { - if( GetItem( guid ) == DELETED_BOARD_ITEM::GetInstance() ) + if( !ResolveItem( guid, true ) ) { delete marker; marker = nullptr; @@ -444,9 +431,9 @@ bool BOARD::ResolveTextVar( wxString* token, int aDepth ) const { if( token->Contains( ':' ) ) { - wxString remainder; - wxString ref = token->BeforeFirst( ':', &remainder ); - BOARD_ITEM* refItem = GetItem( KIID( ref ) ); + wxString remainder; + wxString ref = token->BeforeFirst( ':', &remainder ); + BOARD_ITEM* refItem = ResolveItem( KIID( ref ), true ); if( refItem && refItem->Type() == PCB_FOOTPRINT_T ) { @@ -1322,11 +1309,6 @@ void BOARD::Remove( BOARD_ITEM* aBoardItem, REMOVE_MODE aRemoveMode ) aBoardItem->SetFlags( STRUCT_DELETED ); - EDA_GROUP* parentGroup = aBoardItem->GetParentGroup(); - - if( parentGroup && !( parentGroup->AsEdaItem()->GetFlags() & STRUCT_DELETED ) ) - parentGroup->RemoveItem( aBoardItem ); - m_connectivity->Remove( aBoardItem ); if( aRemoveMode != REMOVE_MODE::BULK ) @@ -1509,7 +1491,7 @@ void BOARD::DetachAllFootprints() } -BOARD_ITEM* BOARD::GetItem( const KIID& aID ) const +BOARD_ITEM* BOARD::ResolveItem( const KIID& aID, bool aAllowNullptrReturn ) const { if( aID == niluuid ) return nullptr; @@ -1517,6 +1499,22 @@ BOARD_ITEM* BOARD::GetItem( const KIID& aID ) const if( m_itemByIdCache.count( aID ) ) return m_itemByIdCache.at( aID ); + // Main clients include highlighting, group undo/redo and DRC items. Since + // everything but group undo/redo will be spread over all object types, we + // might as well prioritize group undo/redo and search them first. + + for( PCB_GROUP* group : m_groups ) + { + if( group->m_Uuid == aID ) + return group; + } + + for( PCB_GENERATOR* generator : m_generators ) + { + if( generator->m_Uuid == aID ) + return generator; + } + for( PCB_TRACK* track : Tracks() ) { if( track->m_Uuid == aID ) @@ -1586,19 +1584,6 @@ BOARD_ITEM* BOARD::GetItem( const KIID& aID ) const return marker; } - for( PCB_GROUP* group : m_groups ) - { - if( group->m_Uuid == aID ) - return group; - } - - for( PCB_GENERATOR* generator : m_generators ) - { - if( generator->m_Uuid == aID ) - return generator; - } - - for( NETINFO_ITEM* netInfo : m_NetInfo ) { if( netInfo->m_Uuid == aID ) @@ -1609,7 +1594,10 @@ BOARD_ITEM* BOARD::GetItem( const KIID& aID ) const return const_cast( this ); // Not found; weak reference has been deleted. - return DELETED_BOARD_ITEM::GetInstance(); + if( aAllowNullptrReturn ) + return nullptr; + else + return DELETED_BOARD_ITEM::GetInstance(); } @@ -1733,7 +1721,7 @@ wxString BOARD::ConvertKIIDsToCrossReferences( const wxString& aSource ) const { wxString remainder; wxString ref = token.BeforeFirst( ':', &remainder ); - BOARD_ITEM* refItem = GetItem( KIID( ref ) ); + BOARD_ITEM* refItem = ResolveItem( KIID( ref ), true ); if( refItem && refItem->Type() == PCB_FOOTPRINT_T ) { diff --git a/pcbnew/board.h b/pcbnew/board.h index 2a1138191c..bac2ec3146 100644 --- a/pcbnew/board.h +++ b/pcbnew/board.h @@ -476,9 +476,10 @@ public: void DetachAllFootprints(); /** - * @return null if aID is null. Returns an object of Type() == NOT_USED if the aID is not found. + * @return null if aID is null. If \a aID cannot be resolved, returns either an object of + * Type() == NOT_USED or null, depending on \a aAllowNullptrReturn. */ - BOARD_ITEM* GetItem( const KIID& aID ) const; + BOARD_ITEM* ResolveItem( const KIID& aID, bool aAllowNullptrReturn = false ) const; void FillItemMap( std::map& aMap ); diff --git a/pcbnew/board_commit.cpp b/pcbnew/board_commit.cpp index d531210500..3e2b1dcbe3 100644 --- a/pcbnew/board_commit.cpp +++ b/pcbnew/board_commit.cpp @@ -94,20 +94,15 @@ BOARD* BOARD_COMMIT::GetBoard() const } -COMMIT& BOARD_COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aScreen ) +COMMIT& BOARD_COMMIT::Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, BASE_SCREEN* aScreen, + RECURSE_MODE aRecurse ) { - // Many operations (move, rotate, etc.) are applied directly to a group's children, so they - // must be staged as well. - if( aChangeType == CHT_MODIFY ) + if( aRecurse == RECURSE_MODE::RECURSE ) { if( PCB_GROUP* group = dynamic_cast( aItem ) ) { - group->RunOnChildren( - [&]( BOARD_ITEM* child ) - { - Stage( child, aChangeType ); - }, - RECURSE_MODE::NO_RECURSE ); + for( EDA_ITEM* member : group->GetItems() ) + Stage( member, aChangeType, aScreen, aRecurse ); } } @@ -175,6 +170,7 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) BOARD* board = static_cast( m_toolMgr->GetModel() ); PCB_BASE_FRAME* frame = dynamic_cast( m_toolMgr->GetToolHolder() ); PCB_SELECTION_TOOL* selTool = m_toolMgr->GetTool(); + PCB_GROUP* enteredGroup = selTool ? selTool->GetEnteredGroup() : nullptr; // Notification info PICKED_ITEMS_LIST undoList; @@ -186,7 +182,6 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) bool autofillZones = false; std::vector staleTeardropPadsAndVias; std::set staleTeardropTracks; - PCB_GROUP* addedGroup = nullptr; std::vector staleZonesStorage; std::vector* staleZones = nullptr; @@ -269,14 +264,21 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) if( !staleTeardropPadsAndVias.empty() || !staleTeardropTracks.empty() ) teardropMgr.RemoveTeardrops( *this, &staleTeardropPadsAndVias, &staleTeardropTracks ); - auto updateComponentClasses = [this]( BOARD_ITEM* boardItem ) - { - if( boardItem->Type() != PCB_FOOTPRINT_T ) - return; + auto updateComponentClasses = + [this]( BOARD_ITEM* boardItem ) + { + if( boardItem->Type() != PCB_FOOTPRINT_T ) + return; + + FOOTPRINT* footprint = static_cast( boardItem ); + GetBoard()->GetComponentClassManager().RebuildRequiredCaches( footprint ); + }; + + // We don't know that anything will be added to the entered group, but it does no harm to + // add it to the commit anyway. + if( enteredGroup ) + Modify( enteredGroup ); - FOOTPRINT* footprint = static_cast( boardItem ); - GetBoard()->GetComponentClassManager().RebuildRequiredCaches( footprint ); - }; for( COMMIT_LINE& ent : m_changes ) { @@ -290,11 +292,8 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) switch( changeType ) { case CHT_ADD: - if( selTool && selTool->GetEnteredGroup() && !boardItem->GetParentGroup() - && PCB_GROUP::IsGroupableType( boardItem->Type() ) ) - { - selTool->GetEnteredGroup()->AddItem( boardItem ); - } + if( enteredGroup && boardItem->IsGroupableType() && !boardItem->GetParentGroup() ) + enteredGroup->AddItem( boardItem ); if( !( aCommitFlags & SKIP_UNDO ) ) undoList.PushItem( ITEM_PICKER( nullptr, boardItem, UNDO_REDO::NEWITEM ) ); @@ -319,12 +318,6 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) } } - if( boardItem->Type() == PCB_GROUP_T || boardItem->Type() == PCB_GENERATOR_T ) - { - wxASSERT_MSG( !addedGroup, "Multiple groups in a single commit. This is not supported." ); - addedGroup = static_cast( boardItem ); - } - if( boardItem->Type() != PCB_MARKER_T ) propagateDamage( boardItem, staleZones ); @@ -341,7 +334,12 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) EDA_GROUP* parentGroup = boardItem->GetParentGroup(); if( !( aCommitFlags & SKIP_UNDO ) ) - undoList.PushItem( ITEM_PICKER( nullptr, boardItem, UNDO_REDO::DELETED ) ); + { + ITEM_PICKER itemWrapper( nullptr, boardItem, UNDO_REDO::DELETED ); + itemWrapper.SetLink( ent.m_copy ); + ent.m_copy = nullptr; // We've transferred ownership to the undo list + undoList.PushItem( itemWrapper ); + } if( boardItem->IsSelected() ) { @@ -417,65 +415,27 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) // The item has been removed from the board; it is now owned by undo/redo. boardItem->SetFlags( UR_TRANSIENT ); - break; } - case CHT_UNGROUP: - if( EDA_GROUP* group = boardItem->GetParentGroup() ) - { - if( !( aCommitFlags & SKIP_UNDO ) ) - { - ITEM_PICKER itemWrapper( nullptr, boardItem, UNDO_REDO::UNGROUP ); - itemWrapper.SetGroupId( group->AsEdaItem()->m_Uuid ); - undoList.PushItem( itemWrapper ); - } - - group->RemoveItem( boardItem ); - } - - break; - - case CHT_GROUP: - if( addedGroup ) - { - addedGroup->AddItem( boardItem ); - - if( !( aCommitFlags & SKIP_UNDO ) ) - undoList.PushItem( ITEM_PICKER( nullptr, boardItem, UNDO_REDO::REGROUP ) ); - } - - break; - case CHT_MODIFY: { - BOARD_ITEM* boardItemCopy = nullptr; - - if( ent.m_copy && ent.m_copy->IsBOARD_ITEM() ) - boardItemCopy = static_cast( ent.m_copy ); + BOARD_ITEM* boardItemCopy = dynamic_cast( ent.m_copy ); if( !( aCommitFlags & SKIP_UNDO ) ) { ITEM_PICKER itemWrapper( nullptr, boardItem, UNDO_REDO::CHANGED ); - wxASSERT( boardItemCopy ); - itemWrapper.SetLink( boardItemCopy ); + itemWrapper.SetLink( ent.m_copy ); + ent.m_copy = nullptr; // We've transferred ownership to the undo list undoList.PushItem( itemWrapper ); } if( !( aCommitFlags & SKIP_CONNECTIVITY ) ) { - if( boardItemCopy ) - connectivity->MarkItemNetAsDirty( boardItemCopy ); - + connectivity->MarkItemNetAsDirty( boardItemCopy ); connectivity->Update( boardItem ); } - if( boardItem->Type() == PCB_GROUP_T || boardItem->Type() == PCB_GENERATOR_T ) - { - wxASSERT_MSG( !addedGroup, "Multiple groups in a single commit. This is not supported." ); - addedGroup = static_cast( boardItem ); - } - if( boardItem->Type() != PCB_MARKER_T ) { propagateDamage( boardItemCopy, staleZones ); // before @@ -488,11 +448,6 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) view->Update( boardItem ); itemsChanged.push_back( boardItem ); - - // if no undo entry is needed, the copy would create a memory leak - if( aCommitFlags & SKIP_UNDO ) - delete ent.m_copy; - break; } @@ -501,6 +456,10 @@ void BOARD_COMMIT::Push( const wxString& aMessage, int aCommitFlags ) break; } + // Delete any copies we still have ownership of + delete ent.m_copy; + ent.m_copy = nullptr; + boardItem->ClearEditFlags(); boardItem->RunOnChildren( [&]( BOARD_ITEM* item ) @@ -656,10 +615,6 @@ EDA_ITEM* BOARD_COMMIT::makeImage( EDA_ITEM* aItem ) const EDA_ITEM* BOARD_COMMIT::MakeImage( EDA_ITEM* aItem ) { EDA_ITEM* clone = aItem->Clone(); - - if( clone->IsBOARD_ITEM() ) - static_cast( clone )->SetParentGroup( nullptr ); - clone->SetFlags( UR_TRANSIENT ); return clone; @@ -700,14 +655,6 @@ void BOARD_COMMIT::Revert() switch( changeType ) { case CHT_ADD: - // Items are auto-added to the parent group by BOARD_ITEM::Duplicate(), not when - // the commit is pushed. - if( EDA_GROUP* parentGroup = boardItem->GetParentGroup() ) - { - if( GetStatus( parentGroup->AsEdaItem() ) == 0 ) - parentGroup->RemoveItem( boardItem ); - } - if( !( changeFlags & CHT_DONE ) ) break; @@ -732,18 +679,17 @@ void BOARD_COMMIT::Revert() view->Add( boardItem ); - // Note: parent can be nullptr, because entry.m_parent is only set for children - // of footprints. - BOARD_ITEM* parent = board->GetItem( entry.m_parent ); - - if( parent && parent->Type() == PCB_FOOTPRINT_T ) + if( BOARD_ITEM* parent = board->ResolveItem( entry.m_parent, true ) ) { - static_cast( parent )->Add( boardItem, ADD_MODE::INSERT ); - } - else - { - board->Add( boardItem, ADD_MODE::INSERT ); - bulkAddedItems.push_back( boardItem ); + if( parent->Type() == PCB_FOOTPRINT_T ) + { + static_cast( parent )->Add( boardItem, ADD_MODE::INSERT ); + } + else + { + board->Add( boardItem, ADD_MODE::INSERT ); + bulkAddedItems.push_back( boardItem ); + } } updateComponentClasses( boardItem ); diff --git a/pcbnew/board_commit.h b/pcbnew/board_commit.h index 0ea127a332..20f209f4f2 100644 --- a/pcbnew/board_commit.h +++ b/pcbnew/board_commit.h @@ -61,7 +61,8 @@ public: virtual void Revert() override; COMMIT& Stage( EDA_ITEM* aItem, CHANGE_TYPE aChangeType, - BASE_SCREEN* aScreen = nullptr ) override; + BASE_SCREEN* aScreen = nullptr, + RECURSE_MODE aRecurse = RECURSE_MODE::NO_RECURSE ) override; COMMIT& Stage( std::vector& container, CHANGE_TYPE aChangeType, BASE_SCREEN* aScreen = nullptr ) override; COMMIT& Stage( const PICKED_ITEMS_LIST& aItems, diff --git a/pcbnew/board_item.cpp b/pcbnew/board_item.cpp index 396025bf84..333ec8685a 100644 --- a/pcbnew/board_item.cpp +++ b/pcbnew/board_item.cpp @@ -38,6 +38,37 @@ #include +bool BOARD_ITEM::IsGroupableType() const +{ + switch ( Type() ) + { + case PCB_FOOTPRINT_T: + case PCB_PAD_T: + case PCB_SHAPE_T: + case PCB_REFERENCE_IMAGE_T: + case PCB_FIELD_T: + case PCB_TEXT_T: + case PCB_TEXTBOX_T: + case PCB_TABLE_T: + case PCB_GROUP_T: + case PCB_GENERATOR_T: + case PCB_TRACE_T: + case PCB_VIA_T: + case PCB_ARC_T: + case PCB_DIMENSION_T: + case PCB_DIM_ALIGNED_T: + case PCB_DIM_LEADER_T: + case PCB_DIM_CENTER_T: + case PCB_DIM_RADIAL_T: + case PCB_DIM_ORTHOGONAL_T: + case PCB_ZONE_T: + return true; + default: + return false; + } +} + + void BOARD_ITEM::CopyFrom( const BOARD_ITEM* aOther ) { wxCHECK( aOther, /* void */ ); @@ -50,12 +81,7 @@ const BOARD* BOARD_ITEM::GetBoard() const if( Type() == PCB_T ) return static_cast( this ); - BOARD_ITEM* parent = GetParent(); - - if( parent ) - return parent->GetBoard(); - - return nullptr; + return static_cast( findParent( PCB_T ) ); } @@ -64,23 +90,28 @@ BOARD* BOARD_ITEM::GetBoard() if( Type() == PCB_T ) return static_cast( this ); - BOARD_ITEM* parent = GetParent(); + return static_cast( findParent( PCB_T ) ); +} - if( parent ) - return parent->GetBoard(); - return nullptr; +FOOTPRINT* BOARD_ITEM::GetParentFootprint() const +{ + return static_cast( findParent( PCB_FOOTPRINT_T ) ); } bool BOARD_ITEM::IsLocked() const { - if( GetParentGroup() && static_cast( GetParentGroup() )->IsLocked() ) - return true; + if( EDA_GROUP* group = GetParentGroup() ) + { + if( group->AsEdaItem()->IsLocked() ) + return true; + } - const BOARD* board = GetBoard(); + if( !GetBoard() || GetBoard()->GetBoardUse() == BOARD_USE::FPHOLDER ) + return false; - return board && board->GetBoardUse() != BOARD_USE::FPHOLDER && m_isLocked; + return m_isLocked; } @@ -220,6 +251,7 @@ void BOARD_ITEM::DeleteStructure() void BOARD_ITEM::swapData( BOARD_ITEM* aImage ) { + UNIMPLEMENTED_FOR( GetClass() ); } @@ -228,26 +260,29 @@ void BOARD_ITEM::SwapItemData( BOARD_ITEM* aImage ) if( aImage == nullptr ) return; - EDA_ITEM* parent = GetParent(); - EDA_GROUP* group = GetParentGroup(); + EDA_ITEM* parent = GetParent(); - SetParentGroup( nullptr ); - aImage->SetParentGroup( nullptr ); swapData( aImage ); - // Restore pointers to be sure they are not broken SetParent( parent ); - SetParentGroup( group ); } -BOARD_ITEM* BOARD_ITEM::Duplicate() const +BOARD_ITEM* BOARD_ITEM::Duplicate( bool addToParentGroup, BOARD_COMMIT* aCommit ) const { BOARD_ITEM* dupe = static_cast( Clone() ); const_cast( dupe->m_Uuid ) = KIID(); - if( dupe->GetParentGroup() ) - dupe->GetParentGroup()->AddItem( dupe ); + if( addToParentGroup ) + { + wxCHECK_MSG( aCommit, dupe, "Must supply a commit to update parent group" ); + + if( dupe->GetParentGroup() ) + { + aCommit->Modify( dupe->GetParentGroup()->AsEdaItem() ); + dupe->GetParentGroup()->AddItem( dupe ); + } + } return dupe; } @@ -296,35 +331,6 @@ std::shared_ptr BOARD_ITEM::GetEffectiveHoleShape() const } -FOOTPRINT* BOARD_ITEM::GetParentFootprint() const -{ - // EDA_ITEM::IsType is too slow here. - auto isContainer = []( BOARD_ITEM_CONTAINER* aTest ) - { - switch( aTest->Type() ) - { - case PCB_GROUP_T: - case PCB_GENERATOR_T: - case PCB_TABLE_T: - return true; - - default: - return false; - } - }; - - BOARD_ITEM_CONTAINER* ancestor = GetParent(); - - while( ancestor && isContainer( ancestor ) ) - ancestor = ancestor->GetParent(); - - if( ancestor && ancestor->Type() == PCB_FOOTPRINT_T ) - return static_cast( ancestor ); - - return nullptr; -} - - VECTOR2I BOARD_ITEM::GetFPRelativePosition() const { VECTOR2I pos = GetPosition(); diff --git a/pcbnew/cross-probing.cpp b/pcbnew/cross-probing.cpp index 45653a2258..5d80d15715 100644 --- a/pcbnew/cross-probing.cpp +++ b/pcbnew/cross-probing.cpp @@ -321,7 +321,6 @@ void collectItemsForSyncParts( ItemContainer& aItems, std::set& parts PCB_GROUP* group = static_cast( item ); collectItemsForSyncParts( group->GetItems(), parts ); - break; } case PCB_FOOTPRINT_T: @@ -330,7 +329,6 @@ void collectItemsForSyncParts( ItemContainer& aItems, std::set& parts wxString ref = footprint->GetReference(); parts.emplace( wxT( "F" ) + EscapeString( ref, CTX_IPC ) ); - break; } @@ -341,7 +339,6 @@ void collectItemsForSyncParts( ItemContainer& aItems, std::set& parts parts.emplace( wxT( "P" ) + EscapeString( ref, CTX_IPC ) + wxT( "/" ) + EscapeString( pad->GetNumber(), CTX_IPC ) ); - break; } diff --git a/pcbnew/dialogs/dialog_cleanup_graphics.cpp b/pcbnew/dialogs/dialog_cleanup_graphics.cpp index bf8765a3c6..13bcf8a3e7 100644 --- a/pcbnew/dialogs/dialog_cleanup_graphics.cpp +++ b/pcbnew/dialogs/dialog_cleanup_graphics.cpp @@ -139,15 +139,18 @@ void DIALOG_CLEANUP_GRAPHICS::doCleanup( bool aDryRun ) void DIALOG_CLEANUP_GRAPHICS::OnSelectItem( wxDataViewEvent& aEvent ) { - const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); - BOARD_ITEM* item = m_parentFrame->GetBoard()->GetItem( itemID ); - WINDOW_THAWER thawer( m_parentFrame ); + const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); - if( item && !item->GetLayerSet().test( m_parentFrame->GetActiveLayer() ) ) - m_parentFrame->SetActiveLayer( item->GetLayerSet().UIOrder().front() ); + if( BOARD_ITEM* item = m_parentFrame->GetBoard()->ResolveItem( itemID, true ) ) + { + WINDOW_THAWER thawer( m_parentFrame ); - m_parentFrame->FocusOnItem( item ); - m_parentFrame->GetCanvas()->Refresh(); + if( !item->GetLayerSet().test( m_parentFrame->GetActiveLayer() ) ) + m_parentFrame->SetActiveLayer( item->GetLayerSet().UIOrder().front() ); + + m_parentFrame->FocusOnItem( item ); + m_parentFrame->GetCanvas()->Refresh(); + } aEvent.Skip(); } diff --git a/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp b/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp index 06095900bb..c68f06ed6c 100644 --- a/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp +++ b/pcbnew/dialogs/dialog_cleanup_tracks_and_vias.cpp @@ -313,12 +313,15 @@ void DIALOG_CLEANUP_TRACKS_AND_VIAS::doCleanup( bool aDryRun ) void DIALOG_CLEANUP_TRACKS_AND_VIAS::OnSelectItem( wxDataViewEvent& aEvent ) { - const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); - BOARD_ITEM* item = m_brd->GetItem( itemID ); - WINDOW_THAWER thawer( m_parentFrame ); + const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); - m_parentFrame->FocusOnItem( item ); - m_parentFrame->GetCanvas()->Refresh(); + if( BOARD_ITEM* item = m_brd->ResolveItem( itemID, true ) ) + { + WINDOW_THAWER thawer( m_parentFrame ); + + m_parentFrame->FocusOnItem( item ); + m_parentFrame->GetCanvas()->Refresh(); + } aEvent.Skip(); } diff --git a/pcbnew/dialogs/dialog_drc.cpp b/pcbnew/dialogs/dialog_drc.cpp index 53b460c42f..f3d4b633fd 100644 --- a/pcbnew/dialogs/dialog_drc.cpp +++ b/pcbnew/dialogs/dialog_drc.cpp @@ -462,22 +462,21 @@ void DIALOG_DRC::OnDRCItemSelected( wxDataViewEvent& aEvent ) } const KIID& itemID = RC_TREE_MODEL::ToUUID( aEvent.GetItem() ); - BOARD_ITEM* item = board->GetItem( itemID ); + BOARD_ITEM* item = board->ResolveItem( itemID, true ); - if( !item || item == DELETED_BOARD_ITEM::GetInstance() ) + if( item ) { // nothing to highlight / focus on - aEvent.Skip(); return; } PCB_LAYER_ID principalLayer; LSET violationLayers; - BOARD_ITEM* a = board->GetItem( rc_item->GetMainItemID() ); - BOARD_ITEM* b = board->GetItem( rc_item->GetAuxItemID() ); - BOARD_ITEM* c = board->GetItem( rc_item->GetAuxItem2ID() ); - BOARD_ITEM* d = board->GetItem( rc_item->GetAuxItem3ID() ); + BOARD_ITEM* a = board->ResolveItem( rc_item->GetMainItemID(), true ); + BOARD_ITEM* b = board->ResolveItem( rc_item->GetAuxItemID(), true ); + BOARD_ITEM* c = board->ResolveItem( rc_item->GetAuxItem2ID(), true ); + BOARD_ITEM* d = board->ResolveItem( rc_item->GetAuxItem3ID(), true ); if( rc_item->GetErrorCode() == DRCE_MALFORMED_COURTYARD ) { @@ -491,7 +490,7 @@ void DIALOG_DRC::OnDRCItemSelected( wxDataViewEvent& aEvent ) principalLayer = F_CrtYd; } } - else if (rc_item->GetErrorCode() == DRCE_INVALID_OUTLINE ) + else if( rc_item->GetErrorCode() == DRCE_INVALID_OUTLINE ) { principalLayer = Edge_Cuts; } @@ -596,7 +595,7 @@ void DIALOG_DRC::OnDRCItemSelected( wxDataViewEvent& aEvent ) for( const KIID& id : rc_item->GetIDs() ) { - auto* candidate = dynamic_cast( board->GetItem( id ) ); + auto* candidate = dynamic_cast( board->ResolveItem( id, true ) ); if( candidate && candidate->GetNetCode() == net ) items.push_back( candidate ); diff --git a/pcbnew/dialogs/dialog_footprint_checker.cpp b/pcbnew/dialogs/dialog_footprint_checker.cpp index b8f23e4ce4..1916a4019b 100644 --- a/pcbnew/dialogs/dialog_footprint_checker.cpp +++ b/pcbnew/dialogs/dialog_footprint_checker.cpp @@ -235,7 +235,7 @@ void DIALOG_FOOTPRINT_CHECKER::OnSelectItem( wxDataViewEvent& aEvent ) BOARD* board = m_frame->GetBoard(); RC_TREE_NODE* node = RC_TREE_MODEL::ToNode( aEvent.GetItem() ); const KIID& itemID = node ? RC_TREE_MODEL::ToUUID( aEvent.GetItem() ) : niluuid; - BOARD_ITEM* item = board->GetItem( itemID ); + BOARD_ITEM* item = board->ResolveItem( itemID, true ); if( m_centerMarkerOnIdle ) { @@ -257,7 +257,7 @@ void DIALOG_FOOTPRINT_CHECKER::OnSelectItem( wxDataViewEvent& aEvent ) if( rc_item->GetErrorCode() == DRCE_MALFORMED_COURTYARD ) { - BOARD_ITEM* a = board->GetItem( rc_item->GetMainItemID() ); + BOARD_ITEM* a = board->ResolveItem( rc_item->GetMainItemID(), true ); if( a && ( a->GetFlags() & MALFORMED_B_COURTYARD ) > 0 && ( a->GetFlags() & MALFORMED_F_COURTYARD ) == 0 ) @@ -275,10 +275,10 @@ void DIALOG_FOOTPRINT_CHECKER::OnSelectItem( wxDataViewEvent& aEvent ) } else { - BOARD_ITEM* a = board->GetItem( rc_item->GetMainItemID() ); - BOARD_ITEM* b = board->GetItem( rc_item->GetAuxItemID() ); - BOARD_ITEM* c = board->GetItem( rc_item->GetAuxItem2ID() ); - BOARD_ITEM* d = board->GetItem( rc_item->GetAuxItem3ID() ); + BOARD_ITEM* a = board->ResolveItem( rc_item->GetMainItemID(), true ); + BOARD_ITEM* b = board->ResolveItem( rc_item->GetAuxItemID(), true ); + BOARD_ITEM* c = board->ResolveItem( rc_item->GetAuxItem2ID(), true ); + BOARD_ITEM* d = board->ResolveItem( rc_item->GetAuxItem3ID(), true ); if( a || b || c || d ) violationLayers = LSET::AllLayersMask(); diff --git a/pcbnew/dialogs/dialog_generators.cpp b/pcbnew/dialogs/dialog_generators.cpp index 6dc0bda76e..dc25d181de 100644 --- a/pcbnew/dialogs/dialog_generators.cpp +++ b/pcbnew/dialogs/dialog_generators.cpp @@ -410,7 +410,7 @@ void DIALOG_GENERATORS::OnItemSelected( wxDataViewEvent& aEvent ) wxVariant var; model->GetValue( var, viewItem, uuidCol ); - BOARD_ITEM* brdItem = m_currentBoard->GetItem( var.GetString() ); + BOARD_ITEM* brdItem = m_currentBoard->ResolveItem( var.GetString() ); if( !brdItem || brdItem->Type() != KICAD_T::PCB_GENERATOR_T ) continue; @@ -447,7 +447,7 @@ void DIALOG_GENERATORS::OnRebuildTypeClick( wxCommandEvent& event ) wxVariant var; model->GetValueByRow( var, row, uuidCol ); - BOARD_ITEM* item = m_currentBoard->GetItem( var.GetString() ); + BOARD_ITEM* item = m_currentBoard->ResolveItem( var.GetString() ); if( !item || item->Type() != KICAD_T::PCB_GENERATOR_T ) continue; diff --git a/pcbnew/dialogs/dialog_global_deletion.cpp b/pcbnew/dialogs/dialog_global_deletion.cpp index 8a502aaa8f..04d71373c1 100644 --- a/pcbnew/dialogs/dialog_global_deletion.cpp +++ b/pcbnew/dialogs/dialog_global_deletion.cpp @@ -134,12 +134,7 @@ void DIALOG_GLOBAL_DELETION::DoGlobalDeletions() [&]( BOARD_ITEM* item, const LSET& layers_mask ) { if( ( item->GetLayerSet() & layers_mask ).any() ) - { - if( item->GetParentGroup() ) - commit.Stage( item, CHT_UNGROUP ); - commit.Remove( item ); - } }; auto processConnectedItem = @@ -147,9 +142,6 @@ void DIALOG_GLOBAL_DELETION::DoGlobalDeletions() { if( ( item->GetLayerSet() & layers_mask ).any() ) { - if( item->GetParentGroup() ) - commit.Stage( item, CHT_UNGROUP ); - commit.Remove( item ); gen_rastnest = true; } diff --git a/pcbnew/dialogs/dialog_pad_properties.cpp b/pcbnew/dialogs/dialog_pad_properties.cpp index 21b2484932..e33c095168 100644 --- a/pcbnew/dialogs/dialog_pad_properties.cpp +++ b/pcbnew/dialogs/dialog_pad_properties.cpp @@ -291,10 +291,6 @@ DIALOG_PAD_PROPERTIES::~DIALOG_PAD_PROPERTIES() m_page = m_notebook->GetSelection(); - // Remove the preview pad from the group of the actual pad before deletion - if( m_previewPad ) - m_previewPad->SetParentGroup( nullptr ); - delete m_previewPad; delete m_axisOrigin; } diff --git a/pcbnew/dialogs/dialog_shape_properties.cpp b/pcbnew/dialogs/dialog_shape_properties.cpp index 5db9471bea..34b6fb2172 100644 --- a/pcbnew/dialogs/dialog_shape_properties.cpp +++ b/pcbnew/dialogs/dialog_shape_properties.cpp @@ -845,9 +845,6 @@ DIALOG_SHAPE_PROPERTIES::DIALOG_SHAPE_PROPERTIES( PCB_BASE_EDIT_FRAME* aParent, m_solderMaskMargin( aParent, m_solderMaskMarginLabel, m_solderMaskMarginCtrl, m_solderMaskMarginUnit ), m_workingCopy( *m_item ) { - m_workingCopy.SetParentGroup( nullptr ); - m_workingCopy.SetParent( nullptr ); - SetTitle( wxString::Format( GetTitle(), m_item->GetFriendlyName() ) ); m_hash_key = TO_UTF8( GetTitle() ); @@ -1135,10 +1132,6 @@ bool DIALOG_SHAPE_PROPERTIES::TransferDataFromWindow() if( !pushCommit ) m_item->SetFlags( IN_EDIT ); - // Ensure parent and parent group item are restored. m_workingCopy had previously - // these parents cleared and not restored for some PCB_SHAPE types (i.e. POLY) - m_workingCopy.SetParentGroup( m_item->GetParentGroup() ); - m_workingCopy.SetParent( m_item->GetParent() ); *m_item = m_workingCopy; bool wasLocked = m_item->IsLocked(); diff --git a/pcbnew/drc/drc_test_provider_edge_clearance.cpp b/pcbnew/drc/drc_test_provider_edge_clearance.cpp index b756ea09c9..b593f03428 100644 --- a/pcbnew/drc/drc_test_provider_edge_clearance.cpp +++ b/pcbnew/drc/drc_test_provider_edge_clearance.cpp @@ -194,22 +194,18 @@ bool DRC_TEST_PROVIDER_EDGE_CLEARANCE::Run() edges.back()->SetShape( SHAPE_T::SEGMENT ); edges.back()->SetEndX( shape->GetStartX() ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); edges.emplace_back( static_cast( shape->Clone() ) ); edges.back()->SetShape( SHAPE_T::SEGMENT ); edges.back()->SetEndY( shape->GetStartY() ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); edges.emplace_back( static_cast( shape->Clone() ) ); edges.back()->SetShape( SHAPE_T::SEGMENT ); edges.back()->SetStartX( shape->GetEndX() ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); edges.emplace_back( static_cast( shape->Clone() ) ); edges.back()->SetShape( SHAPE_T::SEGMENT ); edges.back()->SetStartY( shape->GetEndY() ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); } else if( shape->GetShape() == SHAPE_T::POLY && !shape->IsSolidFill() ) { @@ -225,14 +221,12 @@ bool DRC_TEST_PROVIDER_EDGE_CLEARANCE::Run() edges.back()->SetStart( seg.A ); edges.back()->SetEnd( seg.B ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); } } else { edges.emplace_back( static_cast( shape->Clone() ) ); edges.back()->SetStroke( stroke ); - edges.back()->SetParentGroup( nullptr ); } return true; diff --git a/pcbnew/edit.cpp b/pcbnew/edit.cpp index 21a9a10ac2..16c5a596ec 100644 --- a/pcbnew/edit.cpp +++ b/pcbnew/edit.cpp @@ -155,8 +155,7 @@ void PCB_EDIT_FRAME::OnEditItemRequest( BOARD_ITEM* aItem ) break; case PCB_GROUP_T: - m_toolManager->RunAction( ACTIONS::groupProperties, - static_cast( static_cast( aItem ) ) ); + m_toolManager->RunAction( ACTIONS::groupProperties, static_cast( aItem ) ); break; case PCB_GENERATOR_T: diff --git a/pcbnew/footprint.cpp b/pcbnew/footprint.cpp index 0160086c9e..03aa395cb8 100644 --- a/pcbnew/footprint.cpp +++ b/pcbnew/footprint.cpp @@ -233,13 +233,6 @@ FOOTPRINT::FOOTPRINT( FOOTPRINT&& aFootprint ) : FOOTPRINT::~FOOTPRINT() { - // Untangle group parents before doing any deleting - for( PCB_GROUP* group : m_groups ) - { - for( EDA_ITEM* item : group->GetItems() ) - item->SetParentGroup( nullptr ); - } - // Clean up the owned elements delete m_initial_comments; @@ -1213,11 +1206,6 @@ void FOOTPRINT::Remove( BOARD_ITEM* aBoardItem, REMOVE_MODE aMode ) } aBoardItem->SetFlags( STRUCT_DELETED ); - - EDA_GROUP* parentGroup = aBoardItem->GetParentGroup(); - - if( parentGroup && !( parentGroup->AsEdaItem()->GetFlags() & STRUCT_DELETED ) ) - parentGroup->RemoveItem( aBoardItem ); } @@ -1313,10 +1301,6 @@ BOX2I FOOTPRINT::GetFpPadsLocalBbox() const for( PAD* pad : dummy.Pads() ) bbox.Merge( pad->GetBoundingBox() ); - // Remove the parent and the group from the dummy footprint before deletion - dummy.SetParent( nullptr ); - dummy.SetParentGroup( nullptr ); - return bbox; } @@ -2569,9 +2553,9 @@ void FOOTPRINT::SetOrientation( const EDA_ANGLE& aNewAngle ) } -BOARD_ITEM* FOOTPRINT::Duplicate() const +BOARD_ITEM* FOOTPRINT::Duplicate( bool addToParentGroup, BOARD_COMMIT* aCommit ) const { - FOOTPRINT* dupe = static_cast( BOARD_ITEM::Duplicate() ); + FOOTPRINT* dupe = static_cast( BOARD_ITEM::Duplicate( addToParentGroup, aCommit ) ); dupe->RunOnChildren( [&]( BOARD_ITEM* child ) { @@ -2583,7 +2567,8 @@ BOARD_ITEM* FOOTPRINT::Duplicate() const } -BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootprint ) +BOARD_ITEM* FOOTPRINT::DuplicateItem( bool addToParentGroup, BOARD_COMMIT* aCommit, + const BOARD_ITEM* aItem, bool addToFootprint ) { BOARD_ITEM* new_item = nullptr; @@ -2594,7 +2579,7 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr PAD* new_pad = new PAD( *static_cast( aItem ) ); const_cast( new_pad->m_Uuid ) = KIID(); - if( aAddToFootprint ) + if( addToFootprint ) m_pads.push_back( new_pad ); new_item = new_pad; @@ -2606,7 +2591,7 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr ZONE* new_zone = new ZONE( *static_cast( aItem ) ); const_cast( new_zone->m_Uuid ) = KIID(); - if( aAddToFootprint ) + if( addToFootprint ) m_zones.push_back( new_zone ); new_item = new_zone; @@ -2626,11 +2611,11 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr case FIELD_T::REFERENCE: new_text->SetText( wxT( "${REFERENCE}" ) ); break; case FIELD_T::VALUE: new_text->SetText( wxT( "${VALUE}" ) ); break; case FIELD_T::DATASHEET: new_text->SetText( wxT( "${DATASHEET}" ) ); break; - default: break; + default: break; } } - if( aAddToFootprint ) + if( addToFootprint ) Add( new_text ); new_item = new_text; @@ -2642,7 +2627,7 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr PCB_SHAPE* new_shape = new PCB_SHAPE( *static_cast( aItem ) ); const_cast( new_shape->m_Uuid ) = KIID(); - if( aAddToFootprint ) + if( addToFootprint ) Add( new_shape ); new_item = new_shape; @@ -2654,7 +2639,7 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr PCB_TEXTBOX* new_textbox = new PCB_TEXTBOX( *static_cast( aItem ) ); const_cast( new_textbox->m_Uuid ) = KIID(); - if( aAddToFootprint ) + if( addToFootprint ) Add( new_textbox ); new_item = new_textbox; @@ -2667,9 +2652,10 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr case PCB_DIM_RADIAL_T: case PCB_DIM_ORTHOGONAL_T: { - PCB_DIMENSION_BASE* dimension = static_cast( aItem->Duplicate() ); + PCB_DIMENSION_BASE* dimension = static_cast( aItem->Duplicate( addToParentGroup, + aCommit ) ); - if( aAddToFootprint ) + if( addToFootprint ) Add( dimension ); new_item = dimension; @@ -2678,9 +2664,9 @@ BOARD_ITEM* FOOTPRINT::DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootpr case PCB_GROUP_T: { - PCB_GROUP* group = static_cast( aItem )->DeepDuplicate(); + PCB_GROUP* group = static_cast( aItem )->DeepDuplicate( addToParentGroup, aCommit ); - if( aAddToFootprint ) + if( addToFootprint ) { group->RunOnChildren( [&]( BOARD_ITEM* aCurrItem ) diff --git a/pcbnew/footprint.h b/pcbnew/footprint.h index 8b580b27a3..d8478969ad 100644 --- a/pcbnew/footprint.h +++ b/pcbnew/footprint.h @@ -846,14 +846,15 @@ public: KIID GetLink() const { return m_link; } void SetLink( const KIID& aLink ) { m_link = aLink; } - BOARD_ITEM* Duplicate() const override; + BOARD_ITEM* Duplicate( bool addToParentGroup, BOARD_COMMIT* aCommit = nullptr ) const override; /** * Duplicate a given item within the footprint, optionally adding it to the board. * * @return the new item, or NULL if the item could not be duplicated. */ - BOARD_ITEM* DuplicateItem( const BOARD_ITEM* aItem, bool aAddToFootprint = false ); + BOARD_ITEM* DuplicateItem( bool addToParentGroup, BOARD_COMMIT* aCommit, const BOARD_ITEM* aItem, + bool addToFootprint = false ); /** * Add \a a3DModel definition to the end of the 3D model list. diff --git a/pcbnew/footprint_editor_utils.cpp b/pcbnew/footprint_editor_utils.cpp index b7ccf16424..e45657ac90 100644 --- a/pcbnew/footprint_editor_utils.cpp +++ b/pcbnew/footprint_editor_utils.cpp @@ -244,8 +244,7 @@ void FOOTPRINT_EDIT_FRAME::OnEditItemRequest( BOARD_ITEM* aItem ) } case PCB_GROUP_T: - m_toolManager->RunAction( ACTIONS::groupProperties, - static_cast( static_cast( aItem ) ) ); + m_toolManager->RunAction( ACTIONS::groupProperties, static_cast( aItem ) ); break; case PCB_MARKER_T: diff --git a/pcbnew/footprint_libraries_utils.cpp b/pcbnew/footprint_libraries_utils.cpp index c14cc6a414..e61b436549 100644 --- a/pcbnew/footprint_libraries_utils.cpp +++ b/pcbnew/footprint_libraries_utils.cpp @@ -636,7 +636,7 @@ void PCB_EDIT_FRAME::ExportFootprintsToLibrary( bool aStoreInNewLib, const wxStr if( !footprint->GetFPID().GetLibItemName().empty() ) // Handle old boards. { - FOOTPRINT* fpCopy = static_cast( footprint->Duplicate() ); + FOOTPRINT* fpCopy = static_cast( footprint->Duplicate( IGNORE_PARENT_GROUP ) ); // Reset reference designator and group membership before saving resetReference( fpCopy ); @@ -694,7 +694,7 @@ void PCB_EDIT_FRAME::ExportFootprintsToLibrary( bool aStoreInNewLib, const wxStr { if( !footprint->GetFPID().GetLibItemName().empty() ) // Handle old boards. { - FOOTPRINT* fpCopy = static_cast( footprint->Duplicate() ); + FOOTPRINT* fpCopy = static_cast( footprint->Duplicate( IGNORE_PARENT_GROUP ) ); // Reset reference designator and group membership before saving resetReference( fpCopy ); @@ -743,10 +743,8 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprint( FOOTPRINT* aFootprint ) m_footprintNameWhenLoaded = footprintName; return true; } - else - { - return false; - } + + return false; } else if( libraryName.IsEmpty() || footprintName.IsEmpty() ) { @@ -756,10 +754,8 @@ bool FOOTPRINT_EDIT_FRAME::SaveFootprint( FOOTPRINT* aFootprint ) SyncLibraryTree( true ); return true; } - else - { - return false; - } + + return false; } FP_LIB_TABLE* tbl = PROJECT_PCB::PcbFootprintLibs( &Prj() ); diff --git a/pcbnew/footprint_preview_panel.cpp b/pcbnew/footprint_preview_panel.cpp index 80ffe23cdf..8bdd6d8b9b 100644 --- a/pcbnew/footprint_preview_panel.cpp +++ b/pcbnew/footprint_preview_panel.cpp @@ -182,7 +182,7 @@ bool FOOTPRINT_PREVIEW_PANEL::DisplayFootprint( const LIB_ID& aFPID ) aFPID.GetLibItemName() ); if( fp ) - m_currentFootprint.reset( static_cast( fp->Duplicate() ) ); + m_currentFootprint.reset( static_cast( fp->Duplicate( IGNORE_PARENT_GROUP ) ) ); else m_currentFootprint.reset(); } diff --git a/pcbnew/footprint_viewer_frame.cpp b/pcbnew/footprint_viewer_frame.cpp index fa8ffce640..2212ccda75 100644 --- a/pcbnew/footprint_viewer_frame.cpp +++ b/pcbnew/footprint_viewer_frame.cpp @@ -757,7 +757,7 @@ void FOOTPRINT_VIEWER_FRAME::AddFootprintToPCB() BOARD_COMMIT commit( pcbframe ); // Create the "new" footprint - FOOTPRINT* newFootprint = (FOOTPRINT*) GetBoard()->GetFirstFootprint()->Duplicate(); + FOOTPRINT* newFootprint = (FOOTPRINT*) GetBoard()->GetFirstFootprint()->Duplicate( IGNORE_PARENT_GROUP ); newFootprint->SetParent( pcbframe->GetBoard() ); newFootprint->SetLink( niluuid ); newFootprint->SetFlags(IS_NEW ); // whatever diff --git a/pcbnew/generators/pcb_tuning_pattern.cpp b/pcbnew/generators/pcb_tuning_pattern.cpp index 61558605ba..7e0120727c 100644 --- a/pcbnew/generators/pcb_tuning_pattern.cpp +++ b/pcbnew/generators/pcb_tuning_pattern.cpp @@ -1225,17 +1225,6 @@ void PCB_TUNING_PATTERN::Remove( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_COM PNS::ROUTER* router = aTool->Router(); PNS_KICAD_IFACE* iface = aTool->GetInterface(); - // Ungroup first so that undo works - if( !GetItems().empty() ) - { - PCB_GENERATOR* group = this; - - for( EDA_ITEM* member : group->GetItems() ) - aCommit->Stage( member, CHT_UNGROUP ); - - group->GetItems().clear(); - } - aCommit->Remove( this ); aTool->ClearRouterChanges(); @@ -1637,7 +1626,6 @@ void PCB_TUNING_PATTERN::EditPush( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_C && bounds.PointInside( track->GetEnd(), epsilon ) ) { AddItem( item ); - aCommit->Stage( item, CHT_GROUP ); } } } diff --git a/pcbnew/kicad_clipboard.cpp b/pcbnew/kicad_clipboard.cpp index 7f4e5fcdf0..99d8c3151b 100644 --- a/pcbnew/kicad_clipboard.cpp +++ b/pcbnew/kicad_clipboard.cpp @@ -273,9 +273,10 @@ void CLIPBOARD_IO::SaveSelection( const PCB_SELECTION& aSelected, bool isFootpri // some PCB_TEXT (reference and value) cannot be added to the footprint std::vector skipped_items; - if( copy->Type() == PCB_GROUP_T || copy->Type() == PCB_GENERATOR_T ) + // Will catch at least PCB_GROUP_T and PCB_GENERATOR_T + if( PCB_GROUP* group = dynamic_cast( copy ) ) { - copy->RunOnChildren( + group->RunOnChildren( [&]( BOARD_ITEM* descendant ) { // One cannot add an additional mandatory field to a given footprint: diff --git a/pcbnew/netlist_reader/board_netlist_updater.cpp b/pcbnew/netlist_reader/board_netlist_updater.cpp index 93eda4d9d4..9ffef81f15 100644 --- a/pcbnew/netlist_reader/board_netlist_updater.cpp +++ b/pcbnew/netlist_reader/board_netlist_updater.cpp @@ -864,9 +864,8 @@ bool BOARD_NETLIST_UPDATER::updateFootprintGroup( FOOTPRINT* aPcbFootprint, EscapeHTML( existingGroup->GetName() ) ); changed = true; - m_commit.Modify( aPcbFootprint->GetParentGroup()->AsEdaItem() ); - aPcbFootprint->GetParentGroup()->RemoveItem( aPcbFootprint ); - aPcbFootprint->SetParentGroup( nullptr ); + m_commit.Modify( existingGroup ); + existingGroup->RemoveItem( aPcbFootprint ); } m_reporter->Report( msg, RPT_SEVERITY_ACTION ); @@ -906,7 +905,6 @@ bool BOARD_NETLIST_UPDATER::updateFootprintGroup( FOOTPRINT* aPcbFootprint, } newGroup->AddItem( aPcbFootprint ); - aPcbFootprint->SetParentGroup( newGroup ); } m_reporter->Report( msg, RPT_SEVERITY_ACTION ); @@ -1624,9 +1622,6 @@ bool BOARD_NETLIST_UPDATER::UpdateNetlist( NETLIST& aNetlist ) } else { - if( footprint->GetParentGroup() ) - m_commit.Stage( footprint, CHT_UNGROUP ); - m_commit.Remove( footprint ); msg.Printf( _( "Removed unused footprint %s." ), footprint->GetReference() ); diff --git a/pcbnew/pad.cpp b/pcbnew/pad.cpp index 19d95775f8..1d547e7d58 100644 --- a/pcbnew/pad.cpp +++ b/pcbnew/pad.cpp @@ -2190,7 +2190,7 @@ std::vector PAD::Recombine( bool aIsDryRun, int maxError ) if( EDA_GROUP* group = fpShape->GetParentGroup(); group ) group->RemoveItem( fpShape ); - PCB_SHAPE* primitive = static_cast( fpShape->Duplicate() ); + PCB_SHAPE* primitive = static_cast( fpShape->Duplicate( IGNORE_PARENT_GROUP ) ); primitive->SetParent( nullptr ); diff --git a/pcbnew/pcb_base_frame.cpp b/pcbnew/pcb_base_frame.cpp index 7c6b00b90f..5f8fcfcfe6 100644 --- a/pcbnew/pcb_base_frame.cpp +++ b/pcbnew/pcb_base_frame.cpp @@ -244,9 +244,9 @@ void PCB_BASE_FRAME::AddFootprintToBoard( FOOTPRINT* aFootprint ) } -EDA_ITEM* PCB_BASE_FRAME::GetItem( const KIID& aId ) const +EDA_ITEM* PCB_BASE_FRAME::ResolveItem( const KIID& aId, bool aAllowNullptrReturn ) const { - return GetBoard()->GetItem( aId ); + return GetBoard()->ResolveItem( aId, aAllowNullptrReturn ); } void PCB_BASE_FRAME::FocusOnItem( EDA_ITEM* aItem ) @@ -273,27 +273,9 @@ void PCB_BASE_FRAME::FocusOnItems( std::vector aItems, PCB_LAYER_ID { static std::vector lastBrightenedItemIDs; - BOARD_ITEM* lastItem = nullptr; - for( KIID lastBrightenedItemID : lastBrightenedItemIDs ) { - /// @todo The Boost entropy exception does not exist prior to 1.67. Once the minimum Boost - /// version is raise to 1.67 or greater, this version check can be removed. - #if BOOST_VERSION >= 106700 - try - { - lastItem = GetBoard()->GetItem( lastBrightenedItemID ); - } - catch( const boost::uuids::entropy_error& ) - { - wxLogError( wxT( "A Boost UUID entropy exception was thrown in %s:%s." ), - __FILE__, __FUNCTION__ ); - } - #else - lastItem = GetBoard()->GetItem( lastBrightenedItemID ); - #endif - - if( lastItem && lastItem != DELETED_BOARD_ITEM::GetInstance() ) + if( BOARD_ITEM* lastItem = GetBoard()->ResolveItem( lastBrightenedItemID, true ) ) { lastItem->ClearBrightened(); @@ -1265,9 +1247,7 @@ void PCB_BASE_FRAME::OnFpChangeDebounceTimer( wxTimerEvent& aEvent ) for( const KIID& uuid : selectedItems ) { - BOARD_ITEM* item = GetBoard()->GetItem( uuid ); - - if( item != DELETED_BOARD_ITEM::GetInstance() ) + if( BOARD_ITEM* item = GetBoard()->ResolveItem( uuid, true ) ) sel.push_back( item ); } diff --git a/pcbnew/pcb_design_block_utils.cpp b/pcbnew/pcb_design_block_utils.cpp index b23fab29ee..9fa86e7c37 100644 --- a/pcbnew/pcb_design_block_utils.cpp +++ b/pcbnew/pcb_design_block_utils.cpp @@ -281,7 +281,11 @@ bool PCB_EDIT_FRAME::saveSelectionToDesignBlock( const wxString& aNickname, PCB_ copy->SetParentGroup( nullptr ); if( item->Type() == PCB_FOOTPRINT_T ) + { static_cast( item )->RunOnChildren( addNetIfNeeded, RECURSE_MODE::NO_RECURSE ); + } + // Design Block TODO: does this want to catch design block groups? If so, consider: + // if( PCB_GROUP* group = dynamic_cast( copy ) ) else if( item->Type() == PCB_GROUP_T || item->Type() == PCB_GENERATOR_T ) { PCB_GROUP* group = static_cast( item ); @@ -395,6 +399,8 @@ bool PCB_EDIT_FRAME::SaveSelectionToDesignBlock( const LIB_ID& aLibId ) { EDA_ITEM* item = selection.Front(); + // Design Block TODO: does this want to catch design block groups? If so, consider: + // if( PCB_GROUP* group = dynamic_cast( copy ) ) if( item->Type() == PCB_GROUP_T || item->Type() == PCB_GENERATOR_T ) { group = static_cast( item ); diff --git a/pcbnew/pcb_generator.h b/pcbnew/pcb_generator.h index c578ea0310..d702d72f7a 100644 --- a/pcbnew/pcb_generator.h +++ b/pcbnew/pcb_generator.h @@ -51,7 +51,7 @@ public: /* * Clone() this and all descendants */ - PCB_GENERATOR* DeepClone() const override; + PCB_GENERATOR* DeepClone() const; virtual void EditStart( GENERATOR_TOOL* aTool, BOARD* aBoard, BOARD_COMMIT* aCommit ); diff --git a/pcbnew/pcb_group.cpp b/pcbnew/pcb_group.cpp index f3ead18e44..075f7896a8 100644 --- a/pcbnew/pcb_group.cpp +++ b/pcbnew/pcb_group.cpp @@ -47,90 +47,14 @@ PCB_GROUP::PCB_GROUP( BOARD_ITEM* aParent, KICAD_T idtype, PCB_LAYER_ID aLayer ) } -bool PCB_GROUP::IsGroupableType( KICAD_T aType ) -{ - switch ( aType ) - { - case PCB_FOOTPRINT_T: - case PCB_PAD_T: - case PCB_SHAPE_T: - case PCB_REFERENCE_IMAGE_T: - case PCB_FIELD_T: - case PCB_TEXT_T: - case PCB_TEXTBOX_T: - case PCB_TABLE_T: - case PCB_GROUP_T: - case PCB_GENERATOR_T: - case PCB_TRACE_T: - case PCB_VIA_T: - case PCB_ARC_T: - case PCB_DIMENSION_T: - case PCB_DIM_ALIGNED_T: - case PCB_DIM_LEADER_T: - case PCB_DIM_CENTER_T: - case PCB_DIM_RADIAL_T: - case PCB_DIM_ORTHOGONAL_T: - case PCB_ZONE_T: - return true; - default: - return false; - } -} - - -bool PCB_GROUP::AddItem( EDA_ITEM* aItem ) -{ - wxCHECK_MSG( aItem, false, wxT( "Nullptr added to group." ) ); - - wxCHECK_MSG( IsGroupableType( aItem->Type() ), false, - wxT( "Invalid item type added to group: " ) + aItem->GetTypeDesc() ); - - // Items can only be in one group at a time - if( aItem->GetParentGroup() ) - aItem->GetParentGroup()->RemoveItem( aItem ); - - m_items.insert( aItem ); - aItem->SetParentGroup( this ); - return true; -} - - -bool PCB_GROUP::RemoveItem( EDA_ITEM* aItem ) -{ - wxCHECK_MSG( aItem, false, wxT( "Nullptr removed from group." ) ); - - // Only clear the item's group field if it was inside this group - if( m_items.erase( aItem ) == 1 ) - { - aItem->SetParentGroup( nullptr ); - return true; - } - - return false; -} - - -void PCB_GROUP::RemoveAll() -{ - for( EDA_ITEM* item : m_items ) - item->SetParentGroup( nullptr ); - - m_items.clear(); -} - - std::unordered_set PCB_GROUP::GetBoardItems() const { std::unordered_set items; for( EDA_ITEM* item : m_items ) { - BOARD_ITEM* board_item = dynamic_cast( item ); - - if( board_item ) - { - items.insert( board_item ); - } + if( item->IsBOARD_ITEM() ) + items.insert( static_cast( item ) ); } return items; @@ -244,17 +168,17 @@ PCB_GROUP* PCB_GROUP::DeepClone() const } -PCB_GROUP* PCB_GROUP::DeepDuplicate() const +PCB_GROUP* PCB_GROUP::DeepDuplicate( bool addToParentGroup, BOARD_COMMIT* aCommit ) const { - PCB_GROUP* newGroup = static_cast( Duplicate() ); + PCB_GROUP* newGroup = static_cast( Duplicate( addToParentGroup, aCommit ) ); newGroup->m_items.clear(); for( EDA_ITEM* member : m_items ) { if( member->Type() == PCB_GROUP_T ) - newGroup->AddItem( static_cast( member )->DeepDuplicate() ); + newGroup->AddItem( static_cast( member )->DeepDuplicate( IGNORE_PARENT_GROUP ) ); else - newGroup->AddItem( static_cast( static_cast( member )->Duplicate() ) ); + newGroup->AddItem( static_cast( member )->Duplicate( IGNORE_PARENT_GROUP ) ); } return newGroup; @@ -267,25 +191,6 @@ void PCB_GROUP::swapData( BOARD_ITEM* aImage ) PCB_GROUP* image = static_cast( aImage ); std::swap( *this, *image ); - - // A group doesn't own its children (they're owned by the board), so undo doesn't do a - // deep clone when making an image. However, it's still safest to update the parentGroup - // pointers of the group's children -- we just have to be careful to do it in the right - // order in case any of the children are shared (ie: image first, "this" second so that - // any shared children end up with "this"). - image->RunOnChildren( - [&]( BOARD_ITEM* child ) - { - child->SetParentGroup( image ); - }, - RECURSE_MODE::NO_RECURSE ); - - RunOnChildren( - [&]( BOARD_ITEM* child ) - { - child->SetParentGroup( this ); - }, - RECURSE_MODE::NO_RECURSE ); } @@ -325,8 +230,8 @@ std::shared_ptr PCB_GROUP::GetEffectiveShape( PCB_LAYER_ID aLayer, FLASHI { std::shared_ptr shape = std::make_shared(); - for( EDA_ITEM* item : m_items ) - shape->AddShape( static_cast( item )->GetEffectiveShape( aLayer, aFlash )->Clone() ); + for( BOARD_ITEM* item : GetBoardItems() ) + shape->AddShape( item->GetEffectiveShape( aLayer, aFlash )->Clone() ); return shape; } diff --git a/pcbnew/pcb_group.h b/pcbnew/pcb_group.h index 0888944ce2..8820ab184b 100644 --- a/pcbnew/pcb_group.h +++ b/pcbnew/pcb_group.h @@ -66,22 +66,6 @@ public: return wxT( "PCB_GROUP" ); } - /** - * Add item to group. Does not take ownership of item. - * - * @return true if item was added (false if item belongs to a different group). - */ - bool AddItem( EDA_ITEM* aItem ) override; - - /** - * Remove item from group. - * - * @return true if item was removed (false if item was not in the group). - */ - bool RemoveItem( EDA_ITEM* aItem ) override; - - void RemoveAll() override; - std::unordered_set GetBoardItems() const; /* @@ -134,14 +118,17 @@ public: EDA_ITEM* Clone() const override; /* - * Clone() this and all descendants + * Clone this and all descendants */ - PCB_GROUP* DeepClone() const override; + PCB_GROUP* DeepClone() const; /* - * Duplicate() this and all descendants + * Duplicate this and all descendants + * + * @param addToParentGroup if the original is part of a group then the new member will also + * be added to said group */ - PCB_GROUP* DeepDuplicate() const override; + PCB_GROUP* DeepDuplicate( bool addToParentGroup, BOARD_COMMIT* aCommit = nullptr ) const; /// @copydoc BOARD_ITEM::IsOnLayer bool IsOnLayer( PCB_LAYER_ID aLayer ) const override; @@ -196,13 +183,6 @@ public: ///< @copydoc BOARD_ITEM::RunOnChildren void RunOnChildren( const std::function& aFunction, RECURSE_MODE aMode ) const override; - /** - * Check if the proposed type can be added to a group - * @param aType KICAD_T type to check - * @return true if the type can belong to a group, false otherwise - */ - static bool IsGroupableType( KICAD_T aType ); - protected: PCB_GROUP( BOARD_ITEM* aParent, KICAD_T idtype, PCB_LAYER_ID aLayer = F_Cu ); diff --git a/pcbnew/pcb_io/cadstar/cadstar_pcb_archive_loader.cpp b/pcbnew/pcb_io/cadstar/cadstar_pcb_archive_loader.cpp index fe049fb001..dae3a8f848 100644 --- a/pcbnew/pcb_io/cadstar/cadstar_pcb_archive_loader.cpp +++ b/pcbnew/pcb_io/cadstar/cadstar_pcb_archive_loader.cpp @@ -1766,7 +1766,7 @@ void CADSTAR_PCB_ARCHIVE_LOADER::loadComponents() FOOTPRINT* libFootprint = fpIter->second; // Use Duplicate() to ensure unique KIID for all objects - FOOTPRINT* footprint = static_cast( libFootprint->Duplicate() ); + FOOTPRINT* footprint = static_cast( libFootprint->Duplicate( IGNORE_PARENT_GROUP ) ); m_board->Add( footprint, ADD_MODE::APPEND ); @@ -2746,7 +2746,7 @@ void CADSTAR_PCB_ARCHIVE_LOADER::drawCadstarText( const TEXT& aCadstarText, for( PCB_LAYER_ID layer : getKiCadLayerSet( layersToDrawOn ).Seq() ) { txt->SetLayer( layer ); - PCB_TEXT* newtxt = static_cast( txt->Duplicate() ); + PCB_TEXT* newtxt = static_cast( txt->Duplicate( IGNORE_PARENT_GROUP ) ); m_board->Add( newtxt, ADD_MODE::APPEND ); if( !aCadstarGroupID.IsEmpty() ) @@ -3285,7 +3285,7 @@ void CADSTAR_PCB_ARCHIVE_LOADER::addAttribute( const ATTRIBUTE_LOCATION& aCadsta if( !aFootprint->Value().GetText().IsEmpty() ) { //copy the object - aFootprint->Add( aFootprint->Value().Duplicate() ); + aFootprint->Add( aFootprint->Value().Duplicate( IGNORE_PARENT_GROUP ) ); } aFootprint->SetValue( aAttributeValue ); diff --git a/pcbnew/pcb_io/cadstar/pcb_io_cadstar_archive.cpp b/pcbnew/pcb_io/cadstar/pcb_io_cadstar_archive.cpp index 9182fe06e0..4200915a96 100644 --- a/pcbnew/pcb_io/cadstar/pcb_io_cadstar_archive.cpp +++ b/pcbnew/pcb_io/cadstar/pcb_io_cadstar_archive.cpp @@ -221,7 +221,7 @@ FOOTPRINT* PCB_IO_CADSTAR_ARCHIVE::FootprintLoad( const wxString& aLibrar if( !m_cache.at( aLibraryPath ).at( aFootprintName ) ) return nullptr; - return static_cast( m_cache.at( aLibraryPath ).at( aFootprintName )->Duplicate() ); + return static_cast( m_cache.at( aLibraryPath ).at( aFootprintName )->Duplicate( IGNORE_PARENT_GROUP ) ); } diff --git a/pcbnew/pcb_io/eagle/pcb_io_eagle.cpp b/pcbnew/pcb_io/eagle/pcb_io_eagle.cpp index f5985bfda8..a3c2f118a9 100644 --- a/pcbnew/pcb_io/eagle/pcb_io_eagle.cpp +++ b/pcbnew/pcb_io/eagle/pcb_io_eagle.cpp @@ -1286,7 +1286,7 @@ void PCB_IO_EAGLE::loadElements( wxXmlNode* aElements ) THROW_IO_ERROR( emsg ); } - FOOTPRINT* footprint = static_cast( it->second->Duplicate() ); + FOOTPRINT* footprint = static_cast( it->second->Duplicate( IGNORE_PARENT_GROUP ) ); m_board->Add( footprint, ADD_MODE::APPEND ); @@ -2657,19 +2657,19 @@ void PCB_IO_EAGLE::loadClasses( wxXmlNode* aClasses ) for( ECLASS& eClass : eClasses ) { - for( std::pair entry : eClass.clearanceMap ) + for( const auto& [className, pt] : eClass.clearanceMap ) { - if( m_classMap[ entry.first ] != nullptr ) + if( m_classMap[className] != nullptr ) { wxString rule; rule.Printf( wxT( "(rule \"class %s:%s\"\n" " (condition \"A.NetClass == '%s' && B.NetClass == '%s'\")\n" " (constraint clearance (min %smm)))\n" ), eClass.number, - entry.first, + className, eClass.name, - m_classMap[ entry.first ]->GetName(), - EDA_UNIT_UTILS::UI::StringFromValue( pcbIUScale, EDA_UNITS::MM, entry.second.ToPcbUnits() ) ); + m_classMap[className]->GetName(), + EDA_UNIT_UTILS::UI::StringFromValue( pcbIUScale, EDA_UNITS::MM, pt.ToPcbUnits() ) ); m_customRules += wxT( "\n" ) + rule; } @@ -2823,8 +2823,7 @@ void PCB_IO_EAGLE::loadSignals( wxXmlNode* aSignals ) else { double annulus = drillz * m_rules->rvViaOuter; // eagle "restring" - annulus = eagleClamp( m_rules->rlMinViaOuter, annulus, - m_rules->rlMaxViaOuter ); + annulus = eagleClamp( m_rules->rlMinViaOuter, annulus, m_rules->rlMaxViaOuter ); kidiam = KiROUND( drillz + 2 * annulus ); via->SetWidth( PADSTACK::ALL_LAYERS, kidiam ); } @@ -3226,10 +3225,7 @@ void PCB_IO_EAGLE::cacheLib( const wxString& aLibPath ) wxXmlDocument xmlDocument; if( !stream.IsOk() || !xmlDocument.Load( stream ) ) - { - THROW_IO_ERROR( wxString::Format( _( "Unable to read file '%s'." ), - fn.GetFullPath() ) ); - } + THROW_IO_ERROR( wxString::Format( _( "Unable to read file '%s'." ), fn.GetFullPath() ) ); doc = xmlDocument.GetRoot(); @@ -3304,9 +3300,8 @@ void PCB_IO_EAGLE::FootprintEnumerate( wxArrayString& aFootprintNames, const wxS } -FOOTPRINT* PCB_IO_EAGLE::FootprintLoad( const wxString& aLibraryPath, - const wxString& aFootprintName, bool aKeepUUID, - const std::map* aProperties ) +FOOTPRINT* PCB_IO_EAGLE::FootprintLoad( const wxString& aLibraryPath, const wxString& aFootprintName, + bool aKeepUUID, const std::map* aProperties ) { init( aProperties ); cacheLib( aLibraryPath ); @@ -3316,7 +3311,7 @@ FOOTPRINT* PCB_IO_EAGLE::FootprintLoad( const wxString& aLibraryPath, return nullptr; // Return a copy of the template - FOOTPRINT* copy = (FOOTPRINT*) it->second->Duplicate(); + FOOTPRINT* copy = (FOOTPRINT*) it->second->Duplicate( IGNORE_PARENT_GROUP ); copy->SetParent( nullptr ); return copy; } diff --git a/pcbnew/pcb_io/geda/pcb_io_geda.cpp b/pcbnew/pcb_io/geda/pcb_io_geda.cpp index 305dcd4e96..aa9e456f8e 100644 --- a/pcbnew/pcb_io/geda/pcb_io_geda.cpp +++ b/pcbnew/pcb_io/geda/pcb_io_geda.cpp @@ -928,7 +928,7 @@ FOOTPRINT* PCB_IO_GEDA::FootprintLoad( const wxString& aLibraryPath, if( footprint ) { - FOOTPRINT* copy = (FOOTPRINT*) footprint->Duplicate(); + FOOTPRINT* copy = (FOOTPRINT*) footprint->Duplicate( IGNORE_PARENT_GROUP ); copy->SetParent( nullptr ); return copy; } diff --git a/pcbnew/pcb_io/kicad_legacy/pcb_io_kicad_legacy.cpp b/pcbnew/pcb_io/kicad_legacy/pcb_io_kicad_legacy.cpp index 2b243b2d10..4966cb98c0 100644 --- a/pcbnew/pcb_io/kicad_legacy/pcb_io_kicad_legacy.cpp +++ b/pcbnew/pcb_io/kicad_legacy/pcb_io_kicad_legacy.cpp @@ -3250,7 +3250,7 @@ FOOTPRINT* PCB_IO_KICAD_LEGACY::FootprintLoad( const wxString& aLibraryPath, } // Return copy of already loaded FOOTPRINT - FOOTPRINT* copy = (FOOTPRINT*) it->second->Duplicate(); + FOOTPRINT* copy = (FOOTPRINT*) it->second->Duplicate( IGNORE_PARENT_GROUP ); copy->SetParent( nullptr ); return copy; } diff --git a/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp b/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp index 88f53200dd..a187676694 100644 --- a/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp +++ b/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp @@ -3010,7 +3010,7 @@ FOOTPRINT* PCB_IO_KICAD_SEXPR::FootprintLoad( const wxString& aLibraryPath, if( aKeepUUID ) copy = static_cast( footprint->Clone() ); else - copy = static_cast( footprint->Duplicate() ); + copy = static_cast( footprint->Duplicate( IGNORE_PARENT_GROUP ) ); copy->SetParent( nullptr ); return copy; diff --git a/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp b/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp index 4e2cdc89c3..b41854298c 100644 --- a/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp +++ b/pcbnew/pcb_io/kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp @@ -1255,7 +1255,7 @@ void PCB_IO_KICAD_SEXPR_PARSER::resolveGroups( BOARD_ITEM* aParent ) if( BOARD* board = dynamic_cast( aParent ) ) { - aItem = board->GetItem( aId ); + aItem = board->ResolveItem( aId, true ); } else if( FOOTPRINT* footprint = dynamic_cast( aParent ) ) { @@ -1297,8 +1297,8 @@ void PCB_IO_KICAD_SEXPR_PARSER::resolveGroups( BOARD_ITEM* aParent ) if( !gen ) { - THROW_IO_ERROR( wxString::Format( - _( "Cannot create generated object of type '%s'" ), genInfo->genType ) ); + THROW_IO_ERROR( wxString::Format( _( "Cannot create generated object of type '%s'" ), + genInfo->genType ) ); } gen->SetLayer( genInfo->layer ); @@ -1339,16 +1339,10 @@ void PCB_IO_KICAD_SEXPR_PARSER::resolveGroups( BOARD_ITEM* aParent ) else item = getItem( aUuid ); - if( !item || item->Type() == NOT_USED ) - { - // This is the deleted item singleton, which means we didn't find the uuid. - continue; - } - // We used to allow fp items in non-footprint groups. It was a mistake. Check // to make sure they the item and group are owned by the same parent (will both // be nullptr in the board case). - if( item->GetParentFootprint() == group->GetParentFootprint() ) + if( item && item->GetParentFootprint() == group->GetParentFootprint() ) group->AddItem( item ); } } @@ -1388,9 +1382,8 @@ void PCB_IO_KICAD_SEXPR_PARSER::parseHeader() void PCB_IO_KICAD_SEXPR_PARSER::parseGeneralSection() { - wxCHECK_RET( CurTok() == T_general, - wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + - wxT( " as a general section." ) ); + wxCHECK_RET( CurTok() == T_general, + wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + wxT( " as a general section." ) ); T token; @@ -6171,7 +6164,8 @@ void PCB_IO_KICAD_SEXPR_PARSER::parseGROUP( BOARD_ITEM* aParent ) { if( static_cast( name.size() ) > bad_pos ) { - wxString msg = wxString::Format( _( "Group library link %s contains invalid character '%c'" ), name, + wxString msg = wxString::Format( _( "Group library link %s contains invalid character '%c'" ), + name, name[bad_pos] ); THROW_PARSE_ERROR( msg, CurSource(), CurLine(), CurLineNumber(), CurOffset() ); @@ -6204,8 +6198,8 @@ void PCB_IO_KICAD_SEXPR_PARSER::parseGROUP( BOARD_ITEM* aParent ) void PCB_IO_KICAD_SEXPR_PARSER::parseGENERATOR( BOARD_ITEM* aParent ) { - wxCHECK_RET( CurTok() == T_generated, wxT( "Cannot parse " ) + GetTokenString( CurTok() ) - + wxT( " as PCB_GENERATOR." ) ); + wxCHECK_RET( CurTok() == T_generated, + wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + wxT( " as PCB_GENERATOR." ) ); T token; @@ -6259,7 +6253,9 @@ void PCB_IO_KICAD_SEXPR_PARSER::parseGENERATOR( BOARD_ITEM* aParent ) NeedRIGHT(); break; - case T_members: parseGROUP_members( genInfo ); break; + case T_members: + parseGROUP_members( genInfo ); + break; default: { diff --git a/pcbnew/pcb_marker.cpp b/pcbnew/pcb_marker.cpp index e97b6778d9..6e292d95a7 100644 --- a/pcbnew/pcb_marker.cpp +++ b/pcbnew/pcb_marker.cpp @@ -222,14 +222,8 @@ void PCB_MARKER::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, std::vectorGetMainItemID() != niluuid ) - mainItem = aFrame->GetItem( m_rcItem->GetMainItemID() ); - - if( m_rcItem->GetAuxItemID() != niluuid ) - auxItem = aFrame->GetItem( m_rcItem->GetAuxItemID() ); + EDA_ITEM* mainItem = aFrame->ResolveItem( m_rcItem->GetMainItemID() ); + EDA_ITEM* auxItem = aFrame->ResolveItem( m_rcItem->GetAuxItemID() ); if( mainItem ) mainText = mainItem->GetItemDescription( aFrame, true ); diff --git a/pcbnew/pcb_painter.cpp b/pcbnew/pcb_painter.cpp index 7d81f41d97..22ba69f65c 100644 --- a/pcbnew/pcb_painter.cpp +++ b/pcbnew/pcb_painter.cpp @@ -1581,10 +1581,7 @@ void PCB_PAINTER::draw( const PAD* aPad, int aLayer ) if( pad_size.x + 2 * margin.x <= 0 || pad_size.y + 2 * margin.y <= 0 ) return; - dummyPad.reset( static_cast( aPad->Duplicate() ) ); - - if( dummyPad->GetParentGroup() ) - dummyPad->GetParentGroup()->RemoveItem( dummyPad.get() ); + dummyPad.reset( static_cast( aPad->Duplicate( IGNORE_PARENT_GROUP ) ) ); int initial_radius = dummyPad->GetRoundRectCornerRadius( pcbLayer ); diff --git a/pcbnew/tools/array_tool.cpp b/pcbnew/tools/array_tool.cpp index 6384a85360..0b65961f56 100644 --- a/pcbnew/tools/array_tool.cpp +++ b/pcbnew/tools/array_tool.cpp @@ -127,8 +127,8 @@ void ARRAY_TOOL::onDialogClosed( wxCloseEvent& aEvent ) BOARD_COMMIT commit( editFrame ); - FOOTPRINT* const fp = - m_isFootprintEditor ? editFrame->GetBoard()->GetFirstFootprint() : nullptr; + FOOTPRINT* const fp = m_isFootprintEditor ? editFrame->GetBoard()->GetFirstFootprint() + : nullptr; // Collect a list of pad numbers that will _not_ be counted as "used" // when finding the next pad numbers. @@ -277,7 +277,7 @@ void ARRAY_TOOL::onDialogClosed( wxCloseEvent& aEvent ) // Don't bother incrementing pads: the footprint won't update until commit, // so we can only do this once - this_item = fp->DuplicateItem( item ); + this_item = fp->DuplicateItem( true, &commit, item ); } else { @@ -299,10 +299,12 @@ void ARRAY_TOOL::onDialogClosed( wxCloseEvent& aEvent ) case PCB_DIM_ORTHOGONAL_T: case PCB_DIM_LEADER_T: case PCB_TARGET_T: - case PCB_ZONE_T: this_item = item->Duplicate(); break; + case PCB_ZONE_T: + this_item = item->Duplicate( true, &commit ); + break; case PCB_GROUP_T: - this_item = static_cast( item )->DeepDuplicate(); + this_item = static_cast( item )->DeepDuplicate( true, &commit ); break; default: diff --git a/pcbnew/tools/board_inspection_tool.cpp b/pcbnew/tools/board_inspection_tool.cpp index e355567098..04b3c031e5 100644 --- a/pcbnew/tools/board_inspection_tool.cpp +++ b/pcbnew/tools/board_inspection_tool.cpp @@ -340,8 +340,8 @@ void BOARD_INSPECTION_TOOL::InspectDRCError( const std::shared_ptr& aDR { wxCHECK( m_frame, /* void */ ); - BOARD_ITEM* a = m_frame->GetBoard()->GetItem( aDRCItem->GetMainItemID() ); - BOARD_ITEM* b = m_frame->GetBoard()->GetItem( aDRCItem->GetAuxItemID() ); + BOARD_ITEM* a = m_frame->GetBoard()->ResolveItem( aDRCItem->GetMainItemID() ); + BOARD_ITEM* b = m_frame->GetBoard()->ResolveItem( aDRCItem->GetAuxItemID() ); BOARD_CONNECTED_ITEM* ac = dynamic_cast( a ); BOARD_CONNECTED_ITEM* bc = dynamic_cast( b ); PCB_LAYER_ID layer = m_frame->GetActiveLayer(); @@ -380,7 +380,7 @@ void BOARD_INSPECTION_TOOL::InspectDRCError( const std::shared_ptr& aDR { for( KIID id : aDRCItem->GetIDs() ) { - bc = dynamic_cast( m_frame->GetBoard()->GetItem( id ) ); + bc = dynamic_cast( m_frame->GetBoard()->ResolveItem( id, true ) ); if( ac && bc && ac->GetNetCode() != bc->GetNetCode() ) break; diff --git a/pcbnew/tools/drawing_tool.cpp b/pcbnew/tools/drawing_tool.cpp index c4c9c1bb03..27fb639df2 100644 --- a/pcbnew/tools/drawing_tool.cpp +++ b/pcbnew/tools/drawing_tool.cpp @@ -1838,7 +1838,6 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent ) PCB_SELECTION preview; BOARD_COMMIT commit( m_frame ); PCB_GROUP* group = nullptr; - PICKED_ITEMS_LIST groupUndoList; PCB_LAYER_ID layer = F_Cu; if( dlg.ShouldGroupItems() ) @@ -1881,14 +1880,9 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent ) newItems.push_back( item ); if( group ) - { group->AddItem( item ); - groupUndoList.PushItem( ITEM_PICKER( nullptr, item, UNDO_REDO::REGROUP ) ); - } else - { selectedItems.push_back( item ); - } layer = item->GetLayer(); } @@ -1907,9 +1901,6 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent ) for( BOARD_ITEM* item : newItems ) commit.Add( item ); - if( groupUndoList.GetCount() > 0 ) - commit.Stage( groupUndoList ); - commit.Push( _( "Import Graphics" ) ); return 0; @@ -1970,10 +1961,7 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent ) m_toolMgr->RunAction( ACTIONS::selectionClear ); if( group ) - { preview.Remove( group ); - group->RemoveAll(); - } for( BOARD_ITEM* item : newItems ) delete item; @@ -1999,9 +1987,6 @@ int DRAWING_TOOL::PlaceImportedGraphics( const TOOL_EVENT& aEvent ) for( BOARD_ITEM* item : newItems ) commit.Add( item ); - if( groupUndoList.GetCount() > 0 ) - commit.Stage( groupUndoList ); - commit.Push( _( "Import Graphics" ) ); break; // This is a one-shot command, not a tool diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp index d886edc06c..f0c15451c6 100644 --- a/pcbnew/tools/edit_tool.cpp +++ b/pcbnew/tools/edit_tool.cpp @@ -2115,7 +2115,7 @@ int EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : selection ) { if( !item->IsNew() && !item->IsMoving() ) - commit->Modify( item ); + commit->Modify( item, nullptr, RECURSE_MODE::RECURSE ); if( item->IsBOARD_ITEM() ) { @@ -2453,7 +2453,7 @@ int EDIT_TOOL::Flip( const TOOL_EVENT& aEvent ) BOARD_ITEM* boardItem = static_cast( item ); if( !boardItem->IsNew() && !boardItem->IsMoving() ) - commit->Modify( boardItem ); + commit->Modify( boardItem, nullptr, RECURSE_MODE::RECURSE ); boardItem->Flip( refPt, flipDirection ); boardItem->Normalize(); @@ -2487,53 +2487,6 @@ int EDIT_TOOL::Flip( const TOOL_EVENT& aEvent ) } -void EDIT_TOOL::getChildItemsOfGroupsAndGenerators( EDA_ITEM* item, - std::unordered_set& children ) -{ - wxASSERT( item->Type() == PCB_GROUP_T || item->Type() == PCB_GENERATOR_T ); - - std::unordered_set& childItems = static_cast( item )->GetItems(); - - for( EDA_ITEM* childItem : childItems ) - { - children.insert( childItem ); - - if( childItem->Type() == PCB_GROUP_T || childItem->Type() == PCB_GENERATOR_T ) - getChildItemsOfGroupsAndGenerators( childItem, children ); - } -} - - -void EDIT_TOOL::removeNonRootItems( std::unordered_set& items ) -{ - auto itr = items.begin(); - - while( itr != items.end() ) - { - EDA_ITEM* item = *itr; - - if( item->IsBOARD_ITEM() ) - { - BOARD_ITEM* curItem = static_cast( item ); - - if( curItem->Type() == PCB_GROUP_T || curItem->Type() == PCB_GENERATOR_T ) - { - std::unordered_set childItems; - getChildItemsOfGroupsAndGenerators( curItem, childItems ); - - std::for_each( childItems.begin(), childItems.end(), - [&]( auto eraseItem ) - { - items.erase( eraseItem ); - } ); - } - } - - ++itr; - } -} - - void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) { PCB_BASE_EDIT_FRAME* editFrame = getEditFrame(); @@ -2542,16 +2495,11 @@ void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) // As we are about to remove items, they have to be removed from the selection first m_toolMgr->RunAction( ACTIONS::selectionClear ); - // Only delete items that are the root of a selected set (e.g. only delete grouped / generated - // items from the parent item, not individually) - issue #17527 - std::unordered_set rootItems( aItems.begin(), aItems.end() ); - removeNonRootItems( rootItems ); - int itemsDeleted = 0; int fieldsHidden = 0; int fieldsAlreadyHidden = 0; - for( EDA_ITEM* item : rootItems ) + for( EDA_ITEM* item : aItems ) { if( !item->IsBOARD_ITEM() ) continue; @@ -2559,9 +2507,6 @@ void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) BOARD_ITEM* board_item = static_cast( item ); FOOTPRINT* parentFP = board_item->GetParentFootprint(); - if( board_item->GetParentGroup() && !parentFP ) - commit.Stage( board_item, CHT_UNGROUP ); - switch( item->Type() ) { case PCB_FIELD_T: @@ -2618,13 +2563,6 @@ void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) break; case PCB_GROUP_T: - board_item->RunOnChildren( - [&commit]( BOARD_ITEM* aItem ) - { - commit.Stage( aItem, CHT_UNGROUP ); - }, - RECURSE_MODE::RECURSE ); - board_item->RunOnChildren( [&commit]( BOARD_ITEM* aItem ) { @@ -2682,7 +2620,7 @@ void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) break; case PCB_GENERATOR_T: - if( rootItems.size() == 1 ) + if( aItems.Size() == 1 ) { PCB_GENERATOR* generator = static_cast( board_item ); @@ -2693,9 +2631,6 @@ void EDIT_TOOL::DeleteItems( const PCB_SELECTION& aItems, bool aIsCut ) { PCB_GENERATOR* generator = static_cast( board_item ); - for( EDA_ITEM* member : generator->GetItems() ) - commit.Stage( member, CHT_UNGROUP ); - for( EDA_ITEM* member : generator->GetItems() ) commit.Remove( member ); @@ -2769,6 +2704,7 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent ) selectionCopy = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, PCB_SELECTION_TOOL* sTool ) { + sTool->FilterCollectorForHierarchy( aCollector, true ); } ); size_t beforeFPCount = selectionCopy.CountType( PCB_FOOTPRINT_T ); @@ -2776,6 +2712,7 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent ) m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, PCB_SELECTION_TOOL* sTool ) { + sTool->FilterCollectorForHierarchy( aCollector, true ); sTool->FilterCollectorForFreePads( aCollector ); } ); @@ -2799,6 +2736,7 @@ int EDIT_TOOL::Remove( const TOOL_EVENT& aEvent ) selectionCopy = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, PCB_SELECTION_TOOL* sTool ) { + sTool->FilterCollectorForHierarchy( aCollector, true ); sTool->FilterCollectorForFreePads( aCollector ); }, true /* prompt user regarding locked items */ ); @@ -2864,7 +2802,7 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent ) BOARD_ITEM* boardItem = static_cast( item ); if( !boardItem->IsNew() ) - commit.Modify( boardItem ); + commit.Modify( boardItem, nullptr, RECURSE_MODE::RECURSE ); if( !boardItem->GetParent() || !boardItem->GetParent()->IsSelected() ) boardItem->Move( translation ); @@ -2958,11 +2896,9 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) BOARD_ITEM* dupe_item = nullptr; BOARD_ITEM* orig_item = static_cast( item ); - if( !m_isFootprintEditor - && /*FOOTPRINT* parentFootprint =*/ orig_item->GetParentFootprint() ) + if( !m_isFootprintEditor && orig_item->GetParentFootprint() ) { // No sub-footprint modifications allowed outside of footprint editor - // and parentFootprint is not (yet?) used } else { @@ -2984,9 +2920,9 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) case PCB_DIM_ORTHOGONAL_T: case PCB_DIM_LEADER_T: if( m_isFootprintEditor ) - dupe_item = parentFootprint->DuplicateItem( orig_item ); + dupe_item = parentFootprint->DuplicateItem( true, &commit, orig_item ); else - dupe_item = orig_item->Duplicate(); + dupe_item = orig_item->Duplicate( true, &commit ); // Clear the selection flag here, otherwise the PCB_SELECTION_TOOL // will not properly select it later on @@ -3003,7 +2939,7 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) break; case PCB_PAD_T: - dupe_item = parentFootprint->DuplicateItem( orig_item ); + dupe_item = parentFootprint->DuplicateItem( true, &commit, orig_item ); if( increment && static_cast( dupe_item )->CanHaveNumber() ) { @@ -3028,7 +2964,7 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) case PCB_GENERATOR_T: case PCB_GROUP_T: - dupe_item = static_cast( orig_item )->DeepDuplicate(); + dupe_item = static_cast( orig_item )->DeepDuplicate( true, &commit ); dupe_item->RunOnChildren( [&]( BOARD_ITEM* aItem ) @@ -3045,8 +2981,7 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) break; default: - wxASSERT_MSG( false, wxString::Format( wxT( "Unhandled item type %d" ), - orig_item->Type() ) ); + UNIMPLEMENTED_FOR( orig_item->Type() ); break; } } @@ -3117,14 +3052,6 @@ int EDIT_TOOL::Increment( const TOOL_EVENT& aEvent ) if( !commit ) commit = &localCommit; - const auto modifyItem = [&]( EDA_ITEM& aItem ) - { - if( aItem.IsNew() ) - m_toolMgr->PostAction( ACTIONS::refreshPreview ); - else - commit->Modify( &aItem ); - }; - for( EDA_ITEM* item : selection ) { switch( item->Type() ) @@ -3146,7 +3073,9 @@ int EDIT_TOOL::Increment( const TOOL_EVENT& aEvent ) if( newNumber ) { - modifyItem( pad ); + if( !pad.IsNew() ) + commit->Modify( &pad ); + pad.SetNumber( *newNumber ); } @@ -3161,7 +3090,9 @@ int EDIT_TOOL::Increment( const TOOL_EVENT& aEvent ) if( newText ) { - modifyItem( text ); + if( !text.IsNew() ) + commit->Modify( &text ); + text.SetText( *newText ); } @@ -3172,6 +3103,9 @@ int EDIT_TOOL::Increment( const TOOL_EVENT& aEvent ) } } + if( selection.Front()->IsMoving() ) + m_toolMgr->PostAction( ACTIONS::refreshPreview ); + commit->Push( _( "Increment" ) ); return 0; @@ -3367,23 +3301,8 @@ int EDIT_TOOL::copyToClipboard( const TOOL_EVENT& aEvent ) PCB_SELECTION& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, PCB_SELECTION_TOOL* sTool ) { - for( int i = aCollector.GetCount() - 1; i >= 0; --i ) - { - BOARD_ITEM* item = aCollector[i]; - - // We can't copy both a footprint and its text in the same operation, so if - // both are selected, remove the text - if( ( item->Type() == PCB_FIELD_T || item->Type() == PCB_TEXT_T ) - && aCollector.HasItem( item->GetParentFootprint() ) ) - { - aCollector.Remove( item ); - } - else if( item->Type() == PCB_MARKER_T ) - { - // Don't allow copying marker objects - aCollector.Remove( item ); - } - } + sTool->FilterCollectorForHierarchy( aCollector, true ); + sTool->FilterCollectorForMarkers( aCollector ); }, // Prompt user regarding locked items. diff --git a/pcbnew/tools/edit_tool.h b/pcbnew/tools/edit_tool.h index 0d6c31219e..e9466dfeeb 100644 --- a/pcbnew/tools/edit_tool.h +++ b/pcbnew/tools/edit_tool.h @@ -227,15 +227,6 @@ private: ///< Rebuilds the ratsnest for operations that require it outside the commit rebuild void rebuildConnectivity(); - ///< Removes all items from the set which are children of other PCB_GROUP or PCB_GENERATOR - ///< items in the set - void removeNonRootItems( std::unordered_set& items ); - - ///< Recursively adds any child items of the given item to the set - void getChildItemsOfGroupsAndGenerators( EDA_ITEM* item, - std::unordered_set& children ); - - private: PCB_SELECTION_TOOL* m_selectionTool; bool m_dragging; // Indicates objects are currently being dragged diff --git a/pcbnew/tools/edit_tool_move_fct.cpp b/pcbnew/tools/edit_tool_move_fct.cpp index 383f6a8abe..fa8ff00252 100644 --- a/pcbnew/tools/edit_tool_move_fct.cpp +++ b/pcbnew/tools/edit_tool_move_fct.cpp @@ -93,7 +93,7 @@ int EDIT_TOOL::Swap( const TOOL_EVENT& aEvent ) for( EDA_ITEM* item : selection ) { if( !item->IsNew() && !item->IsMoving() ) - commit->Modify( item ); + commit->Modify( item, nullptr, RECURSE_MODE::RECURSE ); } for( size_t i = 0; i < sorted.size() - 1; i++ ) @@ -576,6 +576,10 @@ bool EDIT_TOOL::doMoveSelection( const TOOL_EVENT& aEvent, BOARD_COMMIT* aCommit m_toolMgr->RunSynchronousAction( PCB_ACTIONS::genStartEdit, aCommit, static_cast( item ) ); } + else if( item->Type() == PCB_GROUP_T || item->Type() == PCB_GENERATOR_T ) + { + aCommit->Modify( item, nullptr, RECURSE_MODE::RECURSE ); + } else { aCommit->Modify( item ); diff --git a/pcbnew/tools/multichannel_tool.cpp b/pcbnew/tools/multichannel_tool.cpp index bba1dc5806..3e73ba94ee 100644 --- a/pcbnew/tools/multichannel_tool.cpp +++ b/pcbnew/tools/multichannel_tool.cpp @@ -567,36 +567,32 @@ int MULTICHANNEL_TOOL::RepeatLayout( const TOOL_EVENT& aEvent, ZONE* aRefZone ) totalCopied++; } - - commit.Push( _( "Repeat layout" ) ); - if( m_areas.m_options.m_groupItems ) { - BOARD_COMMIT grpCommit( GetManager(), true ); - - for( auto& targetArea : m_areas.m_compatMap ) + for( const auto& [targetArea, compatData] : m_areas.m_compatMap ) { - pruneExistingGroups( grpCommit, targetArea.second.m_affectedItems ); + pruneExistingGroups( commit, compatData.m_affectedItems ); - PCB_GROUP* grp = new PCB_GROUP( board() ); + PCB_GROUP* group = new PCB_GROUP( board() ); - grpCommit.Add( grp ); + commit.Add( group ); - for( BOARD_ITEM* item : targetArea.second.m_groupableItems ) + for( BOARD_ITEM* item : compatData.m_groupableItems ) { - grpCommit.Stage( item, CHT_GROUP ); + commit.Modify( item ); + group->AddItem( item ); } } - - grpCommit.Push( _( "Group repeated items" ) ); } + commit.Push( _( "Repeat layout" ) ); if( Pgm().IsGUI() ) { frame()->ShowInfoBarMsg( wxString::Format( _( "Copied to %d Rule Areas." ), totalCopied ), - true ); + true ); } + return 0; } @@ -1054,35 +1050,31 @@ bool MULTICHANNEL_TOOL::resolveConnectionTopology( RULE_AREA* aRefArea, RULE_ARE bool MULTICHANNEL_TOOL::pruneExistingGroups( COMMIT& aCommit, const std::unordered_set& aItemsToRemove ) { - std::deque pending ( board()->Groups() ); - while( !pending.empty() ) + // Note: groups are only collections, not "real" hierarchy. A group's members are still parented + // by the board (and therefore nested groups are still in the board's list of groups). + for( PCB_GROUP* group : board()->Groups() ) { - auto grp = pending.front(); - pending.pop_front(); + std::vector pruneList; - std::unordered_set& grpItems = grp->GetItems(); - size_t n_erased = 0; - - for( EDA_ITEM* refItem : grpItems ) + for( EDA_ITEM* refItem : group->GetItems() ) { - if( refItem->Type() == PCB_GROUP_T ) - pending.push_back( static_cast(refItem) ); - for( BOARD_ITEM* testItem : aItemsToRemove ) { if( refItem->m_Uuid == testItem->m_Uuid ) - { - aCommit.Stage( refItem, CHT_UNGROUP ); - n_erased++; - } + pruneList.push_back( refItem ); } } - if( n_erased == grpItems.size() ) + if( !pruneList.empty() ) { - aCommit.Stage( grp, CHT_REMOVE ); - } + aCommit.Modify( group ); + for( EDA_ITEM* item : pruneList ) + group->RemoveItem( item ); + + if( group->GetItems().empty() ) + aCommit.Remove( group ); + } } return false; @@ -1234,16 +1226,11 @@ int MULTICHANNEL_TOOL::AutogenerateRuleAreas( const TOOL_EVENT& aEvent ) commit.Add( newZone.release() ); } - commit.Push( _( "Auto-generate placement rule areas" ) ); - // fixme: handle corner cases where the items belonging to a Rule Area already // belong to other groups. if( m_areas.m_options.m_groupItems ) { - // fixme: sth gets weird when creating new zones & grouping them within a single COMMIT - BOARD_COMMIT grpCommit( GetManager(), true ); - for( RULE_AREA& ra : m_areas.m_areas ) { if( !ra.m_generateEnabled ) @@ -1260,20 +1247,24 @@ int MULTICHANNEL_TOOL::AutogenerateRuleAreas( const TOOL_EVENT& aEvent ) if( ra.m_existsAlready ) toPrune.insert( ra.m_area ); - pruneExistingGroups( grpCommit, toPrune ); + pruneExistingGroups( commit, toPrune ); - PCB_GROUP* grp = new PCB_GROUP( board() ); + PCB_GROUP* group = new PCB_GROUP( board() ); - grpCommit.Add( grp ); - grpCommit.Stage( ra.m_area, CHT_GROUP ); + commit.Add( group ); + + commit.Modify( ra.m_area ); + group->AddItem( ra.m_area ); for( FOOTPRINT* fp : ra.m_components ) { - grpCommit.Stage( fp, CHT_GROUP ); + commit.Modify( fp ); + group->AddItem( fp ); } } - grpCommit.Push( _( "Group components with their placement rule areas" ) ); } + commit.Push( _( "Auto-generate placement rule areas" ) ); + return true; } diff --git a/pcbnew/tools/pad_tool.cpp b/pcbnew/tools/pad_tool.cpp index 54f1f62434..ae7519ec9a 100644 --- a/pcbnew/tools/pad_tool.cpp +++ b/pcbnew/tools/pad_tool.cpp @@ -65,7 +65,7 @@ void PAD_TOOL::Reset( RESET_REASON aReason ) if( aReason == MODEL_RELOAD ) m_lastPadNumber = wxT( "1" ); - if( board() && board()->GetItem( m_editPad ) == DELETED_BOARD_ITEM::GetInstance() ) + if( board() && board()->ResolveItem( m_editPad ) == DELETED_BOARD_ITEM::GetInstance() ) { PCB_DISPLAY_OPTIONS opts = frame()->GetDisplayOptions(); @@ -730,9 +730,7 @@ int PAD_TOOL::EditPad( const TOOL_EVENT& aEvent ) if( m_editPad != niluuid ) { - PAD* pad = dynamic_cast( frame()->GetItem( m_editPad ) ); - - if( pad ) + if( PAD* pad = dynamic_cast( frame()->ResolveItem( m_editPad ) ) ) { BOARD_COMMIT commit( frame() ); commit.Modify( pad ); @@ -887,7 +885,7 @@ void PAD_TOOL::explodePad( PAD* aPad, PCB_LAYER_ID* aLayer, BOARD_COMMIT& aCommi { for( const std::shared_ptr& primitive : aPad->GetPrimitives( PADSTACK::ALL_LAYERS ) ) { - PCB_SHAPE* shape = static_cast( primitive->Duplicate() ); + PCB_SHAPE* shape = static_cast( primitive->Duplicate( true, &aCommit ) ); shape->SetParent( board()->GetFirstFootprint() ); shape->Rotate( VECTOR2I( 0, 0 ), aPad->GetOrientation() ); diff --git a/pcbnew/tools/pcb_control.cpp b/pcbnew/tools/pcb_control.cpp index 7b3edabbdf..3ba98774f9 100644 --- a/pcbnew/tools/pcb_control.cpp +++ b/pcbnew/tools/pcb_control.cpp @@ -1538,11 +1538,16 @@ bool PCB_CONTROL::placeBoardItems( BOARD_COMMIT* aCommit, std::vectorGetEnteredGroup() && !item->GetParentGroup() ) - selectionTool->GetEnteredGroup()->AddItem( item ); + // While BOARD_COMMIT::Push() will add any new items to the entered group, + // we need to do it earlier so that the previews while moving are correct. + if( PCB_GROUP* enteredGroup = selectionTool->GetEnteredGroup() ) + { + if( item->IsGroupableType() && !item->GetParentGroup() ) + { + aCommit->Modify( enteredGroup ); + enteredGroup->AddItem( item ); + } + } item->SetParent( board() ); } @@ -1729,12 +1734,9 @@ int PCB_CONTROL::AppendBoard( PCB_IO& pi, const wxString& fileName, DESIGN_BLOCK if( placeBoardItems( &commit, brd, false, false /* Don't reannotate dupes on Append Board */ ) ) { - commit.Push( _( "Append Board" ) ); - if( placeAsGroup ) { - BOARD_COMMIT grpCommit( m_toolMgr ); - PCB_GROUP* group = new PCB_GROUP( brd ); + PCB_GROUP* group = new PCB_GROUP( brd ); if( aDesignBlock ) { @@ -1759,16 +1761,17 @@ int PCB_CONTROL::AppendBoard( PCB_IO& pi, const wxString& fileName, DESIGN_BLOCK } } - grpCommit.Add( group ); + commit.Add( group ); for( EDA_ITEM* eda_item : selection ) { if( eda_item->IsBOARD_ITEM() && !static_cast( eda_item )->GetParentFootprint() ) - grpCommit.Stage( static_cast( eda_item ), CHT_GROUP ); + { + commit.Modify( eda_item ); + group->AddItem( eda_item ); + } } - grpCommit.Push( _( "Group Items" ), APPEND_UNDO ); - selTool->ClearSelection(); selTool->select( group ); @@ -1777,6 +1780,8 @@ int PCB_CONTROL::AppendBoard( PCB_IO& pi, const wxString& fileName, DESIGN_BLOCK m_frame->Refresh(); } + commit.Push( _( "Append Board" ) ); + editFrame->GetBoard()->BuildConnectivity(); ret = 0; } @@ -2146,7 +2151,7 @@ int PCB_CONTROL::UpdateMessagePanel( const TOOL_EVENT& aEvent ) // Use dynamic_cast to include PCB_GENERATORs. else if( PCB_GROUP* group = dynamic_cast( aItem ) ) { - group->RunOnChildren( accumulateTrackLength, RECURSE_MODE::NO_RECURSE ); + group->RunOnChildren( accumulateTrackLength, RECURSE_MODE::RECURSE ); } else { @@ -2186,9 +2191,9 @@ int PCB_CONTROL::UpdateMessagePanel( const TOOL_EVENT& aEvent ) return; } - if( aItem->Type() == PCB_GROUP_T || aItem->Type() == PCB_GENERATOR_T ) + if( PCB_GROUP* group = dynamic_cast( aItem ) ) { - static_cast( aItem )->RunOnChildren( accumulateArea, RECURSE_MODE::RECURSE ); + group->RunOnChildren( accumulateArea, RECURSE_MODE::RECURSE ); return; } diff --git a/pcbnew/tools/pcb_group_tool.cpp b/pcbnew/tools/pcb_group_tool.cpp index bc71d5c005..5c4d4fbca7 100644 --- a/pcbnew/tools/pcb_group_tool.cpp +++ b/pcbnew/tools/pcb_group_tool.cpp @@ -172,14 +172,16 @@ int PCB_GROUP_TOOL::Group( const TOOL_EVENT& aEvent ) } } - m_commit->Add( group ); - for( EDA_ITEM* eda_item : selection ) { if( eda_item->IsBOARD_ITEM() ) - m_commit->Stage( static_cast( eda_item ), CHT_GROUP ); + { + m_commit->Modify( eda_item ); + group->AddItem( eda_item ); + } } + m_commit->Add( group ); m_commit->Push( _( "Group Items" ) ); m_toolMgr->RunAction( ACTIONS::selectionClear ); diff --git a/pcbnew/tools/pcb_point_editor.cpp b/pcbnew/tools/pcb_point_editor.cpp index f0b58cb7e7..8b1de8fea4 100644 --- a/pcbnew/tools/pcb_point_editor.cpp +++ b/pcbnew/tools/pcb_point_editor.cpp @@ -1869,7 +1869,6 @@ int PCB_POINT_EDITOR::OnSelectionChange( const TOOL_EVENT& aEvent ) // reference geometry (e.g. for intersections and extensions) BOARD_ITEM* clone = static_cast( item->Clone() ); clone->SetParent( nullptr ); - clone->SetParentGroup( nullptr ); if( PCB_SHAPE* shape= dynamic_cast( item ) ) { @@ -2446,8 +2445,7 @@ int PCB_POINT_EDITOR::addCorner( const TOOL_EVENT& aEvent ) graphicItem->SetEnd( nearestPoint ); // and add another one starting from the break point - PCB_SHAPE* newSegment = static_cast( graphicItem->Duplicate() ); - + PCB_SHAPE* newSegment = static_cast( graphicItem->Duplicate( true, &commit ) ); newSegment->ClearSelected(); newSegment->SetStart( nearestPoint ); newSegment->SetEnd( VECTOR2I( seg.B.x, seg.B.y ) ); @@ -2468,7 +2466,7 @@ int PCB_POINT_EDITOR::addCorner( const TOOL_EVENT& aEvent ) graphicItem->SetEnd( nearestPoint ); // and add another one starting from the break point - PCB_SHAPE* newArc = static_cast( graphicItem->Duplicate() ); + PCB_SHAPE* newArc = static_cast( graphicItem->Duplicate( true, &commit ) ); newArc->ClearSelected(); newArc->SetEnd( arc.GetP1() ); diff --git a/pcbnew/tools/pcb_selection_tool.cpp b/pcbnew/tools/pcb_selection_tool.cpp index b319344b0e..33320fb1a0 100644 --- a/pcbnew/tools/pcb_selection_tool.cpp +++ b/pcbnew/tools/pcb_selection_tool.cpp @@ -613,11 +613,9 @@ void PCB_SELECTION_TOOL::EnterGroup() ClearSelection(); m_enteredGroup = aGroup; m_enteredGroup->SetFlags( ENTERED ); - m_enteredGroup->RunOnChildren( [&]( BOARD_ITEM* titem ) - { - select( titem ); - }, - RECURSE_MODE::NO_RECURSE ); + + for( EDA_ITEM* member : m_enteredGroup->GetItems() ) + select( member ); m_toolMgr->ProcessEvent( EVENTS::SelectedEvent ); diff --git a/pcbnew/tools/position_relative_tool.cpp b/pcbnew/tools/position_relative_tool.cpp index 90a774e505..eddb5036cd 100644 --- a/pcbnew/tools/position_relative_tool.cpp +++ b/pcbnew/tools/position_relative_tool.cpp @@ -62,7 +62,7 @@ static void moveSelectionBy( const PCB_SELECTION& aSelection, const VECTOR2I& aM continue; BOARD_ITEM* boardItem = static_cast( item ); - commit.Modify( boardItem ); + commit.Modify( boardItem, nullptr, RECURSE_MODE::RECURSE ); boardItem->Move( aMoveVec ); } } diff --git a/pcbnew/tracks_cleaner.cpp b/pcbnew/tracks_cleaner.cpp index 0274fd72cd..1f5d3b04af 100644 --- a/pcbnew/tracks_cleaner.cpp +++ b/pcbnew/tracks_cleaner.cpp @@ -730,9 +730,6 @@ bool TRACKS_CLEANER::testMergeCollinearSegments( PCB_TRACK* aSeg1, PCB_TRACK* aS if( !aDummySeg ) aDummySeg = &dummy_seg; - // Do not copy the parent group to the dummy segment - dummy_seg.SetParentGroup( nullptr ); - // Calculate the new ends of the segment to merge, and store them to dummy_seg: int min_x = std::min( aSeg1->GetStart().x, std::min( aSeg1->GetEnd().x, std::min( aSeg2->GetStart().x, aSeg2->GetEnd().x ) ) ); @@ -772,8 +769,6 @@ bool TRACKS_CLEANER::mergeCollinearSegments( PCB_TRACK* aSeg1, PCB_TRACK* aSeg2 { PCB_TRACK dummy_seg( *aSeg1 ); - dummy_seg.SetParentGroup( nullptr ); - if( !testMergeCollinearSegments( aSeg1, aSeg2, &dummy_seg ) ) return false; @@ -787,10 +782,7 @@ bool TRACKS_CLEANER::mergeCollinearSegments( PCB_TRACK* aSeg1, PCB_TRACK* aSeg2 { m_commit.Modify( aSeg1 ); - EDA_GROUP* group = aSeg1->GetParentGroup(); *aSeg1 = dummy_seg; - aSeg1->SetParentGroup( group ); - m_brd->GetConnectivity()->Update( aSeg1 ); diff --git a/pcbnew/undo_redo.cpp b/pcbnew/undo_redo.cpp index 189e27fa23..a1b840ddda 100644 --- a/pcbnew/undo_redo.cpp +++ b/pcbnew/undo_redo.cpp @@ -103,7 +103,7 @@ void PCB_BASE_EDIT_FRAME::saveCopyInUndoList( PICKED_ITEMS_LIST* commandToUndo, const PICKED_ITEMS_LIST& aItemsList, UNDO_REDO aCommandType ) { - int preExisting = commandToUndo->GetCount(); + int preExisting = (int) commandToUndo->GetCount(); for( unsigned ii = 0; ii < aItemsList.GetCount(); ii++ ) commandToUndo->PushItem( aItemsList.GetItemWrapper(ii) ); @@ -135,8 +135,6 @@ void PCB_BASE_EDIT_FRAME::saveCopyInUndoList( PICKED_ITEMS_LIST* commandToUndo, case UNDO_REDO::NEWITEM: case UNDO_REDO::DELETED: case UNDO_REDO::PAGESETTINGS: - case UNDO_REDO::REGROUP: - case UNDO_REDO::UNGROUP: break; default: @@ -357,13 +355,11 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) UNDO_REDO status = aList->GetPickedItemStatus( ii ); if( status != UNDO_REDO::DELETED - && status != UNDO_REDO::REGROUP - && status != UNDO_REDO::UNGROUP && status != UNDO_REDO::DRILLORIGIN // origin markers never on board && status != UNDO_REDO::GRIDORIGIN // origin markers never on board && status != UNDO_REDO::PAGESETTINGS ) // nor are page settings proxy items { - if( GetBoard()->GetItem( eda_item->m_Uuid ) == DELETED_BOARD_ITEM::GetInstance() ) + if( !GetBoard()->ResolveItem( eda_item->m_Uuid, true ) ) { // Remove this non existent item aList->RemovePicker( ii ); @@ -434,13 +430,12 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) { BOARD_ITEM* item = (BOARD_ITEM*) eda_item; BOARD_ITEM_CONTAINER* parent = GetBoard(); - EDA_GROUP* parentGroup = item->GetParentGroup(); if( item->GetParentFootprint() ) { // We need the current item and it's parent, which may be different from what // was stored if we're multiple frames up the undo stack. - item = GetBoard()->GetItem( item->m_Uuid ); + item = GetBoard()->ResolveItem( item->m_Uuid ); parent = item->GetParentFootprint(); } @@ -448,9 +443,6 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) view->Remove( item ); - if( parentGroup ) - parentGroup->RemoveItem( item ); - parent->Remove( item ); item->SwapItemData( image ); @@ -458,22 +450,10 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) item->ClearFlags( UR_TRANSIENT ); image->SetFlags( UR_TRANSIENT ); - if( PCB_GROUP* group = dynamic_cast( item ) ) - { - group->RunOnChildren( [&]( BOARD_ITEM* child ) - { - child->SetParentGroup( group ); - }, - RECURSE_MODE::NO_RECURSE ); - } - view->Add( item ); view->Hide( item, false ); parent->Add( item ); - if( parentGroup ) - parentGroup->AddItem( item ); - update_item_change_state( item, ITEM_CHANGE_TYPE::CHANGED ); break; } @@ -503,39 +483,6 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) break; - case UNDO_REDO::REGROUP: /* grouped items are ungrouped */ - aList->SetPickedItemStatus( UNDO_REDO::UNGROUP, ii ); - - if( eda_item->IsBOARD_ITEM() ) - { - BOARD_ITEM* boardItem = static_cast( eda_item ); - - if( EDA_GROUP* group = boardItem->GetParentGroup() ) - { - aList->SetPickedItemGroupId( group->AsEdaItem()->m_Uuid, ii ); - - group->RemoveItem( boardItem ); - } - } - - break; - - case UNDO_REDO::UNGROUP: /* ungrouped items are re-added to their previuos groups */ - aList->SetPickedItemStatus( UNDO_REDO::REGROUP, ii ); - - if( eda_item->IsBOARD_ITEM() ) - { - BOARD_ITEM* boardItem = static_cast( eda_item ); - - PCB_GROUP* group = dynamic_cast( - GetBoard()->GetItem( aList->GetPickedItemGroupId( ii ) ) ); - - if( group ) - group->AddItem( boardItem ); - } - - break; - case UNDO_REDO::DRILLORIGIN: case UNDO_REDO::GRIDORIGIN: { @@ -573,13 +520,39 @@ void PCB_BASE_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList ) { FOOTPRINT* fp = static_cast( eda_item ); fp->InvalidateComponentClassCache(); - GetBoard()->GetComponentClassManager().RebuildRequiredCaches( fp ); + m_pcb->GetComponentClassManager().RebuildRequiredCaches( fp ); } } if( not_found ) wxMessageBox( _( "Incomplete undo/redo operation: some items not found" ) ); + // We have now swapped all the group parent and group member pointers. But it is a + // risky proposition to bet on the pointers being invariant, so validate them all. + for( int ii = 0; ii < (int) aList->GetCount(); ++ii ) + { + ITEM_PICKER& wrapper = aList->GetItemWrapper( ii ); + + BOARD_ITEM* parentGroup = GetBoard()->ResolveItem( wrapper.GetGroupId(), true ); + wrapper.GetItem()->SetParentGroup( dynamic_cast( parentGroup ) ); + + if( EDA_GROUP* group = dynamic_cast( wrapper.GetItem() ) ) + { + // Items list may contain dodgy pointers, so don't use RemoveAll() + group->GetItems().clear(); + + for( const KIID& member : wrapper.GetGroupMembers() ) + { + if( BOARD_ITEM* memberItem = GetBoard()->ResolveItem( member, true ) ) + group->AddItem( memberItem ); + } + } + + // And prepare for a redo by updating group info based on current image + if( EDA_ITEM* item = wrapper.GetLink() ) + wrapper.SetLink( item ); + } + if( IsType( FRAME_PCB_EDITOR ) ) { if( reBuild_ratsnest || deep_reBuild_ratsnest ) @@ -663,9 +636,6 @@ void PCB_BASE_EDIT_FRAME::ClearListAndDeleteItems( PICKED_ITEMS_LIST* aList ) wxASSERT_MSG( item->HasFlag( UR_TRANSIENT ), "Item on undo/redo list not owned by undo/redo!" ); - if( item->IsBOARD_ITEM() ) - static_cast( item )->SetParentGroup( nullptr ); - delete item; } ); } diff --git a/pcbnew/zone_filler.cpp b/pcbnew/zone_filler.cpp index 387cf133e3..3718c4e95e 100644 --- a/pcbnew/zone_filler.cpp +++ b/pcbnew/zone_filler.cpp @@ -2396,9 +2396,6 @@ void ZONE_FILLER::buildThermalSpokes( const ZONE* aZone, PCB_LAYER_ID aLayer, spokesBox = dummy_pad.GetBoundingBox( aLayer ); position = pad->ShapePos( aLayer ); orientation = pad->GetOrientation(); - - // Remove group membership from dummy item before deleting - dummy_pad.SetParentGroup( nullptr ); } else if( via ) { diff --git a/qa/pcbnew_utils/board_test_utils.cpp b/qa/pcbnew_utils/board_test_utils.cpp index ff994634af..67e699a115 100644 --- a/qa/pcbnew_utils/board_test_utils.cpp +++ b/qa/pcbnew_utils/board_test_utils.cpp @@ -130,7 +130,7 @@ void LoadBoard( SETTINGS_MANAGER& aSettingsManager, const wxString& aRelPath, BOARD_ITEM& RequireBoardItemWithTypeAndId( const BOARD& aBoard, KICAD_T aItemType, const KIID& aID ) { - BOARD_ITEM* item = aBoard.GetItem( aID ); + BOARD_ITEM* item = aBoard.ResolveItem( aID, true ); BOOST_REQUIRE( item ); BOOST_REQUIRE_EQUAL( item->Type(), aItemType ); diff --git a/qa/tests/eeschema/test_sch_reference_list.cpp b/qa/tests/eeschema/test_sch_reference_list.cpp index 026d9f2e3c..f2e385440e 100644 --- a/qa/tests/eeschema/test_sch_reference_list.cpp +++ b/qa/tests/eeschema/test_sch_reference_list.cpp @@ -81,7 +81,7 @@ SCH_SYMBOL* TEST_SCH_REFERENCE_LIST_FIXTURE::getSymbolByKIID( wxString aK SCH_SHEET_PATH* aSymbolPath ) { KIID symKIID( aKIID ); - SCH_ITEM* foundItem = m_schematic.GetItem( symKIID, aSymbolPath ); + SCH_ITEM* foundItem = m_schematic.ResolveItem( symKIID, aSymbolPath ); SCH_SYMBOL* symbol = dynamic_cast( foundItem ); return symbol; diff --git a/qa/tests/pcbnew/drc/test_drc_courtyard_invalid.cpp b/qa/tests/pcbnew/drc/test_drc_courtyard_invalid.cpp index 72b3ca42dc..992d456650 100644 --- a/qa/tests/pcbnew/drc/test_drc_courtyard_invalid.cpp +++ b/qa/tests/pcbnew/drc/test_drc_courtyard_invalid.cpp @@ -234,7 +234,7 @@ static bool InvalidMatchesExpected( BOARD& aBoard, const PCB_MARKER& aMarker, const COURTYARD_INVALID_INFO& aInvalid ) { auto reporter = std::static_pointer_cast( aMarker.GetRCItem() ); - const FOOTPRINT* item_a = dynamic_cast( aBoard.GetItem( reporter->GetMainItemID() ) ); + const FOOTPRINT* item_a = dynamic_cast( aBoard.ResolveItem( reporter->GetMainItemID() ) ); // This one is more than just a mismatch! if( reporter->GetAuxItemID() != niluuid ) diff --git a/qa/tests/pcbnew/drc/test_drc_courtyard_overlap.cpp b/qa/tests/pcbnew/drc/test_drc_courtyard_overlap.cpp index 66ed872813..5e7b14b187 100644 --- a/qa/tests/pcbnew/drc/test_drc_courtyard_overlap.cpp +++ b/qa/tests/pcbnew/drc/test_drc_courtyard_overlap.cpp @@ -390,8 +390,8 @@ static bool CollisionMatchesExpected( BOARD& aBoard, const PCB_MARKER& aMarker, { auto reporter = std::static_pointer_cast( aMarker.GetRCItem() ); - const FOOTPRINT* item_a = dynamic_cast( aBoard.GetItem( reporter->GetMainItemID() ) ); - const FOOTPRINT* item_b = dynamic_cast( aBoard.GetItem( reporter->GetAuxItemID() ) ); + const FOOTPRINT* item_a = dynamic_cast( aBoard.ResolveItem( reporter->GetMainItemID(), true ) ); + const FOOTPRINT* item_b = dynamic_cast( aBoard.ResolveItem( reporter->GetAuxItemID(), true ) ); // can't find the items! if( !item_a || !item_b ) diff --git a/qa/tests/pcbnew/test_board_item.cpp b/qa/tests/pcbnew/test_board_item.cpp index 521d8dd692..4d993b12b7 100644 --- a/qa/tests/pcbnew/test_board_item.cpp +++ b/qa/tests/pcbnew/test_board_item.cpp @@ -191,7 +191,7 @@ BOOST_AUTO_TEST_CASE( Move ) if( originalDimension != nullptr ) originalDimension->Update(); - auto item = std::unique_ptr( aOriginalItem->Duplicate() ); + auto item = std::unique_ptr( aOriginalItem->Duplicate( IGNORE_PARENT_GROUP ) ); VECTOR2I originalPos = item->GetPosition(); // Move to a point, then go back. @@ -235,7 +235,7 @@ BOOST_AUTO_TEST_CASE( Rotate ) if( originalDimension != nullptr ) originalDimension->Update(); - auto item = std::unique_ptr( aOriginalItem->Duplicate() ); + auto item = std::unique_ptr( aOriginalItem->Duplicate( IGNORE_PARENT_GROUP ) ); // Four equivalent 90 degree rotations are an identity. @@ -276,7 +276,7 @@ BOOST_AUTO_TEST_CASE( FlipLeftRight ) if( originalDimension != nullptr ) originalDimension->Update(); - auto item = std::unique_ptr( aOriginalItem->Duplicate() ); + auto item = std::unique_ptr( aOriginalItem->Duplicate( IGNORE_PARENT_GROUP ) ); // Two equivalent flips are an identity. @@ -315,7 +315,7 @@ BOOST_AUTO_TEST_CASE( FlipUpDown ) if( originalDimension != nullptr ) originalDimension->Update(); - auto item = std::unique_ptr( aOriginalItem->Duplicate() ); + auto item = std::unique_ptr( aOriginalItem->Duplicate( IGNORE_PARENT_GROUP ) ); // Two equivalent flips are an identity. diff --git a/qa/tests/pcbnew/test_zone_filler.cpp b/qa/tests/pcbnew/test_zone_filler.cpp index 6e78dc1859..341f9ac754 100644 --- a/qa/tests/pcbnew/test_zone_filler.cpp +++ b/qa/tests/pcbnew/test_zone_filler.cpp @@ -108,11 +108,11 @@ BOOST_FIXTURE_TEST_CASE( BasicZoneFills, ZONE_FILL_TEST_FIXTURE ) { if( aItem->GetErrorCode() == DRCE_CLEARANCE ) { - BOARD_ITEM* item_a = m_board->GetItem( aItem->GetMainItemID() ); + BOARD_ITEM* item_a = m_board->ResolveItem( aItem->GetMainItemID() ); PAD* pad_a = dynamic_cast( item_a ); PCB_TRACK* trk_a = dynamic_cast( item_a ); - BOARD_ITEM* item_b = m_board->GetItem( aItem->GetAuxItemID() ); + BOARD_ITEM* item_b = m_board->ResolveItem( aItem->GetAuxItemID() ); PAD* pad_b = dynamic_cast( item_b ); PCB_TRACK* trk_b = dynamic_cast( item_b ); @@ -210,7 +210,7 @@ BOOST_DATA_TEST_CASE_F( ZONE_FILL_TEST_FIXTURE, RegressionZoneFillTests, if( violations.empty() ) { BOOST_CHECK_EQUAL( 1, 1 ); // quiet "did not check any assertions" warning - BOOST_TEST_MESSAGE( (const char*)(wxString::Format( "Zone fill regression: %s passed", relPath ).utf8_str()) ); + BOOST_TEST_MESSAGE( wxString::Format( "Zone fill regression: %s passed", relPath ) ); } else { @@ -222,7 +222,7 @@ BOOST_DATA_TEST_CASE_F( ZONE_FILL_TEST_FIXTURE, RegressionZoneFillTests, for( const DRC_ITEM& item : violations ) BOOST_TEST_MESSAGE( item.ShowReport( &unitsProvider, RPT_SEVERITY_ERROR, itemMap ) ); - BOOST_ERROR( (const char*)(wxString::Format( "Zone fill regression: %s failed", relPath ).utf8_str()) ); + BOOST_ERROR( wxString::Format( "Zone fill regression: %s failed", relPath ) ); } } @@ -257,7 +257,7 @@ BOOST_DATA_TEST_CASE_F( ZONE_FILL_TEST_FIXTURE, RegressionSliverZoneFillTests, if( violations.empty() ) { BOOST_CHECK_EQUAL( 1, 1 ); // quiet "did not check any assertions" warning - BOOST_TEST_MESSAGE( (const char*)(wxString::Format( "Zone fill copper sliver regression: %s passed", relPath ).utf8_str()) ); + BOOST_TEST_MESSAGE( wxString::Format( "Zone fill copper sliver regression: %s passed", relPath ) ); } else { @@ -269,7 +269,7 @@ BOOST_DATA_TEST_CASE_F( ZONE_FILL_TEST_FIXTURE, RegressionSliverZoneFillTests, for( const DRC_ITEM& item : violations ) BOOST_TEST_MESSAGE( item.ShowReport( &unitsProvider, RPT_SEVERITY_ERROR, itemMap ) ); - BOOST_ERROR( (const char*)(wxString::Format( "Zone fill copper sliver regression: %s failed", relPath ).utf8_str()) ); + BOOST_ERROR( wxString::Format( "Zone fill copper sliver regression: %s failed", relPath ) ); } } @@ -328,14 +328,12 @@ BOOST_FIXTURE_TEST_CASE( RegressionNetTie, ZONE_FILL_TEST_FIXTURE ) std::shared_ptr pad_shape( pad->GetEffectiveShape( layer ) ); int clearance = pad_shape->GetClearance( a_shape.get() ); BOOST_CHECK_MESSAGE( pad->GetNetCode() == zone->GetNetCode() || clearance != 0, - "Pad " << pad->GetNumber() << " from Footprint " - << pad->GetParentFootprint() - ->GetReferenceAsString() - .ToStdString() - << " has net code " - << pad->GetNetname().ToStdString() - << " and is connected to zone with net code " - << zone->GetNetname().ToStdString() ); + wxString::Format( "Pad %s from Footprint %s has net code %s and " + "is connected to zone with net code %s", + pad->GetNumber(), + pad->GetParentFootprint()->GetReferenceAsString(), + pad->GetNetname(), + zone->GetNetname() ) ); } } }