gui: Fix open wallet menu initialization order #16231
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The menu must be created before connecting to aboutToShow signal.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
ryanofsky
approved these changes
Jun 18, 2019
There was a problem hiding this comment.
utACK 5224be5. Looks good, fix is simple and makes perfect sense after seeing explanation in #16118 (comment). Without this change (and since #16118), the menu pointer passed to connect(m_open_wallet_action->menu(), ...) is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work.
|
ACK 5224be5, I have tested the code on Bionic with Qt 5.12.4. |
|
@promag apologies, will review this tomorrow. |
|
@promag Thanks for following up here, and sorry for the slow review.. I've tested that this change fixes the I've also checked that with this change on top of You've mentioned in some other PRs that going forward, when we are testing GUI changes we should be using lldb Bitcoin-Qt.app
(lldb) target create "Bitcoin-Qt.app"
Current executable set to 'Bitcoin-Qt.app' (x86_64).
(lldb) env QT_FATAL_WARNINGS=1
(lldb) run
Process 23386 launched: '/Users/michael/github/bitcoin/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' (x86_64)
2019-06-23 18:21:14.578614+0800 Bitcoin-Qt[23386:57844] MessageTracer: Falling back to default whitelist
Process 23386 stopped
* thread #1, name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = signal SIGABRT
frame #0: 0x00007fff7e3be2c6 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
-> 0x7fff7e3be2c6 <+10>: jae 0x7fff7e3be2d0 ; <+20>
0x7fff7e3be2c8 <+12>: movq %rax, %rdi
0x7fff7e3be2cb <+15>: jmp 0x7fff7e3b8457 ; cerror_nocancel
0x7fff7e3be2d0 <+20>: retq
Target 0: (Bitcoin-Qt) stopped. |
It seems unrelated to this PR as warnings could be produced by the internal Qt code. To verify you could launch: and check out warnings in the |
|
@hebasto Thanks. I had just about figured that out myself, as the debug.log was always cutting off at: So yes, that seems unrelated to this PR, and the changes in #16263 look good. |
This case has been fixed in #16263 ;) |
fanquake
added a commit
that referenced
this pull request
Jun 23, 2019
5224be5 gui: Fix open wallet menu initialization order (João Barbosa) Pull request description: Fixes #16230, the menu must be created before connecting to aboutToShow signal. ACKs for commit 5224be: hebasto: ACK 5224be5, I have tested the code on Bionic with Qt 5.12.4. ryanofsky: utACK 5224be5. Looks good, fix is simple and makes perfect sense after seeing explanation in #16118 (comment). Without this change (and since #16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work. fanquake: ACK 5224be5 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change. Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
Merged
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Jun 23, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 30, 2019
5224be5 gui: Fix open wallet menu initialization order (João Barbosa) Pull request description: Fixes bitcoin#16230, the menu must be created before connecting to aboutToShow signal. ACKs for commit 5224be: hebasto: ACK 5224be5, I have tested the code on Bionic with Qt 5.12.4. ryanofsky: utACK 5224be5. Looks good, fix is simple and makes perfect sense after seeing explanation in bitcoin#16118 (comment). Without this change (and since bitcoin#16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work. fanquake: ACK 5224be5 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change. Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
HashUnlimited
pushed a commit
to HashUnlimited/chaincoin
that referenced
this pull request
Aug 23, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
Bushstar
pushed a commit
to Bushstar/omnicore
that referenced
this pull request
Aug 24, 2019
The menu must be created before connecting to aboutToShow signal. Github-Pull: bitcoin#16231 Rebased-From: 5224be5
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 21, 2020
Summary: The menu must be created before connecting to aboutToShow signal. bitcoin/bitcoin@5224be5 --- Depends on D6170 Backport of Core [[bitcoin/bitcoin#16231 | PR16231]] Test Plan: ninja Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6172
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 25, 2021
5224be5 gui: Fix open wallet menu initialization order (João Barbosa) Pull request description: Fixes bitcoin#16230, the menu must be created before connecting to aboutToShow signal. ACKs for commit 5224be: hebasto: ACK 5224be5, I have tested the code on Bionic with Qt 5.12.4. ryanofsky: utACK 5224be5. Looks good, fix is simple and makes perfect sense after seeing explanation in bitcoin#16118 (comment). Without this change (and since bitcoin#16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work. fanquake: ACK 5224be5 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change. Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
pravblockc
pushed a commit
to pravblockc/dash
that referenced
this pull request
Nov 18, 2021
5224be5 gui: Fix open wallet menu initialization order (João Barbosa) Pull request description: Fixes bitcoin#16230, the menu must be created before connecting to aboutToShow signal. ACKs for commit 5224be: hebasto: ACK 5224be5, I have tested the code on Bionic with Qt 5.12.4. ryanofsky: utACK 5224be5. Looks good, fix is simple and makes perfect sense after seeing explanation in bitcoin#16118 (comment). Without this change (and since bitcoin#16118), the menu pointer passed to `connect(m_open_wallet_action->menu(), ...)` is null and connecting has no effect. With this change, the menu is constructed earlier so the connect call can work. fanquake: ACK 5224be5 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change. Tree-SHA512: 97b42493b37b96683058bccf39a0ee93589293d4ba8f0c60aef7f4fb9dd084cc6d5608cd5ef531cadf5e03b1f01627ef96bc2d79f784fb38cb87aa6643183d41
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16230, the menu must be created before connecting to aboutToShow signal.