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

Allow decimal input in AddTransaction page. #111

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

rcasula
Copy link
Contributor

@rcasula rcasula commented Aug 2, 2023

Add custom formatter to fix various inconsistencies

This PR fixes #95

Add custom formatter to fix various inconsistencies
Copy link
Collaborator

@theperu theperu left a comment

Choose a reason for hiding this comment

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

Good work! From what I can see now it is working as expected. Unfortunately I currently have an issue with the simulator on iOS so I would wait for someone else to look at it and the code

@GBergatto
Copy link
Contributor

I noticed that it's possible to add a period after a comma, but not the other way around (as it should be). Entering 12,34.45 will result in 12.34.

We should put agree on some rules for which symbol to use as the decimal separator. In some places (e.g. home page) we use the comma, while in others (e.g. transactions and graph page) the period. Since the app is in English, we should stick with the period everywhere.

I think the user should not be allowed to enter the comma (which should be use as the thousands separator) to prevent them from adding it in the wrong place (e.g. 1,00.99). Instead, numbers should be formatted automatically to include the thousand separators where needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Entering a comma after a period is not allowed, as it's supposed to be. However, it is possible to add a period after a comma, but this results in some weird behavior. For example 12,34.56 becomes 12.34 and 12,3.45 becomes 12.30.

I guess the best fix would be to use the period as decimal separator across the whole app and prevent the user from entering a comma, which should be added automatically as thousands separator.

lib/pages/add_page/add_page.dart Outdated Show resolved Hide resolved
@rcasula
Copy link
Contributor Author

rcasula commented Aug 9, 2023

I noticed that it's possible to add a period after a comma, but not the other way around (as it should be). Entering 12,34.45 will result in 12.34.

We should put agree on some rules for which symbol to use as the decimal separator. In some places (e.g. home page) we use the comma, while in others (e.g. transactions and graph page) the period. Since the app is in English, we should stick with the period everywhere.

I think the user should not be allowed to enter the comma (which should be use as the thousands separator) to prevent them from adding it in the wrong place (e.g. 1,00.99). Instead, numbers should be formatted automatically to include the thousand separators where needed.

Hi @GBergatto,
quite interesting one. How could you input a comma? Are you using a simulator or a real device? which platform?
This is definitely a bug that I should guard against, but since I've enabled the right decimal keyboard it shouldn't be possible to input the comma, as the decimal separator is hardcoded.

As a side note: I've also opened an issue regarding the decimal separator. In my opinion it shouldn't be hardcoded, instead we should support the default decimal separator of the configured app language (if we plan to support other languages other than English).

@GBergatto
Copy link
Contributor

@rcasula Samsung A33
Screenshot_20230809_105319.jpg

@rcasula
Copy link
Contributor Author

rcasula commented Aug 9, 2023

@GBergatto I've pushed a possible fix in order to accept only the decimal separator. Can you try and let me know if it works correctly?

@mikev-cw
Copy link
Collaborator

Hello, and thank you!
Tested on iOS physical device (iPhone 13 Pro, iOS 16.5.1c).
What I see is a numeric keyboard with a comma on the bottom left (which was missing before), but when I tap it nothing happens, and no decimal separator is added to the amount i'm entering.
In the screen I typed "21,8". Not sure if this issue came out only after your last commit.

IMG_4201

@mikev-cw
Copy link
Collaborator

We should put agree on some rules for which symbol to use as the decimal separator. In some places (e.g. home page) we use the comma, while in others (e.g. transactions and graph page) the period. Since the app is in English, we should stick with the period everywhere.

Definitely a critical issue, never noticed before.
I think we should make a distinction between:

  • what an user can input (we maybe need some filter/sanitize functions)
  • how we save that data in our DB (this is already set by sqlite: is float with only . as decimal separator)
  • and how we show that data to the app (this is something we can determine ourself, I suggest easy peasy . as thousands separator, and , as decimal separator. Later we can change this format when application can run on other locales).

Let me better check on that though

@rcasula
Copy link
Contributor Author

rcasula commented Aug 10, 2023

Hello, and thank you!

Tested on iOS physical device (iPhone 13 Pro, iOS 16.5.1c).

What I see is a numeric keyboard with a comma on the bottom left (which was missing before), but when I tap it nothing happens, and no decimal separator is added to the amount i'm entering.

In the screen I typed "21,8". Not sure if this issue came out only after your last commit.

IMG_4201

Hi,
That's because in the last commit I've disabled the comma as a separator, in order to fix the issue reported above.
In this case, could it be that the keyboard's/phone's locale is not English? iOS should use the app's locale to determine the decimal separator, maybe the project is miscondigured.. I need to take a look at it.

That said, we should consider retrieving the decimal separator from the phone's or app locale. What do you think?

@mikev-cw
Copy link
Collaborator

In this case, OS and keyboard locales are both italian.
In addition, this warning is shown on XCode when keyboard pulls up:

2023-08-10 09:47:17.588068+0200 Runner[36525:13107949] Can't find keyplane that supports type 8 for keyboard iPhone-PortraitChoco-DecimalPad; using 27344_PortraitChoco_iPhone-Simple-Pad_Default

I think that in this stage, the app should determine dates and numbers formats itself, independently on how the device is configured: We're developing mobile + desktop, on Apple, Android, Windows and Linux... handle all systems settings could allocate too much effort for a small team eager to release a first Alpha..

@GBergatto
Copy link
Contributor

That said, we should consider retrieving the decimal separator from the phone's or app locale. What do you think?

That's definitely the best way to do it, but I think it's too complex for the stage we're at. For now, I think we should use the dot as decimal separator, as that's the default in English.

@GBergatto
Copy link
Contributor

@mikev-cw

  • and how we show that data to the app (this is something we can determine ourself, I suggest easy peasy . as thousands separator, and , as decimal separator. Later we can change this format when application can run on other locales).

This seems quite counter intuitive to me. I would stick with the English default of dot for decimal separator and comma as thousands separator.

@mikev-cw
Copy link
Collaborator

@mikev-cw

  • and how we show that data to the app (this is something we can determine ourself, I suggest easy peasy . as thousands separator, and , as decimal separator. Later we can change this format when application can run on other locales).

This seems quite counter intuitive to me. I would stick with the English default of dot for decimal separator and comma as thousands separator.

@GBergatto
Seeing currency format like € 12,345,678.90 could be a bit weird for non Americans, that's why I initially suggested the opposite. But yeah, indeed this make no sense when we've developed the whole app in an English context.. Agreed with you

@rcasula
Copy link
Contributor Author

rcasula commented Aug 10, 2023

@GBergatto @mikev-cw
To summarise:
In the meantime we're going to stick with the . for decimal separator and the , for the thousands separator.

As for the user input I would only allow the decimal separator and the numbers. This way is easier and maintainable.
Regarding how we show the data, that's a different topic: in this case we could also use the thousands separator.

How would that sounds?

EDIT
Separators aside, there could be a misconfiguration on the iOS side. If we support only English, the keyboard, and the regional formats should be independent of the device's language and stick with the English format. Never saw that behaviour in one of my projects, I will investigate further.
Unfortunately I was wrong, I never encountered this issue in an iOS project maybe just because I've always worked with localised projects.
After some research and tests (also creating a brand new vanilla SwiftUI app) I've discovered that the keyboard is always localised with the device's locale, not the app's one.

The solution could be not so easy as we initially thought 😅
A possible solution could be: we should allow the user to input as he wants. Then we parse the input from the user's device locale into English.
In this way though, how we should handle validation errors?

@rcasula
Copy link
Contributor Author

rcasula commented Aug 11, 2023

@GBergatto @mikev-cw I've pushed a fix, can you try it please?

It's really a shame that this is the only solution I've found, so simple thing yet so hard.
Basically the issue is that is not easy to find out the keyboard's language, which depends on the device's region, NOT the locale. I've done a test where I've set the simulator's locale and region to Italian, then the app's locale (Localizations.localeOf(context) is en even though the keyboards shows the comma as decimal separator.

Do you know a better solution other than replacing the comma with a period?

@GBergatto
Copy link
Contributor

I've just tested it on my Samsung phone. The keyboard I get has both comma and period. From what I can tell, it work pretty well. The only issue is that prices are currently displayed with a comma as separator on the home page. So, when editing a price the comma gets transformed into a dot and then back into a comma.

As you @rcasula said, this solution is very strict. Instead of processing the number as it's being written and preventing the user from adding certain chars, we could validate the string only when the "add transaction" button is pressed and display an error message if the format is invalid.

We would still need a way to guarantee that the format entered by the user is consistent with his locale. For example, if the user has set the app to use the dot as separator, he shouldn't enter values with a comma.

@rcasula
Copy link
Contributor Author

rcasula commented Aug 13, 2023

Yes, you are correct. Still feels hacky to me and I don't really like it.
The displayed format it's an easy thing, and I would address it in a separate PR, fixing it consistently in the entire app.

The user input though.. that's the real issue. I was not able to find a way to know which device region the user has set, without that we can't know if it has the dot or the comma as the decimal separator.
Do you know a better way?

@GBergatto
Copy link
Contributor

By comparing these two questions on stackoverflow, I wrote this function that should return a string containing the decimal separator of your locale. I've tested it on my phone and it seems to work.

import 'dart:io';
import 'package:intl/number_symbols_data.dart' show numberFormatSymbols;

String getDecimalSeparator() {
  return numberFormatSymbols[Platform.localeName]?.DECIMAL_SEP ?? "";
}

@rcasula
Copy link
Contributor Author

rcasula commented Aug 16, 2023

By comparing these two questions on stackoverflow, I wrote this function that should return a string containing the decimal separator of your locale. I've tested it on my phone and it seems to work.

import 'dart:io';
import 'package:intl/number_symbols_data.dart' show numberFormatSymbols;

String getDecimalSeparator() {
  return numberFormatSymbols[Platform.localeName]?.DECIMAL_SEP ?? "";
}

I've tried it, it works fine for simple cases, but not for the cases we were discussing earlier.
That function retrieves the decimal separator related to the device's language, which is not entirely correct. The decimal separator should be retrieved by the device's regional settings.
E.g. I have the language set to Italian, but the region set to US; the device's keyboard shows me the ..
Or even a more common use case: Language set to English and region set to Italy (or another region that has the , separator); the function above returns . as a separator, while the keyboard shows the comma.
Keep in mind that I'm using an iPhone simulator to perform these edge cases.

@mikev-cw
Copy link
Collaborator

mikev-cw commented Aug 24, 2023

@GBergatto @mikev-cw I've pushed a fix, can you try it please?

Works good on Android sim. Not tried iPhone sim.
On phisical iPhone there's same problem of this comment.
Also when i edit an existing transaction, number format of the transaction amount is with a comma, and amount is not editable (nothing happens when a do a del, or backspace, or any number)

...seems that a simple task managed to be a huge task... 🤣

@GBergatto
Copy link
Contributor

@mikev-cw

Also when i edit an existing transaction, number format of the transaction amount is with a comma, and amount is not editable (nothing happens when a do a del, or backspace, or any number)

I was experiencing the same thing when trying to edit the amount of a transaction. I'm working on turning the modal to add/edit transactions into a page and now it seems to work. However, I get this warning in my console when I edit the amount

I/IMM_LC  (25079): showSoftInput(View,I)
I/IMM_LC  (25079): ssi() - flag : 0 view : com.example.sossoldi reason = SHOW_SOFT_INPUT
I/IMM_LC  (25079): ssi() view is not EditText
D/InsetsController(25079): show(ime(), fromIme=true)

I'll learn more about Riverpod to see if I can come up with a solution.

Also, prices are currently using the comma as separator. So, when you go to edit one, the comma is replaced with a dot while editing but then replaced again with a comma by currencyToNum inside functions.dart

@rcasula
Copy link
Contributor Author

rcasula commented Aug 27, 2023

Hi @mikev-cw and @GBergatto ,

I suggest to separate the issues in two separate PRs: this one, regarding the user input, and the other one, regarding the user facing number formatting.

@mikev-cw

On phisical iPhone there's same problem of this comment.

Are you sure? I've tested on a physical iPhone with both locale and region set to Italian: Keyboard shows comma, but when pressing it its being replaced with a period.
I've tested it also on an iOS simulator and an Android one. Seems working fine, not pretty but still.

...seems that a simple task managed to be a huge task... 🤣

Indeed.... We are still on-time to revert back to native, just saying... 😌 (joking)

@mikev-cw
Copy link
Collaborator

@rcasula Double checked: dot on iOS simulator, comma on phisical device (iOS 16.6 on iPhone 13 Pro).
I removed and rebuild the app on the device.
Is there something I could check for helping on that?
We can speak about this on Discord if it's more convenient for you.

@mikev-cw
Copy link
Collaborator

mikev-cw commented Sep 12, 2023

Checked with @rcasula: i was building on an old commit... Damn! Sorry guys for time wasting.
Merging this, thank you!!
We will handle all other things with separate issues.

@mikev-cw mikev-cw merged commit 5e3f2dd into RIP-Comm:main Sep 12, 2023
1 check passed
@rcasula rcasula deleted the bugfix/fix-keyboard-decimal-input branch November 1, 2023 10:34
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.

Wrong Keyboard when adding a transaction
4 participants