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

Fix QgsDoubleValidator for positive sign and for negative and exponential signs in some locales (Fix #52341) #52923

Merged
merged 3 commits into from
May 5, 2023

Conversation

agiudiceandrea
Copy link
Contributor

@agiudiceandrea agiudiceandrea commented Apr 27, 2023

Description

Some locales (see #52341 (comment) for the list) use the character Unicode Character “−” (U+2212) as negative sign, instead of the - character Unicode Character “-” (U+002D).

This PR makes the QgsDoubleValidator accept also the Unicode Character “−” (U+2212) as negative sign in the locales in which it is used.

Fixes #52341.

It also makes QgsDoubleValidator accept the plus sign in front of positive numbers and accept the E character as exponential sign and the Unicode Character “е” (U+0435) as exponential sign in the locales in which it is used (uk_UA Ukranian).

NOTE: looking at the original "parametrized" regex PERMISSIVE_DOUBLE = R"(-?[\d]{0,1000}([\.%1][\d]{0,1000})?(e[+-]?[\d]{0,%2})?)"; it seems to me it has other various issues:

  1. it does not allow the plus sign in front of the value: should I also fix it? [ADDRESSED IN THIS PR]
  2. the %2 parameter should be the number of allowed decimal digits but it seems to me it is in the wrong place: it should be in the second {0,1000} (which is the decimals part) and not after the e (which is the exponent part). @SebastienPeillet could you please confirm that it is due to a typo in your PR Fix decimal point issues on raster properties forms, Fixes #33859 #37136 and that I should also fix it? [I WILL FIX IT IN A DIFFERENT PR]
  3. it is only allowed the e exponent character, but it should be allowed also the E character and the character returned by QLocale().exponential() (actually it seems only uk_UA Ukranian laguage uses the character Unicode Character “е” (U+0435) instead of Unicode Character “e” (U+0065) for the exponent): should I also fix it in the same way? [ADDRESSED IN THIS PR]
  4. it does not accept values with the group separator, so there are issues editing a value when the "Show group (thousand) separator" option is checked in the setting [I WILL OPEN AN ISSUE REPORT]

Some locales use the − character [Unicode Character “−” (U+2212)] as negative sign, instead of the - character [Unicode Character “-” (U+002D)]
@github-actions github-actions bot added this to the 3.32.0 milestone Apr 27, 2023
@agiudiceandrea agiudiceandrea changed the title Fix QgsDoubleValidator for negative sign in some locales (Fixe #52341) Fix QgsDoubleValidator for negative sign in some locales (Fix #52341) Apr 27, 2023
@agiudiceandrea agiudiceandrea added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Apr 27, 2023
@elpaso
Copy link
Contributor

elpaso commented Apr 28, 2023

Description

Some locales (see #52341 (comment) for the list) use the character Unicode Character “−” (U+2212) as negative sign, instead of the - character Unicode Character “-” (U+002D).

This PR makes the QgsDoubleValidator accept also the Unicode Character “−” (U+2212) as negative sign in the locales in which it is used.

This PR needs to be backported.

Fixes #52341.

NOTE: looking at the "parametrized" regex PERMISSIVE_DOUBLE = R"([-%3]?[\d]{0,1000}([\.%1][\d]{0,1000})?(e[+\-%3]?[\d]{0,%2})?)"; it seems to me it has other various issues:

1. it does not allow the plus sign in front of the value: should I also fix it?

Yes, please.

2. the `%2` parameter should be the number of allowed decimal digits but it seems to me it is in the wrong place: it should be in the second {0,1000} (which is the decimals part) and not after the `e` (which is the exponent part). @SebastienPeillet could you please confirm that it is due to a typo in your PR [Fix decimal point issues on raster properties forms,  Fixes #33859 #37136](https://github.com/qgis/QGIS/pull/37136) and that I should also fix it?

Yes, please.

Also, we should probably reduce the default limits to fit into a double : approx 17 decimal mantissa digits and +/-308 as the min/max exponent.

3. it is only allowed the `e` exponent character, but it should be allowed also the `E` character and the character returned by `QLocale().exponential()` (actually it seems only uk_UA Ukranian laguage uses the character `Unicode Character “е” (U+0435)` instead of `Unicode Character “e” (U+0065)` for the exponent): should I also fix it in the same way?

Yes, please, make sure they are tested (I think they are not)

4. it does not accept values with the group separator, so there are issues editing a value when the "Show group (thousand) separator" option is checked in the setting

This is probably not necessary, the separator is removed before testing the expression:

double QgsDoubleValidator::toDouble( const QString &input, bool *ok )
{
  double value = QLocale().toDouble( input, ok );

  if ( ! *ok )
  {
    value = QLocale( QLocale::C ).toDouble( input, ok );
  }
  // Still non ok? Try without locale's group separator
  if ( ! *ok && !( QLocale().numberOptions() & QLocale::NumberOption::OmitGroupSeparator ) )
  {
    value = QLocale( ).toDouble( QString( input ).replace( QLocale().groupSeparator(), QString() ), ok );
  }
  return value ;
}

Allow the positive sign in front of a positive number
@agiudiceandrea agiudiceandrea changed the title Fix QgsDoubleValidator for negative sign in some locales (Fix #52341) WIP: Fix QgsDoubleValidator for negative sign in some locales (Fix #52341) Apr 29, 2023
@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented Apr 29, 2023

Also, we should probably reduce the default limits to fit into a double : approx 17 decimal mantissa digits

@elpaso, while I don't know why the range {0,1000} was used (maybe @SebastienPeillet remembers if there is a specific reason), I don't think we should be more restrictive: e.g. "0.00000000000000000000000000000123e30" (with 33 decimals) is a string representing a valid Double and it is correctly converted to 1.23 by QLocale.toDouble.

and +/-308 as the min/max exponent

I think that even an exponent > 308 or < -308 should be allowed, since e.g. the string "0.000000000000000000000000000001234e330" is correctly converted to the valid Double 1.234e+300 by QLocale.toDouble and e.g. a string like "1234" + "0"*997 + "e-700" is correctly converted to the valid Double 1.234e+300 by QLocale.toDouble.

Anyway, I think we could safely enough restrict the number of digits for the exponent part to 3 (with a minimum of 1), i.e. with a range {1,3}.

@elpaso
Copy link
Contributor

elpaso commented Apr 29, 2023

Also, we should probably reduce the default limits to fit into a double : approx 17 decimal mantissa digits

@elpaso, while I don't know why the range {0,1000} was used (maybe @SebastienPeillet remembers if there is a specific reason), I don't think we should be more restrictive: e.g. "0.00000000000000000000000000000123e30" (with 33 decimals) is a string representing a valid Double and it is correctly converted to 1.23 by QLocale.toDouble.

You are right, let's keep it. Maybe useful to add a few test cases for these "extreme" cases.

and +/-308 as the min/max exponent

I think that even an exponent > 308 or < -308 should be allowed, since e.g. the string "0.000000000000000000000000000001234e330" is correctly converted to the valid Double 1.234e+300 by QLocale.toDouble and e.g. a string like "1234" + "0"*997 + "e-700" is correctly converted to the valid Double 1.234e+300 by QLocale.toDouble.

Anyway, I think we could safely enough restrict the number of digits for the exponent part to 3 (with a minimum of 1), i.e. with a range {1,3}.

It makes sense.

Also allow to use <E> and the locale exponential sign (for Ukrainian language)
@agiudiceandrea agiudiceandrea marked this pull request as draft May 3, 2023 04:07
@agiudiceandrea agiudiceandrea changed the title WIP: Fix QgsDoubleValidator for negative sign in some locales (Fix #52341) Fix QgsDoubleValidator for positive sign and negative and exponential signs in some locales (Fix #52341) May 3, 2023
@agiudiceandrea agiudiceandrea marked this pull request as ready for review May 3, 2023 22:39
@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented May 3, 2023

Hi @elpaso, I've fixed in this PR the issue reported in #52341 and also the issues I've found about the plus sign in front of a positive number and the exponential signs.

I prefer to open a different PR for the issue about the max number of decimals incorrectly handled, as it could lead to a behaviour different from the current one for some widgets.

I also prefer to open an issue report for the issue about the values with the group separator: it seems QgsDoubleValidator has been implemented to only "tolerate" numbers with group separators, but not "accept" them, as you can read in

// QgsDoubleValidator doesn't expect group separator but it tolerates it,
// so the result will be QValidator::Intermediate and not QValidator::Acceptable
and in #37136 (comment)

@agiudiceandrea agiudiceandrea changed the title Fix QgsDoubleValidator for positive sign and negative and exponential signs in some locales (Fix #52341) Fix QgsDoubleValidator for positive sign and for negative and exponential signs in some locales (Fix #52341) May 4, 2023
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.

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to change negative values under raster symbology
2 participants