Fix aspect ratio computation in QgsLayoutItemMap::zoomToExtent #54827
Conversation
strk
added
Requires Tests!
strk
force-pushed
the
layoutitemmap-aspect_ratio
branch
from
857c388 to
f097620
Compare
strk
force-pushed
the
layoutitemmap-aspect_ratio
branch
2 times, most recently
from
d1e4819 to
3d16dbd
Compare
strk
removed
the
Requires Tests!
strk
changed the title
strk
force-pushed
the
layoutitemmap-aspect_ratio
branch
from
3d16dbd to
4420c57
Compare
tests/src/core/testqgslayoutmap.cpp
Outdated
| QVERIFY( mi.rect().isNull() ); // is this correct to expect ? | ||
| QVERIFY( mi.extent().isNull() ); // is this correct to expect ? | ||
| QCOMPARE( mi.scale(), 0 ); // is this correct ? |
There was a problem hiding this comment.
these tests are meaningless really, let's drop them. If the default is changed so that initial scale is 10000 in future then it's not a bug which needs a test to fail.
There was a problem hiding this comment.
It's important to know what to expect because the zoomToExtent functionality depends on the current extent so in order to test that I need to know what to expect as a current extent when the item map is constructed. Is it supposed to depend on the layout instance ? The constructor documentation doesn't say anything about what to expect as the initial state, is that to be considered undefined behavior ? I think it's good for tests to fail if defaults are changed, as users and other parts of the code are likely depending on defaults ?
There was a problem hiding this comment.
Maybe we should change defaults now if we believe the current defaults are not good. Like, should scale default to 1 instead of 0 ?
There was a problem hiding this comment.
considered undefined behavior
Yes, I think so. There's a LOT of setup you need to do to get a map item into a valid state, eg:
- set the position and size on the page
- set the map extent
- set the map crs
None of these are things we can set any meaningful defaults for, and it's all properties which aren't available through the layout at time of map item creation.
There was a problem hiding this comment.
should scale default to 1 instead of 0 ?
Scale is derived from the combination of map size & visible extent, it's not an explicit value.
(* that said, it would be very useful if map items and map settings had some mode to calculate the visible extent based on a fixed center point + defined scale. But it's not a trivial thing to introduce.)
There was a problem hiding this comment.
Yes, I think so. There's a LOT of setup you need to do to get a map item into a valid state, eg:
Would it make sense to expose an isValidState() method and throw an exception from method trying to access properties that would only make sense in "valid state" ? Also I see other tests ( like ::rendere() ) using QgsLayout.initializeDefaults(), is that what currently turns the state of the ItemMap valid ?
There was a problem hiding this comment.
Would it make sense to expose an isValidState() method
Yes! 💯
and throw an exception from method trying to access properties that would only make sense in "valid state"
That'd be an API break -- it'd have to wait for 4.0
QgsLayout.initializeDefaults(), is that what currently turns the state of the ItemMap valid ?
"Initializes an empty layout, e.g. by adding a default page to the layout. This should be called after creating
a new layout."
There was a problem hiding this comment.
Ok for now I've rebased against current master branch and removed the rect and scale checks from the constructor test, leaving the one for current extent, which is what I'm using in ::zoomToExtent test. The isValidState() may be better to do in a separate PR
strk
force-pushed
the
layoutitemmap-aspect_ratio
branch
from
4420c57 to
ece0fc1
Compare
|
Spellchecker is not happy with the way I spell "behavior", but doesn't tell what would make it happy. Is developers life not heavy enough that we deserve this ? :) |
I run the spellchecker locally and found how to spell that word (US english) |
Do not even try to retain aspect ratio of current extent when current extent is empty, just zoom to the requested extent. Includes unit test for both constructor and zoomToExtent
strk
force-pushed
the
layoutitemmap-aspect_ratio
branch
from
43483c5 to
62a610e
Compare
The rectangle should be checked for not being empty, not only null, to avoid division by zero
Extrapolated from the branch associated with the rectangle-null branch in GH-54646