Bug fixes for multiple symbol instances in complex hierarchies

1) use SCH_COMPONENT::GetRef(), GetValue() and GetFootprint() when
instance-specific info is needed
2) update UpdateAllScreenReferences() to handle value and footprint.
3) BACKANNO is CvPcb's handler, not back annotation's handler.  Which
means it needs GUI behaviour, not back annotation behaviour.

Fixes https://gitlab.com/kicad/code/kicad/issues/5520

cvpcb/cvpcb_mainframe.cpp

@@ -587,17 +587,9 @@ void CVPCB_MAINFRAME::SetFootprintFilter( FOOTPRINTS_LISTBOX::FP_FILTER_T aFilte
     // Apply the filter accordingly
     switch( aAction )
     {
-    case CVPCB_MAINFRAME::FILTER_DISABLE:
+    case CVPCB_MAINFRAME::FILTER_DISABLE: m_filteringOptions &= ~option; break;
-        m_filteringOptions &= ~option;
+    case CVPCB_MAINFRAME::FILTER_ENABLE:  m_filteringOptions |= option;  break;
-        break;
+    case CVPCB_MAINFRAME::FILTER_TOGGLE:  m_filteringOptions ^= option;  break;
-
-    case CVPCB_MAINFRAME::FILTER_ENABLE:
-        m_filteringOptions |= option;
-        break;
-
-    case CVPCB_MAINFRAME::FILTER_TOGGLE:
-        m_filteringOptions ^= option;
-        break;
     }
 
     wxListEvent l_event;

eeschema/annotate.cpp

@@ -216,13 +216,13 @@ void SCH_EDIT_FRAME::AnnotateComponents( bool              aAnnotateSchematic,
 
             if( comp->GetUnitCount() > 1 )
                 msg.Printf( _( "Updated %s (unit %s) from %s to %s" ),
-                            comp->GetField( VALUE )->GetShownText(),
+                            comp->GetValue( sheet ),
                             LIB_PART::SubReference( comp->GetUnit(), false ),
                             prevRef,
                             newRef );
             else
                 msg.Printf( _( "Updated %s from %s to %s" ),
-                            comp->GetField( VALUE )->GetShownText(),
+                            comp->GetValue( sheet ),
                             prevRef,
                             newRef );
         }
@@ -230,12 +230,12 @@ void SCH_EDIT_FRAME::AnnotateComponents( bool              aAnnotateSchematic,
         {
             if( comp->GetUnitCount() > 1 )
                 msg.Printf( _( "Annotated %s (unit %s) as %s" ),
-                            comp->GetField( VALUE )->GetShownText(),
+                            comp->GetValue( sheet ),
                             LIB_PART::SubReference( comp->GetUnit(), false ),
                             newRef );
             else
                 msg.Printf( _( "Annotated %s as %s" ),
-                            comp->GetField( VALUE )->GetShownText(),
+                            comp->GetValue( sheet ),
                             newRef );
         }
 

eeschema/component_references_lister.cpp

@@ -686,8 +686,8 @@ SCH_REFERENCE::SCH_REFERENCE( SCH_COMPONENT* aComponent, LIB_PART* aLibPart,
 
     m_NumRef = -1;
 
-    if( aComponent->GetField( VALUE )->GetText().IsEmpty() )
+    if( aComponent->GetValue( &aSheetPath ).IsEmpty() )
-        aComponent->GetField( VALUE )->SetText( wxT( "~" ) );
+        aComponent->SetValue( &aSheetPath, wxT( "~" ) );
 
     m_Value = aComponent->GetValue( &aSheetPath );
 }

eeschema/dialogs/dialog_change_symbols.cpp

@@ -70,12 +70,13 @@ DIALOG_CHANGE_SYMBOLS::DIALOG_CHANGE_SYMBOLS( SCH_EDIT_FRAME* aParent, SCH_COMPO
 
     if( m_symbol )
     {
+        SCH_SHEET_PATH* currentSheet = &aParent->Schematic().CurrentSheet();
+
         label.Printf( m_matchBySelection->GetLabel(), verb );
         m_matchBySelection->SetLabel( label );
         m_newId->AppendText( FROM_UTF8( m_symbol->GetLibId().Format().c_str() ) );
-        m_specifiedReference->ChangeValue(
+        m_specifiedReference->ChangeValue( m_symbol->GetRef( currentSheet ) );
-                m_symbol->GetRef( &aParent->Schematic().CurrentSheet() ) );
+        m_specifiedValue->ChangeValue( m_symbol->GetValue( currentSheet ) );
-        m_specifiedValue->ChangeValue( m_symbol->GetField( VALUE )->GetText() );
         m_specifiedId->ChangeValue( FROM_UTF8( m_symbol->GetLibId().Format().c_str() ) );
     }
     else
@@ -216,15 +217,23 @@ bool DIALOG_CHANGE_SYMBOLS::isMatch( SCH_COMPONENT* aSymbol, SCH_SHEET_PATH* aIn
     wxCHECK( frame, false );
 
     if( m_matchAll->GetValue() )
+    {
         return true;
+    }
     else if( m_matchBySelection->GetValue() )
+    {
         return aSymbol == m_symbol;
+    }
     else if( m_matchByReference->GetValue() )
+    {
         return WildCompareString( m_specifiedReference->GetValue(), aSymbol->GetRef( aInstance ),
                                   false );
+    }
     else if( m_matchByValue->GetValue() )
-        return WildCompareString( m_specifiedValue->GetValue(),
+    {
-                aSymbol->GetField( VALUE )->GetText(), false );
+        return WildCompareString( m_specifiedValue->GetValue(), aSymbol->GetValue( aInstance ),
+                                  false );
+    }
     else if( m_matchById )
     {
         id.Parse( m_specifiedId->GetValue(), LIB_ID::ID_SCH );

eeschema/dialogs/dialog_edit_component_in_schematic.cpp

@@ -661,6 +661,11 @@ bool DIALOG_EDIT_COMPONENT_IN_SCHEMATIC::TransferDataFromWindow()
     // reference.
     m_comp->SetRef( &GetParent()->GetCurrentSheet(), m_fields->at( REFERENCE ).GetText() );
 
+    // Similar for Value and Footprint, except that the GUI behaviour is that they are kept
+    // in sync between multiple instances.
+    m_comp->SetValue( m_fields->at( VALUE ).GetText() );
+    m_comp->SetFootprint( m_fields->at( FOOTPRINT ).GetText() );
+
     m_comp->SetIncludeInBom( !m_cbExcludeFromBom->IsChecked() );
     m_comp->SetIncludeOnBoard( !m_cbExcludeFromBoard->IsChecked() );
 

eeschema/dialogs/dialog_edit_components_libid.cpp

@@ -756,7 +756,7 @@ bool DIALOG_EDIT_COMPONENTS_LIBID::TransferDataFromWindow()
 
             // If value is a proxy for the itemName then make sure it gets updated
             if( cmp.m_Component->GetLibId().GetLibItemName().wx_str() == value->GetText() )
-                value->SetText( id.GetLibItemName().wx_str() );
+                cmp.m_Component->SetValue( id.GetLibItemName().wx_str() );
 
             cmp.m_Component->SetLibId( id );
             cmp.m_Component->SetLibSymbol( symbol->Flatten().release() );

eeschema/dialogs/dialog_edit_one_field.cpp

@@ -393,10 +393,16 @@ void DIALOG_SCH_EDIT_ONE_FIELD::UpdateField( SCH_FIELD* aField, SCH_SHEET_PATH*
     SCH_ITEM*       parent = dynamic_cast<SCH_ITEM*>( aField->GetParent() );
     int             fieldType = aField->GetId();
 
-    if( parent && parent->Type() == SCH_COMPONENT_T && fieldType == REFERENCE )
+    if( parent && parent->Type() == SCH_COMPONENT_T )
     {
-        wxASSERT( aSheetPath );
+        SCH_COMPONENT* comp = static_cast<SCH_COMPONENT*>( parent );
-        static_cast<SCH_COMPONENT*>( parent )->SetRef( aSheetPath, m_text );
+
+        if( fieldType == REFERENCE )
+            comp->SetRef( aSheetPath, m_text );
+        else if( fieldType == VALUE )
+            comp->SetValue( m_text );
+        else if( fieldType == FOOTPRINT )
+            comp->SetFootprint( m_text );
     }
 
     bool positioningModified = false;

eeschema/netlist_exporters/netlist_exporter_cadstar.cpp

@@ -88,7 +88,7 @@ bool NETLIST_EXPORTER_CADSTAR::WriteNetlist( const wxString& aOutFileName, unsig
             ret |= fprintf( f, "%s     ", TO_UTF8( StartCmpDesc ) );
             ret |= fprintf( f, "%s", TO_UTF8( msg ) );
 
-            msg = component->GetField( VALUE )->GetShownText();
+            msg = component->GetValue( &sheetList[i] );
             msg.Replace( wxT( " " ), wxT( "_" ) );
             ret |= fprintf( f, "     \"%s\"", TO_UTF8( msg ) );
             ret |= fprintf( f, "     \"%s\"", TO_UTF8( footprint ) );

eeschema/netlist_exporters/netlist_exporter_generic.cpp

@@ -121,20 +121,20 @@ void NETLIST_EXPORTER_GENERIC::addComponentFields( XNODE* xcomp, SCH_COMPONENT*
                 // The lowest unit number wins.  User should only set fields in any one unit.
                 // remark: IsVoid() returns true for empty strings or the "~" string (empty
                 // field value)
-                if( !comp2->GetField( VALUE )->IsVoid()
+                if( !comp2->GetValue( &sheetList[i] ).IsEmpty()
                         && ( unit < minUnit || fields.value.IsEmpty() ) )
                 {
                     if( m_resolveTextVars )
-                        fields.value = comp2->GetField( VALUE )->GetShownText();
+                        fields.value = comp2->GetValue( &sheetList[i] );
                     else
                         fields.value = comp2->GetField( VALUE )->GetText();
                 }
 
-                if( !comp2->GetField( FOOTPRINT )->IsVoid()
+                if( !comp2->GetFootprint( &sheetList[i] ).IsEmpty()
                         && ( unit < minUnit || fields.footprint.IsEmpty() ) )
                 {
                     if( m_resolveTextVars )
-                        fields.footprint = comp2->GetField( FOOTPRINT )->GetShownText();
+                        fields.footprint = comp2->GetFootprint( &sheetList[i] );
                     else
                         fields.footprint = comp2->GetField( FOOTPRINT )->GetText();
                 }

eeschema/netlist_exporters/netlist_exporter_orcadpcb2.cpp

@@ -83,26 +83,20 @@ bool NETLIST_EXPORTER_ORCADPCB2::WriteNetlist( const wxString& aOutFileName,
             if( comp->GetPartRef() && comp->GetPartRef()->GetFootprints().GetCount() != 0  )
                 cmpList.push_back( SCH_REFERENCE( comp, comp->GetPartRef().get(), sheet ) );
 
-            if( !comp->GetField( FOOTPRINT )->IsVoid() )
+            footprint = comp->GetFootprint( &sheet );
-            {
-                footprint = comp->GetField( FOOTPRINT )->GetShownText();
             footprint.Replace( wxT( " " ), wxT( "_" ) );
-            }
-            else
-            {
-                footprint = wxT( "$noname" );
-            }
-
-            field = comp->GetRef( &sheetList[i] );
 
             ret |= fprintf( f, " ( %s %s",
-                            TO_UTF8( sheetList[i].PathAsString() + comp->m_Uuid.AsString() ),
+                            TO_UTF8( sheet.PathAsString() + comp->m_Uuid.AsString() ),
-                            TO_UTF8( footprint ) );
+                            TO_UTF8( footprint.IsEmpty() ? wxT( "$noname" ) : footprint ) );
+
+            field = comp->GetRef( &sheet );
 
             ret |= fprintf( f, "  %s", TO_UTF8( field ) );
 
-            field = comp->GetField( VALUE )->GetShownText();
+            field = comp->GetValue( &sheet );
             field.Replace( wxT( " " ), wxT( "_" ) );
+
             ret |= fprintf( f, " %s", TO_UTF8( field ) );
 
             ret |= fprintf( f, "\n" );

eeschema/sch_component.cpp

@@ -569,7 +569,7 @@ const wxString SCH_COMPONENT::GetValue( const SCH_SHEET_PATH* sheet ) const
 
     for( const COMPONENT_INSTANCE_REFERENCE& instance : m_instanceReferences )
     {
-        if( instance.m_Path == path && !instance.m_Footprint.IsEmpty() )
+        if( instance.m_Path == path && !instance.m_Value.IsEmpty() )
         {
             SCH_FIELD dummy( wxDefaultPosition, VALUE, const_cast<SCH_COMPONENT*>( this ) );
             dummy.SetText( instance.m_Value );
@@ -583,6 +583,16 @@ const wxString SCH_COMPONENT::GetValue( const SCH_SHEET_PATH* sheet ) const
 
 void SCH_COMPONENT::SetValue( const SCH_SHEET_PATH* sheet, const wxString& aValue )
 {
+    if( sheet == nullptr )
+    {
+        // Clear instance overrides and set primary field value
+        for( COMPONENT_INSTANCE_REFERENCE& instance : m_instanceReferences )
+            instance.m_Value = wxEmptyString;
+
+        m_Fields[ VALUE ].SetText( aValue );
+        return;
+    }
+
     KIID_PATH path = sheet->Path();
 
     // check to see if it is already there before inserting it
@@ -620,6 +630,16 @@ const wxString SCH_COMPONENT::GetFootprint( const SCH_SHEET_PATH* sheet ) const
 
 void SCH_COMPONENT::SetFootprint( const SCH_SHEET_PATH* sheet, const wxString& aFootprint )
 {
+    if( sheet == nullptr )
+    {
+        // Clear instance overrides and set primary field value
+        for( COMPONENT_INSTANCE_REFERENCE& instance : m_instanceReferences )
+            instance.m_Value = wxEmptyString;
+
+        m_Fields[ FOOTPRINT ].SetText( aFootprint );
+        return;
+    }
+
     KIID_PATH path = sheet->Path();
 
     // check to see if it is already there before inserting it
@@ -1298,21 +1318,18 @@ void SCH_COMPONENT::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, MSG_PANEL_ITEMS& aL
     wxString msg;
 
     SCH_EDIT_FRAME* schframe = dynamic_cast<SCH_EDIT_FRAME*>( aFrame );
+    SCH_SHEET_PATH* currentSheet = schframe ? &schframe->GetCurrentSheet() : nullptr;
 
     // part and alias can differ if alias is not the root
     if( m_part )
     {
         if( m_part.get() != dummy() )
         {
-            if( schframe )
+            aList.push_back( MSG_PANEL_ITEM( _( "Reference" ), GetRef( currentSheet ), DARKCYAN ) );
-            {
-                aList.push_back( MSG_PANEL_ITEM(
-                        _( "Reference" ), GetRef( &schframe->GetCurrentSheet() ), DARKCYAN ) );
-            }
 
             msg = m_part->IsPower() ? _( "Power symbol" ) : _( "Value" );
 
-            aList.push_back( MSG_PANEL_ITEM( msg, GetField( VALUE )->GetShownText(), DARKCYAN ) );
+            aList.push_back( MSG_PANEL_ITEM( msg, GetValue( currentSheet ), DARKCYAN ) );
 
 #if 0       // Display component flags, for debug only
             aList.push_back( MSG_PANEL_ITEM( _( "flags" ),
@@ -1344,7 +1361,7 @@ void SCH_COMPONENT::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, MSG_PANEL_ITEMS& aL
             }
 
             // Display the current associated footprint, if exists.
-            msg = GetFootprint( &schframe->GetCurrentSheet() );
+            msg = GetFootprint( currentSheet );
 
             if( msg.IsEmpty() )
                 msg = _( "<Unknown>" );
@@ -1359,14 +1376,9 @@ void SCH_COMPONENT::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, MSG_PANEL_ITEMS& aL
     }
     else
     {
-        if( schframe )
+        aList.push_back( MSG_PANEL_ITEM( _( "Reference" ), GetRef( currentSheet ), DARKCYAN ) );
-        {
-            aList.push_back( MSG_PANEL_ITEM(
-                    _( "Reference" ), GetRef( &schframe->GetCurrentSheet() ), DARKCYAN ) );
-        }
 
-        aList.push_back( MSG_PANEL_ITEM( _( "Value" ), GetField( VALUE )->GetShownText(),
+        aList.push_back( MSG_PANEL_ITEM( _( "Value" ), GetValue( currentSheet ), DARKCYAN ) );
-                                         DARKCYAN ) );
         aList.push_back( MSG_PANEL_ITEM( _( "Name" ), GetLibId().GetLibItemName(), BROWN ) );
 
         wxString libNickname = GetLibId().GetLibNickname();

eeschema/sch_component.h

@@ -556,10 +556,22 @@ public:
     const wxString GetValue( const SCH_SHEET_PATH* sheet ) const;
     void SetValue( const SCH_SHEET_PATH* sheet, const wxString& aValue );
 
+    // Set the value for all instances (the default GUI behaviour)
+    void SetValue( const wxString& aValue )
+    {
+        SetValue( nullptr, aValue );
+    }
+
     // Returns the instance-specific footprint assignment for the given sheet path.
     const wxString GetFootprint( const SCH_SHEET_PATH* sheet ) const;
     void SetFootprint( const SCH_SHEET_PATH* sheet, const wxString& aFootprint );
 
+    // Set the value for all instances (the default GUI behaviour)
+    void SetFootprint( const wxString& aFootprint )
+    {
+        SetFootprint( nullptr, aFootprint );
+    }
+
     // Geometric transforms (used in block operations):
 
     void Move( const wxPoint& aMoveVector ) override

eeschema/sch_pin.cpp

@@ -183,8 +183,11 @@ void SCH_PIN::GetMsgPanelInfo( EDA_DRAW_FRAME* aFrame, MSG_PANEL_ITEMS& aList )
     msg = MessageTextFromValue( aFrame->GetUserUnits(), m_position.y, true );
     aList.emplace_back( _( "Pos Y" ), msg, DARKMAGENTA );
 
-    aList.emplace_back( GetParentComponent()->GetField( REFERENCE )->GetShownText(),
+    SCH_EDIT_FRAME* schframe = dynamic_cast<SCH_EDIT_FRAME*>( aFrame );
-                        GetParentComponent()->GetField( VALUE )->GetShownText(), DARKCYAN );
+    SCH_SHEET_PATH* currentSheet = schframe ? &schframe->GetCurrentSheet() : nullptr;
+    SCH_COMPONENT*  comp = GetParentComponent();
+
+    aList.emplace_back( comp->GetRef( currentSheet ), comp->GetValue( currentSheet ), DARKCYAN );
 
 #if defined(DEBUG)
 

eeschema/sch_sheet_path.cpp

@@ -248,6 +248,8 @@ void SCH_SHEET_PATH::UpdateAllScreenReferences()
     {
         auto component = static_cast<SCH_COMPONENT*>( item );
         component->GetField( REFERENCE )->SetText( component->GetRef( this ) );
+        component->GetField( VALUE )->SetText( component->GetValue( this ) );
+        component->GetField( FOOTPRINT )->SetText( component->GetFootprint( this ) );
         component->UpdateUnit( component->GetUnitSelection( this ) );
     }
 }

eeschema/tools/backanno.cpp

@@ -86,7 +86,9 @@ void SCH_EDITOR_CONTROL::BackAnnotateFootprints( const std::string& aChangedSetO
                     // Note: it can be not unique (multiple parts per package)
                     // So we *do not* stop the search here
                     SCH_COMPONENT*  component = refs[ii].GetComp();
-                    SCH_SHEET_PATH* sheetPath = &refs[ii].GetSheetPath();
+                    // For backwards-compatibility CvPcb currently updates all instances of a
+                    // component (even though it lists these instances separately).
+                    SCH_SHEET_PATH* sheetPath = nullptr;    // &refs[ii].GetSheetPath();
                     wxString        oldfp = refs[ii].GetFootprint();
 
                     if( oldfp.IsEmpty() && component->GetField( FOOTPRINT )->IsVisible() )