[Kicad-developers] [PATCH] pcbnew alignment bugfixes

[Seth Hillbrand]

​Attached are 4 patches for various alignment issues.

The first fixes https://bugs.launchpad.net/kicad/+bug/1751352 where pads
are moved despite being locked.

The second is a bugfix where aligning in the X direction moved things in
the Y direction.

The third is a bugfix for when you select both a pad and the pad's parent
module before aligning.

The fourth is not a bugfix and, as such should be considered highly
optional at this point.  But it fixes an annoyance when aligning a group of
things to their centers.  I can never tell which item will be chosen for
the center as currently it chooses the "centermost" item.  If you have
10-20 mostly-aligned items, it's very hard to tell which will be the
center.  You need to move about half to one side and about half to the
other before aligning.  Or align, check the value and do a second move.
Instead, this chooses the left-most for X-center and top-most for
Y-center.  You can then move one item into place and align the others to
it.  Delaying this has no affect of the other patches, so I'm happy to hold
it for post-5.​
From c861f742e42a1cdb3dccf3a6668a375d3b2c4c25 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand 
Date: Mon, 26 Feb 2018 15:13:21 -0800
Subject: [PATCH 1/4] pcbnew: Check locks in alignment

When aligning in pcbnew, check for pad/module locks before performing a
move and query the user.

When aligning on pads, don't move the pad without moving the footprint,
so we don't break footprints.

Fixes: lp:1751352
* https://bugs.launchpad.net/kicad/+bug/1751352
---
 pcbnew/tools/placement_tool.cpp | 184 
 pcbnew/tools/placement_tool.h   |  10 +++
 2 files changed, 160 insertions(+), 34 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index f0ae369ca..d5589fba0 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -182,19 +182,69 @@ ALIGNMENT_RECTS GetBoundingBoxes( const SELECTION &sel )
 }
 
 
+int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
+{
+SELECTION moving_items( selection );
+
+// Remove the anchor from the list
+moving_items.Remove( moving_items.Front() );
+
+bool containsLocked = false;
+
+// Check if the selection contains locked items
+for( const auto& item : moving_items )
+{
+switch ( item->Type() )
+{
+case PCB_MODULE_T:
+if( static_cast< MODULE* >( item )->IsLocked() )
+containsLocked = true;
+break;
+
+case PCB_PAD_T:
+case PCB_MODULE_EDGE_T:
+case PCB_MODULE_TEXT_T:
+if( static_cast< MODULE* >( item->GetParent() )->IsLocked() )
+containsLocked = true;
+break;
+
+default:// suppress warnings
+break;
+}
+}
+
+if( containsLocked )
+{
+if( IsOK( getEditFrame< PCB_EDIT_FRAME >(),
+_( "Selection contains locked items. Do you want to continue?" ) ) )
+{
+return SELECTION_LOCK_OVERRIDE;
+}
+else
+return SELECTION_LOCKED;
+}
+
+return SELECTION_UNLOCKED;
+}
+
+
 int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 {
-const SELECTION& selection = m_selectionTool->GetSelection();
+auto frame = getEditFrame();
+const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
 if( selection.Size() <= 1 )
 return 0;
 
-BOARD_COMMIT commit( getEditFrame() );
-commit.StageItems( selection, CHT_MODIFY );
-
 auto itemsToAlign = GetBoundingBoxes( selection );
 std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortTopmostY );
 
+if( checkLockedStatus( selection ) == SELECTION_LOCKED )
+return 0;
+
+BOARD_COMMIT commit( frame );
+commit.StageItems( selection, CHT_MODIFY );
+
 // after sorting, the fist item acts as the target for all others
 const int targetTop = itemsToAlign.begin()->second.GetY();
 
@@ -202,7 +252,13 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 for( auto& i : itemsToAlign )
 {
 int difference = targetTop - i.second.GetY();
-i.first->Move( wxPoint( 0, difference ) );
+BOARD_ITEM* item = i.first;
+
+// Don't move a pad by itself unless editing the footprint
+if( item->Type() == PCB_PAD_T && frame->IsType( FRAME_PCB ) )
+item = item->GetParent();
+
+item->Move( wxPoint( 0, difference ) );
 }
 
 commit.Push( _( "Align to top" ) );
@@ -213,17 +269,21 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 
 int ALIGN_DISTRIBUTE_TOOL::AlignBottom( const TOOL_EVENT& aEvent )
 {
-const SELECTION& selection = m_selectionTool->GetSelection();
+auto frame = getEditFrame();
+const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
 if( selection.Size() <= 1 )
 return

Re: [Kicad-developers] [PATCH] pcbnew alignment bugfixes

[Seth Hillbrand]

Oops! Noticed that 1+2 should have been in the same patch (fixing my own
bug there)

I've squashed them down to three patches.

-S

2018-02-28 16:52 GMT-08:00 Seth Hillbrand :

> ​Attached are 4 patches for various alignment issues.
>
> The first fixes https://bugs.launchpad.net/kicad/+bug/1751352 where pads
> are moved despite being locked.
>
> The second is a bugfix where aligning in the X direction moved things in
> the Y direction.
>
> The third is a bugfix for when you select both a pad and the pad's parent
> module before aligning.
>
> The fourth is not a bugfix and, as such should be considered highly
> optional at this point.  But it fixes an annoyance when aligning a group of
> things to their centers.  I can never tell which item will be chosen for
> the center as currently it chooses the "centermost" item.  If you have
> 10-20 mostly-aligned items, it's very hard to tell which will be the
> center.  You need to move about half to one side and about half to the
> other before aligning.  Or align, check the value and do a second move.
> Instead, this chooses the left-most for X-center and top-most for
> Y-center.  You can then move one item into place and align the others to
> it.  Delaying this has no affect of the other patches, so I'm happy to hold
> it for post-5.​
>
>
From 31d7b7c92013a5c94f9adc7d97f5d15dd9e29397 Mon Sep 17 00:00:00 2001
From: Seth Hillbrand 
Date: Tues, 27 Feb 2018 16:01:47 -0800
Subject: [PATCH 2/3] pcbnew: Prevent alignment on pads + parents

Filter a selection that contains pads and the pads' parent modules
before performing alignment operations.
---
 pcbnew/tools/placement_tool.cpp | 55 ++---
 pcbnew/tools/placement_tool.h   |  5 
 2 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/pcbnew/tools/placement_tool.cpp b/pcbnew/tools/placement_tool.cpp
index 0c67e865b..54fd089a3 100644
--- a/pcbnew/tools/placement_tool.cpp
+++ b/pcbnew/tools/placement_tool.cpp
@@ -182,6 +182,27 @@ ALIGNMENT_RECTS GetBoundingBoxes( const SELECTION &sel )
 }
 
 
+void ALIGN_DISTRIBUTE_TOOL::filterPadsWithModules( SELECTION &selection )
+{
+std::set rejected;
+for( auto i : selection )
+{
+auto item = static_cast( i );
+if( item->Type() == PCB_PAD_T )
+{
+MODULE* mod = static_cast( item->GetParent() );
+
+// selection contains both the module and its pads - remove the pads
+if( mod && selection.Contains( mod ) )
+rejected.insert( item );
+}
+}
+
+for( BOARD_ITEM* item : rejected )
+selection.Remove( item );
+}
+
+
 int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
 {
 SELECTION moving_items( selection );
@@ -231,14 +252,15 @@ int ALIGN_DISTRIBUTE_TOOL::checkLockedStatus( const SELECTION &selection ) const
 int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 {
 auto frame = getEditFrame();
-const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
 if( selection.Size() <= 1 )
 return 0;
 
+filterPadsWithModules( selection );
+
 auto itemsToAlign = GetBoundingBoxes( selection );
 std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortTopmostY );
-
 if( checkLockedStatus( selection ) == SELECTION_LOCKED )
 return 0;
 
@@ -270,14 +292,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignTop( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::AlignBottom( const TOOL_EVENT& aEvent )
 {
 auto frame = getEditFrame();
-const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
 if( selection.Size() <= 1 )
 return 0;
 
+filterPadsWithModules( selection );
+
 auto itemsToAlign = GetBoundingBoxes( selection );
 std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortBottommostY );
-
 if( checkLockedStatus( selection ) == SELECTION_LOCKED )
 return 0;
 
@@ -324,14 +347,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignLeft( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::doAlignLeft()
 {
 auto frame = getEditFrame();
-const SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
+SELECTION& selection = m_selectionTool->RequestSelection( SELECTION_EDITABLE );
 
 if( selection.Size() <= 1 )
 return 0;
 
+filterPadsWithModules( selection );
+
 auto itemsToAlign = GetBoundingBoxes( selection );
 std::sort( itemsToAlign.begin(), itemsToAlign.end(), SortLeftmostX );
-
 if( checkLockedStatus( selection ) == SELECTION_LOCKED )
 return 0;
 
@@ -378,14 +402,15 @@ int ALIGN_DISTRIBUTE_TOOL::AlignRight( const TOOL_EVENT& aEvent )
 int ALIGN_DISTRIBUTE_TOOL::doAlignRight()
 {
 auto frame = getEditFrame();
-const SELECTION& selection = 

Re: [Kicad-developers] [PATCH] pcbnew alignment bugfixes

[Nick Østergaard]

It looks like this has been merged.

2018-03-01 5:00 GMT+01:00 Seth Hillbrand :

> Oops! Noticed that 1+2 should have been in the same patch (fixing my own
> bug there)
>
> I've squashed them down to three patches.
>
> -S
>
> 2018-02-28 16:52 GMT-08:00 Seth Hillbrand :
>
>> ​Attached are 4 patches for various alignment issues.
>>
>> The first fixes https://bugs.launchpad.net/kicad/+bug/1751352 where pads
>> are moved despite being locked.
>>
>> The second is a bugfix where aligning in the X direction moved things in
>> the Y direction.
>>
>> The third is a bugfix for when you select both a pad and the pad's parent
>> module before aligning.
>>
>> The fourth is not a bugfix and, as such should be considered highly
>> optional at this point.  But it fixes an annoyance when aligning a group of
>> things to their centers.  I can never tell which item will be chosen for
>> the center as currently it chooses the "centermost" item.  If you have
>> 10-20 mostly-aligned items, it's very hard to tell which will be the
>> center.  You need to move about half to one side and about half to the
>> other before aligning.  Or align, check the value and do a second move.
>> Instead, this chooses the left-most for X-center and top-most for
>> Y-center.  You can then move one item into place and align the others to
>> it.  Delaying this has no affect of the other patches, so I'm happy to hold
>> it for post-5.​
>>
>>
>
> ___
> Mailing list: https://launchpad.net/~kicad-developers
> Post to : kicad-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>
>
___
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help   : https://help.launchpad.net/ListHelp