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

Expose QR code generation #60

Closed
tmpfs opened this issue Sep 7, 2023 · 7 comments · Fixed by #61
Closed

Expose QR code generation #60

tmpfs opened this issue Sep 7, 2023 · 7 comments · Fixed by #61
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@tmpfs
Copy link
Contributor

tmpfs commented Sep 7, 2023

When the qr feature is enabled it would be useful if we could access the QR code generation function as there are other parts of my application that also require generating QR codes that are not TOTP URLs.

Furthermore, I need access to the raw PNG bytes so base64 encoding the QR code should be optional.

Currently I am maintaining a fork so I don't have duplicate QR code generation logic but would prefer to just depend on this library.

What do you think? Thanks 🙏

@constantoine
Copy link
Owner

Hi!

Indeed I think it would be nice to have the option to return bytes instead of base64! Plus I still wanna spend some more times trying to optimize this one further, alas it's like the Bench will never be stabilized in the next decade

As for the QRCode generation functions, I feel like they would be a bit redundant? Like totp-rs would become a mix of totp-rs + qrcodegen utils, which I think is out of scope?

@constantoine constantoine added enhancement New feature or request question Further information is requested labels Sep 8, 2023
@constantoine constantoine self-assigned this Sep 8, 2023
@tmpfs
Copy link
Contributor Author

tmpfs commented Sep 9, 2023

Hi @constantoine, I understand the concern about scope however it would certainly be nice for user's of this library to avoid code duplication when they have a requirement to generate QR codes for strings other than TOTP URLs.

I can see a couple of ways to approach this, either mark the functions with #[doc(hidden)] and leave them undocumented. Or be more formal and add a qrcodegen-image crate that contains those utility functions that bridge the qrcodegen and image crates and then depend on that in totp-rs when the qr feature is enabled.

I think the second solution is better but I wonder what you think?

Happy to do the leg work to see this happen as it is my use case that is driving this feature request.

@constantoine
Copy link
Owner

constantoine commented Sep 9, 2023

Hi! Thanks for your feedback. I feel like the second option would be best. I appreciate you volunteering, if you're too busy I can start working on this solution tomorrow :)

For the trivia as to why I returned the image as its b64 digest: At the time, I returned the image as base64 because the rationale was "I want the picture as a blob, not a file served on server", but yeah this was probably mistaken

@tmpfs
Copy link
Contributor Author

tmpfs commented Sep 10, 2023

Thanks @constantoine, I have updated the PR to use a workspace member for qrcodegen-image. Once you have reviewed the PR I think all that is needed is:

  1. Publish the qrcodegen-image crate
  2. Remove the path from the dependency on qrcodegen-image
  3. Push and merge the PR

Hope that helps!

@tmpfs
Copy link
Contributor Author

tmpfs commented Sep 10, 2023

Also, it might be worth deprecating get_qr and exposing get_qr_base64 and get_qr_png. We could just call get_qr_base64 from get_qr for now and then remove it in the next major version bump.

@tmpfs
Copy link
Contributor Author

tmpfs commented Sep 11, 2023

Thanks @constantoine for publishing crates with those updates - that really helped 🙏

@constantoine
Copy link
Owner

Thanks to you for helping out, I really appreciate it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants