diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp index 8cef609f79..1117238ec0 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp @@ -127,7 +127,9 @@ LIB_PART* SCH_SEXPR_PARSER::ParseSymbol( LIB_PART_MAP& aSymbolLibMap, int aFileV wxString name; wxString error; LIB_ITEM* item; + LIB_FIELD* field; std::unique_ptr symbol = std::make_unique( wxEmptyString ); + std::set fieldIDsRead; m_requiredVersion = aFileVersion; symbol->SetUnitCount( 1 ); @@ -197,7 +199,23 @@ LIB_PART* SCH_SEXPR_PARSER::ParseSymbol( LIB_PART_MAP& aSymbolLibMap, int aFileV break; case T_property: - parseProperty( symbol ); + field = parseProperty( symbol ); + + if( field ) + { + // It would appear that at some point we allowed duplicate ids to slip through + // when writing files. The easiest (and most 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 if( field ) + fieldIDsRead.insert( field->GetId() ); + } + break; case T_extends: @@ -609,28 +627,12 @@ void SCH_SEXPR_PARSER::parseEDA_TEXT( EDA_TEXT* aText ) { switch( token ) { - case T_left: - aText->SetHorizJustify( GR_TEXT_HJUSTIFY_LEFT ); - break; - - case T_right: - aText->SetHorizJustify( GR_TEXT_HJUSTIFY_RIGHT ); - break; - - case T_top: - aText->SetVertJustify( GR_TEXT_VJUSTIFY_TOP ); - break; - - case T_bottom: - aText->SetVertJustify( GR_TEXT_VJUSTIFY_BOTTOM ); - break; - - case T_mirror: - aText->SetMirrored( true ); - break; - - default: - Expecting( "left, right, top, bottom, or mirror" ); + case T_left: aText->SetHorizJustify( GR_TEXT_HJUSTIFY_LEFT ); break; + case T_right: aText->SetHorizJustify( GR_TEXT_HJUSTIFY_RIGHT ); break; + case T_top: aText->SetVertJustify( GR_TEXT_VJUSTIFY_TOP ); break; + case T_bottom: aText->SetVertJustify( GR_TEXT_VJUSTIFY_BOTTOM ); break; + case T_mirror: aText->SetMirrored( true ); break; + default: Expecting( "left, right, top, bottom, or mirror" ); } } @@ -712,20 +714,20 @@ void SCH_SEXPR_PARSER::parsePinNames( std::unique_ptr& aSymbol ) } else if( token != T_RIGHT ) { - error.Printf( - _( "Invalid symbol names definition in\nfile: \"%s\"\nline: %d\noffset: %d" ), - CurSource().c_str(), CurLineNumber(), CurOffset() ); + error.Printf( _( "Invalid symbol names definition in\nfile: '%s'\nline: %d\noffset: %d" ), + CurSource().c_str(), + CurLineNumber(), + CurOffset() ); THROW_IO_ERROR( error ); } } -void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) +LIB_FIELD* SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) { - wxCHECK_RET( CurTok() == T_property, - wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + - wxT( " as a property token." ) ); - wxCHECK( aSymbol, /* void */ ); + wxCHECK_MSG( CurTok() == T_property, nullptr, + wxT( "Cannot parse " ) + GetTokenString( CurTok() ) + wxT( " as a property." ) ); + wxCHECK( aSymbol, nullptr ); wxString error; wxString name; @@ -736,8 +738,10 @@ void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) if( !IsSymbol( token ) ) { - error.Printf( _( "Invalid property name in\nfile: \"%s\"\nline: %d\noffset: %d" ), - CurSource().c_str(), CurLineNumber(), CurOffset() ); + error.Printf( _( "Invalid property name in\nfile: '%s'\nline: %d\noffset: %d" ), + CurSource().c_str(), + CurLineNumber(), + CurOffset() ); THROW_IO_ERROR( error ); } @@ -745,8 +749,10 @@ void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) if( name.IsEmpty() ) { - error.Printf( _( "Empty property name in\nfile: \"%s\"\nline: %d\noffset: %d" ), - CurSource().c_str(), CurLineNumber(), CurOffset() ); + error.Printf( _( "Empty property name in\nfile: '%s'\nline: %d\noffset: %d" ), + CurSource().c_str(), + CurLineNumber(), + CurOffset() ); THROW_IO_ERROR( error ); } @@ -755,8 +761,10 @@ void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) if( !IsSymbol( token ) ) { - error.Printf( _( "Invalid property value in\nfile: \"%s\"\nline: %d\noffset: %d" ), - CurSource().c_str(), CurLineNumber(), CurOffset() ); + error.Printf( _( "Invalid property value in\nfile: '%s'\nline: %d\noffset: %d" ), + CurSource().c_str(), + CurLineNumber(), + CurOffset() ); THROW_IO_ERROR( error ); } @@ -801,16 +809,19 @@ void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) existingField = aSymbol->GetFieldById( field->GetId() ); *existingField = *field; + return existingField; } else if( name == "ki_keywords" ) { // Not a LIB_FIELD object yet. aSymbol->SetKeyWords( value ); + return nullptr; } else if( name == "ki_description" ) { // Not a LIB_FIELD object yet. aSymbol->SetDescription( value ); + return nullptr; } else if( name == "ki_fp_filters" ) { @@ -822,24 +833,28 @@ void SCH_SEXPR_PARSER::parseProperty( std::unique_ptr& aSymbol ) filters.Add( tokenizer.GetNextToken() ); aSymbol->SetFPFilters( filters ); + return nullptr; } else if( name == "ki_locked" ) { // This is a temporary LIB_FIELD object until interchangeable units are determined on // the fly. aSymbol->LockUnits( true ); + return nullptr; } else { - existingField = aSymbol->GetFieldById( field->GetId() ); + existingField = aSymbol->FindField( field->GetCanonicalName() ); if( !existingField ) { - aSymbol->AddDrawItem( field.release(), false ); + aSymbol->AddDrawItem( field.get(), false ); + return field.release(); } else { *existingField = *field; + return existingField; } } } @@ -2280,8 +2295,8 @@ SCH_COMPONENT* SCH_SEXPR_PARSER::parseSchematicSymbol() // 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 fields and will - // get written out as the next unused number on save. + // 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 @@ -2533,8 +2548,8 @@ 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 fields and will - // get written out as the next unused number on save. + // 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 diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.h b/eeschema/sch_plugins/kicad/sch_sexpr_parser.h index 9732ca894e..970b1fdc1b 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.h +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.h @@ -173,7 +173,7 @@ class SCH_SEXPR_PARSER : public SCHEMATIC_LEXER void parseEDA_TEXT( EDA_TEXT* aText ); void parsePinNames( std::unique_ptr& aSymbol ); - void parseProperty( std::unique_ptr& aSymbol ); + LIB_FIELD* parseProperty( std::unique_ptr& aSymbol ); LIB_ARC* parseArc(); LIB_BEZIER* parseBezier(); diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp index 2806eb2b4a..deeeac2d97 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp @@ -297,8 +297,7 @@ class SCH_SEXPR_PLUGIN_CACHE bool m_isWritable; bool m_isModified; int m_versionMajor; - int m_versionMinor; - SCH_LIB_TYPE m_libType; // Is this cache a component or symbol library. + SCH_LIB_TYPE m_libType; // Is this cache a component or symbol library. LIB_PART* removeSymbol( LIB_PART* aAlias ); @@ -309,8 +308,8 @@ class SCH_SEXPR_PLUGIN_CACHE int aNestLevel = 0 ); static void saveCircle( LIB_CIRCLE* aCircle, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); - static void saveField( const LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, - int aNestLevel = 0 ); + static void saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, + int& aNextFreeFieldId, int aNestLevel ); static void savePin( LIB_PIN* aPin, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); static void savePolyLine( LIB_POLYLINE* aPolyLine, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); @@ -319,7 +318,7 @@ class SCH_SEXPR_PLUGIN_CACHE static void saveText( LIB_TEXT* aText, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); static void saveDcmInfoAsFields( LIB_PART* aSymbol, OUTPUTFORMATTER& aFormatter, - int aNestLevel = 0, int aFirstId = MANDATORY_FIELDS ); + int& aNextFreeFieldId, int aNestLevel ); friend SCH_SEXPR_PLUGIN; @@ -1317,7 +1316,6 @@ SCH_SEXPR_PLUGIN_CACHE::SCH_SEXPR_PLUGIN_CACHE( const wxString& aFullPathAndFile m_isModified( false ) { m_versionMajor = -1; - m_versionMinor = -1; m_libType = SCH_LIB_TYPE::LT_EESCHEMA; } @@ -1556,7 +1554,7 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_PART* aSymbol, OUTPUTFORMATTER& aFo wxCHECK2( wxLocale::GetInfo( wxLOCALE_DECIMAL_POINT, wxLOCALE_CAT_NUMBER ) == ".", LOCALE_IO toggle ); - int lastFieldId; + int nextFreeFieldId = MANDATORY_FIELDS; std::vector fields; std::string name = aFormatter.Quotew( aSymbol->GetLibId().Format().wx_str() ); std::string unitName = aSymbol->GetLibId().GetLibItemName(); @@ -1612,22 +1610,19 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_PART* aSymbol, OUTPUTFORMATTER& aFo aSymbol->GetFields( fields ); - for( const LIB_FIELD* field : fields ) - saveField( field, aFormatter, aNestLevel + 1 ); - - lastFieldId = fields.back()->GetId() + 1; + for( LIB_FIELD* field : fields ) + saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 ); // @todo At some point in the future the lock status (all units interchangeable) should // be set deterministically. For now a custom lock propertery is used to preserve the // locked flag state. if( aSymbol->UnitsLocked() ) { - LIB_FIELD locked( lastFieldId, "ki_locked" ); - saveField( &locked, aFormatter, aNestLevel + 1 ); - lastFieldId += 1; + LIB_FIELD locked( -1, "ki_locked" ); + saveField( &locked, aFormatter, nextFreeFieldId, aNestLevel + 1 ); } - saveDcmInfoAsFields( aSymbol, aFormatter, aNestLevel, lastFieldId ); + saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel ); // Save the draw items grouped by units. std::vector units = aSymbol->GetUnitDrawItems(); @@ -1667,12 +1662,10 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_PART* aSymbol, OUTPUTFORMATTER& aFo aSymbol->GetFields( fields ); - for( const LIB_FIELD* field : fields ) - saveField( field, aFormatter, aNestLevel + 1 ); + for( LIB_FIELD* field : fields ) + saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 ); - lastFieldId = fields.back()->GetId() + 1; - - saveDcmInfoAsFields( aSymbol, aFormatter, aNestLevel, lastFieldId ); + saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel ); } aFormatter.Print( aNestLevel, ")\n" ); @@ -1680,28 +1673,24 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_PART* aSymbol, OUTPUTFORMATTER& aFo void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_PART* aSymbol, OUTPUTFORMATTER& aFormatter, - int aNestLevel, int aFirstId ) + int& aNextFreeFieldId, int aNestLevel ) { wxCHECK_RET( aSymbol, "Invalid LIB_PART pointer." ); - int id = aFirstId; - if( !aSymbol->GetKeyWords().IsEmpty() ) { - LIB_FIELD keywords( id, wxString( "ki_keywords" ) ); + LIB_FIELD keywords( -1, wxString( "ki_keywords" ) ); keywords.SetVisible( false ); keywords.SetText( aSymbol->GetKeyWords() ); - saveField( &keywords, aFormatter, aNestLevel + 1 ); - id += 1; + saveField( &keywords, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); } if( !aSymbol->GetDescription().IsEmpty() ) { - LIB_FIELD description( id, wxString( "ki_description" ) ); + LIB_FIELD description( -1, wxString( "ki_description" ) ); description.SetVisible( false ); description.SetText( aSymbol->GetDescription() ); - saveField( &description, aFormatter, aNestLevel + 1 ); - id += 1; + saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); } wxArrayString fpFilters = aSymbol->GetFPFilters(); @@ -1718,11 +1707,10 @@ void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_PART* aSymbol, OUTPUTFORMA tmp += " " + filter; } - LIB_FIELD description( id, wxString( "ki_fp_filters" ) ); + LIB_FIELD description( -1, wxString( "ki_fp_filters" ) ); description.SetVisible( false ); description.SetText( tmp ); - saveField( &description, aFormatter, aNestLevel + 1 ); - id += 1; + saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); } } @@ -1874,13 +1862,23 @@ void SCH_SEXPR_PLUGIN_CACHE::saveCircle( LIB_CIRCLE* aCircle, } -void SCH_SEXPR_PLUGIN_CACHE::saveField( const LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, - int aNestLevel ) +void SCH_SEXPR_PLUGIN_CACHE::saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, + int& aNextFreeFieldId, int aNestLevel ) { wxCHECK_RET( aField && aField->Type() == LIB_FIELD_T, "Invalid LIB_FIELD object." ); wxString fieldName = aField->GetName(); + if( aField->GetId() == -1 /* undefined ID */ ) + { + aField->SetId( aNextFreeFieldId ); + aNextFreeFieldId += 1; + } + else if( aField->GetId() >= aNextFreeFieldId ) + { + aNextFreeFieldId = aField->GetId() + 1; + } + if( aField->GetId() >= 0 && aField->GetId() < MANDATORY_FIELDS ) fieldName = TEMPLATE_FIELDNAME::GetDefaultFieldName( aField->GetId(), false );