Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-8186: Workaround for application.confirmExit #8732

Merged
merged 1 commit into from
Nov 18, 2020
Merged

GH-8186: Workaround for application.confirmExit #8732

merged 1 commit into from
Nov 18, 2020

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Nov 7, 2020

What it does

How to test

Electron env:

  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to never.
  • refresh the window with Ctrl/Cmd+R.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to never.
  • run the Reload Window command.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to never.
  • quit the app (on macOS with Cmd+Q)
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to never.
  • close the window by clicking on the X.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to never.
  • close the window by clicking on the X.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to always.
  • close or refresh the window (it's up to you what pick, we've already verified it in 1. - 5.).
  • see the modal dialog, select No.
    Expectation:
    • you must not hit the breakpoint.
    • the window does not close.
    • open src-gen/frontend/index.js and verify the LS is still running. Try the content assist.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to always.
  • Close or refresh the window (it's up to you what pick, we've already verified it in 1. - 5.).
  • see the modal dialog, select Yes.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to ifRequired.
  • open src-gen/frontend/index.js make sure it's not dirty.
  • close or refresh the window (it's up to you what pick, we've already verified it in 1. - 5.).
    Expectation:
    • you do not see the modal dialog.
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to ifRequired.
  • disable the auto-save.
  • open src-gen/frontend/index.js and make it dirty.
  • close or refresh the window (it's up to you what pick, we've already verified it in 1. - 5.).
  • see the modal dialog, select Yes.
    Expectation:
    • you must hit the breakpoint.
    • the window closes.
  • start the electron app in debug mode and open the dev-tools.
  • put a breakpoint to frontend-application.ts, line 190 (inside this.windowsService.onUnload).
  • set confirmExit to ifRequired.
  • disable the auto-save.
  • open src-gen/frontend/index.js and make it dirty.
  • close or refresh the window (it's up to you what pick, we've already verified it in 1. - 5.).
  • see the modal dialog, select No.
    Expectation:
    • you must not hit the breakpoint.
    • the window does not close.
    • open src-gen/frontend/index.js and verify the LS is still running. Try the content assist.

Browser env:

Essentially, do the same as in electron env. However, there is one case that cannot be verified the same way; when you reload the browser window. In such cases, you won't hit the breakpoint in the browser (I use Brave, but it should behave the same in Chrome) but there is a trick for this use-case. Start the backend in debug mode. Check the backend log; you must see root INFO Changed application state from 'ready' to 'closing_window'. when you reload the window even if you do not hit the breakpoint. If you start the backend in debug mode from VS Code, you can even filter the Debug Console with "Changed application state from" and you will see the expected state transition logs.

Review checklist

Reminder for reviewers

@zhaomenghuan
Copy link
Contributor

Thank you very much,I fix the following minor defects and I tested it runs fine.👍

packages/core/src/browser/window/test/mock-window-service.ts:20:14 - error TS2420: Class 'MockWindowService' incorrectly implements interface 'WindowService'.
  Property 'onUnload' is missing in type 'MockWindowService' but required in type 'WindowService'.

@kittaakos
Copy link
Contributor Author

Thank you so much for helping with this, @zhaomenghuan. The "confirm exit" seems to work, unfortunately, I noticed a critical issue; #6530 is back. I need to work on this PR.

@kittaakos kittaakos marked this pull request as ready for review November 12, 2020 15:30
@kittaakos kittaakos changed the title GH-8188: Workaround for application.confirmExit GH-8186: Workaround for application.confirmExit Nov 12, 2020
@kittaakos
Copy link
Contributor Author

kittaakos commented Nov 12, 2020

This is ready for review. I am not totally happy with removing the attachWillPreventUnload method, but it works now. I tried to test all the cases (on macOS) with both the electron and browser examples.

@marechal-p and @zhaomenghuan, could you please help with the verification?

@mcgordonite, I would like to kindly ask you to chime in and help with Windows's verification. I have a strong opinion on moving forward with my proposed change, and I do not want to break something on your side.

Thank you!

@kittaakos kittaakos force-pushed the GH-8186 branch 3 times, most recently from 3b6bbf5 to e2d309e Compare November 13, 2020 15:06
@kittaakos
Copy link
Contributor Author

@marechal-p, I fixed the issue you have noticed. Thank you! Please check if you are fine with the proposed change or not.

@paul-marechal
Copy link
Member

paul-marechal commented Nov 13, 2020

I made a fixup commit based on your work, I would be more comfortable with this logic: https://github.com/eclipse-theia/theia/compare/mp/GH-8186 I applied your "how to test" steps and everything went right on my machine (Windows 10) both for Electron and browser.

@kittaakos
Copy link
Contributor Author

I made a fixup commit based on your work,

Cool. I will take a look 👍

@kittaakos
Copy link
Contributor Author

I made a fixup commit based on your work, I would be more comfortable with this logic:

@marechal-p, I've verified it with both examples on macOS; it works. Let's go with your changes, and thank you for your time to help me. Could you please squash the commits but please do not change anything else.

 - Moved the `beforeunload` handling logic to the frontend.
 - Removed the `attachWillPreventUnload` from the main electron app.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@kittaakos
Copy link
Contributor Author

Thanks, @marechal-p 🙏

I leave the PR open for a few days to see if somebody has a remark.

@kittaakos kittaakos merged commit e209832 into master Nov 18, 2020
@kittaakos kittaakos deleted the GH-8186 branch November 18, 2020 07:26
@github-actions github-actions bot added this to the 1.8.0 milestone Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[electron] Backend processes are already terminated before 'will-prevent-unload' is called
3 participants