From 9113f93ebef736ce57e0466a1c49f14fd0fdbd4c Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Thu, 10 Dec 2020 01:33:24 +0000 Subject: [PATCH] Uniformly apply current locking strategy. I make no claims that this is the right strategy, but at least it's consistent now. Fixes https://gitlab.com/kicad/code/kicad/issues/6369 --- include/tool/selection.h | 7 --- pcbnew/tools/convert_tool.cpp | 16 +++--- pcbnew/tools/edit_tool.cpp | 50 ++++++++++--------- pcbnew/tools/placement_tool.cpp | 24 +++++++-- pcbnew/tools/position_relative_tool.cpp | 8 ++- pcbnew/tools/selection_tool.cpp | 66 +++++-------------------- pcbnew/tools/selection_tool.h | 4 -- qa/qa_utils/mocks.cpp | 8 --- 8 files changed, 71 insertions(+), 112 deletions(-) diff --git a/include/tool/selection.h b/include/tool/selection.h index 591b2f8eb9..d06260304e 100644 --- a/include/tool/selection.h +++ b/include/tool/selection.h @@ -293,12 +293,5 @@ protected: using VIEW_GROUP::Remove; }; -enum SELECTION_LOCK_FLAGS -{ - SELECTION_UNLOCKED = 0, - SELECTION_LOCK_OVERRIDE = 1, - SELECTION_LOCKED = 2 -}; - #endif diff --git a/pcbnew/tools/convert_tool.cpp b/pcbnew/tools/convert_tool.cpp index 87de9f349d..2feddf17ab 100644 --- a/pcbnew/tools/convert_tool.cpp +++ b/pcbnew/tools/convert_tool.cpp @@ -123,8 +123,6 @@ int CONVERT_TOOL::LinesToPoly( const TOOL_EVENT& aEvent ) auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { - EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); - for( int i = aCollector.GetCount() - 1; i >= 0; --i ) { BOARD_ITEM* item = aCollector[i]; @@ -366,9 +364,6 @@ int CONVERT_TOOL::PolyToLines( const TOOL_EVENT& aEvent ) auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { - EditToolSelectionFilter( aCollector, - EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); - for( int i = aCollector.GetCount() - 1; i >= 0; --i ) { BOARD_ITEM* item = aCollector[i]; @@ -399,7 +394,9 @@ int CONVERT_TOOL::PolyToLines( const TOOL_EVENT& aEvent ) aCollector.Remove( item ); } } - } ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; @@ -563,9 +560,6 @@ int CONVERT_TOOL::SegmentToArc( const TOOL_EVENT& aEvent ) auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { - EditToolSelectionFilter( aCollector, - EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); - for( int i = aCollector.GetCount() - 1; i >= 0; --i ) { BOARD_ITEM* item = aCollector[i]; @@ -577,7 +571,9 @@ int CONVERT_TOOL::SegmentToArc( const TOOL_EVENT& aEvent ) aCollector.Remove( item ); } } - } ); + }, + nullptr, + true /* confirm if contains locked items */ ); EDA_ITEM* source = selection.Front(); VECTOR2I start, end, mid; diff --git a/pcbnew/tools/edit_tool.cpp b/pcbnew/tools/edit_tool.cpp index 9603dd8ec1..4640c25fc4 100644 --- a/pcbnew/tools/edit_tool.cpp +++ b/pcbnew/tools/edit_tool.cpp @@ -301,10 +301,15 @@ int EDIT_TOOL::Drag( const TOOL_EVENT& aEvent ) if( aEvent.IsAction( &PCB_ACTIONS::dragFreeAngle ) ) mode |= PNS::DM_FREE_ANGLE; - // deal with locked items (override lock or abort the operation) - SELECTION_LOCK_FLAGS lockFlags = m_selectionTool->CheckLock(); + PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection( + []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) + { + EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS, sTool ); + }, + nullptr, + true /* confirm if contains locked items */ ); - if( lockFlags == SELECTION_LOCKED ) + if( selection.Empty() ) return 0; invokeInlineRouter( mode ); @@ -361,12 +366,14 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) VECTOR2I pickedReferencePoint; // Now filter out locked pads. We cannot do this in the first RequestSelection() as we need - // the item_layers when a pad is the selection front (ie: will become curr_tiem). + // the item_layers when a pad is the selection front. selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS, sTool ); - } ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; @@ -474,13 +481,6 @@ int EDIT_TOOL::doMoveSelection( TOOL_EVENT aEvent, bool aPickReference ) else if( !m_dragging && !evt->IsAction( &ACTIONS::refreshPreview ) ) { // Prepare to start dragging - - // deal with locked items (override lock or abort the operation) - SELECTION_LOCK_FLAGS lockFlags = m_selectionTool->CheckLock(); - - if( lockFlags == SELECTION_LOCKED ) - break; - if( !( evt->IsAction( &PCB_ACTIONS::move ) || evt->IsAction( &PCB_ACTIONS::moveWithReference ) ) && isInteractiveDragEnabled() ) @@ -663,7 +663,9 @@ int EDIT_TOOL::ChangeTrackWidth( const TOOL_EVENT& aEvent ) []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); - } ); + }, + nullptr, + true /* confirm if contains locked items */ ); for( EDA_ITEM* item : selection ) { @@ -725,11 +727,11 @@ int EDIT_TOOL::FilletTracks( const TOOL_EVENT& aEvent ) auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { - EditToolSelectionFilter( aCollector, - EXCLUDE_LOCKED | EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, + EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool ); }, - nullptr, !m_dragging ); + nullptr, + !m_dragging /* confirm if contains locked items */ ); if( selection.Size() < 2 ) { @@ -979,7 +981,8 @@ int EDIT_TOOL::Rotate( const TOOL_EVENT& aEvent ) EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool ); }, - nullptr, ! m_dragging ); + nullptr, + !m_dragging /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; @@ -1081,7 +1084,8 @@ int EDIT_TOOL::Mirror( const TOOL_EVENT& aEvent ) EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool ); }, - nullptr, !m_dragging ); + nullptr, + !m_dragging /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; @@ -1180,7 +1184,8 @@ int EDIT_TOOL::Flip( const TOOL_EVENT& aEvent ) EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool ); }, - nullptr, !m_dragging ); + nullptr, + !m_dragging /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; @@ -1489,10 +1494,11 @@ int EDIT_TOOL::MoveExact( const TOOL_EVENT& aEvent ) const auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) { - EditToolSelectionFilter( aCollector, - EXCLUDE_LOCKED | EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, + EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED_PADS | EXCLUDE_TRANSIENTS, sTool ); - } ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp index 13e5634acd..e25e6b42ae 100644 --- a/pcbnew/tools/placement_tool.cpp +++ b/pcbnew/tools/placement_tool.cpp @@ -139,12 +139,15 @@ int ALIGN_DISTRIBUTE_TOOL::selectTarget( ALIGNMENT_RECTS& aItems, ALIGNMENT_RECT template< typename T > -size_t ALIGN_DISTRIBUTE_TOOL::GetSelections( ALIGNMENT_RECTS& aItems, ALIGNMENT_RECTS& aLocked, T aCompare ) +size_t ALIGN_DISTRIBUTE_TOOL::GetSelections( ALIGNMENT_RECTS& aItems, ALIGNMENT_RECTS& aLocked, + T aCompare ) { PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) - { EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); } ); + { + EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); + } ); if( selection.Size() <= 1 ) return 0; @@ -152,7 +155,10 @@ size_t ALIGN_DISTRIBUTE_TOOL::GetSelections( ALIGNMENT_RECTS& aItems, ALIGNMENT_ std::vector lockedItems; selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) - { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED, sTool ); }, &lockedItems ); + { + EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED, sTool ); + }, + &lockedItems ); aItems = GetBoundingBoxes( selection ); aLocked = GetBoundingBoxes( lockedItems ); @@ -395,7 +401,11 @@ int ALIGN_DISTRIBUTE_TOOL::DistributeHorizontally( const TOOL_EVENT& aEvent ) { PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) - { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } ); + { + EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Size() <= 1 ) return 0; @@ -497,7 +507,11 @@ int ALIGN_DISTRIBUTE_TOOL::DistributeVertically( const TOOL_EVENT& aEvent ) { PCBNEW_SELECTION& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) - { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } ); + { + EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Size() <= 1 ) return 0; diff --git a/pcbnew/tools/position_relative_tool.cpp b/pcbnew/tools/position_relative_tool.cpp index f2ceea0185..4016ce35d9 100644 --- a/pcbnew/tools/position_relative_tool.cpp +++ b/pcbnew/tools/position_relative_tool.cpp @@ -64,11 +64,15 @@ bool POSITION_RELATIVE_TOOL::Init() int POSITION_RELATIVE_TOOL::PositionRelative( const TOOL_EVENT& aEvent ) { - PCB_BASE_FRAME* editFrame = getEditFrame(); + PCB_BASE_FRAME* editFrame = getEditFrame(); const auto& selection = m_selectionTool->RequestSelection( []( const VECTOR2I& aPt, GENERAL_COLLECTOR& aCollector, SELECTION_TOOL* sTool ) - { EditToolSelectionFilter( aCollector, EXCLUDE_LOCKED | EXCLUDE_TRANSIENTS, sTool ); } ); + { + EditToolSelectionFilter( aCollector, EXCLUDE_TRANSIENTS, sTool ); + }, + nullptr, + true /* confirm if contains locked items */ ); if( selection.Empty() ) return 0; diff --git a/pcbnew/tools/selection_tool.cpp b/pcbnew/tools/selection_tool.cpp index 5fc29e0447..5fd4c2f26e 100644 --- a/pcbnew/tools/selection_tool.cpp +++ b/pcbnew/tools/selection_tool.cpp @@ -103,7 +103,6 @@ SELECTION_TOOL::SELECTION_TOOL() : m_exclusive_or( false ), m_multiple( false ), m_skip_heuristics( false ), - m_locked( true ), m_enteredGroup( nullptr ), m_priv( std::make_unique() ) { @@ -175,7 +174,6 @@ bool SELECTION_TOOL::Init() void SELECTION_TOOL::Reset( RESET_REASON aReason ) { m_frame = getEditFrame(); - m_locked = true; if( m_enteredGroup ) ExitGroup(); @@ -475,10 +473,19 @@ PCBNEW_SELECTION& SELECTION_TOOL::RequestSelection( CLIENT_SELECTION_FILTER aCli m_selection.ClearReferencePoint(); } - if ( aConfirmLockedItems && CheckLock() == SELECTION_LOCKED ) + if( aConfirmLockedItems ) { - ClearSelection(); - return m_selection; + // Check if the selection contains locked items + for( EDA_ITEM* item : m_selection ) + { + if( static_cast( item )->IsLocked() ) + { + if( !IsOK( m_frame, _( "Selection contains locked items. Continue anyway?" ) ) ) + ClearSelection(); + + break; + } + } } if( aClientFilter ) @@ -821,50 +828,6 @@ bool SELECTION_TOOL::selectMultiple() } -SELECTION_LOCK_FLAGS SELECTION_TOOL::CheckLock() -{ - if( !m_locked || m_isFootprintEditor ) - return SELECTION_UNLOCKED; - - bool containsLocked = false; - - // Check if the selection contains locked items - for( EDA_ITEM* item : m_selection ) - { - switch( item->Type() ) - { - case PCB_FOOTPRINT_T: - if( static_cast( item )->IsLocked() ) - containsLocked = true; - break; - - case PCB_FP_SHAPE_T: - case PCB_FP_TEXT_T: - case PCB_FP_ZONE_T: - if( static_cast( item->GetParent() )->IsLocked() ) - containsLocked = true; - break; - - default: // suppress warnings - break; - } - } - - if( containsLocked ) - { - if( IsOK( m_frame, _( "Selection contains locked items. Do you want to continue?" ) ) ) - { - m_locked = false; - return SELECTION_LOCK_OVERRIDE; - } - else - return SELECTION_LOCKED; - } - - return SELECTION_UNLOCKED; -} - - int SELECTION_TOOL::CursorSelection( const TOOL_EVENT& aEvent ) { CLIENT_SELECTION_FILTER aClientFilter = aEvent.Parameter(); @@ -1641,8 +1604,6 @@ void SELECTION_TOOL::ClearSelection( bool aQuietMode ) m_selection.SetIsHover( false ); m_selection.ClearReferencePoint(); - m_locked = true; - // Inform other potentially interested tools if( !aQuietMode ) { @@ -2089,9 +2050,6 @@ void SELECTION_TOOL::select( BOARD_ITEM* aItem ) void SELECTION_TOOL::unselect( BOARD_ITEM* aItem ) { unhighlight( aItem, SELECTED, &m_selection ); - - if( m_selection.Empty() ) - m_locked = true; } diff --git a/pcbnew/tools/selection_tool.h b/pcbnew/tools/selection_tool.h index 0d7716a5cf..1977aebb28 100644 --- a/pcbnew/tools/selection_tool.h +++ b/pcbnew/tools/selection_tool.h @@ -98,9 +98,6 @@ public: PCBNEW_SELECTION& RequestSelection( CLIENT_SELECTION_FILTER aClientFilter, std::vector* aFiltered = nullptr, bool aConfirmLockedItems = false ); - ///> Checks if the user has agreed to modify locked items for the given selection. - SELECTION_LOCK_FLAGS CheckLock(); - ///> Select a single item under cursor event handler. int CursorSelection( const TOOL_EVENT& aEvent ); @@ -384,7 +381,6 @@ private: bool m_exclusive_or; // Items' selection state should be toggled bool m_multiple; // Multiple selection mode is active bool m_skip_heuristics; // Heuristics are not allowed when choosing item under cursor - bool m_locked; // Other tools are not allowed to modify locked items PCB_GROUP* m_enteredGroup; // If non-null, selections are limited to // members of this group diff --git a/qa/qa_utils/mocks.cpp b/qa/qa_utils/mocks.cpp index a8d39a338a..e3de9904a7 100644 --- a/qa/qa_utils/mocks.cpp +++ b/qa/qa_utils/mocks.cpp @@ -475,7 +475,6 @@ SELECTION_TOOL::SELECTION_TOOL() : m_exclusive_or( false ), m_multiple( false ), m_skip_heuristics( false ), - m_locked( true ), m_enteredGroup( NULL ), m_priv( nullptr ) { @@ -565,13 +564,6 @@ bool SELECTION_TOOL::selectMultiple() } -SELECTION_LOCK_FLAGS SELECTION_TOOL::CheckLock() -{ - - return SELECTION_UNLOCKED; -} - - int SELECTION_TOOL::CursorSelection( const TOOL_EVENT& aEvent ) {