From 7b05e456cc5a96e8cdd5bf08b7cb9cc01380b70c Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Sun, 6 Sep 2020 13:05:07 +0100 Subject: [PATCH] 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 | 14 ++----- eeschema/annotate.cpp | 8 ++-- eeschema/component_references_lister.cpp | 4 +- eeschema/dialogs/dialog_change_symbols.cpp | 21 +++++++--- .../dialog_edit_component_in_schematic.cpp | 5 +++ .../dialogs/dialog_edit_components_libid.cpp | 2 +- eeschema/dialogs/dialog_edit_one_field.cpp | 12 ++++-- .../netlist_exporter_cadstar.cpp | 2 +- .../netlist_exporter_generic.cpp | 8 ++-- .../netlist_exporter_orcadpcb2.cpp | 22 ++++------ eeschema/sch_component.cpp | 42 ++++++++++++------- eeschema/sch_component.h | 12 ++++++ eeschema/sch_pin.cpp | 7 +++- eeschema/sch_sheet_path.cpp | 2 + eeschema/tools/backanno.cpp | 4 +- 15 files changed, 101 insertions(+), 64 deletions(-) diff --git a/cvpcb/cvpcb_mainframe.cpp b/cvpcb/cvpcb_mainframe.cpp index afa2ddde37..86364aea56 100644 --- a/cvpcb/cvpcb_mainframe.cpp +++ b/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: - m_filteringOptions &= ~option; - break; - - case CVPCB_MAINFRAME::FILTER_ENABLE: - m_filteringOptions |= option; - break; - - case CVPCB_MAINFRAME::FILTER_TOGGLE: - m_filteringOptions ^= option; - break; + case CVPCB_MAINFRAME::FILTER_DISABLE: 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; diff --git a/eeschema/annotate.cpp b/eeschema/annotate.cpp index 1020369e03..05d5444aeb 100644 --- a/eeschema/annotate.cpp +++ b/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 ); } diff --git a/eeschema/component_references_lister.cpp b/eeschema/component_references_lister.cpp index 09235f70f9..3755c07b0d 100644 --- a/eeschema/component_references_lister.cpp +++ b/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() ) - aComponent->GetField( VALUE )->SetText( wxT( "~" ) ); + if( aComponent->GetValue( &aSheetPath ).IsEmpty() ) + aComponent->SetValue( &aSheetPath, wxT( "~" ) ); m_Value = aComponent->GetValue( &aSheetPath ); } diff --git a/eeschema/dialogs/dialog_change_symbols.cpp b/eeschema/dialogs/dialog_change_symbols.cpp index a5d1dc853a..c481d99780 100644 --- a/eeschema/dialogs/dialog_change_symbols.cpp +++ b/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_symbol->GetRef( &aParent->Schematic().CurrentSheet() ) ); - m_specifiedValue->ChangeValue( m_symbol->GetField( VALUE )->GetText() ); + m_specifiedReference->ChangeValue( m_symbol->GetRef( currentSheet ) ); + m_specifiedValue->ChangeValue( m_symbol->GetValue( currentSheet ) ); 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 ); + 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 ); diff --git a/eeschema/dialogs/dialog_edit_component_in_schematic.cpp b/eeschema/dialogs/dialog_edit_component_in_schematic.cpp index 0e9bc6912a..c76963a14f 100644 --- a/eeschema/dialogs/dialog_edit_component_in_schematic.cpp +++ b/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() ); diff --git a/eeschema/dialogs/dialog_edit_components_libid.cpp b/eeschema/dialogs/dialog_edit_components_libid.cpp index 22c19380f4..6257b72700 100644 --- a/eeschema/dialogs/dialog_edit_components_libid.cpp +++ b/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() ); diff --git a/eeschema/dialogs/dialog_edit_one_field.cpp b/eeschema/dialogs/dialog_edit_one_field.cpp index 3cf80db322..15799c8a20 100644 --- a/eeschema/dialogs/dialog_edit_one_field.cpp +++ b/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( aField->GetParent() ); int fieldType = aField->GetId(); - if( parent && parent->Type() == SCH_COMPONENT_T && fieldType == REFERENCE ) + if( parent && parent->Type() == SCH_COMPONENT_T ) { - wxASSERT( aSheetPath ); - static_cast( parent )->SetRef( aSheetPath, m_text ); + SCH_COMPONENT* comp = static_cast( parent ); + + 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; diff --git a/eeschema/netlist_exporters/netlist_exporter_cadstar.cpp b/eeschema/netlist_exporters/netlist_exporter_cadstar.cpp index f80880e124..a160de9eec 100644 --- a/eeschema/netlist_exporters/netlist_exporter_cadstar.cpp +++ b/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 ) ); diff --git a/eeschema/netlist_exporters/netlist_exporter_generic.cpp b/eeschema/netlist_exporters/netlist_exporter_generic.cpp index 5c83d209af..c838c38288 100644 --- a/eeschema/netlist_exporters/netlist_exporter_generic.cpp +++ b/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(); } diff --git a/eeschema/netlist_exporters/netlist_exporter_orcadpcb2.cpp b/eeschema/netlist_exporters/netlist_exporter_orcadpcb2.cpp index 6a0415b165..7dd85e3062 100644 --- a/eeschema/netlist_exporters/netlist_exporter_orcadpcb2.cpp +++ b/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->GetField( FOOTPRINT )->GetShownText(); - footprint.Replace( wxT( " " ), wxT( "_" ) ); - } - else - { - footprint = wxT( "$noname" ); - } - - field = comp->GetRef( &sheetList[i] ); + footprint = comp->GetFootprint( &sheet ); + footprint.Replace( wxT( " " ), wxT( "_" ) ); ret |= fprintf( f, " ( %s %s", - TO_UTF8( sheetList[i].PathAsString() + comp->m_Uuid.AsString() ), - TO_UTF8( footprint ) ); + TO_UTF8( sheet.PathAsString() + comp->m_Uuid.AsString() ), + 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" ); diff --git a/eeschema/sch_component.cpp b/eeschema/sch_component.cpp index c2cbd96150..1054d6b22b 100644 --- a/eeschema/sch_component.cpp +++ b/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( 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( 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( &schframe->GetCurrentSheet() ), DARKCYAN ) ); - } + aList.push_back( MSG_PANEL_ITEM( _( "Reference" ), GetRef( currentSheet ), 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 = _( "" ); @@ -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( &schframe->GetCurrentSheet() ), DARKCYAN ) ); - } + aList.push_back( MSG_PANEL_ITEM( _( "Reference" ), GetRef( currentSheet ), DARKCYAN ) ); - aList.push_back( MSG_PANEL_ITEM( _( "Value" ), GetField( VALUE )->GetShownText(), - DARKCYAN ) ); + aList.push_back( MSG_PANEL_ITEM( _( "Value" ), GetValue( currentSheet ), DARKCYAN ) ); aList.push_back( MSG_PANEL_ITEM( _( "Name" ), GetLibId().GetLibItemName(), BROWN ) ); wxString libNickname = GetLibId().GetLibNickname(); diff --git a/eeschema/sch_component.h b/eeschema/sch_component.h index fa8933989e..26dd966ed2 100644 --- a/eeschema/sch_component.h +++ b/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 diff --git a/eeschema/sch_pin.cpp b/eeschema/sch_pin.cpp index d60b89dc48..2e6eabaae9 100644 --- a/eeschema/sch_pin.cpp +++ b/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(), - GetParentComponent()->GetField( VALUE )->GetShownText(), DARKCYAN ); + SCH_EDIT_FRAME* schframe = dynamic_cast( aFrame ); + 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) diff --git a/eeschema/sch_sheet_path.cpp b/eeschema/sch_sheet_path.cpp index 509e60639a..9f04460daa 100644 --- a/eeschema/sch_sheet_path.cpp +++ b/eeschema/sch_sheet_path.cpp @@ -248,6 +248,8 @@ void SCH_SHEET_PATH::UpdateAllScreenReferences() { auto component = static_cast( 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 ) ); } } diff --git a/eeschema/tools/backanno.cpp b/eeschema/tools/backanno.cpp index 61dbf9685c..b57dab411b 100644 --- a/eeschema/tools/backanno.cpp +++ b/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() )