gui: Break trivial circular dependencies

promag commented at 0:56 am on January 31, 2020: member

ShutdownWindow::showShutdownWindow just needs a widget to center the shutdown window and to borrow its title.

fanquake added the label GUI on Jan 31, 2020

promag requested review from hebasto on Jan 31, 2020

DrahtBot commented at 3:57 am on January 31, 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:

#17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
#15768 (gui: Add close window shortcut by IPGlider)

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.

in test/lint/lint-circular-dependencies.sh:16 in cdd480b484 outdated

12@@ -13,7 +13,6 @@ EXPECTED_CIRCULAR_DEPENDENCIES=(
13     "index/txindex -> validation -> index/txindex"
14     "policy/fees -> txmempool -> policy/fees"
15     "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel"
16-    "qt/bantablemodel -> qt/clientmodel -> qt/bantablemodel"
17     "qt/bitcoingui -> qt/utilitydialog -> qt/bitcoingui"

fanquake commented at 4:00 am on January 31, 2020:

Was this meant to delete the "qt/bitcoingui -> qt/utilitydialog -> qt/bitcoingui" circular dependency instead? Travis thinks it has been removed.

promag commented at 7:31 am on January 31, 2020:

Ops, yes.. this was done on top of the other PR and on rebased I’ve picked the wrong line.

promag commented at 8:03 am on January 31, 2020:

Fixed.

jonasschnelli commented at 7:56 am on January 31, 2020: contributor

Nice. Another one. Thanks

utACK cdd480b484b201afa94d0f86bad04b83ac97e011 modulo fix of the lint-circular-dependencies.sh change https://github.com/bitcoin/bitcoin/pull/18036/files#r373307566

promag force-pushed on Jan 31, 2020

promag commented at 8:06 am on January 31, 2020: member

Rebased with #18035 to avoid conflict, please review last commit only.

jonasschnelli commented at 8:08 am on January 31, 2020: contributor

Maybe close #18035 and extend the scope here?

gui: Drop BanTableModel dependency to ClientModel 61eb058cc1

promag force-pushed on Jan 31, 2020

promag renamed this:
~~gui: Drop ShutdownWindow dependency to BitcoinGUI~~
gui: Break trivial circular dependencies
on Jan 31, 2020

promag commented at 8:21 am on January 31, 2020: member

@jonasschnelli Done!

in src/qt/utilitydialog.cpp:151 in 8ec6210a63 outdated

150+    assert(parent != nullptr);
151 
152     // Show a simple window indicating shutdown status
153     QWidget *shutdownWindow = new ShutdownWindow();
154-    shutdownWindow->setWindowTitle(window->windowTitle());
155+    shutdownWindow->setWindowTitle(parent->windowTitle());

hebasto commented at 9:28 am on January 31, 2020:

nit: there is no guarantee now, that parent is a top-level widget.

May I suggest a following diff:

 0diff --git a/src/qt/utilitydialog.cpp b/src/qt/utilitydialog.cpp
 1index 65e8823cb..a1ce8e741 100644
 2--- a/src/qt/utilitydialog.cpp
 3+++ b/src/qt/utilitydialog.cpp
 4@@ -19,10 +19,12 @@
 5 
 6 #include <QCloseEvent>
 7 #include <QLabel>
 8+#include <QMainWindow>
 9 #include <QRegExp>
10 #include <QTextTable>
11 #include <QTextCursor>
12 #include <QVBoxLayout>
13+#include <QWidget>
14 
15 /** "Help message" or "About" dialog box */
16 HelpMessageDialog::HelpMessageDialog(interfaces::Node& node, QWidget *parent, bool about) :
17@@ -142,16 +144,16 @@ ShutdownWindow::ShutdownWindow(QWidget *parent, Qt::WindowFlags f):
18     setLayout(layout);
19 }
20 
21-QWidget* ShutdownWindow::showShutdownWindow(QWidget* parent)
22+QWidget* ShutdownWindow::showShutdownWindow(QMainWindow* window)
23 {
24-    assert(parent != nullptr);
25+    assert(window);
26 
27     // Show a simple window indicating shutdown status
28     QWidget *shutdownWindow = new ShutdownWindow();
29-    shutdownWindow->setWindowTitle(parent->windowTitle());
30+    shutdownWindow->setWindowTitle(window->windowTitle());
31 
32     // Center shutdown window at where main window was
33-    const QPoint global = parent->mapToGlobal(parent->rect().center());
34+    const QPoint global = window->mapToGlobal(window->rect().center());
35     shutdownWindow->move(global.x() - shutdownWindow->width() / 2, global.y() - shutdownWindow->height() / 2);
36     shutdownWindow->show();
37     return shutdownWindow;
38diff --git a/src/qt/utilitydialog.h b/src/qt/utilitydialog.h
39index b52c26758..fdf1bd385 100644
40--- a/src/qt/utilitydialog.h
41+++ b/src/qt/utilitydialog.h
42@@ -6,6 +6,7 @@
43 #define BITCOIN_QT_UTILITYDIALOG_H
44 
45 #include <QDialog>
46+#include <QMainWindow>
47 #include <QWidget>
48 
49 namespace interfaces {
50@@ -44,7 +45,7 @@ class ShutdownWindow : public QWidget
51 
52 public:
53     explicit ShutdownWindow(QWidget *parent=nullptr, Qt::WindowFlags f=Qt::Widget);
54-    static QWidget* showShutdownWindow(QWidget* window);
55+    static QWidget* showShutdownWindow(QMainWindow* window);
56 
57 protected:
58     void closeEvent(QCloseEvent *event

promag commented at 10:29 am on January 31, 2020:

@hebasto considering https://doc.qt.io/qt-5/qwidget.html#top-level-and-child-widgets how about change the assertion to

0    assert(parent != nullptr && parent->parentWidget() == nullptr);

hebasto commented at 10:54 am on January 31, 2020:

Yes, this will work. But using QMainWindow as a parameter type seems clearer semantically (as BitcoinGUI is a direct sub-class of QMainWindow), and makes checking parent->parentWidget() == nullptr redundant, no?

promag commented at 11:52 am on January 31, 2020:

Now using QMainWindow, I think it’s unnecessary to depend on QMainWindow just to get the title and rect but at the same time it’s no big deal.

hebasto commented at 9:30 am on January 31, 2020: member

ACK 8ec6210a63cf6f1cf14caa909b7111e8c3cd9307, tested on Linux Mint 19.3

The only (non-blocking) suggestion is to use QMainWindow as a parameter type (in 8ec6210a63cf6f1cf14caa909b7111e8c3cd9307 commit).

gui: Drop ShutdownWindow dependency to BitcoinGUI 3aee10b80b

promag force-pushed on Jan 31, 2020

practicalswift commented at 12:28 pm on January 31, 2020: contributor

Concept ACK: very nice to see two circular includes go. Only 14 instances left :)

hebasto approved

hebasto commented at 2:44 pm on January 31, 2020: member

ACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863, since previous review only suggested change QWidget –> QMainWindow

fanquake requested review from jonasschnelli on Feb 1, 2020

jonasschnelli approved

jonasschnelli commented at 9:07 am on February 1, 2020: contributor

utACK 3aee10b80b9d9a0f5172fc2ee75f03a37d5c3863

jonasschnelli referenced this in commit cadb9d3342 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

MarkLTZ referenced this in commit 7fdadc73fc on Apr 6, 2020

MarkLTZ referenced this in commit 8436547baa on Apr 6, 2020

Fabcien referenced this in commit d186cd85cb on Dec 18, 2020

Fabcien referenced this in commit af80fee649 on Dec 18, 2020

DrahtBot locked this on Feb 15, 2022

gui: Break trivial circular dependencies #18036

Conflicts