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.

Sign up for GitHub

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

Jump to bottom

Fix decimal point issues on raster properties forms, Fixes #33859 #37136

Merged
merged 2 commits into from
Jul 14, 2020
Merged

Fix decimal point issues on raster properties forms, Fixes #33859 #37136

merged 2 commits into from
Jul 14, 2020

Conversation

SebastienPeillet
Copy link
Contributor

@SebastienPeillet SebastienPeillet commented Jun 11, 2020 •
edited
Loading

Description

Issue explained and discussed here : #33859

QGIS use QDoubleValidator on QLineEdit in several forms, including raster layer forms.

As QValidator is initialized with the current locale, the expected decimal point is determined by the locale.

For exemple in french, it' is a comma and not a point.

Sometimes the string value is converted to double value, but not with the QLocale().toDouble() method. In this case the string value can't be converted.

I fixed the bug on raster properties form (but I expect it exists in some other form). The affected QLineEdit are :

  • the min and max range QLineEdits in gray single band, pseudo color single band, color multi band forms
  • the transparency QLineEdit

I also fixed values display in the same forms plus palette form

To fix it , I create a new validator, called QgsDoubleValidator (as the Qgslonglongvalidator) using the same principle that use QgsFieldValidator for double type.

The regular expression allows to use the point and the locale decimal point as decimal point

Futhermore a static method toDouble is created to convert the string to double whatever if it is the point or the locale decimal point that is used.

I started as I will fix a bug, but now maybe it's more a feature...

@github-actions github-actions bot added this to the 3.14.0 milestone Jun 11, 2020
elpaso
elpaso requested changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@elpaso elpaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please:

  • add a cpp implementation file
  • tests with group separator and C locale fallback
  • in the new header file: class doxygen documentation for the class and for the methods

@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch 2 times, most recently from a52df08 to 4bc4e2b Compare June 14, 2020 16:11
new validator to handle permissive double writing in form; fix raster…
893bfff 
… properties form

QgsDoubleValidator modification after review

update doc
@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch from 4bc4e2b to 893bfff Compare June 14, 2020 16:19
@SebastienPeillet
Copy link
Contributor Author

@elpaso @nyalldawson thanks for your reviews ! i'm way more satisfy to this PR with your advices.

nyalldawson
nyalldawson reviewed Jun 15, 2020
View reviewed changes
src/gui/qgsdoublevalidator.cpp Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.cpp Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.cpp Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.cpp Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.cpp Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.h Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.h Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.h Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.h Outdated Show resolved Hide resolved
src/gui/qgsdoublevalidator.h Outdated Show resolved Hide resolved
@nyalldawson nyalldawson added the NOT FOR MERGE Don't merge! label Jun 17, 2020
@nyalldawson
Copy link
Collaborator

Too risky for 3.14.0 -- deferred

@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch 3 times, most recently from e2ec46f to cbebe29 Compare June 18, 2020 12:56
@SebastienPeillet
Copy link
Contributor Author

Too risky for 3.14.0 -- deferred

Yes, this is what I wanted to say by writing I started as I will fix a bug, but now maybe it's more a feature...

@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch from cbebe29 to bfba394 Compare June 18, 2020 13:03
@nyalldawson nyalldawson removed the NOT FOR MERGE Don't merge! label Jun 19, 2020
@nyalldawson nyalldawson modified the milestones: 3.14.0, 3.16.0 Jun 19, 2020
@stale
Copy link

stale bot commented Jul 6, 2020

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 Jul 6, 2020
@elpaso elpaso removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 11, 2020
@elpaso
Copy link
Contributor

elpaso commented Jul 11, 2020

@SebastienPeillet can you please make the requested changes so that we can finally merge?

@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch from bfba394 to fd6dcee Compare July 13, 2020 13:03
@SebastienPeillet
Copy link
Contributor Author

@SebastienPeillet can you please make the requested changes so that we can finally merge?

@elpaso It's done, sorry I didn't see the remained requested changes before (hidden in the conversation by default). Thanks for the reminder !

elpaso
elpaso reviewed Jul 13, 2020
View reviewed changes
src/gui/qgsdoublevalidator.h Outdated
/**
* Constructor for QgsDoubleValidator.
*
* bottom is the minimal range limit accepted by the validator
Copy link
Contributor

@elpaso elpaso Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* bottom is the minimal range limit accepted by the validator
* \param bottom the minimal range limit accepted by the validator

can you please use the \param syntax for arguments? (here and everywhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad... I add it !

elpaso
elpaso reviewed Jul 13, 2020
View reviewed changes
src/gui/qgsdoublevalidator.h Outdated
QValidator::State validate( QString &input, int & ) const override SIP_SKIP;

/**
* Evaluates input QString validity according to QRegularExpression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Evaluates input QString validity according to QRegularExpression
* Evaluates \a input string validity according to QRegularExpression

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and all other occurrences too...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done ! To avoid new corrections on this next time, is there a tacit rule for italic in documentation, can't find reference of it in Qgis coding standard ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about italic, it's for "Use this command to refer to member arguments in the running text." instead of using \param you can use \a for an inline description of the argument.

@SebastienPeillet SebastienPeillet force-pushed the several_fix_locale_raster_properties branch from fd6dcee to 887d544 Compare July 14, 2020 10:47
@elpaso elpaso merged commit 15a4c39 into qgis:master Jul 14, 2020
@agiudiceandrea agiudiceandrea mentioned this pull request Jan 31, 2021
Closed
@agiudiceandrea agiudiceandrea mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson left review comments

@elpaso elpaso elpaso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SebastienPeillet @nyalldawson @elpaso