SettingsQR: Final changes before official release #394

kdmukai · 2023-06-21T12:48:12Z

SettingsQR has been quietly supported for a while, but this PR will fully promote SettingsQR to be a user-facing feature.

Two repos have to work hand-in-hand here:

SeedSigner w/updated SettingsQR support (this PR)
SettingsQR Generator

Changes in this PR:

The expected SettingsQR format has been slightly tweaked.

Parsing them has been greatly simplified.

Missing settings (either in an onboard settings.json file or in a scanned SettingsQR) are now set to their default value. The only exception are hidden settings (e.g. qr_brightness); their values are preserved if they are already in the current settings.

How to test

You can use the current public SettingsQR Generator as-is: https://seedsigner.github.io/seedsigner-settings-generator

It is in sync with the changes in this PR.

If you want to build the SettingsQR Generator locally, follow the instructions in the repo. Just note that there is a dependence on the SeedSigner repo and it is currently pointing to this branch in my fork.

Before the SettingsQR Generator is fully released, we must:

Transfer ownership to SeedSigner
Update the developer instructions and the git submodule to point to the main SeedSigner repo and its dev branch.

* more docstrings * misc cleanup

jdlcdl · 2023-06-21T18:47:15Z

As of commit f7c8213, I've taken a look at this; my thoughts: (tl;dr Nice Work!)

It works: when using your SettingsGenerator tool.
It works: If old-style settings display_name is used instead of the new abbreviated_name.
It works: in defaulting missing settings entries to what they would be by default.
It works: in nice display for settings name: underscores become spaces for attrib=name.
It works: in ignoring nonsensical settings attributes that don't exist.
It works: in failing when a settings-attribute is set to a value that is not valid.
After reviewing the code changes, I have no concerns to add at this time.

I did notice a few issues that were unexpected:

If a user fiddles with creating their own SettingsQR in some other tool, it's possible that they'll add invalid settings values which correctly fails... but they never reach an error screen. They're brought back to the main menu instead (as with other exceptions, sometimes with remnants of the last camera image on screen, till button presses can redraw screen). Might it be better to catch these and redirect to a DireWarningView "script_type: 'p2pkh' is invalid" ?
If we add "qr_brightness=32" (or qr_background_color=x) to an otherwise valid SettingsQR, I was unable to actually get this to work (but maybe it shouldn't, since it's hidden?). It was raising an exception related to missing selection_options" in decode_qr at 886.

Traceback (most recent call last):
  File "/home/pi/seedsigner-dev/src/seedsigner/models/decode_qr.py", line 886, in add
    if v not in [opt[0] for opt in settings_entry.selection_options]:
TypeError: 'NoneType' object is not iterable

I was trying to make a SettingsQR out of this content:

settings::v1
name=jdlcdl_Jean_Do
coords=nun,spa
network=T
scripts=nat,tr
priv_warn=D
qr_brightness=32

After a successful load of a SettingsQR, going "BACK" get's us back into the scan screen, but maybe it would be more natural to skip that and be back in MainMenu, as if user had hit "Home".

Very nice work! I'm a big fan of this pr!

kdmukai · 2023-07-06T11:49:11Z

Thank you, @jdlcdl, these are excellent notes! In the next few days I'll try to take a look at the unexpected/not ideal behaviors.

jdlcdl · 2023-07-27T13:28:33Z

Just food for thought, because it's crossed my mind recently.

SettingsQR makes things so simple to do regularly w/o using persistent settings that I wonder:

Should a load of SettingsQR EVER be allowed to set persistent_settings=True?
Maybe this one settings entry should be ignored, always setting it to False after a successful SettingsQR load?

I hope that this PR makes it into the next release. I'll argue to that end.

jdlcdl · 2023-07-30T13:59:10Z

src/seedsigner/models/settings.py

@@ -47,42 +47,22 @@ def save(self):
                os.fsync(settings_file.fileno())


-    def update(self, new_settings: dict, disable_missing_entries: bool = True):
+    def update(self, new_settings: dict, ):


Since this pr was submitted, pr-339 flow-tests framework has been merged.
In that pr, I was using the non-default "disabled_missing_entries=False" feature that is no longer present, so I'll be adjusting my test to setup in a one-by-one manner, otherwise this pr once merged would fail the test suite.

also, I think the trailing "," in the signature isn't intended, but I dont believe it's hurting anything.

Trailing comma fixed!

it's a sneaky trailing comma.

jdlcdl · 2023-07-30T14:21:51Z

Since this pr was submitted, pr-339 flow-tests have been merged, and I had written a test that exploited the non-default boolean of "disabled_missing_entries=True" in settings.update(). This pr will fail tests because of this, but the fix is below:

In tests/test_flows_seed.py's test_export_xpub_skip_non_option_flow() method:

-        self.settings.update({
-            SettingsConstants.SETTING__SIG_TYPES: SettingsConstants.MULTISIG,
-            SettingsConstants.SETTING__SCRIPT_TYPES: SettingsConstants.NESTED_SEGWIT,
-            SettingsConstants.SETTING__COORDINATORS: SettingsConstants.COORDINATOR__SPECTER_DESKTOP,
-        }, disable_missing_entries=False)
+        self.settings.set_value(SettingsConstants.SETTING__SIG_TYPES, [SettingsConstants.MULTISIG])
+        self.settings.set_value(SettingsConstants.SETTING__SCRIPT_TYPES, [SettingsConstants.NESTED_SEGWIT])
+        self.settings.set_value(SettingsConstants.SETTING__COORDINATORS, [SettingsConstants.COORDINATOR__SPECTER_DESKTOP])

jdlcdl · 2023-07-30T15:33:28Z

In my summer-solstice comments, I'd mentioned that invalid settings entry values in a SettingsQR cause an exception that doesn't get raised, so users just fall back to MainMenu without understanding what happened... probably to retry the qrcode scan again and again (only geeky users who create invalid SettingsQRs). Sometimes even, the MainMenu doesn't get rendered and they're left with partial image of the scan, until they move around the MainMenu to repaint it.

The last thing I want to do is break something major in a qr decoder, but I've made the following adjustment to models/decode.qr in SettingsQrDecoder.run() with hopes of seeing what the problem is.
I'm not sure if it wouldn't be better not to have everything in try/except so that the exception bubbles up on its own,
or if the View layer should do something special with DecodeQRStatus.INVALID but it would lose granular exception info.

         except Exception as e:
             logger.exception(e)
-            return DecodeQRStatus.INVALID
+            raise e

which was provoked when trying to scan a SettingsQR with content of:

settings::v1
name=jdlcdl_Jean_Do
coords=nun,spa
network=T
scripts=nat,nes,p2pkh
priv_warn=D

A similar change might also resolve my issue with trying to set qr_brightness in a SettingsQR, at least I'd know:

settings::v1
name=jdlcdl_Jean_Do
coords=nun,spa
network=T
scripts=nat,tr
priv_warn=D
qr_brightness=32

jdlcdl · 2023-07-30T15:44:44Z

This pr plays well with pr #416 (maybe you should check it out?).

jdlcdl · 2023-07-30T16:28:44Z

Another comment I had made back in late June was:

After a successful scan, if user uses the BACK button, they go into camera scan mode again, whereas I felt it would feel more natural (since it already succeeded and settings are updated) to land in the MainMenu. I've adjusted views/scan_views.py in ScanView.run() so that when self.decoder.is_settings, skip_current_view like so:

           elif self.decoder.is_settings:
                from seedsigner.models.settings import Settings
                settings = self.decoder.get_settings_data()
                Settings.get_instance().update(new_settings=settings)

                print(json.dumps(Settings.get_instance()._data, indent=4))
                
-               return Destination(SettingsUpdatedView, {"config_name": self.decoder.get_settings_config_name()})
+               return Destination(SettingsUpdatedView, {"config_name": self.decoder.get_settings_config_name()}, skip_current_view=True)

For a last nit-picky comment, but I'm not really sure, it could go either way:
Maybe the SettingsUpdatedView is more-at-home in settings_views.py than scan_views.py???

kdmukai · 2023-07-30T20:41:25Z

Merged the latest from dev into this branch
Did some final(?) cleanup
Added decoder tests

kdmukai · 2023-07-30T20:54:48Z

re: #394 (comment)

Agreed. Now letting exceptions in the SettingsQR decoder bubble up to the UI, where possible.

* Move post-SettingsQR View to settings_views.py * Improve ingestion cleaning to gracefully handle single-value `Settings.update()` entries for multi-select settings. * Improve Exception handling and onscreen error reporting during SettingsQR parsing. * move tests accordingly out of SettingsQR and into Settings.

kdmukai · 2023-07-31T16:00:41Z

Users will now see show-stopping Exceptions.

src/seedsigner/models/decode_qr.py

tests/test_settings.py

jdlcdl · 2023-07-31T17:55:58Z

as of b14de31 - 9cc5bcd:

tests that I'd done and were working at f7c8213 are still working,

plus what I'd asked for is working too:

exceptions while parsing "incorrect" SettingsQRs are no longer hidden,
no going back into scan screen because BACK is disabled.
plus even more tests.

ACK tested

newtonick · 2023-08-03T03:35:38Z

ACK Tested

I believe this should complete the last todo "Update the developer instructions and the git submodule to point to the main SeedSigner repo and its dev branch." in SeedSigner/seedsigner#394

kdmukai added 4 commits June 19, 2023 20:40

interim commit

8017f11

simplify/improve ingesting SettingsQR

99f1cb3

SettingsQR support for any kind of whitespace

5a95818

* more docstrings * misc cleanup

Debugging cleanup

f7c8213

jdlcdl reviewed Jul 30, 2023

View reviewed changes

jdlcdl mentioned this pull request Jul 30, 2023

[Enhancement] Speed up initial module imports #416

Merged

kdmukai added 2 commits July 30, 2023 14:16

Merge branch 'dev' into settings_qr

809f875

dev merge, cleanup, added tests

4eb9050

Exceptions bubble up to UI

4948724

kdmukai added 2 commits July 31, 2023 10:55

Remove BACK button from SettingsUpdatedScreen

d2838a9

Merge branch 'dev' into settings_qr

b14de31

jdlcdl reviewed Jul 31, 2023

View reviewed changes

src/seedsigner/models/decode_qr.py Outdated Show resolved Hide resolved

kdmukai added 2 commits July 31, 2023 12:44

Imports cleanup

6926f22

remove errant comma

4ee6557

jdlcdl reviewed Jul 31, 2023

View reviewed changes

tests/test_settings.py Show resolved Hide resolved

newtonick added this to the 0.7.0 milestone Jul 31, 2023

kdmukai added 2 commits July 31, 2023 12:58

docstring updates

56b6e23

Merge branch 'dev' into settings_qr

9cc5bcd

newtonick added the enhancement New feature or request label Aug 1, 2023

newtonick mentioned this pull request Aug 3, 2023

Update link and repo clone command to seedsigner from kdmukai newtonick/seedsigner-settings-generator#1

Closed

newtonick mentioned this pull request Aug 3, 2023

Update link and repo clone command to seedsigner from kdmukai SeedSigner/seedsigner-settings-generator#3

Closed

newtonick merged commit c6df80c into SeedSigner:dev Aug 3, 2023

kdmukai deleted the settings_qr branch September 2, 2024 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SettingsQR: Final changes before official release #394

SettingsQR: Final changes before official release #394

kdmukai commented Jun 21, 2023 •

edited by newtonick

Loading

jdlcdl commented Jun 21, 2023 •

edited

Loading

kdmukai commented Jul 6, 2023

jdlcdl commented Jul 27, 2023 •

edited

Loading

jdlcdl Jul 30, 2023 •

edited

Loading

kdmukai Jul 30, 2023

jdlcdl Jul 31, 2023

jdlcdl commented Jul 30, 2023 •

edited

Loading

jdlcdl commented Jul 30, 2023

jdlcdl commented Jul 30, 2023

jdlcdl commented Jul 30, 2023 •

edited

Loading

kdmukai commented Jul 30, 2023

kdmukai commented Jul 30, 2023

kdmukai commented Jul 31, 2023

jdlcdl commented Jul 31, 2023 •

edited

Loading

newtonick commented Aug 3, 2023

SettingsQR: Final changes before official release #394

SettingsQR: Final changes before official release #394

Conversation

kdmukai commented Jun 21, 2023 • edited by newtonick Loading

Changes in this PR:

How to test

jdlcdl commented Jun 21, 2023 • edited Loading

kdmukai commented Jul 6, 2023

jdlcdl commented Jul 27, 2023 • edited Loading

jdlcdl Jul 30, 2023 • edited Loading

Choose a reason for hiding this comment

kdmukai Jul 30, 2023

Choose a reason for hiding this comment

jdlcdl Jul 31, 2023

Choose a reason for hiding this comment

jdlcdl commented Jul 30, 2023 • edited Loading

jdlcdl commented Jul 30, 2023

jdlcdl commented Jul 30, 2023

jdlcdl commented Jul 30, 2023 • edited Loading

kdmukai commented Jul 30, 2023

kdmukai commented Jul 30, 2023

kdmukai commented Jul 31, 2023

jdlcdl commented Jul 31, 2023 • edited Loading

newtonick commented Aug 3, 2023

kdmukai commented Jun 21, 2023 •

edited by newtonick

Loading

jdlcdl commented Jun 21, 2023 •

edited

Loading

jdlcdl commented Jul 27, 2023 •

edited

Loading

jdlcdl Jul 30, 2023 •

edited

Loading

jdlcdl commented Jul 30, 2023 •

edited

Loading

jdlcdl commented Jul 30, 2023 •

edited

Loading

jdlcdl commented Jul 31, 2023 •

edited

Loading