Fix fillets and chamfers when the original lines become zero-length

This needs the ITEM_MODIFICATION_ROUTINE to learn to delete items.
Condense the item change handlers into a single injected object
(ITEM_MODIFICATION_ROUTINE::CHANGE_HANDLER) and provide the basic
implementation that just takes some callables.

This simplifies the construction of the routines and also would make
a CHANGE_HANDLER object possible that can be reused between different
tools.

pcbnew/tools/edit_tool.cpp

@@ -1201,6 +1201,9 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
     // (doing it as we go will invalidate the iterator)
     std::vector<PCB_SHAPE*> items_to_select_on_success;
 
+    // And same for items to deselect
+    std::vector<PCB_SHAPE*> items_to_deselect_on_success;
+
     // Handle modifications to existing items by the routine
     // How to deal with this depends on whether we're in the footprint editor or not
     // and whether the item was conjured up by decomposing a polygon or rectangle
@@ -1214,7 +1217,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
         }
     };
 
-    bool       any_items_created = false;
+    bool       any_items_created = !lines_to_add.empty();
     const auto item_creation_handler = [&]( std::unique_ptr<PCB_SHAPE> aItem )
     {
         any_items_created = true;
@@ -1222,6 +1225,18 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
         commit.Add( aItem.release() );
     };
 
+    bool       any_items_removed = !items_to_remove.empty();
+    const auto item_removal_handler = [&]( PCB_SHAPE& aItem )
+    {
+        any_items_removed = true;
+        items_to_deselect_on_success.push_back( &aItem );
+        commit.Remove( &aItem );
+    };
+
+    // Combine these callbacks into a CHANGE_HANDLER to inject in the ROUTINE
+    ITEM_MODIFICATION_ROUTINE::CALLABLE_BASED_HANDLER change_handler(
+            item_creation_handler, item_modification_handler, item_removal_handler );
+
     // Construct an appropriate tool
     std::unique_ptr<PAIRWISE_LINE_ROUTINE> pairwise_line_routine;
     wxString                               error_message;
@@ -1233,8 +1248,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
         if( filletRadiusIU.has_value() )
         {
             pairwise_line_routine = std::make_unique<LINE_FILLET_ROUTINE>(
-                    frame()->GetModel(), item_creation_handler, item_modification_handler,
+                    frame()->GetModel(), change_handler, *filletRadiusIU );
-                    *filletRadiusIU );
         }
     }
     else if( aEvent.IsAction( &PCB_ACTIONS::chamferLines ) )
@@ -1245,8 +1259,7 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
         if( chamfer_params.has_value() )
         {
             pairwise_line_routine = std::make_unique<LINE_CHAMFER_ROUTINE>(
-                    frame()->GetModel(), item_creation_handler, item_modification_handler,
+                    frame()->GetModel(), change_handler, *chamfer_params );
-                    *chamfer_params );
         }
     }
     else if( aEvent.IsAction( &PCB_ACTIONS::extendLines ) )
@@ -1257,8 +1270,8 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
         }
         else
         {
-            pairwise_line_routine = std::make_unique<LINE_EXTENSION_ROUTINE>(
+            pairwise_line_routine =
-                    frame()->GetModel(), item_creation_handler, item_modification_handler );
+                    std::make_unique<LINE_EXTENSION_ROUTINE>( frame()->GetModel(), change_handler );
         }
     }
 
@@ -1301,10 +1314,14 @@ int EDIT_TOOL::ModifyLines( const TOOL_EVENT& aEvent )
     for( PCB_SHAPE* item : items_to_select_on_success )
         m_selectionTool->AddItemToSel( item, true );
 
-    if( !items_to_remove.empty() )
+    // Deselect removed items
+    for( PCB_SHAPE* item : items_to_deselect_on_success )
+        m_selectionTool->RemoveItemFromSel( item, true );
+
+    if( any_items_removed )
         m_toolMgr->ProcessEvent( EVENTS::UnselectedEvent );
 
-    if( !any_items_created )
+    if( any_items_created )
         m_toolMgr->ProcessEvent( EVENTS::SelectedEvent );
 
     // Notify other tools of the changes

pcbnew/tools/item_modification_routine.cpp

@@ -36,6 +36,28 @@ bool SegmentsShareEndpoint( const SEG& aSegA, const SEG& aSegB )
 
 } // namespace
 
+
+bool ITEM_MODIFICATION_ROUTINE::ModifyLineOrDeleteIfZeroLength( PCB_SHAPE& aLine, const SEG& aSeg )
+{
+    wxASSERT_MSG( aLine.GetShape() == SHAPE_T::SEGMENT, "Can only modify segments" );
+
+    const bool removed = aSeg.Length() == 0;
+    if( !removed )
+    {
+        // Mark modified, then change it
+        GetHandler().MarkItemModified( aLine );
+        aLine.SetStart( aSeg.A );
+        aLine.SetEnd( aSeg.B );
+    }
+    else
+    {
+        // The line has become zero length - delete it
+        GetHandler().DeleteItem( aLine );
+    }
+    return removed;
+}
+
+
 wxString LINE_FILLET_ROUTINE::GetCommitDescription() const
 {
     return _( "Fillet Lines" );
@@ -128,17 +150,15 @@ void LINE_FILLET_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLineB
     tArc->SetLayer( aLineA.GetLayer() );
     tArc->SetLocked( aLineA.IsLocked() );
 
-    MarkItemModified( aLineA );
+    CHANGE_HANDLER& handler = GetHandler();
-    MarkItemModified( aLineB );
 
-    AddNewItem( std::move( tArc ) );
+    handler.AddNewItem( std::move( tArc ) );
 
     *a_pt = t1newPoint;
     *b_pt = t2newPoint;
-    aLineA.SetStart( seg_a.A );
+
-    aLineA.SetEnd( seg_a.B );
+    ModifyLineOrDeleteIfZeroLength( aLineA, seg_a );
-    aLineB.SetStart( seg_b.A );
+    ModifyLineOrDeleteIfZeroLength( aLineB, seg_b );
-    aLineB.SetEnd( seg_b.B );
 
     AddSuccess();
 }
@@ -194,16 +214,12 @@ void LINE_CHAMFER_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLineB
     tSegment->SetLayer( aLineA.GetLayer() );
     tSegment->SetLocked( aLineA.IsLocked() );
 
-    AddNewItem( std::move( tSegment ) );
+    CHANGE_HANDLER& handler = GetHandler();
 
-    MarkItemModified( aLineA );
+    handler.AddNewItem( std::move( tSegment ) );
-    MarkItemModified( aLineB );
 
-    // Shorten the original lines
+    ModifyLineOrDeleteIfZeroLength( aLineA, *chamfer_result->m_updated_seg_a );
-    aLineA.SetStart( chamfer_result->m_updated_seg_a->A );
+    ModifyLineOrDeleteIfZeroLength( aLineB, *chamfer_result->m_updated_seg_b );
-    aLineA.SetEnd( chamfer_result->m_updated_seg_a->B );
-    aLineB.SetStart( chamfer_result->m_updated_seg_b->A );
-    aLineB.SetEnd( chamfer_result->m_updated_seg_b->B );
 
     AddSuccess();
 }
@@ -247,6 +263,8 @@ void LINE_EXTENSION_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLin
         return;
     }
 
+    CHANGE_HANDLER& handler = GetHandler();
+
     const auto line_extender = [&]( const SEG& aSeg, PCB_SHAPE& aLine )
     {
         // If the intersection point is not already n the line, we'll extend to it
@@ -257,7 +275,7 @@ void LINE_EXTENSION_ROUTINE::ProcessLinePair( PCB_SHAPE& aLineA, PCB_SHAPE& aLin
 
             const VECTOR2I& furthest_pt = ( dist_start < dist_end ) ? aSeg.B : aSeg.A;
 
-            MarkItemModified( aLine );
+            handler.MarkItemModified( aLine );
             aLine.SetStart( furthest_pt );
             aLine.SetEnd( *intersection );
         }

pcbnew/tools/item_modification_routine.h

@@ -47,37 +47,112 @@ class ITEM_MODIFICATION_ROUTINE
 {
 public:
     /*
-     * Handlers for receiving changes from the tool
+    * Handlers for receiving changes from the tool
-     *
+    *
-     * These are used to allow the tool's caller to make changes to
+    * These are used to allow the tool's caller to make changes to
-     * affected board items using extra information that the tool
+    * affected board items using extra information that the tool
-     * does not have access to (e.g. is this an FP editor, was
+    * does not have access to (e.g. is this an FP editor, was
-     * the line created from a rectangle and needs to be added, not
+    * the line created from a rectangle and needs to be added, not
-     * modified, etc).
+    * modified, etc).
-     *
+    *
-     * We can't store them up until the end, because modifications
+    * We can't store them up until the end, because modifications
-     * need the old state to be known.
+    * need the old state to be known, so this allows the caller to
-     */
+    * inject the dependencies for how to handle the changes.
+    */
+    class CHANGE_HANDLER
+    {
+    public:
+        virtual ~CHANGE_HANDLER() = default;
+
+        /**
+         * @brief Report that the tools wants to add a new item to the board
+         *
+         * @param aItem the new item
+         */
+        virtual void AddNewItem( std::unique_ptr<PCB_SHAPE> aItem ) = 0;
+
+        /**
+         * @brief Report that the tool has modified an item on the board
+         *
+         * @param aItem the modified item
+         */
+        virtual void MarkItemModified( PCB_SHAPE& aItem ) = 0;
+
+        /**
+         * @brief Report that the tool has deleted an item on the board
+         *
+         * @param aItem the deleted item
+         */
+        virtual void DeleteItem( PCB_SHAPE& aItem ) = 0;
+    };
 
     /**
-     * Handler for creating a new item on the board
+     * @brief A handler that is based on a set of callbacks provided
-     *
+     * by the user of the ITEM_MODIFICATION_ROUTINE
-     * @param PCB_SHAPE& the shape to add
      */
-    using CREATION_HANDLER = std::function<void( std::unique_ptr<PCB_SHAPE> )>;
+    class CALLABLE_BASED_HANDLER : public CHANGE_HANDLER
+    {
+    public:
+        /**
+         * Handler for creating a new item on the board
+         *
+         * @param PCB_SHAPE& the shape to add
+         */
+        using CREATION_HANDLER = std::function<void( std::unique_ptr<PCB_SHAPE> )>;
 
-    /**
+        /**
-     * Handler for modifying an existing item on the board
+         * Handler for modifying or deleting an existing item on the board
-     *
+         *
-     * @param PCB_SHAPE& the shape to modify
+         * @param PCB_SHAPE& the shape to modify
-     */
+         */
-    using MODIFICATION_HANDLER = std::function<void( PCB_SHAPE& )>;
+        using MODIFICATION_HANDLER = std::function<void( PCB_SHAPE& )>;
 
-    ITEM_MODIFICATION_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler,
+        /**
-                               MODIFICATION_HANDLER aModificationHandler ) :
+         * Handler for modifying or deleting an existing item on the board
-            m_board( aBoard ),
+         *
-            m_creationHandler( aCreationHandler ), m_modificationHandler( aModificationHandler ),
+         * @param PCB_SHAPE& the shape to delete
-            m_numSuccesses( 0 ), m_numFailures( 0 )
+         */
+        using DELETION_HANDLER = std::function<void( PCB_SHAPE& )>;
+
+        CALLABLE_BASED_HANDLER( CREATION_HANDLER     aCreationHandler,
+                                MODIFICATION_HANDLER aModificationHandler,
+                                DELETION_HANDLER     aDeletionHandler ) :
+                m_creationHandler( aCreationHandler ),
+                m_modificationHandler( aModificationHandler ), m_deletionHandler( aDeletionHandler )
+        {
+        }
+
+        /**
+         * @brief Report that the tools wants to add a new item to the board
+         *
+         * @param aItem the new item
+         */
+        void AddNewItem( std::unique_ptr<PCB_SHAPE> aItem ) override
+        {
+            m_creationHandler( std::move( aItem ) );
+        }
+
+        /**
+         * @brief Report that the tool has modified an item on the board
+         *
+         * @param aItem the modified item
+         */
+        void MarkItemModified( PCB_SHAPE& aItem ) override { m_modificationHandler( aItem ); }
+
+        /**
+         * @brief Report that the tool has deleted an item on the board
+         *
+         * @param aItem the deleted item
+         */
+        void DeleteItem( PCB_SHAPE& aItem ) override { m_deletionHandler( aItem ); }
+
+        CREATION_HANDLER     m_creationHandler;
+        MODIFICATION_HANDLER m_modificationHandler;
+        DELETION_HANDLER     m_deletionHandler;
+    };
+
+    ITEM_MODIFICATION_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) :
+            m_board( aBoard ), m_handler( aHandler ), m_numSuccesses( 0 ), m_numFailures( 0 )
     {
     }
 
@@ -109,23 +184,22 @@ protected:
     void AddFailure() { ++m_numFailures; }
 
     /**
-     * @brief Report that the tools wants to add a new item to the board
+     *  @brief Helper function useful for multiple tools: modify a line or delete
+     * it if it has zero length
      *
-     * @param aItem the new item
+     * @param aItem the line to modify
+     * @param aSeg the new line geometry
      */
-    void AddNewItem( std::unique_ptr<PCB_SHAPE> aItem ) { m_creationHandler( std::move( aItem ) ); }
+    bool ModifyLineOrDeleteIfZeroLength( PCB_SHAPE& aItem, const SEG& aSeg );
 
     /**
-     * @brief Report that the tool has modified an item on the board
+     * @brief Access the handler for making changes to the board
-     *
-     * @param aItem the modified item
      */
-    void MarkItemModified( PCB_SHAPE& aItem ) { m_modificationHandler( aItem ); }
+    CHANGE_HANDLER& GetHandler() { return m_handler; }
 
 private:
     BOARD_ITEM*          m_board;
-    CREATION_HANDLER     m_creationHandler;
+    CHANGE_HANDLER&      m_handler;
-    MODIFICATION_HANDLER m_modificationHandler;
 
     unsigned m_numSuccesses;
     unsigned m_numFailures;
@@ -137,9 +211,8 @@ private:
 class PAIRWISE_LINE_ROUTINE : public ITEM_MODIFICATION_ROUTINE
 {
 public:
-    PAIRWISE_LINE_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler,
+    PAIRWISE_LINE_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) :
-                           MODIFICATION_HANDLER aModificationHandler ) :
+            ITEM_MODIFICATION_ROUTINE( aBoard, aHandler )
-            ITEM_MODIFICATION_ROUTINE( aBoard, aCreationHandler, aModificationHandler )
     {
     }
 
@@ -167,10 +240,8 @@ public:
 class LINE_FILLET_ROUTINE : public PAIRWISE_LINE_ROUTINE
 {
 public:
-    LINE_FILLET_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler,
+    LINE_FILLET_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler, int filletRadiusIU ) :
-                         MODIFICATION_HANDLER aModificationHandler, int filletRadiusIU ) :
+            PAIRWISE_LINE_ROUTINE( aBoard, aHandler ), m_filletRadiusIU( filletRadiusIU )
-            PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler ),
-            m_filletRadiusIU( filletRadiusIU )
     {
     }
 
@@ -190,10 +261,9 @@ private:
 class LINE_CHAMFER_ROUTINE : public PAIRWISE_LINE_ROUTINE
 {
 public:
-    LINE_CHAMFER_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler,
+    LINE_CHAMFER_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler,
-                          MODIFICATION_HANDLER aModificationHandler,
+                          CHAMFER_PARAMS aChamferParams ) :
-                          CHAMFER_PARAMS       aChamferParams ) :
+            PAIRWISE_LINE_ROUTINE( aBoard, aHandler ),
-            PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler ),
             m_chamferParams( std::move( aChamferParams ) )
     {
     }
@@ -214,9 +284,8 @@ private:
 class LINE_EXTENSION_ROUTINE : public PAIRWISE_LINE_ROUTINE
 {
 public:
-    LINE_EXTENSION_ROUTINE( BOARD_ITEM* aBoard, CREATION_HANDLER aCreationHandler,
+    LINE_EXTENSION_ROUTINE( BOARD_ITEM* aBoard, CHANGE_HANDLER& aHandler ) :
-                            MODIFICATION_HANDLER aModificationHandler ) :
+            PAIRWISE_LINE_ROUTINE( aBoard, aHandler )
-            PAIRWISE_LINE_ROUTINE( aBoard, aCreationHandler, aModificationHandler )
     {
     }