From f58fc0b952af75ea1388b2150889ee983f2d80b6 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Mon, 19 May 2025 21:36:46 +0100 Subject: [PATCH] Rewrite GROUP undo based on uuids. This also removes the GROUP/UNGROUP-specific undo actions. This also fixes a bunch of undo bugs when duplicating group members, creating pins, etc. This also fixes some undo bugs when dividing wires etc. This also fixes some bugs with new sch items not being created within an entered group. --- .../3d_canvas/create_3Dgraphic_brd_items.cpp | 3 - 3d-viewer/3d_canvas/create_layer_items.cpp | 4 +- common/commit.cpp | 55 ++---- common/dialogs/dialog_group_properties.cpp | 4 +- common/eda_group.cpp | 64 +++++- common/eda_group.h | 33 +--- common/eda_item.cpp | 26 +++ common/rc_item.cpp | 13 +- common/tool/group_tool.cpp | 35 +++- common/undo_redo_container.cpp | 92 +++------ cvpcb/display_footprints_frame.cpp | 6 +- eeschema/bus-wire-junction.cpp | 2 +- eeschema/cross-probing.cpp | 2 +- eeschema/dialogs/dialog_erc.cpp | 9 +- eeschema/erc/erc_item.cpp | 8 +- eeschema/lib_symbol.cpp | 4 +- eeschema/sch_bitmap.cpp | 2 - eeschema/sch_bus_entry.cpp | 2 - eeschema/sch_commit.cpp | 182 +++++++----------- eeschema/sch_commit.h | 4 +- eeschema/sch_design_block_utils.cpp | 24 ++- eeschema/sch_edit_frame.cpp | 11 +- eeschema/sch_edit_frame.h | 2 +- eeschema/sch_field.cpp | 2 - eeschema/sch_group.cpp | 112 +---------- eeschema/sch_group.h | 34 +--- .../cadstar/cadstar_sch_archive_loader.cpp | 6 +- eeschema/sch_io/eagle/sch_io_eagle.cpp | 2 +- eeschema/sch_io/http_lib/sch_io_http_lib.cpp | 10 +- .../kicad_sexpr/sch_io_kicad_sexpr_parser.cpp | 18 +- eeschema/sch_item.cpp | 97 ++++++---- eeschema/sch_item.h | 12 +- eeschema/sch_junction.cpp | 2 - eeschema/sch_line.cpp | 6 +- eeschema/sch_line.h | 2 +- eeschema/sch_marker.cpp | 11 +- eeschema/sch_no_connect.cpp | 2 - eeschema/sch_screen.cpp | 7 - eeschema/sch_shape.cpp | 2 - eeschema/sch_sheet.cpp | 2 - eeschema/sch_sheet_path.cpp | 11 +- eeschema/sch_sheet_path.h | 5 +- eeschema/sch_symbol.cpp | 2 - eeschema/sch_table.cpp | 2 - eeschema/sch_text.cpp | 2 - eeschema/schematic.cpp | 8 +- eeschema/schematic.h | 5 +- eeschema/schematic_undo_redo.cpp | 70 +++---- eeschema/sim/simulator_frame_ui.cpp | 4 +- eeschema/symbol.cpp | 11 -- eeschema/symbol.h | 2 +- .../symbol_editor/symbol_editor_undo_redo.cpp | 5 - eeschema/tools/sch_drawing_tools.cpp | 6 +- eeschema/tools/sch_edit_tool.cpp | 33 ++-- eeschema/tools/sch_editor_control.cpp | 30 +-- eeschema/tools/sch_find_replace_tool.cpp | 9 - eeschema/tools/sch_group_tool.cpp | 14 +- eeschema/tools/sch_inspection_tool.cpp | 2 +- eeschema/tools/sch_line_wire_bus_tool.cpp | 71 +++---- eeschema/tools/sch_line_wire_bus_tool.h | 10 +- eeschema/tools/sch_move_tool.cpp | 30 ++- eeschema/tools/sch_selection_tool.cpp | 18 +- .../tools/symbol_editor_drawing_tools.cpp | 2 +- eeschema/tools/symbol_editor_edit_tool.cpp | 6 +- eeschema/tools/symbol_editor_move_tool.cpp | 34 ++-- eeschema/tools/symbol_editor_pin_tool.cpp | 19 +- eeschema/tools/symbol_editor_pin_tool.h | 4 +- include/board_item.h | 8 +- include/commit.h | 16 +- include/eda_draw_frame.h | 5 +- include/eda_item.h | 11 +- include/pcb_base_frame.h | 2 +- include/undo_redo_container.h | 53 +---- pcbnew/api/api_handler_pcb.cpp | 6 +- pcbnew/board.cpp | 64 +++--- pcbnew/board.h | 5 +- pcbnew/board_commit.cpp | 146 +++++--------- pcbnew/board_commit.h | 3 +- pcbnew/board_item.cpp | 110 ++++++----- pcbnew/cross-probing.cpp | 3 - pcbnew/dialogs/dialog_cleanup_graphics.cpp | 17 +- .../dialog_cleanup_tracks_and_vias.cpp | 13 +- pcbnew/dialogs/dialog_drc.cpp | 17 +- pcbnew/dialogs/dialog_footprint_checker.cpp | 12 +- pcbnew/dialogs/dialog_generators.cpp | 4 +- pcbnew/dialogs/dialog_global_deletion.cpp | 8 - pcbnew/dialogs/dialog_pad_properties.cpp | 4 - pcbnew/dialogs/dialog_shape_properties.cpp | 7 - .../drc/drc_test_provider_edge_clearance.cpp | 6 - pcbnew/edit.cpp | 3 +- pcbnew/footprint.cpp | 44 ++--- pcbnew/footprint.h | 5 +- pcbnew/footprint_editor_utils.cpp | 3 +- pcbnew/footprint_libraries_utils.cpp | 16 +- pcbnew/footprint_preview_panel.cpp | 2 +- pcbnew/footprint_viewer_frame.cpp | 2 +- pcbnew/generators/pcb_tuning_pattern.cpp | 12 -- pcbnew/kicad_clipboard.cpp | 5 +- .../netlist_reader/board_netlist_updater.cpp | 9 +- pcbnew/pad.cpp | 2 +- pcbnew/pcb_base_frame.cpp | 28 +-- pcbnew/pcb_design_block_utils.cpp | 6 + pcbnew/pcb_generator.h | 2 +- pcbnew/pcb_group.cpp | 111 +---------- pcbnew/pcb_group.h | 34 +--- .../cadstar/cadstar_pcb_archive_loader.cpp | 6 +- .../pcb_io/cadstar/pcb_io_cadstar_archive.cpp | 2 +- pcbnew/pcb_io/eagle/pcb_io_eagle.cpp | 27 ++- pcbnew/pcb_io/geda/pcb_io_geda.cpp | 2 +- .../kicad_legacy/pcb_io_kicad_legacy.cpp | 2 +- .../pcb_io/kicad_sexpr/pcb_io_kicad_sexpr.cpp | 2 +- .../kicad_sexpr/pcb_io_kicad_sexpr_parser.cpp | 30 ++- pcbnew/pcb_marker.cpp | 10 +- pcbnew/pcb_painter.cpp | 5 +- pcbnew/tools/array_tool.cpp | 12 +- pcbnew/tools/board_inspection_tool.cpp | 6 +- pcbnew/tools/drawing_tool.cpp | 15 -- pcbnew/tools/edit_tool.cpp | 131 +++---------- pcbnew/tools/edit_tool.h | 9 - pcbnew/tools/edit_tool_move_fct.cpp | 6 +- pcbnew/tools/multichannel_tool.cpp | 77 ++++---- pcbnew/tools/pad_tool.cpp | 8 +- pcbnew/tools/pcb_control.cpp | 37 ++-- pcbnew/tools/pcb_group_tool.cpp | 8 +- pcbnew/tools/pcb_point_editor.cpp | 6 +- pcbnew/tools/pcb_selection_tool.cpp | 8 +- pcbnew/tools/position_relative_tool.cpp | 2 +- pcbnew/tracks_cleaner.cpp | 8 - pcbnew/undo_redo.cpp | 90 +++------ pcbnew/zone_filler.cpp | 3 - qa/pcbnew_utils/board_test_utils.cpp | 2 +- qa/tests/eeschema/test_sch_reference_list.cpp | 2 +- .../pcbnew/drc/test_drc_courtyard_invalid.cpp | 2 +- .../pcbnew/drc/test_drc_courtyard_overlap.cpp | 4 +- qa/tests/pcbnew/test_board_item.cpp | 8 +- qa/tests/pcbnew/test_zone_filler.cpp | 26 ++- 136 files changed, 1058 insertions(+), 1631 deletions(-) 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() ) ); } } }