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

Screen Lock / Unlock events #12714

Merged
merged 2 commits into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@MarshallOfSound
Member

MarshallOfSound commented Apr 26, 2018

Closes #12706

@MarshallOfSound MarshallOfSound requested review from electron/docs as code owners Apr 26, 2018

@MarshallOfSound MarshallOfSound changed the title from [WIP] Screen Lock / Unlock events to Screen Lock / Unlock events Apr 26, 2018

@codebytere

This looks good to me, although I would ask if it's possible to test this?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 28, 2018

I though about tests for a while, locking / unlocking a machine during CI runs seems like a good way to break CI so this is probably untestable

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 29, 2018

Can you use anything from Chromium's //ui/base/idle/?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Apr 29, 2018

@alexeykuzmin Unfortunately not, the idle logic in Chromium does do similar stuff (checking for screen lock) but doesn't emit events / monitor when it changes.

@MarshallOfSound MarshallOfSound merged commit 338a816 into master Apr 30, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the screen-lock-events branch Apr 30, 2018

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 2, 2018

@MarshallOfSound

To implement chrome.idle.onStateChanged event, Chromium uses stuff from //ui/base/idle and polling, check the extensions/browser/api/idle/idle_manager.cc.

Can we do something similar? I'm not a big fan of reimplementing something that Chromium already has, especially platform-specific.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 2, 2018

@alexeykuzmin Hooking into events is gonna be way more performant than polling the idle state API. And although the macOS implementation of this contains some similar code to idle the windows implementation is completely new as idle on windows doesn't have a concept of locked afaics. It's not too much code for a pretty performant event implementation

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 2, 2018

@MarshallOfSound

the windows implementation is completely new as idle on windows doesn't have a concept of locked afaics

It does. https://chromium.googlesource.com/chromium/src.git/+/master/ui/base/idle/idle_win.cc#53

bool CheckIdleStateIsLocked() {
  return ui::IsWorkstationLocked() || IsScreensaverRunning();
}
@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 2, 2018

@MarshallOfSound
By "performance" here you mean a delay between a system event and an Electron event?
Polling once a second the typical delay will be about 0.5 seconds, for what use case it not acceptable?
Not using an already existing code you just increase a cost of project maintenance.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 2, 2018

I don't really care about this piece of code, it doesn't matter.
But what I care about is the approach we use, and the priorities we have.

To live in a long run a project has to be maintainable, it has to be able to survive constant changes.
Trading maintainability for performance should be a tough decision with a good reasoning.
Like if you cannot measure a performance gain your changes make, why even bother making them?
No matter how performant and bug-free a certain piece of code is now, at some point in time it will have to be changed, and if it is too complex and doesn't have a good test suite, it will gradually loose its speed and get bugs, and eventually will be thrown away.

I'm not talking about the code in this PR btw =)
But I think we have to have reasons to write new code better than because it's easy and we like to do it.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 7, 2018

@alexeykuzmin

Polling once a second the typical delay will be about 0.5 seconds, for what use case it not acceptable?

In most cases, that's totally OK, but when the machine is locked on macOS process's can and will get slept, you need to do your stuff as fast as possible.

Happy to take this offline to discuss but in general I agree, premature optimization is a bad thing but in this case timing was key and using event based native logic seemed the only way to go. (Initially I did try polling native idle API's using the node-idle-time module (or something like that))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment