qt: Set C locale for amountWidget #14177

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:fix-amount-locale changing 1 files +5 −1

hebasto commented at 11:34 AM on September 9, 2018: member

Fix #13873
fanquake added the label GUI on Sep 9, 2018
in src/qt/transactionview.h:73 in bd5107055d outdated
```
  69 | @@ -70,6 +70,7 @@ class TransactionView : public QWidget
  70 |      QComboBox *watchOnlyWidget;
  71 |      QLineEdit *search_widget;
  72 |      QLineEdit *amountWidget;
  73 | +    QDoubleValidator *amountValidator;
```
luke-jr commented at 12:50 PM on September 9, 2018:

This isn't used anywhere. Probably we either need to delete it in cleanup, or don't need to store it at all.

hebasto commented at 1:25 PM on September 9, 2018:

@luke-jr QDoubleValidator uses system locale if its own one is not set. If system decimal separator is , then QDoubleValidator will not accept . at all. I've used the same approach as BitcoinAmountField class does: https://github.com/bitcoin/bitcoin/blob/af4fa72fac092480a2ae6cdd608e0b982c80f592/src/qt/bitcoinamountfield.cpp#L198

amountValidator member is declared in the same place where amountWidget is.

laanwj commented at 9:47 AM on September 13, 2018:

it is used: it's used to validate what is entered in the amount field

laanwj commented at 10:38 AM on September 13, 2018:

Oh I agree with @luke-jr actually; there's no need to store this in the object, just store it temporarily in a local variable in TransactionView::TransactionView while you need to manipulate it.
luke-jr changes_requested
luke-jr commented at 12:51 PM on September 9, 2018: member

Not sure if this is a good idea. What is the practical effect of setting the locale here?

Can a correct/point-based number still be entered and work properly?
hebasto cross-referenced this on Sep 9, 2018 from issue GUI: Filtering by amount problem in tx list if decimal separator is not a point by kristapsk
laanwj commented at 9:46 AM on September 13, 2018: member

we don't use the system locale for bitcoin amount entry, so this seems right

utACK bd5107055d393e14269faca84741c8167500e7b5

in src/qt/transactionview.cpp:109 in bd5107055d outdated

 105 | @@ -106,7 +106,9 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa
 106 |      } else {
 107 |          amountWidget->setFixedWidth(100);
 108 |      }
 109 | -    amountWidget->setValidator(new QDoubleValidator(0, 1e20, 8, this));
 110 | +    amountValidator = new QDoubleValidator(0, 1e20, 8, this);

laanwj commented at 10:39 AM on September 13, 2018:

e.g. do

QDoubleValidator *amountValidator = new QDoubleValidator(0, 1e20, 8, this);

hebasto force-pushed on Sep 13, 2018
hebasto commented at 1:12 PM on September 13, 2018: member

@luke-jr @laanwj Thank you for your reviews. Fixed.
hebasto commented at 5:42 AM on September 23, 2018: member

I just found out that there is another PR addressed this issue: #13278 @MarcoFalke the @DrahtBot did not report about a PR conflict. Is it ok?
MarcoFalke commented at 8:36 PM on September 24, 2018: member

@hebasto The bot can't check against PRs that conflict with master.
jonasschnelli commented at 6:55 PM on September 25, 2018: contributor

Unsure. Current master won't let me enter a comma (,)... it forces me to use "." for the dec. separator. While in this PR, I can enter a comma (gets ignored) which may confuse users. Thoughts?
Set C locale for amountWidget
Fix #13873
b0510d78ae
hebasto force-pushed on Sep 26, 2018
hebasto commented at 7:36 PM on September 26, 2018: member

@jonasschnelli Thank you for your review.

While in this PR, I can enter a comma (gets ignored) which may confuse users.

The comma , is a group separator in the C locale.

Thoughts?

Fixed. Please re-review.
hebasto renamed this:
~~Qt: Set locale for amountWidget~~
qt: Set C locale for amountWidget
on Sep 26, 2018
Sjors commented at 8:53 AM on October 8, 2018: member

On a macOS machine with Dutch locale (, decimal separator) When I enter an amount with a comma in the Send screen it's automatically converted to a dot, which is nice.

When I enter a comma in the search screen it's ignored, as @jonasschnelli noticed. That's annoying, but at least it fixes the bug. The user will see a list of amounts with . separators so they'll probably figure it out.

tACK b0510d7
laanwj merged this on Oct 18, 2018
laanwj closed this on Oct 18, 2018
laanwj referenced this in commit 32c5f188d4 on Oct 18, 2018
hebasto deleted the branch on Oct 18, 2018
deadalnix referenced this in commit d1dc331b84 on Nov 28, 2020
PastaPastaPasta referenced this in commit 166850ad5d on Jul 17, 2021
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13278 [qt] Fixed tx-view min amount typing period on locales using comma #13873 GUI: Filtering by amount problem in tx list if decimal separator is not a point