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.

Sign up for GitHub

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

Jump to bottom

Added margins option to DataMatrixWriter & AztecWriter #60

Merged
merged 3 commits into from
Dec 24, 2024
Merged

Added margins option to DataMatrixWriter & AztecWriter #60

merged 3 commits into from
Dec 24, 2024

Conversation

snail-with-tea
Copy link
Contributor

By default margins = 0 to match previous behavior.
If margins > 0, behavior matches QRCodeWriter margins option.

@snail-with-tea
Added margins option to AztecWriter
f7e014f 
By default margins = 0 to match previous behavior.
In case margins > 0 behavior matches QRCodeWriter margins behavior.
@snail-with-tea
Added margins option for DataMatrixWriter
287e57b 
By default margins = 0 to match previous behavior.
If margins > 0, behavior matches with QRCodeWriter margins.
@snail-with-tea
Copy link
Contributor Author

Also why DataMatrixWriter removes padding if requested width and height are too small?
QRCodeWriter & AztecWriter do not do that.

@hschimke
Copy link
Collaborator

Also why DataMatrixWriter removes padding if requested width and height are too small? QRCodeWriter & AztecWriter do not do that.

The answer is that it was likely how it worked in ZXing and when I ported it over the behavior came too. It isn't an intentional choice on my part.

@hschimke hschimke self-assigned this Dec 22, 2024
@hschimke hschimke added the bug Something isn't working label Dec 22, 2024
@snail-with-tea
Copy link
Contributor Author

The answer is that it was likely how it worked in ZXing and when I ported it over the behavior came too. It isn't an intentional choice on my part.

So backwards compatibility, understandable.

hschimke
hschimke approved these changes Dec 23, 2024
View reviewed changes
Copy link
Collaborator

@hschimke hschimke left a comment

Choose a reason for hiding this comment

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

Change looks good, approved pending test verification

@hschimke
Copy link
Collaborator

Could you rebase on the latest head commit in the repository? There is an issue with one of my tests that fails which is corrected in the latest commit.

@snail-with-tea
Copy link
Contributor Author

Never done that before. But Github had "Sync fork" button, which seems to do the same.

@hschimke
Copy link
Collaborator

All tests pass. I'll merge this PR. Thank you for your contribution!

@hschimke hschimke merged commit bd52b9a into rxing-core:main Dec 24, 2024
1 check passed
@hschimke
Copy link
Collaborator

this change is now in v0.6.5 on crates.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hschimke hschimke hschimke approved these changes

Assignees

@hschimke hschimke

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@snail-with-tea @hschimke