Skip to content

Conversation

promag
Copy link
Contributor

@promag promag commented Feb 3, 2020

Class PeerTableModel doesn't actually depend on ClientModel.

@promag
Copy link
Contributor Author

promag commented Feb 3, 2020

@jonasschnelli don't know how I've missed this simple change in #18036.

@hebasto
Copy link
Member

hebasto commented Feb 3, 2020

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 3, 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:

  • #18064 (gui: Drop WalletModel dependency to RecentRequestsTableModel by promag)
  • #17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)

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.

@fanquake fanquake removed the Tests label Feb 3, 2020
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 ff59bcd, tested on Linux Mint 19.3. No changes in behavior are observed.

@Empact
Copy link
Contributor

Empact commented Feb 3, 2020

Code Review ACK ff59bcd

fanquake added a commit that referenced this pull request Feb 4, 2020
ff59bcd gui: Drop PeerTableModel dependency to ClientModel (João Barbosa)

Pull request description:

  Class `PeerTableModel` doesn't actually depend on `ClientModel`.

ACKs for top commit:
  Empact:
    Code Review ACK ff59bcd
  hebasto:
    ACK ff59bcd, tested on Linux Mint 19.3. No changes in behavior are observed.

Tree-SHA512: 29fa3c316c05b8f7b9340e5859bbb8c3a0b826aa7c865c892cfa13b5ad30f822fcaae4e01555f7860cd1727f20b7ef555a808235522a04a6eebaaa7b605f8595
@fanquake fanquake merged commit ff59bcd into bitcoin:master Feb 4, 2020
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 22, 2020
Summary:
> Class PeerTableModel doesn't actually depend on ClientModel.

This removes the circular dependency `qt/clientmodel -> qt/peertablemodel -> qt/clientmodel`

This is a backport of Core [[bitcoin/bitcoin#18060 | PR18060]]

Test Plan:
`ninja && src/qt/bitcoin-qt`

PeerTableModel is used in the rpc console GUI.

`test/lint/lint-circular-dependencies.sh`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8737
@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.

5 participants