Merge pull request #30151 from elpaso/bugfix-gh30115-value-relation-w…

…idget-match-name-if-id-fails When loading a QLR, try to match the layer name if id does not
qgis · Jun 11, 2019 · e740688 · e740688
2 parents 502e18f + 2d45ba8
commit e740688
Show file tree

Hide file tree

Showing 13 changed files with 230 additions and 38 deletions.
diff --git a/python/core/auto_generated/fieldformatter/qgsvaluerelationfieldformatter.sip.in b/python/core/auto_generated/fieldformatter/qgsvaluerelationfieldformatter.sip.in
@@ -121,6 +121,15 @@ Check whether the ``feature`` has all values required by the ``expression``
 .. versionadded:: 3.2
 %End
 
+    static QgsVectorLayer *resolveLayer( const QVariantMap &config, const QgsProject *project );
+%Docstring
+Returns the (possibly NULL) layer from the widget's ``config`` and ``project``
+
+.. versionadded:: 3.8
+%End
+
+
+
 };
 
 

diff --git a/python/core/auto_generated/qgsmaplayer.sip.in b/python/core/auto_generated/qgsmaplayer.sip.in
@@ -1260,6 +1260,14 @@ The storage format for the XML is qlr
 .. versionadded:: 3.6
 %End
 
+    static QString generateId( const QString &layerName );
+%Docstring
+Generates an unique identifier for this layer, the generate ID is prefixed by ``layerName``
+
+.. versionadded:: 3.8
+%End
+
+
   public slots:
 
     void setMinimumScale( double scale );

diff --git a/src/core/fieldformatter/qgsvaluerelationfieldformatter.cpp b/src/core/fieldformatter/qgsvaluerelationfieldformatter.cpp
@@ -21,7 +21,7 @@
 #include "qgsexpressionnodeimpl.h"
 #include "qgsapplication.h"
 #include "qgsexpressioncontextutils.h"
-
+#include "qgsvectorlayerref.h"
 
 #include <nlohmann/json.hpp>
 using json = nlohmann::json;
@@ -118,7 +118,7 @@ QgsValueRelationFieldFormatter::ValueRelationCache QgsValueRelationFieldFormatte
 {
   ValueRelationCache cache;
 
-  QgsVectorLayer *layer = QgsProject::instance()->mapLayer<QgsVectorLayer *>( config.value( QStringLiteral( "Layer" ) ).toString() );
+  const QgsVectorLayer *layer = resolveLayer( config, QgsProject::instance() );
 
   if ( !layer )
     return cache;
@@ -273,3 +273,13 @@ bool QgsValueRelationFieldFormatter::expressionIsUsable( const QString &expressi
     return false;
   return true;
 }
+
+QgsVectorLayer *QgsValueRelationFieldFormatter::resolveLayer( const QVariantMap &config, const QgsProject *project )
+{
+  QgsVectorLayerRef ref { config.value( QStringLiteral( "Layer" ) ).toString(),
+                          config.value( QStringLiteral( "LayerName" ) ).toString(),
+                          config.value( QStringLiteral( "LayerSource" ) ).toString(),
+                          config.value( QStringLiteral( "LayerProviderName" ) ).toString() };
+  return ref.resolveByIdOrNameOnly( project );
+}
+
diff --git a/src/core/fieldformatter/qgsvaluerelationfieldformatter.h b/src/core/fieldformatter/qgsvaluerelationfieldformatter.h
@@ -119,6 +119,14 @@ class CORE_EXPORT QgsValueRelationFieldFormatter : public QgsFieldFormatter
      */
     static bool expressionIsUsable( const QString &expression, const QgsFeature &feature );
 
+    /**
+     * Returns the (possibly NULL) layer from the widget's \a config and \a project
+     * \since QGIS 3.8
+     */
+    static QgsVectorLayer *resolveLayer( const QVariantMap &config, const QgsProject *project );
+
+
+
 };
 
 Q_DECLARE_METATYPE( QgsValueRelationFieldFormatter::ValueRelationCache )

diff --git a/src/core/layout/qgslayoutitemattributetable.h b/src/core/layout/qgslayoutitemattributetable.h
@@ -303,7 +303,7 @@ class CORE_EXPORT QgsLayoutItemAttributeTable: public QgsLayoutTable
     //! Attribute source
     ContentSource mSource = LayerAttributes;
     //! Associated vector layer
-    QgsVectorLayerRef mVectorLayer;
+    QgsVectorLayerRef mVectorLayer = nullptr;
 
     //! Data defined vector layer - only
     QPointer< QgsVectorLayer > mDataDefinedVectorLayer;

diff --git a/src/core/qgslayerdefinition.cpp b/src/core/qgslayerdefinition.cpp
@@ -87,26 +87,25 @@ bool QgsLayerDefinition::loadLayerDefinition( QDomDocument doc, QgsProject *proj
 
   // IDs of layers should be changed otherwise we may have more then one layer with the same id
   // We have to replace the IDs before we load them because it's too late once they are loaded
-  QDomNodeList ids = doc.elementsByTagName( QStringLiteral( "id" ) );
-  for ( int i = 0; i < ids.size(); ++i )
+  QDomNodeList treeLayerNodes = doc.elementsByTagName( QStringLiteral( "layer-tree-layer" ) );
+  for ( int i = 0; i < treeLayerNodes.size(); ++i )
   {
-    QDomNode idnode = ids.at( i );
-    QDomElement idElem = idnode.toElement();
-    QString oldid = idElem.text();
-    // Strip the date part because we will replace it.
-    QString layername = oldid.left( oldid.length() - 17 );
-    QDateTime dt = QDateTime::currentDateTime();
-    QString newid = layername + dt.toString( QStringLiteral( "yyyyMMddhhmmsszzz" ) ) + QString::number( qrand() );
-    idElem.firstChild().setNodeValue( newid );
-    QDomNodeList treeLayerNodes = doc.elementsByTagName( QStringLiteral( "layer-tree-layer" ) );
-
-    for ( int i = 0; i < treeLayerNodes.count(); ++i )
+    QDomNode treeLayerNode = treeLayerNodes.at( i );
+    QDomElement treeLayerElem = treeLayerNode.toElement();
+    QString oldid = treeLayerElem.attribute( QStringLiteral( "id" ) );
+    QString layername = treeLayerElem.attribute( QStringLiteral( "name" ) );
+    QString newid = QgsMapLayer::generateId( layername );
+    treeLayerElem.setAttribute( QStringLiteral( "id" ), newid );
+
+    // Replace IDs for map layers
+    QDomNodeList ids = doc.elementsByTagName( QStringLiteral( "id" ) );
+    for ( int i = 0; i < ids.size(); ++i )
     {
-      QDomNode layerNode = treeLayerNodes.at( i );
-      QDomElement layerElem = layerNode.toElement();
-      if ( layerElem.attribute( QStringLiteral( "id" ) ) == oldid )
+      QDomNode idnode = ids.at( i );
+      QDomElement idElem = idnode.toElement();
+      if ( idElem.text() == oldid )
       {
-        layerNode.toElement().setAttribute( QStringLiteral( "id" ), newid );
+        idElem.firstChild().setNodeValue( newid );
       }
     }
 
@@ -137,6 +136,28 @@ bool QgsLayerDefinition::loadLayerDefinition( QDomDocument doc, QgsProject *proj
       }
     }
 
+    // Change IDs of widget config values
+    QDomNodeList widgetConfig = doc.elementsByTagName( QStringLiteral( "editWidget" ) );
+    for ( int i = 0; i < widgetConfig.size(); i++ )
+    {
+      QDomNodeList config = widgetConfig.at( i ).childNodes();
+      for ( int j = 0; j < config.size(); j++ )
+      {
+        QDomNodeList optMap = config.at( j ).childNodes();
+        for ( int z = 0; z < optMap.size(); z++ )
+        {
+          QDomNodeList opts = optMap.at( z ).childNodes();
+          for ( int k = 0; k < opts.size(); k++ )
+          {
+            QDomElement opt = opts.at( k ).toElement();
+            if ( opt.attribute( QStringLiteral( "value" ) ) == oldid )
+            {
+              opt.setAttribute( QStringLiteral( "value" ), newid );
+            }
+          }
+        }
+      }
+    }
   }
 
   QDomElement layerTreeElem = doc.documentElement().firstChildElement( QStringLiteral( "layer-tree-group" ) );

diff --git a/src/core/qgsmaplayer.cpp b/src/core/qgsmaplayer.cpp
@@ -78,24 +78,9 @@ QgsMapLayer::QgsMapLayer( QgsMapLayerType type,
   , mStyleManager( new QgsMapLayerStyleManager( this ) )
   , mRefreshTimer( new QTimer( this ) )
 {
-  //mShortName.replace( QRegExp( "[\\W]" ), "_" );
-
-  // Generate the unique ID of this layer
-  QString uuid = QUuid::createUuid().toString();
-  // trim { } from uuid
-  mID = lyrname + '_' + uuid.mid( 1, uuid.length() - 2 );
-
-  // Tidy the ID up to avoid characters that may cause problems
-  // elsewhere (e.g in some parts of XML). Replaces every non-word
-  // character (word characters are the alphabet, numbers and
-  // underscore) with an underscore.
-  // Note that the first backslashe in the regular expression is
-  // there for the compiler, so the pattern is actually \W
-  mID.replace( QRegExp( "[\\W]" ), QStringLiteral( "_" ) );
-
+  mID = generateId( lyrname );
   connect( this, &QgsMapLayer::crsChanged, this, &QgsMapLayer::configChanged );
   connect( this, &QgsMapLayer::nameChanged, this, &QgsMapLayer::configChanged );
-
   connect( mRefreshTimer, &QTimer::timeout, this, [ = ] { triggerRepaint( true ); } );
 }
 
@@ -1876,6 +1861,22 @@ void QgsMapLayer::setOriginalXmlProperties( const QString &originalXmlProperties
   mOriginalXmlProperties = originalXmlProperties;
 }
 
+QString QgsMapLayer::generateId( const QString &layerName )
+{
+  // Generate the unique ID of this layer
+  QString uuid = QUuid::createUuid().toString();
+  // trim { } from uuid
+  QString id = layerName + '_' + uuid.mid( 1, uuid.length() - 2 );
+  // Tidy the ID up to avoid characters that may cause problems
+  // elsewhere (e.g in some parts of XML). Replaces every non-word
+  // character (word characters are the alphabet, numbers and
+  // underscore) with an underscore.
+  // Note that the first backslash in the regular expression is
+  // there for the compiler, so the pattern is actually \W
+  id.replace( QRegExp( "[\\W]" ), QStringLiteral( "_" ) );
+  return id;
+}
+
 void QgsMapLayer::setProviderType( const QString &providerType )
 {
   mProviderKey = providerType;

diff --git a/src/core/qgsmaplayer.h b/src/core/qgsmaplayer.h
@@ -1133,6 +1133,13 @@ class CORE_EXPORT QgsMapLayer : public QObject
      */
     void setOriginalXmlProperties( const QString &originalXmlProperties );
 
+    /**
+     * Generates an unique identifier for this layer, the generate ID is prefixed by \a layerName
+     * \since QGIS 3.8
+     */
+    static QString generateId( const QString &layerName );
+
+
   public slots:
 
     /**

diff --git a/src/core/qgsmaplayerref.h b/src/core/qgsmaplayerref.h
@@ -162,6 +162,7 @@ struct _LayerRef
    * found, NULLPTR is returned.
    * \see resolve()
    * \see layerMatchesSource()
+   * \see resolveByIdOrNameOnly()
    */
   TYPE *resolveWeakly( const QgsProject *project )
   {
@@ -186,6 +187,52 @@ struct _LayerRef
     }
     return nullptr;
   }
+
+  /**
+   * Resolves the map layer by attempting to find a matching layer
+   * in a \a project using a weak match.
+   *
+   * First, the layer is attempted to match to project layers using the
+   * layer's ID (calling this method implicitly calls resolve()).
+   *
+   * Failing a match by layer ID, the layer will be matched by using
+   * the weak references to layer public source, layer name and data
+   * provider contained in this layer reference.
+   *
+   * Failing a match by weak reference, the layer will be matched by using
+   * the name only.
+   *
+   * If a matching layer is found, this reference will be updated to match
+   * the found layer and the layer will be returned. If no matching layer is
+   * found, NULLPTR is returned.
+   * \see resolve()
+   * \see layerMatchesSource()
+   * \see resolveWeakly()
+   * \since QGIS 3.8
+   */
+  TYPE *resolveByIdOrNameOnly( const QgsProject *project )
+  {
+    // first try by matching by layer ID, or weakly by source, name and provider
+    if ( resolveWeakly( project ) )
+      return layer;
+
+    // fallback to checking by name only
+    if ( project && !name.isEmpty() )
+    {
+      const QList<QgsMapLayer *> layers = project->mapLayersByName( name );
+      for ( QgsMapLayer *l : layers )
+      {
+        if ( TYPE *tl = qobject_cast< TYPE *>( l ) )
+        {
+          setLayer( tl );
+          return tl;
+        }
+      }
+    }
+    return nullptr;
+  }
+
+
 };
 
 typedef _LayerRef<QgsMapLayer> QgsMapLayerRef;

diff --git a/src/gui/editorwidgets/qgsvaluerelationconfigdlg.cpp b/src/gui/editorwidgets/qgsvaluerelationconfigdlg.cpp
@@ -18,6 +18,7 @@
 #include "qgsvectorlayer.h"
 #include "qgsexpressionbuilderdialog.h"
 #include "qgsexpressioncontextutils.h"
+#include "qgsvaluerelationfieldformatter.h"
 
 QgsValueRelationConfigDlg::QgsValueRelationConfigDlg( QgsVectorLayer *vl, int fieldIdx, QWidget *parent )
   : QgsEditorConfigWidget( vl, fieldIdx, parent )
@@ -55,6 +56,11 @@ QVariantMap QgsValueRelationConfigDlg::config()
   QVariantMap cfg;
 
   cfg.insert( QStringLiteral( "Layer" ), mLayerName->currentLayer() ? mLayerName->currentLayer()->id() : QString() );
+  cfg.insert( QStringLiteral( "LayerName" ), mLayerName->currentLayer() ? mLayerName->currentLayer()->name() : QString() );
+  cfg.insert( QStringLiteral( "LayerSource" ), mLayerName->currentLayer() ? mLayerName->currentLayer()->publicSource() : QString() );
+  cfg.insert( QStringLiteral( "LayerProviderName" ), ( mLayerName->currentLayer() && mLayerName->currentLayer()->dataProvider() ) ?
+              mLayerName->currentLayer()->dataProvider()->name() :
+              QString() );
   cfg.insert( QStringLiteral( "Key" ), mKeyColumn->currentField() );
   cfg.insert( QStringLiteral( "Value" ), mValueColumn->currentField() );
   cfg.insert( QStringLiteral( "AllowMulti" ), mAllowMulti->isChecked() );
@@ -69,7 +75,7 @@ QVariantMap QgsValueRelationConfigDlg::config()
 
 void QgsValueRelationConfigDlg::setConfig( const QVariantMap &config )
 {
-  QgsVectorLayer *lyr = QgsProject::instance()->mapLayer<QgsVectorLayer *>( config.value( QStringLiteral( "Layer" ) ).toString() );
+  QgsVectorLayer *lyr = QgsValueRelationFieldFormatter::resolveLayer( config, QgsProject::instance() );
   mLayerName->setLayer( lyr );
   mKeyColumn->setField( config.value( QStringLiteral( "Key" ) ).toString() );
   mValueColumn->setField( config.value( QStringLiteral( "Value" ) ).toString() );

diff --git a/src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.cpp b/src/gui/editorwidgets/qgsvaluerelationwidgetwrapper.cpp
@@ -299,9 +299,10 @@ int QgsValueRelationWidgetWrapper::columnCount() const
   return std::max( 1, config( QStringLiteral( "NofColumns" ) ).toInt() );
 }
 
+
 QVariant::Type QgsValueRelationWidgetWrapper::fkType() const
 {
-  QgsVectorLayer *layer = QgsProject::instance()->mapLayer<QgsVectorLayer *>( config().value( QStringLiteral( "Layer" ) ).toString() );
+  const QgsVectorLayer *layer = QgsValueRelationFieldFormatter::resolveLayer( config(), QgsProject::instance() );
   if ( layer )
   {
     QgsFields fields = layer->fields();

diff --git a/tests/src/core/testqgsmaplayer.cpp b/tests/src/core/testqgsmaplayer.cpp
@@ -72,6 +72,7 @@ class TestQgsMapLayer : public QObject
 
     void layerRef();
     void layerRefListUtils();
+    void layerRefResolveByIdOrNameOnly();
 
     void styleCategories();
 
@@ -287,6 +288,25 @@ void TestQgsMapLayer::layerRefListUtils()
   QCOMPARE( refs.at( 1 ).get(), vlC );
 }
 
+void TestQgsMapLayer::layerRefResolveByIdOrNameOnly()
+{
+  QgsVectorLayer *vlA = new QgsVectorLayer( QStringLiteral( "Point" ), QStringLiteral( "name" ), QStringLiteral( "memory" ) );
+  QgsVectorLayerRef ref;
+  QgsProject::instance()->addMapLayer( vlA );
+  ref.name = vlA->name();
+  QCOMPARE( ref.resolveByIdOrNameOnly( QgsProject::instance() ), vlA );
+  ref.layerId = vlA->id();
+  // Same name, different id
+  QgsVectorLayer *vlB = new QgsVectorLayer( QStringLiteral( "Point" ), QStringLiteral( "name" ), QStringLiteral( "memory" ) );
+  QgsProject::instance()->addMapLayer( vlB );
+  QCOMPARE( ref.resolveByIdOrNameOnly( QgsProject::instance() ), vlA );
+  // Remove layer A and check if B is returned (because they have the same name)
+  QgsProject::instance()->removeMapLayer( vlA );
+  QCOMPARE( ref.resolveByIdOrNameOnly( QgsProject::instance() ), vlB );
+  // Cleanup
+  QgsProject::instance()->removeAllMapLayers();
+}
+
 void TestQgsMapLayer::styleCategories()
 {
   // control that AllStyleCategories is actually complete