Separate selection change updates from properties updates

Fixes https://gitlab.com/kicad/code/kicad/-/issues/13624

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 = true;
-
+        wxVariant commonVal;
-        bool writeable = property->Writeable( aSelection.Front() );
 
         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;
+            wxVariant value = commonVal;
-            const wxAny& any = item->Get( property );
-            bool converted = false;
 
-            if( property->HasChoices() )
+            if( getItemValue( item, property, value ) )
             {
-                // handle enums as ints, since there are no default conversion functions for wxAny
+                // Null value indicates different property values between items
-                int tmp;
+                if( !commonVal.IsNull() && value != commonVal )
-                converted = any.GetAs<int>( &tmp );
+                    commonVal.MakeNull();
-
+                else
-                if( converted )
+                    commonVal = value;
-                    value = wxVariant( tmp );
             }
-
+            else
-            if( !converted )                // all other types
-                converted = any.GetAs( &value );
-
-            if( !converted )
             {
-                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<int>( &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() )

common/widgets/properties_panel.h

@@ -31,6 +31,7 @@
 #include <memory>
 
 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<PROPERTY_BASE*> 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<SELECTION> m_cachedSelection;
 };
 

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<PCB_EDIT_FRAME*>( 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<PCB_SELECTION_TOOL>();
     const SELECTION& selection = selectionTool->GetSelection();
-    BOARD_ITEM* firstItem = static_cast<BOARD_ITEM*>( selection.Front() );
+
+    updatePropertyValues( selection );
+
+    CallAfter( [&]()
+               {
+                   static_cast<PCB_EDIT_FRAME*>( 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<BOARD_ITEM*>( 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<PCB_EDIT_FRAME*>( m_frame )->GetCanvas()->SetFocus();
-               } );
 }
 
 

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;