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

Improve support for Macro PDF417 #973

Merged
merged 6 commits into from
Mar 15, 2018

Conversation

gitu
Copy link
Contributor

@gitu gitu commented Mar 14, 2018

I had troubles reading a PDF417 barcode with multiple symbols. These PR would fix that. It is though not reverse compatible regarding the optionalData field on the ResultMetadata.

@codecov-io
Copy link

codecov-io commented Mar 14, 2018

Codecov Report

Merging #973 into master will increase coverage by 0.13%.
The diff coverage is 90.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #973      +/-   ##
============================================
+ Coverage     75.64%   75.78%   +0.13%     
- Complexity     4153     4180      +27     
============================================
  Files           252      252              
  Lines         13952    14002      +50     
  Branches       2866     2868       +2     
============================================
+ Hits          10554    10611      +57     
+ Misses         2583     2579       -4     
+ Partials        815      812       -3
Impacted Files Coverage Δ Complexity Δ
.../com/google/zxing/pdf417/PDF417ResultMetadata.java 100% <100%> (+15.38%) 23 <14> (+16) ⬆️
...e/zxing/pdf417/decoder/DecodedBitStreamParser.java 80.63% <84.78%> (+3.02%) 74 <0> (+9) ⬆️
...le/zxing/pdf417/decoder/PDF417ScanningDecoder.java 80.65% <0%> (+0.36%) 114% <0%> (+1%) ⬆️
...ava/com/google/zxing/pdf417/detector/Detector.java 96.24% <0%> (+0.75%) 43% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3bbebc...929f531. Read the comment docs.

Copy link
Contributor

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems reasonable

/**
* @deprecated no replacement
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to continue to support this at all? just leave it also populated? I'd hate to introduce any breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just populate the array after the parsing. But there are also no tests for that functionality. I'll give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'd leave the functionality. Deprecating it is OK. This kind of has a replacement, right? the new fields you're adding have some of the same info?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly they now consume the array, the behavior now is not exactly the same as it parses the fields and allows for multiple ones to be set. For symbols that were parsed before the data found in the array should be the same.

int[] additionalOptionCodeWords = new int[codewords[0] - codeIndex];
int additionalOptionCodeWordsIndex = 0;
while (codeIndex < codewords[0]) {
switch (codewords[codeIndex]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The project uses 2-space indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added also an .editorconfig, that way i didn't had to fix the identation manually.

@@ -0,0 +1,9 @@
root = true

[*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sure, what reads this? Eclipse et al? that's fine, those settings look standard and like what the project uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eclipse needs a plugin, but some others support it by default: http://editorconfig.org/

@@ -155,8 +164,8 @@ static DecoderResult decode(int[] codewords, String ecLevel) throws FormatExcept
return decoderResult;
}

private static int decodeMacroBlock(int[] codewords, int codeIndex, PDF417ResultMetadata resultMetadata)
throws FormatException {
protected static int decodeMacroBlock(int[] codewords, int codeIndex, PDF417ResultMetadata resultMetadata)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last nit: if this is just visible for testing, it can probably be package-private only?
I had also done that extra 4-space indent for wrapping continuations like the 'throws' clause on purpose, but, I don't really care.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure no problem; will change the visibility

I'm lazy so I just use the auto layout from IDEA, which works usually quite well, but doesn't indent like that, there are additional changes like that, shall I change them back as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're editing this line anyway, maybe change it back, but wouldn't worry about undoing every whitespace change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it back mostly, makes the change a bit cleaner, shall i rebase or will you do a squash merge?

@srowen srowen merged commit dbfd552 into zxing:master Mar 15, 2018
shixingxing added a commit to shixingxing/zxing that referenced this pull request Mar 20, 2018
Improve support for Macro PDF417 (zxing#973)
@srowen srowen added this to the 3.3.3 milestone May 28, 2018
antimony pushed a commit to antimony/zxing that referenced this pull request Oct 1, 2018
* Improve support for Macro PDF417

* cleanup and move license to it's proper place

* add .editorconfig + proper indentation / add optionalFields array

* rename variables and use Arrays instead of System

* replaced length with to

* restore spaces & switch to package private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants