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

hebasto · 2021-05-16T22:19:53Z

On master (1ef34ee) 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.

jarolrod

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.

hebasto · 2021-05-19T23:49:03Z

Updated 09e84de -> 77a9261 (pr336.02 -> pr336.03, diff):

addressed @jarolrod's comment
fixed bugs

hebasto · 2021-05-29T11:54:59Z

Rebased 77a9261 -> 0f2fff3 (pr336.03 -> pr336.04) on top of the recent CI changes.

hebasto · 2021-06-07T15:11:04Z

Added a note for reviewers to the OP.

src/qt/askpassphrasedialog.cpp

hebasto · 2021-06-07T15:57:30Z

Updated 0f2fff3 -> 2ad7b4d (pr336.04 -> pr336.05):

addressed @promag's comment
rebased on top of the recent GUI changes (signal-slot connection etc)

src/qt/guiutil.h

hebasto · 2021-06-11T07:03:25Z

Updated 2ad7b4d -> b441b47 (pr336.05 -> pr336.06):

rebased due to the conflict with UI external signer support (e.g. hardware wallet) #4
addressed @promag's comment

hebasto · 2021-07-22T17:31:25Z

Rebased b441b47 -> d6dd44c (pr336.06 -> pr336.07) due to the conflict with #381.

hebasto · 2021-08-14T16:29:49Z

Rebased d6dd44c -> 950ed3c (pr336.07 -> pr336.08) due to the conflict with #399.

hebasto · 2021-08-26T15:19:33Z

Rebased 950ed3c -> 2407f0d (pr336.08 -> pr336.09) due to the conflict with #403.

This change is require for the next commit.

hebasto · 2021-09-07T06:40:51Z

Rebased 2407f0d -> 94c395d (pr336.09 -> pr336.10) due to the conflict with #398.

laanwj · 2021-09-29T12:05:17Z

Concept ACK

promag

Tested ACK 94c395d.

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.

src/qt/bitcoingui.h

hebasto · 2021-09-29T14:12:39Z

Updated 94c395d -> 451ca24 (pr336.10 -> pr336.11, diff):

addressed @promag's comment

promag

ACK 451ca24, just changed signal to quitRequested.

hebasto · 2021-09-29T14:26:32Z

@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?

shaavan · 2021-09-29T15:47:23Z

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

Let me take a look!

laanwj · 2021-09-30T09:34:24Z

Code review and lighly tested ACK 451ca24

shaavan

Post-merge ACK 451ca24

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!

ghost · 2021-12-16T12:20:09Z

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

hebasto · 2021-12-16T12:21:27Z

@prayank23

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

I'm working on bitcoin/bitcoin#23790

hebasto · 2021-12-17T17:12:23Z

@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.

ghost · 2021-12-17T17:14:55Z

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.

…ion in wallet unlock f730bd7 scripted-diff: Rename ShowModalDialogAndDeleteOnClose (Hennadii Stepanov) 5d7666b qt: Revert 7fa91e8 partially (Hennadii Stepanov) 89c277a qt: Delay shutdown while a modal dialog is active (Hennadii Stepanov) 8c0eb80 qt: Disable tray icon menu when a modal dialog is active (Hennadii Stepanov) 9242735 qt, refactor: Use local QAction instances for the tray icon menu (Hennadii Stepanov) 58e1603 qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members (Hennadii Stepanov) fd667e7 qt: Make show_hide_action dependent on the main window actual state (Hennadii Stepanov) ee151d0 qt: Drop BitcoinGUI::toggleHideAction member (Hennadii Stepanov) 78189da qt, refactor: Fill up trayIconMenu before connections (Hennadii Stepanov) 66afa28 qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda (Hennadii Stepanov) c3ca836 qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda (Hennadii Stepanov) Pull request description: As pointed in #23790 a regression in wallet unlock was introduced in bitcoin-core/gui#336 when a synchronous `AskPassphraseDialog` has been replaced with an asynchronous one. This PR reverts a call back to a synchronous mode. To make synchronous dialogs behave nice during shutdown some additional changes were made. Please note that disabling the tray icon menu when a modal dialog is active is useful itself as on master (4ad5904) it is possible to switch to the "Receive" tab while the GUI is waiting for a password for the "Send" tab: ![Screenshot from 2021-12-17 18-59-51](https://user-images.githubusercontent.com/32963518/146580710-0a755f24-a166-414b-be60-7863232ac778.png) This is confusing and must be avoided. Fixes #23790. ACKs for top commit: prayank23: tACK bitcoin-core/gui@f730bd7 Tree-SHA512: 2b68275754190e4a9831b96e882d3c5b005e03909aeb6f2c5846da07199bb3efbb74ce87a9d25bb139f643c43d377a2051b221d553281fa5aefdd3181a58077f

e7fc506 qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov) Pull request description: #336 introduced a regression when termination requests from a platform are not handled properly. This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again. Fixes #545. ACKs for top commit: RandyMcMillan: tACK e7fc506 Sjors: tACK e7fc506 (rebased on master) indeed fixes the crash described in #545 promag: Tested ACK e7fc506 on macOS 10.15 with Qt 5.15.2. Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131

…andle QEvent::Quit e7fc506 qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov) Pull request description: bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly. This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again. Fixes bitcoin-core/gui#545. ACKs for top commit: RandyMcMillan: tACK e7fc506 Sjors: tACK e7fc506 (rebased on master) indeed fixes the crash described in #545 promag: Tested ACK e7fc506 on macOS 10.15 with Qt 5.15.2. Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131

hebasto added the Refactoring label May 16, 2021

hebasto marked this pull request as draft May 16, 2021 22:46

hebasto force-pushed the 210516-loop branch from 6507fc4 to 09e84de Compare May 16, 2021 22:52

hebasto marked this pull request as ready for review May 16, 2021 22:53

jarolrod reviewed May 17, 2021

View reviewed changes

hebasto force-pushed the 210516-loop branch from 09e84de to 77a9261 Compare May 19, 2021 23:43

hebasto force-pushed the 210516-loop branch from 77a9261 to 0f2fff3 Compare May 29, 2021 11:51

promag reviewed Jun 7, 2021

View reviewed changes

src/qt/askpassphrasedialog.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 210516-loop branch from 0f2fff3 to 2ad7b4d Compare June 7, 2021 15:53

promag reviewed Jun 7, 2021

View reviewed changes

src/qt/guiutil.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jun 9, 2021

hebasto force-pushed the 210516-loop branch from 2ad7b4d to b441b47 Compare June 11, 2021 07:01

DrahtBot removed the Needs rebase label Jun 11, 2021

DrahtBot added the Needs rebase label Jul 22, 2021

hebasto force-pushed the 210516-loop branch from b441b47 to d6dd44c Compare July 22, 2021 17:28

DrahtBot removed the Needs rebase label Jul 22, 2021

DrahtBot added the Needs rebase label Aug 11, 2021

hebasto force-pushed the 210516-loop branch from d6dd44c to 950ed3c Compare August 14, 2021 16:28

DrahtBot removed the Needs rebase label Aug 14, 2021

DrahtBot added the Needs rebase label Aug 26, 2021

hebasto force-pushed the 210516-loop branch from 950ed3c to 2407f0d Compare August 26, 2021 15:18

DrahtBot removed the Needs rebase label Aug 26, 2021

hebasto added 2 commits September 7, 2021 09:39

qt, refactor: Keep HelpMessageDialog in the main event loop

332dea2

qt, refactor: Allocate SendConfirmationDialog instances on heap

b4e0d2c

This change is require for the next commit.

hebasto force-pushed the 210516-loop branch from 2407f0d to 94c395d Compare September 7, 2021 06:39

promag suggested changes Sep 29, 2021

View reviewed changes

src/qt/bitcoingui.h Outdated Show resolved Hide resolved

hebasto added 2 commits September 29, 2021 17:01

qt: Do not exit and re-enter main event loop during shutdown

f3a17bb

qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot

451ca24

hebasto force-pushed the 210516-loop branch from 94c395d to 451ca24 Compare September 29, 2021 14:10

promag reviewed Sep 29, 2021

View reviewed changes

laanwj merged commit 81e7748 into bitcoin-core:master Sep 30, 2021

hebasto deleted the 210516-loop branch September 30, 2021 10:04

shaavan reviewed Sep 30, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 30, 2021

Merge bitcoin-core/gui#336: Do not exit and re-enter main event loop …

03627b4

hebasto mentioned this pull request Dec 17, 2021

Respect dialog modality and fix a regression in wallet unlock #509

Merged

Sjors mentioned this pull request Feb 8, 2022

Toggle chain from menu (using Settings) #414

Closed

hebasto mentioned this pull request Feb 12, 2022

Override BitcoinApplication::event() to handle QEvent::Quit #547

Merged

hebasto mentioned this pull request May 30, 2022

test, refactor: Use the same shutdown path as in bitcoin-qt binary #611

Closed

bitcoin-core locked and limited conversation to collaborators Dec 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented May 16, 2021 •

edited

Loading

jarolrod left a comment •

edited

Loading

hebasto commented May 19, 2021

hebasto commented May 29, 2021

hebasto commented Jun 7, 2021

hebasto commented Jun 7, 2021

hebasto commented Jun 11, 2021

hebasto commented Jul 22, 2021

hebasto commented Aug 14, 2021

hebasto commented Aug 26, 2021

hebasto commented Sep 7, 2021

laanwj commented Sep 29, 2021

promag left a comment

hebasto commented Sep 29, 2021

promag left a comment

hebasto commented Sep 29, 2021

shaavan commented Sep 29, 2021

laanwj commented Sep 30, 2021

shaavan left a comment

ghost commented Dec 16, 2021

hebasto commented Dec 16, 2021

hebasto commented Dec 17, 2021

ghost commented Dec 17, 2021

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

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

Conversation

hebasto commented May 16, 2021 • edited Loading

jarolrod left a comment • edited Loading

Choose a reason for hiding this comment

hebasto commented May 19, 2021

hebasto commented May 29, 2021

hebasto commented Jun 7, 2021

hebasto commented Jun 7, 2021

hebasto commented Jun 11, 2021

hebasto commented Jul 22, 2021

hebasto commented Aug 14, 2021

hebasto commented Aug 26, 2021

hebasto commented Sep 7, 2021

laanwj commented Sep 29, 2021

promag left a comment

Choose a reason for hiding this comment

hebasto commented Sep 29, 2021

promag left a comment

Choose a reason for hiding this comment

hebasto commented Sep 29, 2021

shaavan commented Sep 29, 2021

laanwj commented Sep 30, 2021

shaavan left a comment

Choose a reason for hiding this comment

ghost commented Dec 16, 2021

hebasto commented Dec 16, 2021

hebasto commented Dec 17, 2021

ghost commented Dec 17, 2021

hebasto commented May 16, 2021 •

edited

Loading

jarolrod left a comment •

edited

Loading