[qt] Removed "Pay only the required fee" checkbox

GreatSock commented at 1:59 pm on May 19, 2018: contributor

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.

[qt] AmountField min/max value, allow empty option

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.

123466cd22

[qt] Removed pay only required fee checkbox

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.

4577166830

in src/qt/bitcoinamountfield.cpp:62 in 123466cd22 outdated

61+            valid = false;
62+            val = parse(input, &valid);
63+        }
64+
65+        if (valid) {
66+            if (val < min_amount) {

promag commented at 2:15 pm on May 19, 2018:

Use http://doc.qt.io/qt-5/qtglobal.html#qBound.

in src/qt/bitcoinamountfield.cpp:104 in 123466cd22 outdated

100     {
101         bool valid = false;
102         CAmount val = value(&valid);
103         val = val + steps * singleStep;
104-        val = qMin(qMax(val, CAmount(0)), BitcoinUnits::maxMoney());
105+        val = qMin(qMax(val, CAmount(min_amount)), max_amount);

promag commented at 2:15 pm on May 19, 2018:

Use http://doc.qt.io/qt-5/qtglobal.html#qBound.

promag commented at 2:16 pm on May 19, 2018: member

Concept ACK. Will test.

[qt] BitcoinAmountField use qBound 7908f18b13

GreatSock commented at 2:52 pm on May 19, 2018: contributor

Any idea why the Travis build failed after changing to qBound?

Edit: Nevermind! Seems to have passed now

laanwj added the label GUI on May 19, 2018

jonasschnelli commented at 8:41 am on May 25, 2018: contributor

Concept ACK

promag commented at 11:26 pm on June 1, 2018: member

It would be nice to see a previous screenshot. I forgot to test this 🙄

in src/qt/bitcoinamountfield.cpp:31 in 7908f18b13

24@@ -25,7 +25,10 @@ class AmountSpinBox: public QAbstractSpinBox
25     explicit AmountSpinBox(QWidget *parent):
26         QAbstractSpinBox(parent),
27         currentUnit(BitcoinUnits::BTC),
28-        singleStep(100000) // satoshis
29+        singleStep(100000), // satoshis
30+        allow_empty(true),
31+        min_amount(0),
32+        max_amount(BitcoinUnits::maxMoney())

MarcoFalke commented at 3:25 pm on July 22, 2018:

Please initialize non-static class members directly where they are declared.

in src/qt/bitcoinamountfield.cpp:161 in 7908f18b13

155@@ -129,6 +156,10 @@ class AmountSpinBox: public QAbstractSpinBox
156     CAmount singleStep;
157     mutable QSize cachedMinimumSizeHint;
158 
159+    bool allow_empty;
160+    CAmount min_amount;
161+    CAmount max_amount;

MarcoFalke commented at 3:26 pm on July 22, 2018:

Please prefix class members with m_, according to the developer notes.

MarcoFalke approved

MarcoFalke commented at 3:28 pm on July 22, 2018: member

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.

DrahtBot commented at 11:49 pm on July 22, 2018: member

DrahtBot closed this on Jul 22, 2018

DrahtBot reopened this on Jul 22, 2018

MarcoFalke added this to the milestone 0.18.0 on Jul 29, 2018

MarcoFalke commented at 4:07 pm on July 29, 2018: member

@GreatSock Are you still working on this?

MarcoFalke added the label Up for grabs on Aug 20, 2018

DrahtBot commented at 10:36 am on August 21, 2018: member

DrahtBot added the label Needs rebase on Aug 21, 2018

Sjors commented at 8:02 pm on September 7, 2018: member

Concept ACK, less UI clutter is great, and it gets rid of a duplicate tooltip.

Also thanks for switching to qBound.

To clarify the minimum fee a bit, as well as make way for a potential future decrease, maybe change the tooltip from:

Paying only the minimum fee is just fine as long as there is less transaction volume than space in the blocks. But be aware that this can end up in a never confirming transaction once there is more demand for bitcoin transactions than the network can process.

To:

When there is less transaction volume than space in the blocks, miners as well as relaying nodes may enforce a minimum fee. Paying only this minimum fee is just fine, but be aware that this can result in a never confirming transaction once there is more demand for bitcoin transactions than the network can process.

Removing the checkbox breaks horizontal alignment. The text needs be aligned with “Confirmation time target”:

Sjors commented at 8:40 am on October 30, 2018: member

@hebasto said on IRC:

provoostenator: hi! have you took #13280 over or it is still up for grabs?

Not yet, feel free to and I can review it.

fanquake commented at 9:25 am on October 30, 2018: member

I’ll close this then. hebasto can cherry-pick and open a new PR when ready.

fanquake closed this on Oct 30, 2018

MarcoFalke removed the label Needs rebase on Oct 30, 2018

MarcoFalke removed the label Up for grabs on Oct 30, 2018

jonasschnelli referenced this in commit 083f535470 on Nov 13, 2018

christiancfifi referenced this in commit b63de04c75 on Aug 27, 2021

christiancfifi referenced this in commit 8816d1aacc on Aug 27, 2021

christiancfifi referenced this in commit f084b84d41 on Aug 27, 2021

christiancfifi referenced this in commit 786dbd0c91 on Aug 28, 2021

christiancfifi referenced this in commit 8ca36a34a8 on Aug 29, 2021

christiancfifi referenced this in commit 4ae3653004 on Aug 29, 2021

christiancfifi referenced this in commit 1457eda124 on Aug 29, 2021

DrahtBot locked this on Sep 8, 2021

[qt] Removed “Pay only the required fee” checkbox #13280