Multi Mode Support

[ssayer]

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.

[whomwah]

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

[ssayer]

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

[whomwah]

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.

[whomwah]

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

[ssayer]

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]

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]

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]

@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]

@ssayer I like the idea of the QRSegment class 👍

[ssayer]

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]

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 😀

[whomwah]

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.

[whomwah]

Really close now. Some very tiny comments.

[ssayer]

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

[ssayer]

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]

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.

[whomwah]

This all looks great! 👍

[ssayer]

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

[whomwah]

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]

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.