Pcbnew: rework point editor behavior handling

This introduces the POINT_EDITOR_BEHAVIOR class, which
allows a "behavior" to be defined, which covers the creation
of edit points, updating the points on edit, and pushing the
edited points back into the object.

This keeps the logic for a single item "type" (e.g. a SEGMENT
or TEXTBOX, etc) in one place, rather than fragmneted throughout
the POINT_EDITOR class, where the invariants like point count
are difficult to keep track of as the TOOL progresses.

For now, it's implemented as an optional class, just for SEGMENT
and other tpyes work as before. Adding new types is then a
"pin-compatible" drop-in process.

include/tool/edit_points.h

@@ -579,4 +579,76 @@ private:
     bool                   m_allowPoints;  ///< If false, only allow editing of EDIT_LINES.
 };
 
+
+/**
+ * A helper class interface to manage the edit points for a single item.
+ * Create one ofthese, and it will provide a way to keep a list of points
+ * updated.
+ *
+ * For the moment this is implemented such that it mutates an external
+ * EDIT_POINTS object, but it might be able to also own the points.
+ */
+class POINT_EDIT_BEHAVIOR
+{
+public:
+    virtual ~POINT_EDIT_BEHAVIOR() = default;
+
+    /**
+     * Construct the initial set of edit points for the item
+     * and append to the given list.
+     *
+     * @param aPoints The list of edit points to fill.
+     */
+    virtual void MakePoints( EDIT_POINTS& aPoints ) = 0;
+
+    /**
+     * Update the list of the edit points for the item.
+     *
+     * If item has changed such that that number of points needs to
+     * change, this method has to handle that (probably by clearing
+     * the list and refilling it).
+     *
+     * @param aPoints The list of edit points to update.
+     */
+    virtual void UpdatePoints( EDIT_POINTS& aPoints ) = 0;
+
+    /**
+     * Update the item with the new positions of the edit points.
+     *
+     * @param aEditedPoint The point that was dragged.
+     *                     You can use this to check by address which point to update.
+     * @param aPoints The new positions of the edit points.
+     */
+    virtual void UpdateItem( const EDIT_POINT& aEditedPoint, EDIT_POINTS& aPoints ) = 0;
+
+    /**
+     * Get the 45-degree constrainer for the item, when the given point is moved.
+     * Return std::nullopt if not, and the caller can decide.
+     *
+     * If you want to actively disable constraining, return the aEditedPoint
+     * position.
+     */
+    virtual OPT_VECTOR2I Get45DegreeConstrainer( const EDIT_POINT& aEditedPoint,
+                                                 EDIT_POINTS&      aPoints ) const
+    {
+        // By default, no constrainer is defined and the caller must decide
+        return std::nullopt;
+    }
+
+protected:
+    /**
+     * Checks if two points are the same instance - which means the point is being edited.
+     */
+    static bool isModified( const EDIT_POINT& aEditedPoint, const EDIT_POINT& aPoint )
+    {
+        return &aEditedPoint == &aPoint;
+    }
+};
+
+// Helper macros to check the number of points in the edit points object
+// Still a bug, but at least it won't segfault if the number of points is wrong
+#define CHECK_POINT_COUNT(aPoints, aExpected) wxCHECK( aPoints.PointsSize() == aExpected, /* void */ )
+#define CHECK_POINT_COUNT_GE(aPoints, aExpected) wxCHECK( aPoints.PointsSize() >= aExpected, /* void */ )
+
+
 #endif /* EDIT_POINTS_H_ */

pcbnew/tools/pcb_point_editor.cpp

@@ -136,6 +136,50 @@ enum TEXTBOX_POINT_COUNT
     WHEN_POLYGON = 0,
 };
 
+class SEGMENT_POINT_EDIT_BEHAVIOR : public POINT_EDIT_BEHAVIOR
+{
+public:
+    SEGMENT_POINT_EDIT_BEHAVIOR( PCB_SHAPE& aSegment ) : m_segment( aSegment )
+    {
+        wxASSERT( m_segment.GetShape() == SHAPE_T::SEGMENT );
+    }
+
+    void MakePoints( EDIT_POINTS& aPoints ) override
+    {
+        aPoints.AddPoint( m_segment.GetStart() );
+        aPoints.AddPoint( m_segment.GetEnd() );
+    }
+
+    void UpdatePoints( EDIT_POINTS& aPoints ) override
+    {
+        CHECK_POINT_COUNT( aPoints, 2 );
+
+        aPoints.Point( SEG_START ) = m_segment.GetStart();
+        aPoints.Point( SEG_END ) = m_segment.GetEnd();
+    }
+
+    void UpdateItem( const EDIT_POINT& aEditedPoint, EDIT_POINTS& aPoints ) override
+    {
+        CHECK_POINT_COUNT( aPoints, 2 );
+
+        if( isModified( aEditedPoint, aPoints.Point( SEG_START ) ) )
+            m_segment.SetStart( aPoints.Point( SEG_START ).GetPosition() );
+
+        else if( isModified( aEditedPoint, aPoints.Point( SEG_END ) ) )
+            m_segment.SetEnd( aPoints.Point( SEG_END ).GetPosition() );
+    }
+
+    OPT_VECTOR2I Get45DegreeConstrainer( const EDIT_POINT& aEditedPoint,
+                                         EDIT_POINTS&      aPoints ) const override
+    {
+        // Select the other end of line
+        return aPoints.Next( aEditedPoint )->GetPosition();
+    }
+
+private:
+    PCB_SHAPE& m_segment;
+};
+
 
 PCB_POINT_EDITOR::PCB_POINT_EDITOR() :
     PCB_TOOL_BASE( "pcbnew.PointEditor" ),
@@ -215,6 +259,8 @@ std::shared_ptr<EDIT_POINTS> PCB_POINT_EDITOR::makePoints( EDA_ITEM* aItem )
     if( !aItem )
         return points;
 
+    m_editorBehavior = nullptr;
+
     if( aItem->Type() == PCB_TEXTBOX_T )
     {
         const PCB_SHAPE* shape = static_cast<const PCB_SHAPE*>( aItem );
@@ -249,15 +295,13 @@ std::shared_ptr<EDIT_POINTS> PCB_POINT_EDITOR::makePoints( EDA_ITEM* aItem )
     case PCB_TEXTBOX_T:
     case PCB_SHAPE_T:
     {
-        const PCB_SHAPE* shape = static_cast<const PCB_SHAPE*>( aItem );
+        PCB_SHAPE* shape = static_cast<PCB_SHAPE*>( aItem );
 
         switch( shape->GetShape() )
         {
         case SHAPE_T::SEGMENT:
-            points->AddPoint( shape->GetStart() );
-            points->AddPoint( shape->GetEnd() );
+            m_editorBehavior = std::make_unique<SEGMENT_POINT_EDIT_BEHAVIOR>( *shape );
             break;
-
         case SHAPE_T::RECTANGLE:
         {
             VECTOR2I topLeft = shape->GetTopLeft();
@@ -483,6 +527,9 @@ std::shared_ptr<EDIT_POINTS> PCB_POINT_EDITOR::makePoints( EDA_ITEM* aItem )
         break;
     }
 
+    if( m_editorBehavior )
+        m_editorBehavior->MakePoints( *points );
+
     return points;
 }
 
@@ -558,6 +605,8 @@ int PCB_POINT_EDITOR::OnSelectionChange( const TOOL_EVENT& aEvent )
     // Use the original object as a construction item
     std::unique_ptr<BOARD_ITEM> clone;
 
+    m_editorBehavior.reset();
+    // Will also make the edit behavior if supported
     m_editPoints = makePoints( item );
 
     if( !m_editPoints )
@@ -1378,6 +1427,11 @@ void PCB_POINT_EDITOR::updateItem( BOARD_COMMIT* aCommit )
     if( !item )
         return;
 
+    if( m_editorBehavior )
+    {
+        m_editorBehavior->UpdateItem( *m_editedPoint, *m_editPoints );
+    }
+
     switch( item->Type() )
     {
     case PCB_REFERENCE_IMAGE_T:
@@ -1467,11 +1521,6 @@ void PCB_POINT_EDITOR::updateItem( BOARD_COMMIT* aCommit )
         switch( shape->GetShape() )
         {
         case SHAPE_T::SEGMENT:
-            if( isModified( m_editPoints->Point( SEG_START ) ) )
-                shape->SetStart( m_editPoints->Point( SEG_START ).GetPosition() );
-            else if( isModified( m_editPoints->Point( SEG_END ) ) )
-                shape->SetEnd( m_editPoints->Point( SEG_END ).GetPosition() );
-
             break;
 
         case SHAPE_T::RECTANGLE:
@@ -2090,6 +2139,14 @@ void PCB_POINT_EDITOR::updatePoints()
     if( !item )
         return;
 
+    if( m_editorBehavior )
+    {
+        // If we have an editor behavior, let it handle the update
+        m_editorBehavior->UpdatePoints( *m_editPoints );
+        getView()->Update( m_editPoints.get() );
+        return;
+    }
+
     switch( item->Type() )
     {
     case PCB_REFERENCE_IMAGE_T:
@@ -2159,8 +2216,6 @@ void PCB_POINT_EDITOR::updatePoints()
         switch( shape->GetShape() )
         {
         case SHAPE_T::SEGMENT:
-            m_editPoints->Point( SEG_START ).SetPosition( shape->GetStart() );
-            m_editPoints->Point( SEG_END ).SetPosition( shape->GetEnd() );
             break;
 
         case SHAPE_T::RECTANGLE:
@@ -2470,6 +2525,15 @@ void PCB_POINT_EDITOR::setAltConstraint( bool aEnabled )
 
 EDIT_POINT PCB_POINT_EDITOR::get45DegConstrainer() const
 {
+    // If there's a behaviour and it provides a constrainer, use that
+    if( m_editorBehavior )
+    {
+        const OPT_VECTOR2I constrainer =
+                m_editorBehavior->Get45DegreeConstrainer( *m_editedPoint, *m_editPoints );
+        if( constrainer )
+            return EDIT_POINT( *constrainer );
+    }
+
     EDA_ITEM* item = m_editPoints->GetParent();
 
     switch( item->Type() )
@@ -2477,9 +2541,6 @@ EDIT_POINT PCB_POINT_EDITOR::get45DegConstrainer() const
     case PCB_SHAPE_T:
         switch( static_cast<const PCB_SHAPE*>( item )->GetShape() )
         {
-        case SHAPE_T::SEGMENT:
-            return *( m_editPoints->Next( *m_editedPoint ) );     // select the other end of line
-
         case SHAPE_T::ARC:
         case SHAPE_T::CIRCLE:
             return m_editPoints->Point( CIRC_CENTER );

pcbnew/tools/pcb_point_editor.h

@@ -36,6 +36,7 @@
 
 
 class PCB_SELECTION_TOOL;
+class POINT_EDIT_BEHAVIOR;
 class SHAPE_POLY_SET;
 
 /**
@@ -185,6 +186,10 @@ private:
 
     bool                          m_inPointEditorTool; // Re-entrancy guard
 
+    // This handles the edit process for a specific tpye of item (not
+    // just C++ type, because PCB_SHAPE is one type that has many subtypes)
+    std::unique_ptr<POINT_EDIT_BEHAVIOR> m_editorBehavior;
+
     static const unsigned int COORDS_PADDING; // Padding from coordinates limits for this tool
 };