-
Notifications
You must be signed in to change notification settings - Fork 38.8k
gui: Break trivial circular dependencies #18036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. |
|
Nice. Another one. Thanks utACK cdd480b484b201afa94d0f86bad04b83ac97e011 modulo fix of the |
b2fe5e2 to
02c14a6
Compare
|
Rebased with #18035 to avoid conflict, please review last commit only. |
|
Maybe close #18035 and extend the scope here? |
02c14a6 to
8ec6210
Compare
|
@jonasschnelli Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8ec6210a63cf6f1cf14caa909b7111e8c3cd9307, tested on Linux Mint 19.3
The only (non-blocking) suggestion is to use QMainWindow as a parameter type (in 8ec6210a63cf6f1cf14caa909b7111e8c3cd9307 commit).
8ec6210 to
3aee10b
Compare
|
Concept ACK: very nice to see two circular includes go. Only 14 instances left :) |
hebasto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3aee10b, since previous review only suggested change QWidget --> QMainWindow
jonasschnelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 3aee10b
3aee10b gui: Drop ShutdownWindow dependency to BitcoinGUI (João Barbosa) 61eb058 gui: Drop BanTableModel dependency to ClientModel (João Barbosa) Pull request description: `ShutdownWindow::showShutdownWindow` just needs a widget to center the shutdown window and to borrow its title. ACKs for top commit: hebasto: ACK 3aee10b, since previous review only suggested change `QWidget` --> `QMainWindow` jonasschnelli: utACK 3aee10b Tree-SHA512: e15cb6ee274730bd071d3d97b540c5059e5c655248d69a37c3fd00f2aacc6cfcb36b9a65755718027e15482ec8e5e85534c1dc13d0ddb4e0680df03fbf6571f2
- gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged bitcoin#18160 - gui: Drop PeerTableModel dependency to ClientModel bitcoin#18060 - gui: Break trivial circular dependencies bitcoin#18036 - gui: Improve "Hide" button tool-tip message bitcoin#17360 - gui: Shortcut to close ModalOverlay bitcoin#17998 - gui: Remove warning "unused variable 'wallet_model'" bitcoin#17939 - refactor: Use PACKAGE_NAME in GUI modal overlay and bitcoin-wallet bitcoin#17923 - gui: remove OpenSSL PRNG seeding (Windows, Qt only) bitcoin#17151 - refactor: Remove unused defines in qt/bitcoinunits.h bitcoin#17869
Summary: Break circular dependencies "qt/bantablemodel -> qt/clientmodel -> qt/bantablemodel" Note that a dependency on QLocale was introduced in D8492. This PR seems to break an implicit include of QLocale, so it needs to be included explicitly. This is a backport of Core [[bitcoin/bitcoin#18036 | PR18036]] [1/2] Commit bitcoin/bitcoin@61eb058 Test Plan: `ninja && src/qt/bitcoin-qt` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8696
Summary: Remove circular dependency "qt/bitcoingui -> qt/utilitydialog -> qt/bitcoingui" This concludes backport of Core [[bitcoin/bitcoin#18036 | PR18036]] [2/2] bitcoin/bitcoin@3aee10b Test Plan: `ninja && src/qt/bitcoin-qt` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8697
ShutdownWindow::showShutdownWindowjust needs a widget to center the shutdown window and to borrow its title.