-
Notifications
You must be signed in to change notification settings - Fork 38.8k
[qt] Removed "Pay only the required fee" checkbox #13280
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
Conversation
This adds functions for specifing a min/max value for a BitcoinAmountField. These options only affect user input, so it's still possible to use setValue to set values outside of the min/max range. The existing value will not be changed when calling these functions even if it's out of range. The min/max range will be reinforced when the field loses focus. This also adds an allow empty function which specifies if the field is allowed to be left empty by the user. If set to false the field will be set to the minimum allowed value if it's empty when focus is lost.
Removes the pay only required fee checkbox from the custom transaction fee section in the SendCoinsDialog. It's replaced by a label giving a more general warning about low fees. The custom fee input box now has a minimum value equal to the minimum required fee. Before a value below the minimum fee could be entered which was confusing since the minimum fee would still be paid even though a lower amount was entered.
promag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. Will test.
src/qt/bitcoinamountfield.cpp
Outdated
| } | ||
|
|
||
| if (valid) { | ||
| if (val < min_amount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/qt/bitcoinamountfield.cpp
Outdated
| CAmount val = value(&valid); | ||
| val = val + steps * singleStep; | ||
| val = qMin(qMax(val, CAmount(0)), BitcoinUnits::maxMoney()); | ||
| val = qMin(qMax(val, CAmount(min_amount)), max_amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Any idea why the Travis build failed after changing to qBound? Edit: Nevermind! Seems to have passed now |
|
Concept ACK |
|
It would be nice to see a previous screenshot. I forgot to test this 🙄 |
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK. Please adjust the code to the developer notes (see two comments)
Also, might want to squash the third commit into one of the earlier ones.
Finally, add empty new lines after the git commit subject line. Otherwise, the commit subject can not be parsed properly.
| singleStep(100000), // satoshis | ||
| allow_empty(true), | ||
| min_amount(0), | ||
| max_amount(BitcoinUnits::maxMoney()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please initialize non-static class members directly where they are declared.
|
|
||
| bool allow_empty; | ||
| CAmount min_amount; | ||
| CAmount max_amount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefix class members with m_, according to the developer notes.
| The last travis run for this pull request was 64 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
|
@GreatSock Are you still working on this? |
| Needs rebase |
|
Concept ACK, less UI clutter is great, and it gets rid of a duplicate tooltip. Also thanks for switching to To clarify the minimum fee a bit, as well as make way for a potential future decrease, maybe change the tooltip from:
To:
Removing the checkbox breaks horizontal alignment. The text needs be aligned with "Confirmation time target": @GreatSock if you don't have time for this in the next month or so, I can take it over. |
|
I'll close this then. hebasto can cherry-pick and open a new PR when ready. |
a16f44c qt: Remove "Pay only required fee" checkbox (Hennadii Stepanov) 8711cc0 qt: Improve BitcoinAmountField class (Hennadii Stepanov) Pull request description: Ref #13280 This PR removes the "Pay only the required fee..." checkbox from the custom transaction fee section in the "Send" tab. Instead, a minimum value will be enforced on the custom fee input box. All comments from #13280 are addressed. Before:  After:  cc: @promag @MarcoFalke @Sjors Tree-SHA512: 073577d38d8353b10e8f36fb52e3c6e81dd45d25d84df9b9e4f78f452ff0bdbff3e225bdd6122b5a03839ffdcc2a2a08175f81c2541cf2d12918536abbfa3fd1

This removes the "Pay only the required fee" checkbox from the custom transaction fee section in the SendCoinsDialog. Instead, a minimum value will be enforced on the custom fee input box.
Before, a user could enter a fee below the minimum required fee but upon pressing send the resulting transaction would still pay the minimum fee. Now, if a user enters a value below the minimum fee the custom fee input box will be set to the minimum fee.
Where the checkbox used to be, there is now a more general warning about low fees. The tooltip is still the same as before.