Fix $length in labels

[mhugo]

Description

This fixes https://issues.qgis.org/issues/19355.

The fix in itself is quite simple, but the main problem is to have access to distance and area units parameters from within QgsPalLayerSettings.
Since access to QgsProject::instance() is not recommended (for good reasons), I had to add members to QgsRenderContext and QgsMapSettings in order to propagate these parameters.

Review welcomed !

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

• 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 contain 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]

Thanks for looking at this one @mhugo!

I haven't looked over your pr yet, but I have been thinking about this issue for some time. The problem is that this same underlying issue keeps popping up over and over as more parts of code use expressions and don't have the proper setup for the distance area, and getting this info to them is more and more intrusive.

I think the ultimate solution here would be to take advantage of expression contexts, as they always have proper knowledge of both the project settings AND the layer settings, and in a safe way.

So we could store the ellipsoid, length, area settings from project in the project expression context scope, and then add the layer specific crs info when the layer scope gets added, and use these together to automatically create the correct QgsDistanceArea for the expression.

This would mean ALL expressions, regardless of where they are, would be guaranteed to have the correct ellipsoid setup and return consistent calculations.

What do you think about this approach?

[mhugo]

@nyalldawson Oh yes it seems way cleaner to use expression contexts for that ! Thanks for the suggestion. I'll work on it.

[mhugo]

@nyalldawson I've updated the PR. It seems cleaner.

I've added a "_project" variable in the project scope, as it is done with the "layer" variable in a layer scope, because I needed a way to access "project->transformContext" (see https://github.com/qgis/QGIS/pull/9063/files#diff-32b4ab0f3d67d8d2708ae40f27886ea9R328)

Tell me what you think

[nyalldawson]

This is looking great! I'm a little concerned about thread safety with the use of project and layer here though. I think we can avoid this by:

1. The layer CRS can be stored as a variable ("_layer_crs"? the _ prefix hides it from the expression builder, where it wouldn't be useful) in the layer expression context scope (since QgsCoordinateReferenceSystem is metatyped and can be stored as a QVariant), and then retrieved in initGeomCalculator. This would avoid the need to access the layer itself.

2. The project ellipsoid is just a string, so this could be set as a variable in the project expression context ("@project_ellipsoid"? I'd expose this one because it may be useful as layout footers/etc) and then retrieved from the context in initGeomCalculator

3. The project distance/area units could also be stored as integer variables in the same way

That leaves just the transform context to handle. I think we could safely make this metatyped and again store as just another (hidden) context variable. QgsCoordinateTransformContext is implicitly shared, so copies are cheap. This means we could eliminate the raw QgsProject access, and everything should be gauranteed thread safe.

What do you think?

Lastly, I think we may need to force a call to initGeomCalculator on first expression evaluation if the expression has not been previously prepared. This would gaurantee that ALL expressions are correct from now on, even if the expression hasn't been previously prepared.

[mhugo]

@nyalldawson Thanks for your feedback.

That leaves just the transform context to handle. I think we could safely make this metatyped and again store as just another (hidden) context variable

It would need to make QgsCoordinateTransformContext a QObject, but that should not hurt. One variable for each parameter is better than one opaque variable "project", I agree.

Lastly, I think we may need to force a call to initGeomCalculator on first expression evaluation if the expression has not been previously prepared. This would gaurantee that ALL expressions are correct from now on, even if the expression hasn't been previously prepared.

You mean just in case the contract stated in the documentation does not hold ? ("\note prepare() should be called before calling this method")

[nyalldawson]

It would need to make QgsCoordinateTransformContext a QObject

I don't think that's necessary for metatyping - e.g. QgsRectangle is metatyped but isn't a QObject.

You mean just in case the contract stated in the documentation does not hold

Yeah, just to be safe. I'd be surprised if even in core qgis we always prepare expressions, not to mention 3rd party scripts/plugins.*

• Now that I think of that, we should throw a qWarning when non prepared expressions are evaluated.

[mhugo]

@nyalldawson Updated PR. The following variables are now used:

• project_ellipsoid
• project_distance_units
• project_area_units
• _project_transform_context
• _layer_crs

I've also added a qWarning in evaluate if prepare() has not been called before

[nyalldawson]

Looks absolutely perfect to me... Great stuff!

[mhugo]

@nyalldawson Thanks for your enthusiasm :)

There was something else missing: setGeomCalculator(), setDistanceUnits() and setAreaUnits() on QgsExpression have now priority on variables set in the expression context for the building of a geometry calculator to avoid any regression.

And calling prepare() in evaluate() revealed a bug that should be fixed by 110eb98

[roya0045]

Is the warning necessary since the preparation is done in the function right after?

[mhugo]

@roya0045 I would say the warning is here to help us fix calls to QgsExpression and make sure prepare() is called before evaluate().