From 937e3c2d4890a9b3f892b586a5c1d0432b12cf98 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Thu, 1 Aug 2019 18:10:25 -0600 Subject: [PATCH] Make m_Text private so we don't keep getting bugs where m_shown_text fails to get updated. Fixes: lp:1838655 * https://bugs.launchpad.net/kicad/+bug/1838655 --- common/base_struct.cpp | 3 --- common/eda_text.cpp | 27 ++++++++++++++++------ eeschema/lib_field.cpp | 20 ++++++----------- eeschema/lib_field.h | 21 ----------------- eeschema/lib_text.cpp | 10 ++------- eeschema/lib_text.h | 12 ---------- eeschema/sch_field.cpp | 9 ++++---- eeschema/sch_legacy_plugin.cpp | 15 ++++++++----- eeschema/sch_sheet.h | 7 ++++-- eeschema/sch_sheet_pin.cpp | 41 ++++++++-------------------------- eeschema/sch_text.cpp | 24 +++++++------------- eeschema/sch_text.h | 7 ++++-- include/base_struct.h | 2 +- include/eda_text.h | 24 ++++++++++++++------ pcbnew/class_text_mod.cpp | 14 ++++++------ 15 files changed, 95 insertions(+), 141 deletions(-) diff --git a/common/base_struct.cpp b/common/base_struct.cpp index 6b4288feb2..63ff40a359 100644 --- a/common/base_struct.cpp +++ b/common/base_struct.cpp @@ -182,9 +182,6 @@ bool EDA_ITEM::Matches( const wxString& aText, wxFindReplaceData& aSearchData ) bool EDA_ITEM::Replace( wxFindReplaceData& aSearchData, wxString& aText ) { - wxCHECK_MSG( IsReplaceable(), false, - wxT( "Attempt to replace text in <" ) + GetClass() + wxT( "> item." ) ); - wxString searchString = (aSearchData.GetFlags() & wxFR_MATCHCASE) ? aText : aText.Upper(); int result = searchString.Find( (aSearchData.GetFlags() & wxFR_MATCHCASE) ? diff --git a/common/eda_text.cpp b/common/eda_text.cpp index ed2f0d207a..6abbb0aa94 100644 --- a/common/eda_text.cpp +++ b/common/eda_text.cpp @@ -87,8 +87,8 @@ EDA_TEXT_VJUSTIFY_T EDA_TEXT::MapVertJustify( int aVertJustify ) EDA_TEXT::EDA_TEXT( const wxString& text ) : - m_Text( text ), - m_e( 1<( aData ) : m_Text; + wxString text = aData ? *static_cast( aData ) : GetText(); GRText( aDC, text_pos, color, text, GetTextAngle(), GetTextSize(), GetHorizJustify(), GetVertJustify(), linewidth, IsItalic(), IsBold() ); @@ -135,8 +135,8 @@ void LIB_FIELD::print( wxDC* aDC, const wxPoint& aOffset, void* aData, bool LIB_FIELD::HitTest( const wxPoint& aPosition, int aAccuracy ) const { - // Because HitTest is mainly used to select the field return false if it is void - if( IsVoid() ) + // Because HitTest is mainly used to select the field return false if it is empty + if( GetText().IsEmpty() ) return false; // Build a temporary copy of the text for hit testing @@ -177,9 +177,9 @@ EDA_ITEM* LIB_FIELD::Clone() const void LIB_FIELD::Copy( LIB_FIELD* aTarget ) const { - aTarget->m_Text = m_Text; aTarget->m_name = m_name; + aTarget->SetText( GetText() ); aTarget->SetEffects( *this ); aTarget->SetParent( m_Parent ); } @@ -194,7 +194,7 @@ int LIB_FIELD::compare( const LIB_ITEM& other ) const if( m_id != tmp->m_id ) return m_id - tmp->m_id; - int result = m_Text.CmpNoCase( tmp->m_Text ); + int result = GetText().CmpNoCase( tmp->GetText() ); if( result != 0 ) return result; @@ -272,7 +272,7 @@ void LIB_FIELD::Rotate( const wxPoint& center, bool aRotateCCW ) void LIB_FIELD::Plot( PLOTTER* aPlotter, const wxPoint& aOffset, bool aFill, const TRANSFORM& aTransform ) { - if( IsVoid() ) + if( GetText().IsEmpty() ) return; // Calculate the text orientation, according to the component orientation/mirror @@ -409,12 +409,6 @@ void LIB_FIELD::SetName( const wxString& aName ) } -void LIB_FIELD::SetText( const wxString& aText ) -{ - m_Text = aText; -} - - wxString LIB_FIELD::GetSelectMenuText( EDA_UNITS_T aUnits ) const { return wxString::Format( _( "Field %s \"%s\"" ), GetName(), ShortenedShownText() ); diff --git a/eeschema/lib_field.h b/eeschema/lib_field.h index efebe66a40..d5c3103c21 100644 --- a/eeschema/lib_field.h +++ b/eeschema/lib_field.h @@ -144,14 +144,6 @@ public: */ void Copy( LIB_FIELD* aTarget ) const; - /** - * @return true if the field value is void (no text in this field) - */ - bool IsVoid() const - { - return m_Text.IsEmpty(); - } - void ViewGetLayers( int aLayers[], int& aCount ) const override; const EDA_RECT GetBoundingBox() const override; @@ -178,19 +170,6 @@ public: void BeginEdit( const wxPoint aStartPoint ) override; - /** - * Sets the field text to \a aText. - * - * This method does more than just set the set the field text. There are special - * cases when changing the text string alone is not enough. If the field is the - * value field, the parent component's name is changed as well. If the field is - * being moved, the name change must be delayed until the next redraw to prevent - * drawing artifacts. - * - * @param aText - New text value. - */ - void SetText( const wxString& aText ) override; - void Offset( const wxPoint& aOffset ) override; bool Inside( EDA_RECT& aRect ) const override; diff --git a/eeschema/lib_text.cpp b/eeschema/lib_text.cpp index 8d0a52191c..b6b88f533d 100644 --- a/eeschema/lib_text.cpp +++ b/eeschema/lib_text.cpp @@ -81,8 +81,8 @@ EDA_ITEM* LIB_TEXT::Clone() const newitem->m_Unit = m_Unit; newitem->m_Convert = m_Convert; newitem->m_Flags = m_Flags; - newitem->m_Text = m_Text; + newitem->SetText( GetText() ); newitem->SetEffects( *this ); return newitem; @@ -95,7 +95,7 @@ int LIB_TEXT::compare( const LIB_ITEM& other ) const const LIB_TEXT* tmp = ( LIB_TEXT* ) &other; - int result = m_Text.CmpNoCase( tmp->m_Text ); + int result = GetText().CmpNoCase( tmp->GetText() ); if( result != 0 ) return result; @@ -293,12 +293,6 @@ const EDA_RECT LIB_TEXT::GetBoundingBox() const } -void LIB_TEXT::SetText( const wxString& aText ) -{ - m_Text = aText; -} - - wxString LIB_TEXT::GetSelectMenuText( EDA_UNITS_T aUnits ) const { return wxString::Format( _( "Graphic Text \"%s\"" ), ShortenedShownText() ); diff --git a/eeschema/lib_text.h b/eeschema/lib_text.h index f87fb6d888..b118d37430 100644 --- a/eeschema/lib_text.h +++ b/eeschema/lib_text.h @@ -61,18 +61,6 @@ public: void ViewGetLayers( int aLayers[], int& aCount ) const override; - /** - * Sets the text item string to \a aText. - * - * This method does more than just set the set the text string. There are special - * cases when changing the text string alone is not enough. If the text item is - * being moved, the name change must be delayed until the next redraw to prevent - * drawing artifacts. - * - * @param aText - New text value. - */ - void SetText( const wxString& aText ) override; - bool HitTest( const wxPoint& aPosition, int aAccuracy = 0 ) const override; bool HitTest( const EDA_RECT& aRect, bool aContained, int aAccuracy = 0 ) const override diff --git a/eeschema/sch_field.cpp b/eeschema/sch_field.cpp index 1494339651..13dbd0a800 100644 --- a/eeschema/sch_field.cpp +++ b/eeschema/sch_field.cpp @@ -74,7 +74,7 @@ EDA_ITEM* SCH_FIELD::Clone() const const wxString SCH_FIELD::GetFullyQualifiedText() const { - wxString text = m_Text; + wxString text = GetText(); /* For more than one part per package, we must add the part selection * A, B, ... or 1, 2, .. to the reference. */ @@ -185,9 +185,8 @@ void SCH_FIELD::SwapData( SCH_ITEM* aItem ) SCH_FIELD* item = (SCH_FIELD*) aItem; - std::swap( m_Text, item->m_Text ); std::swap( m_Layer, item->m_Layer ); - + SwapText( *item ); SwapEffects( *item ); } @@ -257,7 +256,7 @@ bool SCH_FIELD::IsHorizJustifyFlipped() const bool SCH_FIELD::IsVoid() const { - return m_Text.Len() == 0; + return GetText().Len() == 0; } @@ -347,7 +346,7 @@ bool SCH_FIELD::Replace( wxFindReplaceData& aSearchData, void* aAuxData ) } else { - isReplaced = EDA_ITEM::Replace( aSearchData, m_Text ); + isReplaced = EDA_TEXT::Replace( aSearchData ); } return isReplaced; diff --git a/eeschema/sch_legacy_plugin.cpp b/eeschema/sch_legacy_plugin.cpp index 76fe320420..4f68d43bac 100644 --- a/eeschema/sch_legacy_plugin.cpp +++ b/eeschema/sch_legacy_plugin.cpp @@ -2923,6 +2923,7 @@ void SCH_LEGACY_PLUGIN_CACHE::loadField( std::unique_ptr& aPart, wxCHECK_RET( *line == 'F', "Invalid field line" ); + wxString text; int id; if( sscanf( line + 1, "%d", &id ) != 1 || id < 0 ) @@ -2952,12 +2953,16 @@ void SCH_LEGACY_PLUGIN_CACHE::loadField( std::unique_ptr& aPart, if( *line == 0 ) SCH_PARSE_ERROR( _( "unexpected end of line" ), aReader, line ); - parseQuotedString( field->m_Text, aReader, line, &line, true ); + parseQuotedString( text, aReader, line, &line, true ); + + field->SetText( text ); // Doctor the *.lib file field which has a "~" in blank fields. New saves will // not save like this. - if( field->m_Text.size() == 1 && field->m_Text[0] == '~' ) - field->m_Text.clear(); + if( text.size() == 1 && text[0] == '~' ) + field->SetText( wxEmptyString ); + else + field->SetText( text ); wxPoint pos; @@ -3050,7 +3055,7 @@ void SCH_LEGACY_PLUGIN_CACHE::loadField( std::unique_ptr& aPart, // Ensure the VALUE field = the part name (can be not the case // with malformed libraries: edited by hand, or converted from other tools) if( id == VALUE ) - field->m_Text = aPart->GetName(); + field->SetText( aPart->GetName() ); } else { @@ -3873,7 +3878,7 @@ void SCH_LEGACY_PLUGIN_CACHE::saveField( LIB_FIELD* aField, int hjustify, vjustify; int id = aField->GetId(); - wxString text = aField->m_Text; + wxString text = aField->GetText(); hjustify = 'C'; diff --git a/eeschema/sch_sheet.h b/eeschema/sch_sheet.h index 3e4afb4545..8c2d736825 100644 --- a/eeschema/sch_sheet.h +++ b/eeschema/sch_sheet.h @@ -167,11 +167,14 @@ public: void MirrorY( int aYaxis_position ) override; void Rotate( wxPoint aPosition ) override; - bool Matches( wxFindReplaceData& aSearchData, void* aAuxData ) override; + bool Matches( wxFindReplaceData& aSearchData, void* aAuxData ) override + { + return SCH_ITEM::Matches( GetText(), aSearchData ); + } bool Replace( wxFindReplaceData& aSearchData, void* aAuxData = NULL ) override { - return EDA_ITEM::Replace( aSearchData, m_Text ); + return EDA_TEXT::Replace( aSearchData ); } bool IsReplaceable() const override { return true; } diff --git a/eeschema/sch_sheet_pin.cpp b/eeschema/sch_sheet_pin.cpp index 5b34fa149f..4fd476210c 100644 --- a/eeschema/sch_sheet_pin.cpp +++ b/eeschema/sch_sheet_pin.cpp @@ -28,14 +28,11 @@ #include #include #include -#include #include #include - #include #include #include -#include SCH_SHEET_PIN::SCH_SHEET_PIN( SCH_SHEET* parent, const wxPoint& pos, const wxString& text ) : @@ -195,17 +192,6 @@ void SCH_SHEET_PIN::ConstrainOnEdge( wxPoint Pos ) } -bool SCH_SHEET_PIN::Matches( wxFindReplaceData& aSearchData, void* aAuxData ) -{ - wxCHECK_MSG( GetParent() != NULL, false, - wxT( "Sheet pin " ) + m_Text + wxT( " does not have a parent sheet!" ) ); - - wxLogTrace( traceFindItem, wxT( " child item " ) + GetSelectMenuText( MILLIMETRES ) ); - - return SCH_ITEM::Matches( m_Text, aSearchData ); -} - - void SCH_SHEET_PIN::MirrorX( int aXaxis_position ) { int p = GetTextPos().y - aXaxis_position; @@ -255,8 +241,9 @@ void SCH_SHEET_PIN::Rotate( wxPoint aPosition ) void SCH_SHEET_PIN::CreateGraphicShape( std::vector & aPoints, const wxPoint& aPos ) { - /* This is the same icon shapes as SCH_HIERLABEL - * but the graphic icon is slightly different in 2 cases: + /* + * These are the same icon shapes as SCH_HIERLABEL but the graphic icon is slightly + * different in 2 cases: * for INPUT type the icon is the OUTPUT shape of SCH_HIERLABEL * for OUTPUT type the icon is the INPUT shape of SCH_HIERLABEL */ @@ -264,16 +251,9 @@ void SCH_SHEET_PIN::CreateGraphicShape( std::vector & aPoints, const wx switch( m_shape ) { - case NET_INPUT: - m_shape = NET_OUTPUT; - break; - - case NET_OUTPUT: - m_shape = NET_INPUT; - break; - - default: - break; + case NET_INPUT: m_shape = NET_OUTPUT; break; + case NET_OUTPUT: m_shape = NET_INPUT; break; + default: break; } SCH_HIERLABEL::CreateGraphicShape( aPoints, aPos ); @@ -293,6 +273,7 @@ wxString SCH_SHEET_PIN::GetSelectMenuText( EDA_UNITS_T aUnits ) const return wxString::Format( _( "Hierarchical Sheet Pin %s" ), ShortenedShownText() ); } + BITMAP_DEF SCH_SHEET_PIN::GetMenuImage() const { return add_hierar_pin_xpm; @@ -314,13 +295,9 @@ bool SCH_SHEET_PIN::HitTest( const wxPoint& aPoint, int aAccuracy ) const void SCH_SHEET_PIN::Show( int nestLevel, std::ostream& os ) const { // XML output: - wxString s = GetClass(); - - NestedSpace( nestLevel, os ) << '<' << s.Lower().mb_str() << ">" - << " pin_name=\"" << TO_UTF8( m_Text ) + NestedSpace( nestLevel, os ) << '<' << GetClass().Lower().mb_str() << ">" + << " pin_name=\"" << TO_UTF8( GetText() ) << '"' << "/>\n" << std::flush; - -// NestedSpace( nestLevel, os ) << "\n"; } #endif diff --git a/eeschema/sch_text.cpp b/eeschema/sch_text.cpp index 791f72f550..29b261f8aa 100644 --- a/eeschema/sch_text.cpp +++ b/eeschema/sch_text.cpp @@ -138,8 +138,9 @@ EDA_ITEM* SCH_TEXT::Clone() const void SCH_TEXT::IncrementLabel( int aIncrement ) { - IncrementLabelMember( m_Text, aIncrement ); - m_shown_text = UnescapeString( m_Text ); + wxString text = GetText(); + IncrementLabelMember( text, aIncrement ); + SetText(text ); } @@ -163,14 +164,6 @@ wxPoint SCH_TEXT::GetSchematicTextOffset() const } -bool SCH_TEXT::Matches( wxFindReplaceData& aSearchData, void* aAuxData ) -{ - wxLogTrace( traceFindItem, wxT( " item " ) + GetSelectMenuText( MILLIMETRES ) ); - - return SCH_ITEM::Matches( m_Text, aSearchData ); -} - - void SCH_TEXT::MirrorY( int aYaxis_position ) { // Text is NOT really mirrored; it is moved to a suitable horizontal position @@ -274,14 +267,13 @@ void SCH_TEXT::SwapData( SCH_ITEM* aItem ) { SCH_TEXT* item = (SCH_TEXT*) aItem; - std::swap( m_Text, item->m_Text ); - std::swap( m_shown_text, item->m_shown_text ); std::swap( m_Layer, item->m_Layer ); std::swap( m_shape, item->m_shape ); std::swap( m_isDangling, item->m_isDangling ); std::swap( m_spin_style, item->m_spin_style ); + SwapText( *item ); SwapEffects( *item ); } @@ -478,13 +470,13 @@ void SCH_TEXT::GetNetListItem( NETLIST_OBJECT_LIST& aNetListItems, else if( GetLayer() == LAYER_HIERLABEL ) item->m_Type = NET_HIERLABEL; - item->m_Label = m_Text; + item->m_Label = GetText(); item->m_Start = item->m_End = GetTextPos(); aNetListItems.push_back( item ); // If a bus connects to label - if( Connection( *aSheetPath )->IsBusLabel( m_Text ) ) + if( Connection( *aSheetPath )->IsBusLabel( GetText() ) ) item->ConvertBusToNetListItems( aNetListItems ); } @@ -639,7 +631,7 @@ void SCH_TEXT::Show( int nestLevel, std::ostream& os ) const << " shape=\"" << m_shape << '"' << " dangling=\"" << m_isDangling << '"' << '>' - << TO_UTF8( m_Text ) + << TO_UTF8( GetText() ) << "\n"; } @@ -857,7 +849,7 @@ void SCH_GLOBALLABEL::CreateGraphicShape( std::vector & aPoints, const // Use negation bar Y position to calculate full vertical size // Search for overbar symbol - wxString test = m_Text; + wxString test = GetText(); test.Replace( "~~", "" ); bool hasOverBar = test.find( "~" ) != wxString::npos; diff --git a/eeschema/sch_text.h b/eeschema/sch_text.h index b6b31ecc9f..b37752e847 100644 --- a/eeschema/sch_text.h +++ b/eeschema/sch_text.h @@ -161,11 +161,14 @@ public: void MirrorX( int aXaxis_position ) override; void Rotate( wxPoint aPosition ) override; - bool Matches( wxFindReplaceData& aSearchData, void* aAuxData ) override; + bool Matches( wxFindReplaceData& aSearchData, void* aAuxData ) override + { + return SCH_ITEM::Matches( GetText(), aSearchData ); + } bool Replace( wxFindReplaceData& aSearchData, void* aAuxData ) override { - return EDA_ITEM::Replace( aSearchData, m_Text ); + return EDA_TEXT::Replace( aSearchData ); } virtual bool IsReplaceable() const override { return true; } diff --git a/include/base_struct.h b/include/base_struct.h index 9bd05495e6..ecb2aadc5b 100644 --- a/include/base_struct.h +++ b/include/base_struct.h @@ -506,7 +506,7 @@ public: * replaced. * @return True if \a aText was modified, otherwise false. */ - bool Replace( wxFindReplaceData& aSearchData, wxString& aText ); + static bool Replace( wxFindReplaceData& aSearchData, wxString& aText ); /** * Function Replace diff --git a/include/eda_text.h b/include/eda_text.h index 41986bf8a9..3b1650849a 100644 --- a/include/eda_text.h +++ b/include/eda_text.h @@ -121,7 +121,7 @@ public: * * @return a const wxString reference containing the string of the item. */ - virtual const wxString& GetText() const { return m_Text; } + virtual const wxString& GetText() const { return m_text; } /** * Returns the string actually shown after processing of the base @@ -203,6 +203,18 @@ public: */ void SwapEffects( EDA_TEXT& aTradingPartner ); + void SwapText( EDA_TEXT& aTradingPartner ); + + /** + * Helper function used in search and replace dialog + * performs a text replace using the find and replace criteria in \a aSearchData. + * + * @param aSearchData A reference to a wxFindReplaceData object containing the + * search and replace criteria. + * @return True if the text item was modified, otherwise false. + */ + bool Replace( wxFindReplaceData& aSearchData ); + bool IsDefaultFormatting() const; void SetTextSize( const wxSize& aNewSize ) { m_e.size = aNewSize; }; @@ -222,7 +234,7 @@ public: void Offset( const wxPoint& aOffset ) { m_e.pos += aOffset; } - void Empty() { m_Text.Empty(); } + void Empty() { m_text.Empty(); } static int MapOrientation( KICAD_T labelType, int aOrientation ); @@ -352,11 +364,9 @@ public: */ virtual void Format( OUTPUTFORMATTER* aFormatter, int aNestLevel, int aControlBits ) const; -protected: - wxString m_Text; - - /// Cache of unescaped text for efficient access - wxString m_shown_text; +private: + wxString m_text; + wxString m_shown_text; // Cache of unescaped text for efficient access private: /** diff --git a/pcbnew/class_text_mod.cpp b/pcbnew/class_text_mod.cpp index d006323b91..27c94e2b63 100644 --- a/pcbnew/class_text_mod.cpp +++ b/pcbnew/class_text_mod.cpp @@ -204,7 +204,7 @@ void TEXTE_MODULE::Move( const wxPoint& aMoveVector ) int TEXTE_MODULE::GetLength() const { - return m_Text.Len(); + return GetText().Len(); } @@ -461,11 +461,11 @@ unsigned int TEXTE_MODULE::ViewGetLOD( int aLayer, KIGFX::VIEW* aView ) const return HIDE; // Handle Render tab switches - if( ( m_Type == TEXT_is_VALUE || m_Text == wxT( "%V" ) ) + if( ( m_Type == TEXT_is_VALUE || GetText() == wxT( "%V" ) ) && !aView->IsLayerVisible( LAYER_MOD_VALUES ) ) return HIDE; - if( ( m_Type == TEXT_is_REFERENCE || m_Text == wxT( "%R" ) ) + if( ( m_Type == TEXT_is_REFERENCE || GetText() == wxT( "%R" ) ) && !aView->IsLayerVisible( LAYER_MOD_REFERENCES ) ) return HIDE; @@ -495,13 +495,13 @@ wxString TEXTE_MODULE::GetShownText() const * Also it seems wise to only expand macros in user text (but there * is no technical reason, probably) */ - if( (m_Type != TEXT_is_DIVERS) || (wxString::npos == m_Text.find('%')) ) - return m_Text; + if( (m_Type != TEXT_is_DIVERS) || (wxString::npos == GetText().find('%')) ) + return GetText(); wxString newbuf; const MODULE *module = static_cast( GetParent() ); - for( wxString::const_iterator it = m_Text.begin(); it != m_Text.end(); ++it ) + for( wxString::const_iterator it = GetText().begin(); it != GetText().end(); ++it ) { // Process '%' and copy everything else if( *it != '%' ) @@ -512,7 +512,7 @@ wxString TEXTE_MODULE::GetShownText() const * its expansion */ ++it; - if( it != m_Text.end() ) + if( it != GetText().end() ) { switch( char(*it) ) {