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

feat: Windows implementation of powerMonitor "shutdown" event #15620

Closed
wants to merge 2 commits into from

Conversation

tarruda
Copy link
Contributor

@tarruda tarruda commented Nov 7, 2018

Description of Change

The powerMonitor "shutdown" event is only handled on Linux/Mac, this PR adds the Windows implementation.

Handling shutdown correctly on Windows is more complex than Linux/Mac, since any process can be killed during shutdown. That means renderer processes can be killed first (and usually are) , which could result in loss of data even if the event is handled in the browser process.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: Add Windows implementation of powerMonitor "shutdown" event

@tarruda tarruda requested review from zcbenz and a team November 7, 2018 15:25
@tarruda tarruda force-pushed the windows-powermonitor-shutdown-event branch 3 times, most recently from b796546 to 9acf27c Compare November 8, 2018 02:13
@tarruda tarruda force-pushed the windows-powermonitor-shutdown-event branch 3 times, most recently from 9c0f7e7 to d4a4a26 Compare November 8, 2018 17:08
@felixrieseberg
Copy link
Member

This is an excellent effort, thank you! We should add that apps only get five seconds to the docs (right?). I'd also 😍 love to see some specs!

@tarruda
Copy link
Contributor Author

tarruda commented Nov 9, 2018

@felixrieseberg I'm not sure how this could be tested considering it is mostly system interaction code. With the Linux version I was able to do some limited testing through the dbus-mock interface, but I'm not aware of anything similar to Windows.

In any case it seems this change is causing problems on some of the specs (even though our users have been using this code without issues for a few months now) so I still need to do some work.

@bpasero
Copy link
Contributor

bpasero commented Nov 24, 2018

VSCode would be very interested in a way to do a proper shutdown during OS restart, so 👍

@codebytere codebytere changed the title Add Windows implementation of powerMonitor "shutdown" event feat: Windows implementation of powerMonitor "shutdown" event Mar 7, 2019
@miniak
Copy link
Contributor

miniak commented May 4, 2019

@tarruda can you rebase on latest master?

@zcbenz zcbenz added the wip ⚒ label Jul 11, 2019
@codebytere
Copy link
Member

@tarruda please address latest comments or this PR will be closed in 10 days due to inactivity.

@tarruda
Copy link
Contributor Author

tarruda commented Jul 22, 2019

@tarruda please address latest comments or this PR will be closed in 10 days due to inactivity.

@codebytere In a few days I might have some work time allocated on this, and would like to keep this PR open. Is rebasing on latest master enough for now?

@codebytere
Copy link
Member

@tarruda yes, thanks!

@tarruda
Copy link
Contributor Author

tarruda commented Jul 22, 2019

@codebytere I no longer have write access to electron repository, so I can't push to this PR. Should I open another PR with the rebased branch in my electron copy (https://github.com/tarruda/electron/tree/windows-powermonitor-shutdown-event), or is there a way to edit this PR to use my fork?

@zcbenz
Copy link
Contributor

zcbenz commented Dec 16, 2019

@tarruda Yeah the only way seems to be opening a new PR, and please do.

@zcbenz zcbenz closed this Dec 16, 2019
@zcbenz zcbenz deleted the windows-powermonitor-shutdown-event branch December 16, 2019 01:43
@codebytere
Copy link
Member

@tarruda would you be opposed to me trying to pick this up? i'll ensure you have author credit in git!

@tarruda
Copy link
Contributor Author

tarruda commented Jun 23, 2020

@codebytere I don't mind at all, please go ahead 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants