fix(app, components): Fix display suspending when idle time set to "never"

[mjhuff]

Closes RQA-3813

Overview

Woops, turns out our SLEEP_NEVER_MS constant is an actual number that equates to one week. To fix the root problem, simply do not invoke the timeout function if the selected time is exactly is exactly SLEEP_NEVER_MS.

Unfortunately, the fix itself has to be more involved given the fact screen settings are persisted and our idle utility is in the general components dir. We have a few options:

1. If we decide it's important to keep useIdle in the components dir, we have to compromise the API for what is meant to be a generalized util by special-casing the number that is exactly one week in MS or...
2. Update SLEEP_NEVER_MS to be something that is not a "real" value. To do this, we'd need conditional logic in the root of the ODD that checks if the config value is exactly the current SLEEP_NEVER_MS value of one week and updates the config value at runtime to a different value (ex, set SLEEP_NEVER_MS to 0).
3. We move useIdle into app, and update the util to care only about screen brightness.

Option 1 isn't great, and option 2 isn't either, since no matter what we do, we'd have to maintain some sort of hacky "monkey patch the config" code indefinitely.

Because useIdle is only used in the app, it seems more sensible to move the util and rename it to something like useScreenIdle. Historically, some app specific hooks that didn't have a great place to live within the app's file structure would often be placed in components, and I believe this is one of those cases. We now have local-resources and dom-utils.

Test Plan and Hands on Testing

• Covered by updated unit tests.

Changelog

• Fixed a bug in which not interacting with the ODD for more than a week would cause it to dim.

Risk assessment

low - Functionally, the only change is one new condition that's used in exactly one spot.

[koji]

the changes make sense to me.

a little background
we decided to use 1 week for SLEEP_NEVER_MS since Infinity didn't work somehow then we thought 1 week would be enough for SLEEP_NEVER_MS.
Maybe we should try to use Infinity since the dev env is very different from when I added useIdle.

[mjhuff]

a little background we decided to use 1 week for SLEEP_NEVER_MS since Infinity didn't work somehow then we thought 1 week would be enough for SLEEP_NEVER_MS.
Maybe we should try to use Infinity since the dev env is very different from when I added useIdle.

Thank you for the context and the review. I did do some digging into using Infinity. It apparently doesn't play nicely with setTimeout, which makes sense, and the recommended pattern is gating the setTimeout with an Infinity conditional check before passing the timeout value to setTimeout.

I agree with you that using a real number to mean "no actual number" is confusing. Because display suspension is tied to a persisted config value, I believe we are in a bind: we either have to accept that NEVER_SLEEP_MS is a real number and keep it as is or do something like:

1. if user config is exactly 604800000, set the config value to 0/Infinity/whatever...

2. if user config is 0/infinity/whatever, do not run the setTimeout

The problem is because this config value is already out in the wild, we'd end up needing to support the code outlined in 1 indefinitely (or at least for a very long time). Ultimately, we have to include code 2 regardless. Personally, I'd rather avoid including the runtime config patching, but I'd be happy to discuss opening up a separate PR if we decide that is the approach we wish to take.

[SyntaxColoring]

This seems good to me, thanks!

Personally, if we have to choose between:

1. Continuing to have a runtime and on-filesystem constant of 604800000 that we special-case as "infinity", which is weird
2. Fixing the runtime and on-filesystem constant and migrating configs, which means doing more work now
3. Fixing the runtime and on-filesystem constant but not migrating configs, which means RQA-3813 will keep happening in the field until the next time the user toggles the setting

I would probably favor 3. (So long as it wouldn't, like, cause a whitescreen if a user downgrades and the old ODD software chokes on the new "infinity" value where it was expecting to see a real number.)

But I'm just bikeshedding. This totally works.

[mjhuff]

I would probably favor 3. (So long as it wouldn't, like, cause a whitescreen if a user downgrades and the old ODD software chokes on the new "infinity" value where it was expecting to see a real number.)

Yeah, I think the kicker here is unexpected behavior when a user downgrades. Per @koji 's past experience and from my reading about the topic, it does seem like we'd end up with a flavor of "not a whitescreen but quite unexpected" behavior. I wish this were not the case.

We've had to do some backward config support in the past after the release of 7.0 and it was not great. As confusing the variable naming is, I would like to avoid any sort of on-file system changes.