[Layer dependencies] Emit QgsVectorLayer::dataChanged on commitChanges

[Djedouas]

Description

Fixes #57784 where even if the layer is checked as a dependency layer, there is no reloading of data when the layer is saved.

With this change, the dataChanged signal is emitted on commitChanges, so that when a feature is externally modified on save (database trigger for example) we have index and cache reconstruction.

---

Related PRs:

• Reload the data source when a dependency layer is changed #37475
• fix cache renew for layer dependencies #41463

This change is kind of following this particular comment/discussion #41463 (comment)

See the tests explaining the multiple emissions of dataChanged.

Ping @suricactus @m-kuhn @MorriganR @troopa81 @3nids for review

[github-actions]

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit 9bf2d45)

[troopa81]

@Djedouas After a second thought on our discussion I'm wondering if we could not fire only one dataChange on commit instead of 2 when adding (delete the -1 feature +add the inserted one) and 0 (3 with your proposal) when commit. Of course, without changing the general behavior when it's not about committing.

We could use the mCommitChangesActive boolean to have a different behavior in emitDataChanged() method. This behavior would not reset mDataChangedFired to false and it would be done when mCommitChangesActive is reset after commit ? Would that work?

[nyalldawson]

This is potentially a big speed regression -- eg QgsPointLocator connects to QgsVectorLayer::dataChanged and destroys the index when it's emitted, and the attribute table (via QgsVectorLayerCache) will also trigger a full rebuild. My understanding was that dataChanged should only be used when we don't know what's changed, but when we DO know what's changed the atomic signals (featureAdded, etc) are better because we can handle them in optimised ways.

Isn't all this supposed to be handled by the logic in QgsVectorLayer::setDependencies anyway? That's setting up manual connections to emit dataChanged when the atomic signals are received from a dependent layer.

[troopa81]

My understanding was that dataChanged should only be used when we don't know what's changed, but when we DO know what's changed the atomic signals (featureAdded, etc) are better because we can handle them in optimised ways.

That's the case here, a dependency has been committed, and so eventually the layer depending on it has also changed (because of trigger for instance) and need to fire dataChanged . This PR doesn't change this.

Isn't all this supposed to be handled by the logic in QgsVectorLayer::setDependencies anyway? That's setting up manual connections to emit dataChanged when the atomic signals are received from a dependent layer.

Yes, that's what it's intended here (don't know why we reloadData() and not just fire dataChanged signal in that case). I don't see any regression here, except IMHO we fire too many dataChanged per event.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[nyalldawson]

I'm confused now -- was a force push used here to change the approach? The current changes don't seem to match with the PR description (or more recollections from previously reviewing this)

[Djedouas]

Nothing changed since the PR creation

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 98a6c9a)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 98a6c9a)

[troopa81]

@nyalldawson @m-kuhn

We discussed this with @Djedouas , I think the proposal is good as it implies only one additional signal firing when committing instead of 3 (feature added+deleted+commit) when it comes to new feature.