-
Notifications
You must be signed in to change notification settings - Fork 60
Screensaver now appears in screens with infobox #772
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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 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!
I think I know exactly what happened. If you build with a typo like: it just outputs, but nothing is built: If you then run (without a typo 😜 ): 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 #760. |
|
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 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. |
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| return self.ctx.display.draw_hcentered_text( | ||
| info, info_box=True, highlight_prefix=":" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not define it as a private method in the class instead a anon function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined it inside the function because it’s an implementation detail, not a reusable behavior of the class. Its scope is explicit: it cannot be called from anywhere else.
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| self.ctx.display.draw_hcentered_text( | ||
| info_text, | ||
| offset_y=FONT_HEIGHT, | ||
| info_box=True, | ||
| ) | ||
| self.ctx.display.draw_hcentered_text( | ||
| self.ctx.wallet.key.fingerprint_hex_str(pretty=True), | ||
| offset_y=FONT_HEIGHT, | ||
| color=theme.highlight_color, | ||
| bg_color=theme.info_bg_color, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| n_lines = self.ctx.display.draw_hcentered_text( | ||
| rolls_str, info_box=True, max_lines=max_lines | ||
| ) | ||
| self.ctx.display.draw_hcentered_text( | ||
| t("Rolls:"), | ||
| color=theme.highlight_color, | ||
| bg_color=theme.info_bg_color, | ||
| ) | ||
| return n_lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| failures = len([x for x in self.results if not x[1]]) | ||
| total = len(self.results) | ||
| return self.ctx.display.draw_hcentered_text( | ||
| "\n".join( | ||
| [ | ||
| x | ||
| for x in [ | ||
| t("Test Suite Results"), | ||
| t("success rate:") | ||
| + " {}%".format(int((total - failures) / (total) * 100)), | ||
| ( | ||
| t("failed:") + " {}/{}".format(failures, total) | ||
| if failures | ||
| else None | ||
| ), | ||
| ] | ||
| if x is not None | ||
| ] | ||
| if x is not None | ||
| ] | ||
| ), | ||
| info_box=True, | ||
| ) | ||
| ), | ||
| info_box=True, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| num_lines = self.ctx.display.draw_hcentered_text( | ||
| wallet_info, info_box=True | ||
| ) | ||
|
|
||
| # draw fingerprint with highlight color | ||
| self.ctx.display.draw_hcentered_text( | ||
| key.fingerprint_hex_str(True), | ||
| color=theme.highlight_color, | ||
| bg_color=theme.info_bg_color, | ||
| ) | ||
|
|
||
| # draw network with highlight color | ||
| self.ctx.display.draw_hcentered_text( | ||
| network_name, | ||
| DEFAULT_PADDING + FONT_HEIGHT, | ||
| color=Utils.get_network_color(network_name), | ||
| bg_color=theme.info_bg_color, | ||
| ) | ||
|
|
||
| return num_lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| self.ctx.display.draw_hcentered_text( | ||
| t("Res. - Format"), FONT_HEIGHT, info_box=True | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Here
| def _print_infobox(): | ||
| self.ctx.display.clear() | ||
| n_lines = self.ctx.display.draw_hcentered_text( | ||
| wallet_info, info_box=True | ||
| ) | ||
|
|
||
| # draw network with highlight color | ||
| self.ctx.display.draw_hcentered_text( | ||
| network_name, | ||
| color=Utils.get_network_color(network_name), | ||
| bg_color=theme.info_bg_color, | ||
| ) | ||
|
|
||
| return n_lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
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:
Did you build the code and tested on device?
What is the purpose of this pull request?