Do not exit and re-enter main event loop during shutdown #336

pull hebasto wants to merge 10 commits into bitcoin-core:master from hebasto:210516-loop changing 14 files +75 −60
  1. hebasto commented at 10:19 pm on May 16, 2021: member

    On master (1ef34ee25ed34b2b092f15bf3dca5c0508092829) during shutdown QApplication exits the main event loop, then re-enter again.

    This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59.

    Also, blocking QDialog::exec() calls are replaced with safer QDialog::show(), except for SendConfirmationDialog as that change is not trivial (marked as TODO).

    The QDialog::open() was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.

    This PR does not change behavior, and all touched dialogs are still application modal. As a follow up, a design research could suggest to make some dialogs window modal.

    NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.

  2. hebasto added the label Refactoring on May 16, 2021
  3. hebasto marked this as a draft on May 16, 2021
  4. hebasto force-pushed on May 16, 2021
  5. hebasto marked this as ready for review on May 16, 2021
  6. jarolrod commented at 11:43 pm on May 17, 2021: member

    Concept ACK, Tested that each commit can compile and run on its own.

    Testing on macOS, some of the dialogs will now open in a subwindow instead of its own window. Here is an example of this:

    Master PR

    Specifically the OptionsDialog, EditAddressDialog, AskPassphraseDialog undergo this change compared to master. Interestingly, CoinControlDialog, PSBTOperationsDialog, and HelpMessageDialog all still open up in their own window. It is surprising to me that one set of dialogs behave differently from another set under these changes.

  7. hebasto force-pushed on May 19, 2021
  8. hebasto commented at 11:49 pm on May 19, 2021: member

    Updated 09e84de6bbe550d7e1cc3d56efbded73de36dc38 -> 77a926101d91c4e4c0cead97272d99dbc18285b5 (pr336.02 -> pr336.03, diff):

  9. hebasto force-pushed on May 29, 2021
  10. hebasto commented at 11:54 am on May 29, 2021: member
    Rebased 77a926101d91c4e4c0cead97272d99dbc18285b5 -> 0f2fff335a28cc287a1e31f9131d1ce1dc8a4739 (pr336.03 -> pr336.04) on top of the recent CI changes.
  11. hebasto commented at 3:11 pm on June 7, 2021: member
    Added a note for reviewers to the OP.
  12. in src/qt/askpassphrasedialog.cpp:74 in 0f2fff335a outdated 
    69@@ -70,6 +70,9 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent, SecureStri
70     connect(ui->passEdit3, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
71 
72     GUIUtil::handleCloseWindowShortcut(this);
73+
74+    setAttribute(Qt::WA_DeleteOnClose);
    promag commented at 3:23 pm on June 7, 2021:
    For the record, I’ve suggested to add GUIUtil::showModalDialogAndDeleteOnClose instead.
    hebasto commented at 3:57 pm on June 7, 2021:
    Thanks! Updated.
  13. hebasto force-pushed on Jun 7, 2021
  14. hebasto commented at 3:57 pm on June 7, 2021: member

    Updated 0f2fff335a28cc287a1e31f9131d1ce1dc8a4739 -> 2ad7b4d100fbf41d4d7904e6fbeeec39597c9201 (pr336.04 -> pr336.05):

    • addressed @promag’s comment
    • rebased on top of the recent GUI changes (signal-slot connection etc)
  15. in src/qt/guiutil.h:426 in 2ad7b4d100 outdated 
    417@@ -417,6 +418,13 @@ namespace GUIUtil
418             type);
419     }
420 
421+    /**
422+     * Shows a QDialog instance asynchronously, and deletes it on close.
423+     */
424+    void ShowModalDialogAndDeleteOnClose(
425+        QDialog* dialog,
426+        Qt::WindowModality modality = Qt::ApplicationModal);
    promag commented at 4:01 pm on June 7, 2021:
    Why this as an argument when it’s not used? If you keep this then the function could be ShowDialogAndDeleteOnClose?
    hebasto commented at 4:03 pm on June 7, 2021:
    It could be Qt::WindowModal as well.
    hebasto commented at 4:04 pm on June 7, 2021:

    Why this as an argument when it’s not used?

    For generality.

    hebasto commented at 7:03 am on June 11, 2021:
    Thanks! Updated.
  16. DrahtBot added the label Needs rebase on Jun 9, 2021
  17. hebasto force-pushed on Jun 11, 2021
  18. hebasto commented at 7:03 am on June 11, 2021: member

    Updated 2ad7b4d100fbf41d4d7904e6fbeeec39597c9201 -> b441b47b046e9952a6bbc582dc5fa6044961573b (pr336.05 -> pr336.06):

  19. DrahtBot removed the label Needs rebase on Jun 11, 2021
  20. DrahtBot added the label Needs rebase on Jul 22, 2021
  21. hebasto force-pushed on Jul 22, 2021
  22. hebasto commented at 5:31 pm on July 22, 2021: member
    Rebased b441b47b046e9952a6bbc582dc5fa6044961573b -> d6dd44c69428b641609c2564ed39b97fdef7be92 (pr336.06 -> pr336.07) due to the conflict with #381.
  23. DrahtBot removed the label Needs rebase on Jul 22, 2021
  24. DrahtBot added the label Needs rebase on Aug 11, 2021
  25. hebasto force-pushed on Aug 14, 2021
  26. hebasto commented at 4:29 pm on August 14, 2021: member
    Rebased d6dd44c69428b641609c2564ed39b97fdef7be92 -> 950ed3cb529f78255614f2ffa148b04c77e61c2d (pr336.07 -> pr336.08) due to the conflict with #399.
  27. DrahtBot removed the label Needs rebase on Aug 14, 2021
  28. DrahtBot added the label Needs rebase on Aug 26, 2021
  29. hebasto force-pushed on Aug 26, 2021
  30. hebasto commented at 3:19 pm on August 26, 2021: member
    Rebased 950ed3cb529f78255614f2ffa148b04c77e61c2d -> 2407f0d7e0771d6edbaba0b25510cf0448ad2563 (pr336.08 -> pr336.09) due to the conflict with #403.
  31. DrahtBot removed the label Needs rebase on Aug 26, 2021
  32. shaavan commented at 4:58 pm on August 27, 2021: contributor

    tACK 2407f0d7e0771d6edbaba0b25510cf0448ad2563 Tested on Ubuntu 20.04 (Qt version 5.12.8) I tested all the commits and didn’t observe any changes in functionality.

    I would like to add my notes that summarise my understanding of this PR. I hope that this might help fellow reviewers. If any of my observation is erroneous please, do inform me.

    NOTES:

    1. show() is used to replace exec() to make sure this process does not block the main loop.
    2. Window Modality is set to ApplicationModal because we don’t want other windows to be editable while this window is open. exec() used to set the modality to ApplicationModal by default. But we have to set modality ourselves while using show().
    3. WA_DeleteOnClose attribute is assigned so that the object will be destroyed automatically if its parent objects are destroyed. This is done to prevent unnecessary bugs of not properly closing of GUI.
    4. During the shutdown process, instead of directly shutting down GUI. All the shutting down processes are connected to requestShutDown function, which will (other than what it used to do earlier) hide all the topLevelWindows the open windows before emitting a shutdownrequested signal, which will then ultimately shutdown GUI. This is done to streamline the shutting down process and prevent GUI from entering the main loop again.
    5. BitcoinApplication::shutdownResult() has been removed and is replaced everywhere with QApplication::quit. As the function was ultimately calling quit, so this redundant function is removed.
  33. qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose 13f618818d
  34. qt, refactor: Keep OptionsDialog in the main event loop 7830cd0b35
  35. qt, refactor: Keep CoinControlDialog in the main event loop 59f7ba4fd7
  36. qt, refactor: Keep EditAddressDialog in the main event loop 6f6fde30e7
  37. qt, refactor: Keep AskPassphraseDialog in the main event loop 7fa91e8312
  38. qt, refactor: Keep PSBTOperationsDialog in the main event loop c8bae37a7a
  39. qt, refactor: Keep HelpMessageDialog in the main event loop 332dea2852
  40. qt, refactor: Allocate SendConfirmationDialog instances on heap 
    This change is require for the next commit.
    b4e0d2c431
  41. hebasto force-pushed on Sep 7, 2021
  42. hebasto commented at 6:40 am on September 7, 2021: member
    Rebased 2407f0d7e0771d6edbaba0b25510cf0448ad2563 -> 94c395dd815f69c244044a0e772ea443a1b46c8c (pr336.09 -> pr336.10) due to the conflict with #398.
  43. laanwj commented at 12:05 pm on September 29, 2021: member
    Concept ACK
  44. in src/qt/bitcoingui.h:217 in 1d4760cc6b outdated 
    213@@ -214,6 +214,7 @@ class BitcoinGUI : public QMainWindow
214     void openOptionsDialogWithTab(OptionsDialog::Tab tab);
215 
216 Q_SIGNALS:
217+    void quitClicked();
    promag commented at 1:39 pm on September 29, 2021:

    1d4760cc6b939f22f60b24c4989f55c7c4fa79d8

    nit, quitTriggered or quitRequested.

    hebasto commented at 2:12 pm on September 29, 2021:
    Thanks! Updated.
  45. promag changes_requested
  46. promag commented at 1:50 pm on September 29, 2021: contributor

    Tested ACK 94c395dd815f69c244044a0e772ea443a1b46c8c.

    I’ve tested quitting in different cases, with some dialogs opened, also with the stop RPC command.

    Checked all call sites affected by the change to non-blocking.

  47. qt: Do not exit and re-enter main event loop during shutdown f3a17bbe5f
  48. qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot 451ca244db
  49. hebasto force-pushed on Sep 29, 2021
  50. hebasto commented at 2:12 pm on September 29, 2021: member

    Updated 94c395dd815f69c244044a0e772ea443a1b46c8c -> 451ca244db8bc71ffc3cc9982d025f144cc8f3bc (pr336.10 -> pr336.11, diff):

  51. promag commented at 2:20 pm on September 29, 2021: contributor
    ACK 451ca244db8bc71ffc3cc9982d025f144cc8f3bc, just changed signal to quitRequested.
  52. hebasto commented at 2:26 pm on September 29, 2021: member

    @shaavan

    tACK 2407f0d Tested on Ubuntu 20.04 (Qt version 5.12.8) I tested all the commits and didn’t observe any changes in functionality.

    Would you mind re-reviewing this PR after the recent changes?

  53. shaavan commented at 3:47 pm on September 29, 2021: contributor

    Would you mind re-reviewing this PR after the recent changes?

    Let me take a look!

  54. laanwj commented at 9:34 am on September 30, 2021: member
    Code review and lighly tested ACK 451ca244db8bc71ffc3cc9982d025f144cc8f3bc
  55. laanwj merged this on Sep 30, 2021
  56. laanwj closed this on Sep 30, 2021
  57. hebasto deleted the branch on Sep 30, 2021
  58. shaavan commented at 12:07 pm on September 30, 2021: contributor

    Post-merge ACK 451ca244db8bc71ffc3cc9982d025f144cc8f3bc

    Changes since my last review:

    • PR was rebased on Master
    • Function QuitClicked is renamed to QuitRequested

    Tested successfully again on Ubuntu 20.04 (Using Qt version 5.12.8), with no change in functionality. Thanks for this fantastic work @hebasto!

  59. sidhujag referenced this in commit 03627b4244 on Sep 30, 2021
  60. ghost commented at 12:20 pm on December 16, 2021: none
    @hebasto Can we revert the changes made in this pull request?
  61. hebasto commented at 12:21 pm on December 16, 2021: member

    @prayank23

    @hebasto Can we revert the changes made in this pull request?

    I’m working on https://github.com/bitcoin/bitcoin/issues/23790

  62. hebasto commented at 5:12 pm on December 17, 2021: member

    @prayank23

    Can we revert the changes made in this pull request?

    Blocking synchronous dialogs are evil for responsible GUI. Only broken part was reverted in #509.

  63. ghost commented at 5:14 pm on December 17, 2021: none

    Blocking synchronous dialogs are evil for responsible GUI. Only broken part was reverted in #509.

    Compiling PR branch. Will test and comment in PR. Thanks.

  64. hebasto referenced this in commit 1695d6661b on Feb 15, 2022
  65. hebasto referenced this in commit 00f8492eeb on Feb 22, 2022
  66. sidhujag referenced this in commit 703daf767e on Feb 22, 2022
  67. bitcoin-core locked this on Dec 17, 2022


hebasto jarolrod promag shaavan laanwj ghost

Labels
Refactoring

github-metadata-mirror

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