Fix crash when saving vector layer in background task

qgis · Jun 10, 2017 · 326e442 · 326e442
1 parent 30e145a
commit 326e442
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 4 deletions.
diff --git a/python/core/qgsvectorfilewriter.sip b/python/core/qgsvectorfilewriter.sip
@@ -179,6 +179,12 @@ Constructor
  :return: possibly modified value.
  :rtype: QVariant
 %End
+
+        virtual QgsVectorFileWriter::FieldValueConverter *clone() const /Factory/;
+%Docstring
+ Creates a clone of the FieldValueConverter.
+ :rtype: QgsVectorFileWriter.FieldValueConverter
+%End
     };
 
     enum EditionCapability
@@ -409,7 +415,10 @@ Set to true to include z dimension in output. This option is only valid if overr
 
         QgsVectorFileWriter::FieldValueConverter *fieldValueConverter;
 %Docstring
-Field value converter
+ Field value converter.
+
+ Ownership is not transferred and callers must ensure that the lifetime of fieldValueConverter
+ exceeds the lifetime of the QgsVectorFileWriter object.
 %End
 
         QgsFeedback *feedback;

diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp
@@ -6722,8 +6722,10 @@ class QgisAppFieldValueConverter : public QgsVectorFileWriter::FieldValueConvert
 
     virtual QVariant convert( int idx, const QVariant &value ) override;
 
+    QgisAppFieldValueConverter *clone() const override;
+
   private:
-    QgsVectorLayer *mLayer = nullptr;
+    QPointer< QgsVectorLayer > mLayer;
     QgsAttributeList mAttributesAsDisplayedValues;
 };
 
@@ -6735,6 +6737,9 @@ QgisAppFieldValueConverter::QgisAppFieldValueConverter( QgsVectorLayer *vl, cons
 
 QgsField QgisAppFieldValueConverter::fieldDefinition( const QgsField &field )
 {
+  if ( !mLayer )
+    return field;
+
   int idx = mLayer->fields().indexFromName( field.name() );
   if ( mAttributesAsDisplayedValues.contains( idx ) )
   {
@@ -6745,7 +6750,7 @@ QgsField QgisAppFieldValueConverter::fieldDefinition( const QgsField &field )
 
 QVariant QgisAppFieldValueConverter::convert( int idx, const QVariant &value )
 {
-  if ( !mAttributesAsDisplayedValues.contains( idx ) )
+  if ( !mLayer || !mAttributesAsDisplayedValues.contains( idx ) )
   {
     return value;
   }
@@ -6754,6 +6759,11 @@ QVariant QgisAppFieldValueConverter::convert( int idx, const QVariant &value )
   return fieldFormatter->representValue( mLayer, idx, setup.config(), QVariant(), value );
 }
 
+QgisAppFieldValueConverter *QgisAppFieldValueConverter::clone() const
+{
+  return new QgisAppFieldValueConverter( *this );
+}
+
 ///@endcond
 
 void QgisApp::saveAsVectorFileGeneral( QgsVectorLayer *vlayer, bool symbologyOption )

diff --git a/src/core/qgsvectorfilewriter.cpp b/src/core/qgsvectorfilewriter.cpp
@@ -65,6 +65,11 @@ QVariant QgsVectorFileWriter::FieldValueConverter::convert( int /*fieldIdxInLaye
   return value;
 }
 
+QgsVectorFileWriter::FieldValueConverter *QgsVectorFileWriter::FieldValueConverter::clone() const
+{
+  return new FieldValueConverter( *this );
+}
+
 QgsVectorFileWriter::QgsVectorFileWriter(
   const QString &vectorFileName,
   const QString &fileEncoding,

diff --git a/src/core/qgsvectorfilewriter.h b/src/core/qgsvectorfilewriter.h
@@ -202,6 +202,11 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink
          * \returns possibly modified value.
          */
         virtual QVariant convert( int fieldIdxInLayer, const QVariant &value );
+
+        /**
+         * Creates a clone of the FieldValueConverter.
+         */
+        virtual QgsVectorFileWriter::FieldValueConverter *clone() const SIP_FACTORY;
     };
 
     /** Edition capability flags
@@ -394,7 +399,12 @@ class CORE_EXPORT QgsVectorFileWriter : public QgsFeatureSink
         //! Set to true to include z dimension in output. This option is only valid if overrideGeometryType is set
         bool includeZ;
 
-        //! Field value converter
+        /**
+         * Field value converter.
+         *
+         * Ownership is not transferred and callers must ensure that the lifetime of fieldValueConverter
+         * exceeds the lifetime of the QgsVectorFileWriter object.
+         */
         QgsVectorFileWriter::FieldValueConverter *fieldValueConverter = nullptr;
 
         //! Optional feedback object allowing cancelation of layer save

diff --git a/src/core/qgsvectorfilewritertask.cpp b/src/core/qgsvectorfilewritertask.cpp
@@ -24,6 +24,13 @@ QgsVectorFileWriterTask::QgsVectorFileWriterTask( QgsVectorLayer *layer, const Q
   , mDestFileName( fileName )
   , mOptions( options )
 {
+  if ( mOptions.fieldValueConverter )
+  {
+    // fieldValueConverter is not owned - so we need to clone it here
+    // to ensure it exists for lifetime of task
+    mFieldValueConverter.reset( mOptions.fieldValueConverter->clone() );
+    mOptions.fieldValueConverter = mFieldValueConverter.get();
+  }
   if ( !mOptions.feedback )
   {
     mOwnedFeedback.reset( new QgsFeedback() );

diff --git a/src/core/qgsvectorfilewritertask.h b/src/core/qgsvectorfilewritertask.h
@@ -82,6 +82,7 @@ class CORE_EXPORT QgsVectorFileWriterTask : public QgsTask
     QString mErrorMessage;
 
     QgsVectorFileWriter::SaveVectorOptions mOptions;
+    std::unique_ptr< QgsVectorFileWriter::FieldValueConverter > mFieldValueConverter;
 };
 
 #endif
diff --git a/tests/src/python/test_qgsvectorfilewritertask.py b/tests/src/python/test_qgsvectorfilewritertask.py
@@ -116,6 +116,26 @@ def testNoLayer(self):
         self.assertFalse(self.success)
         self.assertTrue(self.fail)
 
+    def testFieldValueConverter(self):
+        """test no crash when fieldValueConverter is used"""
+        self.layer = self.createLayer()
+        options = QgsVectorFileWriter.SaveVectorOptions()
+        converter = QgsVectorFileWriter.FieldValueConverter()
+        options.fieldValueConverter = converter
+        tmp = create_temp_filename('converter.shp')
+        task = QgsVectorFileWriterTask(self.layer, tmp, options)
+
+        task.writeComplete.connect(self.onSuccess)
+        task.errorOccurred.connect(self.onFail)
+
+        del converter
+
+        QgsApplication.taskManager().addTask(task)
+        while not self.success and not self.fail:
+            QCoreApplication.processEvents()
+
+        self.assertTrue(self.success)
+        self.assertFalse(self.fail)
 
 if __name__ == '__main__':
     unittest.main()