[IMP] Disable screensaver during SeedQR transcription #740

FazleRabbbiferdaus172 · 2025-04-17T17:47:24Z

Description

This PR addresses part of the discussed SoB idea in the seedsigner dev group providing a solution for the issue of the screensaver interrupting the seed QR transcribing process. Views now have a property that indicates whether the screensaver is allowed while the view is running. As a result, the screensaver will not start when users are transcribing the QR. This improves usability, especially when transcribing 21×21 SeedQRs, where the screensaver previously interrupted the flow. A future update may introduce a long timeout wipe or reboot as a fallback for inactivity.

This pull request is categorized as a:

Checklist

I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

No, I’m a fool
Yes
N/A

I have tested this PR on the following platforms/os:

Raspberry Pi OS Manual Build
SeedSigner OS on a Pi0/Pi0W board
Other

Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.

kdmukai · 2025-04-19T14:30:22Z

src/seedsigner/controller.py

+
+    @property
+    def active_view(self) -> Optional[View]:
+        return self.back_stack[-1].view if self.back_stack else None


There's a slight conceptual mismatch for me for the MainMenuView, perhaps elsewhere, too:

MainMenuView is currently running. Literally the active View.

BackStack will be empty.

So this active_view() will return None, implying that there is no active View.

You're right — there's a mismatch. I initially considered active_view as a separate property, since in most frontend/UI frameworks, the view manager typically keeps track of the current view, while a back stack handles history (like breadcrumbs). That’s why I assumed the controller, acting as a view manager, should explicitly manage active_view.

However, for simplicity, I aligned it with back_stack. But because the controller clears the stack before displaying MainMenuView, active_view() ends up returning None, which feels inconsistent.

To fix this, I’ll return MainMenuView when the stack is empty — that keeps the logic clean without introducing extra state tracking. Let me know if that sounds good; I’m happy to make the change.

Side note: I haven’t worked much with resource-constrained systems like Pi Zero, so open to suggestions if explicitly tracking active_view has performance implications.

kdmukai · 2025-04-19T14:36:42Z

src/seedsigner/views/seed_views.py

@@ -1567,7 +1567,8 @@ def __init__(self, seed_num: int, seedqr_format: str, initial_block_x: int = 0,
        self.seedqr_format = seedqr_format
        self.seed = self.controller.get_seed(seed_num)
        self.initial_block_x = initial_block_x
-        self.initial_block_y = initial_block_y 
+        self.initial_block_y = initial_block_y
+        self.allow_screensaver = False


I know the rationale for this setting/override is pretty self-evident already, but I always try to add a comment to remind myself of any unusual setting or decision.

I would explicitly explain why it's important to not let the screensaver run here.

I will add a comment .

kdmukai · 2025-04-19T14:39:34Z

I think exposing the Controller.active_view is providing more access than we might actually want.

The real question is just "Hey, Controller, can we run the screensaver on the current View?" I think I'd prefer a Controller function that ONLY answers that very specific question. It would also eliminate almost all of the changes in buttons.py.

FazleRabbbiferdaus172 · 2025-04-20T06:40:16Z

I think exposing the Controller.active_view is providing more access than we might actually want.

The real question is just "Hey, Controller, can we run the screensaver on the current View?" I think I'd prefer a Controller function that ONLY answers that very specific question. It would also eliminate almost all of the changes in buttons.py.

Sure, I will implement a method in the controller.

FazleRabbbiferdaus172 · 2025-04-20T08:34:15Z

Thanks for the review!
I've addressed the feedbacks and pushed new commits

kdmukai · 2025-04-20T13:09:02Z

src/seedsigner/controller.py

@@ -466,3 +466,21 @@ def handle_exception(self, e) -> Destination:
            exception_msg,
        ]
        return Destination(UnhandledExceptionView, view_args={"error": error}, clear_history=True)
+
+    @property
+    def active_view(self) -> View:


As mentioned in a previous comment, I would not expose this.

I would just do this retrieval within the can_run_screensaver() method.

ok, I understand. I explain my reasoning behind why a active_view property might be needed. But python doesn't have access specifier. So, I removed the property and merged the logic with can_run_screensaver

kdmukai · 2025-04-20T14:42:12Z

src/seedsigner/controller.py

+        from seedsigner.views import MainMenuView
+        return self.back_stack[-1].view if self.back_stack else MainMenuView()
+
+    def can_run_screensaver(self) -> bool:


I would make this a property.

Minor nit: Saying that you "can do" something is more often about ability ("can you lift that weight?") whereas a phrase like "is allowed" has a different connotation (e.g. "can you light a match at a gas station?" Well, yes, it's physically possible but is it allowed?).

So if I were to read controller.can_run_screensaver == False it feels like: Car.can_start_engine == False, that means something's broken on my car.

I'm not sure what a better name would be...

screensaver_start_is_allowed?

(I admit that I do fuss quite a bit about how we name things)

My name preference would be something like is_screensaver_startable or is_screensaver_start_allowed. Following the naming convention of is_screensaver_running.

I am pretty bad at naming stuff. Also I have to admit that I am not satisfied with the name I have chosen either.

As newtonick suggested, is_screensaver_start_allowed seems a good choice.

kdmukai · 2025-04-22T12:43:59Z

From the PR description:

This PR addresses part of the discussed behavior.

What discussion? PRs should ideally be completely self-contained (fully explain the problem and/or rationale for the change) or at least point to where some deeper discussion occurred (though even then the PR description shouldn't assume that the reader is already familiar with the prior discussion).

kdmukai · 2025-04-22T12:49:43Z

It might be a slightly difficult test to write (anything is possible with enough Mocking), but you should be able to write a test that at least verifies the result of calling Controller.is_screensaver_start_allowed when running the SeedQR transcription screen.

Leveraging the flow-based test structure should get you most of the way there. There's at least one test somewhere in there that runs a flow-based test sequence and then verifies the status of the Controller or something like that.

If that ends up being too difficult, at least a # TODO comment should be placed somewhere and log the task as a new Github Issue.

FazleRabbbiferdaus172 · 2025-04-24T06:40:31Z

I will update the pr description and will try to add the test cases.

FazleRabbbiferdaus172 · 2025-04-27T08:10:58Z

I have pushed a commit that tests the sreensaver start status when SeedTranscribeSeedQRZoomedInView is the current view in the flow. But I have questions regarding back_stack destination is handled, initial_destination.

Questions

Why is the initial_destination not included in the stack? pop_prev_from_back_stack pops twice. So, the back_stack should always hold the current running view as it's top element. But initial_destination is never in the back_stack. Logical difference arises when the flow starts from any view other than MainMenuView. is_screensaver_start_allowed will fail in that case as the implementation depends on back_stack and defaults to MainManuView when stack is empty.
Is it logical to always consider MainManuView as the destination when the next_destination.View_cls is None?
If is start controller with a initial_destination and then move to another view and then hit the back_button it should return to initial_destination. Current implementation instead return to MainMenuview.

Disables the screensaver entirely while the SeedQR transcription routine is active. Any view can be configured to allow or disallow screensaver activation.

…ogics

…to is_screensaver_start_allowed

…s when the current view is SeedTranscribeSeedQRZoomedInView

kdmukai · 2025-07-29T20:18:25Z

Why is the initial_destination not included in the stack? pop_prev_from_back_stack pops twice. So, the back_stack should always hold the current running view as it's top element. But initial_destination is never in the back_stack. Logical difference arises when the flow starts from any view other than MainMenuView. is_screensaver_start_allowed will fail in that case as the implementation depends on back_stack and defaults to MainManuView when stack is empty.

initial_destination is only used by the test suite and it's a non-issue if its value for a particular flow test isn't preserved in the BackStack.

That being said, I probably could have streamlined the logic to add the current View to the BackStack just AFTER the View is run. In which case the current View wouldn't be at the top of the stack.

Adding it to the BackStack BEFORE the View is run leads to the need to do the double pop when the BACK arrow is returned.

I can't recall if there was a specific reason for doing it this way or maybe I just got myself into a sub-optimal solution while being confused by the loop + stack logic.

Is it logical to always consider MainManuView as the destination when the next_destination.View_cls is None?
If is start controller with a initial_destination and then move to another view and then hit the back_button it should return to initial_destination. Current implementation instead return to MainMenuview.

Again, it's never a user-facing scenario. Strictly for the test suite.

src/seedsigner/controller.py

kdmukai · 2025-07-29T20:28:02Z

tests/test_flows_seed.py

@@ -476,6 +476,26 @@ def load_right_seed_into_decoder(view: View):
            FlowStep(seed_views.SeedOptionsView),
        ])

+    # TODO: ideally this test should be part of controller related test? move it there


I think it's fine to leave this flow test here to explicitly test that the screensaver cannot start during the interactive transcription process.

src/seedsigner/views/view.py

src/seedsigner/controller.py

FazleRabbbiferdaus172 · 2025-07-30T10:17:51Z

Why is the initial_destination not included in the stack? pop_prev_from_back_stack pops twice. So, the back_stack should always hold the current running view as it's top element. But initial_destination is never in the back_stack. Logical difference arises when the flow starts from any view other than MainMenuView. is_screensaver_start_allowed will fail in that case as the implementation depends on back_stack and defaults to MainManuView when stack is empty.

initial_destination is only used by the test suite and it's a non-issue if its value for a particular flow test isn't preserved in the BackStack.

That being said, I probably could have streamlined the logic to add the current View to the BackStack just AFTER the View is run. In which case the current View wouldn't be at the top of the stack.

Adding it to the BackStack BEFORE the View is run leads to the need to do the double pop when the BACK arrow is returned.

I can't recall if there was a specific reason for doing it this way or maybe I just got myself into a sub-optimal solution while being confused by the loop + stack logic.

Is it logical to always consider MainManuView as the destination when the next_destination.View_cls is None?
If is start controller with a initial_destination and then move to another view and then hit the back_button it should return to initial_destination. Current implementation instead return to MainMenuview.

Again, it's never a user-facing scenario. Strictly for the test suite.

purpose of the questions were to determine if i should leave "is_screensaver_start_allowed" method and the "test" as it is or consider the initial_destination in the method. As per my understanding current implementation is ok as initial_destination is only for test suite.

kdmukai reviewed Apr 19, 2025

View reviewed changes

FazleRabbbiferdaus172 force-pushed the improvement/non_emulator/Disable_screensaver branch from 80ccbfa to 393bbd1 Compare April 20, 2025 08:31

kdmukai reviewed Apr 20, 2025

View reviewed changes

bitcoinprecept moved this to SoB 1 in @SeedSigner Development Board Jul 23, 2025

bitcoinprecept added this to @SeedSigner Development Board Jul 23, 2025

bitcoinprecept moved this from SoB In Progress to SoB Needs Code Review in @SeedSigner Development Board Jul 23, 2025

FazleRabbbiferdaus172 added 7 commits July 26, 2025 10:21

[IMP] Disable screensaver during SeedQR transcription

936865b

Disables the screensaver entirely while the SeedQR transcription routine is active. Any view can be configured to allow or disallow screensaver activation.

[FIX] active_view returns MainMenuView when back stack is empty

1c0737b

[IMP] can_run_screensaver implemented to handle screensaver runable l…

b0276b7

…ogics

[IMP] code cleanup

499a17a

[REF] active_view property removed and can_start_screensaver renamed …

a0fdfb1

…to is_screensaver_start_allowed

[Temporary] rough test included that test the screensaver start statu…

0f183fd

…s when the current view is SeedTranscribeSeedQRZoomedInView

[Temporary] TODO added

c85de09

FazleRabbbiferdaus172 force-pushed the improvement/non_emulator/Disable_screensaver branch from e62be31 to c85de09 Compare July 26, 2025 15:22