diff --git a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp index 6b8d664481..9417a49451 100644 --- a/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp +++ b/eeschema/sch_io/kicad_sexpr/sch_io_kicad_sexpr.cpp @@ -811,12 +811,27 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche if( !aSymbol->GetInstances().empty() ) { std::map> projectInstances; + std::set currentProjectKeys; m_out->Print( "(instances" ); wxString projectName; KIID rootSheetUuid = aSchematic.Root().m_Uuid; + // Collect top-level sheet UUIDs to identify current project instances. + // When root is virtual (niluuid), Path() skips it, so instance paths + // start with the real top-level sheet UUID, not niluuid. + + if( rootSheetUuid == niluuid ) + { + for( const SCH_SHEET* sheet : aSchematic.GetTopLevelSheets() ) + currentProjectKeys.insert( sheet->m_Uuid ); + } + else + { + currentProjectKeys.insert( rootSheetUuid ); + } + for( const SCH_SYMBOL_INSTANCE& inst : aSymbol->GetInstances() ) { // Zero length KIID_PATH objects are not valid and will cause a crash below. @@ -830,7 +845,6 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche // paths may include the virtual root, but SCH_SHEET_PATH::Path() skips it. We need // to normalize the path by removing the virtual root before comparison. KIID_PATH pathToCheck = inst.m_Path; - bool hasVirtualRoot = false; // If root is virtual (niluuid) and path starts with virtual root, strip it if( rootSheetUuid == niluuid && !pathToCheck.empty() && pathToCheck[0] == niluuid ) @@ -838,7 +852,6 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche if( pathToCheck.size() > 1 ) { pathToCheck.erase( pathToCheck.begin() ); - hasVirtualRoot = true; } else { @@ -850,7 +863,8 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche // Check if this instance is orphaned (no matching sheet path) // For virtual root, we check if the first real sheet matches one of the top-level sheets // For non-virtual root, we check if it matches the root sheet UUID - bool belongsToThisProject = hasVirtualRoot || pathToCheck[0] == rootSheetUuid; + bool belongsToThisProject = currentProjectKeys.count( pathToCheck[0] ); + bool isOrphaned = belongsToThisProject && !aSheetList.GetSheetPathByKIIDPath( pathToCheck ); // Keep all instance data when copying to the clipboard. They may be needed on paste. @@ -878,7 +892,10 @@ void SCH_IO_KICAD_SEXPR::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSche return aLhs.m_Path < aRhs.m_Path; } ); - projectName = instances[0].m_ProjectName; + if( currentProjectKeys.count( uuid ) ) + projectName = m_schematic->Project().GetProjectName(); + else + projectName = instances[0].m_ProjectName; m_out->Print( "(project %s", m_out->Quotew( projectName ).c_str() ); @@ -1096,6 +1113,18 @@ void SCH_IO_KICAD_SEXPR::saveSheet( SCH_SHEET* aSheet, const SCH_SHEET_LIST& aSh KIID rootSheetUuid = m_schematic->Root().m_Uuid; bool inProjectClause = false; + std::set currentProjectKeys; + + if( rootSheetUuid == niluuid ) + { + for( const SCH_SHEET* sheet : m_schematic->GetTopLevelSheets() ) + currentProjectKeys.insert( sheet->m_Uuid ); + } + else + { + currentProjectKeys.insert( rootSheetUuid ); + } + for( size_t i = 0; i < sheetInstances.size(); i++ ) { // If the instance data is part of this design but no longer has an associated sheet @@ -1103,8 +1132,10 @@ void SCH_IO_KICAD_SEXPR::saveSheet( SCH_SHEET* aSheet, const SCH_SHEET_LIST& aSh // current project from accumulating in the schematic files. // // Keep all instance data when copying to the clipboard. It may be needed on paste. - if( ( sheetInstances[i].m_Path[0] == rootSheetUuid ) - && !aSheetList.GetSheetPathByKIIDPath( sheetInstances[i].m_Path, false ) ) + bool belongsToThisProject = + !sheetInstances[i].m_Path.empty() && currentProjectKeys.count( sheetInstances[i].m_Path[0] ); + + if( belongsToThisProject && !aSheetList.GetSheetPathByKIIDPath( sheetInstances[i].m_Path, false ) ) { if( inProjectClause && ( ( i + 1 == sheetInstances.size() ) || lastProjectUuid != sheetInstances[i+1].m_Path[0] ) ) @@ -1120,7 +1151,7 @@ void SCH_IO_KICAD_SEXPR::saveSheet( SCH_SHEET* aSheet, const SCH_SHEET_LIST& aSh { wxString projectName; - if( sheetInstances[i].m_Path[0] == rootSheetUuid ) + if( belongsToThisProject ) projectName = m_schematic->Project().GetProjectName(); else projectName = sheetInstances[i].m_ProjectName; diff --git a/qa/tests/eeschema/CMakeLists.txt b/qa/tests/eeschema/CMakeLists.txt index 43b451030f..ac1432c6f9 100644 --- a/qa/tests/eeschema/CMakeLists.txt +++ b/qa/tests/eeschema/CMakeLists.txt @@ -124,6 +124,7 @@ set( QA_EESCHEMA_SRCS test_flat_hierarchy.cpp test_multi_top_level_sheets.cpp test_save_load_schematic.cpp + test_project_name_instances.cpp test_legacy_load.cpp test_symbol_library_manager.cpp test_symbol_import_manager.cpp diff --git a/qa/tests/eeschema/test_project_name_instances.cpp b/qa/tests/eeschema/test_project_name_instances.cpp new file mode 100644 index 0000000000..07d8bc14fa --- /dev/null +++ b/qa/tests/eeschema/test_project_name_instances.cpp @@ -0,0 +1,165 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The 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 + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one here: + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html + * or you may search the http://www.gnu.org website for the version 2 license, + * or you may write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/** + * @file test_project_name_instances.cpp + * Test that symbol and sheet instances are saved with the correct project name, + * not an empty string. Regression test for the virtual root project name bug. + */ + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + + +struct PROJECT_NAME_FIXTURE +{ + PROJECT_NAME_FIXTURE() : + m_settingsManager() + { + wxString tempDir = wxStandardPaths::Get().GetTempDir(); + wxString projectPath = tempDir + wxFileName::GetPathSeparator() + wxT( "test_project.kicad_pro" ); + m_tempFiles.push_back( projectPath ); + + m_settingsManager.LoadProject( projectPath.ToStdString() ); + m_schematic = std::make_unique( nullptr ); + m_project = &m_settingsManager.Prj(); + m_schematic->SetProject( m_project ); + } + + ~PROJECT_NAME_FIXTURE() + { + for( const wxString& file : m_tempFiles ) + { + if( wxFileExists( file ) ) + wxRemoveFile( file ); + } + + m_schematic.reset(); + } + + wxString GetTempSchFile( const wxString& aPrefix ) + { + wxString tempDir = wxStandardPaths::Get().GetTempDir(); + wxString fileName = wxFileName::CreateTempFileName( tempDir + wxFileName::GetPathSeparator() + aPrefix ); + fileName += wxT( ".kicad_sch" ); + m_tempFiles.push_back( fileName ); + return fileName; + } + + SETTINGS_MANAGER m_settingsManager; + std::unique_ptr m_schematic; + PROJECT* m_project; + std::vector m_tempFiles; +}; + + +BOOST_FIXTURE_TEST_SUITE( ProjectNameInstances, PROJECT_NAME_FIXTURE ) + + +/** + * Test that symbol instances are saved with the current project name, + * not an empty string, when using the virtual root pattern. + */ +BOOST_AUTO_TEST_CASE( SymbolInstanceProjectName ) +{ + LOCALE_IO dummy; + + m_schematic->CreateDefaultScreens(); + + std::vector topSheets = m_schematic->GetTopLevelSheets(); + BOOST_REQUIRE( !topSheets.empty() ); + + SCH_SCREEN* screen = topSheets[0]->GetScreen(); + BOOST_REQUIRE( screen != nullptr ); + + // Create a symbol + SCH_SYMBOL* symbol = new SCH_SYMBOL(); + symbol->SetLibId( LIB_ID( wxT( "Device" ), wxT( "R" ) ) ); + symbol->SetPosition( VECTOR2I( 0, 0 ) ); + symbol->GetField( FIELD_T::REFERENCE )->SetText( wxT( "R1" ) ); + symbol->SetPrefix( wxT( "R" ) ); + screen->Append( symbol ); + + // Build hierarchy and set the reference (this creates an instance + // with empty m_ProjectName, simulating the bug condition) + m_schematic->RefreshHierarchy(); + SCH_SHEET_LIST hierarchy = m_schematic->Hierarchy(); + BOOST_REQUIRE( !hierarchy.empty() ); + + SCH_SHEET_PATH& path = hierarchy[0]; + symbol->SetRef( &path, wxT( "R1" ) ); + + // Verify the instance exists but has empty project name (the bug) + SCH_SYMBOL_INSTANCE inst; + BOOST_REQUIRE( symbol->GetInstance( inst, path.Path() ) ); + BOOST_CHECK( inst.m_ProjectName.IsEmpty() ); + + // Save the schematic + wxString fileName = GetTempSchFile( wxT( "test_projname" ) ); + + SCH_IO_KICAD_SEXPR io; + io.SaveSchematicFile( fileName, topSheets[0], m_schematic.get() ); + BOOST_REQUIRE( wxFileExists( fileName ) ); + + BOOST_TEST_MESSAGE( "Saved file: " + fileName.ToStdString() ); + BOOST_TEST_MESSAGE( "Project name: " + m_project->GetProjectName().ToStdString() ); + + // Read the saved file as text and check for project name + wxTextFile textFile; + BOOST_REQUIRE( textFile.Open( fileName ) ); + + bool foundProjectClause = false; + bool foundEmptyProject = false; + + for( size_t i = 0; i < textFile.GetLineCount(); i++ ) + { + wxString line = textFile.GetLine( i ); + + if( line.Contains( wxT( "(project \"test_project\"" ) ) ) + foundProjectClause = true; + + if( line.Contains( wxT( "(project \"\"" ) ) ) + foundEmptyProject = true; + } + + textFile.Close(); + + BOOST_CHECK_MESSAGE( foundProjectClause, "Saved file must contain (project \"test_project\")" ); + BOOST_CHECK_MESSAGE( !foundEmptyProject, "Saved file must NOT contain (project \"\")" ); +} + + +BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file