Use weak layer pointers in labeling engine instead of layer ids

nyalldawson · nyalldawson · commit 615745fc8b87 · 2017-02-01T21:44:31.000+10:00
diff --git a/src/core/qgslabelingengine.cpp b/src/core/qgslabelingengine.cpp
@@ -25,6 +25,7 @@
 #include "pal.h"
 #include "problem.h"
 #include "qgsrendercontext.h"
+#include "qgsmaplayer.h"
 
 
 // helper function for checking for job cancelation within PAL
@@ -88,20 +89,20 @@ QgsLabelingEngine::~QgsLabelingEngine()
   qDeleteAll( mSubProviders );
 }
 
-QStringList QgsLabelingEngine::participatingLayerIds() const
+QList< QgsMapLayer* > QgsLabelingEngine::participatingLayers() const
 {
-  QSet< QString > ids;
+  QSet< QgsMapLayer* > layers;
   Q_FOREACH ( QgsAbstractLabelProvider* provider, mProviders )
   {
-    if ( !provider->layerId().isEmpty() )
-      ids << provider->layerId();
+    if ( provider->layer() )
+      layers << provider->layer();
   }
   Q_FOREACH ( QgsAbstractLabelProvider* provider, mSubProviders )
   {
-    if ( !provider->layerId().isEmpty() )
-      ids << provider->layerId();
+    if ( provider->layer() )
+      layers << provider->layer();
   }
-  return ids.toList();
+  return layers.toList();
 }
 
 void QgsLabelingEngine::addProvider( QgsAbstractLabelProvider* provider )
@@ -230,7 +231,7 @@ void QgsLabelingEngine::run( QgsRenderContext& context )
   Q_FOREACH ( QgsAbstractLabelProvider* provider, mProviders )
   {
     bool appendedLayerScope = false;
-    if ( QgsMapLayer* ml = QgsProject::instance()->mapLayer( provider->layerId() ) )
+    if ( QgsMapLayer* ml = provider->layer() )
     {
       appendedLayerScope = true;
       context.expressionContext().appendScope( QgsExpressionContextUtils::layerScope( ml ) );
@@ -415,9 +416,10 @@ QgsAbstractLabelProvider*QgsLabelFeature::provider() const
 
 }
 
-QgsAbstractLabelProvider::QgsAbstractLabelProvider( const QString& layerId, const QString& providerId )
+QgsAbstractLabelProvider::QgsAbstractLabelProvider( QgsMapLayer* layer, const QString& providerId )
     : mEngine( nullptr )
-    , mLayerId( layerId )
+    , mLayerId( layer ? layer->id() : QString() )
+    , mLayer( layer )
     , mProviderId( providerId )
     , mFlags( DrawLabels )
     , mPlacement( QgsPalLayerSettings::AroundPoint )
diff --git a/src/core/qgslabelingengine.h b/src/core/qgslabelingengine.h
@@ -43,7 +43,7 @@ class CORE_EXPORT QgsAbstractLabelProvider
 
   public:
     //! Construct the provider with default values
-    QgsAbstractLabelProvider( const QString& layerId = QString(), const QString& providerId = QString() );
+    QgsAbstractLabelProvider( QgsMapLayer* layer, const QString& providerId = QString() );
 
     virtual ~QgsAbstractLabelProvider() = default;
 
@@ -75,6 +75,9 @@ class CORE_EXPORT QgsAbstractLabelProvider
     //! Returns ID of associated layer, or empty string if no layer is associated with the provider.
     QString layerId() const { return mLayerId; }
 
+    //! Returns the associated layer, or nullptr if no layer is associated with the provider.
+    QgsMapLayer* layer() const { return mLayer.data(); }
+
     //! Returns provider ID - useful in case there is more than one label provider within a layer
     //! (e.g. in case of rule-based labeling - provider ID = rule's key). May be empty string if
     //! layer ID is sufficient for identification of provider's configuration.
@@ -106,6 +109,8 @@ class CORE_EXPORT QgsAbstractLabelProvider
     QString mName;
     //! Associated layer's ID, if applicable
     QString mLayerId;
+    //! Weak pointer to source layer
+    QgsWeakMapLayerPointer mLayer;
     //! Associated provider ID (one layer may have multiple providers, e.g. in rule-based labeling)
     QString mProviderId;
     //! Flags altering drawing and registration of features
@@ -187,10 +192,10 @@ class CORE_EXPORT QgsLabelingEngine
     const QgsMapSettings& mapSettings() const { return mMapSettings; }
 
     /**
-     * Returns a list of layer IDs for layers with providers in the engine.
+     * Returns a list of layers with providers in the engine.
      * @note added in QGIS 3.0
      */
-    QStringList participatingLayerIds() const;
+    QList< QgsMapLayer* > participatingLayers() const;
 
     //! Add provider of label features. Takes ownership of the provider
     void addProvider( QgsAbstractLabelProvider* provider );
diff --git a/src/core/qgsvectorlayerdiagramprovider.cpp b/src/core/qgsvectorlayerdiagramprovider.cpp
@@ -24,29 +24,8 @@
 #include "feature.h"
 #include "labelposition.h"
 
-
-QgsVectorLayerDiagramProvider::QgsVectorLayerDiagramProvider(
-  const QgsDiagramLayerSettings* diagSettings,
-  const QgsDiagramRenderer* diagRenderer,
-  const QString& layerId,
-  const QgsFields& fields,
-  const QgsCoordinateReferenceSystem& crs,
-  QgsAbstractFeatureSource* source,
-  bool ownsSource )
-    : QgsAbstractLabelProvider( layerId )
-    , mSettings( *diagSettings )
-    , mDiagRenderer( diagRenderer->clone() )
-    , mFields( fields )
-    , mLayerCrs( crs )
-    , mSource( source )
-    , mOwnsSource( ownsSource )
-{
-  init();
-}
-
-
 QgsVectorLayerDiagramProvider::QgsVectorLayerDiagramProvider( QgsVectorLayer* layer, bool ownFeatureLoop )
-    : QgsAbstractLabelProvider( layer->id() )
+    : QgsAbstractLabelProvider( layer )
     , mSettings( *layer->diagramLayerSettings() )
     , mDiagRenderer( layer->diagramRenderer()->clone() )
     , mFields( layer->fields() )
diff --git a/src/core/qgsvectorlayerdiagramprovider.h b/src/core/qgsvectorlayerdiagramprovider.h
@@ -62,15 +62,6 @@ class CORE_EXPORT QgsVectorLayerDiagramProvider : public QgsAbstractLabelProvide
     //! Convenience constructor to initialize the provider from given vector layer
     explicit QgsVectorLayerDiagramProvider( QgsVectorLayer* layer, bool ownFeatureLoop = true );
 
-    //! Construct diagram provider with all the necessary configuration parameters
-    QgsVectorLayerDiagramProvider( const QgsDiagramLayerSettings* diagSettings,
-                                   const QgsDiagramRenderer* diagRenderer,
-                                   const QString& layerId,
-                                   const QgsFields& fields,
-                                   const QgsCoordinateReferenceSystem& crs,
-                                   QgsAbstractFeatureSource* source,
-                                   bool ownsSource );
-
     //! Clean up
     ~QgsVectorLayerDiagramProvider();
 
diff --git a/src/core/qgsvectorlayerlabelprovider.cpp b/src/core/qgsvectorlayerlabelprovider.cpp
@@ -35,7 +35,7 @@
 using namespace pal;
 
 QgsVectorLayerLabelProvider::QgsVectorLayerLabelProvider( QgsVectorLayer* layer, const QString& providerId, bool withFeatureLoop, const QgsPalLayerSettings* settings, const QString& layerName )
-    : QgsAbstractLabelProvider( layer->id(), providerId )
+    : QgsAbstractLabelProvider( layer, providerId )
     , mSettings( settings ? *settings : QgsPalLayerSettings::fromLayer( layer ) )
     , mLayerGeometryType( layer->geometryType() )
     , mRenderer( layer->renderer() )
@@ -58,25 +58,6 @@ QgsVectorLayerLabelProvider::QgsVectorLayerLabelProvider( QgsVectorLayer* layer,
   init();
 }
 
-QgsVectorLayerLabelProvider::QgsVectorLayerLabelProvider( const QgsPalLayerSettings& settings,
-    const QString& layerId,
-    const QgsFields& fields,
-    const QgsCoordinateReferenceSystem& crs,
-    QgsAbstractFeatureSource* source,
-    bool ownsSource, QgsFeatureRenderer* renderer )
-    : QgsAbstractLabelProvider( layerId )
-    , mSettings( settings )
-    , mLayerGeometryType( QgsWkbTypes::UnknownGeometry )
-    , mRenderer( renderer )
-    , mFields( fields )
-    , mCrs( crs )
-    , mSource( source )
-    , mOwnsSource( ownsSource )
-{
-  init();
-}
-
-
 void QgsVectorLayerLabelProvider::init()
 {
   mPlacement = mSettings.placement;
diff --git a/src/core/qgsvectorlayerlabelprovider.h b/src/core/qgsvectorlayerlabelprovider.h
@@ -44,15 +44,6 @@ class CORE_EXPORT QgsVectorLayerLabelProvider : public QgsAbstractLabelProvider
                                           const QgsPalLayerSettings* settings = nullptr,
                                           const QString& layerName = QString() );
 
-    //! Construct diagram provider with all the necessary configuration parameters
-    QgsVectorLayerLabelProvider( const QgsPalLayerSettings& settings,
-                                 const QString& layerId,
-                                 const QgsFields& fields,
-                                 const QgsCoordinateReferenceSystem& crs,
-                                 QgsAbstractFeatureSource* source,
-                                 bool ownsSource,
-                                 QgsFeatureRenderer* renderer = nullptr );
-
     ~QgsVectorLayerLabelProvider();
 
     virtual QList<QgsLabelFeature*> labelFeatures( QgsRenderContext& context ) override;
diff --git a/tests/src/core/testqgslabelingengine.cpp b/tests/src/core/testqgslabelingengine.cpp
@@ -538,18 +538,18 @@ void TestQgsLabelingEngine::testCapitalization()
 void TestQgsLabelingEngine::testParticipatingLayers()
 {
   QgsLabelingEngine engine;
-  QVERIFY( engine.participatingLayerIds().isEmpty() );
+  QVERIFY( engine.participatingLayers().isEmpty() );
 
   QgsPalLayerSettings settings1;
   QgsVectorLayerLabelProvider* provider = new QgsVectorLayerLabelProvider( vl, QStringLiteral( "test" ), true, &settings1 );
   engine.addProvider( provider );
-  QCOMPARE( engine.participatingLayerIds(), QStringList() << vl->id() );
+  QCOMPARE( engine.participatingLayers(), QList<QgsMapLayer*>() << vl );
 
   QgsVectorLayer* layer2 = new QgsVectorLayer( QStringLiteral( "Point?field=col1:integer" ), QStringLiteral( "layer2" ), QStringLiteral( "memory" ) );
   QgsPalLayerSettings settings2;
   QgsVectorLayerLabelProvider* provider2 = new QgsVectorLayerLabelProvider( layer2, QStringLiteral( "test2" ), true, &settings2 );
   engine.addProvider( provider2 );
-  QCOMPARE( engine.participatingLayerIds().toSet(), QSet< QString >() << vl->id() << layer2->id() );
+  QCOMPARE( engine.participatingLayers().toSet(), QSet< QgsMapLayer* >() << vl << layer2 );
 
   // add a rule-based labeling node
   QgsRuleBasedLabeling::Rule* root = new QgsRuleBasedLabeling::Rule( 0 );
@@ -561,7 +561,7 @@ void TestQgsLabelingEngine::testParticipatingLayers()
   QgsVectorLayer* layer3 = new QgsVectorLayer( QStringLiteral( "Point?field=col1:integer" ), QStringLiteral( "layer3" ), QStringLiteral( "memory" ) );
   QgsRuleBasedLabelProvider* ruleProvider = new QgsRuleBasedLabelProvider( QgsRuleBasedLabeling( root ), layer3 );
   engine.addProvider( ruleProvider );
-  QCOMPARE( engine.participatingLayerIds().toSet(), QSet< QString >() << vl->id() << layer2->id() << layer3->id() );
+  QCOMPARE( engine.participatingLayers().toSet(), QSet< QgsMapLayer* >() << vl << layer2 << layer3 );
 }
 
 bool TestQgsLabelingEngine::imageCheck( const QString& testName, QImage &image, int mismatchCount )

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`	`#include "pal.h"`
`26`	`26`	`#include "problem.h"`
`27`	`27`	`#include "qgsrendercontext.h"`
	`28`	`+#include "qgsmaplayer.h"`
`28`	`29`
`29`	`30`
`30`	`31`	`// helper function for checking for job cancelation within PAL`
`@@ -88,20 +89,20 @@ QgsLabelingEngine::~QgsLabelingEngine()`
`88`	`89`	`qDeleteAll( mSubProviders );`
`89`	`90`	`}`
`90`	`91`
`91`		`-QStringList QgsLabelingEngine::participatingLayerIds() const`
	`92`	`+QList< QgsMapLayer* > QgsLabelingEngine::participatingLayers() const`
`92`	`93`	`{`
`93`		`- QSet< QString > ids;`
	`94`	`+ QSet< QgsMapLayer* > layers;`
`94`	`95`	`Q_FOREACH ( QgsAbstractLabelProvider* provider, mProviders )`
`95`	`96`	`{`
`96`		`- if ( !provider->layerId().isEmpty() )`
`97`		`- ids << provider->layerId();`
	`97`	`+ if ( provider->layer() )`
	`98`	`+ layers << provider->layer();`
`98`	`99`	`}`
`99`	`100`	`Q_FOREACH ( QgsAbstractLabelProvider* provider, mSubProviders )`
`100`	`101`	`{`
`101`		`- if ( !provider->layerId().isEmpty() )`
`102`		`- ids << provider->layerId();`
	`102`	`+ if ( provider->layer() )`
	`103`	`+ layers << provider->layer();`
`103`	`104`	`}`
`104`		`- return ids.toList();`
	`105`	`+ return layers.toList();`
`105`	`106`	`}`
`106`	`107`
`107`	`108`	`void QgsLabelingEngine::addProvider( QgsAbstractLabelProvider* provider )`
`@@ -230,7 +231,7 @@ void QgsLabelingEngine::run( QgsRenderContext& context )`
`230`	`231`	`Q_FOREACH ( QgsAbstractLabelProvider* provider, mProviders )`
`231`	`232`	`{`
`232`	`233`	`bool appendedLayerScope = false;`
`233`		`- if ( QgsMapLayer* ml = QgsProject::instance()->mapLayer( provider->layerId() ) )`
	`234`	`+ if ( QgsMapLayer* ml = provider->layer() )`
`234`	`235`	`{`
`235`	`236`	`appendedLayerScope = true;`
`236`	`237`	`context.expressionContext().appendScope( QgsExpressionContextUtils::layerScope( ml ) );`
`@@ -415,9 +416,10 @@ QgsAbstractLabelProvider*QgsLabelFeature::provider() const`
`415`	`416`
`416`	`417`	`}`
`417`	`418`
`418`		`-QgsAbstractLabelProvider::QgsAbstractLabelProvider( const QString& layerId, const QString& providerId )`
	`419`	`+QgsAbstractLabelProvider::QgsAbstractLabelProvider( QgsMapLayer* layer, const QString& providerId )`
`419`	`420`	`: mEngine( nullptr )`
`420`		`- , mLayerId( layerId )`
	`421`	`+ , mLayerId( layer ? layer->id() : QString() )`
	`422`	`+ , mLayer( layer )`
`421`	`423`	`, mProviderId( providerId )`
`422`	`424`	`, mFlags( DrawLabels )`
`423`	`425`	`, mPlacement( QgsPalLayerSettings::AroundPoint )`