Skip to content
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

gui: Break trivial circular dependencies #18036

Merged
merged 2 commits into from Feb 1, 2020

Conversation

promag
Copy link
Member

@promag promag commented Jan 31, 2020

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

@fanquake fanquake added the GUI label Jan 31, 2020
@promag promag requested a review from hebasto January 31, 2020 00:58
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 31, 2020

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.

@jonasschnelli
Copy link
Contributor

Nice. Another one. Thanks

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

@promag promag force-pushed the 2020-01-utilitydialog branch 2 times, most recently from b2fe5e2 to 02c14a6 Compare January 31, 2020 08:05
@promag
Copy link
Member Author

promag commented Jan 31, 2020

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

@jonasschnelli
Copy link
Contributor

Maybe close #18035 and extend the scope here?

@promag promag changed the title gui: Drop ShutdownWindow dependency to BitcoinGUI gui: Break trivial circular dependencies Jan 31, 2020
@promag
Copy link
Member Author

promag commented Jan 31, 2020

@jonasschnelli Done!

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8ec6210, tested on Linux Mint 19.3

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

src/qt/utilitydialog.cpp Outdated Show resolved Hide resolved
@practicalswift
Copy link
Contributor

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

Copy link
Member

@hebasto hebasto left a 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

Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3aee10b

jonasschnelli added a commit that referenced this pull request Feb 1, 2020
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
@jonasschnelli jonasschnelli merged commit 3aee10b into bitcoin:master Feb 1, 2020
@promag promag deleted the 2020-01-utilitydialog branch February 1, 2020 12:26
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Apr 6, 2020
- 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
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Apr 6, 2020
- 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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 18, 2020
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 18, 2020
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants