[needs-docs][feature] Deferred handling of bad layers

[elpaso]

Description

This is an implementation of the (optional) deferred bad layers handling, it also offers a "Change data source..." contextual menu for raster and vector layers.

This feature was funded by ARPA Piemonte (regional environment protection agency).

Use cases

User has a project set up to work on an intranet that has access to some internal resources like, for example:

• a WMS layer
• a shapefile/gpkg/spatialite on a shared network folder
• a postgis vector

User wants to be able to open the project when is not connected to the intranet, skip the bad layers dialog by choosing "Ignore all bad layers" and have the bad layers in the TOC marked as bad layers.

By clicking on the bad layer indicator (or right clicking in the contextual menu over the layer entry in the TOC), the user should be able to "fix" the bad layer by selecting another data source through either a data source select dialog filtered by layer type (vector or raster, or maybe also by provider key) or a browser dialog.

When the user makes any modifications to the project and save the project, the bad layers (if not "fixed" as described above) should be stored back in the project file unchanged (so that they are still available when back into the intranet network).

Open issues

• Currently setDataSource reset the style to default, we want to restore original layer properties (not only style!), for doing that, I've come up with hack of storing the original XML layer properties to be able to use to restore the layer properties when setDataSource is called. The very same information could be useful if the user wanted to reset the original layer properties after having changed them, but this is not a priority for now. Needless to say that this is just a hack and we should probably store that information in the layer directly and not as a custom layer property as I did in this POC.
• When saving back a project which contains bad layers, the bad layers are skipped from the drawing layer order (the renderer doesn't know about them?), not a blocker but still an issue.

TODO

• setDataSource for raster layers
• fix setDataSource to be able to preserve layer properties (if possible) when a new dataSource is set
• test relations are preserved and updated when data source changes
• test joins are preserved and updates when data source change
• test drawing order is preserved
• connect the bad layer indicator or the right click context menu to the data source select dialog (or a browser) filtered by layer type (and maybe provider, but maybe not)

[nirvn]

Beautiful to see this happening. IMHO, this should not be optional, but rather our new behavior when handling missing layer source. It'd just be dragging weight to keep the current dialog on project load.

[elpaso]

@nirvn I'm slightly doubtful on this, if we drop the initial dialog, we should still notify the user that there are some "bad" layers, they might be buried inside nested groups and I think that the user should immediately know about "bad" layers.

[nirvn]

@elpaso , yep, beyond the indicator itself (which btw should break away from our standard dark gray outline in favor of a red outline icon) I suggest we through in an error message bar.

I think we can insure that we end up with a superior experience focusing on one handling of bad layers.

[3nids]

some questions:

• what happens if you save the project with invalid layers and reopen it? Is the invalidLayerProperties tag correctly read and restore so the layers will be available again as expected?
• should QgsProject::mapLayers() (and maybe even mapLayer( id )) be modified so they don't return invalid layers by default? Maybe add a boolean tag (or a flag for better flexibility). It might be much safer for plugins behavior.
• what happens when you do a getFeatures on an invalid layer?
• does an invalid layer becomes the active one when it is selected (it think it should not)?

[elpaso]

some questions:

* what happens if you save the project with invalid layers and reopen it? Is the `invalidLayerProperties` tag correctly read and restore so the layers will be available again as expected?

That's the idea, and this is the (initial and too-basic-to-be-safe) test: https://github.com/qgis/QGIS/pull/8359/files#diff-d83688aa49188d5d74b506975e654145

Note that invalidLayerProperties is completely transparent through sessions and it should never be stored in the project, its unique purpose is to be used to restore (or perhaps also reset) layer properties.
Also, there is no guarantee that it exists, for example, you could start a new project, add a valid layer, the layer then for whatever reason becomes invalid, and in this case you would not have the invalidLayerProperties.

* should `QgsProject::mapLayers()` (and maybe even `mapLayer( id )`) be modified so they don't return invalid layers by default? Maybe add a boolean tag. It might be much safer for plugins behavior.

Yes, maybe, the thing is that even now without this patch a layer can become invalid at any time in its lifecycle, so I think it's better to never assume layer validity.

But I started to add some API methods to retrieve only valid layers: 5402505

* what happens when you do a `getFeatures` on an invalid layer?

I didn't test that yet, but I hope we do have checks in place or it will just crash (I'll make sure this part is tested).

* does an invalid layer becomes the active one when it is selected (it think it should not)?

I agree with you that it should not become active.

[wonder-sk]

Great to see work towards this feature - much needed!

the thing is that even now without this patch a layer can become invalid at any time in its lifecycle, so I think it's better to never assume layer validity.

Are you sure about this? I think at this point QgsProject::mapLayers() always gives you valid layers. What are the cases a layer suddenly becomes invalid? It may happen that a layer gets suddenly deleted, but then mapLayers() would not return such layer anymore...

Also I am not sure if I understand the need for "invalidLayerProperties" - why not just save the invalid layers as they were before? This extra packaging of XML into a custom property to be stored inside XML again seems a bit strange.

[elpaso]

Great to see work towards this feature - much needed!

the thing is that even now without this patch a layer can become invalid at any time in its lifecycle, so I think it's better to never assume layer validity.

Are you sure about this? I think at this point QgsProject::mapLayers() always gives you valid layers. What are the cases a layer suddenly becomes invalid? It may happen that a layer gets suddenly deleted, but then mapLayers() would not return such layer anymore...

Yes, I'm sure, see it in action:

Also I am not sure if I understand the need for "invalidLayerProperties" - why not just save the invalid layers as they were before? This extra packaging of XML into a custom property to be stored inside XML again seems a bit strange.

The purpose of "invalidLayerProperties" is exactly to store the bad layer properties like they were before, to be able to store them back unmodified.

But it looks strange to me too, what would you suggest? To scan for bad layers on project save, open the original project file, find the XML for the bad layer id and inject it into the dom document?

I think it's safer to keep an in-memory copy of the layer properties instead of re-opening the original project file and to parse it again to get the original XML.

If you are asking why it is necessary, that's because all the style information part (that would go in the renderer instance) is completely skipped if the layer is not valid at loading time, so we don't have this information for a bad layer unless we store it somewhere (or we re-read the original project file).

But it's possible that I'm missing something, if you have a better idea I'm all ears :)

[elpaso]

@wonder-sk about mapLayers always returning valid layers, it has never been true (because a layer can become invalid), but it was not possible to add an invalid layer to the store, now it is: 5402505

[nyalldawson]

what happens when you do a getFeatures on an invalid layer?

In PyQGIS we should raise an exception.

[nyalldawson]

@elpaso

Currently setDataSource reset the style to default, we want to restore original layer properties (not only style!), for doing that,

[...]

My suspicion -- if the layer data source is broken then the geometry type is being reset to unknown, so later calls to setDataSource reset the style. Maybe you can get away with just storing the original layer geometry type somewhere and using this when initially restoring an invalid layer, instead of resetting it to unknown geometry.

Yes, that's exactly the case, the geometry type is stored in the project but it gets set to unknown when a "bad" layer is loaded, so we currently have that information available only in the original XML layer properties.

[nyalldawson]

Just a heads up -- there's actually two types of invalid layers we need to handle. One is layers which exist and where layer.isValid() returns false. The other is layer nodes in the layer tree which could not be resolved to a valid layer. These return a nullptr when calling QgsLayerTreeLayer::layer(), and result from operations like importing a QLR file with a broken data source. Just something to be aware of here.

[nyalldawson]

@nirvn

I'm slightly doubtful on this, if we drop the initial dialog, we should still notify the user that there are some "bad" layers, they might be buried inside nested groups and I think that the user should immediately know about "bad" layers.

Another thing we need to consider -- the current (bad) bad layers dialog does allow repathing multiple items at once. If we drop the dialog we'd need to add a way for users to set the correct data source for multiple selected layers from the layer tree at once, or it'd be very annoying for fixing projects with many layers...

[nirvn]

@elpaso , @nyalldawson , I gave this some more thoughts overnight. While I'm a big -1 to have any kind of options with regard to how we load a project (i.e. missing data source popup etc.), it's indeed the case that the current (yes, bad) bad layers dialog makes it easier to repath multiple items at once.

Here's my proposal: when loading a project, if layers with missing/inaccessible data sources are found, throw an error message bar which includes a button to open the (again, bad) bad layers dialog. It'd look like:

|_(X) The project has missing or inaccessible data sources.__[ Open bad layers dialog ]_|

IMHO, that creates a much, much better UX overall, and we keep the option for mass repathing.

[elpaso]

Just a heads up -- there's actually two types of invalid layers we need to handle. One is layers which exist and where layer.isValid() returns false. The other is layer nodes in the layer tree which could not be resolved to a valid layer. These return a nullptr when calling QgsLayerTreeLayer::layer(), and result from operations like importing a QLR file with a broken data source. Just something to be aware of here.

Correct, basically a QgsMapLayer instance with a 0x0 data provider and another with valid data provider that points to a broken/not-existing data source, they are both invalid, but the likelihood that the first one crashes if there are no checks in place in the code are much higher.

[elpaso]

@3nids I've added relations to the mix, see ffde3ae#diff-d83688aa49188d5d74b506975e654145R121

[elpaso]

@pblottiere would you please have a look here? This test was failing on travis with:

Traceback (most recent call last):
  File "/root/QGIS/tests/src/python/test_qgsrelation.py", line 187, in testValidRelationAfterChangingStyle
    for t in tab.children():
AttributeError: 'QgsAttributeEditorRelation' object has no attribute 'children'

I cannot reproduce the issue locally, it looks like the QgsAttributeEditorContainer is not added and the QgsAttributeEditorRelation is added directly.

I patched the test here but maybe there is something else I can check.

[pblottiere]

@elpaso I'm glad to see that you've fixed your issue :)

[elpaso]

@pblottiere I can't say I fixed it, I still cannot reproduce it locally so I had to bisect using a docker (that took hours!!!) and found the guilty commit, which contained an apparently harmless change (storing the style as QDomDocument instead of and XML string), I just reverted that commit. So far so good.

[elpaso]

Ready for merge. There might be some remaining corner cases to catch, and the UI can of course be polished, but I think we should merge and and start playing with it.

[haubourg]

Wonderful, I've been hoping for this for years. Kudos

[nirvn]

@elpaso , I've just tested this. Few comments:

• changing data source of a bad layer (via indicator) fails to restore the subsetSqlString (ie the layer's right-click menu "set filter..."
• rule-based renderer settings not properly restored when changing data source of a bad layer (via indicator), instead the renderer is reset back to single symbol
• the buttons in the bad layers dialog when loading a project with missing layer data source is really confusing, especially the "close without saving", which is obscure. IMHO we need to revisit, possibly [ remove bad layers ]
  
• we probably would benefit in revisiting the icon used for broken layer data source:
  

Beyond that, I'm just left with a big thanks for tackling this, it's been on many people's wishlist for a looong time.

[elpaso]

@nirvn thanks for testing!

I'll check the other issues.

the buttons in the bad layers dialog when loading a project with missing layer data source is really confusing, especially the "close without saving", which is obscure. IMHO we need to revisit, possibly [ remove bad layers ]

That button has a discard role with a "Discard" label, and a nice trashbin icon on linux. What it does is really a discard (destructive action, with a confirmation dialog), but you are right that "Close without saving" is confusing. We should add an explicit label, do you have any recommendation for the button text?

I've added tooltips btw.

we probably would benefit in revisiting the icon used for broken layer data source:

Sure, I was going to ask @SrNetoChan (he's my icon master!) to polish that icon, but perhaps you would like to help with that?
I think we may also want icon for the "Change data source action.

[nirvn]

@elpaso , I usually think tooltips aren't the greatest UX to begin with. If a label requires to get the tooltip to understand its meaning, IMHO we need to change the label :)

The screenshot I took is on linux, ubuntu 18.10 (ubuntu has terrrible qt5 action button icons). I'd suggest using a custom icon, our trash icon.

[ Browser ] [ ✔️ Apply ] [ 🗑️ Remove broken layers ] [ Defer broken layer source change ]

Something like that :)

[SrNetoChan]

How about:
[ Browse ] [ ✔️ Apply ] [ 🗑️ Remove broken layers ] [ Keep broken layers ]
?

[SrNetoChan]

Created a pull request #8426 with a new icon:

If possible I would also change the color of the layer name on the layers panel. Either a soft gray (like the one used when the layer is not visible, or a soft red.

I also find the image icon on the left side a bit confusing, would prefer a de-activate check box.

Something like this:

or this:

[elpaso]

@SrNetoChan the standard generic layer image sucks, it's one of the few that are still in png format https://github.com/qgis/QGIS/blob/master/images/themes/default/mIconLayer.png