qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.43k stars 2.99k forks source link

QgsVectorLayer / QgsVectorLayerBuffer does not track #46734

Open jakimowb opened 2 years ago

jakimowb commented 2 years ago

What is the bug or the crash?

Background

The QgsVectorLayerEditBuffer stores changes of features, attributes and geometries until they are committed to the vector data provider. In a working session it might happen that attribute values are updated several times, for example because repeated runs with the field calculator.

Single changes of attributes can be tracked with the attributeValueChanged( QgsFeatureId fid, int idx, const QVariant &value ) signal. In a GUI context we want to reduce overhead and use a signal that is emitted after an edit block has been ended (and thousands of values been updated), similar to the QgsVectorLayer::featuresDeleted(fids) signal.

In such a case it is expected that QgsVectorLayerEditBuffer::changedAttributeValues(QgsChangedAttributesMap) contains all updates and can be used to update affected GUI widgets (unfortunately there is no attributeValuesChanged(QgsChangedAttributesMap) signal).

Problem / Bug?

However, the returned QgsChangedAttributesMap contains only attribute updates for features, which have been committed to data provider before. Changes on new features (negative FID) are missing (see python code example below).

As far as I understand the QGIS code this problem originates in the QgsVectorLayerUndoCommandChangeAttribute. As long as FID_IS_NEW( mFid ) is true, updated values will never be written to mChangedAttributeValues (see C++ code below).

A solution might be to

a) always update the buffer's mChangedAttributeValues, no matter if the feature was new or not, or b) introduce a 2nd QgsChangedAttributesMap just for new features.

QgsVectorLayerUndoCommandChangeAttribute::QgsVectorLayerUndoCommandChangeAttribute( QgsVectorLayerEditBuffer *buffer, QgsFeatureId fid, int fieldIndex, const QVariant &newValue, const QVariant &oldValue )
  : QgsVectorLayerUndoCommand( buffer )
  , mFid( fid )
  , mFieldIndex( fieldIndex )
  , mOldValue( oldValue )
  , mNewValue( newValue )
  , mFirstChange( true )
{
  if ( FID_IS_NEW( mFid ) )
  {
    // work with added feature
    const QgsFeatureMap::const_iterator it = mBuffer->mAddedFeatures.constFind( mFid );
    Q_ASSERT( it != mBuffer->mAddedFeatures.constEnd() );
    if ( it.value().attribute( mFieldIndex ).isValid() )
    {
      mOldValue = it.value().attribute( mFieldIndex );
      mFirstChange = false;
    }
  }
  else if ( mBuffer->mChangedAttributeValues.contains( mFid ) && mBuffer->mChangedAttributeValues[mFid].contains( mFieldIndex ) )
  {
    mOldValue = mBuffer->mChangedAttributeValues[mFid][mFieldIndex];
    mFirstChange = false;
  }

}

Steps to reproduce the issue

This python example creates two feature and changes it's attributes. If not committed first, updates on feature attributes will not be contained in layer.editBuffer().changedAttributeValues():

Features not committed
attribute changed: fid=-2, field=0, new value=A updated
attribute changed: fid=-3, field=0, new value=B updated
buffered changes: {}

If committed first, the output will be:

attribute changed: fid=1, field=0, new value=A updated
attribute changed: fid=2, field=0, new value=B updated
buffered changes: {1: {0: 'A updated'}, 2: {0: 'B updated'}}

In both case I'd expect that the buffered changes are the same.


uri = 'Point?crs=epsg:4326&field=name:string(20)'
layer = QgsVectorLayer(uri, 'Layer', 'memory')
layer.startEditing()

for name in ['A', 'B']:
    f = QgsFeature(layer.fields())
    f.setAttribute('name', name)
    layer.addFeature(f)

# toggle to see the difference
COMMIT_FEATURES_FIRST = False

if COMMIT_FEATURES_FIRST:
    layer.commitChanges(False)
    print('New feature committed')
else:
    print('New feature not committed')

def onAttributeValueChanged(fid, i, newValue):
    print(f'attribute changed: fid={fid}, field={i}, new value={newValue}')

def onEditCommandEnded(*args):
    print(f'buffered changes: {layer.editBuffer().changedAttributeValues()}')

layer.attributeValueChanged.connect(onAttributeValueChanged)
layer.editCommandEnded.connect(onEditCommandEnded)

layer.beginEditCommand('Update attribute value')
i = layer.fields().lookupField('name')
for f in layer.getFeatures():
    old_value = f.attribute('name')
    layer.changeAttributeValue(f.id(), i, f'{old_value} updated')
layer.endEditCommand()

Versions

QGIS version 3.23.0-Master QGIS code revision 307bb854b1

Supported QGIS version

New profile

Additional context

No response

roya0045 commented 2 years ago

I think there is upcoming work on that but I'm not sure if this is related to the recent QEP.

jakimowb commented 2 years ago

Which QEP do you mean?

Reading more into the code I discovered that, for example, qgsattributetablemodel.cpp uses it's own QMap<QPair<QgsFeatureId, int>, QVariant> mAttributeValueChanges; to track attribute value updates that are done during an edit command.

Tracking them in the QgsVectorLayerEditBuffer instead might avoid some overhead in case one vector layer is connected to multiple attribute tables.

roya0045 commented 2 years ago

See #46750 which may be relevant.

couteau commented 2 years ago

@jakimowb are the addedFeatures and/or allAddedOrEditedFeatures methods of QgsVectorLayerEditBuffer not sufficient for your needs?

jakimowb commented 2 years ago

Unfortunatelly not. I want to update GUI objects. This might need some time, so I want to avoid unnecessary loading operations.

Think about a series of being/endEdit commands:

beginEditCommand("edits feature 1 to 1000")
...
endEditCommand()

Now I want to update 1000 plot objects. Note that the modifications have not been commited to the data provider.

beginEditCommand("edit feature 1001 to 2000")
...
endEditCommand()

Now I want to apply updates related to the last 1000 modified features. However, "addedFeatures" would return 2000 features.

alexbruy commented 11 months ago

IMHO this is not a bug. If feature is not commited yet, it is a new feature and it should not be included into changed list just because it does not exists yet and we can't find out previous attribute value.

jakimowb commented 11 months ago

The docs say: void QgsVectorLayer::attributeValueChanged: Emitted whenever an attribute value change is done in the edit buffer. Note that at this point the attribute change is not yet saved to the provider.

Which is fine, as it allows to react all changes which are already visible to the user (e.g. in the attribute table), but not commited yet, as they may be rejected and not commited.

In contrast, void QgsVectorLayer::committedAttributeValuesChanges is emitted when attribute value changes are saved to the provider if not in transaction mode.