From 351f6686451cdf60ebfdb1c2db23a7c982178613 Mon Sep 17 00:00:00 2001 From: Jon Evans Date: Sun, 22 Jan 2023 11:32:38 -0500 Subject: [PATCH] Separate selection change updates from properties updates Fixes https://gitlab.com/kicad/code/kicad/-/issues/13624 --- common/widgets/properties_panel.cpp | 69 +++++++++++++++---------- common/widgets/properties_panel.h | 31 ++++++++++- pcbnew/widgets/pcb_properties_panel.cpp | 41 ++++++++++++--- pcbnew/widgets/pcb_properties_panel.h | 2 + 4 files changed, 107 insertions(+), 36 deletions(-) diff --git a/common/widgets/properties_panel.cpp b/common/widgets/properties_panel.cpp index e21d834bf5..7d20b97431 100644 --- a/common/widgets/properties_panel.cpp +++ b/common/widgets/properties_panel.cpp @@ -129,10 +129,13 @@ void PROPERTIES_PANEL::OnLanguageChanged() } -void PROPERTIES_PANEL::update( const SELECTION& aSelection ) +void PROPERTIES_PANEL::rebuildProperties( const SELECTION& aSelection ) { if( *m_cachedSelection == aSelection ) + { + updatePropertyValues( aSelection ); return; + } *m_cachedSelection = aSelection; @@ -223,47 +226,34 @@ void PROPERTIES_PANEL::update( const SELECTION& aSelection ) // Either determine the common value for a property or "<...>" to indicate multiple values bool available = true; - wxVariant commonVal, itemVal; - - bool writeable = property->Writeable( aSelection.Front() ); + bool writeable = true; + wxVariant commonVal; for( EDA_ITEM* item : aSelection ) { if( !propMgr.IsAvailableFor( TYPE_HASH( *item ), property, item ) ) + { + available = false; break; // there is an item that does not have this property, so do not display it + } // If read-only for any of the selection, read-only for the whole selection. if( !property->Writeable( item ) ) writeable = false; - wxVariant& value = commonVal.IsNull() ? commonVal : itemVal; - const wxAny& any = item->Get( property ); - bool converted = false; + wxVariant value = commonVal; - if( property->HasChoices() ) + if( getItemValue( item, property, value ) ) { - // handle enums as ints, since there are no default conversion functions for wxAny - int tmp; - converted = any.GetAs( &tmp ); - - if( converted ) - value = wxVariant( tmp ); + // Null value indicates different property values between items + if( !commonVal.IsNull() && value != commonVal ) + commonVal.MakeNull(); + else + commonVal = value; } - - if( !converted ) // all other types - converted = any.GetAs( &value ); - - if( !converted ) + else { - wxFAIL_MSG( wxS( "Could not convert wxAny to wxVariant" ) ); available = false; - break; - } - - if( !commonVal.IsNull() && value != commonVal ) - { - commonVal.MakeNull(); // items have different values for this property - break; } } @@ -313,6 +303,31 @@ void PROPERTIES_PANEL::update( const SELECTION& aSelection ) } +bool PROPERTIES_PANEL::getItemValue( EDA_ITEM* aItem, PROPERTY_BASE* aProperty, wxVariant& aValue ) +{ + const wxAny& any = aItem->Get( aProperty ); + bool converted = false; + + if( aProperty->HasChoices() ) + { + // handle enums as ints, since there are no default conversion functions for wxAny + int tmp; + converted = any.GetAs( &tmp ); + + if( converted ) + aValue = wxVariant( tmp ); + } + + if( !converted ) // all other types + converted = any.GetAs( &aValue ); + + if( !converted ) + wxFAIL_MSG( wxS( "Could not convert wxAny to wxVariant" ) ); + + return converted; +} + + void PROPERTIES_PANEL::onShow( wxShowEvent& aEvent ) { if( aEvent.IsShown() ) diff --git a/common/widgets/properties_panel.h b/common/widgets/properties_panel.h index 03ba80b029..d8b6ca1ecd 100644 --- a/common/widgets/properties_panel.h +++ b/common/widgets/properties_panel.h @@ -31,6 +31,7 @@ #include class EDA_BASE_FRAME; +class EDA_ITEM; class SELECTION; class PROPERTY_BASE; class wxStaticText; @@ -71,7 +72,27 @@ public: void OnLanguageChanged(); protected: - virtual void update( const SELECTION& aSelection ); + /** + * Generates the property grid for a given selection of items. + * + * Calling this will remove any existing properties shown, causing visible flicker on some + * platforms and canceling any pending edits. Make sure to only call this when the group of + * selected items has actually changed. + * + * @param aSelection is a set of items to show properties for. + */ + virtual void rebuildProperties( const SELECTION& aSelection ); + + /** + * Updates the values shown in the property grid for the current selection. + * + * Does not add or remove properties from the display (@see rebuildProperties) + * This implies that aSelection must have the same contents as m_cachedSelection. + * + * @param aSelection is the set of selected items. + */ + virtual void updatePropertyValues( const SELECTION& aSelection ) {} + virtual wxPGProperty* createPGProperty( const PROPERTY_BASE* aProperty ) const = 0; // Event handlers @@ -80,6 +101,13 @@ protected: void onCharHook( wxKeyEvent& aEvent ); void onShow( wxShowEvent& aEvent ); + /** + * Utility to fetch a property value and convert to wxVariant + * Precondition: aItem is known to have property aProperty + * @return true if conversion succeeded + */ + bool getItemValue( EDA_ITEM* aItem, PROPERTY_BASE* aProperty, wxVariant& aValue ); + protected: std::vector m_displayed; wxPropertyGrid* m_grid; @@ -89,6 +117,7 @@ protected: /// Proportion of the grid column splitter that is used for the key column (0.0 - 1.0) float m_splitter_key_proportion; + /// A copy of the most recent selection passed to rebuildProperties std::unique_ptr m_cachedSelection; }; diff --git a/pcbnew/widgets/pcb_properties_panel.cpp b/pcbnew/widgets/pcb_properties_panel.cpp index ea53d3ac90..b66711bfad 100644 --- a/pcbnew/widgets/pcb_properties_panel.cpp +++ b/pcbnew/widgets/pcb_properties_panel.cpp @@ -77,7 +77,9 @@ void PCB_PROPERTIES_PANEL::UpdateData() // TODO perhaps it could be called less often? use PROPERTIES_TOOL and catch MODEL_RELOAD? updateLists( static_cast( m_frame )->GetBoard() ); - update( selection ); + + // Will actually just be updatePropertyValues() if selection hasn't changed + rebuildProperties( selection ); } @@ -85,7 +87,20 @@ void PCB_PROPERTIES_PANEL::AfterCommit() { PCB_SELECTION_TOOL* selectionTool = m_frame->GetToolManager()->GetTool(); const SELECTION& selection = selectionTool->GetSelection(); - BOARD_ITEM* firstItem = static_cast( selection.Front() ); + + updatePropertyValues( selection ); + + CallAfter( [&]() + { + static_cast( m_frame )->GetCanvas()->SetFocus(); + } ); +} + + +void PCB_PROPERTIES_PANEL::updatePropertyValues( const SELECTION& aSelection ) +{ + // TODO: Refactor to reduce duplication with PROPERTIES_PANEL::rebuildProperties + BOARD_ITEM* firstItem = static_cast( aSelection.Front() ); for( wxPropertyGridIterator it = m_grid->GetIterator(); !it.AtEnd(); it.Next() ) { @@ -96,17 +111,27 @@ void PCB_PROPERTIES_PANEL::AfterCommit() wxCHECK2( property, continue ); bool writeable = true; + wxVariant commonVal; - for( EDA_ITEM* edaItem : selection ) + for( EDA_ITEM* edaItem : aSelection ) + { writeable &= property->Writeable( edaItem ); + wxVariant value = commonVal; + + if( getItemValue( edaItem, property, value ) ) + { + // Null value indicates different property values between items + if( !commonVal.IsNull() && value != commonVal ) + commonVal.MakeNull(); + else + commonVal = value; + } + } + + pgProp->SetValue( commonVal ); pgProp->Enable( writeable ); } - - CallAfter( [&]() - { - static_cast( m_frame )->GetCanvas()->SetFocus(); - } ); } diff --git a/pcbnew/widgets/pcb_properties_panel.h b/pcbnew/widgets/pcb_properties_panel.h index b4a3c07f37..8cd346bb98 100644 --- a/pcbnew/widgets/pcb_properties_panel.h +++ b/pcbnew/widgets/pcb_properties_panel.h @@ -48,6 +48,8 @@ protected: ///> Regenerates caches storing layer and net names void updateLists( const BOARD* aBoard ); + void updatePropertyValues( const SELECTION& aSelection ) override; + PCB_EDIT_FRAME* m_frame; PROPERTY_MANAGER& m_propMgr; PG_UNIT_EDITOR* m_editor;