gui: Fix manual coin control with multiple wallets loaded #17457

promag · 2019-11-12T22:07:23Z

This PR makes coin selection work when multiple wallets are loaded - each loaded wallet has it's own coin control dialog.

Closes #15725.

DrahtBot · 2019-11-12T22:10:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

gui: Add a Make unsigned button next to Send #18656 (gui: Add a Make unsigned button next to Send by achow101)
"PSBT Operations" dialog #18027 ("PSBT Operations" dialog by gwillen)
gui: Make send and receive widgets extend QWidget #17611 (gui: Make send and receive widgets extend QWidget by promag)
gui: save and load PSBT #17509 (gui: save and load PSBT by Sjors)
Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag · 2019-11-12T22:12:39Z

Note to reviewers: previously CoinControlDialog::updateLabels was called with 2 different "dialogs" (CoinControlDialog and SendCoinsDialog) and both have the same set of labels except for labelCoinControlInsuffFunds. So, in order to remove static members and usages of findChild, most of the code of updateLabels is now duplicate.

Let me know if you prefer the 2nd commit split. IMO I'd prefer to follow up later.

jonasschnelli · 2019-11-16T00:08:50Z

Travis seems to fail:

qt/coincontroldialog.cpp: In member function ‘void CoinControlDialog::updateView()’:
3030qt/coincontroldialog.cpp:607:84: error: invalid conversion from ‘CoinControlDialog*’ to ‘int’ [-fpermissive]
3031         CCoinControlWidgetItem *itemWalletAddress = new CCoinControlWidgetItem(this);
3032                                                                                    ^
3033In file included from qt/coincontroldialog.cpp:9:0:
3034./qt/coincontroldialog.h:35:14: note:   initializing argument 1 of ‘CCoinControlWidgetItem::CCoinControlWidgetItem(int)’
3035     explicit CCoinControlWidgetItem(int type = Type) : QTreeWidgetItem(type) {}

promag · 2019-11-17T12:41:00Z

@jonasschnelli all fixed.

Sjors

I can confirm this fixes #15725, but 7b5b91f introduces a new issue: if you select some coins, close the coin control dialog, click Clear All, then reopen the coin control dialog, the coins are still selected.

hebasto

Concept ACK.

hebasto · 2019-11-20T07:03:52Z

src/qt/sendcoinsdialog.cpp

+CoinControlDialog* SendCoinsDialog::coinControlDialog()
+{
+    if (!m_coin_control_dialog) {
+        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);


Assigning a parent to a QDialog widget changes its default location.
Is it desirable?

I think so, at least it looked fine to me.

hebasto · 2019-11-20T07:06:00Z

src/qt/sendcoinsdialog.cpp

+{
+    if (!m_coin_control_dialog) {
+        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);
+        m_coin_control_dialog->setModel(model);


Why not pass model to the CoinControlDialog constructor?

I can make that change, but I think it's unrelated here.

We use the setModel() approach in various places where the model isn't known at construction time, but that's not the case here. But agree it's not really related.

hebasto · 2019-11-20T07:10:27Z

src/qt/sendcoinsdialog.cpp

-    dlg.setModel(model);
-    dlg.exec();
-    coinControlUpdateLabels();
+    coinControlDialog()->open();


Notes for other reviewers from Qt docs:

Avoid using this [exec()] function; instead, use open().

promag · 2019-11-20T07:50:30Z

@Sjors good catch, will fix.

hebasto · 2019-11-20T08:27:29Z

src/qt/coincontroldialog.cpp

+    QLabel *l1 = ui->labelCoinControlQuantity;
+    QLabel *l2 = ui->labelCoinControlAmount;
+    QLabel *l3 = ui->labelCoinControlFee;
+    QLabel *l4 = ui->labelCoinControlAfterFee;
+    QLabel *l5 = ui->labelCoinControlBytes;
+    QLabel *l7 = ui->labelCoinControlLowOutput;
+    QLabel *l8 = ui->labelCoinControlChange;


As updateLabels() is a member function now, we could get rid of these intermediate variables (lN). Maybe scriptdiff?
This will improve code readability significantly.

I agree but I think that refactor, and others like #17457 (comment), could come next.

I'm also very tempted to fix other things, but I just want to ditch the static coin control to fix the issue.

@promag Agree with your point.

hebasto · 2019-11-20T09:34:45Z

If CoinControlDialog was open once, the output list is not updated when new coins arrive.

This removes the nested event loop when.

instagibbs · 2020-01-09T19:08:27Z

@gwillen take a look at this?

gwillen · 2020-01-27T20:48:30Z

Sorry for the delay, I am happy to take a look, but I'm getting the same build failure that Travis is seeing. Not sure why only some Travis builds get it and not others.

(EDIT, I misread before:) Looks like a rebase refactored something and you need to update a callsite to match.

promag · 2020-01-27T21:17:16Z

@gwillen I'll fix the build, sorry.

DrahtBot · 2020-04-23T02:47:14Z

🐙 This pull request conflicts with the target branch and needs rebase.

promag · 2020-05-05T23:31:14Z

Created #18894 with just the fix. I'll submit the remaining changes later.

e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Taken from #17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked. ACKs for top commit: jonasschnelli: utACK e8123ea hebasto: ACK e8123ea, tested on Linux Mint 19.3. Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e

a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to #17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in #17457 reviews no longer apply due to the approach taken - #17457 (review) and #17457 (comment)) No change in behavior if only one wallet is loaded. Closes #15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65

e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked. ACKs for top commit: jonasschnelli: utACK e8123ea hebasto: ACK e8123ea, tested on Linux Mint 19.3. Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e

…ts loaded a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to bitcoin#17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in bitcoin#17457 reviews no longer apply due to the approach taken - bitcoin#17457 (review) and bitcoin#17457 (comment)) No change in behavior if only one wallet is loaded. Closes bitcoin#15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65

…aded (#3777) * Merge bitcoin#18894: gui: Fix manual coin control with multiple wallets loaded a8b5f1b gui: Fix manual coin control with multiple wallets loaded (João Barbosa) Pull request description: This PR ensures each loaded wallet has a dedicated coin control in the send view which is manipulated by the coin control dialog. This is an alternative to bitcoin#17457. Two main differences are: - scope reduced - no unnecessary changes unrelated to the fix; - approach taken - coin control instance now belongs to the send view. All problems raised in bitcoin#17457 reviews no longer apply due to the approach taken - bitcoin#17457 (review) and bitcoin#17457 (comment)) No change in behavior if only one wallet is loaded. Closes bitcoin#15725. ACKs for top commit: jonasschnelli: utACK a8b5f1b ryanofsky: Code review ACK a8b5f1b. Code changes are very straightforward, just replacing global CCoinControl object with SendCoinsDialog member. Not sure if this means coin control settings are reset between payments. It would be good to note in the PR description or release notes if single wallet behavior is affected hebasto: ACK a8b5f1b Sjors: tACK a8b5f1b Tree-SHA512: 3ad9c51bab6f28ec0e90efbd6f43fa510c81dafb2eff0b8c3724efcee3e030054a10be013e27cefe35763374c5f6d7af8c02658736964f733d7e38b646b5df65 * Update src/qt/coincontroldialog.cpp Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa) Pull request description: Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked. ACKs for top commit: jonasschnelli: utACK e8123ea hebasto: ACK e8123ea, tested on Linux Mint 19.3. Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e

DrahtBot added the GUI label Nov 12, 2019

promag force-pushed the 2019-11-fix-15725 branch 5 times, most recently from 504c0e3 to b89c577 Compare November 17, 2019 01:32

Sjors suggested changes Nov 19, 2019

View reviewed changes

hebasto reviewed Nov 20, 2019

View reviewed changes

DrahtBot added the Needs rebase label Nov 22, 2019

promag added 4 commits November 25, 2019 15:24

gui: Fix Cannot queue arguments of type size_t

b4097bf

gui: Fix itemWalletAddress leak when not tree mode

1017141

gui: Refactor CoinControlDialog usage

04401ed

This removes the nested event loop when.

gui: Remove static members from CoinControlDialog

1d544f7

promag force-pushed the 2019-11-fix-15725 branch from b89c577 to 1d544f7 Compare November 25, 2019 15:36

DrahtBot removed the Needs rebase label Nov 25, 2019

This was referenced Feb 11, 2020

"PSBT Operations" dialog #18027

Merged

gui: Make send and receive widgets extend QWidget #17611

Closed

gui: save and load PSBT #17509

Merged

Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463

Closed

promag mentioned this pull request Apr 9, 2020

gui: Fix leak in CoinControlDialog::updateView #18578

Merged

This was referenced Apr 13, 2020

rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items #18627

Closed

gui: Add a Make unsigned button next to Send #18656

Closed

DrahtBot added the Needs rebase label Apr 23, 2020

promag mentioned this pull request May 5, 2020

gui: Fix manual coin control with multiple wallets loaded #18894

Merged

promag closed this May 5, 2020

promag deleted the 2019-11-fix-15725 branch May 5, 2020 23:31

promag mentioned this pull request May 21, 2020

http: Release work queue after event base finish #19033

Merged

promag mentioned this pull request May 29, 2020

gui, refactor: Register Qt meta types in application constructor #19104

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

gui: Fix manual coin control with multiple wallets loaded #17457

gui: Fix manual coin control with multiple wallets loaded #17457

Uh oh!

Conversation

promag commented Nov 12, 2019

Uh oh!

DrahtBot commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Nov 12, 2019

Uh oh!

jonasschnelli commented Nov 16, 2019

Uh oh!

promag commented Nov 17, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 20, 2019

Uh oh!

hebasto Nov 20, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 20, 2019

Uh oh!

instagibbs commented Jan 9, 2020

Uh oh!

gwillen commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 27, 2020

Uh oh!

DrahtBot commented Apr 23, 2020

Uh oh!

promag commented May 5, 2020

Uh oh!

Uh oh!

DrahtBot commented Nov 12, 2019 •

edited

Loading

hebasto Nov 20, 2019 •

edited

Loading

gwillen commented Jan 27, 2020 •

edited

Loading