Also export rule based labelling in SLD. Follows up to ticket #8925

[aaime]

Description

This pull request adds export of SLD for rule based labels.
While I was at it, found an issue with min/max scale denominator exports. While in the QgsMapLayer mMinScale is a min scale denominator, in labelling the min scale is a minimum scale expressed as its denominator (but the meaning is the opposite, a small scale denominator makes for a large scale!), so I fixed the export of those in labelling as well.

Checklist

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include fixes #11111 in the commit message next to the description
• Commits which add new features are tagged with [FEATURE] in the commit message
• Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
• I have read the QGIS Coding Standards and this PR complies with them
• This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit

[nyalldawson]

While in the QgsMapLayer mMinScale is a min scale denominator, in labelling the min scale is a minimum scale expressed as its denominator (but the meaning is the opposite, a small scale denominator makes for a large scale!), so I fixed the export of those in labelling as well.

That should have been fixed a couple of months back - can you point me to where the issue is?

[m-kuhn]

Generic input, this can be simplified to:

const QgsRuleBasedLabeling::RuleList rules = mRootRule->children();
for ( Rule *rule : rules )
{

[aaime]

Thanks, fixing it

[m-kuhn]

Can you remove all those empty @param and @brief (and document the ones that are important for this implementation)

[aaime]

Adding doc for all of them

[aaime]

For @nyalldawson, here we go.

See here for a comment talking about min scale denominator, but a variable called minScale:
https://github.com/qgis/QGIS/blob/master/src/core/qgsmaplayer.h#L1057
and then code using as a min scale denominator (not scale) in SLD export (tried to export from UI, it works, so I guess that variable really is a denominator?):
https://github.com/qgis/QGIS/blob/master/src/core/qgsmaplayer.cpp#L1438
And also isInScaleRange matches that supposition, its doc says that the "scale" parameter is a scale denominator:

However in qgspallabeling there are similarly named variables with the opposite meaning, mimuminScale is the actual minimum scale (not denominator), but expressed as the denominator:
https://github.com/qgis/QGIS/blob/master/src/core/qgspallabeling.h#L627
And oh, rule based labeling is consistent with the pal labeling too, see:
https://github.com/qgis/QGIS/blob/master/src/core/qgsrulebasedlabeling.h#L83

[aaime]

@m-kuhn feedback applied :-)

[aaime]

@m-kuhn @nyalldawson pull request updated, conflict removed, build is green. Anything else that needs to be done before merge? :-)

[rduivenvoorde]

@m-kuhn @nyalldawson is this ok for you?

I was thinking: should/do we have some sld output tests? I only can think of some tests to have a set of qml files, load them as style and then save them as sld and check schema validity (I think that is most important).

Do we already have a test which does some schema validity? If so I'm happy to try to add an output test ( if I find the docs/marks on how to do/run those :-) )

[aaime]

@rduivenvoorde I've prepared tests in python for every change made, they start with a qml and/or a in memory setup, generate SLD, and perform checks on the tags.

However, they are not testing schema validity, and they will not be schema valid as the outputs use in some places "VendorOption" which is an element that GeoTools/GeoServer introduced many years ago, but it's not part of the standard. Generation of VendorOption was already in the code base before I started working on the fixes, but of course I added more along the way (especially in text symbolizer) to make sure the style is as close as possible when imported in software based on GeoTools.

I guess that a strictly compliant SLD could be generated, add an option in the UI and pass it along as a hint in the SLD generation chain, of course that would significantly degrade what you can actually export (you'd lose most of the point symbols, parametric SVGs, good part of the text symbolizer, and a few other options along the way).

[aaime]

Oh no, more conflicts to solve. If I take the time to solve them will someone merge it? :-)

[elpaso]

Sure, please go ahead.

[rduivenvoorde]

@aaime can you have a look at this please? And ping me, I'll promiss I'm make it merged then

[aaime]

@rduivenvoorde @elpaso conflict fixed, all green :-)

[elpaso]

maybe just ( !mRootRule ), nullptr has the boolean value of false.

[aaime]

Oh ok, was not aware, fixed it.
Just one note, grepping the codebase seems that equality/inequality comparisons against nullptr are rather common:

~/devel/qgis$ git grep -i "== nullptr" | wc -l
44
~/devel/qgis$ git grep -i "!= nullptr" | wc -l
89

[elpaso]

well, not really:

ale@littlestar:~/dev/QGIS$ git grep -i "== nullptr" | grep -v astyle| wc -l
6
ale@littlestar:~/dev/QGIS$ git grep -i "!= nullptr" | grep -v astyle| wc -l
29

btw, there is not such a big difference, I just find it semantically clearer.

[elpaso]

@aaime looks good to me, just a minor change request from my side.
@rduivenvoorde I didn't check: do we "need-docs" for this change ?

[rduivenvoorde]

@elpaso nope, there is some docs already:

http://docs.qgis.org/testing/en/docs/user_manual/introduction/general_tools.html?highlight=sld#save-in-plain-text-file

it is just working better now...

pull :-)

[DelazJ]

note that this doc is rather about rule-based rendering and not labeling. Anyway, i'm not sure we need to go to these details in the docs (nice that all works now) but as a new feature it might be worth mentioning in the changelog, hence [feature] tag...

[elpaso]

@aaime thanks for your patience!

[aaime]

Awesome, thanks for merging :)