gui: Remove WalletView and BitcoinGUI circular dependency #17937

pull promag wants to merge 2 commits into bitcoin:master from promag:2020-01-remove-walletview-bitcoingui changing 4 files +12 −31

promag commented at 11:06 PM on January 15, 2020: member

Essentially moves the code in WalletView::setBitcoinGUI to the only caller. Two new signals are added beforehand in the first commit so that the connections in WalletFrame are all from the wallet view.
promag force-pushed on Jan 15, 2020
fanquake added the label GUI on Jan 15, 2020
promag requested review from hebasto on Jan 15, 2020
promag force-pushed on Jan 16, 2020
DrahtBot commented at 2:55 AM on January 16, 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:
- #16432 (qt: Add privacy to the Overview page by hebasto)
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 requested review from jonasschnelli on Jan 16, 2020
hebasto commented at 10:10 AM on January 16, 2020: member

Concept ACK.

in src/qt/walletframe.cpp:68 in 72b5a2343c outdated

  60 | @@ -62,6 +61,14 @@ bool WalletFrame::addWallet(WalletModel *walletModel)
  61 |      mapWalletViews[walletModel] = walletView;
  62 |  
  63 |      connect(walletView, &WalletView::outOfSyncWarningClicked, this, &WalletFrame::outOfSyncWarningClicked);
  64 | +    connect(walletView, &WalletView::transactionClicked, gui, &BitcoinGUI::gotoHistoryPage);
  65 | +    connect(walletView, &WalletView::coinsSent, gui, &BitcoinGUI::gotoHistoryPage);
  66 | +    connect(walletView, &WalletView::message, [this](const QString& title, const QString& message, unsigned int style) {
  67 | +        gui->message(title, message, style);
  68 | +    });

hebasto commented at 10:36 AM on January 16, 2020:

s/[this]/[gui]/ ?

promag commented at 10:42 AM on January 16, 2020:

Doesn't work, gui is a member.

hebasto commented at 10:53 AM on January 16, 2020:

oops

BitcoinGUI::message is a slot, and there is no need to capture local variable gui. Why not use non-lambda connection syntax?

nm: it does not work too, but could work if remove unused default parameter from BitcoinGUI::message signature.

promag commented at 11:05 AM on January 16, 2020:

Right, originally couldn't connect to slot because of that. I think we can "clean" these later.

hebasto changes_requested
Empact commented at 9:52 PM on January 16, 2020: member

Concept ACK
promag force-pushed on Jan 17, 2020
promag commented at 1:19 AM on January 17, 2020: member

To avoid conflicts rebased on #17943 which has already @Empact ACK (and mine fwiw).
hebasto commented at 10:48 AM on January 19, 2020: member

w.r.t. #17500 (comment)

It seems a bit of formalization could be helpful for our community. How about:

Qt Slots and Signals Do no make connections to external signals or slots in the QObject subclass implementation. Rationale: Do not introduce needless dependencies.
promag commented at 10:51 AM on January 19, 2020: member

@hebasto looks good!
DrahtBot added the label Needs rebase on Jan 22, 2020
gui: Add transactionClicked and coinsSent signals to WalletView ac3d10777d
gui: Remove WalletView and BitcoinGUI circular dependency cb8a86d9f9
promag force-pushed on Jan 30, 2020
promag commented at 11:49 AM on January 30, 2020: member

Rebased and reverted to previous version since #17965 was merged.
DrahtBot removed the label Needs rebase on Jan 30, 2020
promag commented at 4:15 PM on January 31, 2020: member

Appveyor status is wrong, it actually passed.
hebasto approved
hebasto commented at 4:29 PM on January 31, 2020: member

ACK cb8a86d9f952401eaad68b2e3818ce50f7befd91, tested on Linux Mint 19.3.
jonasschnelli approved
jonasschnelli commented at 9:10 AM on February 1, 2020: contributor

utACK cb8a86d9f952401eaad68b2e3818ce50f7befd91
jonasschnelli referenced this in commit f05c1ac444 on Feb 1, 2020
jonasschnelli merged this on Feb 1, 2020
jonasschnelli closed this on Feb 1, 2020
promag deleted the branch on Feb 1, 2020
sidhujag referenced this in commit c5b9ac8c8f on Feb 1, 2020
MarkLTZ referenced this in commit 136a27c84d on Feb 3, 2020
sidhujag referenced this in commit 0e7e1487b2 on Nov 10, 2020
Fabcien referenced this in commit f36cdc7e0c on Dec 18, 2020
Fabcien referenced this in commit 94bf18e8bf on Dec 19, 2020
hebasto referenced this in commit e033ca1379 on Jun 5, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17966 qt, refactor: Optimize signal-slot connections logic

gui: Remove WalletView and BitcoinGUI circular dependency #17937

Conflicts