diff --git a/common/properties/property_mgr.cpp b/common/properties/property_mgr.cpp index d9e300057c..f81aa27e69 100644 --- a/common/properties/property_mgr.cpp +++ b/common/properties/property_mgr.cpp @@ -151,8 +151,8 @@ PROPERTY_BASE& PROPERTY_MANAGER::AddProperty( PROPERTY_BASE* aProperty, const wx } -PROPERTY_BASE& PROPERTY_MANAGER::ReplaceProperty( size_t aBase, const wxString& aName, PROPERTY_BASE* aNew, - const wxString& aGroup ) +PROPERTY_BASE& PROPERTY_MANAGER::ReplaceProperty( size_t aBase, const wxString& aName, + PROPERTY_BASE* aNew, const wxString& aGroup ) { CLASS_DESC& classDesc = getClass( aNew->OwnerHash() ); classDesc.m_replaced.insert( std::make_pair( aBase, aName ) ); diff --git a/common/widgets/properties_panel.cpp b/common/widgets/properties_panel.cpp index 0262ec545a..919e257ff2 100644 --- a/common/widgets/properties_panel.cpp +++ b/common/widgets/properties_panel.cpp @@ -74,6 +74,7 @@ PROPERTIES_PANEL::PROPERTIES_PANEL( wxWindow* aParent, EDA_BASE_FRAME* aFrame ) wxPG_DEFAULT_STYLE ); m_grid->SetUnspecifiedValueAppearance( wxPGCell( wxT( "<...>" ) ) ); m_grid->SetExtraStyle( wxPG_EX_HELP_AS_TOOLTIPS ); + m_grid->SetValidationFailureBehavior( wxPG_VFB_MARK_CELL ); m_grid->AddActionTrigger( wxPG_ACTION_NEXT_PROPERTY, WXK_RETURN ); m_grid->AddActionTrigger( wxPG_ACTION_NEXT_PROPERTY, WXK_NUMPAD_ENTER ); m_grid->AddActionTrigger( wxPG_ACTION_NEXT_PROPERTY, WXK_DOWN ); diff --git a/include/properties/property.h b/include/properties/property.h index caefa33719..e668f8da3d 100644 --- a/include/properties/property.h +++ b/include/properties/property.h @@ -27,6 +27,7 @@ #include <core/wx_stl_compat.h> #include <origin_transforms.h> #include <properties/eda_angle_variant.h> +#include <properties/property_validator.h> #include <wx/any.h> #include <wx/string.h> @@ -187,7 +188,8 @@ PROPERTY_BASE( const wxString& aName, PROPERTY_DISPLAY aDisplay = PT_DEFAULT, m_isInternal( false ), m_isDeprecated( false ), m_availFunc( [](INSPECTABLE*)->bool { return true; } ), - m_writeableFunc( [](INSPECTABLE*)->bool { return true; } ) + m_writeableFunc( [](INSPECTABLE*)->bool { return true; } ), + m_validator( NullValidator ) { } @@ -294,6 +296,22 @@ PROPERTY_BASE( const wxString& aName, PROPERTY_DISPLAY aDisplay = PT_DEFAULT, wxString Group() const { return m_group; } PROPERTY_BASE& SetGroup( const wxString& aGroup ) { m_group = aGroup; return *this; } + PROPERTY_BASE& SetValidator( PROPERTY_VALIDATOR_FN&& aValidator ) + { + m_validator = aValidator; + return *this; + } + + VALIDATOR_RESULT Validate( const wxAny&& aValue, EDA_ITEM* aItem ) + { + return m_validator( std::move( aValue ), aItem ); + } + + static VALIDATOR_RESULT NullValidator( const wxAny&& aValue, EDA_ITEM* aItem ) + { + return std::nullopt; + } + protected: template<typename T> void set( void* aObject, T aValue ) @@ -338,8 +356,17 @@ private: virtual wxAny getter( const void* aObject ) const = 0; private: - const wxString m_name; + /** + * Permanent identifier for this property. Property names are an API contract; changing them + * after release will impact the Custom DRC Rules system as well as the automatic API binding + * system. Never rename properties; instead deprecate them and hide them from the GUI. + */ + const wxString m_name; + + /// The display style controls how properties are edited in the properties manager GUI PROPERTY_DISPLAY m_display; + + /// The coordinate type controls how distances are mapped to the user coordinate system ORIGIN_TRANSFORMS::COORD_TYPES_T m_coordType; /// Internal properties are hidden from the GUI but not from the rules editor autocomplete @@ -355,6 +382,8 @@ private: std::function<bool(INSPECTABLE*)> m_writeableFunc; ///< Eval to determine if prop is read-only + PROPERTY_VALIDATOR_FN m_validator; + friend class INSPECTABLE; }; diff --git a/include/properties/property_validator.h b/include/properties/property_validator.h new file mode 100644 index 0000000000..f6b58bfae2 --- /dev/null +++ b/include/properties/property_validator.h @@ -0,0 +1,53 @@ +/* +* This program source code file is part of KiCad, a free EDA CAD application. +* +* Copyright (C) 2023 Jon Evans <jon@craftyjon.com> +* Copyright (C) 2023 KiCad Developers, see AUTHORS.txt for contributors. +* +* This program is free software: you can redistribute it and/or modify it +* under the terms of the GNU General Public License as published by the +* Free Software Foundation, either version 3 of the License, or (at your +* option) any later version. +* +* This program is distributed in the hope that it will be useful, but +* WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +* General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#ifndef KICAD_PROPERTY_VALIDATOR_H +#define KICAD_PROPERTY_VALIDATOR_H + +#include <functional> +#include <optional> + +#include <wx/any.h> +#include <wx/string.h> + +class EDA_ITEM; +class UNITS_PROVIDER; + +/// Represents an error returned by a validator and contains enough data to format an error message +class VALIDATION_ERROR +{ +public: + virtual ~VALIDATION_ERROR() = default; + + virtual wxString Format( UNITS_PROVIDER* aUnits ) const = 0; +}; + +// TODO: This is a bit of code smell; maybe create a wrapper class to hide the ptr +/// Null optional means validation succeeded +using VALIDATOR_RESULT = std::optional<std::unique_ptr<VALIDATION_ERROR>>; + +/** +* A property validator function takes in the data type of the owning property, and returns a +* VALIDATOR_RESULT that will be empty (null) if validation succeeded, and contain an error message +* otherwise. +*/ +using PROPERTY_VALIDATOR_FN = std::function<VALIDATOR_RESULT( const wxAny&&, EDA_ITEM* aItem )>; + +#endif //KICAD_PROPERTY_VALIDATOR_H diff --git a/include/properties/property_validators.h b/include/properties/property_validators.h new file mode 100644 index 0000000000..b6c89e257d --- /dev/null +++ b/include/properties/property_validators.h @@ -0,0 +1,132 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright (C) 2023 Jon Evans <jon@craftyjon.com> + * Copyright (C) 2023 KiCad Developers, see AUTHORS.txt for contributors. + * + * This program is free software: you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation, either version 3 of the License, or (at your + * option) any later version. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef KICAD_PROPERTY_VALIDATORS_H +#define KICAD_PROPERTY_VALIDATORS_H + +#include <properties/property_validator.h> +#include <units_provider.h> + +/* + * This file includes some standard validators and errors. Include it only where needed, use + * properties/property_validator.h for most things. + */ + + +template<typename T> +class VALIDATION_ERROR_TOO_LARGE : public VALIDATION_ERROR +{ +public: + T Actual; + T Maximum; + EDA_DATA_TYPE DataType; + + VALIDATION_ERROR_TOO_LARGE( T aActual, T aMaximum, + EDA_DATA_TYPE aType = EDA_DATA_TYPE::DISTANCE ) : + Actual( aActual ), + Maximum( aMaximum ), + DataType( aType ) + {} + + wxString Format( UNITS_PROVIDER* aUnits ) const override + { + return wxString::Format( wxS( "Value must be less than or equal to %s" ), + aUnits->StringFromValue( Maximum, true ) ); + } +}; + + +template<typename T> +class VALIDATION_ERROR_TOO_SMALL : public VALIDATION_ERROR +{ +public: + T Actual; + T Minimum; + EDA_DATA_TYPE DataType; + + VALIDATION_ERROR_TOO_SMALL( T aActual, T aMinimum, + EDA_DATA_TYPE aType = EDA_DATA_TYPE::DISTANCE ) : + Actual( aActual ), + Minimum( aMinimum ), + DataType( aType ) + {} + + wxString Format( UNITS_PROVIDER* aUnits ) const override + { + return wxString::Format( wxS( "Value must be greater than or equal to %s" ), + aUnits->StringFromValue( Minimum, true ) ); + } +}; + + +/** + * A validator for use when you just need to return an error string rather than also packaging some + * other data (for example, a limit number) + */ +class VALIDATION_ERROR_MSG : public VALIDATION_ERROR +{ +public: + wxString Message; + + VALIDATION_ERROR_MSG( const wxString& aMessage ) : Message( aMessage ) {} + + wxString Format( UNITS_PROVIDER* aUnits ) const override + { + return Message; + } +}; + + +/** + * A set of generic validators + */ +class PROPERTY_VALIDATORS +{ +public: + + template<int Min, int Max> + static VALIDATOR_RESULT RangeIntValidator( const wxAny&& aValue, EDA_ITEM* aItem ) + { + wxASSERT_MSG( aValue.CheckType<int>(), "Expecting int-containing value" ); + + int val = aValue.As<int>(); + + if( val > Max ) + return std::make_unique<VALIDATION_ERROR_TOO_LARGE<int>>( val, Max ); + else if( val < Min ) + return std::make_unique<VALIDATION_ERROR_TOO_SMALL<int>>( val, Min ); + + return std::nullopt; + } + + static VALIDATOR_RESULT PositiveIntValidator( const wxAny&& aValue, EDA_ITEM* aItem ) + { + wxASSERT_MSG( aValue.CheckType<int>(), "Expecting int-containing value" ); + + int val = aValue.As<int>(); + + if( val < 0 ) + return std::make_unique<VALIDATION_ERROR_TOO_SMALL<int>>( val, 0 ); + + return std::nullopt; + } +}; + +#endif //KICAD_PROPERTY_VALIDATORS_H diff --git a/pcbnew/dialogs/dialog_copper_zones.cpp b/pcbnew/dialogs/dialog_copper_zones.cpp index 4432334f21..711c48a0df 100644 --- a/pcbnew/dialogs/dialog_copper_zones.cpp +++ b/pcbnew/dialogs/dialog_copper_zones.cpp @@ -347,26 +347,8 @@ bool DIALOG_COPPER_ZONE::TransferDataToWindow() m_gridStyleRotation.SetUnits( EDA_UNITS::DEGREES ); m_gridStyleRotation.SetAngleValue( m_settings.m_HatchOrientation ); - - // Gives a reasonable value to grid style parameters, if currently there are no defined - // parameters for grid pattern thickness and gap (if the value is 0) - // the grid pattern thickness default value is (arbitrary) m_ZoneMinThickness * 4 - // or 1mm - // the grid pattern gap default value is (arbitrary) m_ZoneMinThickness * 6 - // or 1.5 mm - int bestvalue = m_settings.m_HatchThickness; - - if( bestvalue <= 0 ) // No defined value for m_HatchThickness - bestvalue = std::max( m_settings.m_ZoneMinThickness * 4, pcbIUScale.mmToIU( 1.0 ) ); - - m_gridStyleThickness.SetValue( std::max( bestvalue, m_settings.m_ZoneMinThickness ) ); - - bestvalue = m_settings.m_HatchGap; - - if( bestvalue <= 0 ) // No defined value for m_HatchGap - bestvalue = std::max( m_settings.m_ZoneMinThickness * 6, pcbIUScale.mmToIU( 1.5 ) ); - - m_gridStyleGap.SetValue( std::max( bestvalue, m_settings.m_ZoneMinThickness ) ); + m_gridStyleThickness.SetValue( m_settings.m_HatchThickness ); + m_gridStyleGap.SetValue( m_settings.m_HatchGap ); m_spinCtrlSmoothLevel->SetValue( m_settings.m_HatchSmoothingLevel ); m_spinCtrlSmoothValue->SetValue( m_settings.m_HatchSmoothingValue ); diff --git a/pcbnew/widgets/pcb_properties_panel.cpp b/pcbnew/widgets/pcb_properties_panel.cpp index ecdd7c7751..8a57fa682c 100644 --- a/pcbnew/widgets/pcb_properties_panel.cpp +++ b/pcbnew/widgets/pcb_properties_panel.cpp @@ -172,19 +172,54 @@ wxPGProperty* PCB_PROPERTIES_PANEL::createPGProperty( const PROPERTY_BASE* aProp } -void PCB_PROPERTIES_PANEL::valueChanged( wxPropertyGridEvent& aEvent ) +PROPERTY_BASE* PCB_PROPERTIES_PANEL::getPropertyFromEvent( const wxPropertyGridEvent& aEvent ) const { PCB_SELECTION_TOOL* selectionTool = m_frame->GetToolManager()->GetTool<PCB_SELECTION_TOOL>(); const SELECTION& selection = selectionTool->GetSelection(); BOARD_ITEM* firstItem = static_cast<BOARD_ITEM*>( selection.Front() ); - wxCHECK_MSG( firstItem, /* void */, - wxT( "valueChanged for a property with nothing selected!") ); + wxCHECK_MSG( firstItem, nullptr, + wxT( "getPropertyFromEvent for a property with nothing selected!") ); PROPERTY_BASE* property = m_propMgr.GetProperty( TYPE_HASH( *firstItem ), aEvent.GetPropertyName() ); - wxCHECK_MSG( property, /* void */, - wxT( "valueChanged for a property not found on the selected item!" ) ); + wxCHECK_MSG( property, nullptr, + wxT( "getPropertyFromEvent for a property not found on the selected item!" ) ); + + return property; +} + + +void PCB_PROPERTIES_PANEL::valueChanging( wxPropertyGridEvent& aEvent ) +{ + PCB_SELECTION_TOOL* selectionTool = m_frame->GetToolManager()->GetTool<PCB_SELECTION_TOOL>(); + const SELECTION& selection = selectionTool->GetSelection(); + EDA_ITEM* item = selection.Front(); + + PROPERTY_BASE* property = getPropertyFromEvent( aEvent ); + wxCHECK( property, /* void */ ); + wxCHECK( item, /* void */ ); + + wxVariant newValue = aEvent.GetPropertyValue(); + + if( VALIDATOR_RESULT validationFailure = property->Validate( newValue.GetAny(), item ) ) + { + wxString errorMsg = wxString::Format( wxS( "%s: %s" ), wxGetTranslation( property->Name() ), + validationFailure->get()->Format( m_frame ) ); + m_frame->ShowInfoBarError( errorMsg ); + aEvent.Veto(); + return; + } +} + + +void PCB_PROPERTIES_PANEL::valueChanged( wxPropertyGridEvent& aEvent ) +{ + PCB_SELECTION_TOOL* selectionTool = m_frame->GetToolManager()->GetTool<PCB_SELECTION_TOOL>(); + const SELECTION& selection = selectionTool->GetSelection(); + + PROPERTY_BASE* property = getPropertyFromEvent( aEvent ); + wxCHECK( property, /* void */ ); wxVariant newValue = aEvent.GetPropertyValue(); BOARD_COMMIT changes( m_frame ); diff --git a/pcbnew/widgets/pcb_properties_panel.h b/pcbnew/widgets/pcb_properties_panel.h index dd8e66a8ec..a789c7f7ae 100644 --- a/pcbnew/widgets/pcb_properties_panel.h +++ b/pcbnew/widgets/pcb_properties_panel.h @@ -44,6 +44,9 @@ public: protected: wxPGProperty* createPGProperty( const PROPERTY_BASE* aProperty ) const override; + PROPERTY_BASE* getPropertyFromEvent( const wxPropertyGridEvent& aEvent ) const; + + void valueChanging( wxPropertyGridEvent& aEvent ) override; void valueChanged( wxPropertyGridEvent& aEvent ) override; ///> Regenerates caches storing layer and net names diff --git a/pcbnew/zone.cpp b/pcbnew/zone.cpp index db8e531909..686daaa6dc 100644 --- a/pcbnew/zone.cpp +++ b/pcbnew/zone.cpp @@ -34,6 +34,7 @@ #include <zone.h> #include <string_utils.h> #include <math_for_graphics.h> +#include <properties/property_validators.h> #include <settings/color_settings.h> #include <settings/settings_manager.h> #include <trigo.h> @@ -1431,6 +1432,8 @@ static struct ZONE_DESC return false; }; + // Layer property is internal because it only holds a single layer and zones actually use + // a layer set propMgr.ReplaceProperty( TYPE_HASH( BOARD_CONNECTED_ITEM ), _HKI( "Layer" ), new PROPERTY_ENUM<ZONE, PCB_LAYER_ID>( _HKI( "Layer" ), &ZONE::SetLayer, @@ -1452,36 +1455,51 @@ static struct ZONE_DESC const wxString groupFill = _HKI( "Fill Style" ); - // Fill mode can't be exposed to the UI until validation is moved to the ZONE class. - // see https://gitlab.com/kicad/code/kicad/-/issues/13811 - propMgr.AddProperty( new PROPERTY_ENUM<ZONE, ZONE_FILL_MODE>( - _HKI( "Fill Mode" ), &ZONE::SetFillMode, &ZONE::GetFillMode ), - groupFill ) - .SetIsInternal(); + propMgr.AddProperty( new PROPERTY_ENUM<ZONE, ZONE_FILL_MODE>( _HKI( "Fill Mode" ), + &ZONE::SetFillMode, + &ZONE::GetFillMode ), + groupFill ); - propMgr.AddProperty( new PROPERTY<ZONE, EDA_ANGLE>( - _HKI( "Orientation" ), &ZONE::SetHatchOrientation, - &ZONE::GetHatchOrientation, PROPERTY_DISPLAY::PT_DEGREE ), + propMgr.AddProperty( new PROPERTY<ZONE, EDA_ANGLE>( _HKI( "Orientation" ), + &ZONE::SetHatchOrientation, + &ZONE::GetHatchOrientation, + PROPERTY_DISPLAY::PT_DEGREE ), groupFill ) - .SetWriteableFunc( isHatchedFill ) - .SetIsInternal(); + .SetWriteableFunc( isHatchedFill ); - //TODO: Switch to translated + // TODO: Switch to translated + auto atLeastMinWidthValidator = + []( const wxAny&& aValue, EDA_ITEM* aZone ) -> VALIDATOR_RESULT + { + int val = aValue.As<int>(); + ZONE* zone = dynamic_cast<ZONE*>( aZone ); + wxCHECK( zone, std::nullopt ); + + if( val < zone->GetMinThickness() ) + { + return std::make_unique<VALIDATION_ERROR_MSG>( + wxT( "Cannot be less than zone minimum width" ) ); + } + + return std::nullopt; + }; + + // TODO: Switch to translated propMgr.AddProperty( new PROPERTY<ZONE, int>( wxT( "Hatch Width" ), &ZONE::SetHatchThickness, &ZONE::GetHatchThickness, PROPERTY_DISPLAY::PT_SIZE ), groupFill ) .SetWriteableFunc( isHatchedFill ) - .SetIsInternal(); + .SetValidator( atLeastMinWidthValidator ); - //TODO: Switch to translated + // TODO: Switch to translated propMgr.AddProperty( new PROPERTY<ZONE, int>( wxT( "Hatch Gap" ), &ZONE::SetHatchGap, &ZONE::GetHatchGap, PROPERTY_DISPLAY::PT_SIZE ), groupFill ) .SetWriteableFunc( isHatchedFill ) - .SetIsInternal(); + .SetValidator( atLeastMinWidthValidator ); // TODO: Smoothing effort needs to change to enum (in dialog too) // TODO: Smoothing amount (double) @@ -1493,11 +1511,16 @@ static struct ZONE_DESC &ZONE::SetLocalClearance, &ZONE::GetLocalClearance, PROPERTY_DISPLAY::PT_SIZE ); clearanceOverride->SetAvailableFunc( isCopperZone ); + constexpr int maxClearance = pcbIUScale.mmToIU( ZONE_CLEARANCE_MAX_VALUE_MM ); + clearanceOverride->SetValidator( PROPERTY_VALIDATORS::RangeIntValidator<0, maxClearance> ); auto minWidth = new PROPERTY<ZONE, int>( _HKI( "Minimum Width" ), &ZONE::SetMinThickness, &ZONE::GetMinThickness, PROPERTY_DISPLAY::PT_SIZE ); minWidth->SetAvailableFunc( isCopperZone ); + constexpr int minMinWidth = pcbIUScale.mmToIU( ZONE_THICKNESS_MIN_VALUE_MM ); + clearanceOverride->SetValidator( PROPERTY_VALIDATORS::RangeIntValidator<minMinWidth, + INT_MAX> ); auto padConnections = new PROPERTY_ENUM<ZONE, ZONE_CONNECTION>( _HKI( "Pad Connections" ), &ZONE::SetPadConnection, &ZONE::GetPadConnection ); @@ -1507,11 +1530,13 @@ static struct ZONE_DESC &ZONE::SetThermalReliefGap, &ZONE::GetThermalReliefGap, PROPERTY_DISPLAY::PT_SIZE ); thermalGap->SetAvailableFunc( isCopperZone ); + thermalGap->SetValidator( PROPERTY_VALIDATORS::PositiveIntValidator ); auto thermalSpokeWidth = new PROPERTY<ZONE, int>( _HKI( "Thermal Relief Spoke Width" ), &ZONE::SetThermalReliefSpokeWidth, &ZONE::GetThermalReliefSpokeWidth, PROPERTY_DISPLAY::PT_SIZE ); thermalSpokeWidth->SetAvailableFunc( isCopperZone ); + thermalSpokeWidth->SetValidator( atLeastMinWidthValidator ); propMgr.AddProperty( clearanceOverride, groupOverrides ); propMgr.AddProperty( minWidth, groupOverrides ); diff --git a/pcbnew/zone.h b/pcbnew/zone.h index 8ee4748ec9..767934662f 100644 --- a/pcbnew/zone.h +++ b/pcbnew/zone.h @@ -251,10 +251,16 @@ public: int GetMinThickness() const { return m_ZoneMinThickness; } void SetMinThickness( int aMinThickness ) { - if( m_ZoneMinThickness != aMinThickness ) + if( m_ZoneMinThickness != aMinThickness + || ( m_fillMode == ZONE_FILL_MODE::HATCH_PATTERN + && ( m_hatchThickness < aMinThickness || m_hatchGap < aMinThickness ) ) ) + { SetNeedRefill( true ); + } m_ZoneMinThickness = aMinThickness; + m_hatchThickness = std::max( m_hatchThickness, aMinThickness ); + m_hatchGap = std::max( m_hatchGap, aMinThickness ); } int GetHatchThickness() const { return m_hatchThickness; }