Skip to content

Conversation

promag
Copy link
Contributor

@promag promag commented Jan 15, 2020

Essentially moves the code in WalletView::setBitcoinGUI to the only caller. Two new signals are added beforehand in the first commit so that the connections in WalletFrame are all from the wallet view.

@promag promag force-pushed the 2020-01-remove-walletview-bitcoingui branch from ac36752 to ba15dae Compare January 15, 2020 23:08
@fanquake fanquake added the GUI label Jan 15, 2020
@promag promag requested a review from hebasto January 15, 2020 23:21
@promag promag force-pushed the 2020-01-remove-walletview-bitcoingui branch from ba15dae to 72b5a23 Compare January 16, 2020 00:04
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 16, 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:

  • #16432 (qt: Add privacy to the Overview page 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.

@promag promag requested a review from jonasschnelli January 16, 2020 08:12
@hebasto
Copy link
Member

hebasto commented Jan 16, 2020

Concept ACK.

@Empact
Copy link
Contributor

Empact commented Jan 16, 2020

Concept ACK

@promag promag force-pushed the 2020-01-remove-walletview-bitcoingui branch from 72b5a23 to 33727e0 Compare January 17, 2020 01:17
@promag
Copy link
Contributor Author

promag commented Jan 17, 2020

To avoid conflicts rebased on #17943 which has already @Empact ACK (and mine fwiw).

@hebasto
Copy link
Member

hebasto commented Jan 19, 2020

w.r.t. #17500 (comment)

It seems a bit of formalization could be helpful for our community. How about:

Qt Slots and Signals
Do no make connections to external signals or slots in the QObject subclass implementation.
Rationale: Do not introduce needless dependencies.

@promag
Copy link
Contributor Author

promag commented Jan 19, 2020

@hebasto looks good!

@promag promag force-pushed the 2020-01-remove-walletview-bitcoingui branch from 33727e0 to cb8a86d Compare January 30, 2020 11:38
@promag
Copy link
Contributor Author

promag commented Jan 30, 2020

Rebased and reverted to previous version since #17965 was merged.

@promag
Copy link
Contributor Author

promag commented Jan 31, 2020

Appveyor status is wrong, it actually passed.

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 cb8a86d, tested on Linux Mint 19.3.

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 cb8a86d

jonasschnelli added a commit that referenced this pull request Feb 1, 2020
cb8a86d gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d107 gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)

Pull request description:

  Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.

ACKs for top commit:
  hebasto:
    ACK cb8a86d, tested on Linux Mint 19.3.
  jonasschnelli:
    utACK cb8a86d

Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
@jonasschnelli jonasschnelli merged commit cb8a86d into bitcoin:master Feb 1, 2020
@promag promag deleted the 2020-01-remove-walletview-bitcoingui branch February 1, 2020 12:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 1, 2020
…ependency

cb8a86d gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d107 gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)

Pull request description:

  Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.

ACKs for top commit:
  hebasto:
    ACK cb8a86d, tested on Linux Mint 19.3.
  jonasschnelli:
    utACK cb8a86d

Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 3, 2020
Fix intro dialog labels when the prune button is toggled (bitcoin#17453)
Rename debug window (bitcoin#17096)
Add close window shortcut (bitcoin#15768)
Add privacy to the Overview page (bitcoin#16432)
Remove WalletView and BitcoinGUI circular dependency (bitcoin#17937)
Force set nPruneSize in QSettings after the intro dialog (bitcoin#17696)
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
…ependency

cb8a86d gui: Remove WalletView and BitcoinGUI circular dependency (João Barbosa)
ac3d107 gui: Add transactionClicked and coinsSent signals to WalletView (João Barbosa)

Pull request description:

  Essentially moves the code in `WalletView::setBitcoinGUI` to the only caller. Two new signals are added beforehand in the first commit so that the connections in `WalletFrame` are all from the wallet view.

ACKs for top commit:
  hebasto:
    ACK cb8a86d, tested on Linux Mint 19.3.
  jonasschnelli:
    utACK cb8a86d

Tree-SHA512: 250316cd3689e51c8cded9ccd75963c836dcafa6db25d684f2aa691dea9738895f9140793e0f925784909e39f8257f7e1c7d611e8bd6d6634e1a50333f4ddb1e
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 18, 2020
Summary:
> Two new signals are added beforehand in the first commit so that the connections in WalletFrame are all from the wallet view.

This is a backport of Core [[bitcoin/bitcoin#17937 | PR17937]] [1/2]
bitcoin/bitcoin@ac3d107

Test Plan:
`ninja && src/qt/bitcoin-qt`
Send a transaction, check that the transaction history page is automatically selected

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8698
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 19, 2020
Summary:
> Essentially moves the code in WalletView::setBitcoinGUI to the only caller.

Remove the circular dependency "qt/bitcoingui -> qt/walletview -> qt/bitcoingui"

This is a backport of Core [[bitcoin/bitcoin#17937 | PR17937]] [2/2]
bitcoin/bitcoin@cb8a86d

Depends on D8698

Test Plan:
`ninja && src/qt/bitcoin-qt`
Test some of the connections

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

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8699
hebasto added a commit to bitcoin-core/gui that referenced this pull request Jun 5, 2021
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
@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.

6 participants