Ctrl+C must work even if a dialog window is open #11876

kiminuo · 2023-11-05T20:46:36Z

Fixes #11803
Alternative to #11857
Relevant read here

This PR is my take how I would approch the issue. I'm not sure if details on the UI side are OK or not. Review from somebody who understands that part of code would be very useful.

UI decoupling was not taken into account as well. If somebody wants to take over this PR, feel free to do so.

edit: It was found during review that this PR does not work when Run in background is turned on. Thanks for pointing it out.

… allow "shutdown" unconditionally

kiminuo · 2023-11-05T20:47:50Z

cc @AlexisKv @turbolay

MarnixCroes

is this OS specific? because for me I can still never shutdown with Ctrl C #11544

kiminuo · 2023-11-06T10:31:55Z

is this OS specific? because for me I can still never shutdown with Ctrl C #11544

It is not supposed to be OS-specific. I tested it on Windows and it seems to work for me. I can test on Linux, that is your OS, right?

MarnixCroes · 2023-11-06T10:43:02Z

is this OS specific? because for me I can still never shutdown with Ctrl C #11544

It is not supposed to be OS-specific. I tested it on Windows and it seems to work for me. I can test on Linux, that is your OS, right?

yes, debian

kiminuo · 2023-11-06T10:52:03Z

@MarnixCroes Does it work if you wait until the wallet is fully loaded? I.e. outside the scope of #11544.

MarnixCroes · 2023-11-06T11:00:14Z

@MarnixCroes Does it work if you wait until the wallet is fully loaded? I.e. outside the scope of #11544.

I can never shutdown fluent.desktop with ctrl c (master).
(So I can't test this PR)
So my comment #11876 (review) was somewhat unrelated to the PR, sorry for the confusion.

AlexisKv · 2023-11-06T11:45:10Z

This solution doesn't close wasabi in the background. And I think it should close fully app from terminal.
You could change e.Cancel = false , but I have noticed that this solution could incorrect close process

private void Console_CancelKeyPress(object? sender, ConsoleCancelEventArgs e)
	{
		Logger.LogWarning($"Process termination was requested using '{e.SpecialKey}' keyboard shortcut.");

		// Do not kill the process ...
		e.Cancel = true;

		// ... instead signal back that the app should terminate.
		SignalTerminate(ctrlCPressed: true);
	}

AlexisKv · 2023-11-06T11:50:03Z

As before, if you try to ctrl+c. than again open wallet, ctrl+c doesn't work anymore.

turbolay

This solution works perfectly on macOs.
All comments seem unrelated to the PR, and I believe should be tackled in subsequent ones:

As mentioned by @AlexisKv and discussed on Slack, this workflow should bypass the setting Run Wasabi in background after closing
It seems that there are some issues with Linux (in particular Debian?)

turbolay · 2023-11-06T12:10:24Z

WalletWasabi/Services/Terminate/TerminateService.cs

 	}

-	public void SignalTerminate()
+	public void SignalTerminate(bool ctrlCPressed)


I believe RPC stop command should also bypass the shutdown prevention. I would remove this parameter and always set result to true. We could also rename function SignalForceTerminate

I think you are right. I'll modify it.

Pushed 432eabc

AlexisKv · 2023-11-06T15:03:56Z

@MarnixCroes Does it work if you wait until the wallet is fully loaded? I.e. outside the scope of #11544.

I can never shutdown fluent.desktop with ctrl c (master). (So I can't test this PR) So my comment #11876 (review) was somewhat unrelated to the PR, sorry for the confusion.

Does Run on background feature is turned off in settings? @MarnixCroes
Can you turn it off and try to ctrl+c?

MarnixCroes · 2023-11-06T15:13:13Z

@MarnixCroes Does it work if you wait until the wallet is fully loaded? I.e. outside the scope of #11544.

I can never shutdown fluent.desktop with ctrl c (master). (So I can't test this PR) So my comment #11876 (review) was somewhat unrelated to the PR, sorry for the confusion.

Does Run on background feature is turned off in settings? @MarnixCroes Can you turn it off and try to ctrl+c?

Run in background is turned on.
Turning it off makes Ctrl C work.
(running master)

turbolay · 2023-11-06T15:39:58Z

@kiminuo So problem was not about linux, but about the "run into background option". We should fix it in another PR, as this one is effectively fixing the issue mentioned. I asked review from @wieslawsoltes, if UI changes are OK please consider #11876 (comment) and proceed to merge.

AlexisKv

It fixes issue. Looks good.

…CtrlC-when-dialog-is-open

MarnixCroes

tack 432eabc
also tested stop RPC #11876 (comment)

adamPetho

tACK on Win11

kiminuo added 3 commits November 5, 2023 21:43

TerminateService: Make it clear how software termination was requested

6ab607d

Services: Add TerminationService

16ee9f6

ApplicationViewModel: Use information about CTRL+C being pressed to…

91c8549

… allow "shutdown" unconditionally

pull-request-size bot added the size/M label Nov 5, 2023

kiminuo requested a review from turbolay November 5, 2023 20:47

MarnixCroes reviewed Nov 6, 2023

View reviewed changes

turbolay approved these changes Nov 6, 2023

View reviewed changes

MarnixCroes mentioned this pull request Nov 6, 2023

Wasabi doesn't shutdown after CTRL C, when run in background is enabled #11544

Closed

turbolay requested a review from wieslawsoltes November 6, 2023 15:38

AlexisKv approved these changes Nov 6, 2023

View reviewed changes

kiminuo requested a review from adamPetho November 6, 2023 20:02

kiminuo added 2 commits November 6, 2023 21:43

Merge remote-tracking branch 'origin/master' into feature/2023-11-05-…

67675ec

…CtrlC-when-dialog-is-open

Feedback

432eabc

MarnixCroes approved these changes Nov 6, 2023

View reviewed changes

adamPetho approved these changes Nov 7, 2023

View reviewed changes

kiminuo merged commit b14fecc into WalletWasabi:master Nov 7, 2023
7 checks passed

kiminuo deleted the feature/2023-11-05-CtrlC-when-dialog-is-open branch November 7, 2023 10:10

kiminuo mentioned this pull request Nov 10, 2023

Close prevention #11857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ctrl+C must work even if a dialog window is open #11876

Ctrl+C must work even if a dialog window is open #11876

kiminuo commented Nov 5, 2023 •

edited

kiminuo commented Nov 5, 2023

MarnixCroes left a comment

kiminuo commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023

kiminuo commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023 •

edited

AlexisKv commented Nov 6, 2023

AlexisKv commented Nov 6, 2023

turbolay left a comment

turbolay Nov 6, 2023

kiminuo Nov 6, 2023

kiminuo Nov 6, 2023

AlexisKv commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023

turbolay commented Nov 6, 2023

AlexisKv left a comment

MarnixCroes left a comment

adamPetho left a comment

Ctrl+C must work even if a dialog window is open #11876

Ctrl+C must work even if a dialog window is open #11876

Conversation

kiminuo commented Nov 5, 2023 • edited

kiminuo commented Nov 5, 2023

MarnixCroes left a comment

Choose a reason for hiding this comment

kiminuo commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023

kiminuo commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023 • edited

AlexisKv commented Nov 6, 2023

AlexisKv commented Nov 6, 2023

turbolay left a comment

Choose a reason for hiding this comment

turbolay Nov 6, 2023

Choose a reason for hiding this comment

kiminuo Nov 6, 2023

Choose a reason for hiding this comment

kiminuo Nov 6, 2023

Choose a reason for hiding this comment

AlexisKv commented Nov 6, 2023

MarnixCroes commented Nov 6, 2023

turbolay commented Nov 6, 2023

AlexisKv left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

adamPetho left a comment

Choose a reason for hiding this comment

kiminuo commented Nov 5, 2023 •

edited

MarnixCroes commented Nov 6, 2023 •

edited