Fix incorrect legend key entries are returned for rule based

nyalldawson · nyalldawson · commit f77c4c84d8b4 · 2023-03-14T17:03:29.000+10:00
renderers with else rules

Fixes map based legend filtering with some rule based renderers
diff --git a/src/core/symbology/qgsrulebasedrenderer.cpp b/src/core/symbology/qgsrulebasedrenderer.cpp
@@ -22,16 +22,14 @@
 #include "qgslogger.h"
 #include "qgsogcutils.h"
 #include "qgssinglesymbolrenderer.h"
-#include "qgspointdisplacementrenderer.h"
 #include "qgsinvertedpolygonrenderer.h"
-#include "qgspainteffect.h"
-#include "qgspainteffectregistry.h"
 #include "qgsproperty.h"
 #include "qgsstyleentityvisitor.h"
 #include "qgsembeddedsymbolrenderer.h"
 #include "qgslinesymbol.h"
 #include "qgsfillsymbol.h"
 #include "qgsmarkersymbol.h"
+#include "qgspointdistancerenderer.h"
 
 #include <QSet>
 
@@ -679,43 +677,52 @@ QgsSymbolList QgsRuleBasedRenderer::Rule::symbolsForFeature( const QgsFeature &f
 
 QSet<QString> QgsRuleBasedRenderer::Rule::legendKeysForFeature( const QgsFeature &feature, QgsRenderContext *context )
 {
-  QSet< QString> lst;
+  QSet< QString> res;
   if ( !isFilterOK( feature, context ) )
-    return lst;
-  lst.insert( mRuleKey );
+    return res;
 
-  const auto constMActiveChildren = mActiveChildren;
-  for ( Rule *rule : constMActiveChildren )
+  res.insert( mRuleKey );
+
+  // first determine if any non else rules match at this level
+  bool matchedNonElseRule = false;
+  for ( Rule *rule : std::as_const( mActiveChildren ) )
   {
-    bool validKey = false;
     if ( rule->isElse() )
     {
-      if ( rule->children().isEmpty() )
-      {
-        RuleList lst = rulesForFeature( feature, context, false );
-        lst.removeOne( rule );
-
-        if ( lst.empty() )
-        {
-          validKey = true;
-        }
-      }
-      else
-      {
-        validKey = true;
-      }
+      continue;
     }
-    else if ( rule->willRenderFeature( feature, context ) )
+    if ( rule->willRenderFeature( feature, context ) )
     {
-      validKey = true;
+      res.unite( rule->legendKeysForFeature( feature, context ) );
+      matchedNonElseRule = true;
     }
+  }
 
-    if ( validKey )
+  // second chance -- allow else rules to take effect if valid
+  if ( !matchedNonElseRule )
+  {
+    for ( Rule *rule : std::as_const( mActiveChildren ) )
     {
-      lst.unite( rule->legendKeysForFeature( feature, context ) );
+      if ( rule->isElse() )
+      {
+        if ( rule->children().isEmpty() )
+        {
+          RuleList lst = rulesForFeature( feature, context, false );
+          lst.removeOne( rule );
+
+          if ( lst.empty() )
+          {
+            res.unite( rule->legendKeysForFeature( feature, context ) );
+          }
+        }
+        else
+        {
+          res.unite( rule->legendKeysForFeature( feature, context ) );
+        }
+      }
     }
   }
-  return lst;
+  return res;
 }
 
 QgsRuleBasedRenderer::RuleList QgsRuleBasedRenderer::Rule::rulesForFeature( const QgsFeature &feature, QgsRenderContext *context, bool onlyActive )
diff --git a/tests/src/core/testqgsrulebasedrenderer.cpp b/tests/src/core/testqgsrulebasedrenderer.cpp
@@ -1179,6 +1179,70 @@ class TestQgsRuleBasedRenderer: public QgsTest
       QCOMPARE( renderer->legendKeys(), expected );
     }
 
+    void testLegendKeysForFeature()
+    {
+      QgsRuleBasedRenderer::Rule *rootRule = new QgsRuleBasedRenderer::Rule( nullptr );
+      std::unique_ptr< QgsRuleBasedRenderer > renderer = std::make_unique< QgsRuleBasedRenderer >( rootRule );
+      std::unique_ptr< QgsMarkerSymbol > symbol( QgsMarkerSymbol::createSimple( {} ) );
+
+      QgsRuleBasedRenderer::Rule *lessThanTwoRule = new QgsRuleBasedRenderer::Rule( symbol->clone(), 0, 0, "\"Importance\" <= 2" );
+      rootRule->appendChild( lessThanTwoRule );
+
+      QgsRuleBasedRenderer::Rule *elseRule = new QgsRuleBasedRenderer::Rule( nullptr, 0, 0, QString(), QString(), QString(), true );
+      rootRule->appendChild( elseRule );
+
+      QgsRuleBasedRenderer::Rule *oneRule = new QgsRuleBasedRenderer::Rule( symbol->clone(), 0, 0, "\"Pilots\" = 1" );
+      elseRule->appendChild( oneRule );
+
+      QgsRuleBasedRenderer::Rule *twoRule = new QgsRuleBasedRenderer::Rule( symbol->clone(), 0, 0, "\"Pilots\" = 2" );
+      elseRule->appendChild( twoRule );
+
+      QgsRuleBasedRenderer::Rule *threeRule = new QgsRuleBasedRenderer::Rule( symbol->clone(), 0, 0, "\"Pilots\" = 3" );
+      elseRule->appendChild( threeRule );
+
+      QgsFields fields;
+      fields.append( QgsField( QStringLiteral( "Importance" ), QVariant::Int ) );
+      fields.append( QgsField( QStringLiteral( "Pilots" ), QVariant::Int ) );
+
+      QgsFeature feature( fields );
+      QgsExpressionContext expContext;
+      expContext.setFields( fields );
+      QgsRenderContext rc;
+      rc.setExpressionContext( expContext );
+
+      renderer->startRender( rc, fields );
+
+      QSet< QString > expected{rootRule->ruleKey(), elseRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 1 << 2 );
+      expected = {rootRule->ruleKey(), lessThanTwoRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 2 << 2 );
+      expected = {rootRule->ruleKey(), lessThanTwoRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 3 << 1 );
+      expected = {rootRule->ruleKey(), elseRule->ruleKey(), oneRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 3 << 2 );
+      expected = {rootRule->ruleKey(), elseRule->ruleKey(), twoRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 3 << 3 );
+      expected = {rootRule->ruleKey(), elseRule->ruleKey(), threeRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      feature.setAttributes( QgsAttributes() << 3 << 4 );
+      expected = {rootRule->ruleKey(), elseRule->ruleKey() };
+      QCOMPARE( renderer->legendKeysForFeature( feature, rc ), expected );
+
+      renderer->stopRender( rc );
+    }
+
+
     void testLegendKeyToExpression()
     {
       QgsRuleBasedRenderer::Rule *rootRule = new QgsRuleBasedRenderer::Rule( nullptr );
