Reload the data source when a dependency layer is changed

[suricactus]

Given layers A and B:

• A is a view from layer B
• A is dependent on layer B

Without the fix, whenever one edits layer B, layer A does not get it's cache invalidated. Because A is a view from B, it means one can modify/create/delete a feature in layer B, but it will not appear in layer A.

Bonus fix:
the toggle edit button on relation editor widget easily get's unsyncronized with the rest of the buttons. If you click on the button when there are changes in the relation and then choose Cancel on the dialog, the button wrongly toggles its state, while the rest of the buttons remain enabled. E.g. toggle edit is toggled to false, but the button "Add new row" is enabled, or vice versa.

[uclaros]

Isn't this supposed to be handled by setting Dependencies in Layer Properties?

[suricactus]

It is, but it was not doing the job, because if there is a WFS layer, it stays cached. This is what I mean with "A is dependent on layer B".

Actually the whole PR is about calling https://github.com/qgis/QGIS/pull/37475/files#diff-106be592650b55ccae79c83eecd15cebR5328 automatically.

[troopa81]

I'm afraid that triggering a reloadData for all type of provider (including the one who does not have cache) every time a dependency has changed will generate an extra read from the provider.

It is, but it was not doing the job, because if there is a WFS layer, it stays cached. This is what I mean with "A is dependent on layer B".

if B is modified, and a dependency is defined between A et B, both will fire the dataChanged. Would not be better to invalidate the cache like it's done here for instance?

[3nids]

I'm afraid that triggering a reloadData for all type of provider (including the one who does not have cache) every time a dependency has changed will generate an extra read from the provider.

Well, isn't it all what dependencies are about? If they are dependent, they need to be reloaded, or am I missing something?

Would not be better to invalidate the cache like it's done here for instance?

WFS provider doesn't use QgsVectorLayerCache. I don't see how we could solve this in the provider code, but I don't know it very well.

[uclaros]

Well, isn't it all what dependencies are about? If they are dependent, they need to be reloaded, or am I missing something?

@3nids Since for other types of vector layers the dependency is allready handled by the four existing signals, featureAdded, featureDeleted, geometryChanged and dataChanged, won't adding the afterCommitChanges signal trigger an extra reload? If this is a WFS specific issue, then it should probably have a WFS specific solution.

[troopa81]

Well, isn't it all what dependencies are about? If they are dependent, they need to be reloaded, or am I missing something?

IIRC, the real point of data dependencies is to refresh the index cache so your snapping information are up to update. When dataChanged is fired, index is rebuilt and data are read again, so if you force reloadData, I would say it triggers an extra read (on Postgres provider for instance).

[suricactus]

I will further investigate whether the rest of the providers handle this situation correctly, then probably makes sense to move the solution to be WFS specific.

[uclaros]

to me a very valid use case is when you have views on the DBs which depends on other tables that are edited in QGIS.

Isn't this use case already handled correctly with the four existing signals? The absence of a bug report is not helping here...

[suricactus]

I have checked with the PostgreSQL provider. Reminder, the situation is "A is a view from layer B". There is no need to setup dependencies, once the user changes the extent, either by panning or zooming, the view is reloaded from the data source. AFAIK, all the providers reread from the dataSource.
IMHO, the argument that this would put too much pressure rereading from the provider does not stand here, because we are going to read once we change the extent anyway. Acatually we are going to provide addtional value to the user with this fix, as the canvas will be updated as soon as the changes are commited.

[uclaros]

How things work so far, without dependencies:

1. B is a PostGIS table layer
2. A is a PostGIS view pointing to table B layer
3. We start editing B and add a feature.
4. We commit layer B
5. Layer B shows the newly added feature, layer A does not.
6. Pan/zoom/refresh triggers a re-read to A
7. Both layers A and B show the newly added feature

How things work so far, with dependencies:

1. B is a PostGIS table layer
2. A is a PostGIS view pointing to table B layer
3. We set A -> dependencies and select layer B
4. We start editing B and add a feature.
5. Upon committing layer B, featureAdded is fired, layer A cache is invalidated and re-read.
6. Both layers A and B show the newly added feature, without having to zoom/pan/refresh the view.

If we add the suggested commit signal, an extra re-read will be issued between steps 5 and 6.

I can't see what is not working as it should, that's why I say it's probably a WFS issue needing a WFS solution.

[m-kuhn]

5. Upon committing layer B, featureAdded is fired, layer A cache is invalidated and re-read.

How does that exactly happen so far?

[uclaros]

How does that exactly happen so far?

For each of the dependent layers, ie layer B, the following signals:

QGIS/src/core/qgsvectorlayer.cpp

Lines 5371 to 5381 in 7b2dc6e

	for ( const QgsMapLayerDependency &dep : qgis::as_const( mDependencies ) )
	{
	QgsVectorLayer *lyr = static_cast<QgsVectorLayer *>( QgsProject::instance()->mapLayer( dep.layerId() ) );
	if ( !lyr )
	continue;
	connect( lyr, &QgsVectorLayer::featureAdded, this, &QgsVectorLayer::emitDataChanged );
	connect( lyr, &QgsVectorLayer::featureDeleted, this, &QgsVectorLayer::emitDataChanged );
	connect( lyr, &QgsVectorLayer::geometryChanged, this, &QgsVectorLayer::emitDataChanged );
	connect( lyr, &QgsVectorLayer::dataChanged, this, &QgsVectorLayer::emitDataChanged );
	connect( lyr, &QgsVectorLayer::repaintRequested, this, &QgsVectorLayer::triggerRepaint );
	}

are connected to layer A's. I think emitDataChanged eventually refreshes the attribute table and triggerRepaint the canvas. I am not familiar with the code but AFAICT it seems to work.

[m-kuhn]

The interesting part is what dataChanged is then connected to.

If it's triggerRepaint, it's impossible for individual providers to implement their own behavior and like in the case of the wfs provider dismiss the cache. Instead it should be connected to a slot that allows the provider to invalidate the cache. reload() sounds like a good candidate to use instead.

[uclaros]

Wouldn't the other signals need to be replaced so that only a single reload is triggered? Maybe ask @mhugo 's opinion who introduced the feature?

[m-kuhn]

Maybe. Not sure without looking deeper into this or getting more information.

To keep the scope, and just by digesting information from this PR:

• the current implementation repaints and refreshes the snapping index. It seems to work fine for postgres and other non-cached sources. A single request is done.
• the current approach does not work for wfs because there is a cache in place and no chance to fix the wfs provider with the current api.
• I conclude: if we replace the current connections with more appropriate ones that also dismiss caches (like reload() does) the issue with wfs should be fixed and other providers will still do a single request because the direct call to repaint is gone

Did I miss something?

[troopa81]

I conclude: if we replace the current connections with more appropriate ones that also dismiss caches (like reload() does) the issue with wfs should be fixed and other providers will still do a single request because the direct call to repaint is gone

If you do so, if A depends on B, and B is modified

• B will be reloaded because of the reload() call
• A will trigger repaint because it has been modified
• A & B providers will be read when rendering

So B is read twice, no?

For Postgres, it's not a problem because the data is not really read again, so data will be read only once. It's the same for Ogr provider and Oracle don't even implement the reload. I'm don't know about other providers, but in the end maybe calling reload doesn't hurt.

[m-kuhn]

• B will be reloaded because of the reload() call

This will not "reload", it will just invalidate caches. Since there is no "main cache" in QGIS it also cannot really "reload" the data anywhere.

• A will trigger repaint because it has been modified

• A & B providers will be read when rendering

Add to that that repaints are not done "right now" but scheduled, so even if in a single event loop run 5 repaints are requested, only 1 will be executed.

Possibly it's more a confusion of "reload" being a bad name and "invalidateCaches" would be better.

[troopa81]

Possibly it's more a confusion of "reload" being a bad name and "invalidateCaches" would be better.

Indeed, and the documentation says it all

Reloads the data from the source by calling reloadProviderData() implemented by providers with data caches to synchronize, changes in the data source, feature counts and other specific actions.

So, +1 on calling reloadData

[suricactus]

So in the end we can merge this fix it as it is? Should we move forward?

[m-kuhn]

Looks good to me.

Waiting for a bit if there are some final considerations and if nothing is brought up will merge soon.
Thanks everyone for the work and review!

[troopa81]

So in the end we can merge this fix it as it is? Should we move forward?

I'm fine with this too, thanks for this work