refactor, qt: Nuke walletframe circular dependency

hebasto commented at 2:24 pm on November 17, 2019: member

This PR:

gets rid of qt/bitcoingui -> qt/walletframe -> qt/bitcoingui circular dependency
~is based on top of #17499 (as a prerequisite)~ (merged)

fanquake added the label GUI on Nov 17, 2019

MarcoFalke added the label Refactoring on Nov 17, 2019

in src/qt/bitcoingui.cpp:101 in 1f913ac9a0 outdated

 97@@ -98,7 +98,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
 98     if(enableWallet)
 99     {
100         /** Create wallet frame and make it the central widget */
101-        walletFrame = new WalletFrame(_platformStyle, this);
102+        walletFrame = new WalletFrame(_platformStyle, this, this);

promag commented at 5:24 pm on November 17, 2019:

1f913ac9a00b982c997e70603ba62eebbe4c9d03

Why set parent if setCentralWidget changes it?

hebasto commented at 6:08 pm on November 17, 2019:

If a widget is not intended to be a window why create it as a window (parent == nullptr)?

promag commented at 7:14 pm on November 17, 2019:

Come on, it’s being set as the central widget in the next line.. a minimal change to remove the circular dependency is

 0--- a/src/qt/walletframe.cpp
 1+++ b/src/qt/walletframe.cpp
 2@@ -5,7 +5,6 @@
 3 #include <qt/walletframe.h>
 4 #include <qt/walletmodel.h>
 5
 6-#include <qt/bitcoingui.h>
 7 #include <qt/walletview.h>
 8
 9 #include <cassert>
10@@ -14,7 +13,7 @@
11 #include <QLabel>
12
13 WalletFrame::WalletFrame(const PlatformStyle *_platformStyle, BitcoinGUI *_gui) :
14-    QFrame(_gui),
15+    QFrame(nullptr),
16     gui(_gui),
17     platformStyle(_platformStyle)
18 {

in src/qt/walletframe.h:20 in 1f913ac9a0 outdated

16@@ -17,6 +17,7 @@ class WalletView;
17 
18 QT_BEGIN_NAMESPACE
19 class QStackedWidget;
20+class QWidget;

promag commented at 5:29 pm on November 17, 2019:

1f913ac9a00b982c997e70603ba62eebbe4c9d03

Why? QFrame extends it so it’s available

hebasto commented at 6:14 pm on November 17, 2019:

Developer Notes - Source code organization:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.

promag commented at 7:15 pm on November 17, 2019:

Doest that apply for forward declarations?

laanwj commented at 11:56 am on November 18, 2019:

I think it’s okay to leave it, though FWIW forward-declaring QWidget should never be necessary. It is impossible to derive from QFrame if the full definition of QWidget (which is its ancestor) isn’t available.

in src/qt/walletframe.cpp:17 in 1f913ac9a0 outdated

18-    QFrame(_gui),
19-    gui(_gui),
20-    platformStyle(_platformStyle)
21+WalletFrame::WalletFrame(const PlatformStyle* _platformStyle, BitcoinGUI* _gui, QWidget* parent)
22+    : QFrame(parent),
23+      gui(_gui),

promag commented at 5:30 pm on November 17, 2019:

1f913ac9a00b982c997e70603ba62eebbe4c9d03

nack style change.

hebasto commented at 6:03 pm on November 17, 2019:

Shouldn’t a contributor consistently apply clang-format-diff.py to his own commits?

promag commented at 7:17 pm on November 17, 2019:

I personally don’t if it makes the diff larger than necessary.

jonasschnelli commented at 8:38 pm on November 17, 2019:

Probably irrelevant… I think clang-format-diff.py changes are fine.

laanwj commented at 11:50 am on November 18, 2019:

I think in general it’s important to limit changes to only the code that is touched, so that it is easy to see the real changes. But in this case, bah, an argument is added and some spaces are added to the other lines it’s fine with me. I don’t think it interferes with review enough to complain.

promag commented at 12:02 pm on November 18, 2019:

See comment above https://github.com/bitcoin/bitcoin/pull/17500/files#r347153585

in src/qt/walletframe.cpp:7 in 1f913ac9a0 outdated

2@@ -3,20 +3,19 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5 #include <qt/walletframe.h>
6-#include <qt/walletmodel.h>
7 
8-#include <qt/bitcoingui.h>
9+#include <qt/walletmodel.h>

promag commented at 5:31 pm on November 17, 2019:

1f913ac9a00b982c997e70603ba62eebbe4c9d03

Looks like this is the only change needed?

promag commented at 5:31 pm on November 17, 2019: member

Concept ACK.

practicalswift commented at 7:15 pm on November 17, 2019: contributor

Concept ACK

Nice to see the list of circular dependencies shrink towards length zero :)

jonasschnelli commented at 8:39 pm on November 17, 2019: contributor

Concept ACK

in src/qt/walletframe.h:35 in 1f913ac9a0 outdated

31@@ -31,7 +32,7 @@ class WalletFrame : public QFrame
32     Q_OBJECT
33 
34 public:
35-    explicit WalletFrame(const PlatformStyle *platformStyle, BitcoinGUI *_gui = nullptr);
36+    WalletFrame(const PlatformStyle* platformStyle, BitcoinGUI* _gui, QWidget* parent);

laanwj commented at 12:00 pm on November 18, 2019:

Please keep the explicit here. Even though this can likely never happen with three arguments without a default value, it’s better to specify it unless you want the object to ever be implicitly constructed.

hebasto commented at 1:18 pm on November 18, 2019:

Agree.

TIL: from C++11 a converting constructor can accept any number of parameters, not a single parameter.

hebasto commented at 7:21 pm on November 18, 2019:

Please keep the explicit here.

Done.

promag commented at 12:18 pm on November 18, 2019: member

If a widget is not intended to be a window why create it as a window (parent == nullptr)?

Good point, from doc https://doc.qt.io/qt-5/qwidget.html#QWidget

From IRC:

promag hebasto: afaik instancing a widget with null parent doesn’t make it a window - at least until you show() it wumpus please check that very carefully wumpus if you’re not sure, err on the safe side

Qt does exactly what @hebasto is doing here so approach ACK. Thanks @laanwj.

Please rebase.

refactor: Nuke walletframe circular dependency 2ee0b8aebc

hebasto force-pushed on Nov 18, 2019

hebasto commented at 7:23 pm on November 18, 2019: member

Please rebase.

Rebased.

promag commented at 1:28 am on December 23, 2019: member

Core review ACK 2ee0b8aebce0d2a889199bde40ec6ba590c42507.

fanquake requested review from jonasschnelli on Jan 15, 2020

DrahtBot commented at 4:04 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:

#17937 (gui: Remove WalletView and BitcoinGUI circular dependency 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.

promag commented at 10:30 am on January 16, 2020: member

Conceptually there’s still a circular reference which is unnecessary. Instead of passing thru BitcoinGUI I think WalletFrame should re-emit wallet view signals.

hebasto commented at 5:46 pm on January 19, 2020: member

Closed in favor of #17937 and #17966.

hebasto closed this on Jan 19, 2020

hebasto referenced this in commit e033ca1379 on Jun 5, 2021

DrahtBot locked this on Feb 15, 2022

refactor, qt: Nuke walletframe circular dependency #17500

Conflicts