Merge pull request #7336 from mhugo/rulebasedlabeling_fix

Fix for rule based labeling toSld()

python/core/auto_generated/qgsrulebasedlabeling.sip.in

@@ -37,7 +37,7 @@ class QgsRuleBasedLabeling : QgsAbstractVectorLayerLabeling
       public:
         Rule( QgsPalLayerSettings *settings /Transfer/, int maximumScale = 0, int minimumScale = 0, const QString &filterExp = QString(), const QString &description = QString(), bool elseRule = false );
 %Docstring
-takes ownership of settings
+takes ownership of settings, settings may be None
 %End
         ~Rule();
 

src/core/qgsrulebasedlabeling.cpp

@@ -65,25 +65,21 @@ QgsRuleBasedLabeling::Rule::Rule( QgsPalLayerSettings *settings, int scaleMinDen
   , mIsActive( true )
 
 {
-  mRuleKey = QUuid::createUuid().toString();
   initFilter();
 }
 
 QgsRuleBasedLabeling::Rule::~Rule()
 {
-  delete mSettings;
-  delete mFilter;
   qDeleteAll( mChildren );
   // do NOT delete parent
 }
 
 void QgsRuleBasedLabeling::Rule::setSettings( QgsPalLayerSettings *settings )
 {
-  if ( mSettings == settings )
+  if ( mSettings.get() == settings )
     return;
 
-  delete mSettings;
-  mSettings = settings;
+  mSettings.reset( settings );
 }
 
 QgsRuleBasedLabeling::RuleList QgsRuleBasedLabeling::Rule::descendants() const
@@ -102,16 +98,15 @@ void QgsRuleBasedLabeling::Rule::initFilter()
   if ( mElseRule || mFilterExp.compare( QLatin1String( "ELSE" ), Qt::CaseInsensitive ) == 0 )
   {
     mElseRule = true;
-    mFilter = nullptr;
+    mFilter.reset( nullptr );
   }
   else if ( !mFilterExp.isEmpty() )
   {
-    delete mFilter;
-    mFilter = new QgsExpression( mFilterExp );
+    mFilter.reset( new QgsExpression( mFilterExp ) );
   }
   else
   {
-    mFilter = nullptr;
+    mFilter.reset( nullptr );
   }
 }
 
@@ -204,7 +199,7 @@ QgsRuleBasedLabeling::Rule *QgsRuleBasedLabeling::Rule::findRuleByKey( const QSt
 
 QgsRuleBasedLabeling::Rule *QgsRuleBasedLabeling::Rule::clone() const
 {
-  QgsPalLayerSettings *s = mSettings ? new QgsPalLayerSettings( *mSettings ) : nullptr;
+  QgsPalLayerSettings *s = mSettings.get() ? new QgsPalLayerSettings( *mSettings ) : nullptr;
   Rule *newrule = new Rule( s, mMaximumScale, mMinimumScale, mFilterExp, mDescription );
   newrule->setActive( mIsActive );
   // clone children
@@ -286,7 +281,7 @@ void QgsRuleBasedLabeling::Rule::createSubProviders( QgsVectorLayer *layer, QgsR
   if ( mSettings )
   {
     // add provider!
-    QgsVectorLayerLabelProvider *p = provider->createProvider( layer, mRuleKey, false, mSettings );
+    QgsVectorLayerLabelProvider *p = provider->createProvider( layer, mRuleKey, false, mSettings.get() );
     delete subProviders.value( this, nullptr );
     subProviders[this] = p;
   }
@@ -499,7 +494,7 @@ void QgsRuleBasedLabeling::toSld( QDomNode &parent, const QgsStringMap &props )
   {
     QgsPalLayerSettings *settings = rule->settings();
 
-    if ( settings->drawLabels )
+    if ( settings && settings->drawLabels )
     {
       QDomDocument doc = parent.ownerDocument();
 

src/core/qgsrulebasedlabeling.h

@@ -52,7 +52,7 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
     class CORE_EXPORT Rule
     {
       public:
-        //! takes ownership of settings
+        //! takes ownership of settings, settings may be nullptr
         Rule( QgsPalLayerSettings *settings SIP_TRANSFER, int maximumScale = 0, int minimumScale = 0, const QString &filterExp = QString(), const QString &description = QString(), bool elseRule = false );
         ~Rule();
 
@@ -72,7 +72,7 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
         /**
          * Gets the labeling settings. May return a null pointer.
          */
-        QgsPalLayerSettings *settings() const { return mSettings; }
+        QgsPalLayerSettings *settings() const { return mSettings.get(); }
 
         /**
          * Determines if scale based labeling is active
@@ -325,7 +325,7 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
 
       private:
         Rule *mParent; // parent rule (NULL only for root rule)
-        QgsPalLayerSettings *mSettings = nullptr;
+        std::unique_ptr<QgsPalLayerSettings> mSettings;
         double mMaximumScale = 0;
         double mMinimumScale = 0;
         QString mFilterExp, mDescription;
@@ -336,8 +336,7 @@ class CORE_EXPORT QgsRuleBasedLabeling : public QgsAbstractVectorLayerLabeling
 
         QString mRuleKey; // string used for unique identification of rule within labeling
 
-        // temporary
-        QgsExpression *mFilter = nullptr;
+        std::unique_ptr<QgsExpression> mFilter;
 
     };
 

tests/src/python/test_qgssymbollayer_createsld.py

@@ -32,7 +32,7 @@
     QgsFontMarkerSymbolLayer, QgsEllipseSymbolLayer, QgsSimpleLineSymbolLayer,
     QgsMarkerLineSymbolLayer, QgsMarkerSymbol, QgsSimpleFillSymbolLayer, QgsSVGFillSymbolLayer,
     QgsLinePatternFillSymbolLayer, QgsPointPatternFillSymbolLayer, QgsVectorLayer, QgsVectorLayerSimpleLabeling,
-    QgsTextBufferSettings, QgsPalLayerSettings, QgsTextBackgroundSettings)
+    QgsTextBufferSettings, QgsPalLayerSettings, QgsTextBackgroundSettings, QgsRuleBasedLabeling)
 from qgis.testing import start_app, unittest
 from utilities import unitTestDataPath
 
@@ -1053,6 +1053,13 @@ def testRuleBasedLabels(self):
         ltValue = gt.childNodes().item(1)
         self.assertEqual("1000000", gtValue.toElement().text())
 
+        # check that adding a rule without settings does not segfault
+        xml1 = dom.toString()
+        layer.labeling().rootRule().appendChild(QgsRuleBasedLabeling.Rule(None))
+        dom, root = self.layerToSld(layer)
+        xml2 = dom.toString()
+        self.assertEqual(xml1, xml2)
+
     def updateLinePlacementProperties(self, layer, linePlacement, distance, repeat, maxAngleInternal=25, maxAngleExternal=-25):
         settings = layer.labeling().settings()
         settings.placement = linePlacement