Pcbnew: Fix incorrect behavior of undo/redo command (this issue happens only when a new track is created, and an old redundant track is deleted)

Minor code cleaning

include/class_undoredo_container.h

@@ -98,6 +98,7 @@ public:
                                         * copy of an active item) and m_Link points the active
                                         * item in schematic */
 
+public:
     ITEM_PICKER( EDA_ITEM* aItem = NULL, UNDO_REDO_T aUndoRedoStatus = UR_UNSPECIFIED );
 
     EDA_ITEM* GetItem() const { return m_PickedItem; }
@@ -106,6 +107,8 @@ public:
 
     KICAD_T GetItemType() const { return m_PickedItemType; }
 
+    void SetStatus( UNDO_REDO_T aStatus ) { m_UndoRedoStatus = aStatus; }
+
     void SetItemType( KICAD_T aType ) { m_PickedItemType = aType; }
 
     void SetLink( EDA_ITEM* aItem ) { m_Link = aItem; }

pcbnew/board_undo_redo.cpp

@@ -399,10 +399,6 @@ void PCB_EDIT_FRAME::SaveCopyInUndoList( PICKED_ITEMS_LIST& aItemsList,
              */
             if( commandToUndo->GetPickedItemLink( ii ) == NULL )
                 commandToUndo->SetPickedItemLink( item->Clone(), ii );
-
-            if( commandToUndo->GetPickedItemLink( ii ) == NULL )
-                 wxMessageBox( wxT( "SaveCopyInUndoList() in UR_CHANGED mode: NULL link" ) );
-
             break;
 
         case UR_MOVED:
@@ -459,7 +455,7 @@ void PCB_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList, bool aRed
     // Undo in the reverse order of list creation: (this can allow stacked changes
     // like the same item can be changes and deleted in the same complex command
 
-    TestForExistingItem( GetBoard(), NULL ); // Build list of existing items, for integrity test
+    bool build_item_list = true;    // if true the list of esiting items must be rebuilt
     for( int ii = aList->GetCount()-1; ii >= 0 ; ii--  )
     {
         item = (BOARD_ITEM*) aList->GetPickedItem( ii );
@@ -470,9 +466,16 @@ void PCB_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList, bool aRed
          *   - if a call to SaveCopyInUndoList was forgotten in Pcbnew
          *   - in zones outlines, when a change in one zone merges this zone with an other
          * This test avoids a Pcbnew crash
+         * the test is made only to avoid crashes, so it is not needed for deleted or new items
          */
-        if( aList->GetPickedItemStatus( ii ) != UR_DELETED )
+        UNDO_REDO_T status = aList->GetPickedItemStatus( ii );
+        if( status != UR_DELETED && status != UR_NEW )
         {
+            if( build_item_list )
+                // Build list of existing items, for integrity test
+                TestForExistingItem( GetBoard(), NULL );
+            build_item_list = false;
+
             if( !TestForExistingItem( GetBoard(), item ) )
             {
                 // Remove this non existant item
@@ -517,6 +520,7 @@ void PCB_EDIT_FRAME::PutDataInPreviousState( PICKED_ITEMS_LIST* aList, bool aRed
         case UR_DELETED:    /* deleted items are put in List, as new items */
             aList->SetPickedItemStatus( UR_NEW, ii );
             GetBoard()->Add( item );
+            build_item_list = true;
             break;
 
         case UR_MOVED:

pcbnew/class_board.cpp

@@ -2074,6 +2074,11 @@ BOARD_CONNECTED_ITEM* BOARD::GetLockPoint( const wxPoint& aPosition, int aLayerM
 
 TRACK* BOARD::CreateLockPoint( wxPoint& aPosition, TRACK* aSegment, PICKED_ITEMS_LIST* aList )
 {
+    /* creates an intermediate point on aSegment and break it into two segments
+     * at aPosition.
+     * The new segment starts from aPosition and ends at the end point of
+     * aSegment. The original segment now ends at aPosition.
+     */
     if( aSegment->m_Start == aPosition || aSegment->m_End == aPosition )
         return NULL;
 
@@ -2087,43 +2092,26 @@ TRACK* BOARD::CreateLockPoint( wxPoint& aPosition, TRACK* aSegment, PICKED_ITEMS
     // Calculation coordinate of intermediate point relative to the start point of aSegment
      wxPoint delta = aSegment->m_End - aSegment->m_Start;
 
-    // Not yet in use:
-#if 0
-    int ox, oy, fx, fy;
-
-    if( aRefSegm )
-    {
-        ox = aRefSegm->m_Start.x - aSegment->m_Start.x;
-        oy = aRefSegm->m_Start.y - aSegment->m_Start.y;
-        fx = aRefSegm->m_End.x - aSegment->m_Start.x;
-        fy = aRefSegm->m_End.y - aSegment->m_Start.y;
-    }
-#endif
-
     // calculate coordinates of aPosition relative to aSegment->m_Start
-    wxPoint newPoint = aPosition - aSegment->m_Start;
+    wxPoint lockPoint = aPosition - aSegment->m_Start;
 
-    // newPoint must be on aSegment:
+    // lockPoint must be on aSegment:
-    // Ensure newPoint.y/newPoint.y = delta.y/delta.x
+    // Ensure lockPoint.y/lockPoint.y = delta.y/delta.x
     if( delta.x == 0 )
-        newPoint.x = 0;         /* horizontal segment*/
+        lockPoint.x = 0;         /* horizontal segment*/
     else
-        newPoint.y = wxRound( ( (double)newPoint.x * delta.y ) / delta.x );
+        lockPoint.y = wxRound( ( (double)lockPoint.x * delta.y ) / delta.x );
 
     /* Create the intermediate point (that is to say creation of a new
      * segment, beginning at the intermediate point.
      */
-    newPoint.x += aSegment->m_Start.x;
+    lockPoint += aSegment->m_Start;
-    newPoint.y += aSegment->m_Start.y;
 
     TRACK* newTrack = (TRACK*)aSegment->Clone();
-
+    // The new segment begins at the new point,
-    if( aList )
+    newTrack->m_Start = lockPoint;
-    {
+    newTrack->start = aSegment;
-        ITEM_PICKER picker( newTrack, UR_NEW );
+    newTrack->SetState( BEGIN_ONPAD, OFF );
-        aList->PushItem( picker );
-    }
-
 
     DLIST<TRACK>* list = (DLIST<TRACK>*)aSegment->GetList();
     wxASSERT( list );
@@ -2131,28 +2119,22 @@ TRACK* BOARD::CreateLockPoint( wxPoint& aPosition, TRACK* aSegment, PICKED_ITEMS
 
     if( aList )
     {
-        ITEM_PICKER picker( aSegment, UR_CHANGED );
+        // Prepare the undo command for the now track segment
-        picker.m_Link = aSegment->Clone();
+        ITEM_PICKER picker( newTrack, UR_NEW );
+        aList->PushItem( picker );
+        // Prepare the undo command for the old track segment
+        // before modifications
+        picker.SetItem( aSegment );
+        picker.SetStatus( UR_CHANGED );
+        picker.SetLink( aSegment->Clone() );
         aList->PushItem( picker );
     }
 
-    /* Correct pointer at the end of the new segment. */
+    // Old track segment now ends at new point.
-    newTrack->end = aSegment->end;
+    aSegment->m_End = lockPoint;
-    newTrack->SetState( END_ONPAD, aSegment->GetState( END_ONPAD ) );
-
-    /* Set connections info relative to the new point
-    */
-
-    /* Old segment now ends at new point. */
-    aSegment->m_End = newPoint;
     aSegment->end = newTrack;
     aSegment->SetState( END_ONPAD, OFF );
 
-    /* The new segment begins at the new point. */
-    newTrack->m_Start = newPoint;
-    newTrack->start = aSegment;
-    newTrack->SetState( BEGIN_ONPAD, OFF );
-
     D_PAD * pad = GetPad( newTrack, START );
 
     if ( pad )
@@ -2163,7 +2145,7 @@ TRACK* BOARD::CreateLockPoint( wxPoint& aPosition, TRACK* aSegment, PICKED_ITEMS
         aSegment->SetState( END_ONPAD, ON );
     }
 
-    aPosition = newPoint;
+    aPosition = lockPoint;
     return newTrack;
 }
 

pcbnew/class_board.h

@@ -1308,10 +1308,10 @@ public:
     /**
      * Function CreateLockPoint
      * creates an intermediate point on \a aSegment and break it into two segments
-     * at \a aPoition.
+     * at \a aPosition.
      * <p>
      * The new segment starts from \a aPosition and ends at the end point of \a
-     * aSegement.  The original segment now ends at \a aPosition.
+     * aSegment.  The original segment now ends at \a aPosition.
      * </p>
      *
      * @param aPosition A wxPoint object containing the position to test and the new

pcbnew/deltrack.cpp

@@ -215,9 +215,6 @@ void PCB_EDIT_FRAME::Remove_One_Track( wxDC* DC, TRACK* pt_segm )
         next_track = tracksegment->Next();
         tracksegment->SetState( BUSY, OFF );
 
-        //D( printf( "%s: track %p status=\"%s\"\n", __func__, tracksegment,
-        //           TO_UTF8( TRACK::ShowState( tracksegment->GetState( -1 ) ) )
-        //          ); )
         D( std::cout << __func__ << ": track " << tracksegment << " status=" \
                      << TO_UTF8( TRACK::ShowState( tracksegment->GetState( -1 ) ) ) \
                      << std::endl; )

pcbnew/editrack.cpp

@@ -50,6 +50,8 @@ static void ComputeBreakPoint( TRACK* track, int n, wxPoint end );
 static void DeleteNullTrackSegments( BOARD* pcb, DLIST<TRACK>& aTrackList );
 static void EnsureEndTrackOnPad( D_PAD* Pad );
 
+// A PICKED_ITEMS_LIST to store tracks  which are modified/added/deleted
+// during a track edition:
 static PICKED_ITEMS_LIST s_ItemsListPicker;
 
 
@@ -456,12 +458,9 @@ bool PCB_EDIT_FRAME::End_Route( TRACK* aTrack, wxDC* aDC )
         else        // If end point of is on a different track,
                     // creates a lock point if not exists
         {
-            TRACK* adr_buf = (TRACK*) LockPoint;
+             // Creates a lock point, if not already exists:
-            GetBoard()->SetHighLightNet( adr_buf->GetNet() );
-
-            // Creates a lock point (if not already exists):
             LockPoint = GetBoard()->CreateLockPoint( g_CurrentTrackSegment->m_End,
-                                                     adr_buf,
+                                                     (TRACK*) LockPoint,
                                                      &s_ItemsListPicker );
         }
     }

pcbnew/tr_modif.cpp

@@ -25,7 +25,8 @@
 
 /**
  * @file tr_modif.cpp
- * @brief Trace editing.
+ * @brief Trace editing: detects an removes a track which is become redunding,
+ * after a new track is craeted.
  */
 
 #include <fctsys.h>
@@ -42,6 +43,10 @@
 static void ListSetState( EDA_ITEM* Start, int NbItem, int State, int onoff );
 
 
+/*
+ * This function try to remove an old track, when a new track is created,
+ * and the old track is no more needed
+ */
 int PCB_EDIT_FRAME::EraseRedundantTrack( wxDC*              aDC,
                                          TRACK*             aNewTrack,
                                          int                aNewTrackSegmentsCount,
@@ -292,14 +297,11 @@ int PCB_EDIT_FRAME::EraseRedundantTrack( wxDC*              aDC,
 }
 
 
-/* Set the bits of .m_State member to on off value, using bit mask State
+/* Set the bits of .m_State member to on/off value, using bit mask State
  * of a list of EDA_ITEM
  */
 static void ListSetState( EDA_ITEM* Start, int NbItem, int State, int onoff )
 {
-    if( Start == NULL )
-        return;
-
     for( ; (Start != NULL ) && ( NbItem > 0 ); NbItem--, Start = Start->Next() )
     {
         Start->SetState( State, onoff );

pcbnew/xchgmod.cpp

@@ -152,7 +152,6 @@ int DIALOG_EXCHANGE_MODULE::Maj_ListeCmp( const wxString& reference,
     FILE*       FichCmp, * NewFile;
     char        line[1024];
     wxString    msg;
-    char*       ignore;
 
     if( old_name == new_name )
         return 0;
@@ -190,9 +189,9 @@ int DIALOG_EXCHANGE_MODULE::Maj_ListeCmp( const wxString& reference,
         return 1;
     }
 
-    ignore = fgets( line, sizeof(line), FichCmp );
+    fgets( line, sizeof(line), FichCmp );
 
-    fprintf( NewFile, "Cmp-Mod V01 Genere par PcbNew le %s\n", TO_UTF8( DateAndTime() ) );
+    fprintf( NewFile, "Cmp-Mod V01 Created by PcbNew date = %s\n", TO_UTF8( DateAndTime() ) );
 
     bool start_descr = false;
 
@@ -588,7 +587,6 @@ void PCB_EDIT_FRAME::RecreateCmpFileFromBoard( wxCommandEvent& aEvent )
     MODULE*     Module = GetBoard()->m_Modules;
     wxString    msg;
     wxString    wildcard;
-    char*       ignore;
 
     if( Module == NULL )
     {
@@ -619,7 +617,7 @@ void PCB_EDIT_FRAME::RecreateCmpFileFromBoard( wxCommandEvent& aEvent )
         return;
     }
 
-    ignore = fgets( line, sizeof(line), FichCmp );
+    fgets( line, sizeof(line), FichCmp );
     fprintf( FichCmp, "Cmp-Mod V01 Genere par PcbNew le %s\n", TO_UTF8( DateAndTime() ) );
 
     for( ; Module != NULL; Module = Module->Next() )