Add support for map-contextual WMS legend

[strk]

See http://hub.qgis.org/issues/11859

[strk]

TODO:

• Add per-layer configuration to enable/disable contextual WMS legend
• Ensure legend is not fetched again while not visible/expanded
• Make sure composer is using the setting

[strk]

The new commits ensures legend is not fetched again while not expanded, implementing the lazy loading approach suggested by @wonder-sk

TODO:

• Add per-layer configuration to enable/disable contextual WMS legend
• Make sure composer is using the setting
• Decide about legend visibility for invisible layer ( see http://lists.osgeo.org/pipermail/qgis-developer/2014-December/036028.html )

[strk]

Per-layer configuration implemented with last commit (at add WMS layer level).

TODO:

• Test composer
• Decide about showing or not the legend when layer is not visible

[strk]

NOTE: @nyalldawson suggested to use an existing class, already tested, in place of the QgsImageDownloader. Nyall, could you take a look at my interface to see how you like it ?

https://github.com/strk/QGIS/blob/wmslegend/src/core/raster/qgsrasterdataprovider.h#L51-L72

I guess it could be renamed to QgsImageFetcher ...

The class you suggested is this one:
http://qgis.org/api/classQgsNetworkContentFetcher.html

[strk]

With the latest commit I'm also happy with the autoredrawing of the legend.
And, I'm deciding now that I don't care if the legend still updates while the layer is unchecked/invisible.
If the user doesn't want the legend updated, she can collapse the legend.

So the only left over is the test for composer.
I'd appreciate if anyone else wants to test the current state.

[nyalldawson]

@strk I'm strongly of the opinion that QgsImageFetcher should be merged with QgsNetworkContentFetcher. The intention with QgsNetworkContentFetcher is to collect all the various implementations of handling redirects, authorisation, conversion of replies to different formats, etc which are currently scattered and duplicated throughout QGIS, so it'd be good to get this image handling added to that class rather then reimplemented here. It's a pretty straightforward change:

• add the missing progress and error signals to QgsNetworkContentFetcher
• add a "QImage* contentAsImage() const" method to QgsNetworkContentFetcher
• I think it'd also benefit from a "contentType()" method, which would return a QgsNetworkContentFetcher::Content enum value for Image, String, etc... that way it would be easier to check for content which is expected as an image but was returned as an error string.
• Add unit tests for image methods, so there's no regression in the coverage of QgsNetworkContentFetcher (see http://dash.orfeo-toolbox.org/viewCoverageFile.php?buildid=166066&fileid=325361)

What's your thoughts? I'd definitely find this useful in composer, as it would allow me to remove some extra duplicated code for downloading images which is present there.

[strk]

I've tested composer to also work, so I think I'm done with this.
The PR is still open for refinements. Possible refinements:

• QgsNetworkContentFetcher enhancement to be usable as QgsImageFetcher
• Streamline of the LayerTreeModel so to not have to recursively signal dataChanged

[strk]

@nyalldawson my ImageFetcher class is virtual, and that allows me to provide a Fetcher that basically returns an already-available (cached) WMS legend. That kind of feature is not possible with the NetworkContentFetched (it needs be a virtual interface for that to work).

[strk]

I've rebased the branch to master and force-pushed here

[strk]

Final squash and forced update. Ready for master merge.

[luipir]

A question to strk (and to all)... reading the code of ImageFetcher is architectural similar to the old code of getLegendGraphic that was modified due to MT adding an internal eventLoop. My question is, if imageChecher works, there is a structural problem or use case in getLegendGraphic that overcomplicate it's code, or ther is something in imageFetcher that is not considered.

[strk]

Who (or what) is RT ?

[strk]

@luipir if that answers your question, the difference between ImageFetcher and getLegendGraphic is that the former is asyncrhonous while the latter (pre-existing one) was syncrhonous.

[strk]

I've added another commit to report download progress

[strk]

@wonder-sk the latest commit here reverts the hack of invoking the recursive dataChanged emission by properly emitting dataChanged from WMSLegendNode::invalidateMapBasedData.

There should be nothing more pending here as far as I can tell.
Anything else from your side ? Or I'd squash merge it.

[wonder-sk]

I haven't had time yet to inspect the code in detail. (and I will be on holiday for the next 10 days)

But in general please:

• try to use ./scripts/prepare-commit.sh to properly indent the code (it would be re-indented later anyway, bringing in unnecessary changes
• do not put sponsor names to the source code - but feel free to mention them in commit messages

Also I believe the newly adopted QEP process expects that a QEP is first accepted before the work based on the QEP is merged.

[strk]

Ok, I'll indent, drop sponsor name from code, squash and force a push of a single commit again.

The QEP, for reference, is here: qgis/QGIS-Enhancement-Proposals#15
It's waiting for votes here: http://lists.osgeo.org/pipermail/qgis-developer/2014-December/036133.html

[strk]

I'd like to merge this to master before end of the year.
All raised points have been addressed, but only Paolo voted on the QEP so far.
Could partecipants to this thread please cast their vote too ?

[NathanW2]

@strk I'm happy for there to be no QEP for something like this. I know it's a bit hard to tell at times what does and doesn't need a QEP. So feel free to close the QEP and we can just work of this PR for comments. (again sorry it took me this long to raise that)

The code looks good to me but that isn't really my area and I don't have anything to test with. I need to talk to Martin tonight about some other things so let me see what he says.

[strk]

Thanks Nathan, QEP closed. Looking forward to Martin's response.

[NathanW2]

@strk @wonder-sk I have merged this for now so we can get some wider testing on it. I didn't see any real issues with it from me looking over it.

@strk it would be nice to see the points that Nyall raised considered, no point having duplicate logic for the same kind of thing if we can avoid it. Do you have any more time/money to make those tweaks?

[strk]

I've commented about Nyall points in a comment:
#1735 (comment)

Basically I don't see it possible to merge the two classes. Eventually it could be that the new class internally uses an enhancement version of the old one. New one is still needed as a virtual interface.