Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uniform the decimal places number in Identify tool #7836

Closed
wants to merge 2 commits into from
Closed

Uniform the decimal places number in Identify tool #7836

wants to merge 2 commits into from

Conversation

agiudiceandrea
Copy link
Contributor

@agiudiceandrea agiudiceandrea commented Sep 8, 2018

Description

Now, in QGIS 3 and also 2.18, the Length, Area and Perimeter values are displayed always with 3 decimal places, while the other values are displayed according to the settings in Project Properties | General | Coordinate Display | Precision.
image

This commit lets the Length, Area and Perimeter values be displayed as the others are.

Fixes #20107

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

Make uniform the number of decimal places of values displayed in "Derived" item tree of Identify Tool.
@nirvn
Copy link
Contributor

nirvn commented Sep 9, 2018

Any chance we could (re-?)add thousands separator to facilitate reading?

@nirvn
Copy link
Contributor

nirvn commented Sep 9, 2018

Example on how to go at it:
1f31a97

@nirvn
Copy link
Contributor

nirvn commented Sep 9, 2018

I'd update the formatDistance and formatArea functions, that'll benefit other parts (measurement tools)

@nyalldawson
Copy link
Collaborator

One thing we need to account for here is very small distances/perimeters, and ensure that they do not display as 0.000. instead we should switch to scientific notation whenever the value is > 0 but would display as 0.000

to reflect the changes in the number of decimal places of Length, Area and Perimeter values displayed by Identity tool
@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Sep 9, 2018

@nirvn, working on formatDistance and formatArea, please also consider that:

QgsUnitTypes/QgsDistanceArea formatDistance and formatArea (and then the homonymous ones, of both QgsMeasureDialog and QgsMapToolIdentify, which are based on them) rely on qgsRound(), through QgsUnitTypes::scaledDistance and QgsUnitTypes::scaledArea, to round measurements to a specified number of decimal places.

It seems to me that there is a flaw in qgsRound() which leads to inconsistent results of the Measure tool (and Identify tool also) in QGIS 3:

see the difference underlined in red between segment length (rounded by QLocale().toString()) and total length (rounded by formatDistance through qgsRound()) values
image

In fact, in qgsround() definition

inline double qgsRound( double number, double places )
   {
    int scaleFactor = std::pow( 10, places );
    return static_cast<double>( static_cast<qlonglong>( number * scaleFactor + 0.5 ) ) / scaleFactor;
   }

scaleFactor is declared as Int and thus it can correctly store up to the 9th power of 10, consequently the rounding provides incorrect results from the 9th decimal place onwards.

Since it is allowed to set up to 12 as number of decimal places in Setting->Options->Map Tools (and moreover in Project Properties->General there is no upper limit to the decimal places allowed to be set!),
scaleFactor should be qlonglong (while places could be Int or less; and the static_cast<qlonglong> could be qulonglong, provided that qgsRound() is only used to round measurements, as it seems to be now).
I don't know if there is a better approach to round numbers avoiding any overflow.

By the way, in QGIS 2.18 this inconsistency problem is not present, since both formatDistance and formatArea rely on QString().arg(), which works similarly to QLocale().toString(), to round measurements, instead of on qgsRound().

@m-kuhn
Copy link
Member

m-kuhn commented Sep 11, 2018

If qlonglong scaleFactor solves an issue for you I don't see a reason not to change it.

@nyalldawson nyalldawson added the UX label Sep 12, 2018
@nyalldawson nyalldawson added this to the 3.4 milestone Sep 12, 2018
@agiudiceandrea
Copy link
Contributor Author

@m-kuhn, taking better account of any side effects of qlonglong scaleFactor, now it does not seem to me the right fix anymore. Please have a look at my post in QGIS-developer

@m-kuhn
Copy link
Member

m-kuhn commented Sep 14, 2018

I don't remember exactly why it was introduced, but possibly because of an issue with rounding around Qt 5.6 (to string conversion didn't trim to a reasonable length). Would relying on toString() now give acceptable results?

@nyalldawson nyalldawson modified the milestones: 3.4, 3.4.0 Sep 14, 2018
@agiudiceandrea
Copy link
Contributor Author

The qgsRound() bug was fixed with PR #7941

@nyalldawson
Copy link
Collaborator

Looking good!

The only remaining thing to test (and add a unit check for) is what happens when the value is small enough and would be shown as 0.000 (even when it's non-zero, .e.g. 0.0001 ). In this case we need to either increase the number of decimals for that value, or (better?) show as scientific notation.

@stale
Copy link

stale bot commented Oct 4, 2018

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

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 4, 2018
@stale
Copy link

stale bot commented Oct 11, 2018

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.

@stale stale bot closed this Oct 11, 2018
@agiudiceandrea
Copy link
Contributor Author

I've filed a bug report (#20107) about the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants