[QGIS-Developer] QGIS/OGR: FeatureIds reassigned on write to data provider?

[Even Rouault]

On mercredi 20 septembre 2017 17:32:27 CEST Sandro Mani wrote:
> On 19.09.2017 19:15, Even Rouault wrote:
> > On mardi 19 septembre 2017 18:30:50 CEST Sandro Mani wrote:
> > > On 11.07.2017 14:31, Even Rouault wrote:
> > > > We could potentially imagine to defer the repacking at
> > > > 
> > > > leaveUpdateMode() time, actually in the close() method which is called
> > > > 
> > > > when the reference counter of the update mode drops to zero (*),
> > > > 
> > > > instead of doing it at the end of each operation.
> > > 
> > > To come back to this, I was looking at moving replack() to close(), but
> > > 
> > > noticed that syncToDisc() has the logic "for shapefiles, remove spatial
> > > 
> > > index files and create a new index" which calls close. And syncToDisk is
> > > 
> > > still called after every change. So should syncToDisk be also moved to
> > > 
> > > close(), or at least blocked if an updateMode is active?
> > 
> > Yes that would probably make sense to block syncToDisk() while in
> > updateMode (so when mUpdateModeStackDepth > 0), but probably only if
> > enterUpdateMode() is called by QgsVectorLayer::startEditing(), and not
> > when called implicitly by doInitialActionsForEdition(), which might be
> > the case if someone directly works at the provider level
> > 
> > bool QgsOgrProvider::doInitialActionsForEdition()
> > 
> > {
> > 
> > if ( !mValid )
> > 
> > return false;
> > 
> > if ( !mWriteAccess && mWriteAccessPossible && mDynamicWriteAccess )
> > 
> > {
> > 
> > QgsDebugMsg( "Enter update mode implictly" );
> > 
> > if ( !enterUpdateMode() )
> > 
> > return false;
> > 
> > }
> > 
> > return true;
> > 
> > }
> > 
> > So probably a mImplicitUpdateMode flag should be set above near the
> > QgsDebugMsg(), and the check to decide whether we can defer
> > syncToDisk() would be ( !mImplicitUpdateMode && mUpdateModeStackDepth > 0)
> 
> Hm but will that flag never be cleared again once set? Since AFAICT a
> there is no leaveUpdateMode matching the enterUpdateMode called by
> doInitialActionsForEdition.
> 
> As I understand, the approach to do changes with stable ids would be to
> first enterUpdateMode, then perform the changes, then leaveUpdateMode,
> at which point the changes are synced and the dataset repacked.

Yes

> With the
> mImplicitUpdateMode flag, if anyone worked directly at provider level
> before you come along, deferring syncToDisk would never happen since the
> mImplicitUpdateMode was set and remains set.

Yes my idea was to keep the current behaviour where repack() is done frequently, as you 
cannot know when the dataset should be in a consistant state.
But we could also decide to defer syncToDisk() at provider deletion (would make the code 
simpler)
I have no idea of the existing use cases behind and if that would break some.

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/qgis-developer/attachments/20170920/472d8c0b/attachment-0001.html>