feat(powerMonitor): expose interface to query system idle state #11807

kwonoj · 2018-02-01T18:52:05Z

This PR intend to expose 2 static method to powerMonitor - querySystemIdleState and querySystemIdleTime. As interface name stands for, both allows to query system's idle status.

There are handful of users need this features (including old backlog issue like #1528), and most cases user relies on native modules (https://www.npmjs.com/package/@paulcbetts/system-idle-time / https://www.npmjs.com/package/desktop-idle, etcs). This PR exposes interface to chromium's implementation removes need of custom native module.

QuerySystemIdleState(idleThreshold: number, 
                          (state: 'active' | 'idle' | 'locked' | 'unknown') => void): void

accepts non-negative number as a threshold in seconds to be considered as system idle, notify callback with current state. Callback can be resolved synchronously or either asynchronously, depends on platform support. Among state, locked state is only available if platform support to return it.

QuerySystemIdleTime((idleTime:number) => void): void

notifys callback with system idle time in seconds.

kwonoj · 2018-02-01T18:52:34Z

I am cc'ing @felixrieseberg here as well for the interested party.

felixrieseberg · 2018-02-01T19:18:19Z

Neat! I love re-using Chromium's well-crafted code. I'm not sure about BrowserWindow though - how do you feel about adding the API to shell instead?

kwonoj · 2018-02-01T19:20:31Z

I'm not sure about BrowserWindow though - how do you feel about adding the API to shell instead?

Good point, this is probably me not very well knowledgeable structure of electron. Let me try.

kwonoj · 2018-02-01T19:45:19Z

@felixrieseberg PR updated per suggestion, now interface is expoed via shell module.

deepak1556

I am not sure about adding it to the shell module when we have a powerMonitor module.

deepak1556 · 2018-02-01T19:47:49Z

atom/common/api/atom_api_shell.cc

+  dict.SetMethod("querySystemIdleTime", &QuerySystemIdleTime);
+
+#if defined(OS_MACOSX)
+  ui::InitIdleMonitor();


Should be moved to AtomBrowserMainParts::PreCreateThreads

kwonoj · 2018-02-01T20:05:50Z

I'm fine to pick anywhere to expose this. @deepak1556 Is suggestion moving this interface to powerMonitor or somewhere else? if it's else, where would be? and @felixrieseberg , what do you think?

deepak1556 · 2018-02-01T20:14:13Z

There isn't a strong argument for choosing between powerMonitor or shell. I will incline to what @felixrieseberg and other maintainers decides on this.

felixrieseberg · 2018-02-01T23:09:08Z

Nah, you're right, powerMonitor is probably better.

kwonoj · 2018-02-02T02:48:25Z

@deepak1556 @felixrieseberg now interface moved into powerMonitor'. it's class method, but I thought it's fine as powerMonitor binded into instance of class. Let me know if things need to be updated.

MarshallOfSound · 2018-02-02T02:51:39Z

atom/browser/api/atom_api_power_monitor.cc

+    ui::CalculateIdleState(idle_threshold, callback);
+  } else {
+    isolate->ThrowException(v8::Exception::TypeError(
+        mate::StringToV8(isolate, "Invalid idle threshold")));


Let's be super nice here, "Invalid idle threshold, must be greater than or equal to 0"

sounds good, message need to be bit amended to greater than 0, having 0 as threshold will not return valid value anyway (while CalculateIdleState can accepts it)

MarshallOfSound · 2018-02-02T02:52:04Z

docs/api/power-monitor.md

+
+The `powerMonitor` module has the following methods:
+
+#### `shell.querySystemIdleState(idle_threshold, callback)`


powerMonitor.

s/idle_threshold/idleThreshold

MarshallOfSound · 2018-02-02T02:52:09Z

docs/api/power-monitor.md

+before considered idle. State will be notified via callback represents current system
+idle state. `locked` is available on supported system only.
+
+#### `shell.querySystemIdleTime(callback)`


powerMonitor.

MarshallOfSound · 2018-02-02T02:52:26Z

docs/api/power-monitor.md

+
+#### `shell.querySystemIdleState(idle_threshold, callback)`
+
+* `idleThreshold` Interger


MarshallOfSound · 2018-02-02T02:53:07Z

docs/api/power-monitor.md

+
+* `idleThreshold` Interger
+* `callback` Function
+  * `active` | `idle` | `locked` | `unknown` String


This line should be

* `idleState` String - Can be `active, `idle`, `locked` or `unknown`

MarshallOfSound · 2018-02-02T02:53:26Z

docs/api/power-monitor.md

+* `callback` Function
+  * `active` | `idle` | `locked` | `unknown` String
+
+Calculate system idle state. `idle_threshold` is the amount of time (in seconds)


s/idle_threshold/idleThreshold

MarshallOfSound · 2018-02-02T02:53:44Z

docs/api/power-monitor.md

+
+Calculate system idle state. `idle_threshold` is the amount of time (in seconds)
+before considered idle. State will be notified via callback represents current system
+idle state. `locked` is available on supported system only.


Can we document which systems currently support locked?

chromium doesn't document it as well (https://cs.chromium.org/chromium/src/ui/base/idle/idle.h?q=idle&sq=package:chromium&l=17) - I'd rather in sync with chromium as it can change anytime due to platform side patch / version up, etcs.

MarshallOfSound · 2018-02-02T02:54:08Z

docs/api/power-monitor.md

+#### `shell.querySystemIdleTime(callback)`
+
+* `callback` Function
+


Should be another line here

* `idleTime` Integer - Idle time in seconds

kwonoj · 2018-02-02T03:07:28Z

PR updated & fixed mistakes per comment / suggestion.

deepak1556

Current specs of PowerMonitor run only on linux, we need to enable the tests for these new apis on all three platforms.

kwonoj · 2018-02-02T05:43:04Z

Noticed that - what's best way? Is it allowed to instantiate powermonitor without dbus mock? (New interface won't nees those mock anyway)

deepak1556 · 2018-02-02T06:32:54Z

Not having to lazily initialize the powerMonitor module should allow enabling these test cases along side the existing dbus specs for linux.

deepak1556 · 2018-02-02T07:29:33Z

Can remove the call to base::PowerMonitorDeviceSource::AllocateSystemIOPorts, its already initialized for the main process by the content layer https://cs.chromium.org/chromium/src/content/app/content_main_runner.cc?type=cs&q=AllocateSystemIOPorts&l=582

deepak1556 · 2018-02-02T07:35:55Z

spec/api-power-monitor-spec.js

@@ -41,7 +43,7 @@ const skip = process.platform !== 'linux' || !process.env.DBUS_SYSTEM_BUS_ADDRES
    before((done) => {
      logindMock.on('MethodCalled', onceMethodCalled(done))
      // lazy load powerMonitor after we listen to MethodCalled mock signal
-      powerMonitor = require('electron').remote.powerMonitor
+      dbusMockPowerMonitor = require('electron').remote.powerMonitor


@tarruda Is it required to lazy initialize the module ? I was hoping we can just initialize the module only once for all the specs here.

It has to be lazy loaded if we want to verify that electron inhibits the system suspend (it does this by calling a method via dbus).

kwonoj · 2018-02-02T17:52:25Z

atom/browser/api/atom_api_power_monitor.cc

@@ -68,10 +110,6 @@ using atom::api::PowerMonitor;

 void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
                v8::Local<v8::Context> context, void* priv) {
-#if defined(OS_MACOSX)
-  base::PowerMonitorDeviceSource::AllocateSystemIOPorts();


per comment suggestion in #11807 (comment) .

ckerr

Mostly LGTM, nice idea && thanks for the PR. 👍

A couple of questions and suggestions inline

ckerr · 2018-02-02T21:44:08Z

atom/browser/api/atom_api_power_monitor.h

+                            int idle_threshold,
+                            const ui::IdleCallback& callback);
+  void QuerySystemIdleTime(const ui::IdleTimeCallback& callback);
+


Let's make these methods private since we're only using them with the bindings. Permissions can be loosened later as needed

makes sense, moved it.

ckerr · 2018-02-02T21:53:10Z

atom/browser/api/atom_api_power_monitor.cc

+  } else {
+    isolate->ThrowException(v8::Exception::TypeError(
+        mate::StringToV8(isolate,
+          "Invalid idle threshold, must be greater than 0")));


I know another reviewer suggested this, but are we sure that we want this?

Since we're not publishing the ui::CheckIdleStateIsLocked(), if all you want to do is check for system lock it's reasonable to pass a -1 in for idle_threshold.

If we include that method, we can avoid this wart by having a recommended way to test for lock

hmm I didn't thought in that way, but also checking lock state is somewhat limited per platform support, so passing -1 doesn't guarantee you can reliable acquire intended state. I think it's rather limit api surface, and expose CheckIdleStateIsLocked separately if there's demand for it.

ckerr · 2018-02-02T21:54:25Z

docs/api/power-monitor.md

+  * `idleState` String - Can be `active`, `idle`, `locked` or `unknown`
+
+Calculate system idle state. `idleThreshold` is the amount of time (in seconds)
+before considered idle. State will be notified via callback represents current system


There's something missing between "notified via callback" and "represents current system", I think

it's wrapped line, full sentence is State will be notified via callback represents current system idle state..

That still doesn't make any sense though. Would this be ok?

+ Calculate the system idle state. `idleThreshold` is the amount of time (in seconds) + before considered idle. `callback` will be called synchronously on some systems + and with an `idleState` argument that describes the system's state.

I think so, it's probably same bucket for my English writing skill. I'll update as suggested.

ckerr · 2018-02-02T21:54:34Z

docs/api/power-monitor.md

+
+Calculate system idle state. `idleThreshold` is the amount of time (in seconds)
+before considered idle. State will be notified via callback represents current system
+idle state. `locked` is available on supported system only.


s/system/systems/

ckerr · 2018-02-02T21:56:46Z

atom/browser/api/atom_api_power_monitor.cc

+  mate::ObjectTemplateBuilder(isolate, prototype->PrototypeTemplate())
+      .MakeDestroyable()
+      .SetMethod("querySystemIdleState", &PowerMonitor::QuerySystemIdleState)
+      .SetMethod("querySystemIdleTime", &PowerMonitor::QuerySystemIdleTime);


Why do we name these with "query" instead of using libcc's "calculate" nomenclature?

Probably cause of my bad non-native-english-naming skill . 😅 open to suggestion if recommended to rename it.

😄 no worries!

My suggestion would be to copy the libchromium API names, e.g. PowerMonitor::CalculateIdleState and powerMonitor.calculateIdleState.

I don't feel very strongly about this; if other reviewers prefer querySystem I will go along

OK, I'll wait for few other chime in and update PR if we'd like to go for chromium api names.

kwonoj · 2018-02-09T17:45:51Z

I have rebased PR to master and seeing test failures on linux, peeking it some other PR sees as well (i.e https://circleci.com/gh/electron/electron/9330) not sure way to resolve it.

codebytere · 2018-02-09T18:30:14Z

@kwonoj it appears these are building in CI under your own name and not the electron org's, so we don't have the ability to restart them; often they flake so try restarting the failures yourself and see what happens

kwonoj · 2018-02-09T18:36:09Z

Bit unsure why fork CI setup reports status to PR itself. I've retriggered couple of times already, but did one more time.

kwonoj · 2018-02-09T23:12:28Z

@codebytere stopping my circleci config makes status notification correctly goes for electron's, but doesn't seem like build failure resolves by retriggering it.

felixrieseberg · 2018-02-12T19:32:21Z

@ckerr What do you think? Anything else OJ could do here to make it mergable?

MarshallOfSound

Loving this 👍

Just need to rebase changes onto current master so that arm64 builds don't blow up 👍

MarshallOfSound · 2018-03-14T02:44:06Z

To be clear for whoever merges this in the future as well, this does not get backported, new versioning scheme means that this will go out in 2.1.x or 3.0.x, we don't backport features 👍

ckerr

Apologies for not getting back to this sooner. Yes 👍 for landing this in master.

It would also make sense in the 2.x series when we open it back up to new nonbreaking features.

…tron#11807) * feat(BrowserWindow): expose interface to query system idle state * test(BrowserWindow): update test cases for querySystemIdle interface * docs(BrowserWindow): add querySystemIdle interface documentation * refactor(powerMonitor): move querySystemIdle into powerMonitor * test(powerMonitor): split test cases for all platform

sachetsharma1 · 2018-06-05T21:04:58Z

Is there any planned release for this feature?

dchest · 2018-10-02T17:22:20Z

Looks like this feature is missing in TypeScript typings.

kwonoj requested review from a team February 1, 2018 18:52

kwonoj changed the title ~~feat(BrowserWindow): expose interface to query system idle state~~ feat(shell): expose interface to query system idle state Feb 1, 2018

deepak1556 reviewed Feb 1, 2018

View reviewed changes

kwonoj changed the title ~~feat(shell): expose interface to query system idle state~~ feat(powerMonitor): expose interface to query system idle state Feb 2, 2018

MarshallOfSound reviewed Feb 2, 2018

View reviewed changes

deepak1556 reviewed Feb 2, 2018

View reviewed changes

kwonoj commented Feb 2, 2018

View reviewed changes

ghost approved these changes Feb 2, 2018

View reviewed changes

ckerr requested changes Feb 2, 2018

View reviewed changes

MarshallOfSound approved these changes Mar 14, 2018

View reviewed changes

kwonoj added 5 commits March 13, 2018 21:22

feat(BrowserWindow): expose interface to query system idle state

b6a4519

test(BrowserWindow): update test cases for querySystemIdle interface

64109da

docs(BrowserWindow): add querySystemIdle interface documentation

0180d73

refactor(powerMonitor): move querySystemIdle into powerMonitor

82cf1d1

test(powerMonitor): split test cases for all platform

1f3b731

MarshallOfSound added the semver/minor backwards-compatible functionality label Mar 14, 2018

ckerr approved these changes Mar 14, 2018

View reviewed changes

ckerr merged commit e7181eb into electron:master Mar 14, 2018

kwonoj deleted the feat-idle-time branch March 14, 2018 05:56

lundibundi mentioned this pull request Jun 30, 2018

start using querySystemIdleTime upon update to electron@3.0.0 hovancik/stretchly#258

Closed


		The `powerMonitor` module has the following methods:

		#### `shell.querySystemIdleState(idle_threshold, callback)`


		#### `shell.querySystemIdleState(idle_threshold, callback)`

		* `idleThreshold` Interger

		#### `shell.querySystemIdleTime(callback)`

		* `callback` Function

feat(powerMonitor): expose interface to query system idle state #11807

feat(powerMonitor): expose interface to query system idle state #11807

Conversation

kwonoj commented Feb 1, 2018 • edited

kwonoj commented Feb 1, 2018

felixrieseberg commented Feb 1, 2018

kwonoj commented Feb 1, 2018

kwonoj commented Feb 1, 2018

deepak1556 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Feb 1, 2018 • edited

deepak1556 commented Feb 1, 2018

felixrieseberg commented Feb 1, 2018

kwonoj commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Feb 2, 2018

deepak1556 left a comment

Choose a reason for hiding this comment

kwonoj commented Feb 2, 2018

deepak1556 commented Feb 2, 2018

deepak1556 commented Feb 2, 2018

deepak1556 Feb 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj commented Feb 9, 2018

codebytere commented Feb 9, 2018

kwonoj commented Feb 9, 2018

kwonoj commented Feb 9, 2018

felixrieseberg commented Feb 12, 2018 • edited

MarshallOfSound left a comment

Choose a reason for hiding this comment

MarshallOfSound commented Mar 14, 2018 • edited

ckerr left a comment

Choose a reason for hiding this comment

sachetsharma1 commented Jun 5, 2018

dchest commented Oct 2, 2018 • edited

kwonoj commented Feb 1, 2018 •

edited

kwonoj commented Feb 1, 2018 •

edited

deepak1556 Feb 2, 2018 •

edited

felixrieseberg commented Feb 12, 2018 •

edited

MarshallOfSound commented Mar 14, 2018 •

edited

dchest commented Oct 2, 2018 •

edited