From 0c91bea75160126e9e5eaf52056afcd976772048 Mon Sep 17 00:00:00 2001 From: Roberto Fernandez Bautista Date: Sun, 11 Apr 2021 16:24:11 +0100 Subject: [PATCH] Fix eeschema reannotation: ensure no duplicate references are created Also implements reannotation of a selection ADDED: Ability to reannotate a selection in eeschema Fixes https://gitlab.com/kicad/code/kicad/-/issues/2209 --- eeschema/annotate.cpp | 139 ++++++++++++++++++----- eeschema/component_references_lister.cpp | 38 ++++++- eeschema/dialogs/dialog_annotate.cpp | 4 +- eeschema/sch_edit_frame.h | 22 ++-- eeschema/sch_reference_list.h | 17 ++- eeschema/sch_sheet_path.cpp | 3 +- eeschema/tools/ee_selection.cpp | 65 +++++++++++ eeschema/tools/ee_selection.h | 31 ++++- 8 files changed, 271 insertions(+), 48 deletions(-) diff --git a/eeschema/annotate.cpp b/eeschema/annotate.cpp index e6a4902f4b..530d5eb75f 100644 --- a/eeschema/annotate.cpp +++ b/eeschema/annotate.cpp @@ -30,6 +30,9 @@ #include #include #include +#include +#include +#include void SCH_EDIT_FRAME::mapExistingAnnotation( std::map& aMap ) @@ -57,29 +60,55 @@ void SCH_EDIT_FRAME::mapExistingAnnotation( std::map& aMap ) } -void SCH_EDIT_FRAME::DeleteAnnotation( bool aCurrentSheetOnly, bool* aAppendUndo ) +void SCH_EDIT_FRAME::DeleteAnnotation( ANNOTATE_SCOPE_T aAnnotateScope, bool* aAppendUndo ) { - auto clearAnnotation = + auto clearSymbolAnnotation = + [&]( EDA_ITEM* aItem, SCH_SCREEN* aScreen, SCH_SHEET_PATH* aSheet ) + { + SCH_COMPONENT* symbol = static_cast( aItem ); + + SaveCopyInUndoList( aScreen, symbol, UNDO_REDO::CHANGED, *aAppendUndo ); + *aAppendUndo = true; + symbol->ClearAnnotation( aSheet ); + }; + + auto clearSheetAnnotation = [&]( SCH_SCREEN* aScreen, SCH_SHEET_PATH* aSheet ) { for( SCH_ITEM* item : aScreen->Items().OfType( SCH_COMPONENT_T ) ) - { - SCH_COMPONENT* symbol = static_cast( item ); - - SaveCopyInUndoList( aScreen, symbol, UNDO_REDO::CHANGED, *aAppendUndo ); - *aAppendUndo = true; - symbol->ClearAnnotation( aSheet ); - } + clearSymbolAnnotation( item, aScreen, aSheet ); }; - if( aCurrentSheetOnly ) + SCH_SCREEN* screen = GetScreen(); + SCH_SHEET_PATH currentSheet = GetCurrentSheet(); + + switch( aAnnotateScope ) { - clearAnnotation( GetScreen(), &GetCurrentSheet() ); - } - else + case ANNOTATE_ALL: { for( const SCH_SHEET_PATH& sheet : Schematic().GetSheets() ) - clearAnnotation( sheet.LastScreen(), nullptr ); + clearSheetAnnotation( sheet.LastScreen(), nullptr ); + + break; + } + case ANNOTATE_CURRENT_SHEET: + { + clearSheetAnnotation( screen, ¤tSheet ); + break; + } + + case ANNOTATE_SELECTION: + { + EE_SELECTION_TOOL* selTool = m_toolManager->GetTool(); + EE_SELECTION& selection = selTool->RequestSelection(); + + for( EDA_ITEM* item : selection.Items() ) + { + if( item->Type() == SCH_COMPONENT_T ) + clearSymbolAnnotation( item, screen, ¤tSheet ); + } + break; + } } // Update the references for the sheet that is currently being displayed. @@ -94,7 +123,7 @@ void SCH_EDIT_FRAME::DeleteAnnotation( bool aCurrentSheetOnly, bool* aAppendUndo } -void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, +void SCH_EDIT_FRAME::AnnotateSymbols( ANNOTATE_SCOPE_T aAnnotateScope, ANNOTATE_ORDER_T aSortOption, ANNOTATE_ALGO_T aAlgoOption, int aStartNumber, @@ -103,9 +132,13 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, bool aLockUnits, REPORTER& aReporter ) { + EE_SELECTION_TOOL* selTool = m_toolManager->GetTool(); + EE_SELECTION& selection = selTool->RequestSelection(); + SCH_REFERENCE_LIST references; SCH_SCREENS screens( Schematic().Root() ); SCH_SHEET_LIST sheets = Schematic().GetSheets(); + SCH_SHEET_PATH currentSheet = GetCurrentSheet(); bool appendUndo = false; // Map of locked symbols @@ -132,10 +165,20 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, // If units must be locked, collect all the sets that must be annotated together. if( aLockUnits ) { - if( aAnnotateSchematic ) + switch( aAnnotateScope ) + { + case ANNOTATE_ALL: sheets.GetMultiUnitSymbols( lockedSymbols ); - else - GetCurrentSheet().GetMultiUnitSymbols( lockedSymbols ); + break; + + case ANNOTATE_CURRENT_SHEET: + currentSheet.GetMultiUnitSymbols( lockedSymbols ); + break; + + case ANNOTATE_SELECTION: + selection.GetMultiUnitSymbols( lockedSymbols, currentSheet ); + break; + } } // Store previous annotations for building info messages @@ -143,16 +186,43 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, // If it is an annotation for all the symbols, reset previous annotation. if( aResetAnnotation ) - DeleteAnnotation( !aAnnotateSchematic, &appendUndo ); + DeleteAnnotation( aAnnotateScope, &appendUndo ); // Set sheet number and number of sheets. SetSheetNumberAndCount(); // Build symbol list - if( aAnnotateSchematic ) + switch( aAnnotateScope ) + { + case ANNOTATE_ALL: sheets.GetSymbols( references ); - else + break; + + case ANNOTATE_CURRENT_SHEET: GetCurrentSheet().GetSymbols( references ); + break; + + case ANNOTATE_SELECTION: + selection.GetSymbols( references, currentSheet ); + break; + } + + // Build additional list of references to be used during reannotation + // to avoid duplicate designators (no additional references when annotating + // the full schematic) + SCH_REFERENCE_LIST additionalRefs; + + if( aAnnotateScope != ANNOTATE_ALL ) + { + SCH_REFERENCE_LIST allRefs; + sheets.GetSymbols( allRefs ); + + for( size_t i = 0; i < allRefs.GetCount(); i++ ) + { + if( !references.Contains( allRefs[i] ) ) + additionalRefs.AddItem( allRefs[i] ); + } + } // Break full symbol reference into name (prefix) and number: // example: IC1 become IC, and 1 @@ -185,7 +255,7 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, } // Recalculate and update reference numbers in schematic - references.Annotate( useSheetNum, idStep, aStartNumber, lockedSymbols ); + references.Annotate( useSheetNum, idStep, aStartNumber, lockedSymbols, additionalRefs ); for( size_t i = 0; i < references.GetCount(); i++ ) { @@ -247,7 +317,7 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, { aReporter.Report( aMsg, RPT_SEVERITY_ERROR ); }, - !aAnnotateSchematic ) ) + aAnnotateScope ) ) { aReporter.ReportTail( _( "Annotation complete." ), RPT_SEVERITY_ACTION ); } @@ -265,16 +335,29 @@ void SCH_EDIT_FRAME::AnnotateSymbols( bool aAnnotateSchematic, } -int SCH_EDIT_FRAME::CheckAnnotate( ANNOTATION_ERROR_HANDLER aErrorHandler, bool aOneSheetOnly ) +int SCH_EDIT_FRAME::CheckAnnotate( ANNOTATION_ERROR_HANDLER aErrorHandler, + ANNOTATE_SCOPE_T aAnnotateScope ) { SCH_REFERENCE_LIST referenceList; constexpr bool includePowerSymbols = false; // Build the list of symbols - if( !aOneSheetOnly ) - Schematic().GetSheets().GetSymbols( referenceList, includePowerSymbols ); - else - GetCurrentSheet().GetSymbols( referenceList ); + switch( aAnnotateScope ) + { + case ANNOTATE_ALL: + Schematic().GetSheets().GetSymbols( referenceList ); + break; + + case ANNOTATE_CURRENT_SHEET: + GetCurrentSheet().GetSymbols( referenceList, includePowerSymbols ); + break; + + case ANNOTATE_SELECTION: + EE_SELECTION_TOOL* selTool = m_toolManager->GetTool(); + EE_SELECTION& selection = selTool->RequestSelection(); + selection.GetSymbols( referenceList, GetCurrentSheet(), includePowerSymbols ); + break; + } // Empty schematic does not need annotation if( referenceList.GetCount() == 0 ) diff --git a/eeschema/component_references_lister.cpp b/eeschema/component_references_lister.cpp index c33f2149c3..0025bb45d6 100644 --- a/eeschema/component_references_lister.cpp +++ b/eeschema/component_references_lister.cpp @@ -48,6 +48,18 @@ void SCH_REFERENCE_LIST::RemoveItem( unsigned int aIndex ) } +bool SCH_REFERENCE_LIST::Contains( const SCH_REFERENCE& aItem ) +{ + for( unsigned ii = 0; ii < GetCount(); ii++ ) + { + if( flatList[ii].IsSameInstance( aItem ) ) + return true; + } + + return false; +} + + bool SCH_REFERENCE_LIST::sortByXPosition( const SCH_REFERENCE& item1, const SCH_REFERENCE& item2 ) { int ii = item1.CompareRef( item2 ); @@ -192,7 +204,8 @@ void SCH_REFERENCE_LIST::GetRefsInUse( int aIndex, std::vector< int >& aIdList, for( const SCH_REFERENCE& ref : flatList ) { - if( flatList[aIndex].CompareRef( ref ) == 0 && ref.m_numRef >= aMinRefId ) + // Don't add new references to the list as we will reannotate those + if( flatList[aIndex].CompareRef( ref ) == 0 && ref.m_numRef >= aMinRefId && !ref.m_isNew ) aIdList.push_back( ref.m_numRef ); } @@ -278,11 +291,24 @@ wxString buildFullReference( const SCH_REFERENCE& aItem, int aUnitNumber = -1 ) void SCH_REFERENCE_LIST::Annotate( bool aUseSheetNum, int aSheetIntervalId, int aStartNumber, - SCH_MULTI_UNIT_REFERENCE_MAP aLockedUnitMap ) + SCH_MULTI_UNIT_REFERENCE_MAP aLockedUnitMap, + const SCH_REFERENCE_LIST& aAdditionalRefs ) { if ( flatList.size() == 0 ) return; + int originalSize = GetCount(); + + // We don't want to reannotate the additional references even if not annotated + // so we change the m_isNew flag to be false after splitting + for( size_t i = 0; i < aAdditionalRefs.GetCount(); i++ ) + { + SCH_REFERENCE additionalRef = aAdditionalRefs[i]; + additionalRef.Split(); + additionalRef.m_isNew = false; + AddItem( additionalRef ); //add to this container + } + int LastReferenceNumber = 0; int NumberOfUnits, Unit; @@ -383,7 +409,7 @@ void SCH_REFERENCE_LIST::Annotate( bool aUseSheetNum, int aSheetIntervalId, int LastReferenceNumber = CreateFirstFreeRefId( idList, minRefId ); ref_unit.m_numRef = LastReferenceNumber; - if( !ref_unit.IsUnitsLocked() ) + if( !ref_unit.IsUnitsLocked() && !lockedList ) ref_unit.m_unit = 1; ref_unit.m_flag = 1; @@ -491,6 +517,12 @@ void SCH_REFERENCE_LIST::Annotate( bool aUseSheetNum, int aSheetIntervalId, int } } } + + // Remove aAdditionalRefs references + for( int i = originalSize; i < ( aAdditionalRefs.GetCount() + originalSize ); i++ ) + RemoveItem( originalSize ); + + wxASSERT( originalSize == GetCount() ); // Make sure we didn't make a mistake } int SCH_REFERENCE_LIST::CheckAnnotation( ANNOTATION_ERROR_HANDLER aHandler ) diff --git a/eeschema/dialogs/dialog_annotate.cpp b/eeschema/dialogs/dialog_annotate.cpp index 1a8377ca6b..9ef04d1b4e 100644 --- a/eeschema/dialogs/dialog_annotate.cpp +++ b/eeschema/dialogs/dialog_annotate.cpp @@ -172,7 +172,7 @@ void DIALOG_ANNOTATE::OnApplyClick( wxCommandEvent& event ) REPORTER& reporter = m_MessageWindow->Reporter(); m_MessageWindow->SetLazyUpdate( true ); // Don't update after each message - m_Parent->AnnotateSymbols( GetScope() == ANNOTATE_ALL, GetSortOrder(), GetAnnotateAlgo(), + m_Parent->AnnotateSymbols( GetScope(), GetSortOrder(), GetAnnotateAlgo(), GetStartNumber(), GetResetItems(), true, GetLockUnits(), reporter ); m_MessageWindow->Flush( true ); // Now update to show all messages @@ -199,7 +199,7 @@ void DIALOG_ANNOTATE::OnClearAnnotationClick( wxCommandEvent& event ) { bool appendUndo = false; - m_Parent->DeleteAnnotation( GetScope() == ANNOTATE_CURRENT_SHEET, &appendUndo ); + m_Parent->DeleteAnnotation( GetScope(), &appendUndo ); m_btnClear->Enable( false ); } diff --git a/eeschema/sch_edit_frame.h b/eeschema/sch_edit_frame.h index 5e974a6e67..026f2a2eca 100644 --- a/eeschema/sch_edit_frame.h +++ b/eeschema/sch_edit_frame.h @@ -82,9 +82,9 @@ enum COMPONENT_ORIENTATION_T /** Schematic annotation scope options. */ enum ANNOTATE_SCOPE_T { - ANNOTATE_ALL, - ANNOTATE_CURRENT_SHEET, - ANNOTATE_SELECTION + ANNOTATE_ALL, ///< Annotate the full schematic + ANNOTATE_CURRENT_SHEET, ///< Annotate the current sheet + ANNOTATE_SELECTION ///< Annotate the selection }; @@ -372,16 +372,15 @@ public: /** * Clear the current component annotation. * - * @param aCurrentSheetOnly Clear only the annotation for the current sheet if true. - * Otherwise clear the entire schematic annotation. + * @param aCurrentSheetOnly Where to clear the annotation. See #ANNOTATE_SCOPE_T + * @param appendUndo true to add the action to the previous undo list */ - void DeleteAnnotation( bool aCurrentSheetOnly, bool* appendUndo ); + void DeleteAnnotation( ANNOTATE_SCOPE_T aAnnotateScope, bool* appendUndo ); /** * Annotate the symbols in the schematic that are not currently annotated. * - * @param aAnnotateSchematic Annotate the entire schematic if true. Otherwise annotate - * the current sheet only. + * @param aAnnotateScope See #ANNOTATE_SCOPE_T * @param aSortOption Define the annotation order. See #ANNOTATE_ORDER_T. * @param aAlgoOption Define the annotation style. See #ANNOTATE_ALGO_T. * @param aStartNumber The start number for non-sheet-based annotation styles. @@ -404,7 +403,7 @@ public: * number * 100. In other words the first sheet uses 100 to 199, the second sheet uses * 200 to 299, and so on. */ - void AnnotateSymbols( bool aAnnotateSchematic, ANNOTATE_ORDER_T aSortOption, + void AnnotateSymbols( ANNOTATE_SCOPE_T aAnnotateScope, ANNOTATE_ORDER_T aSortOption, ANNOTATE_ALGO_T aAlgoOption, int aStartNumber, bool aResetAnnotation, bool aRepairTimestamps, bool aLockUnits, REPORTER& aReporter ); @@ -421,10 +420,11 @@ public: * * @return Number of annotation errors found. * @param aReporter A handler for error reporting. - * @param aOneSheetOnly Check the current sheet only if true. Otherwise check the entire + * @param aAnnotateScope See #ANNOTATE_SCOPE_T Check the current sheet only if true. Otherwise check the entire * schematic. */ - int CheckAnnotate( ANNOTATION_ERROR_HANDLER aErrorHandler, bool aOneSheetOnly = false ); + int CheckAnnotate( ANNOTATION_ERROR_HANDLER aErrorHandler, + ANNOTATE_SCOPE_T aAnnotateScope = ANNOTATE_ALL ); /** * Run a modal version of the annotate dialog for a specific purpose. diff --git a/eeschema/sch_reference_list.h b/eeschema/sch_reference_list.h index 4876d81433..3d636a856b 100644 --- a/eeschema/sch_reference_list.h +++ b/eeschema/sch_reference_list.h @@ -149,7 +149,9 @@ public: */ bool IsSameInstance( const SCH_REFERENCE& other ) const { - // JEY TODO: should this be checking unit as well? + // Only compare symbol and path. + // We may have changed the unit number or the designator but + // can still be referencing the same instance. return GetSymbol() == other.GetSymbol() && GetSheetPath().Path() == other.GetSheetPath().Path(); } @@ -235,6 +237,13 @@ public: */ void RemoveItem( unsigned int aIndex ); + /** + * Return true if aItem exists in this list + * @param aItem Reference to check + * @return true if aItem exists in this list + */ + bool Contains( const SCH_REFERENCE& aItem ); + /* Sort functions: * Sort functions are used to sort symbols for annotation or BOM generation. Because * sorting depends on what we want to do, there are many sort functions. @@ -288,9 +297,13 @@ public: * @param aLockedUnitMap A SCH_MULTI_UNIT_REFERENCE_MAP of reference designator wxStrings * to SCH_REFERENCE_LISTs. May be an empty map. If not empty, any multi-unit parts * found in this map will be annotated as a group rather than individually. + * @param aAdditionalRefs Additional references to use for checking that there a reference + * designator doesn't already exist. The caller must ensure that none of the references + * in aAdditionalRefs exist in this list. */ void Annotate( bool aUseSheetNum, int aSheetIntervalId, int aStartNumber, - SCH_MULTI_UNIT_REFERENCE_MAP aLockedUnitMap ); + SCH_MULTI_UNIT_REFERENCE_MAP aLockedUnitMap, + const SCH_REFERENCE_LIST& aAdditionalRefs ); /** * Check for annotations errors. diff --git a/eeschema/sch_sheet_path.cpp b/eeschema/sch_sheet_path.cpp index dbb2b4ef97..26f3a206c4 100644 --- a/eeschema/sch_sheet_path.cpp +++ b/eeschema/sch_sheet_path.cpp @@ -613,6 +613,7 @@ void SCH_SHEET_LIST::AnnotatePowerSymbols() { // List of reference for power symbols SCH_REFERENCE_LIST references; + SCH_REFERENCE_LIST additionalreferences; // Todo: add as a parameter to this function // Map of locked symbols (not used, but needed by Annotate() SCH_MULTI_UNIT_REFERENCE_MAP lockedSymbols; @@ -674,7 +675,7 @@ void SCH_SHEET_LIST::AnnotatePowerSymbols() } // Recalculate and update reference numbers in schematic - references.Annotate( false, 0, 100, lockedSymbols ); + references.Annotate( false, 0, 100, lockedSymbols, additionalreferences ); references.UpdateAnnotation(); } diff --git a/eeschema/tools/ee_selection.cpp b/eeschema/tools/ee_selection.cpp index 2803264511..fdced379b8 100644 --- a/eeschema/tools/ee_selection.cpp +++ b/eeschema/tools/ee_selection.cpp @@ -24,6 +24,8 @@ #include #include +#include +#include #include #include @@ -77,6 +79,69 @@ EDA_RECT EE_SELECTION::GetBoundingBox() const } +void EE_SELECTION::GetSymbols( SCH_REFERENCE_LIST& aReferences, + const SCH_SHEET_PATH& aSelectionPath, + bool aIncludePowerSymbols, + bool aForceIncludeOrphanSymbols ) +{ + for( EDA_ITEM* item : Items() ) + { + if( item->Type() != SCH_COMPONENT_T ) + continue; + + SCH_COMPONENT* symbol = static_cast( item ); + + // Skip pseudo-symbols, which have a reference starting with #. This mainly + // affects power symbols. + if( aIncludePowerSymbols || symbol->GetRef( &aSelectionPath )[0] != wxT( '#' ) ) + { + LIB_PART* part = symbol->GetPartRef().get(); + + if( part || aForceIncludeOrphanSymbols ) + { + SCH_REFERENCE schReference( symbol, part, aSelectionPath ); + schReference.SetSheetNumber( aSelectionPath.GetVirtualPageNumber() ); + aReferences.AddItem( schReference ); + } + } + } +} + + +void EE_SELECTION::GetMultiUnitSymbols( SCH_MULTI_UNIT_REFERENCE_MAP& aRefList, + const SCH_SHEET_PATH& aSelectionPath, + bool aIncludePowerSymbols ) +{ + for( EDA_ITEM* item : Items() ) + { + if( item->Type() != SCH_COMPONENT_T ) + continue; + + SCH_COMPONENT* symbol = static_cast( item ); + + // Skip pseudo-symbols, which have a reference starting with #. This mainly + // affects power symbols. + if( !aIncludePowerSymbols && symbol->GetRef( &aSelectionPath )[0] == wxT( '#' ) ) + continue; + + LIB_PART* part = symbol->GetPartRef().get(); + + if( part && part->GetUnitCount() > 1 ) + { + SCH_REFERENCE schReference = SCH_REFERENCE( symbol, part, aSelectionPath ); + schReference.SetSheetNumber( aSelectionPath.GetVirtualPageNumber() ); + wxString reference_str = schReference.GetRef(); + + // Never lock unassigned references + if( reference_str[reference_str.Len() - 1] == '?' ) + continue; + + aRefList[reference_str].AddItem( schReference ); + } + } +} + + bool EE_SELECTION::AllItemsHaveLineStroke() const { for( const EDA_ITEM* item : m_items ) diff --git a/eeschema/tools/ee_selection.h b/eeschema/tools/ee_selection.h index 451443ddbf..c43a12386e 100644 --- a/eeschema/tools/ee_selection.h +++ b/eeschema/tools/ee_selection.h @@ -25,10 +25,12 @@ #ifndef EE_SELECTION_H #define EE_SELECTION_H +class SCH_REFERENCE_LIST; class SCH_SCREEN; - +class SCH_SHEET_PATH; #include +#include // SCH_MULTI_UNIT_REFERENCE_MAP class EE_SELECTION : public SELECTION @@ -48,6 +50,33 @@ public: void SetScreen( SCH_SCREEN* aScreen ) { m_screen = aScreen; } SCH_SCREEN* GetScreen() { return m_screen; } + /** + * Adds #SCH_REFERENCE object to \a aReferences for each symbol in the selection. + * + * @param aReferences List of references to populate. + * @param aSelectionPath The path to the sheet containing this selection. + * @param aIncludePowerSymbols set to false to only get normal symbols. + * @param aForceIncludeOrphanSymbols set to true to include symbols having no symbol found + * in lib. The normal option is false, and set to true + * only to build the full list of symbols. + */ + void GetSymbols( SCH_REFERENCE_LIST& aReferences, const SCH_SHEET_PATH& aSelectionPath, + bool aIncludePowerSymbols = true, bool aForceIncludeOrphanSymbols = false ); + + /** + * Add a #SCH_REFERENCE_LIST object to \a aRefList for each same-reference set of + * multi-unit parts in the selection. + * + * The map key for each element will be the reference designator. + * + * @param aRefList Map of reference designators to reference lists + * @param aSelectionPath The path to the sheet containing this selection. + * @param aIncludePowerSymbols Set to false to only get normal symbols. + */ + void GetMultiUnitSymbols( SCH_MULTI_UNIT_REFERENCE_MAP& aRefList, + const SCH_SHEET_PATH& aSelectionPath, + bool aIncludePowerSymbols = true ); + /** * Checks if all items in the selection support line strokes *