Skip to content

refactor, qt: Nuke some circular dependencies - #17513

Merged
laanwj merged 5 commits into
bitcoin:masterfrom
hebasto:20191119-nuke-circular-dep
Nov 21, 2019
Merged

refactor, qt: Nuke some circular dependencies#17513
laanwj merged 5 commits into
bitcoin:masterfrom
hebasto:20191119-nuke-circular-dep

Conversation

@hebasto

Copy link
Copy Markdown
Member

This PR gets rid of the following circular dependencies:

  • qt/guiutil -> qt/walletmodel -> qt/optionsmodel -> qt/guiutil
  • qt/walletmodel -> qt/walletmodeltransaction -> qt/walletmodel
  • qt/paymentserver -> qt/walletmodel -> qt/paymentserver

@practicalswift

Copy link
Copy Markdown
Contributor

Concept ACK

Wow, three nuked circular dependencies in one go! Feels very good to see this suppression list shrink :)

@DrahtBot

Copy link
Copy Markdown
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17492 (QT: bump fee returns PSBT on clipboard for watchonly-only wallets by instagibbs)
  • #17477 (Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals by jnewbery)
  • #15987 (Wallet, GUI: Warn when sending to already-used Bitcoin addresses by luke-jr)
  • #14384 (Fire TransactionRemovedFromMempool callbacks from mempool by l2a5b1)
  • #10785 (Serialization improvements by sipa)

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.

@hebasto
hebastoforce-pushed the 20191119-nuke-circular-dep branch 2 times, most recently from fe054ef to e27c011CompareNovember 19, 2019 11:54
@instagibbs

Copy link
Copy Markdown
Member

literally did a subset of this for #17492

concept ACK

@hebasto

hebasto commented Nov 19, 2019

Copy link
Copy Markdown
MemberAuthor

@instagibbs

literally did a subset of this for #17492

Oh, I didn't notice 450a2f7913bfbcfbc9c9dcc03c468043fe86fd76 before...

@instagibbs

Copy link
Copy Markdown
Member

I was later pretty sure, just noting this would help my PR get smaller :)

@hebasto
hebastoforce-pushed the 20191119-nuke-circular-dep branch from e27c011 to 5f50599CompareNovember 19, 2019 15:07
@hebasto

Copy link
Copy Markdown
MemberAuthor

@instagibbs has been added as a co-author.

@Sjors

Copy link
Copy Markdown
Member

ACK 5f50599

@instagibbs

Copy link
Copy Markdown
Member

code review ACK 5f50599

@practicalswift

Copy link
Copy Markdown
Contributor

ACK 567cb44 -- diff looks correct

@hebasto

Copy link
Copy Markdown
MemberAuthor

@practicalswift

ACK 567cb44 -- diff looks correct

You've ACKed non-top commit ;)

@promagpromag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK 5f50599.

@practicalswift

Copy link
Copy Markdown
Contributor

@hebasto Oh, thanks!

ACK 5f50599 -- diff looks correct

laanwj added a commit that referenced this pull request Nov 21, 2019
5f50599 refactor: Cleanup headers from walletmodel.h (Hennadii Stepanov)
a53e989 refactor: Nuke walletmodel circular dependency (Hennadii Stepanov)
49c4211 refactor: Nuke walletmodeltransaction circular dep (Hennadii Stepanov)
567cb44 refactor: Nuke guiutil circular dependency (Hennadii Stepanov)
73b5505 refactor: Move SendCoinsRecipient in own header (Hennadii Stepanov)
Pull request description:
This PR gets rid of the following circular dependencies:
- `qt/guiutil` -> `qt/walletmodel` -> `qt/optionsmodel` -> `qt/guiutil`
- `qt/walletmodel` -> `qt/walletmodeltransaction` -> `qt/walletmodel`
- `qt/paymentserver` -> `qt/walletmodel` -> `qt/paymentserver`
ACKs for top commit:
Sjors:
ACK 5f50599
instagibbs:
code review ACK 5f50599
practicalswift:
ACK 5f50599 -- diff looks correct
promag:
ACK 5f50599.
Tree-SHA512: 070686ac82b5c68c3ef1b8b4c16b4b916b84d80d1e92e42287fdd9454671bea54779c0d2db4db623750aaaf180beaba212137190d6a427113905e2c4be5c60c5
@laanwj
laanwj merged commit 5f50599 into bitcoin:masterNov 21, 2019
@hebasto
hebasto deleted the 20191119-nuke-circular-dep branch November 21, 2019 18:39
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 29, 2019
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
Backport of Core [[bitcoin/bitcoin#17513 | PR17513]] - Part 1 of 4.
Test Plan: `ninja && ninja check`
Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D7572
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
This is part 3 of 4 for backport of Core [[bitcoin/bitcoin#17513 | PR17513]]
Commit [[bitcoin/bitcoin@5f50599 | 5f50599]]
Depends on D7602
Test Plan: `ninja && ninja check`
Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Subscribers: deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D7603
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
Summary:
This conclude backport of [[bitcoin/bitcoin#17513 | PR17513]]
Commit [[bitcoin/bitcoin@49c4211 | 49c4211 ]]
Depends on D7603
Test Plan:
`ninja && ninja check`
`test/lint/lint-circular-dependencies.sh`
Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D7605
@bitcoinbitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@hebasto@practicalswift@DrahtBot@instagibbs@Sjors@promag@laanwj@fanquake