From 148467d2a1194d8b39bb5fac57b36eeb904201db Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Tue, 11 Oct 2022 16:16:11 -0400 Subject: [PATCH] Fix schematic symbol rescue issues. This fix makes some fundamental changes to the way symbols are rescued: * The new symbol library file format is used when rescuing symbols defined in the symbol library table. * The original library nickname is appended to the symbol name rather than the rescue library nickname when rescuing symbol defined in the symbol library table. * Escaping strings has been removed from legacy library rescues since the legacy library file format never supported it. Fixes https://gitlab.com/kicad/code/kicad/-/issues/12525 Fixes https://gitlab.com/kicad/code/kicad/-/issues/12624 --- eeschema/project_rescue.cpp | 136 +++++++++--------- eeschema/project_rescue.h | 6 +- .../legacy/sch_legacy_lib_plugin_cache.cpp | 9 +- eeschema/sch_screen.cpp | 2 +- 4 files changed, 80 insertions(+), 73 deletions(-) diff --git a/eeschema/project_rescue.cpp b/eeschema/project_rescue.cpp index b4f351067a..b39c914686 100644 --- a/eeschema/project_rescue.cpp +++ b/eeschema/project_rescue.cpp @@ -150,7 +150,6 @@ void RESCUE_CASE_CANDIDATE::FindRescues( RESCUER& aRescuer, for( SCH_SYMBOL* eachSymbol : *( aRescuer.GetSymbols() ) ) { symbol_name = eachSymbol->GetLibId().GetLibItemName(); - search_name = EscapeString ( symbol_name, CTX_LIBID ); if( last_symbol_name != symbol_name ) { @@ -159,25 +158,28 @@ void RESCUE_CASE_CANDIDATE::FindRescues( RESCUER& aRescuer, last_symbol_name = symbol_name; case_insensitive_matches.clear(); - LIB_ID id( wxEmptyString, search_name ); + LIB_ID id( wxEmptyString, symbol_name ); case_sensitive_match = aRescuer.GetPrj()->SchLibs()->FindLibSymbol( id ); + if( case_sensitive_match ) + continue; + // If the case sensitive match failed, try a case insensitive match. - if( !case_sensitive_match ) - aRescuer.GetPrj()->SchLibs()->FindLibraryNearEntries( case_insensitive_matches, - search_name ); + aRescuer.GetPrj()->SchLibs()->FindLibraryNearEntries( case_insensitive_matches, + search_name ); + + // If there are not case insensitive matches either, the symbol cannot be rescued. + if( !case_insensitive_matches.size() ) + continue; + + RESCUE_CASE_CANDIDATE candidate( symbol_name, case_insensitive_matches[0]->GetName(), + case_insensitive_matches[0], + eachSymbol->GetUnit(), + eachSymbol->GetConvert() ); + + candidate_map[symbol_name] = candidate; } - - if( case_sensitive_match || !( case_insensitive_matches.size() ) ) - continue; - - RESCUE_CASE_CANDIDATE candidate( symbol_name, case_insensitive_matches[0]->GetName(), - case_insensitive_matches[0], - eachSymbol->GetUnit(), - eachSymbol->GetConvert() ); - - candidate_map[symbol_name] = candidate; } // Now, dump the map into aCandidates @@ -249,24 +251,31 @@ void RESCUE_CACHE_CANDIDATE::FindRescues( RESCUER& aRescuer, LIB_SYMBOL* cache_match = nullptr; LIB_SYMBOL* lib_match = nullptr; wxString symbol_name; - wxString search_name; wxString old_symbol_name; for( SCH_SYMBOL* eachSymbol : *( aRescuer.GetSymbols() ) ) { - symbol_name = eachSymbol->GetLibId().GetLibItemName(); - search_name = EscapeString ( symbol_name, CTX_LIBID ); + symbol_name = eachSymbol->GetLibId().GetUniStringLibItemName(); if( old_symbol_name != symbol_name ) { // A new symbol name is found (a new group starts here). // Search the symbol names candidates only once for this group: old_symbol_name = symbol_name; - cache_match = findSymbol( search_name, aRescuer.GetPrj()->SchLibs(), true ); - lib_match = findSymbol( search_name, aRescuer.GetPrj()->SchLibs(), false ); + cache_match = findSymbol( symbol_name, aRescuer.GetPrj()->SchLibs(), true ); + lib_match = findSymbol( symbol_name, aRescuer.GetPrj()->SchLibs(), false ); - if( !cache_match && !lib_match ) - continue; + // At some point during V5 development, the LIB_ID delimiter character ':' was + // replaced by '_' when writing the symbol cache library so we have to test for + // the LIB_NICKNAME_LIB_SYMBOL_NAME case. + if( !cache_match && eachSymbol->GetLibId().IsValid() ) + { + wxString tmp; + + tmp = eachSymbol->GetLibId().GetLibNickname().wx_str() + wxT( "_" ) + + eachSymbol->GetLibId().GetLibItemName().wx_str(); + cache_match = findSymbol( tmp, aRescuer.GetPrj()->SchLibs(), true ); + } // Test whether there is a conflict or if the symbol can only be found in the cache // and the symbol name does not have any illegal characters. @@ -278,7 +287,7 @@ void RESCUE_CACHE_CANDIDATE::FindRescues( RESCUER& aRescuer, continue; // Check if the symbol has already been rescued. - RESCUE_CACHE_CANDIDATE candidate( symbol_name, search_name, cache_match, lib_match, + RESCUE_CACHE_CANDIDATE candidate( symbol_name, symbol_name, cache_match, lib_match, eachSymbol->GetUnit(), eachSymbol->GetConvert() ); @@ -378,7 +387,7 @@ void RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::FindRescues( LIB_SYMBOL* lib_match = nullptr; LIB_ID old_symbol_id; - wxString escapedSymbolName; + wxString symbolName; for( SCH_SYMBOL* eachSymbol : *( aRescuer.GetSymbols() ) ) { @@ -390,23 +399,20 @@ void RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::FindRescues( // Search the symbol names candidates only once for this group: old_symbol_id = symbol_id; - escapedSymbolName = EscapeString( symbol_id.Format().wx_str(), CTX_LIBID ); + symbolName = symbol_id.Format().wx_str(); // Get the library symbol from the cache library. It will be a flattened // symbol by default (no inheritance). - cache_match = findSymbol( escapedSymbolName, aRescuer.GetPrj()->SchLibs(), - true ); + cache_match = findSymbol( symbolName, aRescuer.GetPrj()->SchLibs(), true ); // At some point during V5 development, the LIB_ID delimiter character ':' was // replaced by '_' when writing the symbol cache library so we have to test for // the LIB_NICKNAME_LIB_SYMBOL_NAME case. if( !cache_match ) { - escapedSymbolName = EscapeString( symbol_id.GetLibNickname().wx_str() + - wxT( "_" ) + symbol_id.GetLibItemName().wx_str(), - CTX_LIBID ); - cache_match = findSymbol( escapedSymbolName, aRescuer.GetPrj()->SchLibs(), - true ); + symbolName = symbol_id.GetLibNickname().wx_str() + wxT( "_" ) + + symbol_id.GetLibItemName().wx_str(); + cache_match = findSymbol( symbolName, aRescuer.GetPrj()->SchLibs(), true ); } // Get the library symbol from the symbol library table. @@ -442,15 +448,13 @@ void RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::FindRescues( } // Fix illegal LIB_ID name characters. - wxString new_name = LIB_ID::FixIllegalChars( symbol_id.GetLibItemName(), false ); + wxString new_name = EscapeString( symbol_id.GetLibItemName(), CTX_LIBID ); - // Differentiate symbol name in the rescue library by appending the symbol library - // table nickname to the symbol name to prevent name clashes in the rescue library. + // Differentiate symbol name in the rescue library by appending the original symbol + // library table nickname to the symbol name to prevent name clashes in the rescue + // library. wxString libNickname = GetRescueLibraryFileName( aRescuer.Schematic() ).GetName(); - // Spaces in the file name will break the symbol name because they are not - // quoted in the symbol library file format. - libNickname.Replace( " ", "-" ); LIB_ID new_id( libNickname, new_name + "-" + symbol_id.GetLibNickname().wx_str() ); RESCUE_SYMBOL_LIB_TABLE_CANDIDATE candidate( symbol_id, new_id, cache_match, lib_match, @@ -477,19 +481,19 @@ wxString RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::GetActionDescription() const { action.Printf( _( "Cannot rescue symbol %s which is not available in any library or " "the cache." ), - m_requested_id.GetLibItemName().wx_str() ); + UnescapeString( m_requested_id.GetLibItemName().wx_str() ) ); } else if( m_cache_candidate && !m_lib_candidate ) { action.Printf( _( "Rescue symbol %s found only in cache library to %s." ), - m_requested_id.Format().wx_str(), - m_new_id.Format().wx_str() ); + UnescapeString( m_requested_id.Format().wx_str() ), + UnescapeString( m_new_id.Format().wx_str() ) ); } else { action.Printf( _( "Rescue modified symbol %s to %s" ), - m_requested_id.Format().wx_str(), - m_new_id.Format().wx_str() ); + UnescapeString( m_requested_id.Format().wx_str() ), + UnescapeString( m_new_id.Format().wx_str() ) ); } return action; @@ -808,15 +812,19 @@ void SYMBOL_LIB_TABLE_RESCUER::InvokeDialog( wxWindow* aParent, bool aAskShowAga void SYMBOL_LIB_TABLE_RESCUER::OpenRescueLibrary() { - m_pi.set( SCH_IO_MGR::FindPlugin( SCH_IO_MGR::SCH_LEGACY ) ); (*m_properties)[ SCH_LEGACY_PLUGIN::PropBuffering ] = ""; wxFileName fn = GetRescueLibraryFileName( m_schematic ); + SYMBOL_LIB_TABLE_ROW* row = m_prj->SchSymbolLibTable()->FindRow( fn.GetName() ); + // If a rescue library already exists copy the contents of that library so we do not // lose any previous rescues. - if( m_prj->SchSymbolLibTable()->HasLibrary( fn.GetName() ) ) + if( row ) { + if( SCH_IO_MGR::EnumFromStr( row->GetType() ) == SCH_IO_MGR::SCH_KICAD ) + fn.SetExt( KiCadSymbolLibFileExtension ); + std::vector symbols; try @@ -829,7 +837,7 @@ void SYMBOL_LIB_TABLE_RESCUER::OpenRescueLibrary() } for( LIB_SYMBOL* symbol : symbols ) - AddSymbol( symbol ); + m_rescueLibSymbols.emplace_back( std::make_unique( *symbol ) ); } } @@ -838,10 +846,19 @@ bool SYMBOL_LIB_TABLE_RESCUER::WriteRescueLibrary( wxWindow *aParent ) { wxString msg; wxFileName fn = GetRescueLibraryFileName( m_schematic ); + SYMBOL_LIB_TABLE_ROW* row = m_prj->SchSymbolLibTable()->FindRow( fn.GetName() ); + + fn.SetExt( KiCadSymbolLibFileExtension ); + SCH_PLUGIN::SCH_PLUGIN_RELEASER pi; + + pi.set( SCH_IO_MGR::FindPlugin( SCH_IO_MGR::SCH_KICAD ) ); try { - m_pi->SaveLibrary( fn.GetFullPath() ); + for( const std::unique_ptr& symbol : m_rescueLibSymbols ) + pi->SaveSymbol( fn.GetFullPath(), new LIB_SYMBOL( *symbol.get() ), m_properties.get() ); + + pi->SaveLibrary( fn.GetFullPath() ); } catch( const IO_ERROR& ioe ) { @@ -852,17 +869,17 @@ bool SYMBOL_LIB_TABLE_RESCUER::WriteRescueLibrary( wxWindow *aParent ) // If the rescue library already exists in the symbol library table no need save it to add // it to the table. - if( !m_prj->SchSymbolLibTable()->HasLibrary( fn.GetName() ) ) + if( !row || ( SCH_IO_MGR::EnumFromStr( row->GetType() ) == SCH_IO_MGR::SCH_LEGACY ) ) { + if( row && SCH_IO_MGR::EnumFromStr( row->GetType() ) == SCH_IO_MGR::SCH_LEGACY ) + { + m_prj->SchSymbolLibTable()->RemoveRow( row ); + } + wxString uri = "${KIPRJMOD}/" + fn.GetFullName(); wxString libNickname = fn.GetName(); - // Spaces in the file name will break the symbol name because they are not - // quoted in the symbol library file format. - libNickname.Replace( " ", "-" ); - - SYMBOL_LIB_TABLE_ROW* row = new SYMBOL_LIB_TABLE_ROW( libNickname, uri, - wxString( "Legacy" ) ); + row = new SYMBOL_LIB_TABLE_ROW( libNickname, uri, wxT( "KiCad" ) ); m_prj->SchSymbolLibTable()->InsertRow( row ); fn = wxFileName( m_prj->GetProjectPath(), SYMBOL_LIB_TABLE::GetSymbolLibTableFileName() ); @@ -879,7 +896,6 @@ bool SYMBOL_LIB_TABLE_RESCUER::WriteRescueLibrary( wxWindow *aParent ) } } - // Relaod the symbol library table. m_prj->SetElem( PROJECT::ELEM_SYMBOL_LIB_TABLE, nullptr ); // This can only happen if the symbol library table file was corrupted on write. @@ -897,13 +913,5 @@ void SYMBOL_LIB_TABLE_RESCUER::AddSymbol( LIB_SYMBOL* aNewSymbol ) { wxCHECK_RET( aNewSymbol, "Invalid LIB_SYMBOL pointer." ); - wxFileName fn = GetRescueLibraryFileName( m_schematic ); - - try - { - m_pi->SaveSymbol( fn.GetFullPath(), new LIB_SYMBOL( *aNewSymbol ), m_properties.get() ); - } - catch( ... /* IO_ERROR */ ) - { - } + m_rescueLibSymbols.emplace_back( std::make_unique( *aNewSymbol ) ); } diff --git a/eeschema/project_rescue.h b/eeschema/project_rescue.h index 3e723df59a..bf496a3777 100644 --- a/eeschema/project_rescue.h +++ b/eeschema/project_rescue.h @@ -2,7 +2,7 @@ * This program source code file is part of KiCad, a free EDA CAD application. * * Copyright (C) 2015 Chris Pavlina - * Copyright (C) 2015-2021 KiCad Developers, see change_log.txt for contributors. + * Copyright (C) 2015-2022 KiCad Developers, see change_log.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 @@ -379,9 +379,9 @@ public: virtual void AddSymbol( LIB_SYMBOL* aNewSymbol ) override; private: - SCH_PLUGIN::SCH_PLUGIN_RELEASER m_pi; + std::vector> m_rescueLibSymbols; - std::unique_ptr< PROPERTIES > m_properties; ///< Library plugin properties + std::unique_ptr< PROPERTIES > m_properties; ///< Library plugin properties. }; #endif // _LIB_CACHE_RESCUE_H_ diff --git a/eeschema/sch_plugins/legacy/sch_legacy_lib_plugin_cache.cpp b/eeschema/sch_plugins/legacy/sch_legacy_lib_plugin_cache.cpp index 0223f4a057..d1690eb7e6 100644 --- a/eeschema/sch_plugins/legacy/sch_legacy_lib_plugin_cache.cpp +++ b/eeschema/sch_plugins/legacy/sch_legacy_lib_plugin_cache.cpp @@ -303,11 +303,10 @@ LIB_SYMBOL* SCH_LEGACY_PLUGIN_CACHE::LoadPart( LINE_READER& aReader, int aMajorV name = tokens.GetNextToken(); - // Don't escape symbol library name if it's already been escaped. Given that the original - // change to escape the symbol library name has resulted in rescue libraries with escaped - // names, we will have to live with the consequences. - if( name == UnescapeString( name ) ) - name = EscapeString( name, CTX_LIBID ); + // This fixes a dubious decision to escape LIB_ID characters. Escaped LIB_IDs broke rescue + // library look up. Legacy LIB_IDs should not be escaped. + if( name != UnescapeString( name ) ) + name = UnescapeString( name ); pos += name.size() + 1; diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index 4ae0d49a1b..0cb53d52c3 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -934,7 +934,7 @@ void SCH_SCREEN::UpdateSymbolLinks( REPORTER* aReporter ) if( !tmp && legacyLibs && legacyLibs->GetLibraryCount() ) { - SYMBOL_LIB& legacyCacheLib = legacyLibs->at( 0 ); + SYMBOL_LIB& legacyCacheLib = legacyLibs->back(); // It better be the cache library. wxCHECK2( legacyCacheLib.IsCache(), continue );