Fix broken symbol reference designators on paste special.

It appears that in our zeal to prevent file changes when saving shared
schematics, we (I) clobbered saving relative symbol instance data paths
to the clipboard.  This has be restored along with setting the correct
symbol unit for relative clipboard paths.

Fixed a serious issue with KIID_PATH::MakeRelativeTo() where the original
path was not restored when the incremental KIID object test fails.  This
also included a minor optimization using the actual KIID object for
comparison instead of converting it to a string and then comparing the
string.

Fixes https://gitlab.com/kicad/code/kicad/-/issues/15981

common/kiid.cpp

@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2020 Ian McInerney <ian.s.mcinerney@ieee.org>
  * Copyright (C) 2007-2014 Jean-Pierre Charras, jp.charras at wanadoo.fr
- * Copyright (C) 1992-2022 KiCad Developers, see AUTHORS.TXT for contributors.
+ * Copyright (C) 1992-2023 KiCad Developers, see AUTHORS.TXT for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -324,8 +324,11 @@ bool KIID_PATH::MakeRelativeTo( const KIID_PATH& aPath )
 
     for( size_t i = 0; i < aPath.size(); ++i )
     {
-        if( copy.at( i ).AsString() != aPath.at( i ).AsString() )
+        if( copy.at( i ) != aPath.at( i ) )
+        {
+            *this = copy;
             return false; // this path is not contained within aPath
+        }
     }
 
     for( size_t i = aPath.size(); i < copy.size(); ++i )

eeschema/files-io.cpp

@@ -463,6 +463,9 @@ bool SCH_EDIT_FRAME::OpenProjectFiles( const std::vector<wxString>& aFileSet, in
             for( SCH_SCREEN* screen = schematic.GetFirst(); screen; screen = schematic.GetNext() )
                 screen->FixLegacyPowerSymbolMismatches();
 
+            schematic.PruneOrphanedSymbolInstances( Prj().GetProjectName(),
+                                                    Schematic().GetSheets() );
+
             // Allow the schematic to be saved to new file format without making any edits.
             OnModify();
         }

eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp

@@ -554,10 +554,6 @@ void SCH_SEXPR_PLUGIN::Format( EE_SELECTION* aSelection, SCH_SHEET_PATH* aSelect
         m_out->Print( 0, ")\n\n" );
     }
 
-    // Store the selected sheets instance information
-    SCH_SHEET_LIST selectedSheets;
-    SCH_REFERENCE_LIST selectedSymbols;
-
     for( i = 0; i < aSelection->GetSize(); ++i )
     {
         item = (SCH_ITEM*) aSelection->GetItem( i );
@@ -565,10 +561,8 @@ void SCH_SEXPR_PLUGIN::Format( EE_SELECTION* aSelection, SCH_SHEET_PATH* aSelect
         switch( item->Type() )
         {
         case SCH_SYMBOL_T:
-            saveSymbol( static_cast<SCH_SYMBOL*>( item ), aSchematic, 0, aForClipboard );
+            saveSymbol( static_cast<SCH_SYMBOL*>( item ), aSchematic, 0, aForClipboard,
-
+                        aSelectionPath );
-            aSelectionPath->AppendSymbol( selectedSymbols, static_cast<SCH_SYMBOL*>( item ),
-                                          true, true );
             break;
 
         case SCH_BITMAP_T:
@@ -577,15 +571,6 @@ void SCH_SEXPR_PLUGIN::Format( EE_SELECTION* aSelection, SCH_SHEET_PATH* aSelect
 
         case SCH_SHEET_T:
             saveSheet( static_cast< SCH_SHEET* >( item ), 0 );
-
-            {
-                SCH_SHEET_PATH subSheetPath = *aSelectionPath;
-                subSheetPath.push_back( static_cast<SCH_SHEET*>( item ) );
-
-                fullHierarchy.GetSheetsWithinPath( selectedSheets, subSheetPath );
-                fullHierarchy.GetSymbolsWithinPath( selectedSymbols, subSheetPath, true, true );
-            }
-
             break;
 
         case SCH_JUNCTION_T:
@@ -625,33 +610,12 @@ void SCH_SEXPR_PLUGIN::Format( EE_SELECTION* aSelection, SCH_SHEET_PATH* aSelect
             wxASSERT( "Unexpected schematic object type in SCH_SEXPR_PLUGIN::Format()" );
         }
     }
-
-    // Make all instance information relative to the selection path
-    KIID_PATH selectionPath = aSelectionPath->Path();
-
-    selectedSheets.SortByPageNumbers();
-    std::vector<SCH_SHEET_INSTANCE> sheetinstances = selectedSheets.GetSheetInstances();
-
-    for( SCH_SHEET_INSTANCE& sheetInstance : sheetinstances )
-    {
-        wxASSERT_MSG( sheetInstance.m_Path.MakeRelativeTo( selectionPath ),
-                      "Sheet is not inside the selection path?" );
-    }
-
-    selectionPath = aSelectionPath->Path();
-    selectedSymbols.SortByReferenceOnly();
-    std::vector<SCH_SYMBOL_INSTANCE> symbolInstances = selectedSymbols.GetSymbolInstances();
-
-    for( SCH_SYMBOL_INSTANCE& symbolInstance : symbolInstances )
-    {
-        wxASSERT_MSG( symbolInstance.m_Path.MakeRelativeTo( selectionPath ),
-                      "Symbol is not inside the selection path?" );
-    }
 }
 
 
 void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchematic,
-                                   int aNestLevel, bool aForClipboard )
+                                   int aNestLevel, bool aForClipboard,
+                                   const SCH_SHEET_PATH* aRelativePath )
 {
     wxCHECK_RET( aSymbol != nullptr && m_out != nullptr, "" );
 
@@ -721,6 +685,14 @@ void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchema
                aSymbol->GetUnit() :
                aSymbol->GetInstanceReferences()[0].m_Unit;
 
+    if( aForClipboard && aRelativePath )
+    {
+        SCH_SYMBOL_INSTANCE unitInstance;
+
+        if( aSymbol->GetInstance( unitInstance, aRelativePath->Path() ) )
+            unit = unitInstance.m_Unit;
+    }
+
     m_out->Print( 0, " (unit %d)", unit );
 
     if( aSymbol->GetConvert() == LIB_ITEM::LIB_CONVERT::DEMORGAN )
@@ -765,6 +737,14 @@ void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchema
                 field.SetText( aSymbol->GetField( FOOTPRINT_FIELD )->GetText() );
             }
         }
+        else if( aForClipboard && aSymbol->GetInstanceReferences().size() && aRelativePath
+               && ( id == REFERENCE_FIELD ) )
+        {
+            SCH_SYMBOL_INSTANCE instance;
+
+            if( aSymbol->GetInstance( instance, aRelativePath->Path() ) )
+                field.SetText( instance.m_Reference );
+        }
 
         try
         {
@@ -845,7 +825,13 @@ void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchema
                 project_open = true;
             }
 
-            wxString path = aSymbol->GetInstanceReferences()[i].m_Path.AsString();
+            wxString path;
+            KIID_PATH tmp = aSymbol->GetInstanceReferences()[i].m_Path;
+
+            if( aForClipboard && aRelativePath )
+                tmp.MakeRelativeTo( aRelativePath->Path() );
+
+            path = tmp.AsString();
 
             m_out->Print( aNestLevel + 3, "(path %s\n",
                           m_out->Quotew( path ).c_str() );

eeschema/sch_plugins/kicad/sch_sexpr_plugin.h

@@ -150,7 +150,7 @@ private:
     void loadFile( const wxString& aFileName, SCH_SHEET* aSheet );
 
     void saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchematic, int aNestLevel,
-                     bool aForClipboard );
+                     bool aForClipboard, const SCH_SHEET_PATH* aRelativePath = nullptr );
     void saveField( SCH_FIELD* aField, int aNestLevel );
     void saveBitmap( SCH_BITMAP* aBitmap, int aNestLevel );
     void saveSheet( SCH_SHEET* aSheet, int aNestLevel );

eeschema/sch_screen.cpp

@@ -36,6 +36,7 @@
 #include <project.h>
 #include <project_sch.h>
 #include <reporter.h>
+#include <trace_helpers.h>
 #include <sch_edit_frame.h>
 #include <sch_item.h>
 
@@ -1637,6 +1638,55 @@ size_t SCH_SCREEN::getLibSymbolNameMatches( const SCH_SYMBOL& aSymbol,
 }
 
 
+void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName,
+                                               const SCH_SHEET_LIST& aValidSheetPaths )
+{
+    wxCHECK( !aProjectName.IsEmpty(), /* void */ );
+
+    for( SCH_ITEM* item : Items().OfType( SCH_SYMBOL_T ) )
+    {
+        SCH_SYMBOL* symbol = static_cast<SCH_SYMBOL*>( item );
+
+        wxCHECK2( symbol, continue );
+
+        std::set<SCH_SHEET_PATH> pathsToPrune;
+        const std::vector<SCH_SYMBOL_INSTANCE> instances = symbol->GetInstanceReferences();
+
+        for( const SCH_SYMBOL_INSTANCE& instance : instances )
+        {
+            if( aProjectName != instance.m_ProjectName )
+                continue;
+
+            std::optional<SCH_SHEET_PATH> pathFound =
+                    aValidSheetPaths.GetSheetPathByKIIDPath( instance.m_Path );
+
+            // Check for paths from other projects.
+            if( !pathFound )
+            {
+                pathsToPrune.emplace( pathFound.value() );
+                continue;
+            }
+
+            // Check for paths that are in that are not valid for this screen.
+            for( const SCH_SHEET_PATH& path : aValidSheetPaths )
+            {
+                if( path.LastScreen() == this )
+                    continue;
+
+                 pathsToPrune.emplace( path );
+            }
+        }
+
+        for( const SCH_SHEET_PATH& sheetPath : pathsToPrune )
+        {
+            wxLogTrace( traceSchSheetPaths, wxS( "Pruning project '%s' symbol instance %s." ),
+                        aProjectName, sheetPath.Path().AsString() );
+            symbol->RemoveInstance( sheetPath );
+        }
+    }
+}
+
+
 #if defined(DEBUG)
 void SCH_SCREEN::Show( int nestLevel, std::ostream& os ) const
 {
@@ -2060,3 +2110,14 @@ void SCH_SCREEN::MigrateSimModels()
         SIM_MODEL::MigrateSimModel<SCH_SYMBOL, SCH_FIELD>( *symbol, &Schematic()->Prj() );
     }
 }
+
+
+void SCH_SCREENS::PruneOrphanedSymbolInstances( const wxString& aProjectName,
+
+                                                const SCH_SHEET_LIST& aValidSheetPaths )
+{
+    wxCHECK( !aProjectName.IsEmpty(), /* void */ );
+
+    for( SCH_SCREEN* screen = GetFirst(); screen; screen = GetNext() )
+        screen->PruneOrphanedSymbolInstances( aProjectName, aValidSheetPaths );
+}

eeschema/sch_screen.h

@@ -551,6 +551,9 @@ public:
      */
     void MigrateSimModels();
 
+    void PruneOrphanedSymbolInstances( const wxString& aProjectName,
+                                       const SCH_SHEET_LIST& aValidSheetPaths );
+
 private:
     friend SCH_EDIT_FRAME;     // Only to populate m_symbolInstances.
     friend SCH_SEXPR_PARSER;   // Only to load instance information from schematic file.
@@ -805,6 +808,9 @@ public:
      */
     void FixLegacyPowerSymbolMismatches();
 
+    void PruneOrphanedSymbolInstances( const wxString& aProjectName,
+                                       const SCH_SHEET_LIST& aValidSheetPaths );
+
 private:
     void addScreenToList( SCH_SCREEN* aScreen, SCH_SHEET* aSheet );
     void buildScreenList( SCH_SHEET* aSheet);

eeschema/sch_sheet.cpp

@@ -1239,6 +1239,39 @@ bool SCH_SHEET::operator <( const SCH_ITEM& aItem ) const
 }
 
 
+void SCH_SHEET::RemoveInstance( const KIID_PATH& aInstancePath )
+{
+    // Search for an existing path and remove it if found (should not occur)
+    for( unsigned ii = 0; ii < m_instances.size(); ii++ )
+    {
+        if( m_instances[ii].m_Path == aInstancePath )
+        {
+            wxLogTrace( traceSchSheetPaths, "Removing sheet instance:\n"
+                                            "  sheet path %s\n"
+                                            "  page %s, from project %s.",
+                        aInstancePath.AsString(),
+                        m_instances[ii].m_PageNumber,
+                        m_instances[ii].m_ProjectName );
+
+            m_instances.erase( m_instances.begin() + ii );
+            ii--;
+        }
+    }
+}
+
+
+void SCH_SHEET::AddInstance( const SCH_SHEET_INSTANCE& aInstance )
+{
+    SCH_SHEET_INSTANCE oldInstance;
+
+    if( getInstance( oldInstance, aInstance.m_Path ) )
+        RemoveInstance( aInstance.m_Path );
+
+    m_instances.emplace_back( aInstance );
+
+}
+
+
 bool SCH_SHEET::addInstance( const SCH_SHEET_PATH& aSheetPath )
 {
     wxCHECK( aSheetPath.IsFullPath(), false );

eeschema/sch_sheet.h

@@ -408,6 +408,10 @@ public:
      */
     const SCH_SHEET_INSTANCE& GetRootInstance() const;
 
+    void RemoveInstance( const KIID_PATH& aInstancePath );
+
+    void AddInstance( const SCH_SHEET_INSTANCE& aInstance );
+
     /**
      * Compares page numbers of schematic sheets.
      *

eeschema/sch_symbol.cpp

@@ -598,16 +598,22 @@ bool SCH_SYMBOL::GetInstance( SCH_SYMBOL_INSTANCE& aInstance,
 
 
 void SCH_SYMBOL::RemoveInstance( const SCH_SHEET_PATH& aInstancePath )
+{
+    RemoveInstance( aInstancePath.Path() );
+}
+
+
+void SCH_SYMBOL::RemoveInstance( const KIID_PATH& aInstancePath )
 {
     // Search for an existing path and remove it if found (should not occur)
     for( unsigned ii = 0; ii < m_instanceReferences.size(); ii++ )
     {
-        if( m_instanceReferences[ii].m_Path == aInstancePath.Path() )
+        if( m_instanceReferences[ii].m_Path == aInstancePath )
         {
             wxLogTrace( traceSchSheetPaths, "Removing symbol instance:\n"
                                             "  sheet path %s\n"
                                             "  reference %s, unit %d from symbol %s.",
-                        aInstancePath.Path().AsString(),
+                        aInstancePath.AsString(),
                         m_instanceReferences[ii].m_Reference,
                         m_instanceReferences[ii].m_Unit,
                         m_Uuid.AsString() );

eeschema/sch_symbol.h

@@ -147,6 +147,8 @@ public:
 
     void RemoveInstance( const SCH_SHEET_PATH& aInstancePath );
 
+    void RemoveInstance( const KIID_PATH& aInstancePath );
+
     void SortInstances( bool ( *aSortFunction )( const SCH_SYMBOL_INSTANCE& aLhs,
                                                  const SCH_SYMBOL_INSTANCE& aRhs ) );
 

eeschema/tools/sch_editor_control.cpp

@@ -1392,6 +1392,8 @@ void SCH_EDITOR_CONTROL::updatePastedSymbol( SCH_SYMBOL* aSymbol, SCH_SCREEN* aP
     wxString reference, value, footprint;
     int      unit;
 
+    clipItemPath.push_back( aSymbol->m_Uuid );
+
     if( m_clipboardSymbolInstances.count( clipItemPath ) > 0 )
     {
         SCH_SYMBOL_INSTANCE instance = m_clipboardSymbolInstances.at( clipItemPath );
@@ -1471,17 +1473,59 @@ SCH_SHEET_PATH SCH_EDITOR_CONTROL::updatePastedSheet( const SCH_SHEET_PATH& aPas
 }
 
 
-void SCH_EDITOR_CONTROL::setClipboardInstances( const SCH_SCREEN* aPastedScreen )
+void SCH_EDITOR_CONTROL::setPastedSheetInstances( const SCH_SHEET* aPastedSheet )
 {
-    m_clipboardSheetInstances.clear();
+    wxCHECK( aPastedSheet, /* void */ );
 
-    for( const SCH_SHEET_INSTANCE& sheet : aPastedScreen->GetSheetInstances() )
+    for( const SCH_SHEET_INSTANCE& sheetInstance : aPastedSheet->GetInstances() )
-        m_clipboardSheetInstances[sheet.m_Path] = sheet;
+    {
+        KIID_PATH pathWithSheet = sheetInstance.m_Path;
 
-    m_clipboardSymbolInstances.clear();
+        pathWithSheet.push_back( aPastedSheet->m_Uuid );
+        m_clipboardSheetInstances[pathWithSheet] = sheetInstance;
+    }
 
-    for( const SCH_SYMBOL_INSTANCE& symbol : aPastedScreen->GetSymbolInstances() )
+    const SCH_SCREEN* screen = aPastedSheet->GetScreen();
-        m_clipboardSymbolInstances[symbol.m_Path] = symbol;
+
+    wxCHECK( screen, /* void */ );
+
+    for( SCH_ITEM* item : screen->Items() )
+    {
+        if( item->Type() == SCH_SHEET_T )
+        {
+            const SCH_SHEET* sheet = static_cast<SCH_SHEET*>( item );
+
+            wxCHECK2( sheet, continue );
+
+            setPastedSheetInstances( sheet );
+        }
+    }
+}
+
+
+void SCH_EDITOR_CONTROL::setPastedSymbolInstances( SCH_SCREENS& aScreenList )
+{
+    for( SCH_SCREEN* screen = aScreenList.GetFirst(); screen; screen = aScreenList.GetNext() )
+    {
+        for( const SCH_ITEM* item : screen->Items() )
+        {
+            if( item->Type() == SCH_SYMBOL_T )
+            {
+                const SCH_SYMBOL* symbol = static_cast<const SCH_SYMBOL*>( item );
+
+                wxCHECK2( symbol, continue );
+
+                for( const SCH_SYMBOL_INSTANCE& symbolInstance : symbol->GetInstanceReferences() )
+                {
+                    KIID_PATH pathWithSymbol = symbolInstance.m_Path;
+
+                    pathWithSymbol.push_back( symbol->m_Uuid );
+
+                    m_clipboardSymbolInstances[pathWithSymbol] = symbolInstance;
+                }
+            }
+        }
+    }
 }
 
 
@@ -1533,8 +1577,14 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
         tempScreen->Append( text_item );
     }
 
-    // Save loaded screen instances to m_clipboardSheetInstances
+    SCH_SCREENS tempScreens( tempSheet );
-    setClipboardInstances( tempScreen );
+
+    m_clipboardSheetInstances.clear();
+    m_clipboardSymbolInstances.clear();
+
+    // Save pasted sheet and symbol instances.
+    setPastedSheetInstances( &tempSheet );
+    setPastedSymbolInstances( tempScreens );
 
     tempScreen->MigrateSimModels();
 

eeschema/tools/sch_editor_control.h

@@ -191,7 +191,8 @@ private:
                                       SCH_SHEET_LIST* aPastedSheetsSoFar,
                                       SCH_REFERENCE_LIST* aPastedSymbolsSoFar );
 
-    void setClipboardInstances( const SCH_SCREEN* aPastedScreen );
+    void setPastedSheetInstances( const SCH_SHEET* aPastedSheet );
+    void setPastedSymbolInstances( SCH_SCREENS& aScreenList );
 
     /**
      * Read the footprint info from each line in the stuff file by reference designator.