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

Encapsulate Electron Window Logic - Allow Exit during Load #10600

Merged
merged 5 commits into from
Feb 18, 2022

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #10586
This PR is a pretty big intervention into the logic of the ElectronMainApplication. The narrow goal is to track whether a window on the frontend is in a state to handle a close request so that windows that aren't ready to handle the close request can be closed immediately. The larger goal is to begin to extract the logic of handling Theia windows so that we can develop an abstraction that will work for both Electron and browser with an eye to doing things like running cleanup when the last window is closed. There are much smaller interventions that can address the narrow goal, so if there isn't an appetite for this size of refactor, that's fine, and I'm happy to reduce it to the minimum necessary to ensure that close gets handled correctly.

How to test

It would be nice to write automated tests for some of this logic, but I haven't interacted with our Electron-specific test system much. Any pointers are appreciated.

  1. Add some code that will prevent the frontend from ever achieving a ready state.

e.g. add a return to FrontendApplication#start that terminates it early.

  1. Run the application and try to close the window.
  2. It should close.

  1. Let the application get ready normally.
  2. Dirty a file, or do something else that would generate a close prompt.
  3. Observe that the prompt is displayed correctly and the application closes or doesn't close correctly
  4. Change the window.titleBarStyle (if not on Mac) preference and observe that the application restarts correctly.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work added electron issues related to the electron target proposal feature proposals (potential future features) labels Jan 5, 2022
@colin-grant-work colin-grant-work force-pushed the bugfix/permit-close-before-load branch from a904f2a to b837b7d Compare January 5, 2022 20:42
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong pull-request :)

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are looking really good to me! After applying the fix from #10597 I was able to successfully execute all testing steps:

  • Shutting off the application before it reaches its ready state is possible
  • Different configurations of the application.confirmOnExit preference, dirty editors, etc. work correctly when trying to exit the application
  • Commands such as Configure Display Language or Reset Workbench Layout still ask for confirmation for a reload
  • Switching between different window.titleBarStyle on different windows and reloading the windows continues to work correctly

packages/core/src/electron-main/theia-electron-window.ts Outdated Show resolved Hide resolved
@colin-grant-work colin-grant-work force-pushed the bugfix/permit-close-before-load branch from b837b7d to 4783f01 Compare January 19, 2022 20:24
@colin-grant-work
Copy link
Contributor Author

Thanks, @msujew. I'll merge this after the release.

@colin-grant-work colin-grant-work force-pushed the bugfix/permit-close-before-load branch 2 times, most recently from 7e47f0c to ed87621 Compare January 28, 2022 18:30
- Extracts logic for handling various window events from the
  ElectronMainApplication class into a TheiaElectronWindow wrapper class
- Adds communication about application state between window and the
  TheiaElectronWindow
@colin-grant-work colin-grant-work force-pushed the bugfix/permit-close-before-load branch from ed87621 to 5b03793 Compare February 7, 2022 21:40
@colin-grant-work colin-grant-work force-pushed the bugfix/permit-close-before-load branch from 870b600 to 95875fc Compare February 17, 2022 23:00
@colin-grant-work colin-grant-work merged commit f8c0527 into master Feb 18, 2022
@colin-grant-work colin-grant-work deleted the bugfix/permit-close-before-load branch February 18, 2022 17:07
@github-actions github-actions bot added this to the 1.23.0 milestone Feb 18, 2022
colin-grant-work pushed a commit to colin-grant-work/theia that referenced this pull request Feb 23, 2022
…heia#10600)

- Extracts logic for handling various window events from the
  ElectronMainApplication class into a TheiaElectronWindow wrapper class
- Adds communication about application state between window and the
  TheiaElectronWindow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target proposal feature proposals (potential future features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to close the window of theia electron version while loading
3 participants