From 9ef05fb762406b50e927c63753f3fa5731e791e5 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Fri, 10 Nov 2023 18:36:02 +0000 Subject: [PATCH] Don't store document values in global PROPERTY_MANAGER. At best it leads to the wrong units being used if they're different between (for instance) PCB Editor and Footprint Editor. (And this will only get worse if we ever to to a single binary.) At worst it causes crashes when accessing freed ORIGIN_TRANSFORMS. Fixes https://gitlab.com/kicad/code/kicad/-/issues/16062 --- common/properties/pg_properties.cpp | 37 ++++++++++++------------- common/widgets/properties_panel.cpp | 16 ++++------- include/properties/pg_properties.h | 30 +++++++++----------- include/properties/property_mgr.h | 29 ++----------------- pcbnew/widgets/pcb_properties_panel.cpp | 3 +- 5 files changed, 39 insertions(+), 76 deletions(-) diff --git a/common/properties/pg_properties.cpp b/common/properties/pg_properties.cpp index e0bc1ee6dc..93b758158b 100644 --- a/common/properties/pg_properties.cpp +++ b/common/properties/pg_properties.cpp @@ -123,13 +123,12 @@ wxPGProperty* PGPropertyFactory( const PROPERTY_BASE* aProperty, EDA_DRAW_FRAME* switch( display ) { case PROPERTY_DISPLAY::PT_SIZE: - ret = new PGPROPERTY_SIZE(); + ret = new PGPROPERTY_SIZE( aFrame ); ret->SetEditor( PG_UNIT_EDITOR::BuildEditorName( aFrame ) ); break; case PROPERTY_DISPLAY::PT_COORD: - ret = new PGPROPERTY_COORD(); - static_cast( ret )->SetCoordType( aProperty->CoordType() ); + ret = new PGPROPERTY_COORD( aFrame, aProperty->CoordType() ); ret->SetEditor( PG_UNIT_EDITOR::BuildEditorName( aFrame ) ); break; @@ -209,8 +208,9 @@ wxPGProperty* PGPropertyFactory( const PROPERTY_BASE* aProperty, EDA_DRAW_FRAME* } -PGPROPERTY_DISTANCE::PGPROPERTY_DISTANCE( const wxString& aRegEx, +PGPROPERTY_DISTANCE::PGPROPERTY_DISTANCE( EDA_DRAW_FRAME* aParentFrame, const wxString& aRegEx, ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType ) : + m_parentFrame( aParentFrame ), m_coordType( aCoordType ) { m_regExValidator.reset( new REGEX_VALIDATOR( aRegEx ) ); @@ -236,22 +236,21 @@ wxString PGPROPERTY_DISTANCE::DistanceToString( wxVariant& aVariant, int aArgFla long distanceIU = aVariant.GetLong(); - ORIGIN_TRANSFORMS* transforms = PROPERTY_MANAGER::Instance().GetTransforms(); - const EDA_IU_SCALE* iuScale = PROPERTY_MANAGER::Instance().GetIuScale(); + ORIGIN_TRANSFORMS& transforms = m_parentFrame->GetOriginTransforms(); + const EDA_IU_SCALE& iuScale = m_parentFrame->GetIuScale(); - if( transforms ) - distanceIU = transforms->ToDisplay( static_cast( distanceIU ), m_coordType ); + distanceIU = transforms.ToDisplay( static_cast( distanceIU ), m_coordType ); - switch( PROPERTY_MANAGER::Instance().GetUnits() ) + switch( m_parentFrame->GetUserUnits() ) { case EDA_UNITS::INCHES: - return wxString::Format( wxS( "%g in" ), iuScale->IUToMils( distanceIU ) / 1000.0 ); + return wxString::Format( wxS( "%g in" ), iuScale.IUToMils( distanceIU ) / 1000.0 ); case EDA_UNITS::MILS: - return wxString::Format( wxS( "%d mils" ), iuScale->IUToMils( distanceIU ) ); + return wxString::Format( wxS( "%d mils" ), iuScale.IUToMils( distanceIU ) ); case EDA_UNITS::MILLIMETRES: - return wxString::Format( wxS( "%g mm" ), iuScale->IUTomm( distanceIU ) ); + return wxString::Format( wxS( "%g mm" ), iuScale.IUTomm( distanceIU ) ); case EDA_UNITS::UNSCALED: return wxString::Format( wxS( "%li" ), distanceIU ); @@ -266,9 +265,9 @@ wxString PGPROPERTY_DISTANCE::DistanceToString( wxVariant& aVariant, int aArgFla } -PGPROPERTY_SIZE::PGPROPERTY_SIZE( const wxString& aLabel, const wxString& aName, - long aValue ) - : wxUIntProperty( aLabel, aName, aValue ), PGPROPERTY_DISTANCE( REGEX_UNSIGNED_DISTANCE ) +PGPROPERTY_SIZE::PGPROPERTY_SIZE( EDA_DRAW_FRAME* aParentFrame ) : + wxUIntProperty( wxPG_LABEL, wxPG_LABEL, 0 ), + PGPROPERTY_DISTANCE( aParentFrame, REGEX_UNSIGNED_DISTANCE, ORIGIN_TRANSFORMS::NOT_A_COORD ) { } @@ -279,10 +278,10 @@ wxValidator* PGPROPERTY_SIZE::DoGetValidator() const } -PGPROPERTY_COORD::PGPROPERTY_COORD( const wxString& aLabel, const wxString& aName, - long aValue, ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType ) : - wxIntProperty( aLabel, aName, aValue ), - PGPROPERTY_DISTANCE( REGEX_SIGNED_DISTANCE, aCoordType ) +PGPROPERTY_COORD::PGPROPERTY_COORD( EDA_DRAW_FRAME* aParentFrame, + ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType ) : + wxIntProperty( wxPG_LABEL, wxPG_LABEL, 0 ), + PGPROPERTY_DISTANCE( aParentFrame, REGEX_SIGNED_DISTANCE, aCoordType ) { } diff --git a/common/widgets/properties_panel.cpp b/common/widgets/properties_panel.cpp index bb7ee3f7bf..2335716b02 100644 --- a/common/widgets/properties_panel.cpp +++ b/common/widgets/properties_panel.cpp @@ -166,13 +166,10 @@ void PROPERTIES_PANEL::rebuildProperties( const SELECTION& aSelection ) wxCHECK( !types.empty(), /* void */ ); - PROPERTY_MANAGER& propMgr = PROPERTY_MANAGER::Instance(); - propMgr.SetUnits( m_frame->GetUserUnits() ); - propMgr.SetIuScale( &m_frame->GetIuScale() ); - propMgr.SetTransforms( &m_frame->GetOriginTransforms() ); - + PROPERTY_MANAGER& propMgr = PROPERTY_MANAGER::Instance(); std::set commonProps; - const PROPERTY_LIST& allProperties = propMgr.GetProperties( *types.begin() ); + const PROPERTY_LIST& allProperties = propMgr.GetProperties( *types.begin() ); + copy( allProperties.begin(), allProperties.end(), inserter( commonProps, commonProps.begin() ) ); PROPERTY_DISPLAY_ORDER displayOrder = propMgr.GetDisplayOrder( *types.begin() ); @@ -346,11 +343,8 @@ bool PROPERTIES_PANEL::extractValueAndWritability( const SELECTION& aSelection, wxVariant& aValue, bool& aWritable ) { PROPERTY_MANAGER& propMgr = PROPERTY_MANAGER::Instance(); - propMgr.SetUnits( m_frame->GetUserUnits() ); - propMgr.SetTransforms( &m_frame->GetOriginTransforms() ); - - bool different = false; - wxVariant commonVal; + bool different = false; + wxVariant commonVal; aWritable = true; diff --git a/include/properties/pg_properties.h b/include/properties/pg_properties.h index e6766c9afd..151578a26a 100644 --- a/include/properties/pg_properties.h +++ b/include/properties/pg_properties.h @@ -41,17 +41,18 @@ wxPGProperty* PGPropertyFactory( const PROPERTY_BASE* aProperty, EDA_DRAW_FRAME* class PGPROPERTY_DISTANCE { public: - PGPROPERTY_DISTANCE( const wxString& aRegEx, - ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType = ORIGIN_TRANSFORMS::NOT_A_COORD ); + PGPROPERTY_DISTANCE( EDA_DRAW_FRAME* aParentFrame, const wxString& aRegEx, + ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType ); virtual ~PGPROPERTY_DISTANCE() = 0; - void SetCoordType( ORIGIN_TRANSFORMS::COORD_TYPES_T aType ) { m_coordType = aType; } ORIGIN_TRANSFORMS::COORD_TYPES_T CoordType() const { return m_coordType; } protected: bool StringToDistance( wxVariant& aVariant, const wxString& aText, int aArgFlags = 0 ) const; wxString DistanceToString( wxVariant& aVariant, int aArgFlags = 0 ) const; +protected: + EDA_DRAW_FRAME* m_parentFrame; std::unique_ptr m_regExValidator; ORIGIN_TRANSFORMS::COORD_TYPES_T m_coordType; }; @@ -60,8 +61,7 @@ protected: class PGPROPERTY_SIZE : public wxUIntProperty, public PGPROPERTY_DISTANCE { public: - PGPROPERTY_SIZE( const wxString& aLabel = wxPG_LABEL, const wxString& aName = wxPG_LABEL, - long aValue = 0 ); + PGPROPERTY_SIZE( EDA_DRAW_FRAME* aParentFrame ); bool StringToValue( wxVariant& aVariant, const wxString& aText, int aArgFlags = 0 ) const override @@ -81,9 +81,7 @@ public: class PGPROPERTY_COORD : public wxIntProperty, public PGPROPERTY_DISTANCE { public: - PGPROPERTY_COORD( const wxString& aLabel = wxPG_LABEL, const wxString& aName = wxPG_LABEL, - long aValue = 0, - ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType = ORIGIN_TRANSFORMS::NOT_A_COORD ); + PGPROPERTY_COORD( EDA_DRAW_FRAME* aParentFrame, ORIGIN_TRANSFORMS::COORD_TYPES_T aCoordType ); bool StringToValue( wxVariant& aVariant, const wxString& aText, int aArgFlags = 0 ) const override @@ -104,9 +102,9 @@ public: class PGPROPERTY_ANGLE : public wxFloatProperty { public: - PGPROPERTY_ANGLE( const wxString& aLabel = wxPG_LABEL, const wxString& aName = wxPG_LABEL, - double aValue = 0 ) - : wxFloatProperty( aLabel, aName, aValue ), m_scale( 1.0 ) + PGPROPERTY_ANGLE() : + wxFloatProperty( wxPG_LABEL, wxPG_LABEL, 0 ), + m_scale( 1.0 ) { } @@ -134,9 +132,8 @@ protected: class PGPROPERTY_COLORENUM : public wxEnumProperty { public: - PGPROPERTY_COLORENUM( const wxString& aLabel, const wxString& aName, wxPGChoices* aChoices, - int aValue = 0 ) : - wxEnumProperty( aLabel, aName, *aChoices, aValue ), + PGPROPERTY_COLORENUM( wxPGChoices* aChoices ) : + wxEnumProperty( wxPG_LABEL, wxPG_LABEL, *aChoices, 0 ), m_colorFunc( []( int aDummy ) { return wxNullColour; } ) { SetFlag( wxPG_PROP_CUSTOMIMAGE ); @@ -164,9 +161,8 @@ protected: class PGPROPERTY_STRING : public wxStringProperty { public: - PGPROPERTY_STRING( const wxString& aLabel = wxPG_LABEL, const wxString& aName = wxPG_LABEL, - const wxString& aValue = wxEmptyString ) : - wxStringProperty( aLabel, aName, aValue ) + PGPROPERTY_STRING() : + wxStringProperty( wxPG_LABEL, wxPG_LABEL, wxEmptyString ) {} virtual ~PGPROPERTY_STRING() = default; diff --git a/include/properties/property_mgr.h b/include/properties/property_mgr.h index 66842d3acb..746e90db2d 100644 --- a/include/properties/property_mgr.h +++ b/include/properties/property_mgr.h @@ -2,7 +2,7 @@ * This program source code file is part of KiCad, a free EDA CAD application. * * Copyright (C) 2020 CERN - * Copyright (C) 2021 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2021-2023 KiCad Developers, see AUTHORS.txt for contributors. * * @author Tomasz Wlostowski * @author Maciej Suminski @@ -230,22 +230,6 @@ public: */ bool IsOfType( TYPE_ID aDerived, TYPE_ID aBase ) const; - EDA_UNITS GetUnits() const - { - return m_units; - } - - void SetUnits( EDA_UNITS aUnits ) - { - m_units = aUnits; - } - - ORIGIN_TRANSFORMS* GetTransforms() const { return m_originTransforms; } - void SetTransforms( ORIGIN_TRANSFORMS* aTransforms ) { m_originTransforms = aTransforms; } - - const EDA_IU_SCALE* GetIuScale() const { return m_iuScale; } - void SetIuScale( const EDA_IU_SCALE* aScale ) { m_iuScale = aScale; } - /** * Rebuild the list of all registered properties. Needs to be called * once before GetProperty()/GetProperties() are used. @@ -267,10 +251,7 @@ public: private: PROPERTY_MANAGER() : - m_dirty( false ), - m_units( EDA_UNITS::MILLIMETRES ), - m_originTransforms( nullptr ), - m_iuScale( &pcbIUScale ) + m_dirty( false ) { } @@ -343,12 +324,6 @@ private: /// Flag indicating that the list of properties needs to be rebuild (RebuildProperties()) bool m_dirty; - - EDA_UNITS m_units; - - ORIGIN_TRANSFORMS* m_originTransforms; - - const EDA_IU_SCALE* m_iuScale; }; diff --git a/pcbnew/widgets/pcb_properties_panel.cpp b/pcbnew/widgets/pcb_properties_panel.cpp index e7315f8a1a..44ac54a245 100644 --- a/pcbnew/widgets/pcb_properties_panel.cpp +++ b/pcbnew/widgets/pcb_properties_panel.cpp @@ -135,8 +135,7 @@ wxPGProperty* PCB_PROPERTIES_PANEL::createPGProperty( const PROPERTY_BASE* aProp boardLayerIDs.push_back( canonicalLayers.GetValue( ii ) ); } - auto ret = new PGPROPERTY_COLORENUM( wxPG_LABEL, wxPG_LABEL, - new wxPGChoices( boardLayerNames, boardLayerIDs ) ); + auto ret = new PGPROPERTY_COLORENUM( new wxPGChoices( boardLayerNames, boardLayerIDs ) ); ret->SetColorFunc( [&]( int aValue ) -> wxColour