Repack shapefiles when saving after deleting features

 * QgsVectorDataProvider::dataChanged() will be emitted
 * QgsVectorLayer::dataChanged() will be emitted
 * Clears QgsVectorLayerCache
 * Reloads the attribute table
 * Clears the selection

Looking forward to people complaining about their lost selection...

Fix #10560
Fix #11989
Refs #8317
Refs #8822
Refs #10483
Refs #11007
Refs #7540
Refs #11398
Refs #11296

Author: m-kuhn

src/core/qgsdataprovider.h

@@ -308,6 +308,9 @@ class CORE_EXPORT QgsDataProvider : public QObject
     /**
      *   This is emitted whenever an asynchronous operation has finished
      *   and the data should be redrawn
+     *
+     *   When emitted from a QgsVectorDataProvider, any cached information such as
+     *   feature ids should be invalidated.
      */
     void dataChanged();
 

src/core/qgsvectorlayer.cpp

@@ -1366,6 +1366,7 @@ bool QgsVectorLayer::setDataProvider( QString const & provider )
 {
   // XXX should I check for and possibly delete any pre-existing providers?
   // XXX How often will that scenario occur?
+  Q_ASSERT( !mDataProvider );
 
   mProviderKey = provider;     // XXX is this necessary?  Usually already set
   // XXX when execution gets here.
@@ -1457,6 +1458,8 @@ bool QgsVectorLayer::setDataProvider( QString const & provider )
     return false;
   }
 
+  connect( mDataProvider, SIGNAL( dataChanged() ), this, SIGNAL( dataChanged() ) );
+  connect( mDataProvider, SIGNAL( dataChanged() ), this, SLOT( removeSelection() ) );
   return true;
 
 } // QgsVectorLayer:: setDataProvider

src/core/qgsvectorlayer.h

@@ -1753,7 +1753,6 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer
      */
     void writeCustomSymbology( QDomElement& element, QDomDocument& doc, QString& errorMessage ) const;
 
-
   private slots:
     void onRelationsLoaded();
     void onJoinedFieldsChanged();

src/core/qgsvectorlayercache.cpp

@@ -35,7 +35,8 @@ QgsVectorLayerCache::QgsVectorLayerCache( QgsVectorLayer* layer, int cacheSize,
   setCacheAddedAttributes( true );
 
   connect( mLayer, SIGNAL( attributeDeleted( int ) ), SLOT( attributeDeleted( int ) ) );
-  connect( mLayer, SIGNAL( updatedFields() ), SLOT( updatedFields() ) );
+  connect( mLayer, SIGNAL( updatedFields() ), SLOT( invalidate() ) );
+  connect( mLayer, SIGNAL( dataChanged() ), SLOT( invalidate() ) );
   connect( mLayer, SIGNAL( attributeValueChanged( QgsFeatureId, int, const QVariant& ) ), SLOT( onAttributeValueChanged( QgsFeatureId, int, const QVariant& ) ) );
 }
 
@@ -231,9 +232,15 @@ void QgsVectorLayerCache::attributeAdded( int field )
 
 void QgsVectorLayerCache::attributeDeleted( int field )
 {
-  foreach ( QgsFeatureId fid, mCache.keys() )
+  QgsAttributeList attrs = mCachedAttributes;
+  mCachedAttributes.clear();
+
+  Q_FOREACH ( int attr, attrs )
   {
-    mCache[ fid ]->mFeature->deleteAttribute( field );
+    if ( attr < field )
+      mCachedAttributes << attr;
+    else if ( attr > field )
+      mCachedAttributes << attr - 1;
   }
 }
 
@@ -253,9 +260,10 @@ void QgsVectorLayerCache::layerDeleted()
   mLayer = NULL;
 }
 
-void QgsVectorLayerCache::updatedFields()
+void QgsVectorLayerCache::invalidate()
 {
   mCache.clear();
+  emit invalidated();
 }
 
 QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &featureRequest )

src/core/qgsvectorlayercache.h

@@ -256,6 +256,11 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
      */
     void featureAdded( QgsFeatureId fid );
 
+    /**
+     * The cache has been invalidated and cleared.
+     */
+    void invalidated();
+
   private slots:
     void onAttributeValueChanged( QgsFeatureId fid, int field, const QVariant& value );
     void featureDeleted( QgsFeatureId fid );
@@ -264,7 +269,7 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject
     void attributeDeleted( int field );
     void geometryChanged( QgsFeatureId fid, QgsGeometry& geom );
     void layerDeleted();
-    void updatedFields();
+    void invalidate();
 
   private:
 

src/core/raster/qgsrasterlayer.h

@@ -375,11 +375,6 @@ class CORE_EXPORT QgsRasterLayer : public QgsMapLayer
     /** \brief Signal for notifying listeners of long running processes */
     void progressUpdate( int theValue );
 
-    /**
-     *   This is emitted whenever data or metadata (e.g. color table, extent) has changed
-     */
-    void dataChanged();
-
   protected:
     /** \brief Read the symbology for the current layer from the Dom node supplied */
     bool readSymbology( const QDomNode& node, QString& errorMessage ) override;

src/gui/attributetable/qgsattributetablemodel.cpp

@@ -369,6 +369,8 @@ void QgsAttributeTableModel::loadLayer()
 
   emit finished();
 
+  connect( mLayerCache, SIGNAL( invalidated() ), this, SLOT( loadLayer() ) );
+
   mFieldCount = mAttributes.size();
 }
 

src/gui/attributetable/qgsattributetablemodel.h

@@ -62,12 +62,6 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
      */
     QgsAttributeTableModel( QgsVectorLayerCache *layerCache, QObject *parent = 0 );
 
-    /**
-     * Loads the layer into the model
-     * Preferably to be called, before basing any other models on this model
-     */
-    virtual void loadLayer();
-
     /**
      * Returns the number of rows
      * @param parent parent index
@@ -224,6 +218,13 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel
      */
     const QgsAttributeEditorContext& editorContext() const { return mEditorContext; }
 
+  public slots:
+    /**
+     * Loads the layer into the model
+     * Preferably to be called, before using this model as source for any other proxy model
+     */
+    virtual void loadLayer();
+
   signals:
     /**
      * Model has been changed

src/providers/ogr/qgsogrprovider.cpp

@@ -132,52 +132,53 @@ void QgsOgrProvider::repack()
   QByteArray layerName = OGR_FD_GetName( OGR_L_GetLayerDefn( ogrOrigLayer ) );
 
   // run REPACK on shape files
-  if ( mDataModified )
-  {
-    QByteArray sql = QByteArray( "REPACK " ) + layerName;   // don't quote the layer name as it works with spaces in the name and won't work if the name is quoted
-    QgsDebugMsg( QString( "SQL: %1" ).arg( FROM8( sql ) ) );
-    OGR_DS_ExecuteSQL( ogrDataSource, sql.constData(), NULL, NULL );
+  QByteArray sql = QByteArray( "REPACK " ) + layerName;   // don't quote the layer name as it works with spaces in the name and won't work if the name is quoted
+  QgsDebugMsg( QString( "SQL: %1" ).arg( FROM8( sql ) ) );
+  OGR_DS_ExecuteSQL( ogrDataSource, sql.constData(), NULL, NULL );
 
-    if ( mFilePath.endsWith( ".shp", Qt::CaseInsensitive ) || mFilePath.endsWith( ".dbf", Qt::CaseInsensitive ) )
+  if ( mFilePath.endsWith( ".shp", Qt::CaseInsensitive ) || mFilePath.endsWith( ".dbf", Qt::CaseInsensitive ) )
+  {
+    QString packedDbf( mFilePath.left( mFilePath.size() - 4 ) + "_packed.dbf" );
+    if ( QFile::exists( packedDbf ) )
     {
-      QString packedDbf( mFilePath.left( mFilePath.size() - 4 ) + "_packed.dbf" );
-      if ( QFile::exists( packedDbf ) )
-      {
-        QgsMessageLog::logMessage( tr( "Possible corruption after REPACK detected. %1 still exists. This may point to a permission or locking problem of the original DBF." ).arg( packedDbf ), tr( "OGR" ), QgsMessageLog::CRITICAL );
+      QgsMessageLog::logMessage( tr( "Possible corruption after REPACK detected. %1 still exists. This may point to a permission or locking problem of the original DBF." ).arg( packedDbf ), tr( "OGR" ), QgsMessageLog::CRITICAL );
 
-        OGR_DS_Destroy( ogrDataSource );
-        ogrLayer = ogrOrigLayer = 0;
+      OGR_DS_Destroy( ogrDataSource );
+      ogrLayer = ogrOrigLayer = 0;
 
-        ogrDataSource = OGROpen( TO8F( mFilePath ), true, NULL );
-        if ( ogrDataSource )
+      ogrDataSource = OGROpen( TO8F( mFilePath ), true, NULL );
+      if ( ogrDataSource )
+      {
+        if ( mLayerName.isNull() )
         {
-          if ( mLayerName.isNull() )
-          {
-            ogrOrigLayer = OGR_DS_GetLayer( ogrDataSource, mLayerIndex );
-          }
-          else
-          {
-            ogrOrigLayer = OGR_DS_GetLayerByName( ogrDataSource, TO8( mLayerName ) );
-          }
-
-          if ( !ogrOrigLayer )
-          {
-            QgsMessageLog::logMessage( tr( "Original layer could not be reopened." ), tr( "OGR" ), QgsMessageLog::CRITICAL );
-            valid = false;
-          }
-
-          ogrLayer = ogrOrigLayer;
+          ogrOrigLayer = OGR_DS_GetLayer( ogrDataSource, mLayerIndex );
         }
         else
         {
-          QgsMessageLog::logMessage( tr( "Original datasource could not be reopened." ), tr( "OGR" ), QgsMessageLog::CRITICAL );
-          valid = false;
+          ogrOrigLayer = OGR_DS_GetLayerByName( ogrDataSource, TO8( mLayerName ) );
         }
+
+        if ( !ogrOrigLayer )
+        {
+          QgsMessageLog::logMessage( tr( "Original layer could not be reopened." ), tr( "OGR" ), QgsMessageLog::CRITICAL );
+          mValid = false;
+        }
+
+        ogrLayer = ogrOrigLayer;
+      }
+      else
+      {
+        QgsMessageLog::logMessage( tr( "Original datasource could not be reopened." ), tr( "OGR" ), QgsMessageLog::CRITICAL );
+        mValid = false;
       }
     }
 
-    mDataModified = false;
   }
+
+  long oldcount = mFeaturesCounted;
+  recalculateFeatureCount();
+  if ( oldcount != mFeaturesCounted )
+    emit dataChanged();
 }
 
 
@@ -265,14 +266,12 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
     , mIsSubLayer( false )
     , mOgrGeometryTypeFilter( wkbUnknown )
     , ogrDriver( 0 )
-    , valid( false )
+    , mValid( false )
     , geomType( wkbUnknown )
-    , featuresCounted( -1 )
-    , mDataModified( false )
+    , mFeaturesCounted( -1 )
     , mWriteAccess( false )
+    , mShapefileMayBeCorrupted( false )
 {
-  QgsCPLErrorHandler handler;
-
   QgsApplication::registerOgrDrivers();
 
   QSettings settings;
@@ -414,8 +413,8 @@ QgsOgrProvider::QgsOgrProvider( QString const & uri )
       // check that the initial encoding setting is fit for this layer
       setEncoding( encoding() );
 
-      valid = setSubsetString( mSubsetString );
-      if ( valid )
+      mValid = setSubsetString( mSubsetString );
+      if ( mValid )
       {
         QgsDebugMsg( "Data source is valid" );
       }
@@ -480,7 +479,7 @@ bool QgsOgrProvider::setSubsetString( QString theSQL, bool updateFeatureCount )
 {
   QgsCPLErrorHandler handler;
 
-  if ( theSQL == mSubsetString && featuresCounted >= 0 )
+  if ( theSQL == mSubsetString && mFeaturesCounted >= 0 )
     return true;
 
   OGRLayerH prevLayer = ogrLayer;
@@ -609,7 +608,7 @@ OGRwkbGeometryType QgsOgrProvider::ogrWkbGeometryTypeFromName( QString typeName
 QStringList QgsOgrProvider::subLayers() const
 {
   QgsDebugMsg( "Entered." );
-  if ( !valid )
+  if ( !mValid )
   {
     return QStringList();
   }
@@ -954,7 +953,7 @@ QGis::WkbType QgsOgrProvider::geometryType() const
  */
 long QgsOgrProvider::featureCount() const
 {
-  return featuresCounted;
+  return mFeaturesCounted;
 }
 
 
@@ -969,7 +968,7 @@ const QgsFields & QgsOgrProvider::fields() const
 //       actually has features
 bool QgsOgrProvider::isValid()
 {
-  return valid;
+  return mValid;
 }
 
 bool QgsOgrProvider::addFeature( QgsFeature& f )
@@ -1347,6 +1346,7 @@ bool QgsOgrProvider::changeGeometryValues( QgsGeometryMap & geometry_map )
       theNewGeometry = 0;
       continue;
     }
+    mShapefileMayBeCorrupted = true;
 
     OGR_F_Destroy( theOGRFeature );
   }
@@ -1389,8 +1389,6 @@ bool QgsOgrProvider::createAttributeIndex( int field )
 
 bool QgsOgrProvider::deleteFeatures( const QgsFeatureIds & id )
 {
-  QgsCPLErrorHandler handler;
-
   bool returnvalue = true;
   for ( QgsFeatureIds::const_iterator it = id.begin(); it != id.end(); ++it )
   {
@@ -1432,6 +1430,8 @@ bool QgsOgrProvider::deleteFeature( QgsFeatureId id )
     return false;
   }
 
+  mShapefileMayBeCorrupted = true;
+
   return true;
 }
 
@@ -2457,6 +2457,11 @@ bool QgsOgrProvider::syncToDisc()
     pushError( tr( "OGR error syncing to disk: %1" ).arg( CPLGetLastErrorMsg() ) );
   }
 
+  if ( mShapefileMayBeCorrupted )
+    repack();
+
+  mShapefileMayBeCorrupted = false;
+
   //for shapefiles: is there already a spatial index?
   if ( !mFilePath.isEmpty() )
   {
@@ -2478,8 +2483,6 @@ bool QgsOgrProvider::syncToDisc()
     }
   }
 
-  mDataModified = true;
-
   return true;
 }
 
@@ -2496,11 +2499,11 @@ void QgsOgrProvider::recalculateFeatureCount()
   // so we remove it if there's any and then put it back
   if ( mOgrGeometryTypeFilter == wkbUnknown )
   {
-    featuresCounted = OGR_L_GetFeatureCount( ogrLayer, true );
+    mFeaturesCounted = OGR_L_GetFeatureCount( ogrLayer, true );
   }
   else
   {
-    featuresCounted = 0;
+    mFeaturesCounted = 0;
     OGR_L_ResetReading( ogrLayer );
     setRelevantFields( ogrLayer, true, QgsAttributeList() );
     OGR_L_ResetReading( ogrLayer );
@@ -2511,7 +2514,7 @@ void QgsOgrProvider::recalculateFeatureCount()
       if ( geom )
       {
         OGRwkbGeometryType gType = OGR_G_GetGeometryType( geom );
-        if ( gType == mOgrGeometryTypeFilter ) featuresCounted++;
+        if ( gType == mOgrGeometryTypeFilter ) mFeaturesCounted++;
       }
       OGR_F_Destroy( fet );
     }

src/providers/ogr/qgsogrprovider.h

@@ -322,13 +322,10 @@ class QgsOgrProvider : public QgsVectorDataProvider
     // Friendly name of the OGR Driver that was actually used to open the layer
     QString ogrDriverName;
 
-    bool valid;
+    bool mValid;
 
     OGRwkbGeometryType geomType;
-    long featuresCounted;
-
-    //! Data has been modified - REPACK before creating a spatialindex
-    bool mDataModified;
+    long mFeaturesCounted;
 
     mutable QStringList mSubLayerList;
 
@@ -346,6 +343,8 @@ class QgsOgrProvider : public QgsVectorDataProvider
 
     /** whether the file is opened in write mode*/
     bool mWriteAccess;
+
+    bool mShapefileMayBeCorrupted;
 };