Add SubFeeFromAmount to options

psancheti110 commented at 4:33 pm on July 29, 2021: contributor

This PR adds SubFeeFromAmount option which lets the user select their preferred setting of whether fee for a transaction is to be subtracted from the amount or not for future transactions. The setting chosen by the user is remembered even when the GUI mode is turned off.

Functionality and Usage:

Go to Settings > Options > Wallet on Windows/Linux or bitcoin-qt > Preferences > Wallet on macOS.
The checkbox Subtract Fee From Amount corresponds to the added option SubFeeFromAmount.
The preferred setting intended to be the default for all future send transactions should be selected by the user.
Click on OK.
Go to the Send tab in the wallet.
You shall notice, any new Send transaction created will have the preferred setting as chosen by the user. (Try clicking on Add recipient or even restarting the Node in GUI)

Attaching ScreenRecordings to explain the added feature.

Master.mov: Master Branch

https://user-images.githubusercontent.com/54016434/127763378-be91837d-d0ab-4ae5-87c0-d303fa70a336.mov

PR.mov: PullRequest

https://user-images.githubusercontent.com/54016434/127763404-05b834c1-4082-4fbd-9b05-1528ac898a21.mov

Close #386

psancheti110 force-pushed on Jul 31, 2021

psancheti110 closed this on Aug 1, 2021

psancheti110 reopened this on Aug 1, 2021

psancheti110 renamed this:
~~[WIP] Add SubFeeFromAmount to options~~
Add SubFeeFromAmount to options
on Aug 1, 2021

psancheti110 marked this as ready for review on Aug 1, 2021

shaavan commented at 3:23 pm on August 2, 2021: contributor

tACK 4dc91813aca000336cec2cb40244d006a88b7d6a Tested on Ubuntu 20.04

This PR is adding a feature in the wallet settings so that the GUI will remember the default behavior of whether to subtract the fee from the transaction or not. As said by op this setting will be remembered even when the GUI is closed and reopened. This is done by setting a bool value based on if the checkbox is selected in settings or not, and then using that bool value to determine the default state of the SubFeeFromAmount option. The PR is doing what it describes in the description. It is also not modifying any part of the code, which it should be modifying. Adding some screenshots to show the proper working of the PR. The PR was compiled and ran on the signet chain.

The subtract fee from amount option in settings (highlighted in green)
Selection of “subtract fee from amount” option by default:

In my opinion, this PR is an improvement over the current master. For those who often use the “Subtract fee from amount” option, this feature can be quite handy.

in src/qt/forms/optionsdialog.ui:215 in 4dc91813ac outdated

210+           <widget class="QCheckBox" name="subFeeFromAmount">
211+            <property name="toolTip">
212+             <string>Whether to Subtact Fee From Amount or not.</string>
213+            </property>
214+            <property name="text">
215+             <string>Subtract &amp;Fee From Amount</string>

jarolrod commented at 2:07 am on August 3, 2021:

For any string in a UI file, you can add an extracomment field within the opening string tag. The value of this field is equivalent to a translator comment in our qt code

0             <string extracomment="An options menu setting to always subtract the fee from the amount a user is sending.">Subtract &amp;Fee From Amount</string>

in src/qt/forms/optionsdialog.ui:212 in 4dc91813ac outdated

205@@ -206,6 +206,16 @@
206           <string>Expert</string>
207          </property>
208          <layout class="QVBoxLayout" name="verticalLayout_2">
209+          <item>
210+           <widget class="QCheckBox" name="subFeeFromAmount">
211+            <property name="toolTip">
212+             <string>Whether to Subtact Fee From Amount or not.</string>

jarolrod commented at 2:08 am on August 3, 2021:

0             <string>Whether to always subtract fee from amount or not.</string>

jarolrod commented at 2:08 am on August 3, 2021: member

Concept ACK, please add translator comments

psancheti110 force-pushed on Aug 3, 2021

psancheti110 commented at 4:04 am on August 4, 2021: contributor

PR Updated from 4dc91813aca000336cec2cb40244d006a88b7d6a -> 17d7b21a862835a768fd436dc43da1bb60b4fa25 Addressed changes suggested by @jarolrod (PR#390 Comment)

in src/qt/sendcoinsentry.cpp:100 in 17d7b21a86 outdated

 96@@ -97,7 +97,9 @@ void SendCoinsEntry::clear()
 97     ui->payTo->clear();
 98     ui->addAsLabel->clear();
 99     ui->payAmount->clear();
100-    ui->checkboxSubtractFeeFromAmount->setCheckState(Qt::Unchecked);
101+    if(model && model->getOptionsModel()){

jarolrod commented at 1:23 am on August 5, 2021:

0    if(model && model->getOptionsModel()) {

hebasto commented at 6:41 am on August 5, 2021:

0    if (model && model->getOptionsModel()) {

There is a very useful tool to check commit code style before pushing – clang-format-diff.py.

in src/qt/optionsmodel.cpp:128 in 17d7b21a86 outdated

123@@ -124,6 +124,11 @@ void OptionsModel::Init(bool resetSettings)
124     if (!gArgs.SoftSetArg("-signer", settings.value("external_signer_path").toString().toStdString())) {
125         addOverriddenOption("-signer");
126     }
127+
128+    if (!settings.contains("SubFeeFromAmount")){

jarolrod commented at 1:24 am on August 5, 2021:

0    if (!settings.contains("SubFeeFromAmount")) {

in src/qt/forms/optionsdialog.ui:215 in 17d7b21a86 outdated

210+           <widget class="QCheckBox" name="subFeeFromAmount">
211+            <property name="toolTip">
212+             <string extracomment="This is a toolTip, Located at Options dialog under the Wallet tab. Select default setting for subtract the fee from the amount checkbox while creating new recipients.">Default setting to whether Subtact Fee From Amount or not.</string>
213+            </property>
214+            <property name="text">
215+             <string extracomment="Located at Options dialog under the Wallet tab. Select default setting for subtract the fee from the amount checkbox while creating new recipients.">Subtract &amp;Fee From Amount</string>

jarolrod commented at 2:00 am on August 5, 2021:

0             <string extracomment="An options menu setting to set subtracting the fee from the amount a user is sending as default.">Subtract &amp;Fee From Amount</string>

jarolrod commented at 6:15 pm on August 6, 2021:

@psancheti110 updated strings to remove the word always as it is not appropriate

in src/qt/forms/optionsdialog.ui:212 in 17d7b21a86 outdated

205@@ -206,6 +206,16 @@
206           <string>Expert</string>
207          </property>
208          <layout class="QVBoxLayout" name="verticalLayout_2">
209+          <item>
210+           <widget class="QCheckBox" name="subFeeFromAmount">
211+            <property name="toolTip">
212+             <string extracomment="This is a toolTip, Located at Options dialog under the Wallet tab. Select default setting for subtract the fee from the amount checkbox while creating new recipients.">Default setting to whether Subtact Fee From Amount or not.</string>

jarolrod commented at 2:32 am on August 5, 2021:

0             <string extracomment="Tooltip text for options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default or not.</string>

jarolrod commented at 6:16 pm on August 6, 2021:

updated suggestion, see #390 (review)

jarolrod commented at 2:32 am on August 5, 2021: member

Concept ACK

Existing recipients should respond to the ‘SubFeeFromAmountChanged’ signal.

There are scenarios where you have multiple recipients filled out. After you’ve filled them out, you now want all of them to ‘Subtract fee from amount’. If we allow the existing recipients to respond to the ‘SubFeeFromAmountChanged’ signal, then you don’t have to manually check every box. You can go into settings, set the option, come back and all of your recipients will not have the ‘Subtract Fee from amount’ box checked.

Similarly, if you already had the option set and created a bunch of recipients; you could unselect the option in settings and then all the boxes become unchecked.

jarolrod added the label UX on Aug 5, 2021

jarolrod added the label Wallet on Aug 5, 2021

hebasto commented at 6:38 am on August 5, 2021: member

@jarolrod

Existing recipients should respond to the ‘SubFeeFromAmountChanged’ signal.

I don’t agree.

The Options Dialog sets the default value of the “Subtract Fee…” setting. That means it is used only once per every recipient – just initialize its own checkbox. In other words, it is not supposed to be a batch switcher.

There are scenarios where you have multiple recipients filled out. After you’ve filled them out, you now want all of them to ‘Subtract fee from amount’. If we allow the existing recipients to respond to the ‘SubFeeFromAmountChanged’ signal, then you don’t have to manually check every box. You can go into settings, set the option, come back and all of your recipients will not have the ‘Subtract Fee from amount’ box checked.

I’m not sure if this scenario would be ever convenient for users.

in src/qt/optionsmodel.h:64 in 17d7b21a86 outdated

60@@ -61,6 +61,7 @@ class OptionsModel : public QAbstractListModel
61         Language,               // QString
62         UseEmbeddedMonospacedFont, // bool
63         CoinControlFeatures,    // bool
64+        SubFeeFromAmount,       //bool

hebasto commented at 6:42 am on August 5, 2021:

nit, why not follow the surrounding style?

0        SubFeeFromAmount,       // bool

psancheti110 force-pushed on Aug 6, 2021

psancheti110 commented at 5:26 pm on August 6, 2021: contributor

PR Updated from 17d7b21a862835a768fd436dc43da1bb60b4fa25 -> 50216bc8e28415ce9615b7ed1108c6730280d31b . Click here to check diff.

Addressed changes suggested by @hebasto about coding style using clang-format-diff.py

jarolrod commented at 6:21 pm on August 6, 2021: member

Approach ACK

I rescind my comments here about the behavior of this option. The appropriate place for a batch switch would be in the Send tab itself. Please update the translator comments 🥃

hebasto commented at 12:33 pm on August 7, 2021: member

Concept ACK.

Why a new “subFeeFromAmount” checkbox is placed within the “Expert” group? Shouldn’t it be an independent option?

psancheti110 commented at 7:50 pm on August 7, 2021: contributor

@hebasto

Why a new “subFeeFromAmount” checkbox is placed within the “Expert” group? Shouldn’t it be an independent option?

Agreed, It doesn’t belong to an “expert” group. That’s quite some observation there.

Will make the required changes and add.

psancheti110 force-pushed on Aug 7, 2021

psancheti110 commented at 8:53 pm on August 7, 2021: contributor

PR Updated from 50216bc8e28415ce9615b7ed1108c6730280d31b -> https://github.com/bitcoin-core/gui/commit/fb194c347e95e20bb682de75e51db084c7a4855c. Click here to check diff.

Addressed changes suggested by @hebasto for moving SubFeeFromAmount option outside “Expert” group and translator comments as suggested by @jarolrod.

hebasto commented at 8:55 pm on August 7, 2021: member

Approach ACK fb194c347e95e20bb682de75e51db084c7a4855c, going to test and review more closely.

in src/qt/forms/optionsdialog.ui:212 in fb194c347e outdated

207+        </property>
208+        <property name="text">
209+         <string extracomment="An options menu setting to set subtracting the fee from the amount a user is sending as default.">Subtract &amp;Fee From Amount</string>
210+        </property>
211+       </widget>
212+       </item>

hebasto commented at 7:40 pm on August 8, 2021:

nit: indentation (Qt Designer should handle it):

0       <item>
1        <widget class="QCheckBox" name="subFeeFromAmount">
2         <property name="toolTip">
3          <string extracomment="Tooltip text for options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default or not.</string>
4         </property>
5         <property name="text">
6          <string extracomment="An options menu setting to set subtracting the fee from the amount a user is sending as default.">Subtract &amp;Fee From Amount</string>
7         </property>
8        </widget>
9       </item>

in src/qt/optionsmodel.cpp:355 in fb194c347e outdated

350@@ -346,6 +351,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
351             return m_use_embedded_monospaced_font;
352         case CoinControlFeatures:
353             return fCoinControlFeatures;
354+        case SubFeeFromAmount:
355+            return m_sub_fee_from_amount;

hebasto commented at 7:45 pm on August 8, 2021:

Should these lines be guarded by #ifdef ENABLE_WALLET?

in src/qt/optionsmodel.cpp:501 in fb194c347e outdated

493@@ -487,6 +494,11 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
494             settings.setValue("fCoinControlFeatures", fCoinControlFeatures);
495             Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures);
496             break;
497+        case SubFeeFromAmount:
498+            m_sub_fee_from_amount = value.toBool();
499+            settings.setValue("SubFeeFromAmount", m_sub_fee_from_amount);
500+            Q_EMIT SubFeeFromAmountChanged(m_sub_fee_from_amount);
501+            break;

hebasto commented at 7:46 pm on August 8, 2021:

Should these lines be guarded by #ifdef ENABLE_WALLET?

in src/qt/forms/optionsdialog.ui:206 in fb194c347e outdated

199@@ -200,6 +200,16 @@
200        <string>W&amp;allet</string>
201       </attribute>
202       <layout class="QVBoxLayout" name="verticalLayout_Wallet">
203+      <item>
204+       <widget class="QCheckBox" name="subFeeFromAmount">
205+        <property name="toolTip">
206+         <string extracomment="Tooltip text for options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default or not.</string>

hebasto commented at 7:57 pm on August 8, 2021:

0         <string extracomment="Tooltip text for Options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default.</string>

in src/qt/forms/optionsdialog.ui:209 in fb194c347e outdated

204+       <widget class="QCheckBox" name="subFeeFromAmount">
205+        <property name="toolTip">
206+         <string extracomment="Tooltip text for options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default or not.</string>
207+        </property>
208+        <property name="text">
209+         <string extracomment="An options menu setting to set subtracting the fee from the amount a user is sending as default.">Subtract &amp;Fee From Amount</string>

hebasto commented at 8:00 pm on August 8, 2021:

0         <string extracomment="An Options window setting to set subtracting the fee from a sending amount as default.">Subtract &amp;fee from amount by default</string>

hebasto commented at 8:09 pm on August 8, 2021: member

Tested fb194c347e95e20bb682de75e51db084c7a4855c on Linux Mint 20.2 (Qt 5.12.8).

It works flawlessly, and it looks great:

Screenshot from 2021-08-08 22-54-14

psancheti110 force-pushed on Aug 9, 2021

psancheti110 commented at 8:27 pm on August 9, 2021: contributor

PR Updated from fb194c347e95e20bb682de75e51db084c7a4855c -> 54ce50987092b60a77677a4e59e8fd0a250b7bcd. Click here to check diff.

Addressed changes suggested by @hebasto. Moved the definitions of SubFeeFromAmount under #ifdef ENABLE_WALLET because this functionality cannot work without a wallet being loaded.

in src/qt/forms/optionsdialog.ui:54 in 54ce509870 outdated

63-         </widget>
64-        </item>
65-        <item>
66-         <widget class="QSpinBox" name="pruneSize"/>
67-        </item>
68+        <layout class="QHBoxLayout" name="horizontalLayout_Main_Prune">

jarolrod commented at 3:13 am on August 10, 2021:

nit, fixing the indentations could have gone in its own commit

in src/qt/forms/optionsdialog.ui:206 in 54ce509870 outdated

199@@ -200,6 +200,16 @@
200        <string>W&amp;allet</string>
201       </attribute>
202       <layout class="QVBoxLayout" name="verticalLayout_Wallet">
203+       <item>
204+        <widget class="QCheckBox" name="subFeeFromAmount">
205+         <property name="toolTip">
206+          <string extracomment="Tooltip text for Options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default.</string>

jarolrod commented at 3:15 am on August 10, 2021:

0          <string extracomment="Tooltip text for Options window setting that sets subtracting the fee from a sending amount as default.">Whether to set subtract fee from amount as default or not.</string>

jarolrod commented at 3:16 am on August 10, 2021: member

Almost there, one last thing: #390 (review)

psancheti110 force-pushed on Aug 10, 2021

psancheti110 commented at 6:19 am on August 10, 2021: contributor

PR Updated from 54ce50987092b60a77677a4e59e8fd0a250b7bcd -> ec25093a5793451aa997b5ff8a05d1631b41e05c. Click here to check diff.

psancheti110 force-pushed on Aug 10, 2021

hebasto approved

hebasto commented at 2:43 pm on August 10, 2021: member

ACK 7bf1a2bf569fe9356592b94531c0bbe0093ca302, tested on Linux Mint 20.2 (Qt 5.12.8).

Also I agree with the “qt, refactor: Fix indentation” commit (7bf1a2bf569fe9356592b94531c0bbe0093ca302) because every time optionsdialog.ui being opened in the Qt Designer, the latter tries to fix indentation, and produces unexpected changes.

jarolrod commented at 3:13 pm on August 10, 2021: member

tACK 7bf1a2bf569fe9356592b94531c0bbe0093ca302

tested on macOS 12 and Ubuntu 20.04. The setting works nicely and is a definite UX improvement. Nice job 🥃

shaavan commented at 4:43 pm on August 10, 2021: contributor

tACK 7bf1a2bf569fe9356592b94531c0bbe0093ca302 Tested on Ubuntu 20.04

Changes that OP made since my previous review:

Formatting is fixed according to clang-format.
Moved case SubFeeAmount under #ifdef ENABLE_WALLET to prevent this feature from working when wallet functionality is disabled.
The AddSubFeeAmount options have been moved out of the Expert menu.
Indentations in optionsdialog.ui have been fixed not to create unnecessary indentation changes when the file is opened in QTDesigner.

Screenshot: Screenshot from 2021-08-10 20-13-32

Just want to add one small thing; you should modify the video describing the PR, as it is still showing the option under the Expert menu :)

hebasto requested review from Sjors on Aug 10, 2021

hebasto requested review from meshcollider on Aug 10, 2021

in src/qt/optionsmodel.h:129 in 7bf1a2bf56 outdated

125@@ -123,6 +126,7 @@ class OptionsModel : public QAbstractListModel
126 Q_SIGNALS:
127     void displayUnitChanged(int unit);
128     void coinControlFeaturesChanged(bool);
129+    void SubFeeFromAmountChanged(bool);

meshcollider commented at 0:11 am on August 11, 2021:

Why is this Q_SIGNAL needed if it isn’t connect()ed anywhere?

psancheti110 commented at 9:35 am on August 11, 2021:

Addressed the suggested changes in the recent push.

qt: Add SubFeeFromAmount option ad28b66e98

qt, refactor: Fix indentation 62b125fd19

psancheti110 force-pushed on Aug 11, 2021

psancheti110 commented at 9:37 am on August 11, 2021: contributor

PR Updated from 7bf1a2bf569fe9356592b94531c0bbe0093ca302 -> 62b125fd197879f112322a1f67a318d6ab22e67a. Click here to check diff.

Addressed changes suggested by @meshcollider.

hebasto approved

hebasto commented at 11:18 am on August 11, 2021: member

re-ACK 62b125fd197879f112322a1f67a318d6ab22e67a, only removed the unused SubFeeFromAmountChanged signal since my previous review.

hebasto requested review from meshcollider on Aug 11, 2021

hebasto requested review from jarolrod on Aug 11, 2021

Talkless commented at 12:03 pm on August 11, 2021: none

tACK 62b125fd197879f112322a1f67a318d6ab22e67a, tested on Debian Sid with 5.15.2 and it works as described.

meshcollider commented at 10:48 pm on August 11, 2021: contributor

utACK 62b125fd197879f112322a1f67a318d6ab22e67a

hebasto merged this on Aug 11, 2021

hebasto closed this on Aug 11, 2021

psancheti110 commented at 5:54 pm on August 12, 2021: contributor

A big thanks to all the contributors who took out time to test and review my PR. Appreciate each and every comment made here by fellow mates 🙌🏻

sidhujag referenced this in commit 163a8bdf25 on Aug 15, 2021

bitcoin-core locked this on Aug 16, 2022

Add SubFeeFromAmount to options #390