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

Multi Mode Support #25

Merged
merged 15 commits into from Aug 12, 2021
Merged

Multi Mode Support #25

merged 15 commits into from Aug 12, 2021

Conversation

@ssayer
Copy link
Contributor

@ssayer ssayer commented Jul 7, 2021

Hey @whomwah,

Here's my first pass at addressing #23. It's not done, but I thought this was a good point to get feedback:

  • Calculating the minimum version for a multi mode QR code means I have to iteratively calculate the length of the payload on the fly based on the actual input.
  • Users must declare mode: multi and pass in an array of segments (see test for example input). I did this to ensure backwards compatibility. I had to move some code from the constructor into methods because I needed to reuse it for either size calculation or writing the QR code.
  • Added support for a "max_size" option, in order to limit the size to a version other than 40. This isn't strictly related to multi mode support, but it's useful due to the need for calculation.
  • I tried to keep my code separated where possible (e.g. MultiUtil, MultiTest), but I can move these into existing qr_code, qr_util etc. if you prefer.

Let me know what you think! I'll be working on documentation and cleanup in the meantime.

lib/rqrcode_core/qrcode/multi_util.rb Outdated Show resolved Hide resolved
@whomwah
Copy link
Owner

@whomwah whomwah commented Jul 10, 2021

Hey, thanks for this @ssayer. I'll try and get to look at this at my first opportunity.

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Jul 12, 2021

@whomwah in the meantime, can you approve me to run the workflows so I can see if all the tests/builds pass?

@whomwah whomwah assigned whomwah and ssayer and unassigned whomwah Jul 15, 2021
Copy link
Owner

@whomwah whomwah left a comment

This looks great! Some of it is over my head to be honest, but the fact it's opt in and is tested is fine with me. I like the approach. I've also made a note that the whole lib probably needs some named params love at some point where we pass multiple args to methods.

lib/rqrcode_core/qrcode/multi_util.rb Outdated Show resolved Hide resolved
test/rqrcode_core/multi_test.rb Outdated Show resolved Hide resolved
@whomwah
Copy link
Owner

@whomwah whomwah commented Jul 15, 2021

I seem to have to approve to run the CI every time. I wonder if it only remembers once you are a contributor?

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Jul 15, 2021

Thanks for the feedback, I'll get started on these items. I would like to add named params in a couple of places. I didn't see it used so I wasn't sure if that was a backwards compatibility thing, but it looks like we're targeting 2.5+ right?

@whomwah
Copy link
Owner

@whomwah whomwah commented Jul 15, 2021

I would like to add named params in a couple of places. I didn't see it used so I wasn't sure if that was a backwards compatibility thing, but it looks like we're targeting 2.5+ right?

Yes, spec.required_ruby_version = ">= 2.3". It's purely a legacy thing where I've just not had the time to make the change.

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Jul 16, 2021

One thing I was considering is adding a QRSegment class to use instead of the hashes in the Array. The original constructor style w/ string could be mapped to use this internally in the constructor. This would allow the constructor to be used in different ways:

QRCode.new('foo', mode: 'byte_8bit')
QRCode.new(QRSegment.new(mode: 'byte_8bit', data: 'foo'))
QRCode.new([QRSegment.new(mode: 'byte_8bit', data: 'foo'), QRSegment.new(mode: 'alphanumeric', data: 'bar1')])

It's a little more verbose, but I think a bit cleaner and could consolidate some of the code related to handling modes.

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Jul 20, 2021

@whomwah ^ Can you let me know what you think of the QRSegment idea? Otherwise if you'd prefer to keep the code how it is, I'll mark this as ready.

@whomwah
Copy link
Owner

@whomwah whomwah commented Jul 23, 2021

@ssayer I like the idea of the QRSegment class 👍

@ssayer ssayer marked this pull request as ready for review Jul 28, 2021
@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Jul 28, 2021

Once this is merged, we should probably add an equivalent class in RQRCode for QRSegment like you do with QRCode so we're not mixing RQRCode and RQRCodeCore when using segments.

@whomwah
Copy link
Owner

@whomwah whomwah commented Jul 28, 2021

Thanks. I should have time to have a good look at all these changes tomorrow at last. I'll also have a think about the RQRCode interface too. I'd really want to that interface to remain as simple to use as possible.

The RQRCode gem is a conversation on its own 😀

Copy link
Owner

@whomwah whomwah left a comment

A few comments now I've had time to sit a look things over. Nothing too controversial I hope. Thanks again for the work you have done so far.

lib/rqrcode_core/qrcode/qr_code.rb Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_code.rb Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_numeric.rb Outdated Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_segment.rb Show resolved Hide resolved
test/rqrcode_core/qr_segment_test.rb Show resolved Hide resolved
test/rqrcode_core/qr_segment_test.rb Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_code.rb Outdated Show resolved Hide resolved
Copy link
Owner

@whomwah whomwah left a comment

Really close now. Some very tiny comments.

README.md Outdated Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_code.rb Outdated Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_code.rb Show resolved Hide resolved
lib/rqrcode_core/qrcode/qr_code.rb Outdated Show resolved Hide resolved
@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Aug 9, 2021

Sounds good, I'll knock these out later today.

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Aug 9, 2021

I think that covers all the comments. I'll be available for any further changes until Thursday, when I leave for vacation until the 23rd.

@whomwah
Copy link
Owner

@whomwah whomwah commented Aug 12, 2021

Just seen your comment. I will look over this and do a test with RQRcode and then I think it will be ready to merge. Thank you for this piece of work and your patience getting it added. I'll wait until you are back from your vacation before I release a new version of the Gem.

Copy link
Owner

@whomwah whomwah left a comment

This all looks great! 👍

@whomwah whomwah merged commit b176d07 into whomwah:master Aug 12, 2021
8 checks passed
@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Aug 23, 2021

That's great! I'm back from vacation as of today, so let me know if you need anything.

@ssayer ssayer deleted the multi_mode_support branch Aug 23, 2021
@whomwah
Copy link
Owner

@whomwah whomwah commented Aug 23, 2021

I'm due to be working on the project this Thu so I will likely do a new release then 👍

PS I know this is not related but I'd be interested in your opinion if you have any spare time: #28

@ssayer
Copy link
Contributor Author

@ssayer ssayer commented Aug 24, 2021

Like you, I'm not an expert in bitwise operators but it would make sense that using them would be more efficient than doing math with a bunch of integer literals in Ruby (though I am surprised it makes that much difference in memory).

In regards to your comment about hardcoding to 32bits: I suppose you could try hardcoding the result of num.size * 8 into a constant in the library and just use that instead of running num.size every single time. I'm a little surprised using either 32 or 64b it works, but like I said I'm not an expert.

If you want to hold off a little on a release till both these are in, I'm not in a huge hurry. For the time being I can just point our Gemfile at GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants