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

Fixed confirmExit behaviour for Electron apps #6285

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Fixed confirmExit behaviour for Electron apps #6285

merged 1 commit into from
Oct 8, 2019

Conversation

mcgordonite
Copy link
Contributor

What it does

This PR restores the confirmation dialog shown in Electron mode when the user tries to exit without saving changes. On master, there is no confirmation, the app exits without saving.

This relates to an existing Electron issue: electron/electron#9966. Not opening a dialog in the beforeunload handler and using the will-prevent-unload event gets around this.

theia-confirm

How to test

Start the Electron app then disable Auto Save. Either:

  • Set the "application.confirmExit" preference to "ifRequired", make a change in a file but do not save it.
  • Set the "application.confirmExit" preference to "always".

Then try:

  • Opening a new workspace.
  • Refreshing the page.
  • Closing the app.

In these cases, a confirmation dialog should be displayed.

  • Clicking "Yes" should close the modal and unload the window.
  • Clicking "No" should close the modal without unloading.

In all other cases, the app should behave as before with no confirmation modal.

Review checklist

Reminder for reviewers

The previous implementation did not work because the window was unloaded
before the dialog could be opened.

This implementation gets around some… unusual behaviour of Electron's
beforeunload event by using the will-prevent-unload WebContents event.

Signed-off-by: Matthew Gordon <matthew.gordon@arm.com>
@vince-fugnitto vince-fugnitto added the electron issues related to the electron target label Sep 27, 2019
@kittaakos
Copy link
Contributor

I am verifying this...

@kittaakos
Copy link
Contributor

On master, there is no confirmation, the app exits without saving.

It works for me. See the commit hash and the behavior. Do not get me wrong, I do not mind switching to your solution, but I want to reproduce the problem fist. What should I do to not get notified before closing the application? Thanks!

ifRequired:
screencast 2019-10-07 17-09-44

always:
screencast 2019-10-07 17-14-03

@mcgordonite
Copy link
Contributor Author

@kittaakos that's interesting, I can reproduce it every time. I have tried it on:

  • Windows 10 (1809), node 10.16.1.
  • Ubuntu 19.04, node 10.16.3.
  • My colleague's mac.

I start from a fresh Theia clone, then:

  1. Checkout 24f5d20
  2. yarn
  3. yarn rebuild:electron
  4. cd examples/electron
  5. yarn start

Then I follow the steps in the PR description.

For what it's worth, here's Ubuntu 19.04 with always:
theia-ubuntu

Windows 10 with ifRequired:
theia-windows

One possible difference is that I use the close button in the title bar, but it also happens when I use the ALT-F4 shortcut.

@kittaakos
Copy link
Contributor

I can reproduce it every time

I will try it on my Windows image.

@kittaakos
Copy link
Contributor

I will try it on my Windows image.

The master state is broken on Windows so I am going to review your changes and merge if it is OK.

buttons: ['Yes', 'No'],
title: 'Confirm',
message: 'Are you sure you want to quit?',
detail: 'Any unsaved changes will not be saved.'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the default Chrome message is: https://github.com/eclipse-theia/theia/pull/6285/files#diff-b23416bfa30b320828382edc2ba8ea30L35
Do we want to align electron with Chome?

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have verified it on my Windows image; it worked nicely.

Thank you for your contribution, @mcgordonite 👍

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants