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-6530: Set the shutdown hook to beforeunload. #6532

Merged
merged 1 commit into from
Nov 13, 2019
Merged

GH-6530: Set the shutdown hook to beforeunload. #6532

merged 1 commit into from
Nov 13, 2019

Conversation

kittaakos
Copy link
Contributor

Closes #6530.

Signed-off-by: Akos Kitta [email protected]

What it does

This PR moves the frontend application's shutdown hook from unload to beforeunload as it the master state does not work with the electron example.

How to test

  • Build the electron example from the source,
  • refresh the browser a few times,
  • check the logs,
  • you can see the layout is stored. ✨

Review checklist

Reminder for reviewers

@kittaakos kittaakos force-pushed the GH-6530 branch 2 times, most recently from afe6b66 to 9e67168 Compare November 12, 2019 12:28
@akosyakov
Copy link
Member

We need to test the browser example as well.

@akosyakov akosyakov added electron issues related to the electron target shell issues related to the core shell labels Nov 12, 2019
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.

I verified both the electron and browser targets:

  • created a complex layout, performed reload and the layout was successfully restored.
  • logs successfully adds detail about the state of the application.

Are there any other use cases I need to validate?

 - `unload` was not triggered in electron. (See: electron/electron#7278 (comment))
 - Exposed `ShellLayoutRestorer` APIs.
 - Added logging.
 - Fixed typos.

Closes #6530.

Signed-off-by: Akos Kitta <[email protected]>
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.

I'm happy with the changes :)
We get more information about the state of the application when reloading, and restoring layout works correctly on both targets.

@kittaakos
Copy link
Contributor Author

I am going to merge this tomorrow, please object if you want.

@kittaakos kittaakos merged commit d7f7c90 into master Nov 13, 2019
@kittaakos kittaakos deleted the GH-6530 branch November 13, 2019 07:25
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 shell issues related to the core shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[electron] The 'unload' event listener on the window is never invoked inside electron
3 participants