Skip to content
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

[Screenshot Generator] Add explicit Toast duration, additional code comments #613

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Sep 21, 2024

Description

Light refactor of RemoveSDCardToastManagerThread to give the screenshot generator more explicit control over that Toast.

Screenshot generator now sets RemoveSDCardToastManagerThread.duration=0 to guarantee that the "infinite" Toast would never block during the screenshot generation. It was succeeding (not blocking) before because MicroSD.is_inserted checks Settings.HOSTNAME == Settings.SEEDSIGNER_OS, but this took too much digging to understand that Toast's behavior in the screenshot generator.

How the Toast screenshots are rendered at all is quite confusing so additional step-by-step explanatory comments were added.


This pull request is categorized as a:

  • Code refactor

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?

  • N/A

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

kdmukai added a commit to kdmukai/seedsigner that referenced this pull request Sep 21, 2024
I instead submitted them as a PR to the main repo's `dev` branch: SeedSigner#613
@newtonick newtonick added this to the 0.9.0 milestone Sep 22, 2024
@jdlcdl
Copy link

jdlcdl commented Sep 23, 2024

Viewed code changes, which I like, but have not yet tested.

@alvroble
Copy link
Contributor

Looks neat @kdmukai! I noticed this behavior in test case while developing PR #600. From what I understand, setting it to 0 in the test cases, or potentially a different value in future developments, seems like a solid approach, so ACK for me. I will update that PR to prevent conflicts if this one gets merged, as I also have changes in this file: https:/SeedSigner/seedsigner/pull/600/files#diff-887aac80585a20b298dabebe9d8fc3b36151a83b79f30c963f79f1585dae96e2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants