From 83ae28f04f9cfcb9975ea9f578bc2c6965f1fba4 Mon Sep 17 00:00:00 2001 From: Ian McInerney Date: Sat, 3 Apr 2021 19:08:04 +0100 Subject: [PATCH] Fix selection when pasting/duplicating items The item flags weren't getting reset after placing the new items, so the next operation on them was having weird behavior. Fixes https://gitlab.com/kicad/code/kicad/issues/7528 --- pcbnew/tools/edit_tool.cpp | 38 ++++++++++++++++++++++-------------- pcbnew/tools/pcb_control.cpp | 24 +++++++++++------------ 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp index ae3ac9472b..2a528e9ac7 100644 --- a/pcbnew/tools/edit_tool.cpp +++ b/pcbnew/tools/edit_tool.cpp @@ -628,7 +628,7 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) return 0; LSET item_layers = selection.GetSelectionLayers(); - bool unselect = selection.IsHover(); // N.B. This must be saved before the re-selection + bool is_hover = selection.IsHover(); // N.B. This must be saved before the re-selection // below VECTOR2I pickedReferencePoint; @@ -710,14 +710,15 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) if( aPickReference && !pickReferencePoint( _( "Select reference point for move..." ), "", "", pickedReferencePoint ) ) { - if( unselect ) + if( is_hover ) m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true ); editFrame->PopTool( tool ); return 0; } - std::vector sel_items; + std::vector sel_items; // All the items operated on by the move below + std::vector orig_items; // All the original items in the selection for( EDA_ITEM* item : selection ) { @@ -725,7 +726,10 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) FOOTPRINT* footprint = dynamic_cast( item ); if( boardItem ) + { + orig_items.push_back( boardItem ); sel_items.push_back( boardItem ); + } if( footprint ) { @@ -963,8 +967,12 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) // Remove the dynamic ratsnest from the screen m_toolMgr->RunAction( PCB_ACTIONS::hideDynamicRatsnest, true ); - if( unselect ) - m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true ); + // Unselect all items to update flags + m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true ); + + // Reselect items if they were already selected and we completed the move + if( !is_hover && !restore_state ) + m_toolMgr->RunAction( PCB_ACTIONS::selectItems, true, &orig_items ); editFrame->PopTool( tool ); @@ -1980,7 +1988,7 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) bool increment = aEvent.IsAction( &PCB_ACTIONS::duplicateIncrement ); // Be sure that there is at least one item that we can modify - const auto& selection = m_selectionTool->RequestSelection( + const PCB_SELECTION& selection = m_selectionTool->RequestSelection( []( const VECTOR2I&, GENERAL_COLLECTOR& aCollector, PCB_SELECTION_TOOL* sTool ) { std::set added_items; @@ -2119,19 +2127,19 @@ int EDIT_TOOL::Duplicate( const TOOL_EVENT& aEvent ) editFrame->DisplayToolMsg( wxString::Format( _( "Duplicated %d item(s)" ), (int) new_items.size() ) ); + // TODO(ISM): This line can't be used to activate the tool until we allow multiple activations + // m_toolMgr->RunAction( PCB_ACTIONS::move, true ); + // Instead we have to create the event and call the tool's function + // directly + // If items were duplicated, pick them up // this works well for "dropping" copies around and pushes the commit TOOL_EVENT evt = PCB_ACTIONS::move.MakeEvent(); - bool move_cancelled = Move( evt ) == -1; - - // After moving the new items, we need to refresh the group and view flags - m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true ); + Move( evt ); - if( !is_hover && !move_cancelled ) - { - // Do not select new items if they've been deleted again by cancelling Move() - m_toolMgr->RunAction( PCB_ACTIONS::selectItems, true, &new_items ); - } + // Deslect the duplicated item if we originally started as a hover selection + if( is_hover ) + m_toolMgr->RunAction( PCB_ACTIONS::selectionClear, true ); } return 0; diff --git a/pcbnew/tools/pcb_control.cpp b/pcbnew/tools/pcb_control.cpp index ada5cd4c76..480f9950c7 100644 --- a/pcbnew/tools/pcb_control.cpp +++ b/pcbnew/tools/pcb_control.cpp @@ -850,7 +850,9 @@ int PCB_CONTROL::placeBoardItems( std::vector& aItems, bool aIsNew, PCB_SELECTION_TOOL* selectionTool = m_toolMgr->GetTool(); EDIT_TOOL* editTool = m_toolMgr->GetTool(); - PCB_SELECTION& selection = selectionTool->GetSelection(); + + std::vector itemsToSel; + itemsToSel.reserve( aItems.size() ); for( BOARD_ITEM* item : aItems ) { @@ -897,20 +899,18 @@ int PCB_CONTROL::placeBoardItems( std::vector& aItems, bool aIsNew, else editTool->GetCurrentCommit()->Added( item ); - // Matching the logic of PCB_SELECTION_TOOL::select for PCB_GROUP_T, there - // is a distinction between which items are SetSelected and which are in - // the selection object. Top-level groups or items not in groups are - // added to the selection object (via selection.Add(), below), but all - // items have SetSelected called. This is because much of the selection - // management logic (e.g. move) recursively acts on groups in the - // selection, so descendents of groups should not be in the selection - // object. - item->SetSelected(); - + // We only need to add the items that aren't inside a group currently selected + // to the selection. If an item is inside a group and that group is selected, + // then the selection tool will select it for us. if( !item->GetParentGroup() || !alg::contains( aItems, item->GetParentGroup() ) ) - selection.Add( item ); + itemsToSel.push_back( item ); } + // Select the items that should be selected + m_toolMgr->RunAction( PCB_ACTIONS::selectItems, true, &itemsToSel ); + + PCB_SELECTION& selection = selectionTool->GetSelection(); + if( selection.Size() > 0 ) { if( aAnchorAtOrigin )