RIP-Comm / sossoldi

"Sossoldi" is a wealth management / personal finance / Net Worth tracking app, made with Flutter.
MIT License
262 stars 73 forks source link

Allow decimal input in AddTransaction page. #111

Closed rcasula closed 11 months ago

rcasula commented 1 year ago

Add custom formatter to fix various inconsistencies

This PR fixes #95

GBergatto commented 1 year ago

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.

rcasula commented 1 year ago

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 commented 1 year ago

@rcasula Samsung A33 Screenshot_20230809_105319.jpg

rcasula commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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:

Let me better check on that though

rcasula commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 12 months ago

@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 commented 12 months ago

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 commented 11 months ago

@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 commented 11 months ago

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.