Merge pull request #4503 from boundlessgeo/attributetable-fixes

[bugfix] Fixes attribute table duplicated rows #15974
qgis · May 9, 2017 · 212acc1 · 212acc1
2 parents a4e3c83 + 3784892
commit 212acc1
Show file tree

Hide file tree

Showing 7 changed files with 76 additions and 27 deletions.
diff --git a/src/gui/attributetable/qgsattributetablefiltermodel.cpp b/src/gui/attributetable/qgsattributetablefiltermodel.cpp
@@ -247,13 +247,14 @@ void QgsAttributeTableFilterModel::setSourceModel( QgsAttributeTableModel *sourc
   QSortFilterProxyModel::setSourceModel( sourceModel );
 
   // Disconnect any code to update columns in the parent, we handle this manually
-  disconnect( sourceModel, SIGNAL( columnsAboutToBeInserted( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsAboutToBeInserted( QModelIndex, int, int ) ) );
-  disconnect( sourceModel, SIGNAL( columnsInserted( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsInserted( QModelIndex, int, int ) ) );
-  disconnect( sourceModel, SIGNAL( columnsAboutToBeRemoved( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsAboutToBeRemoved( QModelIndex, int, int ) ) );
-  disconnect( sourceModel, SIGNAL( columnsRemoved( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsRemoved( QModelIndex, int, int ) ) );
-
+  disconnect( mTableModel, SIGNAL( columnsAboutToBeInserted( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsAboutToBeInserted( QModelIndex, int, int ) ) );
+  disconnect( mTableModel, SIGNAL( columnsInserted( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsInserted( QModelIndex, int, int ) ) );
+  disconnect( mTableModel, SIGNAL( columnsAboutToBeRemoved( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsAboutToBeRemoved( QModelIndex, int, int ) ) );
+  disconnect( mTableModel, SIGNAL( columnsRemoved( QModelIndex, int, int ) ), this, SLOT( _q_sourceColumnsRemoved( QModelIndex, int, int ) ) );
+  // The following connections are needed in order to keep the filter model in sync, see: regression #15974
   connect( mTableModel, &QAbstractItemModel::columnsAboutToBeInserted, this, &QgsAttributeTableFilterModel::onColumnsChanged );
   connect( mTableModel, &QAbstractItemModel::columnsAboutToBeRemoved, this, &QgsAttributeTableFilterModel::onColumnsChanged );
+
 }
 
 bool QgsAttributeTableFilterModel::selectedOnTop()

diff --git a/src/gui/attributetable/qgsattributetablemodel.cpp b/src/gui/attributetable/qgsattributetablemodel.cpp
@@ -150,10 +150,12 @@ void QgsAttributeTableModel::featuresDeleted( const QgsFeatureIds &fids )
 
 bool QgsAttributeTableModel::removeRows( int row, int count, const QModelIndex &parent )
 {
+
   if ( row < 0 || count < 1 )
     return false;
 
   beginRemoveRows( parent, row, row + count - 1 );
+
 #ifdef QGISDEBUG
   if ( 3 <= QgsLogger::debugLevel() )
     QgsDebugMsgLevel( QString( "remove %2 rows at %1 (rows %3, ids %4)" ).arg( row ).arg( count ).arg( mRowIdMap.size() ).arg( mIdRowMap.size() ), 3 );
@@ -222,15 +224,16 @@ void QgsAttributeTableModel::featureAdded( QgsFeatureId fid )
       mSortCache.insert( mFeat.id(), sortValue );
     }
 
-    int n = mRowIdMap.size();
-    beginInsertRows( QModelIndex(), n, n );
-
-    mIdRowMap.insert( fid, n );
-    mRowIdMap.insert( n, fid );
-
-    endInsertRows();
-
-    reload( index( rowCount() - 1, 0 ), index( rowCount() - 1, columnCount() ) );
+    // Skip if the fid is already in the map (do not add twice)!
+    if ( ! mIdRowMap.contains( fid ) )
+    {
+      int n = mRowIdMap.size();
+      beginInsertRows( QModelIndex(), n, n );
+      mIdRowMap.insert( fid, n );
+      mRowIdMap.insert( n, fid );
+      endInsertRows();
+      reload( index( rowCount() - 1, 0 ), index( rowCount() - 1, columnCount() ) );
+    }
   }
 }
 
@@ -431,10 +434,10 @@ void QgsAttributeTableModel::loadLayer()
   emit finished();
 
   connect( mLayerCache, &QgsVectorLayerCache::invalidated, this, &QgsAttributeTableModel::loadLayer, Qt::UniqueConnection );
-
   endResetModel();
 }
 
+
 void QgsAttributeTableModel::fieldConditionalStyleChanged( const QString &fieldName )
 {
   if ( fieldName.isNull() )
@@ -472,6 +475,8 @@ void QgsAttributeTableModel::swapRows( QgsFeatureId a, QgsFeatureId b )
   mIdRowMap.remove( b );
   mIdRowMap.insert( a, rowB );
   mIdRowMap.insert( b, rowA );
+  Q_ASSERT( mRowIdMap.size() == mIdRowMap.size() );
+
 
   //emit layoutChanged();
 }

diff --git a/src/gui/attributetable/qgsattributetablemodel.h b/src/gui/attributetable/qgsattributetablemodel.h
@@ -377,6 +377,9 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
     QgsAttributeEditorContext mEditorContext;
 
     int mExtraColumns;
+
+    friend class TestQgsAttributeTable;
+
 };
 
 

diff --git a/src/gui/attributetable/qgsdualview.cpp b/src/gui/attributetable/qgsdualview.cpp
@@ -111,6 +111,7 @@ void QgsDualView::init( QgsVectorLayer *layer, QgsMapCanvas *mapCanvas, const Qg
   connect( mMasterModel, &QgsAttributeTableModel::modelChanged, mAttributeForm, &QgsAttributeForm::refreshFeature );
   connect( mAttributeForm, &QgsAttributeForm::filterExpressionSet, this, &QgsDualView::filterExpressionSet );
   connect( mFilterModel, &QgsAttributeTableFilterModel::sortColumnChanged, this, &QgsDualView::onSortColumnChanged );
+
   if ( mFeatureListPreviewButton->defaultAction() )
     mFeatureList->setDisplayExpression( mDisplayExpression );
   else
@@ -302,6 +303,11 @@ void QgsDualView::initModels( QgsMapCanvas *mapCanvas, const QgsFeatureRequest &
 
   mFilterModel = new QgsAttributeTableFilterModel( mapCanvas, mMasterModel, mMasterModel );
 
+  // The following connections to invalidate() are necessary to keep the filter model in sync
+  // see regression https://issues.qgis.org/issues/15974
+  connect( mMasterModel, &QgsAttributeTableModel::rowsRemoved, mFilterModel, &QgsAttributeTableFilterModel::invalidate );
+  connect( mMasterModel, &QgsAttributeTableModel::rowsInserted, mFilterModel, &QgsAttributeTableFilterModel::invalidate );
+
   connect( mFeatureList, &QgsFeatureListView::displayExpressionChanged, this, &QgsDualView::displayExpressionChanged );
 
   mFeatureListModel = new QgsFeatureListModel( mFilterModel, mFilterModel );

diff --git a/src/gui/attributetable/qgsdualview.h b/src/gui/attributetable/qgsdualview.h
@@ -356,6 +356,7 @@ class GUI_EXPORT QgsDualView : public QStackedWidget, private Ui::QgsDualViewBas
     QgsMapCanvas *mMapCanvas = nullptr;
 
     friend class TestQgsDualView;
+    friend class TestQgsAttributeTable;
 };
 
 /** \ingroup gui

diff --git a/src/gui/layertree/qgslayertreeview.cpp b/src/gui/layertree/qgslayertreeview.cpp
@@ -253,20 +253,23 @@ void QgsLayerTreeView::updateExpandedStateFromNode( QgsLayerTreeNode *node )
 
 QgsMapLayer *QgsLayerTreeView::layerForIndex( const QModelIndex &index ) const
 {
-  QgsLayerTreeNode *node = layerTreeModel()->index2node( index );
-  if ( node )
+  // Check if model has been set and index is valid
+  if ( layerTreeModel() && index.isValid( ) )
   {
-    if ( QgsLayerTree::isLayer( node ) )
-      return QgsLayerTree::toLayer( node )->layer();
-  }
-  else
-  {
-    // possibly a legend node
-    QgsLayerTreeModelLegendNode *legendNode = layerTreeModel()->index2legendNode( index );
-    if ( legendNode )
-      return legendNode->layerNode()->layer();
+    QgsLayerTreeNode *node = layerTreeModel()->index2node( index );
+    if ( node )
+    {
+      if ( QgsLayerTree::isLayer( node ) )
+        return QgsLayerTree::toLayer( node )->layer();
+    }
+    else
+    {
+      // possibly a legend node
+      QgsLayerTreeModelLegendNode *legendNode = layerTreeModel()->index2legendNode( index );
+      if ( legendNode )
+        return legendNode->layerNode()->layer();
+    }
   }
-
   return nullptr;
 }
 

diff --git a/tests/src/app/testqgsattributetable.cpp b/tests/src/app/testqgsattributetable.cpp
@@ -25,6 +25,7 @@
 #include "qgsmapcanvas.h"
 #include "qgsunittypes.h"
 #include "qgssettings.h"
+#include "qgsvectorfilewriter.h"
 
 #include "qgstest.h"
 
@@ -42,6 +43,7 @@ class TestQgsAttributeTable : public QObject
     void cleanupTestCase();// will be called after the last testfunction was executed.
     void init() {} // will be called before each testfunction is executed.
     void cleanup() {} // will be called after every testfunction.
+    void testRegression15974();
     void testFieldCalculation();
     void testFieldCalculationArea();
     void testNoGeom();
@@ -247,6 +249,34 @@ void TestQgsAttributeTable::testSelected()
   QVERIFY( dlg->mMainView->masterModel()->request().filterFids().isEmpty() );
 }
 
+void TestQgsAttributeTable::testRegression15974()
+{
+  QString path = QDir::tempPath() + "/testshp15974.shp";
+  std::unique_ptr< QgsVectorLayer> tempLayer( new QgsVectorLayer( QStringLiteral( "polygon?crs=epsg:4326&field=id:integer" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
+  QVERIFY( tempLayer->isValid() );
+  QgsVectorFileWriter::writeAsVectorFormat( tempLayer.get( ), path, "system", QgsCoordinateReferenceSystem( 4326 ), "ESRI Shapefile" );
+  std::unique_ptr< QgsVectorLayer> shpLayer( new QgsVectorLayer( path, QStringLiteral( "test" ),  QStringLiteral( "ogr" ) ) );
+  QgsFeature f1( shpLayer->dataProvider()->fields(), 1 );
+  f1.setGeometry( QgsGeometry().fromWkt( QStringLiteral( "polygon(0 0, 1 1, 1 2, 1 0, 0 0))" ) ) );
+  QgsFeature f2( shpLayer->dataProvider()->fields(), 2 );
+  f2.setGeometry( QgsGeometry().fromWkt( QStringLiteral( "polygon(0 0, 1 1, 1 2, 1 0, 0 0))" ) ) );
+  QgsFeature f3( shpLayer->dataProvider()->fields(), 3 );
+  f3.setGeometry( QgsGeometry().fromWkt( QStringLiteral( "polygon(0 0, 1 1, 1 2, 1 0, 0 0))" ) ) );
+  QVERIFY( shpLayer->startEditing( ) );
+  QVERIFY( shpLayer->addFeatures( QgsFeatureList() << f1 << f2 << f3 ) );
+  std::unique_ptr< QgsAttributeTableDialog > dlg( new QgsAttributeTableDialog( shpLayer.get() ) );
+  QCOMPARE( shpLayer->featureCount( ), 3L );
+  mQgisApp->saveEdits( shpLayer.get( ) );
+  QCOMPARE( shpLayer->featureCount( ), 3L );
+  QCOMPARE( dlg->mMainView->masterModel()->rowCount(), 3 );
+  QCOMPARE( dlg->mMainView->mLayerCache->cachedFeatureIds( ).count(), 3 );
+  QCOMPARE( dlg->mMainView->featureCount( ), 3 );
+  // All the following instructions made the test pass, before the connections to invalidate()
+  // were introduced in QgsDualView::initModels
+  // dlg->mMainView->mFilterModel->setSourceModel( dlg->mMainView->masterModel() );
+  // dlg->mMainView->mFilterModel->invalidate();
+  QCOMPARE( dlg->mMainView->filteredFeatureCount( ), 3 );
+}
 
 QGSTEST_MAIN( TestQgsAttributeTable )
 #include "testqgsattributetable.moc"