Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Don't loop through all selected features multiple times (once per
field) when the attribute form is opened.

This is incredibly expensive, yet only required in a very very small
corner case (field is from a joined layer without the upsert on edit
capabilities).

Refine logic to avoid the scan wherever we can.

Fixes #41366
Fixes #36863

(cherry picked from commit c661359)
  • Loading branch information
nyalldawson committed Feb 19, 2021
1 parent e431192 commit f88729c
Show file tree
Hide file tree
Showing 5 changed files with 266 additions and 24 deletions.
28 changes: 26 additions & 2 deletions python/core/auto_generated/qgsvectorlayerutils.sip.in
Expand Up @@ -288,13 +288,37 @@ Optionally, ``sinkFlags`` can be specified to further refine the compatibility l

static bool fieldIsEditable( const QgsVectorLayer *layer, int fieldIndex, const QgsFeature &feature );
%Docstring
Tests whether a field is editable for a particular ``feature``.

:return: ``True`` if the:param feature: field at index:param fieldIndex: from:param layer:
is editable, ``False`` if the field is readonly
:return: ``True`` if the field at index ``fieldIndex`` from ``layer``
is editable, ``False`` if the field is read only.

.. versionadded:: 3.10
%End

static bool fieldIsReadOnly( const QgsVectorLayer *layer, int fieldIndex );
%Docstring

:return: ``True`` if the field at index ``fieldIndex`` from ``layer``
is editable, ``False`` if the field is read only.

If this function returns ``True`` then the editability of the field may still vary feature by
feature. See :py:func:`~QgsVectorLayerUtils.fieldIsEditable` to determine this on a feature by feature basis.

.. versionadded:: 3.18
%End

static bool fieldEditabilityDependsOnFeature( const QgsVectorLayer *layer, int fieldIndex );
%Docstring
Returns ``True`` if the editability of the field at index ``fieldIndex`` from ``layer`` may vary
feature by feature.

I.e. if the field is taken from a joined layer, the value may or may not be editable for any individual
feature depending on the join's "upsert on edit" capabilities.

.. versionadded:: 3.18
%End



static QString getFeatureDisplayString( const QgsVectorLayer *layer, const QgsFeature &feature );
Expand Down
48 changes: 48 additions & 0 deletions src/core/qgsvectorlayerutils.cpp
Expand Up @@ -866,6 +866,53 @@ bool _fieldIsEditable( const QgsVectorLayer *layer, int fieldIndex, const QgsFea
( ( layer->dataProvider() && layer->dataProvider()->capabilities() & QgsVectorDataProvider::ChangeAttributeValues ) || FID_IS_NEW( feature.id() ) );
}

bool QgsVectorLayerUtils::fieldIsReadOnly( const QgsVectorLayer *layer, int fieldIndex )
{
if ( layer->fields().fieldOrigin( fieldIndex ) == QgsFields::OriginJoin )
{
int srcFieldIndex;
const QgsVectorLayerJoinInfo *info = layer->joinBuffer()->joinForFieldIndex( fieldIndex, layer->fields(), srcFieldIndex );

if ( !info || !info->isEditable() || !info->joinLayer() )
return true;

return fieldIsReadOnly( info->joinLayer(), srcFieldIndex );
}
else
{
// any of these properties makes the field read only
if ( !layer->isEditable() ||
layer->editFormConfig().readOnly( fieldIndex ) ||
!layer->dataProvider() ||
( !( layer->dataProvider()->capabilities() & QgsVectorDataProvider::ChangeAttributeValues )
&& !( layer->dataProvider()->capabilities() & QgsVectorDataProvider::AddFeatures ) ) )
return true;

return false;
}
}

bool QgsVectorLayerUtils::fieldEditabilityDependsOnFeature( const QgsVectorLayer *layer, int fieldIndex )
{
// editability will vary feature-by-feature only for joined fields
if ( layer->fields().fieldOrigin( fieldIndex ) == QgsFields::OriginJoin )
{
int srcFieldIndex;
const QgsVectorLayerJoinInfo *info = layer->joinBuffer()->joinForFieldIndex( fieldIndex, layer->fields(), srcFieldIndex );

if ( !info || !info->isEditable() || info->hasUpsertOnEdit() )
return false;

// join does not have upsert capabilities, so the ability to edit the joined field will
// vary feature-by-feature, depending on whether the join target feature already exists
return true;
}
else
{
return false;
}
}

bool QgsVectorLayerUtils::fieldIsEditable( const QgsVectorLayer *layer, int fieldIndex, const QgsFeature &feature )
{
if ( layer->fields().fieldOrigin( fieldIndex ) == QgsFields::OriginJoin )
Expand All @@ -890,6 +937,7 @@ bool QgsVectorLayerUtils::fieldIsEditable( const QgsVectorLayer *layer, int fiel
return _fieldIsEditable( layer, fieldIndex, feature );
}


QHash<QString, QHash<QString, QSet<QgsSymbolLayerId>>> QgsVectorLayerUtils::labelMasks( const QgsVectorLayer *layer )
{
class LabelMasksVisitor : public QgsStyleEntityVisitorInterface
Expand Down
28 changes: 26 additions & 2 deletions src/core/qgsvectorlayerutils.h
Expand Up @@ -298,13 +298,37 @@ class CORE_EXPORT QgsVectorLayerUtils
static QgsFeatureList makeFeaturesCompatible( const QgsFeatureList &features, const QgsVectorLayer *layer, QgsFeatureSink::SinkFlags sinkFlags = QgsFeatureSink::SinkFlags() );

/**
* \return TRUE if the \param feature field at index \param fieldIndex from \param layer
* is editable, FALSE if the field is readonly
* Tests whether a field is editable for a particular \a feature.
*
* \returns TRUE if the field at index \a fieldIndex from \a layer
* is editable, FALSE if the field is read only.
*
* \since QGIS 3.10
*/
static bool fieldIsEditable( const QgsVectorLayer *layer, int fieldIndex, const QgsFeature &feature );

/**
* \returns TRUE if the field at index \a fieldIndex from \a layer
* is editable, FALSE if the field is read only.
*
* If this function returns TRUE then the editability of the field may still vary feature by
* feature. See fieldIsEditable() to determine this on a feature by feature basis.
*
* \since QGIS 3.18
*/
static bool fieldIsReadOnly( const QgsVectorLayer *layer, int fieldIndex );

/**
* Returns TRUE if the editability of the field at index \a fieldIndex from \a layer may vary
* feature by feature.
*
* I.e. if the field is taken from a joined layer, the value may or may not be editable for any individual
* feature depending on the join's "upsert on edit" capabilities.
*
* \since QGIS 3.18
*/
static bool fieldEditabilityDependsOnFeature( const QgsVectorLayer *layer, int fieldIndex );

/**
* Returns masks defined in labeling options of a layer.
* The returned type associates a labeling rule identifier to a set of layers that are masked given by their layer id,
Expand Down
67 changes: 47 additions & 20 deletions src/gui/qgsattributeformeditorwidget.cpp
Expand Up @@ -230,30 +230,59 @@ void QgsAttributeFormEditorWidget::updateWidgets()
//first update the tool buttons
bool hasMultiEditButton = ( editPage()->layout()->indexOf( mMultiEditButton ) >= 0 );

const int fieldIndex = mEditorWidget->fieldIdx();

bool fieldReadOnly = false;
QgsFeature feature;
auto it = layer()->getSelectedFeatures();
while ( it.nextFeature( feature ) )
bool shouldShowMultiEditButton = false;
switch ( mode() )
{
fieldReadOnly |= !QgsVectorLayerUtils::fieldIsEditable( layer(), fieldIndex, feature );
}
case QgsAttributeFormWidget::DefaultMode:
case QgsAttributeFormWidget::SearchMode:
case QgsAttributeFormWidget::AggregateSearchMode:
// in these modes we don't show the multi edit button
shouldShowMultiEditButton = false;
break;

if ( hasMultiEditButton )
{
if ( mode() != MultiEditMode || fieldReadOnly )
case QgsAttributeFormWidget::MultiEditMode:
{
editPage()->layout()->removeWidget( mMultiEditButton );
mMultiEditButton->setParent( nullptr );
// in multi-edit mode we need to know upfront whether or not to allow add the multiedit buttons
// for this field.
// if the field is always read only regardless of the feature, no need to dig further. But otherwise
// we may need to test editability for the actual selected features...
const int fieldIndex = mEditorWidget->fieldIdx();
shouldShowMultiEditButton = !QgsVectorLayerUtils::fieldIsReadOnly( layer(), fieldIndex );
if ( shouldShowMultiEditButton )
{
// depending on the field type, the editability of the field may vary feature by feature (e.g. for joined
// fields coming from joins without the upsert on edit capabilities).
// But this feature-by-feature check is EXPENSIVE!!! (see https://github.com/qgis/QGIS/issues/41366), so
// avoid it whenever we can...
const bool fieldEditabilityDependsOnFeature = QgsVectorLayerUtils::fieldEditabilityDependsOnFeature( layer(), fieldIndex );
if ( fieldEditabilityDependsOnFeature )
{
QgsFeature feature;
QgsFeatureIterator it = layer()->getSelectedFeatures();
while ( it.nextFeature( feature ) )
{
const bool isEditable = QgsVectorLayerUtils::fieldIsEditable( layer(), fieldIndex, feature );
if ( !isEditable )
{
// as soon as we find one read-only feature for the field, we can break early...
shouldShowMultiEditButton = false;
break;
}
}
}
}
}
break;
}

if ( hasMultiEditButton && !shouldShowMultiEditButton )
{
editPage()->layout()->removeWidget( mMultiEditButton );
mMultiEditButton->setParent( nullptr );
}
else
else if ( !hasMultiEditButton && shouldShowMultiEditButton )
{
if ( mode() == MultiEditMode && !fieldReadOnly )
{
editPage()->layout()->addWidget( mMultiEditButton );
}
editPage()->layout()->addWidget( mMultiEditButton );
}

switch ( mode() )
Expand All @@ -262,9 +291,7 @@ void QgsAttributeFormEditorWidget::updateWidgets()
case MultiEditMode:
{
stack()->setCurrentWidget( editPage() );

editPage()->layout()->addWidget( mConstraintResultLabel );

break;
}

Expand Down
119 changes: 119 additions & 0 deletions tests/src/python/test_qgsvectorlayerutils.py
Expand Up @@ -53,6 +53,125 @@ def createLayerWithOnePoint():

class TestQgsVectorLayerUtils(unittest.TestCase):

def test_field_is_read_only(self):
"""
Test fieldIsReadOnly
"""
layer = createLayerWithOnePoint()
# layer is not editable => all fields are read only
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))

layer.startEditing()
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))

field = QgsField('test', QVariant.String)
layer.addAttribute(field)
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 2))

layer.rollBack()
layer.startEditing()

# edit form config specifies read only
form_config = layer.editFormConfig()
form_config.setReadOnly(1, True)
layer.setEditFormConfig(form_config)
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))
form_config.setReadOnly(1, False)
layer.setEditFormConfig(form_config)
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))

# joined field
layer2 = QgsVectorLayer("Point?field=fldtxt2:string&field=fldint:integer",
"addfeat", "memory")
join_info = QgsVectorLayerJoinInfo()
join_info.setJoinLayer(layer2)
join_info.setJoinFieldName('fldint')
join_info.setTargetFieldName('fldint')
join_info.setUsingMemoryCache(True)
layer.addJoin(join_info)
layer.updateFields()

self.assertEqual([f.name() for f in layer.fields()], ['fldtxt', 'fldint', 'addfeat_fldtxt2'])
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 1))
# join layer is not editable
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 2))

# make join editable
layer.removeJoin(layer2.id())
join_info.setEditable(True)
layer.addJoin(join_info)
layer.updateFields()
self.assertEqual([f.name() for f in layer.fields()], ['fldtxt', 'fldint', 'addfeat_fldtxt2'])

# should still be read only -- the join layer itself is not editable
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 2))

layer2.startEditing()
self.assertFalse(QgsVectorLayerUtils.fieldIsReadOnly(layer, 2))

# but now we set a property on the join layer which blocks editing for the feature...
form_config = layer2.editFormConfig()
form_config.setReadOnly(0, True)
layer2.setEditFormConfig(form_config)
# should now be read only -- the joined layer edit form config prohibits edits
self.assertTrue(QgsVectorLayerUtils.fieldIsReadOnly(layer, 2))

def test_field_editability_depends_on_feature(self):
"""
Test QgsVectorLayerUtils.fieldEditabilityDependsOnFeature
"""
layer = createLayerWithOnePoint()

# not joined fields, so answer should be False
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 1))

# joined field
layer2 = QgsVectorLayer("Point?field=fldtxt2:string&field=fldint:integer",
"addfeat", "memory")
join_info = QgsVectorLayerJoinInfo()
join_info.setJoinLayer(layer2)
join_info.setJoinFieldName('fldint')
join_info.setTargetFieldName('fldint')
join_info.setUsingMemoryCache(True)
layer.addJoin(join_info)
layer.updateFields()

self.assertEqual([f.name() for f in layer.fields()], ['fldtxt', 'fldint', 'addfeat_fldtxt2'])
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 0))
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 1))
# join layer is not editable => regardless of the feature, the field will always be read-only
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 2))

# make join editable
layer.removeJoin(layer2.id())
join_info.setEditable(True)
join_info.setUpsertOnEdit(True)
layer.addJoin(join_info)
layer.updateFields()
self.assertEqual([f.name() for f in layer.fields()], ['fldtxt', 'fldint', 'addfeat_fldtxt2'])

# has upsert on edit => regardless of feature, we can create the join target to make the field editable
self.assertFalse(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 2))

layer.removeJoin(layer2.id())
join_info.setEditable(True)
join_info.setUpsertOnEdit(False)
layer.addJoin(join_info)
layer.updateFields()
self.assertEqual([f.name() for f in layer.fields()], ['fldtxt', 'fldint', 'addfeat_fldtxt2'])

# No upsert on edit => depending on feature, we either can edit the field or not, depending on whether
# the join target feature already exists or not
self.assertTrue(QgsVectorLayerUtils.fieldEditabilityDependsOnFeature(layer, 2))

def test_value_exists(self):
layer = createLayerWithOnePoint()
# add some more features
Expand Down

0 comments on commit f88729c

Please sign in to comment.