SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
721 stars 168 forks source link

Inserting a microsd w/ settings.json implies nothing #447

Closed jdlcdl closed 1 year ago

jdlcdl commented 1 year ago

This pr CHANGES EXISTING BEHAVIOR SINCE RELEASE 0.6.0

Where inserting a microsd w/ a settings.json file assumes that the user wants: 1) persistent settings to be enabled, and 2) for settings.json to be written by the current settings values immediately.

This existing behavior could be fine if the user always waits for seedsigner to start-up first before removing their microsd, but it is too much implied action w/o informing the user about what is happening in the case that they have pulled the microsd by habit before seedsigner starts, and are re-inserting the microsd so that they can restart the app to regain the persistent settings on the microsd. The existing 'magic' more likely replaces the user's custom persistent settings with defaults that were set when seedsigner started w/o microsd.

This pr does NOT add any notifications, it does NOT read the existing settings.json file, it does NOT assume to enable persistent settings, and it does not write current settings... though all of these actions MIGHT be a better solution than this pr, as long as the user is notified.

What this pr does do, is to remove assumptions that anything 'magic' should happen just because the user re-inserted the microsd with a settings.json file... and it enables them to restart the app regaining their persistent settings. Knowing that they previously removed the sdcard, they'll understand, or quickly learn, that persistent_settings were disabled at that time. They can continue using their seedsigner with current settings (defaults or whatever they've changed since boot) and must explicitely re-enable persistent-settings to replace whatever settings.json is already on the microsd.

Also, this pr does not remove any lines of code or any existing comments, because they are all still valid and informative. It simply comments-out logic until better notifications and/or user choices can be added.

kdmukai commented 1 year ago

We have essentially three options upon reinserting the SD card:

  1. write the current settings to SD (current approach)
  2. load the SD's settings into SeedSigner
  3. do nothing (this PR)

I dislike the assumption in 1 as that is auto-magically changing the one place where settings are truly persistent across sessions. So

I prefer 2 as it is intuitive to me that the thing I just introduced should take precedence, BUT I still don't like the auto-magic assumption (unless, as you say, there was a notification or a prompt).

I initially disliked 3 (this PR) but after laying out the above options...

concept ACK

jdlcdl commented 1 year ago

I too prefer the user being notified that settings.json exists and having a choice to load from or save to.

This is a less than ideal pr, with fewest lines of code-changes just before release, in case it's more consistent like this until we can get to a much more flexible user choice.

newtonick commented 1 year ago

NACK

This pr CHANGES EXISTING BEHAVIOR SINCE RELEASE 0.6.0

Where inserting a microsd w/ a settings.json file assumes that the user wants:

  1. persistent settings to be enabled, and
  2. for settings.json to be written by the current settings values immediately.

The behavior of saving persistent settings to disk when microSD card is re-inserted back into pi, but only if persistent settings was previously enabled (aka settings.json file exists) was intentional. See #234

This existing behavior could be fine if the user always waits for seedsigner to start-up first before removing their microsd, but it is too much implied action w/o informing the user about what is happening in the case that they have pulled the microsd by habit before seedsigner starts, and are re-inserting the microsd so that they can restart the app to regain the persistent settings on the microsd. The existing 'magic' more likely replaces the user's custom persistent settings with defaults that were set when SeedSigner started w/o MicroSD.

I personally think this senerio is a bit contrived. Only the most technical of users even know they can remove the microSD when the green light goes off (if they have a case, or no case, that allows them to see the green light). In fact I would never suggest we educate users to pull the MicroSD before the splash. It'll only cause confusion.

Back in August of 2022 when this was being discussed, the thought process was that settings in memory trumped settings on disk. If the user previously saved settings to disk (the settings.json file exists), the only reason they have to insert the MicroSD back into the SeedSigner is to persist setting changes. When/if we add additional MicroSD functionality, that changes the conversation and can be addressed then. I'm all for prompting with options about saving or loading preferences.

What this pr does do, is to remove assumptions that anything 'magic' should happen just because the user re-inserted the microsd with a settings.json file... and it enables them to restart the app regaining their persistent settings. Knowing that they previously removed the sdcard, they'll understand, or quickly learn, that persistent_settings were disabled at that time. They can continue using their SeedSigner with current settings (defaults or whatever they've changed since boot) and must explicitly re-enable persistent-settings to replace whatever settings.json is already on the MicroSD.

I disagree that it's anything magical. I don't see it as 'magic', but simply performing the exact behavior most users would expect. I believe the main reason someone puts a MicroSD back into a running SeedSigner without powering it down is to save settings. The real issue is there is no indication that the settings successfully were saved. You could argue someone would want to load settings, but we have given no UI or educational material to indicate that is possible.

Ultimately I think the user needs to be prompted. But until that features is coded, I think changing the default behavior is more of a preference than anything else. I see no good reason to change the behavior set in 0.6.0.