Improve handling of close and reload events

[colin-grant-work]

What it does

• Fixes Dirty Files + Reset Workbench Layout ---> Exit Rather than Reload #10090 by implementing messaging between the Electron main application and Electron windows that runs instead of normal unload lifecycle, which allows Electron to resume the operation that was blocked (reload or close).
• Also modifies WindowService to allow for safe ready checks before programmatic close or reload in both browser and electron contexts.

Will help with resolving issues in #10357 and will help make it possible to show a custom message in #10374.

How to test

In Electron

1. Dirty an editor
2. Try to close / reload / reset workbench layout.
3. See a confirmation dialog.
   • If you say you're sure you want to exit, the operation should continue and close or reload appropriately.
   • If you say you're not sure, nothing should happen.

In Browser

1. Dirty an editor
2. Try to close / reload
3. See no in-app dialog, but see a browser confirmation dialog.
4. Try to reset workbench layout.
5. See same behavior as in Electron.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[msujew]

The confirmation seems to be triggered twice in my tests, both in the browser and in electron:

Browser:

2021-11-04.16-40-25.mp4

Electron (it's kind of difficult to see):

2021-11-04.16-47-03.mp4

[colin-grant-work]

@msujew good point. In the Browser, for the reset workbench layout, there's a check ahead of time (the in-app dialog), but it's still triggering the original check as well (the browser confirmation). I'm not sure I do see it in Electron mode - were you clicking on the same dialogue twice? I don't have an explanation for why it would happen in Electron, since the original check has been removed, but I'll do some further checking.

[colin-grant-work]

@msujew, there's another question in this area that falls under your expertise. For the window title bar preference, you've implemented a check that says that the application needs to be restarted, but you haven't done so for changing the configuration language, where VSCode also has a check. How do you think that should work - where in the process should we interrupt things? VSCode has it a bit easier - if it knows it's restarting, it does local backups of unsaved work, so there's only the check for whether you want to restart. In our case, we don't have a local backup system, so we want to tell the user we're going to restart and give them a chance to save their work, but that makes it tricky. There are at least three options:

• Show a dialog that says we're going to have to restart and warn the user to save their work. If they confirm, we do a veto-proof restart process without consulting FrontendApplicationContributions about whether they're ready to shut down or not (or ignoring what they say, anyway).
• Show the dialog indicating the need for a restart, then do a veto-able shutdown informing the user if there's anything they may need to do first. This is safe, but could leave us / the user in a bit of a weird state: they said they wanted to restart, but then said they didn't.
• We could implement a local backup system before we introduce a more elaborate veto system so that restarts can be handled as straightforwardly as in VSCode.

[msujew]

I'm not sure I do see it in Electron mode - were you clicking on the same dialogue twice?

Yes, my screencaping program doesn't capture it well (as it captures on 30fps) but I clicked on the same dialogue twice, and after a frame of seeing plain Theia the same dialogue appears again. Confirming the second one actually closes the application. Perhaps it only happens on my machine. ¯\_(ツ)_/¯

For the window title bar preference, you've implemented a check that says that the application needs to be restarted, but you haven't done so for changing the configuration language, where VSCode also has a check.

Right, I didn't implement that back then because a simple window reload was enough for Theia to apply a different locale. VSCode needs to restart the whole application (including its backend to apply different locales). Although thinking about it now, we probably should include a warning in there as well, as unsaved data can be lost during the reload process. But your current changes actually address this issue perfectly 👍 The difference to the title bar styling is that it required a whole electron window restart to apply.

so we want to tell the user we're going to restart and give them a chance to save their work, but that makes it tricky. There are at least three options

I like the 3rd proposal the most, as it is the most intuitive and user friendliest. Though, it is of course the most complex solution as well. However, I think the first option is good as well.

[colin-grant-work]

@msujew, I've pushed up changes that should remove the double prompts, adds confirmation to the language change restart, and adds the ability to tell the window service to ignore additional checks, so that one confirmation for restart for language change or window title bar style change is enough to ensure restart.

[msujew]

The changes look good to me:

• Reloading the browser window with dirty editors prompts the dialog once
• Changing the display language (with and without dirty edits) prompts the dialog once
• Changing the titleBarStyle prompts the dialog once
• The dialog behaves as expected on browser/electron apps.

I have some nitpicks in regards to localization, but everything else looks good.

[colin-grant-work]

@msujew, thanks for the comments. I've made the changes you suggested.

@vince-fugnitto, I converted your terminal code to use the new system: in Electron, it should only prevent exit if some terminal widget reports that it does have a child process. Browser should behave the same way as before.

[colin-grant-work]

@vince-fugnitto (forgot to implement the dialog for terminals; one moment)

[colin-grant-work]

@msujew, when creating the terminal dialog, I lifted the strings straight from VSCode (OK, I fixed a typo in the plural one... couldn't help it.), but I got these logs:

I suppose that means that they aren't in the set of translated strings / weren't in it at the version on which our translation set is based? In that case, what's the ideal procedure, since they likely will be in the set, eventually?

[msujew]

@colin-grant-work Hm, difficult one. As they aren't in the current set of translations, they won't be translated with the 1.53.2 versions of the vscode language packs (only newer ones). I would argue that we should use the usual nls.localize function for these cases, but add a todo comment (?) that these should be replaced once the appropriate version has been reached. The eslint rule should take care of this automatically.

[msujew]

Great, looks good to me!

• Editing a file without saving and closing the browser tab/electron window will display an appropriate message.
• Changing the display language or the title bar style will also show a message, which shadows the "unsaved files" message.
• The different values of terminal.integrated.confirmOnExit behave as expected and work well together with the other exiting criteria.

[colin-grant-work]

@tsmaeder, @JonasHelming, before we merge this, I just want to check with y'all whether you're OK with the changes to the WindowService interface and behavior. Basically the changes are these:

• remove canUnload(): boolean- it's replaced by isSafeToShutDown(): Promise<boolean> to allow asynchronous handling in Electron.
• add isSafeToShutDown() - it replaces canUnload(), because it seemed safer to change the name so that code that expected a boolean return didn't keep running on Promise<boolean> (always truthy).
• add setSafeToShutDown() - handles user-triggered shutdowns that have their own confirmation systems so that we avoid also triggering the default shutdown handlers.
• add reload() - so that Electron and browser WindowServices can handle reloading differently.

[tsmaeder]

@colin-grant-work If I understand correctly, the breaking change is not really necessary in order to implement the fix to the missing reload on electron. As a general rule, I think it makes sense to separate bugfixes from new features in PR's. I don't think it's such a big deal in this case, though.

What I don't really understand is in what scenarios this is a breaking change for clients: I don't expect too many folks to call "canUnload", so the breakage would mostly for folks overriding WindowService, right?

[colin-grant-work]

@colin-grant-work If I understand correctly, the breaking change is not really necessary in order to implement the fix to the missing reload on electron. As a general rule, I think it makes sense to separate bugfixes from new features in PR's. I don't think it's such a big deal in this case, though.

If it sets your mind at ease, consider this an implementation of the feature of allowing variable (per contribution) and asynchronous handling of application shutdown. It only just happens to fix the reload bug because that was a symptom of the same underlying deficiency previously preventing us from doing asynchronous shutdown.

What I don't really understand is in what scenarios this is a breaking change for clients: I don't expect too many folks to call "canUnload", so the breakage would mostly for folks overriding WindowService, right?

Yes, the changes to the WindowService interface only affect adopters who override or otherwise interact with the WindowService.

[tsmaeder]

lgtm, on a high level :-)