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

qt: Set C locale for amountWidget #14177

Merged
merged 1 commit into from Oct 18, 2018

Conversation

Projects
None yet
7 participants
@hebasto
Contributor

hebasto commented Sep 9, 2018

Fix #13873

@fanquake fanquake added the GUI label Sep 9, 2018

@luke-jr

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?

Show outdated Hide outdated src/qt/transactionview.h Outdated
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 13, 2018

Member

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

utACK bd51070

Member

laanwj commented Sep 13, 2018

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

utACK bd51070

Show outdated Hide outdated src/qt/transactionview.cpp Outdated
@hebasto

This comment has been minimized.

Show comment
Hide comment
@hebasto

hebasto Sep 13, 2018

Contributor

@luke-jr @laanwj
Thank you for your reviews.
Fixed.

Contributor

hebasto commented Sep 13, 2018

@luke-jr @laanwj
Thank you for your reviews.
Fixed.

@hebasto

This comment has been minimized.

Show comment
Hide comment
@hebasto

hebasto Sep 23, 2018

Contributor

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?

Contributor

hebasto commented Sep 23, 2018

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 24, 2018

Member

@hebasto The bot can't check against PRs that conflict with master.

Member

MarcoFalke commented Sep 24, 2018

@hebasto The bot can't check against PRs that conflict with master.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Sep 25, 2018

Member

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?

Member

jonasschnelli commented Sep 25, 2018

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?

@hebasto

This comment has been minimized.

Show comment
Hide comment
@hebasto

hebasto Sep 26, 2018

Contributor

@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.

Contributor

hebasto commented Sep 26, 2018

@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 hebasto changed the title from Qt: Set locale for amountWidget to qt: Set C locale for amountWidget Sep 26, 2018

@Sjors

This comment has been minimized.

Show comment
Hide comment
@Sjors

Sjors Oct 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

Member

Sjors commented Oct 8, 2018

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 laanwj merged commit b0510d7 into bitcoin:master Oct 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #14177: qt: Set C locale for amountWidget
b0510d7 Set C locale for amountWidget (Hennadii Stepanov)

Pull request description:

  Fix #13873

Tree-SHA512: ef26b35ef83c3a87ebd90650f6d833b00a24f6c114b68fe01acd4a14d1f5bdec066f438eb7781c1e55c32640838c54e00b8f082c390639ade8d9a58830833d4a

@hebasto hebasto deleted the hebasto:fix-amount-locale branch Oct 18, 2018

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