Merge pull request #8200 from elpaso/bugfix-20094-followup-096b4ce

Slow field calculator - Bugfix 20094 followup 096b4ce
qgis · Oct 18, 2018 · 4010e79 · 4010e79
2 parents 009fcd8 + 7e9c874
commit 4010e79
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 95 deletions.
diff --git a/python/core/auto_generated/qgsapplication.sip.in b/python/core/auto_generated/qgsapplication.sip.in
@@ -824,7 +824,6 @@ Emits the signal to collect all the strings of .qgs to be included in ts file
 .. versionadded:: 3.4
 %End
 
-
 %If (ANDROID)
     //dummy method to workaround sip generation issue
     bool x11EventFilter( XEvent *event );
@@ -853,7 +852,6 @@ In order to register translatable strings, connect to this signal and register t
 .. versionadded:: 3.4
 %End
 
-
 };
 
 

diff --git a/src/app/qgisapp.cpp b/src/app/qgisapp.cpp
@@ -7118,14 +7118,13 @@ void QgisApp::fieldCalculator()
 
 void QgisApp::attributeTable( QgsAttributeTableFilterModel::FilterMode filter )
 {
-  QgsVectorLayer *vectorLayer = qobject_cast<QgsVectorLayer *>( activeLayer() );
-  if ( !vectorLayer )
+  QgsVectorLayer *myLayer = qobject_cast<QgsVectorLayer *>( activeLayer() );
+  if ( !myLayer )
   {
     return;
   }
 
-  QgsAttributeTableDialog *mDialog = new QgsAttributeTableDialog( vectorLayer, filter );
-
+  QgsAttributeTableDialog *mDialog = new QgsAttributeTableDialog( myLayer, filter );
   mDialog->show();
   // the dialog will be deleted by itself on close
 }
@@ -9217,7 +9216,6 @@ bool QgisApp::toggleEditing( QgsMapLayer *layer, bool allowCancel )
   return res;
 }
 
-
 void QgisApp::saveActiveLayerEdits()
 {
   saveEdits( activeLayer(), true, true );
@@ -14175,8 +14173,3 @@ void QgisApp::triggerCrashHandler()
   RaiseException( 0x12345678, 0, 0, nullptr );
 #endif
 }
-
-void QgisApp::blockAttributeTableUpdates( const QgsVectorLayer *layer, const bool blocked )
-{
-  emit attributeTableUpdateBlocked( layer, blocked );
-}
diff --git a/src/app/qgisapp.h b/src/app/qgisapp.h
@@ -1012,13 +1012,6 @@ class APP_EXPORT QgisApp : public QMainWindow, private Ui::MainWindow
      */
     void triggerCrashHandler();
 
-    /**
-     * Emits the signal to set the \a blocked state of attribute tables connected a particular \a layer
-     *
-     * \since QGIS 3.4
-     */
-    void blockAttributeTableUpdates( const QgsVectorLayer *layer, const bool blocked );
-
   protected:
 
     //! Handle state changes (WindowTitleChange)
@@ -1747,13 +1740,6 @@ class APP_EXPORT QgisApp : public QMainWindow, private Ui::MainWindow
      */
     void activeLayerChanged( QgsMapLayer *layer );
 
-    /**
-     * Emitted when \a blocked status of attribute table updates for a particular \a layer must change
-     *
-     * \since QGIS 3.4
-     */
-    void attributeTableUpdateBlocked( const QgsVectorLayer *layer, const bool blocked );
-
   private:
     void startProfile( const QString &name );
     void endProfile();

diff --git a/src/app/qgsattributetabledialog.cpp b/src/app/qgsattributetabledialog.cpp
@@ -113,16 +113,6 @@ QgsAttributeTableDialog::QgsAttributeTableDialog( QgsVectorLayer *layer, QgsAttr
   connect( mActionExpressionSelect, &QAction::triggered, this, &QgsAttributeTableDialog::mActionExpressionSelect_triggered );
   connect( mMainView, &QgsDualView::showContextMenuExternally, this, &QgsAttributeTableDialog::showContextMenu );
 
-  // Block/unblock table updates (feature cache signals)
-  connect( QgisApp::instance(), &QgisApp::attributeTableUpdateBlocked, this, [ = ]( const QgsVectorLayer * layer, const bool blocked )
-  {
-    if ( layer == mLayer )
-      this->blockCacheUpdateSignals( blocked );
-  } );
-  // Massive rollbacks can also freeze the GUI due to the feature cache signals
-  connect( mLayer, &QgsVectorLayer::beforeRollBack, this, [ = ] { this->blockCacheUpdateSignals( true ); } );
-  connect( mLayer, &QgsVectorLayer::afterRollBack, this, [ = ] { this->blockCacheUpdateSignals( false ); } );
-
   const QgsFields fields = mLayer->fields();
   for ( const QgsField &field : fields )
   {
@@ -743,6 +733,7 @@ void QgsAttributeTableDialog::mActionOpenFieldCalculator_triggered()
   if ( calc.exec() == QDialog::Accepted )
   {
     int col = masterModel->fieldCol( calc.changedAttributeId() );
+
     if ( col >= 0 )
     {
       masterModel->reload( masterModel->index( 0, col ), masterModel->index( masterModel->rowCount() - 1, col ) );
@@ -1154,15 +1145,6 @@ void QgsAttributeTableDialog::setFilterExpression( const QString &filterString,
   mMainView->setFilterMode( QgsAttributeTableFilterModel::ShowFilteredList );
 }
 
-void QgsAttributeTableDialog::blockCacheUpdateSignals( const bool block )
-{
-  QgsAttributeTableModel *masterModel = mMainView->masterModel();
-
-  if ( ! masterModel )
-    return;
-
-  masterModel->layerCache()->blockSignals( block );
-}
 
 void QgsAttributeTableDialog::deleteFeature( const QgsFeatureId fid )
 {

diff --git a/src/app/qgsattributetabledialog.h b/src/app/qgsattributetabledialog.h
@@ -242,7 +242,6 @@ class APP_EXPORT QgsAttributeTableDialog : public QDialog, private Ui::QgsAttrib
 
     void updateMultiEditButtonState();
     void deleteFeature( QgsFeatureId fid );
-    void blockCacheUpdateSignals( const bool block );
 
     friend class TestQgsAttributeTable;
 };

diff --git a/src/app/qgsfieldcalculator.cpp b/src/app/qgsfieldcalculator.cpp
@@ -159,10 +159,8 @@ void QgsFieldCalculator::accept()
 {
   builder->saveToRecent( QStringLiteral( "fieldcalc" ) );
 
-  if ( ! mVectorLayer )
-  {
+  if ( !mVectorLayer )
     return;
-  }
 
   // Set up QgsDistanceArea each time we (re-)calculate
   QgsDistanceArea myDa;
@@ -262,10 +260,6 @@ void QgsFieldCalculator::accept()
       return;
     }
 
-    // Begin feature modifications, block updates for attr tables
-    // connected to this layer
-    QgisApp::instance()->blockAttributeTableUpdates( mVectorLayer, true );
-
     //go through all the features and change the new attribute
     QgsFeature feature;
     bool calculationSuccess = true;
@@ -326,8 +320,6 @@ void QgsFieldCalculator::accept()
       rownum++;
     }
 
-    QgisApp::instance()->blockAttributeTableUpdates( mVectorLayer, false );
-
     if ( !calculationSuccess )
     {
       cursorOverride.release();
@@ -336,6 +328,7 @@ void QgsFieldCalculator::accept()
       mVectorLayer->destroyEditCommand();
       return;
     }
+
     mVectorLayer->endEditCommand();
   }
   QDialog::accept();

diff --git a/src/core/qgsapplication.cpp b/src/core/qgsapplication.cpp
@@ -1528,7 +1528,6 @@ void QgsApplication::collectTranslatableObjects( QgsTranslationContext *translat
   emit requestForTranslatableObjects( translationContext );
 }
 
-
 QString QgsApplication::nullRepresentation()
 {
   ApplicationMembers *appMembers = members();

diff --git a/src/core/qgsapplication.h b/src/core/qgsapplication.h
@@ -757,7 +757,6 @@ class CORE_EXPORT QgsApplication : public QApplication
      */
     void collectTranslatableObjects( QgsTranslationContext *translationContext );
 
-
 #ifdef SIP_RUN
     SIP_IF_FEATURE( ANDROID )
     //dummy method to workaround sip generation issue
@@ -789,7 +788,6 @@ class CORE_EXPORT QgsApplication : public QApplication
      */
     void requestForTranslatableObjects( QgsTranslationContext *translationContext );
 
-
   private:
 
     static void copyPath( const QString &src, const QString &dst );

diff --git a/src/gui/attributetable/qgsattributetablemodel.cpp b/src/gui/attributetable/qgsattributetablemodel.cpp
@@ -62,13 +62,19 @@ QgsAttributeTableModel::QgsAttributeTableModel( QgsVectorLayerCache *layerCache,
 
   loadAttributes();
 
-  connect( mLayerCache, &QgsVectorLayerCache::attributeValueChanged, this, &QgsAttributeTableModel::attributeValueChanged );
   connect( layer(), &QgsVectorLayer::featuresDeleted, this, &QgsAttributeTableModel::featuresDeleted );
   connect( layer(), &QgsVectorLayer::attributeDeleted, this, &QgsAttributeTableModel::attributeDeleted );
   connect( layer(), &QgsVectorLayer::updatedFields, this, &QgsAttributeTableModel::updatedFields );
+
+  connect( layer(), &QgsVectorLayer::editCommandStarted, this, &QgsAttributeTableModel::bulkEditCommandStarted );
+  connect( layer(), &QgsVectorLayer::beforeRollBack, this,  &QgsAttributeTableModel::bulkEditCommandStarted );
+  connect( layer(), &QgsVectorLayer::afterRollBack, this, &QgsAttributeTableModel::bulkEditCommandEnded );
+
   connect( layer(), &QgsVectorLayer::editCommandEnded, this, &QgsAttributeTableModel::editCommandEnded );
+  connect( mLayerCache, &QgsVectorLayerCache::attributeValueChanged, this, &QgsAttributeTableModel::attributeValueChanged );
   connect( mLayerCache, &QgsVectorLayerCache::featureAdded, this, [ = ]( QgsFeatureId id ) { featureAdded( id ); } );
   connect( mLayerCache, &QgsVectorLayerCache::cachedLayerDeleted, this, &QgsAttributeTableModel::layerDeleted );
+
 }
 
 bool QgsAttributeTableModel::loadFeatureAtId( QgsFeatureId fid ) const
@@ -255,7 +261,7 @@ void QgsAttributeTableModel::editCommandEnded()
 {
   // do not do reload(...) due would trigger (dataChanged) row sort
   // giving issue: https://issues.qgis.org/issues/15976
-  mChangedCellBounds = QRect();
+  bulkEditCommandEnded( );
 }
 
 void QgsAttributeTableModel::attributeDeleted( int idx )
@@ -294,7 +300,13 @@ void QgsAttributeTableModel::fieldFormatterRemoved( QgsFieldFormatter *fieldForm
 
 void QgsAttributeTableModel::attributeValueChanged( QgsFeatureId fid, int idx, const QVariant &value )
 {
-  QgsDebugMsgLevel( QStringLiteral( "(%4) fid: %1, idx: %2, value: %3" ).arg( fid ).arg( idx ).arg( value.toString() ).arg( mFeatureRequest.filterType() ), 3 );
+  // Defer all updates if a bulk edit/rollback command is running
+  if ( mBulkEditCommandRunning )
+  {
+    mAttributeValueChanges.insert( QPair<QgsFeatureId, int>( fid, idx ), value );
+    return;
+  }
+  QgsDebugMsgLevel( QStringLiteral( "(%4) fid: %1, idx: %2, value: %3" ).arg( fid ).arg( idx ).arg( value.toString() ).arg( mFeatureRequest.filterType() ), 2 );
 
   for ( SortCache &cache : mSortCaches )
   {
@@ -731,30 +743,6 @@ bool QgsAttributeTableModel::setData( const QModelIndex &index, const QVariant &
   if ( !layer()->isModified() )
     return false;
 
-  if ( mChangedCellBounds.isNull() )
-  {
-    mChangedCellBounds = QRect( index.column(), index.row(), 1, 1 );
-  }
-  else
-  {
-    if ( index.column() < mChangedCellBounds.left() )
-    {
-      mChangedCellBounds.setLeft( index.column() );
-    }
-    if ( index.row() < mChangedCellBounds.top() )
-    {
-      mChangedCellBounds.setTop( index.row() );
-    }
-    if ( index.column() > mChangedCellBounds.right() )
-    {
-      mChangedCellBounds.setRight( index.column() );
-    }
-    if ( index.row() > mChangedCellBounds.bottom() )
-    {
-      mChangedCellBounds.setBottom( index.row() );
-    }
-  }
-
   mRowStylesMap.remove( index.row() );
 
   return true;
@@ -797,6 +785,56 @@ bool QgsAttributeTableModel::fieldIsEditable( const QgsVectorLayer &layer, int f
            ( ( layer.dataProvider() && layer.dataProvider()->capabilities() & QgsVectorDataProvider::ChangeAttributeValues ) || FID_IS_NEW( fid ) ) );
 }
 
+void QgsAttributeTableModel::bulkEditCommandStarted()
+{
+  mBulkEditCommandRunning = true;
+  mAttributeValueChanges.clear();
+}
+
+void QgsAttributeTableModel::bulkEditCommandEnded()
+{
+  mBulkEditCommandRunning = false;
+  // Full model update if the changed rows are more than half the total rows
+  // or if their count is > layer cache size
+  int changeCount( mAttributeValueChanges.count() );
+  bool fullModelUpdate = changeCount > mLayerCache->cacheSize() ||
+                         changeCount > rowCount() * 0.5;
+
+  QgsDebugMsgLevel( QStringLiteral( "Bulk edit command ended with %1 modified rows over (%4), cache size is %2, starting %3 update." )
+                    .arg( changeCount )
+                    .arg( mLayerCache->cacheSize() )
+                    .arg( fullModelUpdate ? QStringLiteral( "full" ) :  QStringLiteral( "incremental" ) )
+                    .arg( rowCount() ),
+                    3 );
+  // Invalidates the whole model
+  if ( fullModelUpdate )
+  {
+    // Invalidates the cache (there is no API for doing this directly)
+    mLayerCache->layer()->dataChanged();
+    emit dataChanged( createIndex( 0, 0 ), createIndex( rowCount() - 1, columnCount() - 1 ) );
+  }
+  else
+  {
+    int minRow = rowCount();
+    int minCol = columnCount();
+    int maxRow = 0;
+    int maxCol = 0;
+    const auto keys = mAttributeValueChanges.keys();
+    for ( const auto &key : keys )
+    {
+      attributeValueChanged( key.first, key.second, mAttributeValueChanges.value( key ) );
+      int row( idToRow( key.first ) );
+      int col( fieldCol( key.second ) );
+      minRow = std::min<int>( row, minRow );
+      minCol = std::min<int>( col, minCol );
+      maxRow = std::max<int>( row, maxRow );
+      maxCol = std::max<int>( col, maxCol );
+    }
+    emit dataChanged( createIndex( minRow, minCol ), createIndex( maxRow, maxCol ) );
+  }
+  mAttributeValueChanges.clear();
+}
+
 void QgsAttributeTableModel::reload( const QModelIndex &index1, const QModelIndex &index2 )
 {
   mFeat.setId( std::numeric_limits<int>::min() );

diff --git a/src/gui/attributetable/qgsattributetablemodel.h b/src/gui/attributetable/qgsattributetablemodel.h
@@ -309,7 +309,7 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
     virtual void attributeValueChanged( QgsFeatureId fid, int idx, const QVariant &value );
 
     /**
-     * Launched when eatures have been deleted
+     * Launched when features have been deleted
      * \param fids feature ids
      */
     virtual void featuresDeleted( const QgsFeatureIds &fids );
@@ -378,19 +378,22 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
 
     std::vector<SortCache> mSortCaches;
 
-    /**
-     * Holds the bounds of changed cells while an update operation is running
-     * top    = min row
-     * left   = min column
-     * bottom = max row
-     * right  = max column
-     */
-    QRect mChangedCellBounds;
-
     QgsAttributeEditorContext mEditorContext;
 
     int mExtraColumns = 0;
 
+    //! Flag for massive changes operations, set by edit command or rollback
+    bool mBulkEditCommandRunning = false;
+
+    //! Sets the flag for massive changes operations
+    void bulkEditCommandStarted();
+
+    //! Clears the flag for massive changes operations, updates/rebuilds the layer cache and tells the view to update
+    void bulkEditCommandEnded();
+
+    //! Changed attribute values within a bulk edit command
+    QMap<QPair<QgsFeatureId, int>, QVariant> mAttributeValueChanges;
+
     friend class TestQgsAttributeTable;
 
 };