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

Reminder to remove SD card via Toast notification #424

Merged
merged 22 commits into from
Aug 15, 2023

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Aug 5, 2023

Alternative to #410 but builds upon behind-the-scenes updates from that branch. Would close #344 upon merge.

Display a toast notification popup on the Main Menu if the SD card is still inserted.

As has been thoroughly, arduously(!) discussed, my preferred presentation for this notification is to simply let the user know that they can remove the SD card, but omit any mention of security or warnings.

The precursor PR, #410 has evolved into an optional Must Remove hard stop which can work in conjunction with this notification:

  • Default: no hard stop; user sees inobtrusive toast notification (this PR)
  • Option enabled: User faces hard stop until card is removed; the unobtrusive toast notification probably doesn't need to be displayed (either by SD card state or explicit additional logic.

  • waits 3s before displaying (if SD card is inserted).
  • persists "forever" (1mil secs) while user remains on the Main Menu.
  • dismissed with any user input (otherwise navigating to the bottom row would be slightly crappy).
  • does not reappear after being dismissed while you're on that screen.
  • does reappear on each subsequent visit to Main Menu (if SD card is inserted).
  • Toast UI intentionally violates the normal 8px edge padding (see screenshot; toast edges are wider than the Main Menu button options).

MainMenuView_RemoveSDCardToast


  • Refactors the entire Toast system to move all timing and control logic out of the rendering components and into BaseToastOverlayManagerThread implementations.
  • Adds Controller.activate_toast() to act as a central coordinator to manage the Renderer.lock between competing toasts, the screensaver, and the currently rendered/rendering Screen.
  • Adds support for the screenshot generator to create screenshots that include toast overlays.
  • Adds screenshots

@jdlcdl
Copy link

jdlcdl commented Aug 6, 2023

I've tested this on a dev pi2 in raspi-os (thank you for fast screensaver while testing, also showing this on raspi-os which wouldnt be a problem for me if it remained, since it's true). Then I moved onto testing in seedsigner-os on pi0.

All works as described above except:

does reappear on each subsequent visit to Main Menu (if SD card is inserted).

If I never remove the card, I am not reminded. If I remove it and then re-insert the card and forget to remove it, I'm not reminded. I see that MicroSD.warn_to_remove boolean still exists but I don't believe it's being consulted. It was originally added so that the same code that gives a warning to the user can note that it did and avoid nagging. MicroSD thread will set it again if they reinsert the card.... if that helps.

As well, I'm able to confuse it in the following way:

  • let the toast appear informing "remove microsd card"
  • w/o any button actions, remove the microsd to see the "microsd removed" toast which renders on top.
  • when the 2nd microsd toast disappears on its own in 2s, the first toast warning doesn't recapture control (is it the render lock?) and the "remove microsd card" remains on screen...even after button presses, until the entire screen is re-rendered. It's almost as if the warning thread does have control of the buttons (because a single "press" on the joystick doesn't get us to the next menu), but doesn't have control of the renderer to repaint the screen.

This is very nice, unobstrusive, makes a connection for the user between this notification and what they should be looking for (the sd toast overlays).

My biased preference would be more reminders if leaving the microsd inserted since it goes away so fast when being actively used (in fact, if you're fast enough, you'll never see it even once), but it might get really old quickly. Maybe MicroSD.warn_to_remove could be repurposed as "MicroSD.notified" and default to 0 whenever sdcard is reinserted, then incremented for every notification shown, until we hit some max "x reminders per sdcard-inserted is sufficient" threshold???

I enjoyed reading your code changes commit-by-commit as well as the overall diffs.

p.s. After leaving this review, and editing multiple times, I saw this from easyuxd and I realy liked it.
https://t.me/seedsigner_new_devs/3751

@jdlcdl
Copy link

jdlcdl commented Aug 6, 2023

As of 7e964c7:

  • removing the card appears to dismiss the the warning and show the sdcard removal status.
  • also just noticing that if I quickly move to a submenu, I'll still get the notification to remove the card. Very nice!

@newtonick newtonick added the enhancement New feature or request label Aug 7, 2023
@jdlcdl
Copy link

jdlcdl commented Aug 8, 2023

@kdmukai,

With the recent merge of #423, I was worried there may be plenty of conflicts in this pr since easyuxd was moving icons and colors around, and because everything has been a moving target recently. I've worked to try to resolve some of these so that when you get to doing so, maybe I'll be able to have saved you some time.

Conflicts to resolve are:

  • tests/base.py needs to mock-out seedsigner.gui.toast (and then they mostly pass except for a single test for pr#410... which can almost be ignored since this pr replaces it.)
  • src/seedsigner/gui/screens/screen.py to import ToastOverLay from ..toast instead of ..components
  • src/seedsigner/gui/toast.py needing SDCARD from SeedSignerIconConstants instead of FontAwesomeIconContants as well as having a new choice like MICROSD

I think the rest of the merge went without probs, but I know you'll get it right.

@kdmukai kdmukai marked this pull request as ready for review August 9, 2023 16:26
@kdmukai kdmukai changed the title [Work-in-Progress] Reminder to remove SD card via Toast notification Reminder to remove SD card via Toast notification Aug 9, 2023
@jdlcdl
Copy link

jdlcdl commented Aug 11, 2023

As of cf2d7b9,

ACK tested

w/ only one request: to remove attribute 'warn_to_remove' from MicroSD as it's set there but is no longer used/read anywhere else; it's a remnant of pr_410.

Why my recent preference for this over pr_410 (or a mix of both for those who want sd-card-removal required)?

  • because I also like the threading cleanup/refactoring that comes along with this notification.
  • because a notification that doesn't "hinder" the user but appears consistently each session will not be a nuisance to anyone; the repetition might be appreciated by many, eventually.
  • because I'd rather teach humans good habits via frictionless notification so that they are more likely to avoid harm when (and not if) an eventual vulnerability sneaks past our eyes... than to obstruct them so much that we need to give them a choice to turn it off forever, supporting their blissful-ignorance that Gigabytes of persistent storage remains available when it could have just as easily been taken out of the picture for such an important endeavor as loading secrets. Just as I will continue to support clipping the inductor off the pi0W and pi02W boards, and using the pwr-only usb port, and all the other good ideas that are easy to do... even if those things are already disabled in seedsigner.

@kdmukai
Copy link
Contributor Author

kdmukai commented Aug 15, 2023

Per @newtonick's comments in #437, the notification toast at startup will not be displayed at all if there is user input before the toast's activation delay elapses.

@newtonick
Copy link
Collaborator

ACK Tested

This PR for MicroSD removal notifications feels right to me now. It provides discovery of the feature (removing the microsd), without creating fear or any barriers leading to confusion.

@jdlcdl
Copy link

jdlcdl commented Aug 15, 2023

As of fb4a8d8, I reiterate my ACK.

I note a similar issue mentioned by Nick regarding "awareness of microsd state" as the app starts... to be resolved soon-ish, or maybe with a full-stop version of pr-410?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Feature Request: reminder to pull SD after boot
3 participants