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.

Sign up for GitHub

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

Jump to bottom

refactor: Signal-slot connections cleanup #333

Merged
merged 3 commits into from
Jun 12, 2021
Merged

refactor: Signal-slot connections cleanup #333

merged 3 commits into from
Jun 12, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 15, 2021

This PR:

  • removes slots whose only job is to emit a signal, since we can use the signal as a slot
  • connects theWalletView::outOfSyncWarningClicked signal to the BitcoinGUI::showModalOverlay slot directly, and removes intermediate WalletFrame slot and signal
  • split from refactor: Optimize signal-slot connections logic #29

This PR does not change behavior.

@hebasto hebasto mentioned this pull request May 15, 2021
Merged
jarolrod
jarolrod reviewed May 18, 2021
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK faf0639

Reviewed the code and tested that each commit can compile and run on its own. Also tested to ensure there is no change in functionality. This is a nice simplification of the signal/slot logic

@hebasto
Copy link
Member Author

hebasto commented May 29, 2021

@promag @ryanofsky @Sjors @Talkless

Could you look into this PR, please?

@hebasto
qt: Drop redundant WalletView::requestedSyncWarningInfo slot
793f195 
This change makes a connection directly to the signal that was emitted
in the removed slot.

This commit does not change behavior.
@hebasto
qt: Drop redundant OverviewPage::handleOutOfSyncWarningClicks slot
bd50ff9 
This change makes a connection directly to the signal that was emitted
in the removed slot.

This commit does not change behavior.
@hebasto
qt: Connect WalletView signal to BitcoinGUI slot directly
f507681 
This change removes redundant intermediate WalletFrame connections.

This commit does not change behavior.
@hebasto
Copy link
Member Author

hebasto commented Jun 5, 2021

Rebased faf0639 -> f507681 (pr333.01 -> pr333.02) due to the conflict with #29.

Talkless
Talkless approved these changes Jun 10, 2021
View reviewed changes
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

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

tACK f507681, tested on Debian Sid with Qt 5.15.2, no any behavioral changes noticed.

promag
promag reviewed Jun 11, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK f507681.

@hebasto hebasto merged commit 6f3fbc0 into bitcoin-core:master Jun 12, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 13, 2021
@hebasto
Merge bitcoin-core/gui#333: refactor: Signal-slot connections cleanup
990da14
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge 6f3fbc0 into merged_master (Bitcoin PR bitcoin-core/gui#333)
525ef7f
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@promag promag promag left review comments

@jarolrod jarolrod jarolrod left review comments

@Talkless Talkless Talkless approved these changes

Assignees
No one assigned
Labels
Refactoring Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hebasto @Talkless @promag @jarolrod