refactor, qt: Nuke walletframe circular dependency#17500
refactor, qt: Nuke walletframe circular dependency#17500hebasto wants to merge 1 commit intobitcoin:masterfrom
Conversation
src/qt/walletframe.h
Outdated
There was a problem hiding this comment.
1f913ac9a00b982c997e70603ba62eebbe4c9d03
Why? QFrame extends it so it's available
There was a problem hiding this comment.
Developer Notes - Source code organization:
- Every
.cppand.hfile should#includeevery 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.
There was a problem hiding this comment.
Doest that apply for forward declarations?
There was a problem hiding this comment.
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.
src/qt/walletframe.cpp
Outdated
There was a problem hiding this comment.
1f913ac9a00b982c997e70603ba62eebbe4c9d03
Looks like this is the only change needed?
|
Concept ACK Nice to see the list of circular dependencies shrink towards length zero :) |
|
Concept ACK |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
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.
hebasto
force-pushed
the
20191117-nuke-circular
branch
from
1f913ac to
2ee0b8a
Compare
Rebased. |
|
Core review ACK 2ee0b8a. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Conceptually there's still a circular reference which is unnecessary. Instead of passing thru BitcoinGUI I think WalletFrame should re-emit wallet view signals. |
62cb8d9 qt: Drop BitcoinGUI* WalletFrame data member (Hennadii Stepanov) f73e5c9 qt: Move CreateWalletActivity connection from WalletFrame to BitcoinGUI (Hennadii Stepanov) 20e2e24 qt: Move WalletView connections from WalletFrame to BitcoinGUI (Hennadii Stepanov) Pull request description: This PR: - implements an idea from bitcoin/bitcoin#17937 (comment) - simplifies `WalletFrame` class interface - as a side effect, removes `bitcoingui` -> `walletframe` -> `bitcoingui` circular dependency - is an alternative to bitcoin/bitcoin#17500 ACKs for top commit: promag: Tested ACK 62cb8d9 on macos 11.2.3 with depends build. jarolrod: ACK 62cb8d9 Tree-SHA512: 633b526a8499ba9ab4b16928daf4de4f6d610284bb9fa51891cad35300a03bde740df3466a71b46e87a62121330fcc9e606eac7666ea5e45fa6d5785b60dcbbd
8 participants
This PR:
qt/bitcoingui->qt/walletframe->qt/bitcoinguicircular dependencyis based on top of refactor, qt: Remove unused signal from WalletView class #17499 (as a prerequisite)(merged)