From 1ca0737c34adaba4d0a7d4ac6854ea7f609094e1 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Wed, 24 Apr 2024 12:44:32 -0700 Subject: [PATCH] Replace stale pin references with UNDO copy When we replace a symbol with one that has fewer pins, the old pins are released, which leaves points to them in the connection graph dangling. This updates the pointer to use the cloned copy in the undo stack until the connection graph is rebuilt with the new data Fixes https://gitlab.com/kicad/code/kicad/-/issues/17851 (cherry picked from commit 315ad0e071ea0ff638051d44b437cb87bb2ef933) --- eeschema/connection_graph.cpp | 88 ++++++++++++++++++++++ eeschema/connection_graph.h | 10 +++ eeschema/dialogs/dialog_change_symbols.cpp | 13 +++- eeschema/sch_edit_frame.cpp | 8 +- include/commit.h | 2 +- 5 files changed, 115 insertions(+), 6 deletions(-) diff --git a/eeschema/connection_graph.cpp b/eeschema/connection_graph.cpp index f992498c4f..a21db33bdd 100644 --- a/eeschema/connection_graph.cpp +++ b/eeschema/connection_graph.cpp @@ -85,6 +85,47 @@ void CONNECTION_SUBGRAPH::RemoveItem( SCH_ITEM* aItem ) } +void CONNECTION_SUBGRAPH::ExchangeItem( SCH_ITEM* aOldItem, SCH_ITEM* aNewItem ) +{ + m_items.erase( aOldItem ); + m_items.insert( aNewItem ); + + m_drivers.erase( aOldItem ); + m_drivers.insert( aNewItem ); + + if( aOldItem == m_driver ) + { + m_driver = aNewItem; + m_driver_connection = aNewItem->GetOrInitConnection( m_sheet, m_graph ); + } + + SCH_CONNECTION* old_conn = aOldItem->Connection( &m_sheet ); + SCH_CONNECTION* new_conn = aNewItem->GetOrInitConnection( m_sheet, m_graph ); + + if( old_conn && new_conn ) + { + new_conn->Clone( *old_conn ); + + if( old_conn->IsDriver() ) + new_conn->SetDriver( aNewItem ); + + new_conn->ClearDirty(); + } + + if( aOldItem->Type() == SCH_SHEET_PIN_T ) + { + m_hier_pins.erase( static_cast( aOldItem ) ); + m_hier_pins.insert( static_cast( aNewItem ) ); + } + + if( aOldItem->Type() == SCH_HIER_LABEL_T ) + { + m_hier_ports.erase( static_cast( aOldItem ) ); + m_hier_ports.insert( static_cast( aNewItem ) ); + } +} + + bool CONNECTION_SUBGRAPH::ResolveDrivers( bool aCheckMultipleDrivers ) { std::lock_guard lock( m_driver_mutex ); @@ -576,6 +617,53 @@ void CONNECTION_GRAPH::Merge( CONNECTION_GRAPH& aGraph ) } +void CONNECTION_GRAPH::ExchangeItem( SCH_ITEM* aOldItem, SCH_ITEM* aNewItem ) +{ + wxCHECK2( aOldItem->Type() == aNewItem->Type(), return ); + + auto exchange = [&]( SCH_ITEM* aOld, SCH_ITEM* aNew ) + { + auto it = m_item_to_subgraph_map.find( aOld ); + + if( it == m_item_to_subgraph_map.end() ) + return; + + CONNECTION_SUBGRAPH* sg = it->second; + + sg->ExchangeItem( aOld, aNew ); + + m_item_to_subgraph_map.erase( it ); + m_item_to_subgraph_map.emplace( aNew, sg ); + + for( auto it2 = m_items.begin(); it2 != m_items.end(); ++it2 ) + { + if( *it2 == aOld ) + { + *it2 = aNew; + break; + } + } + }; + + exchange( aOldItem, aNewItem ); + + if( aOldItem->Type() == SCH_SYMBOL_T ) + { + SCH_SYMBOL* oldSymbol = static_cast( aOldItem ); + SCH_SYMBOL* newSymbol = static_cast( aNewItem ); + std::vector oldPins = oldSymbol->GetPins( &m_schematic->CurrentSheet() ); + std::vector newPins = newSymbol->GetPins( &m_schematic->CurrentSheet() ); + + wxCHECK2( oldPins.size() == newPins.size(), return ); + + for( size_t ii = 0; ii < oldPins.size(); ii++ ) + { + exchange( oldPins[ii], newPins[ii] ); + } + } +} + + void CONNECTION_GRAPH::Reset() { for( auto& subgraph : m_subgraphs ) diff --git a/eeschema/connection_graph.h b/eeschema/connection_graph.h index 884f09b1b4..d930ab819b 100644 --- a/eeschema/connection_graph.h +++ b/eeschema/connection_graph.h @@ -199,6 +199,11 @@ public: void RemoveItem( SCH_ITEM* aItem ); + /** + * Replaces all references to #aOldItem with #aNewItem in the subgraph. + */ + void ExchangeItem( SCH_ITEM* aOldItem, SCH_ITEM* aNewItem ); + // Use this to keep a connection pointer that is not owned by any item // This will be destroyed with the subgraph void StoreImplicitConnection( SCH_CONNECTION* aConnection ) @@ -456,6 +461,11 @@ public: void RemoveItem( SCH_ITEM* aItem ); + /** + * Replace all references to #aOldItem with #aNewItem in the graph. + */ + void ExchangeItem( SCH_ITEM* aOldItem, SCH_ITEM* aNewItem ); + private: /** * Update the graphical connectivity between items (i.e. where they touch) diff --git a/eeschema/dialogs/dialog_change_symbols.cpp b/eeschema/dialogs/dialog_change_symbols.cpp index c66f8ea0ba..f40151d38b 100644 --- a/eeschema/dialogs/dialog_change_symbols.cpp +++ b/eeschema/dialogs/dialog_change_symbols.cpp @@ -23,6 +23,7 @@ #include #include +#include #include // WildCompareString #include #include @@ -602,7 +603,17 @@ int DIALOG_CHANGE_SYMBOLS::processSymbols( SCH_COMMIT* aCommit, wxCHECK( screen, 0 ); screen->Remove( symbol ); - aCommit->Modify( symbol, screen ); + SCH_SYMBOL* symbol_copy = static_cast( symbol->Clone() ); + aCommit->Modified( symbol, symbol_copy, screen ); + + CONNECTION_GRAPH* connectionGraph = screen->Schematic()->ConnectionGraph(); + + // When we replace the lib symbol below, we free the associated pins if the new symbol has + // fewer than the original. This will cause the connection graph to be out of date unless + // we replace references in the graph to the old symbol/pins with references to the ones stored + // in the undo stack. + if( connectionGraph ) + connectionGraph->ExchangeItem( symbol, symbol_copy ); } for( const auto& [ symbol, symbol_change_info ] : symbols ) diff --git a/eeschema/sch_edit_frame.cpp b/eeschema/sch_edit_frame.cpp index ba5148430e..9f25b6629c 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -1718,7 +1718,7 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_COMMIT* aCommit, SCH_CLEANUP_FL { PICKED_ITEMS_LIST* changed_list = m_undoList.m_CommandsList.back(); std::set changed_items; - std::vector pts; + std::set pts; std::set> item_paths; for( unsigned ii = 0; ii < changed_list->GetCount(); ++ii ) @@ -1733,7 +1733,7 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_COMMIT* aCommit, SCH_CLEANUP_FL SCH_SHEET_PATHS& paths = screen->GetClientSheetPaths(); std::vector tmp_pts = item->GetConnectionPoints(); - pts.insert( pts.end(), tmp_pts.begin(), tmp_pts.end() ); + pts.insert( tmp_pts.begin(), tmp_pts.end() ); changed_items.insert( item ); for( SCH_SHEET_PATH& path : paths ) @@ -1745,7 +1745,7 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_COMMIT* aCommit, SCH_CLEANUP_FL continue; tmp_pts = item->GetConnectionPoints(); - pts.insert( pts.end(), tmp_pts.begin(), tmp_pts.end() ); + pts.insert( tmp_pts.begin(), tmp_pts.end() ); changed_items.insert( item ); // We have to directly add the pins here because the link may not exist on the schematic @@ -1760,7 +1760,7 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_COMMIT* aCommit, SCH_CLEANUP_FL item_paths.insert( std::make_pair( path, item ) ); } - for( VECTOR2I& pt: pts ) + for( const VECTOR2I& pt: pts ) { for( SCH_ITEM* item : GetScreen()->Items().Overlapping( pt ) ) { diff --git a/include/commit.h b/include/commit.h index 3e5d085114..c67204dae4 100644 --- a/include/commit.h +++ b/include/commit.h @@ -111,7 +111,7 @@ public: ///< before the modification. COMMIT& Modified( EDA_ITEM* aItem, EDA_ITEM* aCopy, BASE_SCREEN *aScreen = nullptr ) { - return createModified( aItem, aCopy ); + return createModified( aItem, aCopy, 0, aScreen ); } template