Fix model sync when fields change

python/gui/auto_generated/qgsfieldmappingmodel.sip.in

@@ -119,9 +119,9 @@ optionally specified through ``expressions`` which is a map from the original
 field name to the corresponding expression.
 %End
 
-    virtual int rowCount( const QModelIndex &parent ) const;
+    virtual int rowCount( const QModelIndex &parent = QModelIndex() ) const;
 
-    virtual int columnCount( const QModelIndex &parent ) const;
+    virtual int columnCount( const QModelIndex &parent = QModelIndex() ) const;
 
     virtual QVariant data( const QModelIndex &index, int role ) const;
 

python/gui/auto_generated/qgsfieldmappingwidget.sip.in

@@ -23,10 +23,10 @@ for each set of "destination" fields an expression defines how to obtain the val
 %End
   public:
 
-    explicit QgsFieldMappingWidget( const QgsFields &sourceFields = QgsFields(),
+    explicit QgsFieldMappingWidget( QWidget *parent = 0,
+                                    const QgsFields &sourceFields = QgsFields(),
                                     const QgsFields &destinationFields = QgsFields(),
-                                    const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>(),
-                                    QWidget *parent = 0 );
+                                    const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>() );
 %Docstring
 Constructs a QgsFieldMappingWidget from a set of ``sourceFields``
 and ``destinationFields``, initial values for the expressions can be
@@ -72,6 +72,11 @@ Set destination fields to ``destinationFields`` in the underlying model,
 initial values for the expressions can be optionally specified through
 ``expressions`` which is a map from the original field name to the
 corresponding expression.
+%End
+
+    void scrollTo( const QModelIndex &index ) const;
+%Docstring
+Scroll the fields view to ``index``
 %End
 
   public slots:

src/gui/qgsfieldmappingmodel.cpp

@@ -57,6 +57,10 @@ QVariant QgsFieldMappingModel::headerData( int section, Qt::Orientation orientat
         {
           return tr( "Precision" );
         }
+        case static_cast<int>( ColumnDataIndex::DestinationConstraints ):
+        {
+          return tr( "Constraints" );
+        }
       }
     }
     else if ( orientation == Qt::Vertical )
@@ -275,9 +279,51 @@ bool QgsFieldMappingModel::moveUpOrDown( const QModelIndex &index, bool up )
   return true;
 }
 
+QString QgsFieldMappingModel::bestMatchforField( const QgsFieldMappingModel::Field &f, QStringList &excludedFieldNames )
+{
+  // Search for fields in the source
+  // 1. match by name
+  for ( const auto &sf : qgis::as_const( mSourceFields ) )
+  {
+    if ( sf.name() == f.field.name() )
+    {
+      return QgsExpression::quotedColumnRef( sf.name() );
+    }
+  }
+  // 2. match by type
+  for ( const auto &sf : qgis::as_const( mSourceFields ) )
+  {
+    if ( excludedFieldNames.contains( sf.name() ) || sf.type() != f.field.type() )
+      continue;
+    excludedFieldNames.push_back( sf.name() );
+    return QgsExpression::quotedColumnRef( sf.name() );
+  }
+  return QString();
+}
+
 void QgsFieldMappingModel::setSourceFields( const QgsFields &sourceFields )
 {
   mSourceFields = sourceFields;
+  QStringList usedFields;
+  beginResetModel();
+  for ( const Field &f : qgis::as_const( mMapping ) )
+  {
+    if ( f.expression.isField() )
+    {
+      usedFields.push_back( f.expression.expression().mid( 1, f.expression.expression().length() - 2 ) );
+    }
+  }
+  // not const on purpose
+  for ( Field &f : mMapping )
+  {
+    if ( f.expression.expression().isEmpty() )
+    {
+      const QString expression { bestMatchforField( f, usedFields ) };
+      if ( ! expression.isEmpty() )
+        f.expression = expression;
+    }
+  }
+  endResetModel();
 }
 
 void QgsFieldMappingModel::setDestinationFields( const QgsFields &destinationFields,
@@ -304,31 +350,9 @@ void QgsFieldMappingModel::setDestinationFields( const QgsFields &destinationFie
     }
     else
     {
-      bool found { false };
-      // Search for fields in the source
-      // 1. match by name
-      for ( const auto &sf : qgis::as_const( mSourceFields ) )
-      {
-        if ( sf.name() == f.field.name() )
-        {
-          f.expression = QgsExpression::quotedColumnRef( sf.name() );
-          found = true;
-          usedFields.push_back( sf.name() );
-          break;
-        }
-      }
-      // 2. match by type
-      if ( ! found )
-      {
-        for ( const auto &sf : qgis::as_const( mSourceFields ) )
-        {
-          if ( usedFields.contains( sf.name() ) || sf.type() != f.field.type() )
-            continue;
-          f.expression = QgsExpression::quotedColumnRef( sf.name() );
-          usedFields.push_back( sf.name() );
-          found = true;
-        }
-      }
+      const QString expression { bestMatchforField( f, usedFields ) };
+      if ( ! expression.isEmpty() )
+        f.expression = expression;
     }
     mMapping.push_back( f );
   }

src/gui/qgsfieldmappingmodel.h

@@ -125,8 +125,8 @@ class GUI_EXPORT QgsFieldMappingModel: public QAbstractTableModel
                                const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>() );
 
     // QAbstractItemModel interface
-    int rowCount( const QModelIndex &parent ) const override;
-    int columnCount( const QModelIndex &parent ) const override;
+    int rowCount( const QModelIndex &parent = QModelIndex() ) const override;
+    int columnCount( const QModelIndex &parent = QModelIndex() ) const override;
     QVariant data( const QModelIndex &index, int role ) const override;
     QVariant headerData( int section, Qt::Orientation orientation, int role ) const override;
     Qt::ItemFlags flags( const QModelIndex &index ) const override;
@@ -154,6 +154,8 @@ class GUI_EXPORT QgsFieldMappingModel: public QAbstractTableModel
 
     bool moveUpOrDown( const QModelIndex &index, bool up = true );
 
+    QString bestMatchforField( const QgsFieldMappingModel::Field &field, QStringList &excludedFieldNames );
+
     QList<Field> mMapping;
     bool mDestinationEditable = false;
     QgsFields mSourceFields;

src/gui/qgsfieldmappingwidget.cpp

@@ -17,10 +17,10 @@
 #include "qgsfieldmappingwidget.h"
 #include "qgsfieldexpressionwidget.h"
 
-QgsFieldMappingWidget::QgsFieldMappingWidget( const QgsFields &sourceFields,
+QgsFieldMappingWidget::QgsFieldMappingWidget( QWidget *parent,
+    const QgsFields &sourceFields,
     const QgsFields &destinationFields,
-    const QMap<QString, QgsExpression> &expressions,
-    QWidget *parent )
+    const QMap<QString, QgsExpression> &expressions )
   : QWidget( parent )
 {
 
@@ -31,6 +31,9 @@ QgsFieldMappingWidget::QgsFieldMappingWidget( const QgsFields &sourceFields,
   mTableView->setItemDelegateForColumn( static_cast<int>( QgsFieldMappingModel::ColumnDataIndex::SourceExpression ), new ExpressionDelegate( mTableView ) );
   mTableView->setItemDelegateForColumn( static_cast<int>( QgsFieldMappingModel::ColumnDataIndex::DestinationType ), new TypeDelegate( mTableView ) );
   updateColumns();
+  // Make sure columns are updated when rows are added
+  connect( mModel, &QgsFieldMappingModel::rowsInserted, this, [ = ] { updateColumns(); } );
+  connect( mModel, &QgsFieldMappingModel::modelReset, this, [ = ] { updateColumns(); } );
 }
 
 void QgsFieldMappingWidget::setDestinationEditable( bool editable )
@@ -69,6 +72,11 @@ void QgsFieldMappingWidget::setDestinationFields( const QgsFields &destinationFi
   model()->setDestinationFields( destinationFields, expressions );
 }
 
+void QgsFieldMappingWidget::scrollTo( const QModelIndex &index ) const
+{
+  mTableView->scrollTo( index );
+}
+
 void QgsFieldMappingWidget::appendField( const QgsField &field, const QgsExpression &expression )
 {
   model()->appendField( field, expression );
@@ -138,8 +146,7 @@ void QgsFieldMappingWidget::updateColumns()
   for ( int i = 0; i < mModel->rowCount(); ++i )
   {
     mTableView->openPersistentEditor( mModel->index( i, static_cast<int>( QgsFieldMappingModel::ColumnDataIndex::SourceExpression ) ) );
-    if ( destinationEditable() )
-      mTableView->openPersistentEditor( mModel->index( i, static_cast<int>( QgsFieldMappingModel::ColumnDataIndex::DestinationType ) ) );
+    mTableView->openPersistentEditor( mModel->index( i, static_cast<int>( QgsFieldMappingModel::ColumnDataIndex::DestinationType ) ) );
   }
 
   for ( int i = 0; i < mModel->columnCount(); ++i )
@@ -225,14 +232,21 @@ QWidget *QgsFieldMappingWidget::TypeDelegate::createEditor( QWidget *parent, con
     editor->setItemData( i, static_cast<int>( type ), Qt::UserRole );
     ++i;
   }
-  connect( editor,
-           qgis::overload<int >::of( &QComboBox::currentIndexChanged ),
-           this,
-           [ = ]( int currentIndex )
+  if ( ! model->destinationEditable() )
   {
-    Q_UNUSED( currentIndex )
-    const_cast< QgsFieldMappingWidget::TypeDelegate *>( this )->emit commitData( editor );
-  } );
+    editor->setEnabled( false );
+  }
+  else
+  {
+    connect( editor,
+             qgis::overload<int >::of( &QComboBox::currentIndexChanged ),
+             this,
+             [ = ]( int currentIndex )
+    {
+      Q_UNUSED( currentIndex )
+      const_cast< QgsFieldMappingWidget::TypeDelegate *>( this )->emit commitData( editor );
+    } );
+  }
   return editor;
 }
 

src/gui/qgsfieldmappingwidget.h

@@ -44,10 +44,10 @@ class GUI_EXPORT QgsFieldMappingWidget : public QWidget, private Ui::QgsFieldMap
      * field name to the corresponding expression. A \param parent object
      * can also be specified.
      */
-    explicit QgsFieldMappingWidget( const QgsFields &sourceFields = QgsFields(),
+    explicit QgsFieldMappingWidget( QWidget *parent = nullptr,
+                                    const QgsFields &sourceFields = QgsFields(),
                                     const QgsFields &destinationFields = QgsFields(),
-                                    const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>(),
-                                    QWidget *parent = nullptr );
+                                    const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>() );
 
     //! Sets the destination fields editable state to \a ditable
     void setDestinationEditable( bool editable );
@@ -76,6 +76,11 @@ class GUI_EXPORT QgsFieldMappingWidget : public QWidget, private Ui::QgsFieldMap
     void setDestinationFields( const QgsFields &destinationFields,
                                const QMap<QString, QgsExpression> &expressions = QMap<QString, QgsExpression>() );
 
+    /**
+     * Scroll the fields view to \a index
+     */
+    void scrollTo( const QModelIndex &index ) const;
+
   public slots:
 
     //! Appends a new \a field to the model, with an optional \a expression

tests/src/python/test_qgsfieldmappingwidget.py

@@ -155,6 +155,24 @@ class TestPyQgsFieldMappingModel(unittest.TestCase):
         self.assertEqual(mapping[2].originalName, 'destination_field2')
         self.assertEqual(mapping[1].originalName, 'destination_field3')
 
+    def testSetSourceFields(self):
+        """Test that changing source fields also empty expressions are updated"""
+
+        model = QgsFieldMappingModel(self.source_fields, self.destination_fields)
+        self.assertEqual(model.data(model.index(2, 0), Qt.DisplayRole), QVariant())
+        self.assertEqual(model.data(model.index(2, 1), Qt.DisplayRole), 'destination_field3')
+
+        f = QgsField('source_field3', QVariant.String)
+        fields = self.source_fields
+        fields.append(f)
+        model.setSourceFields(fields)
+        self.assertEqual(model.data(model.index(0, 0), Qt.DisplayRole), '"source_field2"')
+        self.assertEqual(model.data(model.index(0, 1), Qt.DisplayRole), 'destination_field1')
+        self.assertEqual(model.data(model.index(1, 0), Qt.DisplayRole), '"source_field1"')
+        self.assertEqual(model.data(model.index(1, 1), Qt.DisplayRole), 'destination_field2')
+        self.assertEqual(model.data(model.index(2, 0), Qt.DisplayRole), '"source_field3"')
+        self.assertEqual(model.data(model.index(2, 1), Qt.DisplayRole), 'destination_field3')
+
     def testWidget(self):
         """Test widget operations"""
 
@@ -205,7 +223,7 @@ class TestPyQgsFieldMappingModel(unittest.TestCase):
         self.assertEqual(mapping[2].originalName, 'destination_field3')
 
         # Test constraints
-        f = QgsField('constraint_field')
+        f = QgsField('constraint_field', QVariant.Int)
         constraints = QgsFieldConstraints()
         constraints.setConstraint(QgsFieldConstraints.ConstraintNotNull, QgsFieldConstraints.ConstraintOriginProvider)
         constraints.setConstraint(QgsFieldConstraints.ConstraintExpression, QgsFieldConstraints.ConstraintOriginProvider)
@@ -218,6 +236,8 @@ class TestPyQgsFieldMappingModel(unittest.TestCase):
         self.assertEqual(widget.model().data(widget.model().index(0, 5, QModelIndex()), Qt.ToolTipRole), "Unique<br>Not null<br>Expression")
         self.assertEqual(widget.model().data(widget.model().index(0, 5, QModelIndex()), Qt.BackgroundColorRole), QColor(255, 224, 178))
 
+        #self._showDialog(widget)
+
 
 if __name__ == '__main__':
     unittest.main()