From a5d601f63393ed0ea132fd837889228153d97331 Mon Sep 17 00:00:00 2001 From: signedav Date: Mon, 27 Feb 2023 16:37:25 +0100 Subject: [PATCH 1/3] Fixes index lost (jumping to 0) when making a yellow selection. This because before it did not change the inSelection after the first instance of the connection. --- src/gui/attributetable/qgsfeaturelistview.cpp | 88 ++++++++++++------- src/gui/attributetable/qgsfeaturelistview.h | 5 +- tests/src/app/testqgsattributetable.cpp | 68 ++++++++++++++ 3 files changed, 126 insertions(+), 35 deletions(-) diff --git a/src/gui/attributetable/qgsfeaturelistview.cpp b/src/gui/attributetable/qgsfeaturelistview.cpp index e245aa68da7..2ccbb19bcc7 100644 --- a/src/gui/attributetable/qgsfeaturelistview.cpp +++ b/src/gui/attributetable/qgsfeaturelistview.cpp @@ -34,6 +34,22 @@ QgsFeatureListView::QgsFeatureListView( QWidget *parent ) : QListView( parent ) { setSelectionMode( QAbstractItemView::ExtendedSelection ); + + mUpdateEditSelectionTimerWithSelection.setSingleShot( true ); + connect( &mUpdateEditSelectionTimerWithSelection, &QTimer::timeout, this, [ this ]() + { + updateEditSelection( true ); + } ); + + mUpdateEditSelectionTimerWithSelection.setInterval( 0 ); + + mUpdateEditSelectionTimerWithoutSelection.setSingleShot( true ); + connect( &mUpdateEditSelectionTimerWithoutSelection, &QTimer::timeout, this, [ this ]() + { + updateEditSelection( false ); + } ); + + mUpdateEditSelectionTimerWithoutSelection.setInterval( 0 ); } QgsVectorLayerCache *QgsFeatureListView::layerCache() @@ -406,6 +422,19 @@ void QgsFeatureListView::selectRow( const QModelIndex &index, bool anchor ) } void QgsFeatureListView::ensureEditSelection( bool inSelection ) +{ + + if ( inSelection ) + { + mUpdateEditSelectionTimerWithSelection.start(); + } + else + { + mUpdateEditSelectionTimerWithoutSelection.start(); + } +} + +void QgsFeatureListView::updateEditSelection( bool inSelection ) { if ( !mModel->rowCount() ) { @@ -465,46 +494,37 @@ void QgsFeatureListView::ensureEditSelection( bool inSelection ) if ( editSelectionUpdateRequested ) { - if ( !mUpdateEditSelectionTimer.isSingleShot() ) + // The layer might have been removed between timer start and timer triggered + // in this case there is nothing left for us to do. + if ( !layerCache() ) + return; + + int rowToSelect = -1; + + if ( inSelection ) { - mUpdateEditSelectionTimer.setSingleShot( true ); - connect( &mUpdateEditSelectionTimer, &QTimer::timeout, this, [ this, inSelection, validEditSelectionAvailable ]() + const QgsFeatureIds selectedFids = layerCache()->layer()->selectedFeatureIds(); + const int rowCount = mModel->rowCount(); + + for ( int i = 0; i < rowCount; i++ ) { - // The layer might have been removed between timer start and timer triggered - // in this case there is nothing left for us to do. - if ( !layerCache() ) - return; - - int rowToSelect = -1; - - if ( inSelection ) + if ( selectedFids.contains( mModel->idxToFid( mModel->index( i, 0 ) ) ) ) { - const QgsFeatureIds selectedFids = layerCache()->layer()->selectedFeatureIds(); - const int rowCount = mModel->rowCount(); - - for ( int i = 0; i < rowCount; i++ ) - { - if ( selectedFids.contains( mModel->idxToFid( mModel->index( i, 0 ) ) ) ) - { - rowToSelect = i; - break; - } - - if ( rowToSelect == -1 && !validEditSelectionAvailable ) - rowToSelect = 0; - } + rowToSelect = i; + break; } - else + + if ( rowToSelect == -1 && !validEditSelectionAvailable ) rowToSelect = 0; - - if ( rowToSelect != -1 ) - { - setEditSelection( mModel->mapToMaster( mModel->index( rowToSelect, 0 ) ), QItemSelectionModel::ClearAndSelect ); - } - } ); - mUpdateEditSelectionTimer.setInterval( 0 ); + } + } + else + rowToSelect = 0; + + if ( rowToSelect != -1 ) + { + setEditSelection( mModel->mapToMaster( mModel->index( rowToSelect, 0 ) ), QItemSelectionModel::ClearAndSelect ); } - mUpdateEditSelectionTimer.start(); } } diff --git a/src/gui/attributetable/qgsfeaturelistview.h b/src/gui/attributetable/qgsfeaturelistview.h index 3bd455f663b..22cae58c959 100644 --- a/src/gui/attributetable/qgsfeaturelistview.h +++ b/src/gui/attributetable/qgsfeaturelistview.h @@ -234,6 +234,8 @@ class GUI_EXPORT QgsFeatureListView : public QListView private: void selectRow( const QModelIndex &index, bool anchor ); + void updateEditSelection( bool inSelection = false ); + enum PositionInList { First, @@ -264,7 +266,8 @@ class GUI_EXPORT QgsFeatureListView : public QListView int mRowAnchor = 0; QItemSelectionModel::SelectionFlags mCtrlDragSelectionFlag; - QTimer mUpdateEditSelectionTimer; + QTimer mUpdateEditSelectionTimerWithSelection; + QTimer mUpdateEditSelectionTimerWithoutSelection; friend class QgsDualView; }; diff --git a/tests/src/app/testqgsattributetable.cpp b/tests/src/app/testqgsattributetable.cpp index 57864167af1..0f49cce8c5f 100644 --- a/tests/src/app/testqgsattributetable.cpp +++ b/tests/src/app/testqgsattributetable.cpp @@ -66,6 +66,7 @@ class TestQgsAttributeTable : public QObject void testStartMultiEditNoChanges(); void testMultiEditMakeUncommittedChanges(); void testInvalidView(); + void testEnsureEditSelection(); private: QgisApp *mQgisApp = nullptr; @@ -830,5 +831,72 @@ void TestQgsAttributeTable::testInvalidView() QCOMPARE( dlg->mMainView->filteredFeatures(), QgsFeatureIds() << 1 << 3 ); } +void TestQgsAttributeTable::testEnsureEditSelection() +{ + std::unique_ptr< QgsVectorLayer > layer = std::make_unique< QgsVectorLayer >( QStringLiteral( "Point?field=col0:integer&field=col1:integer" ), QStringLiteral( "test" ), QStringLiteral( "memory" ) ); + QVERIFY( layer->isValid() ); + + QgsFeature ft1( layer->dataProvider()->fields(), 1 ); + ft1.setAttributes( QgsAttributes() << 1 << 2 ); + layer->dataProvider()->addFeature( ft1 ); + QgsFeature ft2( layer->dataProvider()->fields(), 2 ); + ft2.setAttributes( QgsAttributes() << 3 << 4 ); + layer->dataProvider()->addFeature( ft2 ); + QgsFeature ft3( layer->dataProvider()->fields(), 3 ); + ft3.setAttributes( QgsAttributes() << 5 << 6 ); + layer->dataProvider()->addFeature( ft3 ); + QgsFeature ft4( layer->dataProvider()->fields(), 4 ); + ft4.setAttributes( QgsAttributes() << 7 << 8 ); + layer->dataProvider()->addFeature( ft4 ); + + layer->removeSelection(); + + std::unique_ptr< QgsAttributeTableDialog > dlg( new QgsAttributeTableDialog( layer.get() ) ); + + //since the update is done by timer we need to wait for update (at least one milisecond) + QEventLoop loop; + + // we set the index to ft3 + dlg->mMainView->setCurrentEditSelection( {ft3.id()} ); + // ... and the currentEditSelection is on ft3 + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 3 ) ); + + // we make a featureselection on ft1, ft2 and ft3 + layer->selectByIds( QgsFeatureIds() << 1 << 2 << 3 ); + QTimer::singleShot( 1, this, [&] { loop.quit(); } ); + loop.exec(); + // ... and the currentEditSelection stays on ft3 (since it's in the featureselection) + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 3 ) ); + + // we release the featureselection + layer->removeSelection(); + QTimer::singleShot( 1, this, [&] { loop.quit(); } ); + loop.exec(); + // ... and the currentEditSelection persists on 3 (since it does not make an update) + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 3 ) ); + + // we make afeatureselection on ft4 + layer->selectByIds( QgsFeatureIds() << 4 ); + QTimer::singleShot( 1, this, [&] { loop.quit(); } ); + loop.exec(); + // ... and the currentEditSelection goes to ft4 + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 4 ) ); + + // we make afeatureselection on ft2 and ft3 + layer->selectByIds( QgsFeatureIds() << 2 << 3 ); + QTimer::singleShot( 1, this, [&] { loop.quit(); } ); + loop.exec(); + // ... and the currentEditSelection goes to the first one of the featureselection (means ft2) + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 2 ) ); + + // we reload the layer + layer->reload(); + QTimer::singleShot( 1, this, [&] { loop.quit(); } ); + loop.exec(); + // ... and the currentEditSelection jumps to the first one (instead of staying at 2, since it's NOT persistend) + QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 1 ) ); + +} + QGSTEST_MAIN( TestQgsAttributeTable ) #include "testqgsattributetable.moc" From c2c4b5b0053cfa9c43833ff1ca73e61cc8242ff1 Mon Sep 17 00:00:00 2001 From: signedav Date: Tue, 28 Feb 2023 11:12:00 +0100 Subject: [PATCH 2/3] fix typo --- tests/src/app/testqgsattributetable.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/app/testqgsattributetable.cpp b/tests/src/app/testqgsattributetable.cpp index 0f49cce8c5f..7ad66bd5caa 100644 --- a/tests/src/app/testqgsattributetable.cpp +++ b/tests/src/app/testqgsattributetable.cpp @@ -853,7 +853,7 @@ void TestQgsAttributeTable::testEnsureEditSelection() std::unique_ptr< QgsAttributeTableDialog > dlg( new QgsAttributeTableDialog( layer.get() ) ); - //since the update is done by timer we need to wait for update (at least one milisecond) + //since the update is done by timer, we have to wait for the update (at least one millisecond) QEventLoop loop; // we set the index to ft3 From c540c50c8a22bc368105ac094b3833f7192fba25 Mon Sep 17 00:00:00 2001 From: signedav Date: Wed, 1 Mar 2023 10:42:31 +0100 Subject: [PATCH 3/3] replace QTimer/eventloop with QSignalSpy to wait untill change is done - times out 1 millisecond or receives currentEditSelectionChanged --- tests/src/app/testqgsattributetable.cpp | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/tests/src/app/testqgsattributetable.cpp b/tests/src/app/testqgsattributetable.cpp index 7ad66bd5caa..73e05a79ec0 100644 --- a/tests/src/app/testqgsattributetable.cpp +++ b/tests/src/app/testqgsattributetable.cpp @@ -32,6 +32,8 @@ #include "qgsgui.h" #include "qgseditorwidgetregistry.h" +#include + /** * \ingroup UnitTests * This is a unit test for the attribute table dialog @@ -853,8 +855,9 @@ void TestQgsAttributeTable::testEnsureEditSelection() std::unique_ptr< QgsAttributeTableDialog > dlg( new QgsAttributeTableDialog( layer.get() ) ); - //since the update is done by timer, we have to wait for the update (at least one millisecond) - QEventLoop loop; + //since the update is done by timer, we have to wait (at least one millisecond) or until the current edit selection changed + qRegisterMetaType( "QgsFeature&" ); + QSignalSpy spy( dlg->mMainView->mFeatureListView, &QgsFeatureListView::currentEditSelectionChanged ); // we set the index to ft3 dlg->mMainView->setCurrentEditSelection( {ft3.id()} ); @@ -863,36 +866,31 @@ void TestQgsAttributeTable::testEnsureEditSelection() // we make a featureselection on ft1, ft2 and ft3 layer->selectByIds( QgsFeatureIds() << 1 << 2 << 3 ); - QTimer::singleShot( 1, this, [&] { loop.quit(); } ); - loop.exec(); + spy.wait( 1 ); // ... and the currentEditSelection stays on ft3 (since it's in the featureselection) QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 3 ) ); // we release the featureselection layer->removeSelection(); - QTimer::singleShot( 1, this, [&] { loop.quit(); } ); - loop.exec(); + spy.wait( 1 ); // ... and the currentEditSelection persists on 3 (since it does not make an update) QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 3 ) ); // we make afeatureselection on ft4 layer->selectByIds( QgsFeatureIds() << 4 ); - QTimer::singleShot( 1, this, [&] { loop.quit(); } ); - loop.exec(); + spy.wait( 1 ); // ... and the currentEditSelection goes to ft4 QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 4 ) ); // we make afeatureselection on ft2 and ft3 layer->selectByIds( QgsFeatureIds() << 2 << 3 ); - QTimer::singleShot( 1, this, [&] { loop.quit(); } ); - loop.exec(); + spy.wait( 1 ); // ... and the currentEditSelection goes to the first one of the featureselection (means ft2) QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 2 ) ); // we reload the layer layer->reload(); - QTimer::singleShot( 1, this, [&] { loop.quit(); } ); - loop.exec(); + spy.wait( 1 ); // ... and the currentEditSelection jumps to the first one (instead of staying at 2, since it's NOT persistend) QVERIFY( dlg->mMainView->mFeatureListView->currentEditSelection().contains( 1 ) );