Honour the close settings when quitting from the tray #4068
Conversation
|
Nice, thank you @Artem-OSSRevival! It looks like the Lint job is failing in CI. Would you mind formatting the code as recommended in the log? |
There was a problem hiding this comment.
Hi @Artem-OSSRevival, thank you for the PR!
This is certainly an improvement. There is only a case that I am thinking we could handle better (comment inline). I would like to know your thoughts.
Also, it appears you haven't signed the CLA yet? Would you mind doing so, please?
src/client/gui/lib/main.dart
Outdated
| if (!await windowManager.isVisible() || | ||
| await windowManager.isMinimized()) { | ||
| windowManager.showAndRestore(); | ||
| } |
There was a problem hiding this comment.
I am debating whether the window should be shown also when stopping all instances (case stop). It can take a while to stop all instances. If the window is visible, there's a spinner informing the user of what is going on, but not if it's hidden. So I think I am inclined that way. On the other hand, if there are no instances to stop, there is no point bringing the window back up...
There was a problem hiding this comment.
If there is no need for a dialogue show, a pop-up window can be just interfering... If someone closes the app from the tray, he hardly wants to see a pop-up window without need (which will disappear itself in a couple of seconds).
There was a problem hiding this comment.
I understand what you mean, that pop-up is not ideal, particularly if there are no instances to stop. However, depending on how many VMs there are and what they are doing, stopping them can take a lot longer than a couple of seconds (we even had to increase the timeout to 5 minutes in the past). Until they stop, there would be no feedback and it would just feel like the tray wasn't reacting.
There was a problem hiding this comment.
Yes, with a very long delay, it would be better to show the presence of a stopping process.
May be implemented something like:
final closeJob = ref.read(guiSettingProvider(onAppCloseKey);
if (closeJob != 'nothing' &&
(!await windowManager.isVisible() ||
await windowManager.isMinimized())
{
windowManager.showAndRestore();
}
switch (closeJob) { ...
or even restore only invisible:
case 'stop':
if (!await windowManager.isVisible()) windowManager.showAndRestore();
stopAllInstances();
default:
There was a problem hiding this comment.
Something like that, although I think we'd better drop the switch at that point. Perhaps something like, in line 169, get the setting and add || closeJob == 'nothing' to the if (and return). Then add an else to take care of stop and default cases, where the window should be restored if invisible/minimized. WDYT?
There was a problem hiding this comment.
Yes, I thought about it initially, but was afraid that you would not want to get rid of switch...case. Without the switch, I can do something like this (already tried locally):
void onWindowClose() async {
if (!await windowManager.isPreventClose()) return;
final daemonAvailable = ref.read(daemonAvailableProvider);
final vmsRunning =
ref.read(vmStatusesProvider).values.contains(Status.RUNNING);
final closeJob = ref.read(guiSettingProvider(onAppCloseKey));
// nothing to do
if (!daemonAvailable ||
!vmsRunning ||
closeJob == 'nothing') {
windowManager.destroy();
return;
}
// checking the need to restore the window
if (!await windowManager.isVisible() ||
await windowManager.isMinimized()) {
windowManager.showAndRestore();
}
stopAllInstances() {
final notificationsNotifier = ref.read(notificationsProvider.notifier);
notificationsNotifier.addOperation(
ref.read(grpcClientProvider).stop([]),
loading: 'Stopping all instances',
onError: (error) => 'Failed to stop all instances: $error',
onSuccess: (_) {
windowManager.destroy();
return 'Stopped all instances';
},
);
}
if (closeJob == 'ask') {
showDialog(
context: context,
barrierDismissible: false,
builder: (context) => BeforeQuitDialog(
onStop: (remember) {
ref
.read(guiSettingProvider(onAppCloseKey).notifier)
.set(remember ? 'stop' : 'ask');
stopAllInstances();
Navigator.pop(context);
},
onKeep: (remember) {
ref
.read(guiSettingProvider(onAppCloseKey).notifier)
.set(remember ? 'nothing' : 'ask');
windowManager.destroy();
},
),
);
} else {
stopAllInstances();
}
}
There was a problem hiding this comment.
Yep, that makes the most sense to me.
f5cd212 to
9c77a59
Compare
|
Hey @Artem-OSSRevival, would you mind rebasing this and doing a formatting pass to get it through CI? Thanks! |
Previously, the tray menu action "Quit" caused a function to immediately destroy the window. Now the usual process of closing window is taking place. Signed-off-by: Artem <artem.vlasenko@ossrevival.org>
|
Hi @Artem-OSSRevival, our CI is unfortunately unable to deal with external PRs properly ATM. I have opened #4134 for that, but for the time being we'll need to push your branch to our repo and make a clone of this PR. You will still retain authorship of the commits, of course, and we'll be sure to mention you in the PR that gets merged! I hope you don't mind 🙂 Thanks again for your valuable contribution! |
|
Hi @ricab. No problems. The main thing is the prosperity of the project :) |
1da2a90
Closes: #3919
Previously, the tray menu action 'Quit' caused a function to immediately destroy the window. Now the usual process of closing window is taking place. This allows to consider the settings and perform the necessary tasks before closing.
Additionally, the restoration functionality of a hidden/minimized window has been added if there is a setting of the need to show a dialog before closing ('Ask about running instances').
To reproduce:
Set setting 'Stop running instances' or 'Ask about running instances'
Launch instance:
Don't close the app Window, but Quit from the tray.
Before:
After:
