fix(RuleBasedRendering): keep rule key while cloning when needed

rule key is used by other object as reference (masking for instance) Fixes #46402
qgis · Feb 7, 2025 · c5dee65 · c5dee65
1 parent b7bd622
commit c5dee65
Show file tree

Hide file tree

Showing 8 changed files with 135 additions and 12 deletions.
diff --git a/python/PyQt6/core/auto_generated/labeling/qgsrulebasedlabeling.sip.in b/python/PyQt6/core/auto_generated/labeling/qgsrulebasedlabeling.sip.in
@@ -219,9 +219,13 @@ Try to find a rule given its unique key
 %End
 
 
-        QgsRuleBasedLabeling::Rule *clone() const /Factory/;
+        QgsRuleBasedLabeling::Rule *clone( bool resetRuleKey = true ) const /Factory/;
 %Docstring
-clone this rule, return new instance
+clone this rule
+
+:param resetRuleKey: ``True`` if this rule and its children rule key need to be reset to new unique ones.
+
+:return: new instance
 %End
 
 

diff --git a/python/core/auto_generated/labeling/qgsrulebasedlabeling.sip.in b/python/core/auto_generated/labeling/qgsrulebasedlabeling.sip.in
@@ -219,9 +219,13 @@ Try to find a rule given its unique key
 %End
 
 
-        QgsRuleBasedLabeling::Rule *clone() const /Factory/;
+        QgsRuleBasedLabeling::Rule *clone( bool resetRuleKey = true ) const /Factory/;
 %Docstring
-clone this rule, return new instance
+clone this rule
+
+:param resetRuleKey: ``True`` if this rule and its children rule key need to be reset to new unique ones.
+
+:return: new instance
 %End
 
 

diff --git a/src/core/labeling/qgsrulebasedlabeling.cpp b/src/core/labeling/qgsrulebasedlabeling.cpp
@@ -233,14 +233,18 @@ QgsRuleBasedLabeling::Rule *QgsRuleBasedLabeling::Rule::findRuleByKey( const QSt
   return nullptr;
 }
 
-QgsRuleBasedLabeling::Rule *QgsRuleBasedLabeling::Rule::clone() const
+QgsRuleBasedLabeling::Rule *QgsRuleBasedLabeling::Rule::clone( bool resetRuleKey ) const
 {
   QgsPalLayerSettings *s = mSettings.get() ? new QgsPalLayerSettings( *mSettings ) : nullptr;
   Rule *newrule = new Rule( s, mMaximumScale, mMinimumScale, mFilterExp, mDescription );
   newrule->setActive( mIsActive );
+  if ( !resetRuleKey )
+    newrule->setRuleKey( ruleKey() );
+
   // clone children
   for ( Rule *rule : mChildren )
-    newrule->appendChild( rule->clone() );
+    newrule->appendChild( rule->clone( resetRuleKey ) );
+
   return newrule;
 }
 
@@ -639,4 +643,3 @@ void QgsRuleBasedLabeling::multiplyOpacity( double opacityFactor )
 
   }
 }
-
diff --git a/src/core/labeling/qgsrulebasedlabeling.h b/src/core/labeling/qgsrulebasedlabeling.h
@@ -241,8 +241,12 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
          */
         QgsRuleBasedLabeling::Rule *findRuleByKey( const QString &key ) SIP_SKIP;
 
-        //! clone this rule, return new instance
-        QgsRuleBasedLabeling::Rule *clone() const SIP_FACTORY;
+        /**
+         * clone this rule
+         * \param resetRuleKey TRUE if this rule and its children rule key need to be reset to new unique ones.
+         * \returns new instance
+         */
+        QgsRuleBasedLabeling::Rule *clone( bool resetRuleKey = true ) const SIP_FACTORY;
 
         // load / save
 

diff --git a/src/gui/labeling/qgslabelingwidget.cpp b/src/gui/labeling/qgslabelingwidget.cpp
@@ -133,8 +133,7 @@ void QgsLabelingWidget::writeSettingsToLayer()
     case ModeRuleBased:
     {
       const QgsRuleBasedLabeling::Rule *rootRule = qobject_cast<QgsRuleBasedLabelingWidget *>( mWidget )->rootRule();
-
-      mLayer->setLabeling( new QgsRuleBasedLabeling( rootRule->clone() ) );
+      mLayer->setLabeling( new QgsRuleBasedLabeling( rootRule->clone( false ) ) );
       mLayer->setLabelsEnabled( true );
       break;
     }

diff --git a/src/gui/labeling/qgsrulebasedlabelingwidget.cpp b/src/gui/labeling/qgsrulebasedlabelingwidget.cpp
@@ -88,7 +88,7 @@ QgsRuleBasedLabelingWidget::QgsRuleBasedLabelingWidget( QgsVectorLayer *layer, Q
   if ( mLayer->labeling() && mLayer->labeling()->type() == QLatin1String( "rule-based" ) )
   {
     const QgsRuleBasedLabeling *rl = static_cast<const QgsRuleBasedLabeling *>( mLayer->labeling() );
-    mRootRule = rl->rootRule()->clone();
+    mRootRule = rl->rootRule()->clone( false );
   }
   else if ( mLayer->labeling() && mLayer->labeling()->type() == QLatin1String( "simple" ) )
   {

diff --git a/tests/src/gui/CMakeLists.txt b/tests/src/gui/CMakeLists.txt
@@ -79,6 +79,7 @@ set(TESTS
   testqgscompoundcolorwidget.cpp
   testqgsmaskingwidget.cpp
   testqgssvgselectorwidget.cpp
+  testqgslabelingwidget.cpp
 )
 
 foreach(TESTSRC ${TESTS})

diff --git a/tests/src/gui/testqgslabelingwidget.cpp b/tests/src/gui/testqgslabelingwidget.cpp
@@ -0,0 +1,108 @@
+/***************************************************************************
+    testqgslabelingwidget.cpp
+    ---------------------
+    begin                : 2025/02/06
+    copyright            : (C) 2025 by Julien Cabieces
+    email                : julien dot cabieces at oslandia dot com
+ ***************************************************************************
+ *                                                                         *
+ *   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 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+
+#include "qgstest.h"
+#include "qgslabelingwidget.h"
+#include "qgsvectorlayer.h"
+#include "qgssymbollayerreference.h"
+#include "qgsrulebasedlabeling.h"
+
+class TestQgsLabelingWidget : public QgsTest
+{
+    Q_OBJECT
+
+  public:
+    TestQgsLabelingWidget()
+      : QgsTest( QStringLiteral( "Labeling Widget Tests" ) ) {}
+
+  private slots:
+    void initTestCase();    // will be called before the first testfunction is executed.
+    void cleanupTestCase(); // will be called after the last testfunction was executed.
+    void init();            // will be called before each testfunction is executed.
+    void cleanup();         // will be called after every testfunction.
+
+    void testRuleKeyPreserved();
+};
+
+void TestQgsLabelingWidget::initTestCase()
+{
+}
+
+void TestQgsLabelingWidget::cleanupTestCase()
+{
+}
+
+void TestQgsLabelingWidget::init()
+{
+}
+
+void TestQgsLabelingWidget::cleanup()
+{
+}
+
+void TestQgsLabelingWidget::testRuleKeyPreserved()
+{
+  // test that rule keys are preserved and not resetted when editing labels with a rule based rendering
+  QgsVectorLayer layer( QStringLiteral( "Point?field=pk:int" ), QStringLiteral( "layer" ), QStringLiteral( "memory" ) );
+
+  QgsFeature ft1( layer.fields() );
+  ft1.setAttribute( QStringLiteral( "pk" ), 1 );
+  ft1.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "POINT( 1 1 )" ) ) );
+  layer.dataProvider()->addFeature( ft1 );
+
+  QgsFeature ft2( layer.fields() );
+  ft2.setAttribute( QStringLiteral( "pk" ), 2 );
+  ft2.setGeometry( QgsGeometry::fromWkt( QStringLiteral( "POINT( 2 2 )" ) ) );
+  layer.dataProvider()->addFeature( ft2 );
+
+  auto label_settings = std::make_unique<QgsPalLayerSettings>();
+  label_settings->fieldName = QStringLiteral( "pk" );
+
+  QgsTextMaskSettings mask;
+  mask.setEnabled( true );
+  mask.setMaskedSymbolLayers( QList<QgsSymbolLayerReference>() << QgsSymbolLayerReference( layer.id(), QStringLiteral( "test_unique_id" ) ) );
+
+  QgsTextFormat text_format = label_settings->format();
+  text_format.setMask( mask );
+  label_settings->setFormat( text_format );
+
+  auto root = std::make_unique<QgsRuleBasedLabeling::Rule>( new QgsPalLayerSettings() );
+
+  auto rule = std::make_unique<QgsRuleBasedLabeling::Rule>( label_settings.release() );
+  rule->setDescription( "test rule" );
+  rule->setFilterExpression( QStringLiteral( "\"{pk}\" % 2 = 0" ) );
+  rule->setActive( true );
+
+  const QString rootRuleKey = root->ruleKey();
+  const QString ruleKey = rule->ruleKey();
+
+  root->appendChild( rule.release() );
+
+  auto ruleSettings = std::make_unique<QgsRuleBasedLabeling>( root.release() );
+  layer.setLabelsEnabled( true );
+  layer.setLabeling( ruleSettings.release() );
+
+  QgsLabelingWidget widget( &layer, nullptr );
+  widget.writeSettingsToLayer();
+
+  QgsRuleBasedLabeling *labelingAfter = dynamic_cast<QgsRuleBasedLabeling *>( layer.labeling() );
+  QVERIFY( labelingAfter );
+  QCOMPARE( labelingAfter->rootRule()->ruleKey(), rootRuleKey );
+  QCOMPARE( labelingAfter->rootRule()->children().count(), 1 );
+  QCOMPARE( labelingAfter->rootRule()->children().at( 0 )->ruleKey(), ruleKey );
+}
+
+QGSTEST_MAIN( TestQgsLabelingWidget )
+#include "testqgslabelingwidget.moc"