refactor, qt: Nuke walletframe circular dependency #17500

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20191117-nuke-circular changing 4 files +8 −9

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

--- a/src/qt/walletframe.cpp
+++ b/src/qt/walletframe.cpp
@@ -5,7 +5,6 @@
 #include <qt/walletframe.h>
 #include <qt/walletmodel.h>

-#include <qt/bitcoingui.h>
 #include <qt/walletview.h>

 #include <cassert>
@@ -14,7 +13,7 @@
 #include <QLabel>

 WalletFrame::WalletFrame(const PlatformStyle *_platformStyle, BitcoinGUI *_gui) :
-    QFrame(_gui),
+    QFrame(nullptr),
     gui(_gui),
     platformStyle(_platformStyle)
 {

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

Contributors

Review Requested
jonasschnelli

Labels

GUI Refactoring

Linked (view graph)

#17499 refactor, qt: Remove unused signal from WalletView class #17966 qt, refactor: Optimize signal-slot connections logic

refactor, qt: Nuke walletframe circular dependency #17500

Conflicts