Merge pull request #9620 from elpaso/bugfix-badlayers-storage

Bugfix badlayers storage
qgis · Mar 28, 2019 · 5b5cc50 · 5b5cc50
2 parents 20ec29a + bd9b373
commit 5b5cc50
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 15 deletions.
diff --git a/python/core/auto_generated/qgsmaplayerstore.sip.in b/python/core/auto_generated/qgsmaplayerstore.sip.in
@@ -120,6 +120,13 @@ The layersAdded() and layerWasAdded() signals will always be emitted.
 
 :param layers: A list of layer which should be added to the store.
 
+.. note::
+
+   If a layer with the same id is already in the store it is not added again,
+   but if the validity of the layer has changed from ``False`` to ``True``, the
+   layer data source is updated to the new one.
+
+
 :return: a list of the map layers that were added
          successfully. If a layer already exists in the store,
          it will not be part of the returned list.

diff --git a/src/app/qgshandlebadlayers.cpp b/src/app/qgshandlebadlayers.cpp
@@ -366,7 +366,6 @@ void QgsHandleBadLayers::apply()
   QHash<QString, QString> baseChange;
 
 
-
   for ( int i = 0; i < mLayerList->rowCount(); i++ )
   {
     int idx = mLayerList->item( i, 0 )->data( Qt::UserRole ).toInt();
@@ -405,27 +404,27 @@ void QgsHandleBadLayers::apply()
       datasource = datasource.replace( basepath, baseChange[basepath] );
 
 
-    bool dataSourceChanged { false };
+    bool dataSourceFixed { false };
     const QString layerId { node.namedItem( QStringLiteral( "id" ) ).toElement().text() };
     const QString provider { node.namedItem( QStringLiteral( "provider" ) ).toElement().text() };
 
     // Try first to change the datasource of the existing layers, this will
     // maintain the current status (checked/unchecked) and group
     if ( QgsProject::instance()->mapLayer( layerId ) )
     {
-      QgsDataProvider::ProviderOptions options;
       QgsMapLayer *mapLayer = QgsProject::instance()->mapLayer( layerId );
       if ( mapLayer )
       {
+        QgsDataProvider::ProviderOptions options;
         mapLayer->setDataSource( datasource, name, provider, options );
-        dataSourceChanged = mapLayer->isValid();
+        dataSourceFixed  = mapLayer->isValid();
       }
     }
 
     // If the data source was changed successfully, remove the bad layer from the dialog
     // otherwise, try to set the new datasource in the XML node and reload the layer,
     // finally marks with red all remaining bad layers.
-    if ( dataSourceChanged )
+    if ( dataSourceFixed )
     {
       mLayerList->removeRow( i-- );
     }
@@ -447,7 +446,8 @@ void QgsHandleBadLayers::apply()
     }
   }
 
-  // Final cleanup: remove any bad layer (it should not be any btw)
+  // Final cleanup: remove any remaining bad layer
+  // (there should not be any at this point)
   if ( mLayerList->rowCount() == 0 )
   {
     QList<QgsMapLayer *> toRemove;

diff --git a/src/core/qgsmaplayerstore.cpp b/src/core/qgsmaplayerstore.cpp
@@ -74,6 +74,11 @@ QList<QgsMapLayer *> QgsMapLayerStore::addMapLayers( const QList<QgsMapLayer *>
       QgsDebugMsg( QStringLiteral( "Cannot add null layers" ) );
       continue;
     }
+    // If the layer is already in the store but its validity has flipped to TRUE reset data source
+    if ( mMapLayers.contains( myLayer->id() ) && ! mMapLayers[myLayer->id()]->isValid() && myLayer->isValid() && myLayer->dataProvider() )
+    {
+      mMapLayers[myLayer->id()]->setDataSource( myLayer->dataProvider()->dataSourceUri(), myLayer->name(), myLayer->providerType(), QgsDataProvider::ProviderOptions() );
+    }
     //check the layer is not already registered!
     if ( !mMapLayers.contains( myLayer->id() ) )
     {

diff --git a/src/core/qgsmaplayerstore.h b/src/core/qgsmaplayerstore.h
@@ -149,6 +149,11 @@ class CORE_EXPORT QgsMapLayerStore : public QObject
      *                      If you specify FALSE here you have take care of deleting
      *                      the layers yourself. Not available in Python.
      *
+     *
+     * \note If a layer with the same id is already in the store it is not added again,
+     *       but if the validity of the layer has changed from FALSE to TRUE, the
+     *       layer data source is updated to the new one.
+     *
      * \returns a list of the map layers that were added
      *         successfully. If a layer already exists in the store,
      *         it will not be part of the returned list.

diff --git a/src/core/qgsproject.cpp b/src/core/qgsproject.cpp
@@ -944,30 +944,30 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
 {
   QString type = layerElem.attribute( QStringLiteral( "type" ) );
   QgsDebugMsgLevel( "Layer type is " + type, 4 );
-  QgsMapLayer *mapLayer = nullptr;
+  std::unique_ptr<QgsMapLayer> mapLayer;
 
   if ( type == QLatin1String( "vector" ) )
   {
-    mapLayer = new QgsVectorLayer;
+    mapLayer = qgis::make_unique<QgsVectorLayer>();
 
     // apply specific settings to vector layer
-    if ( QgsVectorLayer *vl = qobject_cast<QgsVectorLayer *>( mapLayer ) )
+    if ( QgsVectorLayer *vl = qobject_cast<QgsVectorLayer *>( mapLayer.get() ) )
     {
       vl->setReadExtentFromXml( mTrustLayerMetadata );
     }
   }
   else if ( type == QLatin1String( "raster" ) )
   {
-    mapLayer = new QgsRasterLayer;
+    mapLayer =  qgis::make_unique<QgsRasterLayer>();
   }
   else if ( type == QLatin1String( "mesh" ) )
   {
-    mapLayer = new QgsMeshLayer;
+    mapLayer = qgis::make_unique<QgsMeshLayer>();
   }
   else if ( type == QLatin1String( "plugin" ) )
   {
     QString typeName = layerElem.attribute( QStringLiteral( "name" ) );
-    mapLayer = QgsApplication::pluginLayerRegistry()->createLayer( typeName );
+    mapLayer.reset( QgsApplication::pluginLayerRegistry()->createLayer( typeName ) );
   }
 
   if ( !mapLayer )
@@ -978,13 +978,19 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
 
   Q_CHECK_PTR( mapLayer ); // NOLINT
 
+  // This is tricky: to avoid a leak we need to check if the layer was already in the store
+  // because if it was, the newly created layer will not be added to the store and it would leak.
+  const QString layerId { layerElem.namedItem( QStringLiteral( "id" ) ).toElement().text() };
+  Q_ASSERT( ! layerId.isEmpty() );
+  const bool layerWasStored { layerStore()->mapLayer( layerId ) };
+
   // have the layer restore state that is stored in Dom node
   bool layerIsValid = mapLayer->readLayerXml( layerElem, context ) && mapLayer->isValid();
   QList<QgsMapLayer *> newLayers;
-  newLayers << mapLayer;
+  newLayers << mapLayer.get();
   if ( layerIsValid )
   {
-    emit readMapLayer( mapLayer, layerElem );
+    emit readMapLayer( mapLayer.get(), layerElem );
     addMapLayers( newLayers );
   }
   else
@@ -995,6 +1001,14 @@ bool QgsProject::addLayer( const QDomElement &layerElem, QList<QDomNode> &broken
     QgsDebugMsg( "Unable to load " + type + " layer" );
     brokenNodes.push_back( layerElem );
   }
+
+  // It should be safe to delete the layer now if layer was stored, because all the store
+  // had to to was to reset the data source in case the validity changed.
+  if ( ! layerWasStored )
+  {
+    mapLayer.release();
+  }
+
   return layerIsValid;
 }
 

diff --git a/tests/src/python/test_qgsmaplayerstore.py b/tests/src/python/test_qgsmaplayerstore.py
@@ -12,14 +12,25 @@
 # This will get replaced with a git SHA1 when you do a git archive
 __revision__ = '$Format:%H$'
 
-from qgis.core import QgsMapLayerStore, QgsVectorLayer, QgsMapLayer
+import os
+from qgis.core import (
+    QgsMapLayerStore,
+    QgsVectorLayer,
+    QgsMapLayer,
+    QgsDataProvider,
+    QgsProject,
+    QgsReadWriteContext,
+)
 from qgis.testing import start_app, unittest
 from qgis.PyQt.QtCore import QT_VERSION_STR
 from qgis.PyQt import sip
 from qgis.PyQt.QtTest import QSignalSpy
+from qgis.PyQt.QtXml import QDomDocument, QDomNode
 from time import sleep
+from utilities import unitTestDataPath
 
 start_app()
+TEST_DATA_DIR = unitTestDataPath()
 
 
 def createLayer(name):
@@ -529,6 +540,38 @@ def testTransferLayers(self):
         self.assertEqual(len(store1.mapLayers()), 3)
         self.assertEqual(store1.mapLayers(), {l1.id(): l1, l2.id(): l2, l3.id(): l3})
 
+    def testLayerDataSourceReset(self):
+        """When adding a layer with the same id to the store make sure
+        the data source is also updated in case the layer validity has
+        changed from False to True"""
+
+        p = QgsProject()
+        store = p.layerStore()
+        vl1 = createLayer('valid')
+        vl2 = QgsVectorLayer('/not_a_valid_path.shp', 'invalid', 'ogr')
+        self.assertTrue(vl1.isValid())
+        self.assertFalse(vl2.isValid())
+        store.addMapLayers([vl1, vl2])
+        self.assertEqual(store.validCount(), 1)
+        self.assertEqual(len(store.mapLayers()), 2)
+
+        # Re-add the bad layer
+        store.addMapLayers([vl2])
+        self.assertEqual(store.validCount(), 1)
+        self.assertEqual(len(store.mapLayers()), 2)
+
+        doc = QDomDocument()
+        doc.setContent('<maplayer><provider encoding="UTF-8">ogr</provider><layername>fixed</layername><id>%s</id></maplayer>' % vl2.id())
+        layer_node = QDomNode(doc.firstChild())
+        self.assertTrue(vl2.writeXml(layer_node, doc, QgsReadWriteContext()))
+        datasource_node = doc.createElement("datasource")
+        datasource_node.appendChild(doc.createTextNode(os.path.join(TEST_DATA_DIR, 'points.shp')))
+        layer_node.appendChild(datasource_node)
+        p.readLayer(layer_node)
+        self.assertEqual(store.validCount(), 2)
+        self.assertEqual(len(store.mapLayers()), 2)
+        self.assertEqual(store.mapLayers()[vl2.id()].name(), 'fixed')
+
 
 if __name__ == '__main__':
     unittest.main()