Eeschema: fix a multitude of LIB_FIELD object comparison and index bugs.

It is no longer acceptable to set a LIB_FIELD index to -1.  This was
trashing the sorting on load cause all kinds of false field comparison
failures.  There are now assertions when attempting to use -1 as a field
index.

Fixes https://gitlab.com/kicad/code/kicad/-/issues/9811

eeschema/lib_field.cpp

@@ -39,24 +39,24 @@
 #include <settings/color_settings.h>
 
 
-LIB_FIELD::LIB_FIELD( LIB_SYMBOL* aParent, int idfield ) :
+LIB_FIELD::LIB_FIELD( LIB_SYMBOL* aParent, int aId ) :
     LIB_ITEM( LIB_FIELD_T, aParent )
 {
-    Init( idfield );
+    Init( aId );
 }
 
 
-LIB_FIELD::LIB_FIELD( int idfield ) :
+LIB_FIELD::LIB_FIELD( int aId ) :
     LIB_ITEM( LIB_FIELD_T, nullptr )
 {
-    Init( idfield );
+    Init( aId );
 }
 
 
-LIB_FIELD::LIB_FIELD( int aID, const wxString& aName ) :
+LIB_FIELD::LIB_FIELD( int aId, const wxString& aName ) :
     LIB_ITEM( LIB_FIELD_T, nullptr )
 {
-    Init( aID );
+    Init( aId );
     m_name = aName;
 }
 
@@ -79,23 +79,32 @@ LIB_FIELD& LIB_FIELD::operator=( const LIB_FIELD& field )
 }
 
 
-void LIB_FIELD::Init( int id )
+void LIB_FIELD::Init( int aId )
 {
-    m_id = id;
+    wxCHECK2( aId >= 0, aId = MANDATORY_FIELDS );
+
+    m_id = aId;
 
     SetTextAngle( TEXT_ANGLE_HORIZ );    // constructor already did this.
 
     // Fields in RAM must always have names, because we are trying to get less dependent on
     // field ids and more dependent on names. Plus assumptions are made in the field editors.
-    m_name = TEMPLATE_FIELDNAME::GetDefaultFieldName( id );
+    m_name = TEMPLATE_FIELDNAME::GetDefaultFieldName( aId );
 
     // By contrast, VALUE and REFERENCE are are always constructed as initially visible, and
     // template fieldsnames' initial visibility is controlled by the template fieldname config.
-    if( id == DATASHEET_FIELD || id == FOOTPRINT_FIELD )
+    if( aId == DATASHEET_FIELD || aId == FOOTPRINT_FIELD )
         SetVisible( false );
 }
 
 
+void LIB_FIELD::SetId( int aId )
+{
+    wxCHECK2( aId >= 0, aId = MANDATORY_FIELDS );
+    m_id = aId;
+}
+
+
 int LIB_FIELD::GetPenWidth() const
 {
     return GetEffectiveTextPenWidth();
@@ -181,13 +190,35 @@ int LIB_FIELD::compare( const LIB_ITEM& aOther, LIB_ITEM::COMPARE_FLAGS aCompare
 
     const LIB_FIELD* tmp = ( LIB_FIELD* ) &aOther;
 
-    if( m_id != tmp->m_id )
+    // Equality test will vary depending whether or not the field is mandatory.  Otherwise,
-        return m_id - tmp->m_id;
+    // sorting is done by ordinal.
+    if( aCompareFlags & LIB_ITEM::COMPARE_FLAGS::EQUALITY )
+    {
+        // Mandatory fields have fixed ordinals and their names can vary due to translated field
+        // names.  Optional fields have fixed names and their ordinals can vary.
+        if( IsMandatory() )
+        {
+            if( m_id != tmp->m_id )
+                return m_id - tmp->m_id;
+        }
+        else
+        {
+            retv = m_name.Cmp( tmp->m_name );
 
-    int result = GetText().CmpNoCase( tmp->GetText() );
+            if( retv )
+                return retv;
+        }
+    }
+    else
+    {
+        if( m_id != tmp->m_id )
+            return m_id - tmp->m_id;
+    }
 
-    if( result != 0 )
+    retv = GetText().CmpNoCase( tmp->GetText() );
-        return result;
+
+    if( retv != 0 )
+        return retv;
 
     if( GetTextPos().x != tmp->GetTextPos().x )
         return GetTextPos().x - tmp->GetTextPos().x;

eeschema/lib_field.h

@@ -59,11 +59,11 @@ class SCH_LEGACY_PLUGIN_CACHE;
 class LIB_FIELD : public LIB_ITEM, public EDA_TEXT
 {
 public:
-    LIB_FIELD( int idfield = 2 );
+    LIB_FIELD( int aId = 2 );
 
-    LIB_FIELD( int aID, const wxString& aName );
+    LIB_FIELD( int aId, const wxString& aName );
 
-    LIB_FIELD( LIB_SYMBOL* aParent, int idfield = 2 );
+    LIB_FIELD( LIB_SYMBOL* aParent, int aId = 2 );
 
     // Do not create a copy constructor.  The one generated by the compiler is adequate.
 
@@ -82,7 +82,7 @@ public:
     /**
      * Object constructor initialization helper.
      */
-    void Init( int idfield );
+    void Init( int aId );
 
     /**
      * Return the field name.
@@ -91,7 +91,7 @@ public:
      * names.
      *
      * The user definable fields will return FieldN where N is the ID of the field when the
-     * m_name member is empyt unless false is passed to \a aUseDefaultName.
+     * m_name member is empty unless false is passed to \a aUseDefaultName.
      */
     wxString GetName( bool aUseDefaultName = true ) const;
 
@@ -113,7 +113,7 @@ public:
     void SetName( const wxString& aName );
 
     int GetId() const { return m_id; }
-    void SetId( int aId ) { m_id = aId; }
+    void SetId( int aId );
 
     int GetPenWidth() const override;
 
@@ -206,8 +206,8 @@ private:
 
     friend class SCH_LEGACY_PLUGIN_CACHE;   // Required to access m_name.
 
-    int      m_id;           ///< @see enum MANDATORY_FIELD_T
+    int      m_id;         ///< @see enum MANDATORY_FIELD_T
-    wxString m_name;         ///< Name (not the field text value itself, that is .m_Text)
+    wxString m_name;       ///< Name (not the field text value itself, that is #EDA_TEXT::m_Text)
 };
 
 #endif  //  CLASS_LIBENTRY_FIELDS_H

eeschema/lib_item.cpp

@@ -2,7 +2,7 @@
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
  * Copyright (C) 2015 Jean-Pierre Charras, jaen-pierre.charras at wanadoo.fr
- * Copyright (C) 2015 Wayne Stambaugh <stambaughw@verizon.net>
+ * Copyright (C) 2015 Wayne Stambaugh <stambaughw@gmail.com>
  * Copyright (C) 2004-2021 KiCad Developers, see change_log.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
@@ -85,7 +85,7 @@ bool LIB_ITEM::operator==( const LIB_ITEM& aOther ) const
     if( Type() != aOther.Type() )
         return false;
 
-    return compare( aOther ) == 0;
+    return compare( aOther, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) == 0;
 }
 
 

eeschema/lib_item.h

@@ -77,7 +77,7 @@ public:
      * - UNIT This compare flag ignores unit and convert and pin number information when
      *        comparing #LIB_ITEM objects for unit comparison.
      */
-    enum COMPARE_FLAGS : int { NORMAL = 0x00, UNIT = 0x01 };
+    enum COMPARE_FLAGS : int { NORMAL = 0x00, UNIT = 0x01, EQUALITY = 0x02 };
 
     /**
      * Provide a user-consumable name of the object type.  Perform localization when

eeschema/lib_symbol.cpp

@@ -209,7 +209,7 @@ const LIB_SYMBOL& LIB_SYMBOL::operator=( const LIB_SYMBOL& aSymbol )
 }
 
 
-int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const
+int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, LIB_ITEM::COMPARE_FLAGS aCompareFlags ) const
 {
     if( m_me == aRhs.m_me )
         return 0;
@@ -252,7 +252,19 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const
         if( lhsItem->Type() != rhsItem->Type() )
             return lhsItem->Type() - rhsItem->Type();
 
-        retv = lhsItem->compare( *rhsItem );
+        // Non-mandatory fields are a special case.  They can have different ordinal numbers
+        // and are compared separately below.
+        if( lhsItem->Type() == LIB_FIELD_T )
+        {
+            const LIB_FIELD* lhsField = static_cast<const LIB_FIELD*>( lhsItem );
+
+            if( lhsField->IsMandatory() )
+                retv = lhsItem->compare( *rhsItem, aCompareFlags );
+        }
+        else
+        {
+            retv = lhsItem->compare( *rhsItem, aCompareFlags );
+        }
 
         if( retv )
             return retv;
@@ -261,6 +273,28 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const
         ++rhsItemIt;
     }
 
+    // Compare the optional fields.
+    for( const LIB_ITEM& item : m_drawings[ LIB_FIELD_T ] )
+    {
+        const LIB_FIELD* field = dynamic_cast<const LIB_FIELD*>( &item );
+
+        wxCHECK2( field, continue );
+
+        // Mandatory fields were already compared above.
+        if( field->IsMandatory() )
+            continue;
+
+        const LIB_FIELD* foundField = aRhs.FindField( field->GetName() );
+
+        if( foundField == nullptr )
+            return 1;
+
+        retv = item.compare( static_cast<const LIB_ITEM&>( *foundField ), aCompareFlags );
+
+        if( retv )
+            return retv;
+    }
+
     if( m_fpFilters.GetCount() != aRhs.m_fpFilters.GetCount() )
         return m_fpFilters.GetCount() - aRhs.m_fpFilters.GetCount();
 
@@ -342,7 +376,8 @@ std::unique_ptr< LIB_SYMBOL > LIB_SYMBOL::Flatten() const
         // Copy the parent.
         retv.reset( new LIB_SYMBOL( *parent.get() ) );
 
-        retv->SetName( m_name );
+        retv->m_name = m_name;
+        retv->SetLibId( m_libId );
 
         // Now add the inherited part mandatory field (this) information.
         for( int i = 0; i < MANDATORY_FIELDS; i++ )
@@ -386,6 +421,7 @@ std::unique_ptr< LIB_SYMBOL > LIB_SYMBOL::Flatten() const
         retv->SetKeyWords( m_keyWords.IsEmpty() ? parent->GetKeyWords() : m_keyWords );
         retv->SetDescription( m_description.IsEmpty() ? parent->GetDescription() : m_description );
         retv->SetFPFilters( m_fpFilters.IsEmpty() ? parent->GetFPFilters() : m_fpFilters );
+        retv->UpdateFieldOrdinals();
     }
     else
     {
@@ -1032,6 +1068,45 @@ LIB_FIELD& LIB_SYMBOL::GetDatasheetField()
 }
 
 
+int LIB_SYMBOL::UpdateFieldOrdinals()
+{
+    int retv = 0;
+    int lastOrdinal = MANDATORY_FIELDS;
+
+    for( LIB_ITEM& item : m_drawings[ LIB_FIELD_T ] )
+    {
+        LIB_FIELD* field = dynamic_cast<LIB_FIELD*>( &item );
+
+        wxCHECK2( field, continue );
+
+        // Mandatory fields were already resolved always have the same ordinal values.
+        if( field->IsMandatory() )
+            continue;
+
+        if( field->GetId() != lastOrdinal )
+        {
+            field->SetId( lastOrdinal );
+            retv += 1;
+        }
+
+        lastOrdinal += 1;
+    }
+
+    return retv;
+}
+
+
+int LIB_SYMBOL::GetNextAvailableFieldId() const
+{
+    int retv = MANDATORY_FIELDS;
+
+    while( GetFieldById( retv ) )
+        retv += 1;
+
+    return retv;
+}
+
+
 void LIB_SYMBOL::SetOffset( const wxPoint& aOffset )
 {
     for( LIB_ITEM& item : m_drawings )

eeschema/lib_symbol.h

@@ -292,6 +292,17 @@ public:
     /** Return reference to the datasheet field. */
     LIB_FIELD& GetDatasheetField();
 
+    /**
+     * Order optional field indices.
+     *
+     * It's possible when calling #LIB_SYMBOL::Flatten that there can be gaps and/or duplicate
+     * optional field indices.  This method correctly orders the indices so there are no gaps
+     * and/or duplicate indices.
+     */
+    int UpdateFieldOrdinals();
+
+    int GetNextAvailableFieldId() const;
+
     /**
      * Print symbol.
      *
@@ -601,11 +612,19 @@ public:
      *         1 if this symbol is greater than \a aRhs
      *         0 if this symbol is the same as \a aRhs
      */
-    int Compare( const LIB_SYMBOL& aRhs ) const;
+    int Compare( const LIB_SYMBOL& aRhs,
+                 LIB_ITEM::COMPARE_FLAGS aCompareFlags = LIB_ITEM::COMPARE_FLAGS::NORMAL ) const;
 
     bool operator==( const LIB_SYMBOL* aSymbol ) const { return this == aSymbol; }
-    bool operator==( const LIB_SYMBOL& aSymbol ) const { return Compare( aSymbol ) == 0; }
+    bool operator==( const LIB_SYMBOL& aSymbol ) const
-    bool operator!=( const LIB_SYMBOL& aSymbol ) const { return Compare( aSymbol ) != 0; }
+    {
+        return Compare( aSymbol, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) == 0;
+    }
+
+    bool operator!=( const LIB_SYMBOL& aSymbol ) const
+    {
+        return Compare( aSymbol, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) != 0;
+    }
 
     const LIB_SYMBOL& operator=( const LIB_SYMBOL& aSymbol );
 

eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp

@@ -236,17 +236,23 @@ LIB_SYMBOL* SCH_SEXPR_PARSER::ParseSymbol( LIB_SYMBOL_MAP& aSymbolLibMap, int aF
 
             if( field )
             {
-                // It would appear that at some point we allowed duplicate ids to slip through
+                // Due to an bug when in #LIB_SYMBOL::Flatten, duplicate ids slipped through
-                // when writing files.  The easiest (and most complete) solution is to disallow
+                // when writing files.  This section replaces duplicate #LIB_FIELD indices on
-                // multiple instances of the same id (for all files since the source of the error
+                // load.
-                // *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 );
+                {
+                    int nextAvailableId = field->GetId() + 1;
+
+                    while( fieldIDsRead.count( nextAvailableId ) )
+                        nextAvailableId += 1;
+
+                    fieldIDsRead.insert( nextAvailableId );
+                    field->SetId( nextAvailableId );
+                }
                 else if( field )
+                {
                     fieldIDsRead.insert( field->GetId() );
+                }
             }
 
             break;
@@ -2099,6 +2105,7 @@ void SCH_SEXPR_PARSER::ParseSchematic( SCH_SHEET* aSheet, bool aIsCopyableOnly,
         {
             // Dummy map.  No derived symbols are allowed in the library cache.
             LIB_SYMBOL_MAP symbolLibMap;
+            LIB_SYMBOL* symbol;
 
             for( token = NextTok();  token != T_RIGHT;  token = NextTok() )
             {
@@ -2110,7 +2117,9 @@ void SCH_SEXPR_PARSER::ParseSchematic( SCH_SHEET* aSheet, bool aIsCopyableOnly,
                 switch( token )
                 {
                 case T_symbol:
-                    screen->AddLibSymbol( ParseSymbol( symbolLibMap, m_requiredVersion ) );
+                    symbol = ParseSymbol( symbolLibMap, m_requiredVersion );
+                    symbol->UpdateFieldOrdinals();
+                    screen->AddLibSymbol( symbol );
                     break;
 
                 default:

eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp

@@ -445,8 +445,7 @@ class SCH_SEXPR_PLUGIN_CACHE
 
     static void saveSymbolDrawItem( LIB_ITEM* aItem, OUTPUTFORMATTER& aFormatter,
                                     int aNestLevel );
-    static void saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, int& aNextFreeFieldId,
+    static void saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, int aNestLevel );
-                           int aNestLevel );
     static void savePin( LIB_PIN* aPin, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 );
     static void saveText( LIB_TEXT* aText, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 );
 
@@ -1797,15 +1796,18 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_SYMBOL* aSymbol, OUTPUTFORMATTER& a
         aSymbol->GetFields( fields );
 
         for( LIB_FIELD* field : fields )
-            saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 );
+            saveField( field, aFormatter, aNestLevel + 1 );
+
+        nextFreeFieldId = aSymbol->GetNextAvailableFieldId();
 
         // @todo At some point in the future the lock status (all units interchangeable) should
         // be set deterministically.  For now a custom lock property is used to preserve the
         // locked flag state.
         if( aSymbol->UnitsLocked() )
         {
-            LIB_FIELD locked( -1, "ki_locked" );
+            LIB_FIELD locked( nextFreeFieldId, "ki_locked" );
-            saveField( &locked, aFormatter, nextFreeFieldId, aNestLevel + 1 );
+            saveField( &locked, aFormatter, aNestLevel + 1 );
+            nextFreeFieldId += 1;
         }
 
         saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel );
@@ -1861,7 +1863,9 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_SYMBOL* aSymbol, OUTPUTFORMATTER& a
         aSymbol->GetFields( fields );
 
         for( LIB_FIELD* field : fields )
-            saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 );
+            saveField( field, aFormatter, aNestLevel + 1 );
+
+        nextFreeFieldId = aSymbol->GetNextAvailableFieldId();
 
         saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel );
     }
@@ -1877,18 +1881,20 @@ void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_SYMBOL* aSymbol, OUTPUTFOR
 
     if( !aSymbol->GetKeyWords().IsEmpty() )
     {
-        LIB_FIELD keywords( -1, wxString( "ki_keywords" ) );
+        LIB_FIELD keywords( aNextFreeFieldId, wxString( "ki_keywords" ) );
         keywords.SetVisible( false );
         keywords.SetText( aSymbol->GetKeyWords() );
-        saveField( &keywords, aFormatter, aNextFreeFieldId, aNestLevel + 1 );
+        saveField( &keywords, aFormatter, aNestLevel + 1 );
+        aNextFreeFieldId += 1;
     }
 
     if( !aSymbol->GetDescription().IsEmpty() )
     {
-        LIB_FIELD description( -1, wxString( "ki_description" ) );
+        LIB_FIELD description( aNextFreeFieldId, wxString( "ki_description" ) );
         description.SetVisible( false );
         description.SetText( aSymbol->GetDescription() );
-        saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 );
+        saveField( &description, aFormatter, aNestLevel + 1 );
+        aNextFreeFieldId += 1;
     }
 
     wxArrayString fpFilters = aSymbol->GetFPFilters();
@@ -1908,10 +1914,11 @@ void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_SYMBOL* aSymbol, OUTPUTFOR
                 tmp += " " + curr_filter;
         }
 
-        LIB_FIELD description( -1, wxString( "ki_fp_filters" ) );
+        LIB_FIELD description( aNextFreeFieldId, wxString( "ki_fp_filters" ) );
         description.SetVisible( false );
         description.SetText( tmp );
-        saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 );
+        saveField( &description, aFormatter, aNestLevel + 1 );
+        aNextFreeFieldId += 1;
     }
 }
 
@@ -1980,22 +1987,12 @@ void SCH_SEXPR_PLUGIN_CACHE::saveSymbolDrawItem( LIB_ITEM* aItem, OUTPUTFORMATTE
 
 
 void SCH_SEXPR_PLUGIN_CACHE::saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter,
-                                        int& aNextFreeFieldId, int aNestLevel  )
+                                        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 );