Skip to content

Conversation

@repolevedavaj
Copy link
Contributor

Hello! I would like to use this library to generate QR codes, but need the structured append mode. I tried to stick to the style of the QrCode class and the structured append mode implementation of Pdf417, but the implementation could be optimized (e.g. I could move the automated structured append symbol generation of both symbol classes to a common place). Please let me know what you think.

@gredler
Copy link
Collaborator

gredler commented Jun 8, 2025

Thanks for the contribution! It may be a few days before I'm able to review it, but I will get to it later this week.

@repolevedavaj
Copy link
Contributor Author

repolevedavaj commented Jun 10, 2025

I just saw that there is a Checkstyle violation. I will take care about it asap.

Update: I added the missing license header.

@repolevedavaj repolevedavaj force-pushed the fearture/qr-code-structured-append branch from 557c76c to c021f45 Compare June 10, 2025 19:47
@gredler
Copy link
Collaborator

gredler commented Jun 11, 2025

Starting to review, but can you add 7 test cases under src/test/resources/uk/org/okapibarcode/backend/qrcode?

  • One series of 4 symbols which do not use ECI, duplicating the example included in the QR Code spec (see below)
  • A series of 3 symbols which also uses ECI (maybe using some Japanese test text).

Here's the 4-symbol sequence in the standard which it would be nice to recreate for the first symbol sequence:

image

@repolevedavaj
Copy link
Contributor Author

Sure, i will work on that on the weekend.

@repolevedavaj
Copy link
Contributor Author

@gredler done. I detected an issue in the creation of the parity: I always used a fixed encoding. Now I use the same which will be used to encode the actual data (as defined in the spec).

Copy link
Collaborator

@gredler gredler left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall!

repolevedavaj and others added 20 commits June 14, 2025 20:42
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
repolevedavaj and others added 6 commits June 14, 2025 20:58
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
Co-authored-by: Daniel Gredler <daniel.gredler@gmail.com>
@repolevedavaj
Copy link
Contributor Author

repolevedavaj commented Jun 14, 2025

@gredler thank you very much for the review. I resolved or responded to all comments, except the ones regarding the additional error test cases (I am still working on that). The commit history got a bit messy, as I took over the suggestions one by one, but I assume you agree if I squash my commits after all comments have been resolved.

I will let you know as soon as I created the additional test cases.

@repolevedavaj repolevedavaj requested a review from gredler June 15, 2025 21:08
@repolevedavaj
Copy link
Contributor Author

@gredler I am done with my changes. Let me know what you think.

@gredler
Copy link
Collaborator

gredler commented Jun 16, 2025

Did you regenerate the three ECI images after changing the parity? It looks like the tests are failing during the image comparison.

@repolevedavaj
Copy link
Contributor Author

Did you regenerate the three ECI images after changing the parity? It looks like the tests are failing during the image comparison.

Ah, I was just wondering why the test fails... thanks!

@repolevedavaj repolevedavaj requested a review from gredler June 16, 2025 20:31
Copy link
Collaborator

@gredler gredler left a comment

Choose a reason for hiding this comment

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

Looks good! I may look at refactoring a few pieces to reduce duplication, but I need to check whether it makes sense or not. Thanks for the contribution!

@gredler gredler merged commit f4d6e2c into woo-j:master Jun 16, 2025
2 checks passed
@repolevedavaj repolevedavaj deleted the fearture/qr-code-structured-append branch June 16, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants