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

QR Encoder refactor + fountain encoder restart() #494

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

kdmukai
Copy link
Contributor

@kdmukai kdmukai commented Sep 23, 2023

Description

Refactors the QR encoders to be more organized and more DRY. The new QREncoder classes clean up all the base classes and inheritance to make each one as simple as possible.

Also adds restart() option to fountain encoders (not used yet; further experiments with optimizing animated QRs still ongoing and will be separate PRs).

Additional details:

  • Moves models.encode_qr.py to helpers.qr_encoders.py; in a more traditional app, a "model" is a biz obj that would roughly map to a database table. The QR encoders are really more like utils.
  • Remove already-deprecated QRType.PSBT__SPECTER encoder and associated test.
  • Slightly expand the SeedQR encoder tests.
  • Initial attempt to standardize on "mnemonic" over "seed_phrase"

Include screenshots of any new or modified screens (or at least explain why they were omitted)

n/a

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?

  • Yes

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

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 kdmukai changed the title QR Decoder refactor + fountain encoder restart() QR Encoder refactor + fountain encoder restart() Sep 23, 2023
@jdlcdl
Copy link

jdlcdl commented Sep 23, 2023

seen as of 2ce691a,

I'm assuming this is a prerequisite to #495.
I've read the code changes just once, so far.
Tests/screenshots are all passing: above, on my desktop, and on raspi-os on pi2.
I'll play with scanning different types of qrcodes on the raspi-os-pi2, and on ss-os-pi0 with image: 9dad21e0a9e6aa83526871bd1c4c11ff4aa9f24d015fba4054d92a1f99da8ac1

After initial testing on both setups: seeds, addresses, wallet descriptors, messages, unsupported xpubs, are all scanning without errors. No strange breakages found.

@kdmukai kdmukai force-pushed the qr_encoding_refactor branch from 3ecd606 to 98e9ca8 Compare March 3, 2024 15:12
@newtonick
Copy link
Collaborator

ACK and Tested

@newtonick newtonick merged commit b17677c into SeedSigner:dev Apr 23, 2024
1 check passed
@kdmukai kdmukai deleted the qr_encoding_refactor branch September 2, 2024 14:01
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.

3 participants