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: 3723 - now always replaces decimal separator for number format #3734

Merged
merged 7 commits into from
Feb 23, 2023

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Feb 23, 2023

Impacted files:

  • strings_helper.dart: removed now useless methods
  • text_input_formatters_helper.dart: now works in all cases, regardless of the presence of group separator in the target language, given that we don't use group separators in our decimal number formats anyway

What

  • The automatic number field fix (decimal separator tap dancing) was not working at all in some cases.
  • For instance, if the app locale was en_US, it was disabled.
  • That meant that on a French device with an US app, the user could type 6,85 which in US was parsed as 685.
  • The reason behind that was that for locale where group separator is , (and decimal is .) the string fix was problematic.
  • But in Smoothie we don't use group separator in the number format anyway.
  • For the record I was only able to test that on a test app, not in Smoothie, for reasons I will not explain again.
  • That means that additional tests would be appreciated, especially in the bug cases with a , device and a . app. Typically, when the user is (was) able to type 6,85 while the app language was set to en_US.

Fixes bug(s)

# Conflicts:
#	packages/smooth_app/pubspec.lock
#	packages/smooth_app/pubspec.yaml
Impacted files:
* `strings_helper.dart`: removed now useless methods
* `text_input_formatters_helper.dart`: now works in all cases, regardless of the presence of group separator in the target language, given that we don't use group separators in our decimal number formats anyway
@teolemon
Copy link
Member

Thank you very much @monsieurtanuki
I'm going to thoroughly take time to test this time

@monsieurtanuki monsieurtanuki merged commit fd868c7 into openfoodfacts:develop Feb 23, 2023
@monsieurtanuki
Copy link
Contributor Author

Thank you @teolemon for the review!
I've also fixed #3735.

@@ -107,10 +103,9 @@ class DecimalSeparatorRewriter extends TextInputFormatter {
/// Replaces a "." by a "," or a "," by a "." only if
/// the group separator is an empty character
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment about the group separator being an empty character should now be removed I guess

///
/// Also, if a separator is already displayed, it will be move to the new
/// Also, if a separator is already displayed, it will be moved to the new
/// position
class DecimalSeparatorRewriter extends TextInputFormatter {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have unit tests for those functions, as it would be easy to break something that works today in future changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad plural translation Nutrition amounts are saved without decimal separator 1.51% → 151%
3 participants