-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
Fix merge mistake in Code128Reader #574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to counters = counters.slice(2);
@@ -199,7 +199,7 @@ export default class Code128Reader extends OneDReader { | |||
} | |||
patternStart += counters[0] + counters[1]; | |||
|
|||
counters = counters.slice(2, counters.length - 1); | |||
counters = counters.slice(2, counters.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately splice() cannot be used here, as that method is only available on normal arrays, but counters
is a TypedArray.
The code indeed used to use splice() prior to 0.18.3 when counters
was just a regular array, but that has changed a few commits back (and I don't understand the underlying algorithms enough to know whether TypedArrays are really necessary, but they might, considering that the original java ZXing code uses integers there, not floats)
And as to why I chose to go with the longer counters = counters.slice(2, counters.length);
- it's because that's what #536 did and I figured I'd just do the same for the sake of consistency, but you are right, the second argument is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctly slices the counters
TypedArray to the end.
Can we get a patch release with this change? Would be greatly appreciated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks
Sorry for the delay, I have actually not watched this repo...... |
This PR is pretty much the same as #536, same commit, same issue, different Reader.
Fixes #566
Fixes #394