Show inherited fields in symbols

When working with derived symbols, make sure that we show all of the
fields, not just the ones that are defined in the current level since
they are all attached to the output.

Inherited fields are shown in italics until changed

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

(cherry picked from commit ff5a309386)

eeschema/dialogs/dialog_lib_symbol_properties.cpp

@@ -84,6 +84,7 @@ DIALOG_LIB_SYMBOL_PROPERTIES::DIALOG_LIB_SYMBOL_PROPERTIES( SYMBOL_EDIT_FRAME* a
                                                       } ) );
     m_grid->SetSelectionMode( wxGrid::wxGridSelectRows );
 
+
     // Show/hide columns according to the user's preference
     SYMBOL_EDITOR_SETTINGS* cfg = m_Parent->GetSettings();
     m_grid->ShowHideColumns( cfg->m_EditSymbolVisibleColumns );
@@ -110,8 +111,11 @@ DIALOG_LIB_SYMBOL_PROPERTIES::DIALOG_LIB_SYMBOL_PROPERTIES( SYMBOL_EDIT_FRAME* a
 
     // wxFormBuilder doesn't include this event...
     m_grid->Connect( wxEVT_GRID_CELL_CHANGING,
-                     wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging ),
-                     nullptr, this );
+                     wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging ), nullptr, this );
+    m_grid->Connect( wxEVT_GRID_CELL_CHANGED,
+                     wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanged ), nullptr, this );
+    m_grid->GetGridWindow()->Bind( wxEVT_MOTION, &DIALOG_LIB_SYMBOL_PROPERTIES::OnGridMotion, this );
+
 
     // Forward the delete button to the tricks
     m_deleteFilterButton->Bind( wxEVT_BUTTON,
@@ -161,8 +165,10 @@ DIALOG_LIB_SYMBOL_PROPERTIES::~DIALOG_LIB_SYMBOL_PROPERTIES()
     m_grid->DestroyTable( m_fields );
 
     m_grid->Disconnect( wxEVT_GRID_CELL_CHANGING,
-                        wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging ),
-                        nullptr, this );
+                        wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging ), nullptr, this );
+    m_grid->Disconnect( wxEVT_GRID_CELL_CHANGED,
+                        wxGridEventHandler( DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanged ), nullptr, this );
+    m_grid->GetGridWindow()->Unbind( wxEVT_MOTION, &DIALOG_LIB_SYMBOL_PROPERTIES::OnGridMotion, this );
 
     // Delete the GRID_TRICKS.
     m_grid->PopEventHandler( true );
@@ -174,9 +180,58 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataToWindow()
     if( !wxDialog::TransferDataToWindow() )
         return false;
 
-    // Push a copy of each field into m_updateFields
+    // Ensure the fields vector is empty before (re)loading.  Constructor no longer
+    // pre-populates fields to avoid double insertion of mandatory fields.
+    if( m_fields->size() > 0 )
+    {
+        // Clear existing entries; we recreate inheritance metadata via push_back()
+        // Note: We cannot simply reuse existing entries because alias inheritance
+        // state may have changed if the parent was edited prior to dialog open.
+        m_fields->clear();
+    }
+
+    // Load current symbol fields (mandatory first, then user fields)
     m_libEntry->CopyFields( *m_fields );
 
+    // Handle alias (derived) symbol inherited fields.  We only inherit non-mandatory fields
+    // and mark any matching names as inherited rather than duplicating them.
+    if( m_libEntry->IsAlias() )
+    {
+        if( LIB_SYMBOL_SPTR parent = m_libEntry->GetParent().lock() )
+        {
+            std::vector<SCH_FIELD*> parentFields;
+            parent->GetFields( parentFields );
+
+            for( SCH_FIELD* pf : parentFields )
+            {
+                if( pf->IsMandatory() )
+                    continue; // Never inherit mandatory fields
+
+                bool found = false;
+
+                for( size_t jj = 0; jj < m_fields->size(); ++jj )
+                {
+                    SCH_FIELD& f = m_fields->at( jj );
+
+                    if( f.IsMandatory() )
+                        continue; // Skip mandatory in child list for inheritance match
+
+                    if( f.GetCanonicalName() == pf->GetCanonicalName() )
+                    {
+                        m_fields->SetFieldInherited( jj, *pf );
+                        found = true;
+                        break;
+                    }
+                }
+
+                if( !found )
+                    m_fields->AddInheritedField( *pf );
+            }
+        }
+    }
+
+    // Fields already loaded above; avoid re-copying to prevent duplication.
+
     std::set<wxString> defined;
 
     for( SCH_FIELD& field : *m_fields )
@@ -416,6 +471,8 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataFromWindow()
         m_Parent->SaveCopyInUndoList( _( "Edit Symbol Properties" ), m_libEntry );
     }
 
+    std::vector<SCH_FIELD> fieldsToSave;
+
     // The Y axis for components in lib is from bottom to top while the screen axis is top
     // to bottom: we must change the y coord sign when writing back to the library
     for( int ii = 0; ii < (int) m_fields->size(); ++ii )
@@ -430,23 +487,32 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataFromWindow()
     {
         SCH_FIELD& field = m_fields->at( ii );
 
-        if( field.IsMandatory() )
-            continue;
+        VECTOR2I pos = field.GetPosition();
+        pos.y = -pos.y;
+        field.SetPosition( pos );
 
-        const wxString& fieldName = field.GetCanonicalName();
+        // if( !field.IsMandatory() )
+            field.SetId( ii );
+
+        wxString fieldName = field.GetCanonicalName();
+
+        if( m_fields->IsInherited( ii ) && field == m_fields->ParentField( ii ) )
+            continue; // Skip inherited fields
 
         if( field.GetText().IsEmpty() )
         {
             if( fieldName.IsEmpty() || m_addedTemplateFields.contains( fieldName ) )
-                m_fields->erase( m_fields->begin() + ii );
+                continue; // Skip empty fields that are not mandatory or template fields
         }
         else if( fieldName.IsEmpty() )
         {
-            field.SetName( _( "untitled" ) );
+            field.SetName( _( "untitled" ) ); // Set a default name for unnamed fields
         }
+
+        fieldsToSave.push_back( field );
     }
 
-    m_libEntry->SetFields( *m_fields );
+    m_libEntry->SetFields( fieldsToSave );
 
     // Update the parent for inherited symbols
     if( m_libEntry->IsAlias() )
@@ -517,6 +583,27 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataFromWindow()
 }
 
 
+void DIALOG_LIB_SYMBOL_PROPERTIES::OnGridMotion( wxMouseEvent& aEvent )
+{
+    aEvent.Skip();
+
+    wxPoint pos = aEvent.GetPosition();
+    wxPoint unscolled_pos = m_grid->CalcUnscrolledPosition( pos );
+    int row = m_grid->YToRow( unscolled_pos.y );
+    int col = m_grid->XToCol( unscolled_pos.x );
+
+    if( row == wxNOT_FOUND || col == wxNOT_FOUND || !m_fields->IsInherited( row ) )
+    {
+        m_grid->SetToolTip( "" );
+        return;
+    }
+
+    m_grid->SetToolTip(
+            wxString::Format( _( "This field is inherited from '%s'." ),
+                              m_fields->ParentField( row ).GetName() ) );
+}
+
+
 void DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging( wxGridEvent& event )
 {
     wxGridCellEditor* editor = m_grid->GetCellEditor( event.GetRow(), event.GetCol() );
@@ -555,6 +642,13 @@ void DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanging( wxGridEvent& event )
 }
 
 
+void DIALOG_LIB_SYMBOL_PROPERTIES::OnGridCellChanged( wxGridEvent& event )
+{
+    m_grid->ForceRefresh();
+    OnModify();
+}
+
+
 void DIALOG_LIB_SYMBOL_PROPERTIES::OnSymbolNameText( wxCommandEvent& event )
 {
     if( m_OptionPower->IsChecked() )

eeschema/dialogs/dialog_lib_symbol_properties.h

@@ -68,6 +68,8 @@ private:
     void OnEditFootprintFilter( wxCommandEvent& event ) override;
     void OnSizeGrid( wxSizeEvent& event ) override;
     void OnGridCellChanging( wxGridEvent& event );
+    void OnGridCellChanged( wxGridEvent& event );
+    void OnGridMotion( wxMouseEvent& event );
     void OnEditSpiceModel( wxCommandEvent& event ) override;
     void OnUpdateUI( wxUpdateUIEvent& event ) override;
     void OnCancelButtonClick( wxCommandEvent& event ) override;

eeschema/fields_grid_table.cpp

@@ -225,6 +225,15 @@ int FIELDS_GRID_TABLE::GetMandatoryRowCount() const
 }
 
 
+void FIELDS_GRID_TABLE::push_back( const SCH_FIELD& aField )
+{
+    std::vector<SCH_FIELD>::push_back( aField );
+
+    m_isInherited.resize( size() );
+    m_parentFields.resize( size() );
+}
+
+
 void FIELDS_GRID_TABLE::initGrid( WX_GRID* aGrid )
 {
     // Build the various grid cell attributes.
@@ -399,6 +408,9 @@ FIELDS_GRID_TABLE::~FIELDS_GRID_TABLE()
     m_fontAttr->DecRef();
     m_colorAttr->DecRef();
 
+    for( SCH_FIELD& field : m_parentFields )
+        field.SetParent( nullptr );
+
     m_frame->Unbind( EDA_EVT_UNITS_CHANGED, &FIELDS_GRID_TABLE::onUnitsChanged, this );
 }
 
@@ -538,33 +550,34 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAttr::
     wxCHECK( aRow < GetNumberRows(), nullptr );
 
     const SCH_FIELD& field = getField( aRow );
-    wxGridCellAttr*  tmp;
+    wxGridCellAttr*  attr = nullptr;
 
     switch( aCol )
     {
     case FDC_NAME:
         if( field.IsMandatory() )
         {
-            tmp = m_fieldNameAttr->Clone();
-            tmp->SetReadOnly( true );
-            return enhanceAttr( tmp, aRow, aCol, aKind );
+            attr = m_fieldNameAttr->Clone();
+            attr->SetReadOnly( true );
         }
         else
         {
             m_fieldNameAttr->IncRef();
-            return enhanceAttr( m_fieldNameAttr, aRow, aCol, aKind );
+            attr = m_fieldNameAttr;
         }
 
+        break;
+
     case FDC_VALUE:
         if( m_parentType == SCH_SYMBOL_T && field.GetId() == REFERENCE_FIELD )
         {
             m_referenceAttr->IncRef();
-            return enhanceAttr( m_referenceAttr, aRow, aCol, aKind );
+            attr = m_referenceAttr;
         }
         else if( m_parentType == SCH_SYMBOL_T && field.GetId() == VALUE_FIELD )
         {
             m_valueAttr->IncRef();
-            return enhanceAttr( m_valueAttr, aRow, aCol, aKind );
+            attr = m_valueAttr;
         }
         else if( m_parentType == SCH_SYMBOL_T && field.GetId() == FOOTPRINT_FIELD )
         {
@@ -574,34 +587,34 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAttr::
             if( m_part && m_part->IsPower() )
             {
                 m_readOnlyAttr->IncRef();
-                return enhanceAttr( m_readOnlyAttr, aRow, aCol, aKind );
+                attr = m_readOnlyAttr;
             }
             else
             {
                 m_footprintAttr->IncRef();
-                return enhanceAttr( m_footprintAttr, aRow, aCol, aKind );
+                attr = m_footprintAttr;
             }
         }
         else if( m_parentType == SCH_SYMBOL_T && field.GetId() == DATASHEET_FIELD )
         {
             m_urlAttr->IncRef();
-            return enhanceAttr( m_urlAttr, aRow, aCol, aKind );
+            attr = m_urlAttr;
         }
         else if( m_parentType == SCH_SHEET_T && field.GetId() == SHEETNAME )
         {
             m_referenceAttr->IncRef();
-            return enhanceAttr( m_referenceAttr, aRow, aCol, aKind );
+            attr = m_referenceAttr;
         }
         else if( m_parentType == SCH_SHEET_T && field.GetId() == SHEETFILENAME )
         {
             m_filepathAttr->IncRef();
-            return enhanceAttr( m_filepathAttr, aRow, aCol, aKind );
+            attr = m_filepathAttr;
         }
         else if( ( m_parentType == SCH_LABEL_LOCATE_ANY_T )
                 && field.GetCanonicalName() == wxT( "Netclass" ) )
         {
             m_netclassAttr->IncRef();
-            return enhanceAttr( m_netclassAttr, aRow, aCol, aKind );
+            attr = m_netclassAttr;
         }
         else
         {
@@ -615,31 +628,36 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAttr::
             if( ( templateFn && templateFn->m_URL ) || field.IsHypertext() )
             {
                 m_urlAttr->IncRef();
-                return enhanceAttr( m_urlAttr, aRow, aCol, aKind );
+                attr = m_urlAttr;
             }
             else
             {
                 m_nonUrlAttr->IncRef();
-                return enhanceAttr( m_nonUrlAttr, aRow, aCol, aKind );
+                attr = m_nonUrlAttr;
             }
         }
 
+        break;
+
     case FDC_TEXT_SIZE:
     case FDC_POSX:
     case FDC_POSY:
-        return enhanceAttr( nullptr, aRow, aCol, aKind );
+        break;
 
     case FDC_H_ALIGN:
         m_hAlignAttr->IncRef();
-        return enhanceAttr( m_hAlignAttr, aRow, aCol, aKind );
+        attr = m_hAlignAttr;
+        break;
 
     case FDC_V_ALIGN:
         m_vAlignAttr->IncRef();
-        return enhanceAttr( m_vAlignAttr, aRow, aCol, aKind );
+        attr = m_vAlignAttr;
+        break;
 
     case FDC_ORIENTATION:
         m_orientationAttr->IncRef();
-        return enhanceAttr( m_orientationAttr, aRow, aCol, aKind );
+        attr = m_orientationAttr;
+        break;
 
     case FDC_SHOWN:
     case FDC_SHOW_NAME:
@@ -648,20 +666,50 @@ wxGridCellAttr* FIELDS_GRID_TABLE::GetAttr( int aRow, int aCol, wxGridCellAttr::
     case FDC_ALLOW_AUTOPLACE:
     case FDC_PRIVATE:
         m_boolAttr->IncRef();
-        return enhanceAttr( m_boolAttr, aRow, aCol, aKind );
+        attr = m_boolAttr;
+        break;
 
     case FDC_FONT:
         m_fontAttr->IncRef();
-        return enhanceAttr( m_fontAttr, aRow, aCol, aKind );
+        attr = m_fontAttr;
+        break;
 
     case FDC_COLOR:
         m_colorAttr->IncRef();
-        return enhanceAttr( m_colorAttr, aRow, aCol, aKind );
+        attr = m_colorAttr;
+        break;
 
     default:
-        wxFAIL;
-        return enhanceAttr( nullptr, aRow, aCol, aKind );
+        attr = nullptr;
+        break;
     }
+
+    if( !attr )
+        return nullptr;
+
+    attr = enhanceAttr( attr, aRow, aCol, aKind );
+
+    if( IsInherited( aRow ) )
+    {
+        wxGridCellAttr* text_attr = attr ? attr->Clone() : new wxGridCellAttr;
+        wxFont font;
+
+        if( !text_attr->HasFont() )
+            font = wxSystemSettings::GetFont( wxSYS_DEFAULT_GUI_FONT );
+        else
+            font = text_attr->GetFont();
+
+        font.MakeItalic();
+        text_attr->SetFont( font );
+        text_attr->SetTextColour( wxSystemSettings::GetColour( wxSYS_COLOUR_GRAYTEXT ) );
+
+        if( attr )
+            attr->DecRef();
+
+        attr = text_attr;
+    }
+
+    return attr;
 }
 
 
@@ -1042,10 +1090,82 @@ bool FIELDS_GRID_TABLE::BoolFromString( const wxString& aValue ) const
 }
 
 
+SCH_FIELD* FIELDS_GRID_TABLE::GetField( MANDATORY_FIELD_T aFieldId )
+{
+    for( SCH_FIELD& field : *this )
+    {
+        if( field.GetId() == aFieldId )
+            return &field;
+    }
+
+    return nullptr;
+}
+
+
+int FIELDS_GRID_TABLE::GetFieldRow( MANDATORY_FIELD_T aFieldId )
+{
+    for( int ii = 0; ii < (int) this->size(); ++ii )
+    {
+        if( this->at( ii ).GetId() == aFieldId )
+            return ii;
+    }
+
+    return -1;
+}
+
+
+void FIELDS_GRID_TABLE::AddInheritedField( const SCH_FIELD& aParent )
+{
+    push_back( aParent );
+    back().SetParent( m_part );
+    m_isInherited.back() = true;
+    m_parentFields.back() = aParent;
+}
+
+
+bool FIELDS_GRID_TABLE::EraseRow( size_t aRow )
+{
+    if( m_isInherited.size() > aRow )
+    {
+        // You can't erase inherited fields, but you can reset them to the parent value.
+        if( m_isInherited[aRow] )
+        {
+            at( aRow ) = m_parentFields[aRow];
+            return false;
+        }
+
+        m_isInherited.erase( m_isInherited.begin() + aRow );
+    }
+
+    if( m_parentFields.size() > aRow )
+        m_parentFields.erase( m_parentFields.begin() + aRow );
+
+    std::vector<SCH_FIELD>::erase( begin() + aRow );
+    return true;
+}
+
+
+void FIELDS_GRID_TABLE::SwapRows( size_t a, size_t b )
+{
+    wxCHECK( a < this->size() && b < this->size(), /*void*/ );
+
+    std::swap( at( a ), at( b ) );
+
+    bool tmpInherited = m_isInherited[a];
+    m_isInherited[a] = m_isInherited[b];
+    m_isInherited[b] = tmpInherited;
+
+    std::swap( m_parentFields[a], m_parentFields[b] );
+}
+
+
 void FIELDS_GRID_TABLE::DetachFields()
 {
     for( SCH_FIELD& field : *this )
         field.SetParent( nullptr );
+
+    for( SCH_FIELD& field : m_parentFields )
+        field.SetParent( nullptr );
 }
 
 

eeschema/fields_grid_table.h

@@ -28,6 +28,7 @@
 #include <sch_symbol.h>
 #include <grid_tricks.h>
 #include <validators.h>
+#include <vector>
 
 class SCH_BASE_FRAME;
 class DIALOG_SHIM;
@@ -120,6 +121,37 @@ public:
     wxString StringFromBool( bool aValue ) const;
     bool BoolFromString( const wxString& aValue ) const;
 
+    SCH_FIELD* GetField( MANDATORY_FIELD_T aFieldId );
+    int GetFieldRow( MANDATORY_FIELD_T aFieldId );
+
+    void AddInheritedField( const SCH_FIELD& aParent );
+
+    void SetFieldInherited( size_t aRow, const SCH_FIELD& aParent )
+    {
+        m_isInherited.resize( aRow + 1, false );
+        m_parentFields.resize( aRow + 1 );
+        m_parentFields[aRow] = aParent;
+        m_isInherited[aRow] = true;
+    }
+
+
+    bool IsInherited( size_t aRow ) const
+    {
+        if( aRow >= m_isInherited.size() || aRow >= m_parentFields.size() )
+            return false;
+
+        return m_isInherited[aRow] && m_parentFields[aRow].GetText() == at( aRow ).GetText();
+    }
+
+    const SCH_FIELD& ParentField( size_t row ) const { return m_parentFields[row]; }
+
+    void push_back( const SCH_FIELD& field );
+    // For std::vector compatibility, but we don't use it directly.
+    void emplace_back( const SCH_FIELD& field ) { push_back( field ); }
+
+    bool EraseRow( size_t row );
+    void SwapRows( size_t a, size_t b );
+
     void DetachFields();
 
 protected:
@@ -164,6 +196,9 @@ private:
     wxGridCellAttr*   m_fontAttr;
     wxGridCellAttr*   m_colorAttr;
 
+    std::vector<bool>      m_isInherited;
+    std::vector<SCH_FIELD> m_parentFields;
+
     std::unique_ptr<NUMERIC_EVALUATOR>        m_eval;
     std::map< std::pair<int, int>, wxString > m_evalOriginal;
 };

eeschema/sch_field.cpp

@@ -47,6 +47,20 @@
 static const std::vector<KICAD_T> labelTypes = { SCH_LABEL_LOCATE_ANY_T };
 
 
+SCH_FIELD::SCH_FIELD() :
+        SCH_ITEM( nullptr, SCH_FIELD_T ),
+        EDA_TEXT( schIUScale, wxEmptyString ),
+        m_id( MANDATORY_FIELD_T::INVALID_FIELD ),
+        m_showName( false ),
+        m_allowAutoPlace( true ),
+        m_isGeneratedField( false ),
+        m_autoAdded( false ),
+        m_showInChooser( true ),
+        m_renderCacheValid( false ),
+        m_lastResolvedColor( COLOR4D::UNSPECIFIED )
+{
+}
+
 SCH_FIELD::SCH_FIELD( const VECTOR2I& aPos, int aFieldId, SCH_ITEM* aParent,
                       const wxString& aName ) :
         SCH_ITEM( aParent, SCH_FIELD_T ),

eeschema/sch_field.h

@@ -52,6 +52,8 @@ class SCH_TEXT;
 class SCH_FIELD : public SCH_ITEM, public EDA_TEXT
 {
 public:
+    SCH_FIELD();    // For std::map::operator[]
+
     SCH_FIELD( const VECTOR2I& aPos, int aFieldId, SCH_ITEM* aParent,
                const wxString& aName = wxEmptyString );