From f1c599fa4d4dbeb4007e5e9832d70d05737dd697 Mon Sep 17 00:00:00 2001
From: Jon Evans <jon@craftyjon.com>
Date: Sat, 20 Mar 2021 12:46:19 -0400
Subject: [PATCH] Tweak PCB selection behavior to reduce unintuitive behavior

We were discarding shapes too aggressively for having a
larger area than a shape underneath.

Let's also try showing fewer disambiguation menus, in particular
always preferring items on the active layer when the candidates
include overlapping items of similar area on other layers.

Fixes https://gitlab.com/kicad/code/kicad/-/issues/7949
---
 libs/kimath/src/geometry/shape_arc.cpp |  3 +++
 pcbnew/footprint.cpp                   | 36 +++++++++++++++++++++++++-
 pcbnew/tools/pcb_selection_tool.cpp    | 24 ++++++++++++++++-
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/libs/kimath/src/geometry/shape_arc.cpp b/libs/kimath/src/geometry/shape_arc.cpp
index c4151341c5..36252e0121 100644
--- a/libs/kimath/src/geometry/shape_arc.cpp
+++ b/libs/kimath/src/geometry/shape_arc.cpp
@@ -190,6 +190,9 @@ SHAPE_ARC& SHAPE_ARC::ConstructFromStartEndAngle( const VECTOR2I& aStart, const
 
 bool SHAPE_ARC::Collide( const SEG& aSeg, int aClearance, int* aActual, VECTOR2I* aLocation ) const
 {
+    if( aSeg.A == aSeg.B )
+        return Collide( aSeg.A, aClearance, aActual, aLocation );
+
     int minDist = aClearance + m_width / 2;
     VECTOR2I center = GetCenter();
     ecoord dist_sq;
diff --git a/pcbnew/footprint.cpp b/pcbnew/footprint.cpp
index 71b23e0e66..411d2fa896 100644
--- a/pcbnew/footprint.cpp
+++ b/pcbnew/footprint.cpp
@@ -1766,7 +1766,6 @@ double FOOTPRINT::GetCoverageArea( const BOARD_ITEM* aItem, const GENERAL_COLLEC
 
         return combinedArea;
     }
-
     if( aItem->Type() == PCB_FOOTPRINT_T )
     {
         const FOOTPRINT* footprint = static_cast<const FOOTPRINT*>( aItem );
@@ -1780,6 +1779,41 @@ double FOOTPRINT::GetCoverageArea( const BOARD_ITEM* aItem, const GENERAL_COLLEC
         text->TransformTextShapeWithClearanceToPolygon( poly, UNDEFINED_LAYER, textMargin,
                                                         ARC_LOW_DEF, ERROR_OUTSIDE );
     }
+    else if( aItem->Type() == PCB_SHAPE_T )
+    {
+        // Approximate "linear" shapes with just their width squared, as we don't want to consider
+        // a linear shape as being much bigger than another for purposes of selection filtering
+        // just because it happens to be really long.
+
+        const PCB_SHAPE* shape = static_cast<const PCB_SHAPE*>( aItem );
+
+        switch( shape->GetShape() )
+        {
+        case S_SEGMENT:
+        case S_ARC:
+        case S_CURVE:
+            return shape->GetWidth() * shape->GetWidth();
+
+        case S_RECT:
+        case S_CIRCLE:
+        case S_POLYGON:
+        {
+            if( !shape->IsFilled() )
+                return shape->GetWidth() * shape->GetWidth();
+
+            KI_FALLTHROUGH;
+        }
+
+        default:
+            aItem->TransformShapeWithClearanceToPolygon( poly, UNDEFINED_LAYER, 0,
+                                                         ARC_LOW_DEF, ERROR_OUTSIDE );
+        }
+    }
+    else if( aItem->Type() == PCB_TRACE_T || aItem->Type() == PCB_ARC_T )
+    {
+        double width = static_cast<const TRACK*>( aItem )->GetWidth();
+        return width * width;
+    }
     else
     {
         aItem->TransformShapeWithClearanceToPolygon( poly, UNDEFINED_LAYER, 0,
diff --git a/pcbnew/tools/pcb_selection_tool.cpp b/pcbnew/tools/pcb_selection_tool.cpp
index b5abfd186f..a722436fd0 100644
--- a/pcbnew/tools/pcb_selection_tool.cpp
+++ b/pcbnew/tools/pcb_selection_tool.cpp
@@ -2411,7 +2411,7 @@ void PCB_SELECTION_TOOL::GuessSelectionCandidates( GENERAL_COLLECTOR& aCollector
     // If the user clicked on a small item within a much larger one then it's pretty clear
     // they're trying to select the smaller one.
 
-    constexpr double sizeRatio = 1.3;
+    constexpr double sizeRatio = 1.5;
 
     std::vector<std::pair<BOARD_ITEM*, double>> itemsByArea;
 
@@ -2486,6 +2486,28 @@ void PCB_SELECTION_TOOL::GuessSelectionCandidates( GENERAL_COLLECTOR& aCollector
         for( BOARD_ITEM* item : rejected )
             aCollector.Transfer( item );
     }
+
+    // Finally, what we are left with is a set of items of similar coverage area.  We now reject
+    // any that are not on the active layer, to reduce the number of disambiguation menus shown.
+    // If the user wants to force-disambiguate, they can either switch layers or use the modifier
+    // key to force the menu.
+    if( aCollector.GetCount() > 1 )
+    {
+        bool haveItemOnActive = false;
+        rejected.clear();
+
+        for( int i = 0; i < aCollector.GetCount(); ++i )
+        {
+            if( !aCollector[i]->IsOnLayer( activeLayer ) )
+                rejected.insert( aCollector[i] );
+            else
+                haveItemOnActive = true;
+        }
+
+        if( haveItemOnActive )
+            for( BOARD_ITEM* item : rejected )
+                aCollector.Transfer( item );
+    }
 }