gui: Fix open wallet menu initialization order #16231

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-06-fix-open-menu changing 2 files +7 −5
  1. promag commented at 1:22 pm on June 18, 2019: member
    Fixes #16230, the menu must be created before connecting to aboutToShow signal.
  2. gui: Fix open wallet menu initialization order 
    The menu must be created before connecting to aboutToShow signal.
    5224be5a33
  3. fanquake added the label GUI on Jun 18, 2019
  4. fanquake added the label Needs backport on Jun 18, 2019
  5. fanquake added this to the milestone 0.18.1 on Jun 18, 2019
  6. DrahtBot commented at 6:45 pm on June 18, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16106 (gui: Sort wallets in open wallet menu by promag)

    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.

  7. ryanofsky approved
  8. ryanofsky commented at 8:23 pm on June 18, 2019: member
    utACK 5224be5a3354e1a22ea4d7f0e40aadfccdf67064. 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.
  9. hebasto commented at 6:37 pm on June 21, 2019: member
    ACK 5224be5a3354e1a22ea4d7f0e40aadfccdf67064, I have tested the code on Bionic with Qt 5.12.4.
  10. fanquake commented at 4:07 pm on June 22, 2019: member
    @promag apologies, will review this tomorrow.
  11. fanquake commented at 10:23 am on June 23, 2019: member

    @promag Thanks for following up here, and sorry for the slow review..

    I’ve tested that this change fixes the File -> Open Wallet -> behaviour (https://github.com/bitcoin/bitcoin/issues/16230):

    file_open

    I’ve also checked that with this change on top of master (2cbcc55ba6aea26d64eae3981b83dac04f70240f), the File menu is still disabled during load (#16118):

    no_use_before_load

    You’ve mentioned in some other PRs that going forward, when we are testing GUI changes we should be using QT_FATAL_WARNINGS=1. When I use that env variable with this PR, bitcoin-qt segfaults when it finishes loading:

     0lldb Bitcoin-Qt.app
 1(lldb) target create "Bitcoin-Qt.app"
 2Current executable set to 'Bitcoin-Qt.app' (x86_64).
 3(lldb) env QT_FATAL_WARNINGS=1
 4(lldb) run
 5Process 23386 launched: '/Users/michael/github/bitcoin/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' (x86_64)
 62019-06-23 18:21:14.578614+0800 Bitcoin-Qt[23386:57844] MessageTracer: Falling back to default whitelist
 7Process 23386 stopped
 8* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = signal SIGABRT
 9    frame [#0](/bitcoin-bitcoin/0/): 0x00007fff7e3be2c6 libsystem_kernel.dylib`__pthread_kill + 10
10libsystem_kernel.dylib`__pthread_kill:
11->  0x7fff7e3be2c6 <+10>: jae    0x7fff7e3be2d0            ; <+20>
12    0x7fff7e3be2c8 <+12>: movq   %rax, %rdi
13    0x7fff7e3be2cb <+15>: jmp    0x7fff7e3b8457            ; cerror_nocancel
14    0x7fff7e3be2d0 <+20>: retq   
15Target 0: (Bitcoin-Qt) stopped.
  12. hebasto commented at 10:33 am on June 23, 2019: member

    @fanquake

    You’ve mentioned in some other PRs that going forward, when we are testing GUI changes we should be using QT_FATAL_WARNINGS=1. When I use that env variable with this PR, bitcoin-qt segfaults when it finishes loading…

    It seems unrelated to this PR as warnings could be produced by the internal Qt code. To verify you could launch:

    0QT_FATAL_WARNINGS=7 bitcoin-qt -debug=qt

    and check out warnings in the debug.log.

  13. fanquake commented at 10:36 am on June 23, 2019: member

    @hebasto Thanks. I had just about figured that out myself, as the debug.log was always cutting off at:

    02019-06-23T10:21:21Z UpdateTip: new best=000000000000000000b6a9e3c4aa5619a48023818df48f757f8fff650b51de85 height=494994 version=0x20000002 log2_work=87.490425 tx=272517027 date='2017-11-18T23:38:38Z' progress=0.642811 cache=4.2MiB(31387txo)
12019-06-23T10:21:21Z UpdateTip: new best=000000000000000000aa16a81a9b3b74f47f07bd6184705a3525e8325dffa54f height=494995 version=0x20000000 log2_work=87.490464 tx=272519017 date='2017-11-19T00:03:10Z' progress=0.642816 cache=5.2MiB(39744txo)
22019-06-23T10:21:21Z UpdateTip: new best=00000000000000000076be6678a2efa7793f98900ac0490863514f291e0f90f8 height=494996 version=0x20000000 log2_work=87.490502 tx=272519980 date='2017-11-19T00:03:48Z' progress=0.642818 cache=6.1MiB(47055txo)
32019-06-23T10:21:21Z GUI: Platform customization: "macosx"

    So yes, that seems unrelated to this PR, and the changes in #16263 look good.

  14. fanquake approved
  15. fanquake commented at 10:39 am on June 23, 2019: member
    ACK 5224be5a3354e1a22ea4d7f0e40aadfccdf67064 Testing included in a comment above. The segfaulting with QT_FATAL_WARNINGS is unrelated to this change.
  16. hebasto commented at 10:43 am on June 23, 2019: member

    @fanquake

    I had just about figured that out myself, as the debug.log was always cutting off at:

    02019-06-23T10:21:21Z UpdateTip: new best=000000000000000000b6a9e3c4aa5619a48023818df48f757f8fff650b51de85 height=494994 version=0x20000002 log2_work=87.490425 tx=272517027 date='2017-11-18T23:38:38Z' progress=0.642811 cache=4.2MiB(31387txo)
12019-06-23T10:21:21Z UpdateTip: new best=000000000000000000aa16a81a9b3b74f47f07bd6184705a3525e8325dffa54f height=494995 version=0x20000000 log2_work=87.490464 tx=272519017 date='2017-11-19T00:03:10Z' progress=0.642816 cache=5.2MiB(39744txo)
22019-06-23T10:21:21Z UpdateTip: new best=00000000000000000076be6678a2efa7793f98900ac0490863514f291e0f90f8 height=494996 version=0x20000000 log2_work=87.490502 tx=272519980 date='2017-11-19T00:03:48Z' progress=0.642818 cache=6.1MiB(47055txo)
32019-06-23T10:21:21Z GUI: Platform customization: "macosx"

    This case has been fixed in #16263 ;)

  17. fanquake merged this on Jun 23, 2019
  18. fanquake closed this on Jun 23, 2019
  19. fanquake referenced this in commit c1bab5052a on Jun 23, 2019
  20. MarcoFalke referenced this in commit 2800b3d5c1 on Jun 23, 2019
  21. MarcoFalke removed the label Needs backport on Jun 23, 2019
  22. promag deleted the branch on Jun 24, 2019
  23. sidhujag referenced this in commit d0a515884f on Jun 30, 2019
  24. HashUnlimited referenced this in commit 83ff000351 on Aug 23, 2019
  25. Bushstar referenced this in commit 8ddbd02123 on Aug 24, 2019
  26. Bushstar referenced this in commit 70ce4f69c7 on Aug 24, 2019
  27. Bushstar referenced this in commit fc0aeaf314 on Aug 24, 2019
  28. Bushstar referenced this in commit b6a095d2c4 on Aug 24, 2019
  29. deadalnix referenced this in commit 27ebbd764c on May 21, 2020
  30. PastaPastaPasta referenced this in commit ce9d941767 on Oct 25, 2021
  31. pravblockc referenced this in commit c0de9c96cb on Nov 18, 2021
  32. MarcoFalke locked this on Dec 16, 2021


promag DrahtBot ryanofsky hebasto fanquake

Labels
GUI

Milestone
0.18.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me