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

gui: fix visual "overflow" of amount input. #13284

Merged
merged 1 commit into from May 24, 2018

Conversation

Projects
None yet
8 participants
@brandonrninefive
Copy link
Contributor

brandonrninefive commented May 20, 2018

Fixes #13231.

I was able to reproduce this bug within my own Fedora 27 VM. Following @jonasschnelli's advice, I first tried to change setAlignment(Qt::AlignRight); to setAlignment(Qt::AlignLeft);, however, I realized that this wouldn't fix the underlying overflow problem, as it would only make it easier to see the most significant digits under certain scenarios. The reason for the overflow is that Fedora uses plus and minus buttons on the Qt spin box class, rather than up and down arrows, which is what happens on most other operating systems. These plus and minus buttons take up more width, and therefore provide less space for text.

The solution I went with was the second suggestion by @jonasschnelli, which was to just increase the maximum width of the amount box. After some experimentation, 240 seemed to be the smallest max width that would allow as many digits as one would want in the amount box without overflow, even with the plus and minus buttons in Fedora.

Please let me know if there are any issues with this PR and I will work to fix them. Thank you!

@fanquake fanquake added the GUI label May 20, 2018

@GreatSock

This comment has been minimized.

Copy link
Contributor

GreatSock commented May 20, 2018

utACK

I think you need to use the "fixes" or "closes" keyword before the referenced issue to make sure it closes on merge.

@brandonrninefive

This comment has been minimized.

Copy link
Contributor Author

brandonrninefive commented May 20, 2018

@GreatSock Done!

@brandonrninefive brandonrninefive changed the title Fix for Issue #13231 - [gui] visual "overflow" of amount input. Qt: Fix for Issue #13231 - [gui] visual "overflow" of amount input. May 20, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented May 21, 2018

utACK 5f3cbde.

@brandonrninefive

This comment has been minimized.

Copy link
Contributor Author

brandonrninefive commented May 23, 2018

@marcoagner Tagging you here just so you're aware of the PR. Thanks!

@marcoagner

This comment has been minimized.

Copy link
Contributor

marcoagner commented May 24, 2018

ACK 5f3cbde.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented May 24, 2018

I did a quick test with a Fedora VM with master (6378eef):
fedora - unfixed

and this PR (5f3cbde):
fedora - fixed

Also tested on macOS master (6378eef):
macos unfixed

and this PR (5f3cbde):
macos fixed

where I couldn't reproduce the problem (as per the original issue), and the fix looks fine.

tACK 5f3cbde

@fanquake fanquake changed the title Qt: Fix for Issue #13231 - [gui] visual "overflow" of amount input. gui: fix visual "overflow" of amount input. May 24, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented May 24, 2018

utACK 5f3cbde. Good to have some white space around the amounts!

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 24, 2018

Build: https://bitcoin.jonasschnelli.ch/build/625

utACK 5f3cbde.
Someone willing to test this on Windows?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 24, 2018

utACK 5f3cbde

@laanwj laanwj merged commit 5f3cbde into bitcoin:master May 24, 2018

1 check passed

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

laanwj added a commit that referenced this pull request May 24, 2018

Merge #13284: gui: fix visual "overflow" of amount input.
5f3cbde Increased max width of amount field to prevent number overflow bug. (Brandon Ruggles)

Pull request description:

  Fixes #13231.

  I was able to reproduce this bug within my own Fedora 27 VM. Following @jonasschnelli's advice, I first tried to change `setAlignment(Qt::AlignRight);` to `setAlignment(Qt::AlignLeft);`, however, I realized that this wouldn't fix the underlying overflow problem, as it would only make it easier to see the most significant digits under certain scenarios. The reason for the overflow is that Fedora uses plus and minus buttons on the Qt spin box class, rather than up and down arrows, which is what happens on **most** other operating systems. These plus and minus buttons take up more width, and therefore provide less space for text.

  The solution I went with was the second suggestion by @jonasschnelli, which was to just increase the maximum width of the amount box. After some experimentation, 240 seemed to be the smallest max width that would allow as many digits as one would want in the amount box without overflow, even with the plus and minus buttons in Fedora.

  Please let me know if there are any issues with this PR and I will work to fix them. Thank you!

Tree-SHA512: 155f34cec74af46ec1fe723a5241798d8e15607a4e1cdc493014dcc0ae9818a001c7901831168b5f26a6953ec5a992e4a67c57db1ad377bcf10f12941688ee93

@brandonrninefive brandonrninefive deleted the brandonrninefive:ui_amount_overflow_fix branch May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.