gui: Fix manual coin control with multiple wallets loaded

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

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#pullrequestreview-319297589 and #17457 (comment))

No change in behavior if only one wallet is loaded.

Closes #15725.

gui: Fix manual coin control with multiple wallets loaded a8b5f1b133

fanquake added the label GUI on May 5, 2020

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

If possible tag 0.20. @Sjors @hebasto @gwillen would ❤️ some reviews.

ryanofsky approved

ryanofsky commented at 1:01 am on May 6, 2020: member

Code review ACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee. 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

promag commented at 1:05 am on May 6, 2020: member

Not sure if this means coin control settings are reset between payments.

Yes, and new change shows if coin control dialog is opened again.

It would be good to note in the PR description or release notes if single wallet behavior is affected

Yes, at least that’s what I expect - I’ll note it.

in src/qt/coincontroldialog.h:50 in a8b5f1b133

49 
50-    void setModel(WalletModel *model);
51-
52     // static because also called from sendcoinsdialog
53-    static void updateLabels(WalletModel*, QDialog*);
54+    static void updateLabels(CCoinControl& m_coin_control, WalletModel*, QDialog*);

hebasto commented at 2:49 am on May 6, 2020:

Why updateLabels() remains static?

promag commented at 10:48 pm on May 6, 2020:

That’s unrelated to the fix. I do plan to pick the refactor done in #17457.

Sjors commented at 1:12 pm on May 8, 2020:

Good to know you plan to get rid of this static method in another PR.

DrahtBot commented at 2:54 am on May 6, 2020: 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:

#17611 (gui: Make send and receive widgets extend QWidget by promag)

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.

hebasto changes_requested

hebasto commented at 2:55 am on May 6, 2020: member

Approach ACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee, tested on Linux Mint 19.3: #15725 is fixed.

hebasto approved

hebasto commented at 3:50 am on May 7, 2020: member

ACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee

jonasschnelli commented at 7:48 am on May 7, 2020: contributor

Concept ACK. Will detail review and test soon. Could be a 0.20 bugfix.

in src/qt/coincontroldialog.cpp:141 in a8b5f1b133

136@@ -136,6 +137,13 @@ CoinControlDialog::CoinControlDialog(const PlatformStyle *_platformStyle, QWidge
137         sortView(settings.value("nCoinControlSortColumn").toInt(), (static_cast<Qt::SortOrder>(settings.value("nCoinControlSortOrder").toInt())));
138 
139     GUIUtil::handleCloseWindowShortcut(this);
140+
141+    if(_model->getOptionsModel() && _model->getAddressTableModel())

Sjors commented at 1:04 pm on May 8, 2020:

This needs an assert that _model isn’t nullptr.

promag commented at 1:24 pm on May 8, 2020:

This will crash anyway.

Sjors commented at 1:43 pm on May 8, 2020:

assert crashes are marginally easier to debug afaik

promag commented at 1:51 pm on May 8, 2020:

Yes. But note that the model is received in the constructor and this dialog is instanced in SendCoinsDialog which already has the model set.

I’ll add the assertion if I had to push again for some reason or if others think the same.

Sjors approved

Sjors commented at 1:17 pm on May 8, 2020: member

tACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee

jonasschnelli added the label Needs backport (0.20) on May 13, 2020

jonasschnelli added this to the milestone 0.20.0 on May 13, 2020

jonasschnelli commented at 8:15 am on May 13, 2020: contributor

utACK a8b5f1b133d4f23975a3fbfb7a415b17261466ee

jonasschnelli merged this on May 13, 2020

jonasschnelli closed this on May 13, 2020

promag deleted the branch on May 13, 2020

in src/qt/coincontroldialog.cpp:587 in a8b5f1b133

579@@ -584,12 +580,6 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
580         label->setVisible(nChange < 0);
581 }
582 
583-CCoinControl* CoinControlDialog::coinControl()

laanwj commented at 5:50 pm on May 13, 2020:

Thanks for getting rid of this.

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

fanquake referenced this in commit ea36c39f39 on May 14, 2020

fanquake removed the label Needs backport (0.20) on May 14, 2020

fanquake referenced this in commit ff4dc20750 on May 15, 2020

MarcoFalke referenced this in commit 17bdf2afae on May 15, 2020

jnewbery added this to the "Done" 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

Fabcien referenced this in commit b6b0d79d24 on Jan 28, 2021

backpacker69 referenced this in commit 20b8f93806 on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts