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

Revert "gui: provide wallet controller context to wallet actions" #770

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Oct 23, 2023

The commit 7066e89 from #765 breaks "Open Wallet", "Close Wallet" and "Close All Wallets" items in the File menu (at least on Ubuntu 23.10 + Wayland).

Reverting it to avoid this regression in the 26.0 release.

@hebasto hebasto added this to the 26.0 milestone Oct 23, 2023
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 23, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK furszy, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member Author

hebasto commented Oct 23, 2023

cc @furszy @jarolrod @Sjors

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tack f09bfab

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Upps.. it is because m_wallet_controller is null when the events are connected (at widget creation time). We need to move the connection to setWalletController(), otherwise there is also a possible crash that could arise when any of these events is triggered prior to the wallet controller is set.

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK f09bfab for including it in v26.

@fanquake
Copy link
Member

Upps.. it is because m_wallet_controller is null when the events are connected (at widget creation time). We need to move the connection to setWalletController(), otherwise there is also a possible crash that could arise when any of these events is triggered prior to the wallet controller is set.

So this isn't actually platform specific, it was just always broken?

@furszy
Copy link
Member

furszy commented Oct 23, 2023

Upps.. it is because m_wallet_controller is null when the events are connected (at widget creation time). We need to move the connection to setWalletController(), otherwise there is also a possible crash that could arise when any of these events is triggered prior to the wallet controller is set.

So this isn't actually platform specific, it was just always broken?

Since #765 merge, last week. Yes. It tell us how much we need to improve the GUI test coverage (and its code structure..).
We fixed an issue and broke something else.

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 f09bfab

The specified menu options don't lead to crash on macOS, but are unusable. This was an oversight in review of #765. The specified menu actions work again with this branch.

@hebasto hebasto merged commit 565c551 into bitcoin-core:master Oct 23, 2023
@hebasto hebasto deleted the 231023-revert branch October 23, 2023 14:16
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Nov 28, 2023
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants