Add SubFeeFromAmount to options #390

psancheti110 · 2021-07-29T16:33:34Z

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

Master.mov

PR.mov: PullRequest

PR.mov

Close #386

shaavan

tACK 4dc9181
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.

jarolrod

Concept ACK, please add translator comments

src/qt/forms/optionsdialog.ui

psancheti110 · 2021-08-04T04:04:12Z

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

jarolrod

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.

src/qt/sendcoinsentry.cpp

src/qt/optionsmodel.cpp

src/qt/forms/optionsdialog.ui

hebasto · 2021-08-05T06:38:33Z

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

src/qt/sendcoinsentry.cpp

src/qt/optionsmodel.h

psancheti110 · 2021-08-06T17:26:49Z

PR Updated from 17d7b21 -> 50216bc . Click here to check diff.

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

jarolrod · 2021-08-06T18:21:02Z

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 · 2021-08-07T12:33:56Z

Concept ACK.

Why a new "subFeeFromAmount" checkbox is placed within the "Expert" group? Shouldn't it be an independent option?

psancheti110 · 2021-08-07T19:50:19Z

@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 · 2021-08-07T20:53:38Z

PR Updated from 50216bc -> fb194c3. 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

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

hebasto

Tested fb194c3 on Linux Mint 20.2 (Qt 5.12.8).

It works flawlessly, and it looks great:

src/qt/forms/optionsdialog.ui

src/qt/optionsmodel.cpp

src/qt/forms/optionsdialog.ui

psancheti110 · 2021-08-09T20:27:40Z

PR Updated from fb194c3 -> 54ce509. 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.

jarolrod

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

src/qt/forms/optionsdialog.ui

psancheti110 · 2021-08-10T06:19:13Z

PR Updated from 54ce509 -> ec25093. Click here to check diff.

hebasto

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

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

jarolrod · 2021-08-10T15:13:52Z

tACK 7bf1a2b

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

shaavan

tACK 7bf1a2b
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:

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 :)

src/qt/optionsmodel.h

psancheti110 · 2021-08-11T09:37:05Z

PR Updated from 7bf1a2b -> 62b125f. Click here to check diff.

Addressed changes suggested by @meshcollider.

hebasto

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

Talkless · 2021-08-11T12:03:53Z

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

meshcollider

utACK 62b125f

psancheti110 · 2021-08-12T17:54:18Z

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 🙌🏻

psancheti110 force-pushed the i386_0729 branch 2 times, most recently from 1d52b5d to 4dc9181 Compare July 31, 2021 20:11

psancheti110 closed this Aug 1, 2021

psancheti110 reopened this Aug 1, 2021

psancheti110 changed the title ~~[WIP] Add SubFeeFromAmount to options~~ Add SubFeeFromAmount to options Aug 1, 2021

psancheti110 marked this pull request as ready for review August 1, 2021 08:03

shaavan reviewed Aug 2, 2021

View reviewed changes

jarolrod reviewed Aug 3, 2021

View reviewed changes

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

psancheti110 force-pushed the i386_0729 branch from 4dc9181 to 17d7b21 Compare August 3, 2021 15:04

jarolrod reviewed Aug 5, 2021

View reviewed changes

src/qt/sendcoinsentry.cpp Outdated Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

jarolrod added UX All about "how to get things done" Wallet labels Aug 5, 2021

hebasto reviewed Aug 5, 2021

View reviewed changes

src/qt/sendcoinsentry.cpp Outdated Show resolved Hide resolved

hebasto reviewed Aug 5, 2021

View reviewed changes

src/qt/optionsmodel.h Outdated Show resolved Hide resolved

psancheti110 force-pushed the i386_0729 branch from 17d7b21 to 50216bc Compare August 6, 2021 09:03

psancheti110 force-pushed the i386_0729 branch from 50216bc to fb194c3 Compare August 7, 2021 20:48

hebasto reviewed Aug 7, 2021

View reviewed changes

hebasto reviewed Aug 8, 2021

View reviewed changes

psancheti110 force-pushed the i386_0729 branch from fb194c3 to 54ce509 Compare August 9, 2021 20:16

jarolrod reviewed Aug 10, 2021

View reviewed changes

src/qt/forms/optionsdialog.ui Show resolved Hide resolved

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

psancheti110 force-pushed the i386_0729 branch from 54ce509 to ec25093 Compare August 10, 2021 06:16

psancheti110 force-pushed the i386_0729 branch from ec25093 to 7bf1a2b Compare August 10, 2021 09:46

hebasto approved these changes Aug 10, 2021

View reviewed changes

shaavan reviewed Aug 10, 2021

View reviewed changes

hebasto requested review from Sjors and meshcollider August 10, 2021 17:18

meshcollider reviewed Aug 11, 2021

View reviewed changes

src/qt/optionsmodel.h Outdated Show resolved Hide resolved

psancheti110 added 2 commits August 11, 2021 14:48

qt: Add SubFeeFromAmount option

ad28b66

qt, refactor: Fix indentation

62b125f

psancheti110 force-pushed the i386_0729 branch from 7bf1a2b to 62b125f Compare August 11, 2021 09:21

hebasto approved these changes Aug 11, 2021

View reviewed changes

hebasto requested review from meshcollider and jarolrod August 11, 2021 11:20

meshcollider reviewed Aug 11, 2021

View reviewed changes

hebasto merged commit 9948f11 into bitcoin-core:master Aug 11, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 15, 2021

Merge bitcoin-core/gui#390: Add SubFeeFromAmount to options

163a8bd

ryanofsky mentioned this pull request Aug 18, 2021

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Add SubFeeFromAmount to options #390

Add SubFeeFromAmount to options #390

Uh oh!

Conversation

psancheti110 commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psancheti110 commented Aug 4, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Aug 5, 2021

Uh oh!

Uh oh!

Uh oh!

psancheti110 commented Aug 6, 2021

Uh oh!

jarolrod commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psancheti110 commented Aug 7, 2021

Uh oh!

psancheti110 commented Aug 7, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psancheti110 commented Aug 9, 2021

Uh oh!

jarolrod left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psancheti110 commented Aug 10, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Aug 10, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

psancheti110 commented Aug 11, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Talkless commented Aug 11, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

psancheti110 commented Jul 29, 2021 •

edited

Loading

jarolrod commented Aug 6, 2021 •

edited

Loading

hebasto commented Aug 7, 2021 •

edited

Loading

hebasto left a comment •

edited

Loading

jarolrod left a comment •

edited

Loading