From 15166a9f1469d7b5d9081faf5e3b8e35cc3c9b22 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Fri, 25 Jul 2025 16:13:35 -0700 Subject: [PATCH] Refactor REFDES_TRACKER Keep state for reuse in the class. This allows us to properly pick the value when fully reannotating Don't annotate when placing new units. Just step ahead in the unit value while keeping the refdes number constant. This eliminates the jumping around to unplaced units when placing new multi-unit symbols Fixes https://gitlab.com/kicad/code/kicad/-/issues/21378 --- eeschema/annotate.cpp | 6 +- eeschema/dialogs/panel_setup_annotation.cpp | 7 +- eeschema/refdes_tracker.cpp | 84 ++--- eeschema/refdes_tracker.h | 59 +++- eeschema/sch_reference_list.h | 15 +- eeschema/schematic_settings.cpp | 16 +- eeschema/schematic_settings.h | 6 - eeschema/tools/sch_drawing_tools.cpp | 37 +- eeschema/tools/sch_editor_control.cpp | 2 - .../test_annotation_refdes_tracker_units.cpp | 321 ++++++++++++------ .../test_annotation_units_conflicts.cpp | 101 ++++-- .../test_annotation_units_integration.cpp | 145 ++++++-- qa/tests/eeschema/test_sch_reference_list.cpp | 2 - 13 files changed, 558 insertions(+), 243 deletions(-) diff --git a/eeschema/annotate.cpp b/eeschema/annotate.cpp index 61bd2d0d1b..a720a66989 100644 --- a/eeschema/annotate.cpp +++ b/eeschema/annotate.cpp @@ -360,11 +360,7 @@ void SCH_EDIT_FRAME::AnnotateSymbols( SCH_COMMIT* aCommit, ANNOTATE_SCOPE_T aAn } } - std::shared_ptr refdes = Schematic().Settings().m_refDesTracker; - bool reuseRefDes = Schematic().Settings().m_reuseRefDes; - - references.SetReuseRefDes( reuseRefDes ); - references.SetRefDesTracker( refdes ); + references.SetRefDesTracker( Schematic().Settings().m_refDesTracker ); // Break full symbol reference into name (prefix) and number: // example: IC1 become IC, and 1 diff --git a/eeschema/dialogs/panel_setup_annotation.cpp b/eeschema/dialogs/panel_setup_annotation.cpp index 17d24870ec..7e16764fa2 100644 --- a/eeschema/dialogs/panel_setup_annotation.cpp +++ b/eeschema/dialogs/panel_setup_annotation.cpp @@ -17,6 +17,7 @@ * with this program. If not, see . */ +#include #include #include #include @@ -52,7 +53,7 @@ bool PANEL_SETUP_ANNOTATION::TransferDataToWindow() m_choiceSeparatorRefId->SetSelection( getRefStyleMenuIndex( settings.m_SubpartIdSeparator, settings.m_SubpartFirstId ) ); - m_checkReuseRefdes->SetValue( settings.m_reuseRefDes ); + m_checkReuseRefdes->SetValue( settings.m_refDesTracker->GetReuseRefDes() ); return true; } @@ -75,7 +76,7 @@ bool PANEL_SETUP_ANNOTATION::TransferDataFromWindow() case 6: settings.m_SubpartFirstId = '1'; settings.m_SubpartIdSeparator = '_'; break; } - settings.m_reuseRefDes = m_checkReuseRefdes->GetValue(); + settings.m_refDesTracker->SetReuseRefDes( m_checkReuseRefdes->GetValue() ); return true; } @@ -85,7 +86,7 @@ void PANEL_SETUP_ANNOTATION::ImportSettingsFrom( SCHEMATIC_SETTINGS& aSettings ) m_choiceSeparatorRefId->SetSelection( getRefStyleMenuIndex( aSettings.m_SubpartIdSeparator, aSettings.m_SubpartFirstId ) ); - m_checkReuseRefdes->SetValue( aSettings.m_reuseRefDes ); + m_checkReuseRefdes->SetValue( aSettings.m_refDesTracker->GetReuseRefDes() ); } diff --git a/eeschema/refdes_tracker.cpp b/eeschema/refdes_tracker.cpp index 9e4f459efd..fd7999e613 100644 --- a/eeschema/refdes_tracker.cpp +++ b/eeschema/refdes_tracker.cpp @@ -26,7 +26,7 @@ #include "refdes_tracker.h" REFDES_TRACKER::REFDES_TRACKER( bool aThreadSafe ) : - m_threadSafe( aThreadSafe ) + m_threadSafe( aThreadSafe ), m_reuseRefDes( true ) { } @@ -68,6 +68,12 @@ bool REFDES_TRACKER::insertNumber( const std::string& aPrefix, int aNumber ) return true; } + +bool REFDES_TRACKER::containsImpl( const std::string& aRefDes ) const +{ + return m_allRefDes.contains( aRefDes ); +} + bool REFDES_TRACKER::Contains( const std::string& aRefDes ) const { std::unique_lock lock; @@ -75,41 +81,7 @@ bool REFDES_TRACKER::Contains( const std::string& aRefDes ) const if( m_threadSafe ) lock = std::unique_lock( m_mutex ); - return m_allRefDes.find( aRefDes ) != m_allRefDes.end(); -} - -int REFDES_TRACKER::GetNextRefDes( const std::string& aPrefix, int aMinValue ) -{ - std::unique_lock lock; - - if( m_threadSafe ) - lock = std::unique_lock( m_mutex ); - - auto it = m_prefixData.find( aPrefix ); - - if( it == m_prefixData.end() ) - { - // No data for this prefix yet - int nextNum = std::max( 1, aMinValue ); - std::string result = aPrefix + std::to_string( nextNum ); - - // Insert this designator since we're reserving it - insertNumber( aPrefix, nextNum ); - m_allRefDes.insert( result ); - - return aMinValue; - } - - PREFIX_DATA& data = it->second; - int nextAvailable = findNextAvailable( data, aMinValue ); - - std::string result = aPrefix + std::to_string( nextAvailable ); - - // Insert this designator since we're reserving it - insertNumber( aPrefix, nextAvailable ); - m_allRefDes.insert( result ); - - return nextAvailable; + return containsImpl( aRefDes ); } @@ -141,7 +113,7 @@ int REFDES_TRACKER::GetNextRefDesForUnits( const SCH_REFERENCE& aRef, // Not currently in use - check if it was previously used std::string candidateRefDes = aRef.GetRef().ToStdString() + std::to_string( candidate ); - if( !Contains( candidateRefDes ) ) + if( m_reuseRefDes || !containsImpl( candidateRefDes ) ) { // Completely unused - this is our answer insertNumber( aRef.GetRefStr(), candidate ); @@ -165,7 +137,20 @@ int REFDES_TRACKER::GetNextRefDesForUnits( const SCH_REFERENCE& aRef, continue; } - if( areUnitsAvailable( aRef, mapIt->second, validUnits ) ) + // Check if required units are available + bool unitsAvailable; + if( m_externalUnitsChecker ) + { + // Use external units checker if available + unitsAvailable = m_externalUnitsChecker( aRef, mapIt->second, validUnits ); + } + else + { + // Use default implementation + unitsAvailable = areUnitsAvailable( aRef, mapIt->second, validUnits ); + } + + if( unitsAvailable ) { // All required units are available - this is our answer // Note: Don't insert into tracker since reference is already in use @@ -299,7 +284,6 @@ int REFDES_TRACKER::findNextAvailable( const PREFIX_DATA& aData, int aMinValue ) if( auto cacheIt = aData.m_nextCache.find( aMinValue ); cacheIt != aData.m_nextCache.end() ) return cacheIt->second; - updateBaseNext( const_cast( aData ) ); int candidate; @@ -565,4 +549,26 @@ std::vector REFDES_TRACKER::splitString( const std::string& aStr, c result.push_back( current ); return result; +} + + +void REFDES_TRACKER::SetUnitsChecker( const UNITS_CHECKER_FUNC& aChecker ) +{ + std::unique_lock lock; + + if( m_threadSafe ) + lock = std::unique_lock( m_mutex ); + + m_externalUnitsChecker = aChecker; +} + + +void REFDES_TRACKER::ClearUnitsChecker() +{ + std::unique_lock lock; + + if( m_threadSafe ) + lock = std::unique_lock( m_mutex ); + + m_externalUnitsChecker = nullptr; } \ No newline at end of file diff --git a/eeschema/refdes_tracker.h b/eeschema/refdes_tracker.h index e1900228ad..e7fbb16492 100644 --- a/eeschema/refdes_tracker.h +++ b/eeschema/refdes_tracker.h @@ -27,10 +27,28 @@ #include #include #include +#include +#include // Forward declaration class SCH_REFERENCE; +/** + * Function type for external units availability checking. + * + * This allows the REFDES_TRACKER to work with mock objects or custom logic + * without requiring actual SCH_REFERENCE dependencies. + * + * @param aTestRef Reference object being tested for compatibility + * @param aExistingRefs Vector of existing references for the same reference number + * @param aRequiredUnits Vector of unit numbers needed + * @return true if all required units are available (no conflicts) + */ +template +using UNITS_CHECKER_FUNC = std::function& aExistingRefs, + const std::vector& aRequiredUnits)>; + /** * Class to efficiently track reference designators and provide next available designators. * @@ -92,6 +110,21 @@ public: const std::vector& aRequiredUnits, int aMinValue ); + /** + * Set an external units checker function for SCH_REFERENCE objects. + * + * This allows overriding the default units availability logic without + * requiring LIB_SYMBOL dependencies. + * + * @param aChecker function to use for checking unit availability + */ + void SetUnitsChecker( const UNITS_CHECKER_FUNC& aChecker ); + + /** + * Clear the external units checker, reverting to default behavior. + */ + void ClearUnitsChecker(); + /** * Serialize the tracker data to a compact string representation. * @@ -121,16 +154,19 @@ public: */ size_t Size() const; + bool GetReuseRefDes() const { return m_reuseRefDes; } + void SetReuseRefDes( bool aReuse ) { m_reuseRefDes = aReuse; } + private: /** * Data structure for tracking used numbers and caching next available values. */ struct PREFIX_DATA { - std::set m_usedNumbers; ///< Sorted set of used numbers for this prefix - mutable std::map m_nextCache; ///< Cache of next available number for given min values - mutable int m_baseNext; ///< Next available from 1 (cached) - mutable bool m_cacheValid; ///< True if m_baseNext cache is valid + std::set m_usedNumbers; ///< Sorted set of used numbers for this prefix + mutable std::map m_nextCache; ///< Cache of next available number for given min values + mutable int m_baseNext; ///< Next available from 1 (cached) + mutable bool m_cacheValid; ///< True if m_baseNext cache is valid PREFIX_DATA() : m_baseNext( 1 ), m_cacheValid( false ) {} }; @@ -140,10 +176,13 @@ private: /// Map from prefix to its tracking data std::unordered_map m_prefixData; - - /// Set of all reference designators for O(1) lookup std::unordered_set m_allRefDes; + bool m_reuseRefDes; ///< If true, allows reusing existing reference designators + + /// External units checker function (optional) + UNITS_CHECKER_FUNC m_externalUnitsChecker; + /** * Internal implementation of Insert without locking. * @@ -159,6 +198,14 @@ private: */ void clearImpl(); + /** + * Check if a reference designator exists in the tracker without locking. + * + * @param aRefDes reference designator to check + * @return true if the reference designator exists + */ + bool containsImpl( const std::string& aRefDes ) const; + /** * Parse a reference designator into prefix and numerical suffix. * diff --git a/eeschema/sch_reference_list.h b/eeschema/sch_reference_list.h index 3939bac421..c4f10c0fe5 100644 --- a/eeschema/sch_reference_list.h +++ b/eeschema/sch_reference_list.h @@ -275,9 +275,7 @@ class SCH_REFERENCE_LIST { public: SCH_REFERENCE_LIST() - { - m_reuseRefDes = true; // Default to reusing reference designators after deletion - } + {} SCH_REFERENCE& operator[]( int aIndex ) { @@ -594,16 +592,6 @@ public: m_refDesTracker = aTracker; } - bool ReuseRefDes() const - { - return m_reuseRefDes; - } - - void SetReuseRefDes( bool aReuse ) - { - m_reuseRefDes = aReuse; - } - friend class BACK_ANNOTATION; typedef std::vector::iterator iterator; @@ -637,7 +625,6 @@ private: std::vector m_flatList; - bool m_reuseRefDes; // True if the reference designators should be reused after being deleted std::shared_ptr m_refDesTracker; ///< A list of previously used reference designators. }; diff --git a/eeschema/schematic_settings.cpp b/eeschema/schematic_settings.cpp index c7247b0ef0..b75b83b2c3 100644 --- a/eeschema/schematic_settings.cpp +++ b/eeschema/schematic_settings.cpp @@ -255,8 +255,18 @@ SCHEMATIC_SETTINGS::SCHEMATIC_SETTINGS( JSON_SETTINGS* aParent, const std::strin m_NgspiceSettings = std::make_shared( this, "ngspice" ); - m_params.emplace_back( new PARAM( "reuse_designators", - &m_reuseRefDes, true ) ); + m_params.emplace_back( new PARAM_LAMBDA( "reuse_designators", + [&]() -> bool + { + return m_refDesTracker ? m_refDesTracker->GetReuseRefDes() : false; + }, + [&]( bool aReuse ) + { + if( !m_refDesTracker ) + m_refDesTracker = std::make_shared(); + + m_refDesTracker->SetReuseRefDes( aReuse ); + }, true ) ); m_params.emplace_back( new PARAM_LAMBDA( "used_designators", [&]() -> std::string @@ -269,7 +279,7 @@ SCHEMATIC_SETTINGS::SCHEMATIC_SETTINGS( JSON_SETTINGS* aParent, const std::strin [&]( const std::string& aData ) { if( !m_refDesTracker ) - m_refDesTracker = std::make_unique(); + m_refDesTracker = std::make_shared(); m_refDesTracker->Deserialize( aData ); }, {} ) ); diff --git a/eeschema/schematic_settings.h b/eeschema/schematic_settings.h index 0e20d8d961..e43ed2372f 100644 --- a/eeschema/schematic_settings.h +++ b/eeschema/schematic_settings.h @@ -126,12 +126,6 @@ public: */ std::shared_ptr m_NgspiceSettings; - /** - * True if we want allow deleted schematic reference designators to be reused - * in the same project. - */ - bool m_reuseRefDes; - /** * A list of previously used schematic reference designators. * This is used to avoid reusing designators in the same project. diff --git a/eeschema/tools/sch_drawing_tools.cpp b/eeschema/tools/sch_drawing_tools.cpp index 0e57cd52b1..8c8f008353 100644 --- a/eeschema/tools/sch_drawing_tools.cpp +++ b/eeschema/tools/sch_drawing_tools.cpp @@ -233,7 +233,6 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) SCH_REFERENCE_LIST refs; refs.AddItem( newReference ); refs.SetRefDesTracker( schSettings.m_refDesTracker ); - refs.SetReuseRefDes( schSettings.m_reuseRefDes ); if( cfg->m_AnnotatePanel.automatic || newReference.AlwaysAnnotate() ) { @@ -473,32 +472,36 @@ int SCH_DRAWING_TOOLS::PlaceSymbol( const TOOL_EVENT& aEvent ) if( m_frame->eeconfig()->m_SymChooserPanel.place_all_units || m_frame->eeconfig()->m_SymChooserPanel.keep_symbol ) { - int new_unit = symbol->GetUnit(); + SCH_REFERENCE currentReference( symbol, m_frame->GetCurrentSheet() ); + SCHEMATIC& schematic = m_frame->Schematic(); - if( m_frame->eeconfig()->m_SymChooserPanel.place_all_units - && symbol->GetUnit() < symbol->GetUnitCount() ) + if( m_frame->eeconfig()->m_SymChooserPanel.place_all_units ) { - new_unit++; - } - else - { - new_unit = 1; + while( currentReference.GetUnit() <= symbol->GetUnitCount() + && schematic.Contains( currentReference ) ) + { + currentReference.SetUnit( currentReference.GetUnit() + 1 ); + } + + if( currentReference.GetUnit() > symbol->GetUnitCount() ) + { + currentReference.SetUnit( 1 ); + } } // We are either stepping to the next unit or next symbol - if( m_frame->eeconfig()->m_SymChooserPanel.keep_symbol || new_unit > 1 ) + if( m_frame->eeconfig()->m_SymChooserPanel.keep_symbol || + currentReference.GetUnit() > 1 ) { nextSymbol = static_cast( symbol->Duplicate( IGNORE_PARENT_GROUP ) ); - nextSymbol->SetUnit( new_unit ); - nextSymbol->SetUnitSelection( new_unit ); - - // Start new annotation sequence at first unit - if( new_unit == 1 ) - nextSymbol->ClearAnnotation( nullptr, false ); + nextSymbol->SetUnit( currentReference.GetUnit() ); + nextSymbol->SetUnitSelection( currentReference.GetUnit() ); addSymbol( nextSymbol ); symbol = nextSymbol; - annotate(); + + if( currentReference.GetUnit() == 1 ) + annotate(); // Update the list of references for the next symbol placement. SCH_REFERENCE placedSymbolReference( symbol, m_frame->GetCurrentSheet() ); diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index e1ef9a02ab..9e7bc990d6 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -2108,7 +2108,6 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent ) { annotatedSymbols[path].SortByReferenceOnly(); annotatedSymbols[path].SetRefDesTracker( schematicSettings.m_refDesTracker ); - annotatedSymbols[path].SetReuseRefDes( schematicSettings.m_reuseRefDes ); if( pasteMode == PASTE_MODE::UNIQUE_ANNOTATIONS ) { @@ -2132,7 +2131,6 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent ) { annotatedSymbols[pastedSheetPath].SortByReferenceOnly(); annotatedSymbols[pastedSheetPath].SetRefDesTracker( schematicSettings.m_refDesTracker ); - annotatedSymbols[pastedSheetPath].SetReuseRefDes( schematicSettings.m_reuseRefDes ); if( pasteMode == PASTE_MODE::UNIQUE_ANNOTATIONS ) { diff --git a/qa/tests/eeschema/test_annotation_refdes_tracker_units.cpp b/qa/tests/eeschema/test_annotation_refdes_tracker_units.cpp index 18ca446fa3..d294787b92 100644 --- a/qa/tests/eeschema/test_annotation_refdes_tracker_units.cpp +++ b/qa/tests/eeschema/test_annotation_refdes_tracker_units.cpp @@ -23,47 +23,12 @@ #include #include -// Mock SCH_REFERENCE class for testing GetNextRefDesForUnits -class MOCK_SCH_REFERENCE -{ -public: - MOCK_SCH_REFERENCE( const wxString& aRef, const wxString& aValue, - const wxString& aLibName, int aUnit ) : - m_ref( aRef ), m_value( aValue ), m_libName( aLibName ), m_unit( aUnit ), - m_refStr( aRef.ToStdString() ) - { - } - - wxString GetRef() const { return m_ref; } - const char* GetRefStr() const { return m_refStr.c_str(); } - wxString GetValue() const { return m_value; } - int GetUnit() const { return m_unit; } - - int CompareValue( const MOCK_SCH_REFERENCE& other ) const - { - return m_value.Cmp( other.m_value ); - } - - int CompareLibName( const MOCK_SCH_REFERENCE& other ) const - { - return m_libName.Cmp( other.m_libName ); - } - -private: - wxString m_ref; - wxString m_value; - wxString m_libName; - int m_unit; - std::string m_refStr; // Store as std::string to avoid temp object issues -}; - struct REFDES_UNITS_TEST_CASE { std::string m_caseName; std::string m_testRefPrefix; std::string m_testRefValue; - std::string m_testRefLibName; - std::map> m_refNumberMap; + std::map>> m_refNumberMap; // Map of ref number to vector of (value, unit) tuples std::vector m_requiredUnits; int m_minValue; int m_expectedResult; @@ -74,6 +39,44 @@ class TEST_REFDES_TRACKER_UNITS : public KI_TEST::SCHEMATIC_TEST_FIXTURE { protected: void runTestCase( const REFDES_UNITS_TEST_CASE& testCase ); + + // Helper method to create test references + SCH_REFERENCE createTestReference( const std::string& aRefPrefix, const std::string& aValue, int aUnit ) + { + SCH_SYMBOL dummySymbol; + SCH_SHEET_PATH dummyPath; + + SCH_REFERENCE ref( &dummySymbol, dummyPath ); + ref.SetRef( aRefPrefix ); + ref.SetValue( aValue ); + ref.SetUnit( aUnit ); + + return ref; + } + + // Helper method to setup units checker + void setupRefDesTracker( REFDES_TRACKER& tracker ) + { + tracker.SetReuseRefDes( false ); // Disable reuse for these tests + tracker.SetUnitsChecker( []( const SCH_REFERENCE& aTestRef, + const std::vector& aExistingRefs, + const std::vector& aRequiredUnits ) + { + // Check if all required units are available + for( int unit : aRequiredUnits ) + { + for( const auto& ref : aExistingRefs ) + { + if( ref.GetUnit() == unit + || ref.CompareValue( aTestRef ) != 0 ) + { + return false; // Conflict found + } + } + } + return true; // All required units are available + } ); + } }; void TEST_REFDES_TRACKER_UNITS::runTestCase( const REFDES_UNITS_TEST_CASE& testCase ) @@ -87,26 +90,50 @@ void TEST_REFDES_TRACKER_UNITS::runTestCase( const REFDES_UNITS_TEST_CASE& testC } // Create test reference - MOCK_SCH_REFERENCE testRef( testCase.m_testRefPrefix, testCase.m_testRefValue, - testCase.m_testRefLibName, 1 ); + SCH_REFERENCE testRef = createTestReference( testCase.m_testRefPrefix, testCase.m_testRefValue, 1 ); - // Convert mock references to SCH_REFERENCE map (this would need adaptation for real SCH_REFERENCE) - // For now, we'll test the core logic conceptually + // Convert test case data to actual SCH_REFERENCE map + std::map> refNumberMap; + for( const auto& [refNum, tupleVec] : testCase.m_refNumberMap ) + { + std::vector refs; + for( const auto& [value, unit] : tupleVec ) + { + refs.push_back( createTestReference( testCase.m_testRefPrefix, value, unit ) ); + } + refNumberMap[refNum] = refs; + } BOOST_TEST_INFO( "Testing case: " + testCase.m_caseName ); - // Test the basic tracker functionality first - BOOST_CHECK_EQUAL( tracker.Contains( testCase.m_testRefPrefix + "1" ), - testCase.m_trackerPreloads.end() != - std::find( testCase.m_trackerPreloads.begin(), - testCase.m_trackerPreloads.end(), - testCase.m_testRefPrefix + "1" ) ); + setupRefDesTracker( tracker ); + + // Test GetNextRefDesForUnits logic using the 4-parameter method + int result = tracker.GetNextRefDesForUnits( testRef, + refNumberMap, + testCase.m_requiredUnits, + testCase.m_minValue ); + + BOOST_CHECK_EQUAL( result, testCase.m_expectedResult ); + + // Additional verification: check that the result reference is in the tracker + // (unless it was a case where units were available in existing reference) + std::string resultRefDes = testCase.m_testRefPrefix + std::to_string( result ); + + // Check if this reference number was already in use + bool wasInUse = testCase.m_refNumberMap.find( result ) != testCase.m_refNumberMap.end(); + + if( !wasInUse && !testCase.m_requiredUnits.empty() ) + { + // For completely new references, it should be added to tracker + BOOST_CHECK( tracker.Contains( resultRefDes ) ); + } } static const std::vector refdesUnitsTestCases = { { "Case 1: Completely unused reference - empty units", - "U", "LM358", "OpAmp", + "U", "LM358", {}, // No currently used references {}, // Empty required units (need completely unused) 1, // Min value @@ -115,7 +142,7 @@ static const std::vector refdesUnitsTestCases = { }, { "Case 2: Completely unused reference - with units", - "U", "LM358", "OpAmp", + "U", "LM358", {}, // No currently used references {1, 2}, // Need units 1 and 2 1, // Min value @@ -124,9 +151,9 @@ static const std::vector refdesUnitsTestCases = { }, { "Case 3: Skip currently in use reference", - "U", "LM358", "OpAmp", + "U", "LM358", { - { 1, { MOCK_SCH_REFERENCE("U", "LM358", "OpAmp", 1) } } // U1 unit 1 in use + { 1, { std::make_tuple("LM358", 1) } } // U1 unit 1 in use }, {1, 2}, // Need units 1 and 2 1, // Min value @@ -135,10 +162,10 @@ static const std::vector refdesUnitsTestCases = { }, { "Case 4: Units available in currently used reference", - "U", "LM358", "OpAmp", + "U", "LM358", { - { 1, { MOCK_SCH_REFERENCE("U", "LM358", "OpAmp", 3), - MOCK_SCH_REFERENCE("U", "LM358", "OpAmp", 4) } } // U1 units 3,4 in use + { 1, { std::make_tuple("LM358", 3), + std::make_tuple("LM358", 4) } } // U1 units 3,4 in use }, {1, 2}, // Need units 1 and 2 (available) 1, // Min value @@ -147,9 +174,9 @@ static const std::vector refdesUnitsTestCases = { }, { "Case 5: Different value conflict", - "U", "LM358", "OpAmp", + "U", "LM358", { - { 1, { MOCK_SCH_REFERENCE("U", "LM741", "OpAmp", 1) } } // U1 different value + { 1, { std::make_tuple("LM741", 1) } } // U1 different value }, {1}, // Need unit 1 1, // Min value @@ -157,19 +184,8 @@ static const std::vector refdesUnitsTestCases = { {} }, { - "Case 6: Different library conflict", - "U", "LM358", "OpAmp", - { - { 1, { MOCK_SCH_REFERENCE("U", "LM358", "DifferentLib", 2) } } // U1 different lib - }, - {1}, // Need unit 1 - 1, // Min value - 2, // Expected: U2 (can't share with different library) - {} - }, - { - "Case 7: Previously used reference in tracker", - "U", "LM358", "OpAmp", + "Case 6: Previously used reference in tracker", + "U", "LM358", {}, // No currently used references {1}, // Need unit 1 1, // Min value @@ -177,10 +193,10 @@ static const std::vector refdesUnitsTestCases = { {"U1"} // U1 preloaded in tracker }, { - "Case 8: Min value higher than available", - "U", "LM358", "OpAmp", + "Case 7: Min value higher than available", + "U", "LM358", { - { 5, { MOCK_SCH_REFERENCE("U", "LM358", "OpAmp", 1) } } // U5 unit 1 in use + { 5, { std::make_tuple("LM358", 1) } } // U5 unit 1 in use }, {2}, // Need unit 2 10, // Min value = 10 @@ -188,8 +204,8 @@ static const std::vector refdesUnitsTestCases = { {} }, { - "Case 9: Negative units filtered out", - "U", "LM358", "OpAmp", + "Case 8: Negative units filtered out", + "U", "LM358", {}, {-1, 1, -5, 2}, // Mix of negative and positive units 1, // Min value @@ -197,11 +213,11 @@ static const std::vector refdesUnitsTestCases = { {} }, { - "Case 10: Complex scenario with gaps", - "IC", "74HC00", "Logic", + "Case 9: Complex scenario with gaps", + "IC", "74HC00", { - { 2, { MOCK_SCH_REFERENCE("IC", "74HC00", "Logic", 1) } }, // IC2 unit 1 used - { 4, { MOCK_SCH_REFERENCE("IC", "74HC00", "Logic", 2) } } // IC4 unit 2 used + { 2, { std::make_tuple("74HC00", 1) } }, // IC2 unit 1 used + { 4, { std::make_tuple("74HC00", 2) } } // IC4 unit 2 used }, {1, 3}, // Need units 1 and 3 1, // Min value @@ -224,26 +240,93 @@ BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_EdgeCases ) { REFDES_TRACKER tracker; - // Test empty required units vector - MOCK_SCH_REFERENCE testRef( "R", "1k", "Resistor", 1 ); + // Test empty required units vector - should find completely unused reference + SCH_REFERENCE testRef = createTestReference( "R", "1k", 1 ); std::map> emptyMap; std::vector emptyUnits; - // This should return the first unused reference number - // Note: This test framework is conceptual since we can't easily mock SCH_REFERENCE + setupRefDesTracker( tracker ); + int result = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); + BOOST_CHECK_EQUAL( result, 1 ); + BOOST_CHECK( tracker.Contains( "R1" ) ); - BOOST_TEST_MESSAGE( "Edge case testing would require full SCH_REFERENCE mock implementation" ); + // Test with some references already in tracker + tracker.Insert( "R3" ); + result = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); + BOOST_CHECK_EQUAL( result, 2 ); // Should skip R1 (already inserted above) and get R2 + + // Test with negative units (should be filtered out) + std::vector mixedUnits = {-1, 1, -5, 2}; + SCH_REFERENCE testRef2 = createTestReference( "C", "100nF", 1 ); + result = tracker.GetNextRefDesForUnits( testRef2, emptyMap, mixedUnits, 1 ); + BOOST_CHECK_EQUAL( result, 1 ); +} + +BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_UsagePattern ) +{ + REFDES_TRACKER tracker; + + // Demonstrate actual usage pattern for GetNextRefDesForUnits with our test helper + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + + // Create map of currently used references + std::map> refNumberMap; + refNumberMap[1] = { createTestReference("U", "LM358", 1), + createTestReference("U", "LM358", 2) }; // U1 has units 1,2 used + refNumberMap[3] = { createTestReference("U", "LM358", 1) }; // U3 has unit 1 used + + // Specify required units for new symbol + std::vector requiredUnits = {1, 2}; + + setupRefDesTracker( tracker ); + + // Get next available reference number + int nextRefNum = tracker.GetNextRefDesForUnits( testRef, refNumberMap, requiredUnits, 1 ); + + // Should return 2 (U2) since U1 conflicts (units 1,2 already used) and U2 is available + BOOST_CHECK_EQUAL( nextRefNum, 2 ); + BOOST_CHECK( tracker.Contains( "U2" ) ); + + // Test case where units are available in existing reference + std::vector requiredUnits2 = {3, 4}; // These should be available in U1 + refNumberMap[3] = { createTestReference("U", "LM358", 1) }; // U1 only has unit 1 and 2 used + + nextRefNum = tracker.GetNextRefDesForUnits( testRef, refNumberMap, requiredUnits2, 1 ); + BOOST_CHECK_EQUAL( nextRefNum, 1 ); // U1 should work since units 3,4 are available + + // Test different value conflict + SCH_REFERENCE differentValueRef = createTestReference( "U", "LM741", 1 ); + refNumberMap[4] = { createTestReference("U", "LM358", 1) }; // U4 has different value + + nextRefNum = tracker.GetNextRefDesForUnits( differentValueRef, refNumberMap, {1}, 4 ); + BOOST_CHECK_EQUAL( nextRefNum, 5 ); // Should skip U4 due to value conflict } BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_ThreadSafety ) { REFDES_TRACKER tracker( true ); // Enable thread safety - // Basic test that thread-safe version can be created and used - int result = tracker.GetNextRefDes( "U", 1 ); - BOOST_CHECK_EQUAL( result, 1 ); + // Test that GetNextRefDesForUnits works with thread safety enabled + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector requiredUnits = {1, 2}; + setupRefDesTracker( tracker ); + int result = tracker.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( result, 1 ); BOOST_CHECK( tracker.Contains( "U1" ) ); + + // Test multiple calls work correctly with thread safety + result = tracker.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( result, 2 ); + BOOST_CHECK( tracker.Contains( "U2" ) ); + + // Test with conflicts and thread safety + std::map> conflictMap; + conflictMap[3] = { createTestReference("U", "LM358", 1) }; // U3 unit 1 in use + + result = tracker.GetNextRefDesForUnits( testRef, conflictMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( result, 4 ); // Should skip U1,U2 (in tracker) and U3 (conflicted) } BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) @@ -258,24 +341,63 @@ BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) BOOST_CHECK( tracker.Contains( "U3" ) ); BOOST_CHECK( !tracker.Contains( "U2" ) ); - // GetNextRefDes should return U2 - int next = tracker.GetNextRefDes( "U", 1 ); + // Test GetNextRefDesForUnits with preloaded tracker + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector requiredUnits = {1, 2}; + + setupRefDesTracker( tracker ); + + // Should get U2 since U1 is already in tracker (preloaded) and U3 is also preloaded + int next = tracker.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); BOOST_CHECK( tracker.Contains( "U2" ) ); + + // Test with higher minimum value + next = tracker.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 5 ); + BOOST_CHECK_EQUAL( next, 5 ); + BOOST_CHECK( tracker.Contains( "U5" ) ); + + // Test integration with serialization + std::string serialized = tracker.Serialize(); + REFDES_TRACKER tracker2; + BOOST_CHECK( tracker2.Deserialize( serialized ) ); + + // Verify deserialized tracker has the same state + BOOST_CHECK( tracker2.Contains( "U1" ) ); + BOOST_CHECK( tracker2.Contains( "U2" ) ); + BOOST_CHECK( tracker2.Contains( "U3" ) ); + BOOST_CHECK( tracker2.Contains( "U5" ) ); + + setupRefDesTracker( tracker2 ); + + // GetNextRefDesForUnits should work with deserialized tracker + next = tracker2.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( next, 4 ); // Should get U4 (first available after U1,U2,U3,U5) } BOOST_AUTO_TEST_CASE( Serialization_WithTrackedReferences ) { REFDES_TRACKER tracker; - // Add some references using both Insert and GetNextRefDes + // Add some references using both Insert and GetNextRefDesForUnits tracker.Insert( "R1" ); tracker.Insert( "R3" ); - int next = tracker.GetNextRefDes( "R", 1 ); // Should get R2 - BOOST_CHECK_EQUAL( next, 2 ); - next = tracker.GetNextRefDes( "C", 5 ); // Should get C5 - BOOST_CHECK_EQUAL( next, 5 ); + setupRefDesTracker( tracker ); + + // Use GetNextRefDesForUnits to get next reference + SCH_REFERENCE testRef = createTestReference( "R", "1k", 1 ); + std::map> emptyMap; + std::vector requiredUnits = {1}; + + int next = tracker.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( next, 2 ); // Should get R2 + + // Test with different prefix + SCH_REFERENCE capacitorRef = createTestReference( "C", "100nF", 1 ); + next = tracker.GetNextRefDesForUnits( capacitorRef, emptyMap, requiredUnits, 5 ); + BOOST_CHECK_EQUAL( next, 5 ); // Should get C5 // Test serialization std::string serialized = tracker.Serialize(); @@ -290,9 +412,18 @@ BOOST_AUTO_TEST_CASE( Serialization_WithTrackedReferences ) BOOST_CHECK( tracker2.Contains( "R3" ) ); BOOST_CHECK( tracker2.Contains( "C5" ) ); - // Next reference should be R4 - next = tracker2.GetNextRefDes( "R", 1 ); - BOOST_CHECK_EQUAL( next, 4 ); + setupRefDesTracker( tracker2 ); + + // Test GetNextRefDesForUnits with deserialized tracker + next = tracker2.GetNextRefDesForUnits( testRef, emptyMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( next, 4 ); // Next reference should be R4 + + // Test with unit conflicts after deserialization + std::map> conflictMap; + conflictMap[4] = { createTestReference("R", "2k", 1) }; // R4 different value + + next = tracker2.GetNextRefDesForUnits( testRef, conflictMap, requiredUnits, 1 ); + BOOST_CHECK_EQUAL( next, 5 ); // Should skip R4 due to value conflict } BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file diff --git a/qa/tests/eeschema/test_annotation_units_conflicts.cpp b/qa/tests/eeschema/test_annotation_units_conflicts.cpp index b93aa54a0b..a86a537b54 100644 --- a/qa/tests/eeschema/test_annotation_units_conflicts.cpp +++ b/qa/tests/eeschema/test_annotation_units_conflicts.cpp @@ -23,6 +23,48 @@ #include #include +class TEST_ANNOTATION_UNITS_CONFLICTS : public KI_TEST::SCHEMATIC_TEST_FIXTURE +{ +protected: + // Helper method to create SCH_REFERENCE objects for testing + SCH_REFERENCE createTestReference( const wxString& aRef, const wxString& aValue, int aUnit ) + { + SCH_SYMBOL dummySymbol; + SCH_SHEET_PATH dummyPath; + + SCH_REFERENCE ref( &dummySymbol, dummyPath ); + ref.SetRef( aRef ); + ref.SetValue( aValue ); + ref.SetUnit( aUnit ); + + return ref; + } + + // Helper method to setup units checker for testing + void setupRefDesTracker( REFDES_TRACKER& tracker ) + { + tracker.SetReuseRefDes( false ); // Disable reuse for these tests + tracker.SetUnitsChecker( []( const SCH_REFERENCE& aTestRef, + const std::vector& aExistingRefs, + const std::vector& aRequiredUnits ) + { + // Mock implementation for unit availability check + for( int unit : aRequiredUnits ) + { + for( const auto& ref : aExistingRefs ) + { + if( ref.GetUnit() == unit + && ref.CompareValue( aTestRef ) == 0 ) + { + return false; // Conflict found + } + } + } + return true; // All required units are available + } ); + } +}; + // Test cases that specifically validate the unit conflict detection logic // These tests focus on the areUnitsAvailable method and related functionality @@ -40,17 +82,6 @@ struct UNIT_CONFLICT_TEST_CASE std::string m_reason; // Reason for expected result }; -class TEST_UNIT_CONFLICTS_FIXTURE : public KI_TEST::SCHEMATIC_TEST_FIXTURE -{ -protected: - // Helper to create mock SCH_REFERENCE objects for testing - SCH_REFERENCE createMockReference( const std::string& prefix, int number, int unit, - const std::string& value, const std::string& libName ); -}; - -// Note: In a real implementation, this would need actual SCH_SYMBOL and SCH_SHEET_PATH objects -// For testing purposes, we'll focus on the logical validation - static const std::vector unitConflictCases = { { "Units available - no conflicts", @@ -135,7 +166,7 @@ static const std::vector unitConflictCases = { } }; -BOOST_FIXTURE_TEST_SUITE( UnitConflicts, TEST_UNIT_CONFLICTS_FIXTURE ) +BOOST_FIXTURE_TEST_SUITE( UnitConflicts, TEST_ANNOTATION_UNITS_CONFLICTS ) BOOST_AUTO_TEST_CASE( ValidateUnitConflictDetection ) { @@ -201,6 +232,7 @@ BOOST_AUTO_TEST_CASE( ValidateUnitConflictDetection ) BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) { REFDES_TRACKER tracker; + setupRefDesTracker( tracker ); // Test the overall GetNextRefDesForUnits logic using the tracker @@ -210,13 +242,15 @@ BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) // Test case 1: Completely unused reference with empty units // Should get U2 (first unused after U1) - // Note: This test is conceptual since we need real SCH_REFERENCE objects + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector emptyUnits; - int nextRef = tracker.GetNextRefDes( "U", 1 ); + int nextRef = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( nextRef, 2 ); // Should skip U1, get U2 // Test case 2: Min value higher than next available - nextRef = tracker.GetNextRefDes( "U", 10 ); + nextRef = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 10 ); BOOST_CHECK_EQUAL( nextRef, 10 ); // Should start from min value // Verify references were inserted @@ -224,7 +258,8 @@ BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) BOOST_CHECK( tracker.Contains( "U10" ) ); // Test case 3: New prefix - nextRef = tracker.GetNextRefDes( "IC", 1 ); + SCH_REFERENCE icRef = createTestReference( "IC", "74HC00", 1 ); + nextRef = tracker.GetNextRefDesForUnits( icRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( nextRef, 1 ); // New prefix should start at 1 BOOST_CHECK( tracker.Contains( "IC1" ) ); } @@ -232,6 +267,7 @@ BOOST_AUTO_TEST_CASE( GetNextRefDesForUnits_Integration ) BOOST_AUTO_TEST_CASE( RefDesTracker_StateConsistency ) { REFDES_TRACKER tracker; + setupRefDesTracker( tracker ); // Test that the tracker maintains consistent state across operations @@ -247,18 +283,22 @@ BOOST_AUTO_TEST_CASE( RefDesTracker_StateConsistency ) BOOST_CHECK( !tracker.Contains( "R2" ) ); BOOST_CHECK( !tracker.Contains( "R4" ) ); - // Get next reference - should fill gap at R2 - int next = tracker.GetNextRefDes( "R", 1 ); + // Test GetNextRefDesForUnits with empty units - should fill gap at R2 + SCH_REFERENCE testRef = createTestReference( "R", "1k", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + int next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); BOOST_CHECK( tracker.Contains( "R2" ) ); // Get next reference - should fill gap at R4 - next = tracker.GetNextRefDes( "R", 1 ); + next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 4 ); BOOST_CHECK( tracker.Contains( "R4" ) ); // Get next reference - should go to R6 - next = tracker.GetNextRefDes( "R", 1 ); + next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 6 ); BOOST_CHECK( tracker.Contains( "R6" ) ); @@ -269,27 +309,32 @@ BOOST_AUTO_TEST_CASE( RefDesTracker_StateConsistency ) BOOST_AUTO_TEST_CASE( CacheConsistency_AfterInserts ) { REFDES_TRACKER tracker; + setupRefDesTracker( tracker ); - // Test that cache remains consistent after mixed Insert/GetNextRefDes operations + // Test that cache remains consistent after mixed Insert/GetNextRefDesForUnits operations // Start with some manual inserts tracker.Insert( "C1" ); tracker.Insert( "C5" ); tracker.Insert( "C10" ); + SCH_REFERENCE testRef = createTestReference( "C", "100nF", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + // Get next ref - should use cached logic to find C2 - int next = tracker.GetNextRefDes( "C", 1 ); + int next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); // Insert C3 manually tracker.Insert( "C3" ); // Get next ref - cache should be updated, should get C4 - next = tracker.GetNextRefDes( "C", 1 ); + next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 4 ); // Test with minimum value - should respect cache - next = tracker.GetNextRefDes( "C", 7 ); + next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 7 ); BOOST_CHECK_EQUAL( next, 7 ); // C6 available but min is 7 // Verify all references exist @@ -306,12 +351,18 @@ BOOST_AUTO_TEST_CASE( CacheConsistency_AfterInserts ) BOOST_AUTO_TEST_CASE( ThreadSafety_BasicValidation ) { REFDES_TRACKER tracker( true ); // Enable thread safety + setupRefDesTracker( tracker ); // Basic validation that thread-safe operations work BOOST_CHECK( tracker.Insert( "U1" ) ); BOOST_CHECK( tracker.Contains( "U1" ) ); - int next = tracker.GetNextRefDes( "U", 1 ); + // Test GetNextRefDesForUnits with thread safety + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + int next = tracker.GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); // Test serialization with thread safety diff --git a/qa/tests/eeschema/test_annotation_units_integration.cpp b/qa/tests/eeschema/test_annotation_units_integration.cpp index 80079646ff..db81303762 100644 --- a/qa/tests/eeschema/test_annotation_units_integration.cpp +++ b/qa/tests/eeschema/test_annotation_units_integration.cpp @@ -24,6 +24,48 @@ #include #include +class TEST_ANNOTATION_UNITS_INTEGRATION : public KI_TEST::SCHEMATIC_TEST_FIXTURE +{ +protected: + // Helper method to create SCH_REFERENCE objects for testing + SCH_REFERENCE createTestReference( const wxString& aRef, const wxString& aValue, int aUnit ) + { + SCH_SYMBOL dummySymbol; + SCH_SHEET_PATH dummyPath; + + SCH_REFERENCE ref( &dummySymbol, dummyPath ); + ref.SetRef( aRef ); + ref.SetValue( aValue ); + ref.SetUnit( aUnit ); + + return ref; + } + + // Helper method to setup units checker for testing + void setupRefDesTracker( REFDES_TRACKER& tracker ) + { + tracker.SetReuseRefDes( false ); // Disable reuse for these tests + tracker.SetUnitsChecker( []( const SCH_REFERENCE& aTestRef, + const std::vector& aExistingRefs, + const std::vector& aRequiredUnits ) + { + // Mock implementation for unit availability check + for( int unit : aRequiredUnits ) + { + for( const auto& ref : aExistingRefs ) + { + if( ref.GetUnit() == unit + && ref.CompareValue( aTestRef ) == 0 ) + { + return false; // Conflict found + } + } + } + return true; // All required units are available + } ); + } +}; + struct REANNOTATED_REFERENCE { wxString m_KIID; ///< KIID of the symbol to reannotate @@ -39,6 +81,29 @@ protected: void setupRefDesTracker(); void testFindFirstUnusedReferenceWithUnits(); void verifyUnitAvailabilityLogic(); + + // Helper method to setup units checker for testing + void setupRefDesTracker( REFDES_TRACKER& tracker ) + { + tracker.SetUnitsChecker( []( const SCH_REFERENCE& aTestRef, + const std::vector& aExistingRefs, + const std::vector& aRequiredUnits ) + { + // Mock implementation for unit availability check + for( int unit : aRequiredUnits ) + { + for( const auto& ref : aExistingRefs ) + { + if( ref.GetUnit() == unit + && ref.CompareValue( aTestRef ) == 0 ) + { + return false; // Conflict found + } + } + } + return true; // All required units are available + } ); + } }; void TEST_SCH_REFERENCE_LIST_UNITS_FIXTURE::setupRefDesTracker() @@ -55,7 +120,7 @@ void TEST_SCH_REFERENCE_LIST_UNITS_FIXTURE::setupRefDesTracker() m_schematic->Settings().m_refDesTracker = tracker; } -BOOST_FIXTURE_TEST_SUITE( SchReferenceListUnits, TEST_SCH_REFERENCE_LIST_UNITS_FIXTURE ) +BOOST_FIXTURE_TEST_SUITE( SchReferenceListUnits, TEST_ANNOTATION_UNITS_INTEGRATION ) struct UNIT_ANNOTATION_CASE { @@ -159,18 +224,26 @@ BOOST_AUTO_TEST_CASE( RefDesTrackerIntegration ) tracker->Insert( "R2" ); tracker->Insert( "R5" ); - // Test that GetNextRefDes respects previously inserted references - int nextU = tracker->GetNextRefDes( "U", 1 ); + // Test GetNextRefDesForUnits respects previously inserted references + SCH_REFERENCE uRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + setupRefDesTracker( *tracker ); + + int nextU = tracker->GetNextRefDesForUnits( uRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( nextU, 2 ); // Should skip U1, get U2 - int nextR = tracker->GetNextRefDes( "R", 1 ); + SCH_REFERENCE rRef = createTestReference( "R", "1k", 1 ); + int nextR = tracker->GetNextRefDesForUnits( rRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( nextR, 3 ); // Should skip R1,R2, get R3 - int nextIC = tracker->GetNextRefDes( "IC", 1 ); + SCH_REFERENCE icRef = createTestReference( "IC", "74HC00", 1 ); + int nextIC = tracker->GetNextRefDesForUnits( icRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( nextIC, 1 ); // New prefix, should get IC1 // Test with minimum values - int nextU_min5 = tracker->GetNextRefDes( "U", 5 ); + int nextU_min5 = tracker->GetNextRefDesForUnits( uRef, emptyMap, emptyUnits, 5 ); BOOST_CHECK_EQUAL( nextU_min5, 5 ); // Should get U5 (min value 5) // Verify all references were inserted @@ -188,13 +261,7 @@ BOOST_AUTO_TEST_CASE( FindFirstUnusedReferenceWithUnits ) refList.SetRefDesTracker( tracker ); - // Simulate some existing references in the list - // This would normally come from the schematic, but we'll create mock data - - // Test the concept of unit availability checking - // (The actual implementation would require real SCH_REFERENCE objects) - - BOOST_TEST_MESSAGE( "Testing unit availability concept - requires full schematic integration" ); + // Test the concept of unit availability checking with GetNextRefDesForUnits // Test that tracker properly handles unit conflicts tracker->Insert( "U1" ); // Simulate U1 being previously used @@ -202,8 +269,16 @@ BOOST_AUTO_TEST_CASE( FindFirstUnusedReferenceWithUnits ) // The FindFirstUnusedReference method should now use GetNextRefDesForUnits // and properly consider unit conflicts when finding available references - int next = tracker->GetNextRefDes( "U", 1 ); + SCH_REFERENCE testRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + setupRefDesTracker( *tracker ); + + int next = tracker->GetNextRefDesForUnits( testRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); // Should skip U1 + + BOOST_TEST_MESSAGE( "Testing unit availability concept - integrated with GetNextRefDesForUnits" ); } BOOST_AUTO_TEST_CASE( SerializationWithComplexRefs ) @@ -213,13 +288,23 @@ BOOST_AUTO_TEST_CASE( SerializationWithComplexRefs ) // Add references through various methods tracker->Insert( "U1" ); tracker->Insert( "U3" ); - int next = tracker->GetNextRefDes( "U", 1 ); // Gets U2 - BOOST_CHECK_EQUAL( next, 2 ); + + // Use GetNextRefDesForUnits to get U2 + SCH_REFERENCE uRef = createTestReference( "U", "LM358", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + setupRefDesTracker( *tracker ); + + int next = tracker->GetNextRefDesForUnits( uRef, emptyMap, emptyUnits, 1 ); + BOOST_CHECK_EQUAL( next, 2 ); // Gets U2 tracker->Insert( "IC1" ); tracker->Insert( "IC5" ); - next = tracker->GetNextRefDes( "IC", 3 ); // Gets IC3 - BOOST_CHECK_EQUAL( next, 3 ); + + SCH_REFERENCE icRef = createTestReference( "IC", "74HC00", 1 ); + next = tracker->GetNextRefDesForUnits( icRef, emptyMap, emptyUnits, 3 ); + BOOST_CHECK_EQUAL( next, 3 ); // Gets IC3 // Test serialization captures all state std::string serialized = tracker->Serialize(); @@ -235,11 +320,12 @@ BOOST_AUTO_TEST_CASE( SerializationWithComplexRefs ) BOOST_CHECK( tracker2->Contains( "IC3" ) ); BOOST_CHECK( tracker2->Contains( "IC5" ) ); - // Verify next references continue correctly - next = tracker2->GetNextRefDes( "U", 1 ); + // Verify next references continue correctly using GetNextRefDesForUnits + setupRefDesTracker( *tracker2 ); + next = tracker2->GetNextRefDesForUnits( uRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 4 ); - next = tracker2->GetNextRefDes( "IC", 1 ); + next = tracker2->GetNextRefDesForUnits( icRef, emptyMap, emptyUnits, 1 ); BOOST_CHECK_EQUAL( next, 2 ); } @@ -253,22 +339,29 @@ BOOST_AUTO_TEST_CASE( CachingEfficiency ) tracker->Insert( "R" + std::to_string( i ) ); } + SCH_REFERENCE rRef = createTestReference( "R", "1k", 1 ); + std::map> emptyMap; + std::vector emptyUnits; + + setupRefDesTracker( *tracker ); + // Test that repeated calls with same parameters are fast (cached) auto start = std::chrono::high_resolution_clock::now(); for( int i = 0; i < 100; ++i ) { - int result1 = tracker->GetNextRefDes( "R", 1 ); // Should be cached after first call - int result50 = tracker->GetNextRefDes( "R", 50 ); // Should be cached after first call + // These calls should benefit from internal caching in REFDES_TRACKER + int result1 = tracker->GetNextRefDesForUnits( rRef, emptyMap, emptyUnits, 1 ); + int result50 = tracker->GetNextRefDesForUnits( rRef, emptyMap, emptyUnits, 50 ); } auto end = std::chrono::high_resolution_clock::now(); auto duration = std::chrono::duration_cast( end - start ); - BOOST_TEST_MESSAGE( "200 cached calls took: " + std::to_string( duration.count() ) + " microseconds" ); + BOOST_TEST_MESSAGE( "200 GetNextRefDesForUnits calls took: " + std::to_string( duration.count() ) + " microseconds" ); - // The actual time will vary, but cached calls should be very fast - BOOST_CHECK_LT( duration.count(), 10000 ); // Should be under 10ms for 200 cached calls + // The actual time will vary, but the calls should complete in reasonable time + BOOST_CHECK_LT( duration.count(), 50000 ); // Should be under 50ms for 200 calls } BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file diff --git a/qa/tests/eeschema/test_sch_reference_list.cpp b/qa/tests/eeschema/test_sch_reference_list.cpp index 629c176e07..6135896e87 100644 --- a/qa/tests/eeschema/test_sch_reference_list.cpp +++ b/qa/tests/eeschema/test_sch_reference_list.cpp @@ -248,7 +248,6 @@ BOOST_AUTO_TEST_CASE( Reannotate ) loadTestCase( c.m_SchematicRelativePath, c.m_ExpectedReannotations ); m_refsToReannotate.SetRefDesTracker( m_schematic->Settings().m_refDesTracker ); - m_refsToReannotate.SetReuseRefDes( m_schematic->Settings().m_reuseRefDes ); m_refsToReannotate.RemoveAnnotation(); m_refsToReannotate.SplitReferences(); m_refsToReannotate.Annotate( false, 0, c.m_StartNumber, m_lockedRefs, getAdditionalRefs() ); @@ -290,7 +289,6 @@ BOOST_AUTO_TEST_CASE( ReannotateDuplicates ) loadTestCase( c.m_SchematicRelativePath, c.m_ExpectedReannotations ); m_refsToReannotate.SetRefDesTracker( m_schematic->Settings().m_refDesTracker ); - m_refsToReannotate.SetReuseRefDes( m_schematic->Settings().m_reuseRefDes ); m_refsToReannotate.ReannotateDuplicates( getAdditionalRefs() ); m_refsToReannotate.UpdateAnnotation();