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

Use decimal value separator depending on locale ("." or ",") on EditTextActivity #2998

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gusakovgiorgi
Copy link

@gusakovgiorgi gusakovgiorgi commented Aug 12, 2023

When creating a new alarm and entering a decimal threshold value some European languages (Ukrainian, for example) languages use , for fractions. However, the xDrip keypad does not allow you to use ,. In this case, if you enter 4.3 it'll be parsed as 4,0. To avoid that we have to use a decimal line separator depending on the current locale.

Additionally, when alertThreshold.setKeyListener(DigitsKeyListener.getInstance("0123456789" + decimalValueSeparator)); is applied there is no need to set input type because DigitsKeyListener.getInstance modifies that anyway.

Part of the documentation for setKeyListener

Note that this method has significant and subtle interactions with soft keyboards and other input method: see KeyListener.getInputType() for important details. Calling this method will replace the current content type of the text view with the content type returned by the key listener

Use decimal value separator depending on locale ("." or ",")
@Navid200
Copy link
Collaborator

Navid200 commented Aug 12, 2023

Can I recreate the problem by choosing the Ukrainian language and then doing what?

@gusakovgiorgi
Copy link
Author

gusakovgiorgi commented Aug 12, 2023

Can I recreate the problem by choosing the Ukrainian language and then doing what?

Then go to the alert list -> edit or add a new alarm.
In the threshold enter 4.5 (units should be set to mmol/L)
Press save.
Value will be saved as 4.0

@gusakovgiorgi
Copy link
Author

Screenrecorder-2023-08-12-18-25-48-106.mp4

@gusakovgiorgi
Copy link
Author

That is expected behavior. If you choose mg/dL, the blood glucose parameter will be an integer. Your readings will be around 110.

If you want an alert at 4.4, you need to choose mmol/L. Then, the parameter will be a fractional with one decimal point.

I've chosen mmol/L.

@gusakovgiorgi
Copy link
Author

That is expected behavior. If you choose mg/dL, the blood glucose parameter will be an integer. Your readings will be around 110.

If you want an alert at 4.4, you need to choose mmol/L. Then, the parameter will be a fractional with one decimal point.

See the video above, please.

@gusakovgiorgi
Copy link
Author

gusakovgiorgi commented Aug 12, 2023

See the video above, please.

I did. Screenshot 2023-08-12 123954

If you watch further you'll see that I changed it to mmol/L and reproduced the issue.

@gusakovgiorgi
Copy link
Author

I changed the language in my xDrip. My unit is already mmol/L. I went to look at the alerts and can see that the alert threshold uses , instead of . to identify the fraction. I changed the fraction leaving the , in place and it worked fine.

Yes, if you leave the comma then it works. If you delete, then you can enter only "." and the fractional part will be dropped.
Also, if you create a new alarm then there won't be any predefined value with the ",". So you have to enter it from scratch.

I then replaced , with . and I can see that the fractional part is dropped. But, why would someone who uses the Ukrainian language in which , is used to represent fraction use . for that?

You can't use "," in this case, because the current implementation is locale independent and allows entering only dots from your keyboard.

I don't understand why anyone would have a problem with xDrip as is?

@gusakovgiorgi
Copy link
Author

You're right. The description might've been more elaborated. Sorry for taking your time.

@Navid200
Copy link
Collaborator

Thanks for reporting the problem and suggesting a fix.
Let's wait for a review to see if this is the best solution or not.

@Navid200 Navid200 requested a review from jamorham August 12, 2023 18:48
@jamorham
Copy link
Collaborator

This is a perfectly reasonable and useful change. The problem is that this particular activity is in a very old part of the code and in other places we have had to be careful with parsing numeric values containing separates other than . because they are not supported natively by java. There are also no unit test for this part of the code.
I previously created a simplistic utility method of tolerantParseDouble() which we then use to wrap parsing of decimals in many places. I would have to check that this change doesn't affect parsing and that these code paths are safe. I haven't done that yet.

@gusakovgiorgi
Copy link
Author

This is a perfectly reasonable and useful change. The problem is that this particular activity is in a very old part of the code and in other places we have had to be careful with parsing numeric values containing separates other than . because they are not supported natively by java. There are also no unit test for this part of the code.
I previously created a simplistic utility method of tolerantParseDouble() which we then use to wrap parsing of decimals in many places. I would have to check that this change doesn't affect parsing and that these code paths are safe. I haven't done that yet.

Thank you for the insights! The code that I used here is just for enabling entering decimal separator to the EditText. Currently, the value is still saved as , for some locales so this part didn't change.

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

Successfully merging this pull request may close these issues.

None yet

3 participants