From 315ad0e071ea0ff638051d44b437cb87bb2ef933 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 --- 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 13c292dd37..2e63d19e81 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 ); @@ -580,6 +621,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 8c6359871b..91975ecc85 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 8f8b7df973..d600f854c0 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 d2f3ff89c4..e126eccccf 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -1769,7 +1769,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 ) @@ -1784,7 +1784,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 ) @@ -1796,7 +1796,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 @@ -1811,7 +1811,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