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

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

As pointed in bitcoin/bitcoin#23790 a regression in wallet unlock was introduced in #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:

This is confusing and must be avoided.

Fixes bitcoin/bitcoin#23790.

ghost · 2021-12-17T18:29:44Z

I was unable to reproduce the issue mentioned in bitcoin/bitcoin#23790 using PR branch, so issue might be fixed. There was a password prompt each time I tried to send bitcoin from an encrypted wallet.

Would like to test few more things and go through each commit since there are 11 commits in this PR before ACK

willyko · 2021-12-18T02:18:51Z

testedACK for the issue mentioned in bitcoin/bitcoin#23790

hebasto · 2021-12-18T18:29:43Z

Updated 702a243 -> 80e7e30 (pr509.01 -> pr509.02, diff).

Dropped QAction::setStatusTip() calls:

status tip works only for top-level parent widget, tray icon's QMenu instance is not a top-level one
even on master this feature does not work consistently across all supported platforms
the tray icon context menu is not local to the main window status bar, so status tips are likely to be unnoticed by users

ghost · 2021-12-21T15:26:10Z

~~Is this error related to this PR in any way?~~ UPDATE: Its not related to PR. I tried testing this with 1 commit removed from PR branch in #399 but still got same error so I will create new issue for this.

I get this when I try to load a PSBT for signing. This does not happen in v22.0

PSBT:

cHNidP8BAFICAAAAAYLWIrucosFvAaD6/JAOgihjyt2xcshd2VdRQd2VUknSAAAAAAD9////ARAnAAAAAAAAFgAUX2caQkxHKburbHQctmMM7btDNunWiiAAAAEA3gIAAAAAAQE6UEKkqOk4pmFFsjgCdJWcr8o/e05sonzbu2r9WuUNIAEAAAAA/f///wINKAAAAAAAABYAFOgtXS/Ov298HhtS87BZe0K/OQ8ooIYBAAAAAAAWABQqd5hFsp4qwKj0vyfuSRN8nFEBoAJHMEQCIBIgXxY+MF/b4MsjbK3D1zE1bcrsIFYNOEA8WmHwAGZeAiBarlG/F19lu3vRY9YznD3rBy6pF5RnjSitGdxUpIml9wEhAhtZklcfD/gpfbAtqeqetj+NSHzadZkQp2+odLAQEp/lp4cgAAAA

ghost

tACK 80e7e30

This PR fixes the bug reported in bitcoin/bitcoin#23790 and I did not find any issues with the code or testing other things.

ghost · 2021-12-24T11:57:17Z

There is some error in CI though which I missed earlier: https://github.com/bitcoin-core/gui/pull/509/checks?check_run_id=4570631121

hebasto · 2021-12-26T09:13:27Z

Updated 80e7e30 -> 356e21f (pr509.02 -> pr509.03, diff).

Silenced -Wunused-variable warnings for macOS.

ghost

reACK 356e21f

promag · 2022-01-05T01:35:11Z

@hebasto have you tried to fix the regression without reverting the other change?

hebasto · 2022-01-05T16:09:20Z

@promag

@hebasto have you tried to fix the regression without reverting the other change?

Yes. Although, did not finish it. Changes got more and more evolved.

promag · 2022-01-10T09:35:03Z

@hebasto I can see why, there are multiple cases. Not sure how I didn't catch unlockWallet blocking case on #336 (review) review.

I think the best course of action is to revert and follow up with necessary refactors.

Concept ACK.

w0xlt

tACK 356e21f

Verified that the error mentioned in bitcoin/bitcoin#23790 happens in master branch and this PR fixes it.

Sjors

a632303 indeed fixes the no lock after send regression for me. But it introduces some (much less problematic) regressions, at least on macOS.

src/qt/bitcoingui.cpp

Sjors · 2022-02-08T11:01:19Z

src/qt/bitcoingui.cpp

-                sign_action->setEnabled(signMessageAction->isEnabled());
-                verify_action->setEnabled(verifyMessageAction->isEnabled());
+            if (QApplication::activeModalWidget()) {
+                for (QAction* a : trayIconMenu.get()->actions()) {


97692af: I don't see any difference in behavior on macOS; before and after this commit, if I open coin selection, it disables everything except the macOS standard items (Options, Show all windows, Hide, Stop)

Sjors · 2022-02-08T11:06:26Z

src/qt/bitcoin.cpp

@@ -265,7 +265,11 @@ void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
    connect(window, &BitcoinGUI::quitRequested, this, &BitcoinApplication::requestShutdown);

    pollShutdownTimer = new QTimer(window);
-    connect(pollShutdownTimer, &QTimer::timeout, window, &BitcoinGUI::detectShutdown);
+    connect(pollShutdownTimer, &QTimer::timeout, [this]{


cda881a : on macOS when I'm in coin control, before and after this commit, Command + Q is ignored (not delayed). Same with Stop from the tray menu. (that's fine by the way, a delayed shutdown seems confusing)

But when I hit ctrl + c in the terminal, before this commit it would shut QT down, with this commit it indeed delays the shutdown until the dialog is closed. (that's also fine, because this method of shutdown is probably only used by developers)

This change is required for the following commits.

Also dropped useless tooltip.

Sjors

tACK 74e638a

Sjors · 2022-02-08T16:50:31Z

src/qt/bitcoingui.cpp

+            if (show_hide_action) show_hide_action->setText(
+                (!isHidden() && !isMinimized() && !GUIUtil::isObscured(this)) ?
+                    tr("&Hide") :
+                    tr("S&how"));


fd667e7: you may want to delay this keyboard shortcut, because we're past the translation cut-off?

src/qt/bitcoingui.cpp

hebasto · 2022-02-09T04:10:21Z

@Sjors

you may want to delay this keyboard shortcut, because we're past the translation cut-off?

Change is small, so it's okay to get it merged before string freeze (now we are in "soft translation string freeze").

@prayank23 @w0xlt @promag

Mind having another look into this PR?

hebasto · 2022-02-09T05:04:15Z

On macOS another behavior change noticed. On master, due to

gui/src/qt/bitcoingui.cpp

Line 316 in b7942c9

optionsAction->setMenuRole(QAction::PreferencesRole);

optionsAction has forcibly been hidden in the Dock menu (according to design guide, it is moved into application menu):

With this PR options_action has no QAction::PreferencesRole set, and it is active:

@Sjors What do think about this behavior change?

Sjors · 2022-02-09T09:39:21Z

It seems confusing to have "Options..." and "Options ->" in a single dropdown. I don't care deeply, but maybe just keep the original behavior?

ghost · 2022-02-10T15:41:11Z

@prayank23 @w0xlt @promag

Mind having another look into this PR?

I don't care about UI issues with Mac but this pull request addresses an issue which affects security on every platform, works as expected so I have no issues with it. Would have been better if reverting changes were simple and less line of code in this PR. Hopefully we don't ship v23.0 with a known issue that affects security.

ACK 74e638a

This change is required for the following commit.

The AskPassphraseDialog modal dialog must be synchronous here as expected in the WalletModel::requestUnlock() function. Fixed an introduced regression.

-BEGIN VERIFY SCRIPT- sed -i 's/ShowModalDialogAndDeleteOnClose/ShowModalDialogAsynchronously/' -- $(git grep -l -e "ShowModalDialogAndDeleteOnClose") -END VERIFY SCRIPT- It is important to highlight that a modal dialog is showed asynchronously as there are cases when the synchronous QDialog::exec() is required.

hebasto · 2022-02-12T21:12:50Z

Updated 74e638a -> f730bd7 (pr509.04 -> pr509.05, diff).

It seems confusing to have "Options..." and "Options ->" in a single dropdown. I don't care deeply, but maybe just keep the original behavior?

Fixed.

hebasto · 2022-02-14T07:52:54Z

@prayank23 @Sjors

Would you mind re-acking?

ghost

tACK f730bd7

Tried on Fedora and found no issues.

Sjors · 2022-02-15T10:21:38Z

Post merge tACK, only difference is the PreferencesRole and the duplicate Options is gone.

jarolrod · 2022-02-15T11:58:12Z

Post merge tACK, I was able to reproduce the original issue and I've done testing to ensure that this PR fixes this.

c515829 qt: Update translation source file (Hennadii Stepanov) Pull request description: As the part of the v23.0 [release process](bitcoin/bitcoin#22969) this PR updates translation source file, `src/qt/locale/bitcoin_en.xlf`. The following changes are reflected in this PR: - small string changes from #509 - internal technical details from bitcoin/bitcoin#22151 ACKs for top commit: laanwj: ACK c515829 Tree-SHA512: 2cf08f5b356dca25f99b0342645db5253eab0854796cf44fa52f8a6cf28f6d3f973e21589e0f9d3fef40a1b21b3f0aee00c9ca0897109a1967f9ef3320dd508f

Sjors · 2022-03-29T11:12:42Z

I wonder if this was also a problem when creating a wallet, see #571.

This was referenced Dec 17, 2021

qt, wallet: wallet does not lock after first send bitcoin/bitcoin#23790

Closed

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

Merged

hebasto added this to the 23.0 milestone Dec 17, 2021

hebasto added Bug Something isn't working Wallet labels Dec 17, 2021

hebasto force-pushed the 211217-unlock branch from 702a243 to 80e7e30 Compare December 18, 2021 18:19

ghost approved these changes Dec 24, 2021

View reviewed changes

ghost mentioned this pull request Dec 24, 2021

Unable to load PSBT #511

Open

hebasto force-pushed the 211217-unlock branch from 80e7e30 to 356e21f Compare December 26, 2021 09:11

ghost approved these changes Dec 26, 2021

View reviewed changes

w0xlt approved these changes Jan 23, 2022

View reviewed changes

hebasto requested a review from achow101 January 24, 2022 08:12

Sjors reviewed Feb 8, 2022

View reviewed changes

hebasto added 2 commits February 8, 2022 16:19

qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda

c3ca836

qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda

66afa28

hebasto force-pushed the 211217-unlock branch from 356e21f to 3daaa4c Compare February 8, 2022 16:20

hebasto marked this pull request as draft February 8, 2022 16:29

hebasto added 4 commits February 8, 2022 18:41

qt, refactor: Fill up trayIconMenu before connections

78189da

This change is required for the following commits.

qt: Drop BitcoinGUI::toggleHideAction member

ee151d0

Also dropped useless tooltip.

qt: Make show_hide_action dependent on the main window actual state

fd667e7

qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members

58e1603

hebasto force-pushed the 211217-unlock branch from 3daaa4c to 74e638a Compare February 8, 2022 16:42

hebasto marked this pull request as ready for review February 8, 2022 16:43

Sjors approved these changes Feb 8, 2022

View reviewed changes

hebasto added 5 commits February 12, 2022 23:00

qt, refactor: Use local QAction instances for the tray icon menu

9242735

This change is required for the following commit.

qt: Disable tray icon menu when a modal dialog is active

8c0eb80

qt: Delay shutdown while a modal dialog is active

89c277a

qt: Revert 7fa91e8 partially

5d7666b

The AskPassphraseDialog modal dialog must be synchronous here as expected in the WalletModel::requestUnlock() function. Fixed an introduced regression.

hebasto force-pushed the 211217-unlock branch from 74e638a to f730bd7 Compare February 12, 2022 21:09

ghost approved these changes Feb 14, 2022

View reviewed changes

hebasto requested a review from jarolrod February 14, 2022 22:24

hebasto merged commit 1695d66 into bitcoin-core:master Feb 15, 2022

hebasto deleted the 211217-unlock branch February 15, 2022 06:39

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 15, 2022

Merge bitcoin-core/gui#509: Respect dialog modality and fix a regress…

1aaffec

rebroad mentioned this pull request Feb 15, 2022

regression testing for password tailgating #550

Open

hebasto mentioned this pull request Feb 18, 2022

qt: Update translation source file bitcoin/bitcoin#24377

Merged

Sjors mentioned this pull request Mar 29, 2022

Bech32m unavailable for encrypted wallets #571

Closed

bitcoin-core locked and limited conversation to collaborators Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented Dec 17, 2021 •

edited

Loading

ghost commented Dec 17, 2021

willyko commented Dec 18, 2021

hebasto commented Dec 18, 2021

ghost commented Dec 21, 2021 •

edited by ghost

Loading

ghost left a comment

ghost commented Dec 24, 2021

hebasto commented Dec 26, 2021

ghost left a comment

promag commented Jan 5, 2022

hebasto commented Jan 5, 2022

promag commented Jan 10, 2022

w0xlt left a comment

Sjors left a comment •

edited

Loading

Sjors Feb 8, 2022

Sjors Feb 8, 2022 •

edited

Loading

Sjors left a comment

Sjors Feb 8, 2022

hebasto commented Feb 9, 2022

hebasto commented Feb 9, 2022 •

edited

Loading

Sjors commented Feb 9, 2022

ghost commented Feb 10, 2022

hebasto commented Feb 12, 2022

hebasto commented Feb 14, 2022

ghost left a comment

Sjors commented Feb 15, 2022 •

edited

Loading

jarolrod commented Feb 15, 2022

Sjors commented Mar 29, 2022

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

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

Conversation

hebasto commented Dec 17, 2021 • edited Loading

ghost commented Dec 17, 2021

willyko commented Dec 18, 2021

hebasto commented Dec 18, 2021

ghost commented Dec 21, 2021 • edited by ghost Loading

ghost left a comment

Choose a reason for hiding this comment

ghost commented Dec 24, 2021

hebasto commented Dec 26, 2021

ghost left a comment

Choose a reason for hiding this comment

promag commented Jan 5, 2022

hebasto commented Jan 5, 2022

promag commented Jan 10, 2022

w0xlt left a comment

Choose a reason for hiding this comment

Sjors left a comment • edited Loading

Choose a reason for hiding this comment

Sjors Feb 8, 2022

Choose a reason for hiding this comment

Sjors Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Sjors Feb 8, 2022

Choose a reason for hiding this comment

hebasto commented Feb 9, 2022

hebasto commented Feb 9, 2022 • edited Loading

Sjors commented Feb 9, 2022

ghost commented Feb 10, 2022

hebasto commented Feb 12, 2022

hebasto commented Feb 14, 2022

ghost left a comment

Choose a reason for hiding this comment

Sjors commented Feb 15, 2022 • edited Loading

jarolrod commented Feb 15, 2022

Sjors commented Mar 29, 2022

hebasto commented Dec 17, 2021 •

edited

Loading

ghost commented Dec 21, 2021 •

edited by ghost

Loading

Sjors left a comment •

edited

Loading

Sjors Feb 8, 2022 •

edited

Loading

hebasto commented Feb 9, 2022 •

edited

Loading

Sjors commented Feb 15, 2022 •

edited

Loading