Fix undo/redo and revert for libedit name changes and aliases

Three interrelated issues:
1) Implement an undo/redo type for renames so that we know to delete
the old lib entry and add a new lib entry
2) When doing so (for the undo/redo OR the original edit), we must
make a copy of the 'original' LIB_PART which is used for revert, and
hand it to the new lib entry
3) When comparing a modified component tree item with the current item
we must also check for aliases.

Fixes: lp:1743857
* https://bugs.launchpad.net/kicad/+bug/1743857

Fixes: lp:1744371
* https://bugs.launchpad.net/kicad/+bug/1744371

Fixes: lp:1744373
* https://bugs.launchpad.net/kicad/+bug/1744373

common/class_undoredo_container.cpp

@@ -148,9 +148,8 @@ void PICKED_ITEMS_LIST::ClearListAndDeleteItems()
             break;
 
         case UR_DELETED:            // the picker is owner of this item
-        case UR_LIBEDIT:            /* Libedit save always a copy of the current item
-                                     *  So, the picker is always owner of the picked item
-                                     */
+        case UR_LIBEDIT:            // LIBEDIT and LIB_RENAME save a copy of the current item
+        case UR_LIB_RENAME:         //    so the picker is the owner of the picked item
             delete wrapper.GetItem();
             break;
 

eeschema/lib_manager.cpp

@@ -367,6 +367,32 @@ bool LIB_MANAGER::UpdatePart( LIB_PART* aPart, const wxString& aLibrary )
 }
 
 
+bool LIB_MANAGER::UpdatePartAfterRename( LIB_PART* aPart, const wxString& oldAlias,
+                                         const wxString& aLibrary )
+{
+    // This is essentially a delete/update, but we have to make a copy of the "original"
+    // LIB_PART from the old buffer to give to the new one.
+
+    LIB_BUFFER& libBuf = getLibraryBuffer( aLibrary );
+    auto partBuf = libBuf.GetBuffer( oldAlias );
+    wxCHECK( partBuf, false );
+
+    LIB_PART* original = new LIB_PART( *partBuf->GetOriginal() );
+
+    if( !libBuf.DeleteBuffer( partBuf ) )
+        return false;
+
+    if( !UpdatePart( aPart, aLibrary ))
+        return false;
+
+    partBuf = libBuf.GetBuffer( aPart->GetName() );
+    wxCHECK( partBuf, false );
+    partBuf->SetOriginal( original ); // part buffer takes ownership of pointer
+
+    return true;
+}
+
+
 bool LIB_MANAGER::FlushPart( const wxString& aAlias, const wxString& aLibrary )
 {
     auto it = m_libs.find( aLibrary );
@@ -381,19 +407,28 @@ bool LIB_MANAGER::FlushPart( const wxString& aAlias, const wxString& aLibrary )
 }
 
 
-bool LIB_MANAGER::RevertPart( const wxString& aAlias, const wxString& aLibrary )
+LIB_ID LIB_MANAGER::RevertPart( const wxString& aAlias, const wxString& aLibrary )
 {
     auto it = m_libs.find( aLibrary );
 
     if( it == m_libs.end() )    // no items to flush
-        return true;
+        return LIB_ID( aLibrary, aAlias );
 
     auto partBuf = it->second.GetBuffer( aAlias );
-    wxCHECK( partBuf, false );
-    partBuf->SetPart( new LIB_PART( *partBuf->GetOriginal() ) );
-    m_frame.SyncLibraries( false );
+    wxCHECK( partBuf, LIB_ID( aLibrary, aAlias ) );
+    LIB_PART original( *partBuf->GetOriginal() );
 
-    return true;
+    if( original.GetName() != aAlias )
+    {
+        UpdatePartAfterRename( &original, aAlias, aLibrary );
+    }
+    else
+    {
+        partBuf->SetPart( new LIB_PART( original ) );
+        m_frame.SyncLibraries( false );
+    }
+
+    return LIB_ID( aLibrary, original.GetName() );
 }
 
 

eeschema/lib_manager.h

@@ -102,6 +102,13 @@ public:
      */
     bool UpdatePart( LIB_PART* aPart, const wxString& aLibrary );
 
+    /**
+     * Updates the part buffer with a new version of the part when the name has changed.
+     * The old library buffer will be deleted and a new one created with the new name.
+     */
+    bool UpdatePartAfterRename( LIB_PART* aPart, const wxString& oldAlias,
+                                const wxString& aLibrary );
+
     /**
      * Removes the part from the part buffer.
      * It is required to save the library to have the part removed in the schematic editor.
@@ -192,9 +199,10 @@ public:
 
     /**
      * Reverts unsaved changes for a particular part.
-     * @return True on success, false otherwise.
+     * @return The LIB_ID of the reverted part (which may be different in the case
+     * of a rename)
      */
-    bool RevertPart( const wxString& aAlias, const wxString& aLibrary );
+    LIB_ID RevertPart( const wxString& aAlias, const wxString& aLibrary );
 
     /**
      * Reverts unsaved changes for a particular library.

eeschema/libedit.cpp

@@ -466,8 +466,10 @@ void LIB_EDIT_FRAME::OnRevertPart( wxCommandEvent& aEvent )
     if( currentPart )
         emptyScreen();
 
-    if( m_libMgr->RevertPart( libId.GetLibItemName(), libId.GetLibNickname() ) )
-        m_libMgr->ClearPartModified( libId.GetLibItemName(), libId.GetLibNickname() );
+    libId = m_libMgr->RevertPart( libId.GetLibItemName(), libId.GetLibNickname() );
+
+    m_treePane->GetCmpTree()->SelectLibId( libId );
+    m_libMgr->ClearPartModified( libId.GetLibItemName(), libId.GetLibNickname() );
 
     if( currentPart && m_libMgr->PartExists( libId.GetLibItemName(), libId.GetLibNickname() ) )
         loadPart( libId.GetLibItemName(), libId.GetLibNickname(), unit );

eeschema/libedit_undo_redo.cpp

@@ -27,9 +27,12 @@
 
 #include <libeditframe.h>
 #include <class_libentry.h>
+#include <lib_manager.h>
+#include <component_tree.h>
+#include <cmp_tree_pane.h>
 
 
-void LIB_EDIT_FRAME::SaveCopyInUndoList( EDA_ITEM* ItemToCopy )
+void LIB_EDIT_FRAME::SaveCopyInUndoList( EDA_ITEM* ItemToCopy, UNDO_REDO_T undoType )
 {
     LIB_PART*          CopyItem;
     PICKED_ITEMS_LIST* lastcmd = new PICKED_ITEMS_LIST();
@@ -39,7 +42,7 @@ void LIB_EDIT_FRAME::SaveCopyInUndoList( EDA_ITEM* ItemToCopy )
     // Clear current flags (which can be temporary set by a current edit command).
     CopyItem->ClearStatus();
 
-    ITEM_PICKER wrapper( CopyItem, UR_LIBEDIT );
+    ITEM_PICKER wrapper( CopyItem, undoType );
     lastcmd->PushItem( wrapper );
     GetScreen()->PushCommandToUndoList( lastcmd );
 
@@ -53,18 +56,19 @@ void LIB_EDIT_FRAME::GetComponentFromRedoList( wxCommandEvent& event )
     if( GetScreen()->GetRedoCommandCount() <= 0 )
         return;
 
-    // Store the current part in the undo buffer
-    PICKED_ITEMS_LIST* lastcmd = new PICKED_ITEMS_LIST();
-    LIB_PART* part = GetCurPart();
-    ITEM_PICKER wrapper( part, UR_LIBEDIT );
-    lastcmd->PushItem( wrapper );
-    GetScreen()->PushCommandToUndoList( lastcmd );
-
     // Load the last redo entry
-    lastcmd = GetScreen()->PopCommandFromRedoList();
-    wrapper = lastcmd->PopItem();
-    delete lastcmd;
-    part = (LIB_PART*) wrapper.GetItem();
+    PICKED_ITEMS_LIST* redoCommand = GetScreen()->PopCommandFromRedoList();
+    ITEM_PICKER redoWrapper = redoCommand->PopItem();
+    delete redoCommand;
+    LIB_PART* part = (LIB_PART*) redoWrapper.GetItem();
+    UNDO_REDO_T undoRedoType = redoWrapper.GetStatus();
+
+    // Store the current part in the undo buffer
+    PICKED_ITEMS_LIST* undoCommand = new PICKED_ITEMS_LIST();
+    LIB_PART* oldPart = GetCurPart();
+    ITEM_PICKER undoWrapper( oldPart, undoRedoType );
+    undoCommand->PushItem( undoWrapper );
+    GetScreen()->PushCommandToUndoList( undoCommand );
 
     // Do not delete the previous part by calling SetCurPart( part )
     // which calls delete <previous part>.
@@ -75,6 +79,15 @@ void LIB_EDIT_FRAME::GetComponentFromRedoList( wxCommandEvent& event )
     if( !part )
         return;
 
+    if( undoRedoType == UR_LIB_RENAME )
+    {
+        wxString lib = GetCurLib();
+        m_libMgr->UpdatePartAfterRename( part, oldPart->GetName(), lib );
+
+        // Reselect the renamed part
+        m_treePane->GetCmpTree()->SelectLibId( LIB_ID( lib, part->GetName() ) );
+    }
+
     if( !m_aliasName.IsEmpty() && !part->HasAlias( m_aliasName ) )
         m_aliasName = part->GetName();
 
@@ -94,18 +107,19 @@ void LIB_EDIT_FRAME::GetComponentFromUndoList( wxCommandEvent& event )
     if( GetScreen()->GetUndoCommandCount() <= 0 )
         return;
 
-    // Store the current part in the redo buffer
-    PICKED_ITEMS_LIST* lastcmd = new PICKED_ITEMS_LIST();
-    LIB_PART* part = GetCurPart();
-    ITEM_PICKER wrapper( part, UR_LIBEDIT );
-    lastcmd->PushItem( wrapper );
-    GetScreen()->PushCommandToRedoList( lastcmd );
-
     // Load the last undo entry
-    lastcmd = GetScreen()->PopCommandFromUndoList();
-    wrapper = lastcmd->PopItem();
-    delete lastcmd;
-    part = (LIB_PART*) wrapper.GetItem();
+    PICKED_ITEMS_LIST* undoCommand = GetScreen()->PopCommandFromUndoList();
+    ITEM_PICKER undoWrapper = undoCommand->PopItem();
+    delete undoCommand;
+    LIB_PART* part = (LIB_PART*) undoWrapper.GetItem();
+    UNDO_REDO_T undoRedoType = undoWrapper.GetStatus();
+
+    // Store the current part in the redo buffer
+    PICKED_ITEMS_LIST* redoCommand = new PICKED_ITEMS_LIST();
+    LIB_PART* oldPart = GetCurPart();
+    ITEM_PICKER redoWrapper( oldPart, undoRedoType );
+    redoCommand->PushItem( redoWrapper );
+    GetScreen()->PushCommandToRedoList( redoCommand );
 
     printf("RestoreCopy [%p]\n", part);
 
@@ -118,6 +132,15 @@ void LIB_EDIT_FRAME::GetComponentFromUndoList( wxCommandEvent& event )
     if( !part )
         return;
 
+    if( undoRedoType == UR_LIB_RENAME )
+    {
+        wxString lib = GetCurLib();
+        m_libMgr->UpdatePartAfterRename( part, oldPart->GetName(), lib );
+
+        // Reselect the renamed part
+        m_treePane->GetCmpTree()->SelectLibId( LIB_ID( lib, part->GetName() ) );
+    }
+
     if( !m_aliasName.IsEmpty() && !part->HasAlias( m_aliasName ) )
         m_aliasName = part->GetName();
 

eeschema/libeditframe.cpp

@@ -1732,7 +1732,10 @@ void LIB_EDIT_FRAME::storeCurrentPart()
 
 bool LIB_EDIT_FRAME::isCurrentPart( const LIB_ID& aLibId ) const
 {
-    return ( GetCurPart() && aLibId == GetCurPart()->GetLibId() );
+    // This will return the root part of any alias
+    LIB_PART* part = m_libMgr->GetBufferedPart( aLibId.GetLibItemName(), aLibId.GetLibNickname() );
+    // Now we can compare the libId of the current part and the root part
+    return ( GetCurPart() && part->GetLibId() == GetCurPart()->GetLibId() );
 }
 
 

eeschema/libeditframe.h

@@ -555,10 +555,10 @@ public:
     /**
      * Create a copy of the current component, and save it in the undo list.
      *
-     * Because a component in library editor does not a lot of primitives,
-     * the full data is duplicated. It is not worth to try to optimize this save funtion
+     * Because a component in library editor does not have a lot of primitives,
+     * the full data is duplicated. It is not worth to try to optimize this save function.
      */
-    void SaveCopyInUndoList( EDA_ITEM* ItemToCopy );
+    void SaveCopyInUndoList( EDA_ITEM* ItemToCopy, UNDO_REDO_T undoType = UR_LIBEDIT );
 
 private:
     void GetComponentFromUndoList( wxCommandEvent& event );
@@ -737,7 +737,7 @@ private:
     ///> Stores the currently modified part in the library manager buffer.
     void storeCurrentPart();
 
-    ///> Returns true if currently modified part has the same LIB_ID.
+    ///> Returns true if \a aLibId is an alias for the editor screen part.
     bool isCurrentPart( const LIB_ID& aLibId ) const;
 
     ///> Restores the empty editor screen, without any part or library selected.

eeschema/libfield.cpp

@@ -94,13 +94,13 @@ void LIB_EDIT_FRAME::EditField( LIB_FIELD* aField )
             return;
         }
 
+        SaveCopyInUndoList( parent, UR_LIB_RENAME );
         parent->SetName( newFieldValue );
 
         if( !parent->HasAlias( m_aliasName ) )
             m_aliasName = newFieldValue;
 
-        m_libMgr->RemovePart( oldFieldValue, lib );
-        m_libMgr->UpdatePart( parent, lib );
+        m_libMgr->UpdatePartAfterRename( parent, oldFieldValue, lib );
 
         // Reselect the renamed part
         m_treePane->GetCmpTree()->SelectLibId( LIB_ID( lib, newFieldValue ) );

include/class_undoredo_container.h

@@ -71,6 +71,7 @@ enum UNDO_REDO_T {
     UR_WIRE_IMAGE,          // Specific to Eeschema for handling wires changes.
     UR_LIBEDIT,             // Specific to the component editor (libedit creates a full copy
                             // of the current component when changed)
+    UR_LIB_RENAME,          // As UR_LIBEDIT, but old copy should be removed from library
     UR_EXCHANGE_T           ///< Use for changing the schematic text type where swapping
                             ///< data structure is insufficient to restor the change.
 };