Fix layout corruption because of bad scalebar calculations #58677
Conversation
Eg when trying to show a 1m scalebar for a global map
Instead of just silently return "0", which is misleading and can result in data corruption/incorrect analysis results. Better to be safe and force callers to handle transformation errors appropriately. In this case we: - raise QgsProcessingExceptions when the failed measurement is coming from a processing tool, so that the user is forced to deal with the issue and we aren't providing meaningless/misleading measurements - report evaluation errors if the measurement is coming from a QGIS expression, so the user must appropriately handle the situation - for all other cases we currently just write a console error and maintain the current behavior of treating the measurement length as 0. TODO notes have been added to handle this cases better.
Instead of rendering a corrupted zero width scale bar and creating a layout item with 99999999999999mm width, which breaks the layout.
Instead of only calculating the scale at the bottom of the linked map, calculate at the top, middle and bottom and average the results
| // measure across top, center, and bottom of map, and return average of these calculated lengths | ||
| int validMeasureCount = 0; | ||
| double sumValidMeasures = 0; | ||
| for ( const double y : { mapExtent.yMinimum(), mapExtent.yMaximum(), ( mapExtent.yMaximum() - mapExtent.yMinimum() ) * 0.5 } ) |
There was a problem hiding this comment.
| for ( const double y : { mapExtent.yMinimum(), mapExtent.yMaximum(), ( mapExtent.yMaximum() - mapExtent.yMinimum() ) * 0.5 } ) | |
| for ( const double y : { mapExtent.yMinimum(), mapExtent.yMaximum(), ( mapExtent.yMaximum() + mapExtent.yMinimum() ) * 0.5 } ) |
Shouldn't it be ( mapExtent.yMaximum() + mapExtent.yMinimum() ) * 0.5?
Wouldn't it be better to make the new calculation logic optional, in order to avoid to break previous layout? Well aware map makers may have also specified in the layout: "scale calculated at the bottom of the map", and this will no longer be true printing the layout with newer QGIS versions. Anyway, I think the exact way the scale is calculated (which is different between layout and decorations) should be explicitly indicated in the docs. |
Tests failed for Qt 5One or more tests failed using the build from commit cf2be36 layoutscalebar_singlebox (singleBox)layoutscalebar_singleboxTest failed at singleBox at tests/src/core/testqgslayoutscalebar.cpp:119 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox/expected_layoutscalebar_singlebox.png (found 1074 pixels different) layoutscalebar_singlebox_linesymbol (singleBoxLineSymbol)layoutscalebar_singlebox_linesymbolTest failed at singleBoxLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:161 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_linesymbol/expected_layoutscalebar_singlebox_linesymbol.png (found 7548 pixels different) layoutscalebar_singlebox_fillsymbol (singleBoxFillSymbol)layoutscalebar_singlebox_fillsymbolTest failed at singleBoxFillSymbol at tests/src/core/testqgslayoutscalebar.cpp:202 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_fillsymbol/expected_layoutscalebar_singlebox_fillsymbol.png (found 4421 pixels different) layoutscalebar_singlebox_labelbelowsegment (singleBoxLabelBelowSegment)layoutscalebar_singlebox_labelbelowsegmentTest failed at singleBoxLabelBelowSegment at tests/src/core/testqgslayoutscalebar.cpp:232 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_labelbelowsegment/expected_layoutscalebar_singlebox_labelbelowsegment.png (found 2555 pixels different) layoutscalebar_singlebox_alpha (singleBoxAlpha)layoutscalebar_singlebox_alphaTest failed at singleBoxAlpha at tests/src/core/testqgslayoutscalebar.cpp:271 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_alpha/expected_layoutscalebar_singlebox_alpha.png (found 1233 pixels different) layoutscalebar_doublebox (doubleBox)layoutscalebar_doubleboxTest failed at doubleBox at tests/src/core/testqgslayoutscalebar.cpp:307 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox/expected_layoutscalebar_doublebox.png (found 1053 pixels different) layoutscalebar_doublebox_linesymbol (doubleBoxLineSymbol)layoutscalebar_doublebox_linesymbolTest failed at doubleBoxLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:349 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_linesymbol/expected_layoutscalebar_doublebox_linesymbol.png (found 6947 pixels different) layoutscalebar_doublebox_fillsymbol (doubleBoxFillSymbol)layoutscalebar_doublebox_fillsymbolTest failed at doubleBoxFillSymbol at tests/src/core/testqgslayoutscalebar.cpp:390 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_fillsymbol/expected_layoutscalebar_doublebox_fillsymbol.png (found 4429 pixels different) layoutscalebar_doublebox_labelcenteredsegment (doubleBoxLabelCenteredSegment)layoutscalebar_doublebox_labelcenteredsegmentTest failed at doubleBoxLabelCenteredSegment at tests/src/core/testqgslayoutscalebar.cpp:429 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_labelcenteredsegment/expected_layoutscalebar_doublebox_labelcenteredsegment.png (found 4332 pixels different) layoutscalebar_tick (tick)layoutscalebar_tickTest failed at tick at tests/src/core/testqgslayoutscalebar.cpp:497 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick/expected_layoutscalebar_tick.png (found 1076 pixels different) layoutscalebar_tick_linesymbol (tickLineSymbol)layoutscalebar_tick_linesymbolTest failed at tickLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:543 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick_linesymbol/expected_layoutscalebar_tick_linesymbol.png (found 9594 pixels different) layoutscalebar_datadefined (dataDefined)layoutscalebar_datadefinedTest failed at dataDefined at tests/src/core/testqgslayoutscalebar.cpp:615 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_datadefined/expected_layoutscalebar_datadefined.png (found 3477 pixels different) layoutscalebar_textformat (textFormat)layoutscalebar_textformatTest failed at textFormat at tests/src/core/testqgslayoutscalebar.cpp:672 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_textformat/expected_layoutscalebar_textformat.png (found 2007 pixels different) layoutscalebar_numericformat (numericFormat)layoutscalebar_numericformatTest failed at numericFormat at tests/src/core/testqgslayoutscalebar.cpp:706 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_numericformat/expected_layoutscalebar_numericformat.png (found 1394 pixels different) layoutscalebar_stepped (steppedLine)layoutscalebar_steppedTest failed at steppedLine at tests/src/core/testqgslayoutscalebar.cpp:749 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_stepped/expected_layoutscalebar_stepped.png (found 8398 pixels different) layoutscalebar_hollow (hollow)layoutscalebar_hollowTest failed at hollow at tests/src/core/testqgslayoutscalebar.cpp:806 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_hollow/expected_layoutscalebar_hollow.png (found 7401 pixels different) layoutscalebar_tick_subdivisions (tickSubdivisions)layoutscalebar_tick_subdivisionsTest failed at tickSubdivisions at tests/src/core/testqgslayoutscalebar.cpp:906 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick_subdivisions/expected_layoutscalebar_tick_subdivisions.png (found 4491 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
Tests failed for Qt 6One or more tests failed using the build from commit cf2be36 layoutscalebar_singlebox (singleBox)layoutscalebar_singleboxTest failed at singleBox at tests/src/core/testqgslayoutscalebar.cpp:119 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox/expected_layoutscalebar_singlebox.png (found 1067 pixels different) layoutscalebar_singlebox_linesymbol (singleBoxLineSymbol)layoutscalebar_singlebox_linesymbolTest failed at singleBoxLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:161 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_linesymbol/expected_layoutscalebar_singlebox_linesymbol.png (found 7554 pixels different) layoutscalebar_singlebox_fillsymbol (singleBoxFillSymbol)layoutscalebar_singlebox_fillsymbolTest failed at singleBoxFillSymbol at tests/src/core/testqgslayoutscalebar.cpp:202 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_fillsymbol/expected_layoutscalebar_singlebox_fillsymbol.png (found 4448 pixels different) layoutscalebar_singlebox_labelbelowsegment (singleBoxLabelBelowSegment)layoutscalebar_singlebox_labelbelowsegmentTest failed at singleBoxLabelBelowSegment at tests/src/core/testqgslayoutscalebar.cpp:232 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_labelbelowsegment/expected_layoutscalebar_singlebox_labelbelowsegment.png (found 2559 pixels different) layoutscalebar_singlebox_alpha (singleBoxAlpha)layoutscalebar_singlebox_alphaTest failed at singleBoxAlpha at tests/src/core/testqgslayoutscalebar.cpp:271 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_singlebox_alpha/expected_layoutscalebar_singlebox_alpha.png (found 1226 pixels different) layoutscalebar_doublebox (doubleBox)layoutscalebar_doubleboxTest failed at doubleBox at tests/src/core/testqgslayoutscalebar.cpp:307 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox/expected_layoutscalebar_doublebox.png (found 1049 pixels different) layoutscalebar_doublebox_linesymbol (doubleBoxLineSymbol)layoutscalebar_doublebox_linesymbolTest failed at doubleBoxLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:349 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_linesymbol/expected_layoutscalebar_doublebox_linesymbol.png (found 6939 pixels different) layoutscalebar_doublebox_fillsymbol (doubleBoxFillSymbol)layoutscalebar_doublebox_fillsymbolTest failed at doubleBoxFillSymbol at tests/src/core/testqgslayoutscalebar.cpp:390 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_fillsymbol/expected_layoutscalebar_doublebox_fillsymbol.png (found 4460 pixels different) layoutscalebar_doublebox_labelcenteredsegment (doubleBoxLabelCenteredSegment)layoutscalebar_doublebox_labelcenteredsegmentTest failed at doubleBoxLabelCenteredSegment at tests/src/core/testqgslayoutscalebar.cpp:429 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_doublebox_labelcenteredsegment/expected_layoutscalebar_doublebox_labelcenteredsegment.png (found 4337 pixels different) layoutscalebar_tick (tick)layoutscalebar_tickTest failed at tick at tests/src/core/testqgslayoutscalebar.cpp:497 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick/expected_layoutscalebar_tick.png (found 1067 pixels different) layoutscalebar_tick_linesymbol (tickLineSymbol)layoutscalebar_tick_linesymbolTest failed at tickLineSymbol at tests/src/core/testqgslayoutscalebar.cpp:543 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick_linesymbol/expected_layoutscalebar_tick_linesymbol.png (found 9604 pixels different) layoutscalebar_datadefined (dataDefined)layoutscalebar_datadefinedTest failed at dataDefined at tests/src/core/testqgslayoutscalebar.cpp:615 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_datadefined/expected_layoutscalebar_datadefined.png (found 3487 pixels different) layoutscalebar_textformat (textFormat)layoutscalebar_textformatTest failed at textFormat at tests/src/core/testqgslayoutscalebar.cpp:672 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_textformat/expected_layoutscalebar_textformat.png (found 2013 pixels different) layoutscalebar_numericformat (numericFormat)layoutscalebar_numericformatTest failed at numericFormat at tests/src/core/testqgslayoutscalebar.cpp:706 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_numericformat/expected_layoutscalebar_numericformat.png (found 1411 pixels different) layoutscalebar_stepped (steppedLine)layoutscalebar_steppedTest failed at steppedLine at tests/src/core/testqgslayoutscalebar.cpp:749 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_stepped/expected_layoutscalebar_stepped.png (found 8424 pixels different) layoutscalebar_hollow (hollow)layoutscalebar_hollowTest failed at hollow at tests/src/core/testqgslayoutscalebar.cpp:806 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_hollow/expected_layoutscalebar_hollow.png (found 7400 pixels different) layoutscalebar_tick_subdivisions (tickSubdivisions)layoutscalebar_tick_subdivisionsTest failed at tickSubdivisions at tests/src/core/testqgslayoutscalebar.cpp:906 Rendered image did not match tests/testdata/control_images/layout_scalebar/expected_layoutscalebar_tick_subdivisions/expected_layoutscalebar_tick_subdivisions.png (found 4517 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
github-actions
bot
added
the
stale
|
While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist. |
Avoids print layouts getting corrupted because of scalebars where the scale can't be calculated, which result in a massive 99999999999 cm wide scalebar object which effectively destroys a layout (page size calculation goes wonky, item placement breaks, etc).
This involves a few parts:
In future I would like to extend (3), and show an explicit warning when the calculated scale across the top/middle/bottom of the map differs by more than some threshold (eg 5%), advising the user that a scalebar is an inappropriate choice for their map and that a graticule should be used instead...