From 93239516d9cb3026277f60dc22ede387c7a40650 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Fri, 30 Sep 2022 17:16:24 -0700 Subject: [PATCH] Remove ID from property fields ID was not maintained or used other than to ensure unique fields. Instead of saving, we assign the known IDs to specific field names and sequentially create new IDs on load Fixes https://gitlab.com/kicad/code/kicad/issues/12390 --- common/template_fieldnames.cpp | 1 + eeschema/sch_file_versions.h | 7 +-- .../kicad/sch_sexpr_lib_plugin_cache.cpp | 3 +- .../sch_plugins/kicad/sch_sexpr_parser.cpp | 51 ++++++++++++++++--- .../sch_plugins/kicad/sch_sexpr_plugin.cpp | 4 +- eeschema/sch_sheet.cpp | 5 +- 6 files changed, 55 insertions(+), 16 deletions(-) diff --git a/common/template_fieldnames.cpp b/common/template_fieldnames.cpp index c4cee8eaa9..3511f02ded 100644 --- a/common/template_fieldnames.cpp +++ b/common/template_fieldnames.cpp @@ -30,6 +30,7 @@ using namespace TFIELD_T; +// N.B. Do not change these values without transitioning the file format #define REFERENCE_CANONICAL "Reference" #define VALUE_CANONICAL "Value" #define FOOTPRINT_CANONICAL "Footprint" diff --git a/eeschema/sch_file_versions.h b/eeschema/sch_file_versions.h index 3455bc6931..6177a9ba04 100644 --- a/eeschema/sch_file_versions.h +++ b/eeschema/sch_file_versions.h @@ -46,8 +46,8 @@ //#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220126 // Text boxes. //#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220328 // Text box start/end -> at/size. //#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220331 // Text colors. -#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220914 // Symbol unit display names. - +//#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220914 // Symbol unit display names. +#define SEXPR_SYMBOL_LIB_FILE_VERSION 20220914 // Don't save property ID /** * Schematic file version. @@ -87,4 +87,5 @@ //#define SEXPR_SCHEMATIC_FILE_VERSION 20220822 // Hyperlinks in text objects //#define SEXPR_SCHEMATIC_FILE_VERSION 20220903 // Field name visibility //#define SEXPR_SCHEMATIC_FILE_VERSION 20220904 // Do not autoplace field option -#define SEXPR_SCHEMATIC_FILE_VERSION 20220914 // Add support for DNP +//#define SEXPR_SCHEMATIC_FILE_VERSION 20220914 // Add support for DNP +#define SEXPR_SCHEMATIC_FILE_VERSION 20220929 // Don't save property ID diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_lib_plugin_cache.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_lib_plugin_cache.cpp index 5a69116871..f7063c9110 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_lib_plugin_cache.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_lib_plugin_cache.cpp @@ -398,10 +398,9 @@ void SCH_SEXPR_PLUGIN_CACHE::saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFor if( aField->GetId() >= 0 && aField->GetId() < MANDATORY_FIELDS ) fieldName = TEMPLATE_FIELDNAME::GetDefaultFieldName( aField->GetId(), false ); - aFormatter.Print( aNestLevel, "(property %s %s (id %d) (at %s %s %g)", + aFormatter.Print( aNestLevel, "(property %s %s (at %s %s %g)", aFormatter.Quotew( fieldName ).c_str(), aFormatter.Quotew( aField->GetText() ).c_str(), - aField->GetId(), EDA_UNIT_UTILS::FormatInternalUnits( schIUScale, aField->GetPosition().x ).c_str(), EDA_UNIT_UTILS::FormatInternalUnits( schIUScale, aField->GetPosition().y ).c_str(), aField->GetTextAngle().AsDegrees() ); diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp index 3636038436..26a9615258 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp @@ -770,6 +770,18 @@ LIB_FIELD* SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol } field->SetName( name ); + + // Correctly set the ID based on canonical (untranslated) field name + // If ID is stored in the file (old versions), it will overwrite this + for( int ii = 0; ii < MANDATORY_FIELDS; ++ii ) + { + if( !name.CmpNoCase( TEMPLATE_FIELDNAME::GetDefaultFieldName( ii ) ) ) + { + field->SetId( ii ); + break; + } + } + token = NextTok(); if( !IsSymbol( token ) ) @@ -1900,6 +1912,31 @@ SCH_FIELD* SCH_SEXPR_PARSER::parseSchField( SCH_ITEM* aParent ) field->SetText( value ); field->SetVisible( true ); + // Correctly set the ID based on canonical (untranslated) field name + // If ID is stored in the file (old versions), it will overwrite this + if( aParent->Type() == SCH_SYMBOL_T ) + { + for( int ii = 0; ii < MANDATORY_FIELDS; ++ii ) + { + if( name == TEMPLATE_FIELDNAME::GetDefaultFieldName( ii, false ) ) + { + field->SetId( ii ); + break; + } + } + } + else if( aParent->Type() == SCH_SHEET_T ) + { + for( int ii = 0; ii < SHEET_MANDATORY_FIELDS; ++ii ) + { + if( name == SCH_SHEET::GetDefaultFieldName( ii, false ) ) + { + field->SetId( ii ); + break; + } + } + } + for( token = NextTok(); token != T_RIGHT; token = NextTok() ) { if( token != T_LEFT ) @@ -2878,12 +2915,14 @@ SCH_SHEET* SCH_SEXPR_PARSER::parseSheet() // complete) solution is to disallow multiple instances of the same id (for all // files since the source of the error *might* in fact be hand-edited files). // - // While no longer used, -1 is still a valid id for user field. It gets converted - // to the next unused number on save. - if( fieldIDsRead.count( field->GetId() ) ) - field->SetId( -1 ); - else - fieldIDsRead.insert( field->GetId() ); + // While no longer used, -1 is still a valid id for user field. We convert it to + // the first available ID after the mandatory fields + + if( field->GetId() < 0 ) + field->SetId( SHEET_MANDATORY_FIELDS ); + + while( !fieldIDsRead.insert( field->GetId() ).second ) + field->SetId( field->GetId() + 1 ); fields.emplace_back( *field ); delete field; diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp index 01048769d5..873fb00528 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp @@ -816,7 +816,6 @@ void SCH_SEXPR_PLUGIN::saveField( SCH_FIELD* aField, int aNestLevel ) wxCHECK_RET( aField != nullptr && m_out != nullptr, "" ); wxString fieldName = aField->GetCanonicalName(); - // For some reason (bug in legacy parser?) the field ID for non-mandatory fields is -1 so // check for this in order to correctly use the field name. @@ -830,10 +829,9 @@ void SCH_SEXPR_PLUGIN::saveField( SCH_FIELD* aField, int aNestLevel ) m_nextFreeFieldId = aField->GetId() + 1; } - m_out->Print( aNestLevel, "(property %s %s (id %d) (at %s %s %s)", + m_out->Print( aNestLevel, "(property %s %s (at %s %s %s)", m_out->Quotew( fieldName ).c_str(), m_out->Quotew( aField->GetText() ).c_str(), - aField->GetId(), EDA_UNIT_UTILS::FormatInternalUnits( schIUScale, aField->GetPosition().x ).c_str(), EDA_UNIT_UTILS::FormatInternalUnits( schIUScale, aField->GetPosition().y ).c_str(), EDA_UNIT_UTILS::FormatAngle( aField->GetTextAngle() ).c_str() ); diff --git a/eeschema/sch_sheet.cpp b/eeschema/sch_sheet.cpp index a343e5a187..c3b1d819b3 100644 --- a/eeschema/sch_sheet.cpp +++ b/eeschema/sch_sheet.cpp @@ -46,8 +46,9 @@ #include #include -#define SHEET_NAME_CANONICAL "Sheet name" -#define SHEET_FILE_CANONICAL "Sheet file" +// N.B. Do not change these values without transitioning the file format +#define SHEET_NAME_CANONICAL "Sheetname" +#define SHEET_FILE_CANONICAL "Sheetfile" #define USER_FIELD_CANONICAL "Field%d"