constantoine/totp-rs

Expose QR code generation

Closed this issue · 7 comments

tmpfs commented

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 commented

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?

tmpfs commented

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 commented

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 commented

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 commented

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 commented

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

  • 👍1
constantoine commented

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

  • 👍1