Screensaver now appears in screens with infobox
What is this PR for?
Screensaver now appear in all menu screens (even with info boxes) as requests from issue #760
Tested all screens that are changed by this PR, waiting for screensaver and seeing the result after, more than one time
Changes made to:
- [x] Code
- [ ] Tests
- [ ] Docs
- [x] CHANGELOG
Did you build the code and tested on device?
- [x] Yes, build and tested on Embed Fire
What is the purpose of this pull request?
- [ ] Bug fix
- [ ] New feature
- [ ] Docs update
- [x] Other
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 97.38%. Comparing base (23dd732) to head (67a62f0).
Additional details and impacted files
@@ Coverage Diff @@
## develop #772 +/- ##
========================================
Coverage 97.37% 97.38%
========================================
Files 83 83
Lines 10521 10537 +16
========================================
+ Hits 10245 10261 +16
Misses 276 276
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Tested ACK, though haven't reviewed / not really capable of reviewing the code changes.
Screensaver w/this PR successfully running on tzt, wonder_k, and m5stickv.
One oddity: My initial rebuild and flash of this PR on the wonder_k just made it freeze. Unplug, re-plug: blank gray screen. Reflashed. Same. Rebuilt the main branch and flashed: totally fine. Rebuilt the PR branch and flashed: ...totally fine!
So maybe something went wrong the first time I compiled this PR for the wonder_k? I didn't notice any errors (but wasn't paying close attention) and as far as I could tell those first two flashing attempts went as expected.
Hey @kdmukai, thanks for testing this PR so quickly! Yeah, asking you for a full code review would be a bit much. You already helped a ton by testing on three devices (two touch models and one with battery + button only). Really appreciate it! :heart:
One oddity: My initial rebuild and flash of this PR on the wonder_k just made it freeze. … I didn’t notice any errors (but wasn’t paying close attention) …
I think I know exactly what happened. If you build with a typo like:
./krux build maixpy_wonderk
it just outputs, but nothing is built:
Wrong or unsupported device name, use maixpy_devicename
If you then run (without a typo :stuck_out_tongue_winking_eye: ):
./krux flash maixpy_wonder_k
It will flash whatever was previously built. So if your last successful build was for TZT, for example, that firmware ends up being flashed onto the Wonder K, and of course it won’t behave correctly 😅
On the next build attempt (using the correct device name), the files get overwritten properly. Flashing after that installs the correct firmware, which I think is why everything worked afterward.
I think this is not the way to go for a more consistent Screensaver https://github.com/selfcustody/krux/issues/760. IMO it should be called by system timer, just like "auto shutdown", and its code should be self contained in a single file, not spread across multiple files. We should be able to disable it in a few situations, like QR scanning and animated QR codes, and re-enable when finished.
I asked in #760 which situations should trigger the screensaver, but the issue only mentions "being active only in some menu screens". This PR covers all menu screens, so we might need to refine the criteria later. Ideally, #760 should clarify exactly where the screensaver should or shouldn’t appear.
This PR improves the screensaver behavior, and I think @kdmukai as "a user" can provide more insight based on how it felt to use this PR compared to the current state, and whether it should appear on additional screens.
My hands-on testing was brief. I set the timeout to 1min and so far only monitored the initial menu screen. So not much input for me to contribute at the moment.
Even if this PR isn't the final solution, it improves readability and brings the code more in line with pylint by shortening functions and moving the infobox drawing into its own helper. It adds very little code and doesn't change any logic, so I think we can merge it now and continue iterating later.
From what I've seen, no one reported screensaver issues before infoboxes were introduced. After we chose to suppress the screensaver whenever an infobox was visible, users started interpreting that behavior as a malfunction. This PR fixes that.