gui: Fix manual coin control with multiple wallets loaded

promag commented at 10:07 pm on November 12, 2019: member

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

Closes #15725.

DrahtBot commented at 10:10 pm on November 12, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18656 (gui: Add a Make unsigned button next to Send by achow101)
#18027 (“PSBT Operations” dialog by gwillen)
#17611 (gui: Make send and receive widgets extend QWidget by promag)
#17509 (gui: save and load PSBT by Sjors)
#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 commented at 10:12 pm on November 12, 2019: member

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.

DrahtBot added the label GUI on Nov 12, 2019

jonasschnelli commented at 0:08 am on November 16, 2019: contributor

Travis seems to fail:

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

promag force-pushed on Nov 16, 2019

promag force-pushed on Nov 17, 2019

promag commented at 12:41 pm on November 17, 2019: member

@jonasschnelli all fixed.

Sjors changes_requested

Sjors commented at 7:32 pm on November 19, 2019: member

I can confirm this fixes #15725, but 7b5b91f37b1158d9d1280d7d43858a56936de372 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.

in src/qt/sendcoinsdialog.cpp:698 in 7b5b91f37b outdated

651@@ -652,6 +652,16 @@ void SendCoinsDialog::updateFeeMinimizedLabel()
652     }
653 }
654 
655+CoinControlDialog* SendCoinsDialog::coinControlDialog()
656+{
657+    if (!m_coin_control_dialog) {
658+        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);

hebasto commented at 7:03 am on November 20, 2019:

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

promag commented at 7:49 am on November 20, 2019:

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

in src/qt/sendcoinsdialog.cpp:699 in 7b5b91f37b outdated

651@@ -652,6 +652,16 @@ void SendCoinsDialog::updateFeeMinimizedLabel()
652     }
653 }
654 
655+CoinControlDialog* SendCoinsDialog::coinControlDialog()
656+{
657+    if (!m_coin_control_dialog) {
658+        m_coin_control_dialog = new CoinControlDialog(platformStyle, this);
659+        m_coin_control_dialog->setModel(model);

hebasto commented at 7:06 am on November 20, 2019:

Why not pass model to the CoinControlDialog constructor?

promag commented at 7:47 am on November 20, 2019:

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

Sjors commented at 8:41 am on November 20, 2019:

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.

in src/qt/sendcoinsdialog.cpp:808 in 7b5b91f37b outdated

762@@ -753,10 +763,7 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked)
763 // Coin Control: button inputs -> show actual coin control dialog
764 void SendCoinsDialog::coinControlButtonClicked()
765 {
766-    CoinControlDialog dlg(platformStyle);
767-    dlg.setModel(model);
768-    dlg.exec();
769-    coinControlUpdateLabels();
770+    coinControlDialog()->open();

hebasto commented at 7:10 am on November 20, 2019:

Notes for other reviewers from Qt docs:

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

hebasto commented at 7:11 am on November 20, 2019: member

Concept ACK.

promag commented at 7:50 am on November 20, 2019: member

@Sjors good catch, will fix.

in src/qt/coincontroldialog.cpp:542 in b89c5778ca outdated

544+    QLabel *l2 = ui->labelCoinControlAmount;
545+    QLabel *l3 = ui->labelCoinControlFee;
546+    QLabel *l4 = ui->labelCoinControlAfterFee;
547+    QLabel *l5 = ui->labelCoinControlBytes;
548+    QLabel *l7 = ui->labelCoinControlLowOutput;
549+    QLabel *l8 = ui->labelCoinControlChange;

hebasto commented at 8:27 am on November 20, 2019:

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

promag commented at 11:36 am on November 20, 2019:

I agree but I think that refactor, and others like #17457 (review), 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.

hebasto commented at 11:42 am on November 20, 2019:

@promag Agree with your point.

hebasto commented at 9:34 am on November 20, 2019: member

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

DrahtBot added the label Needs rebase on Nov 22, 2019

gui: Fix Cannot queue arguments of type size_t b4097bf91c

gui: Fix itemWalletAddress leak when not tree mode 101714135e

gui: Refactor CoinControlDialog usage

This removes the nested event loop when.

04401ed679

gui: Remove static members from CoinControlDialog 1d544f7303

promag force-pushed on Nov 25, 2019

DrahtBot removed the label Needs rebase on Nov 25, 2019

instagibbs commented at 7:08 pm on January 9, 2020: member

@gwillen take a look at this?

fanquake added this to the "In progress" column in a project

gwillen commented at 8:48 pm on January 27, 2020: contributor

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 commented at 9:17 pm on January 27, 2020: member

@gwillen I’ll fix the build, sorry.

DrahtBot added the label Needs rebase on Apr 23, 2020

DrahtBot commented at 2:47 am on April 23, 2020: member

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

promag commented at 11:31 pm on May 5, 2020: member

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

promag closed this on May 5, 2020

promag deleted the branch on May 5, 2020

jonasschnelli referenced this in commit 8d17f8dc17 on May 13, 2020

jonasschnelli referenced this in commit 246e878e78 on May 13, 2020

sidhujag referenced this in commit 4460ecfc57 on May 14, 2020

sidhujag referenced this in commit 2dde03529c on May 14, 2020

jnewbery removed this from the "In progress" column in a project

UdjinM6 referenced this in commit 32122a5b9d on Oct 16, 2020

UdjinM6 referenced this in commit 98cc6a3562 on Oct 16, 2020

UdjinM6 referenced this in commit cde2defb62 on Oct 16, 2020

UdjinM6 referenced this in commit dbabf35b43 on Oct 16, 2020

UdjinM6 referenced this in commit 02efeb0c74 on Nov 9, 2020

PastaPastaPasta referenced this in commit 66a9fa0c7c on Jun 27, 2021

PastaPastaPasta referenced this in commit 472a487b3b on Jun 28, 2021

PastaPastaPasta referenced this in commit 0863d79bb4 on Jun 29, 2021

PastaPastaPasta referenced this in commit b03649bacf on Jul 1, 2021

PastaPastaPasta referenced this in commit 7434733e47 on Jul 1, 2021

PastaPastaPasta referenced this in commit 1421e714ed on Jul 14, 2021

PastaPastaPasta referenced this in commit 8af8bbc20e on Jul 15, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts