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 RXingResultPoint usage #13

Merged
merged 19 commits into from
Feb 17, 2023
Merged

Conversation

Asha20
Copy link
Contributor

@Asha20 Asha20 commented Feb 15, 2023

Closes #12

Previously, the struct was passed around everywhere as a
reference. It only holds two floats, so there's no real
need for indirection. Now it's being passed as a value.
Build fails because the OneDReader proc macro expects
that a RXingResultPoint type exists.
Currently it's not possible to rename RXingResultPoint
to Point cleanly, since the external OneDReader proc macro
makes use of them. For the time being, simple aliases
are introduced to get the code to compile, but the plan
is to remove them at a later point.
Shorter than writing `Point::new`
Point already has `x` and `y` properties, so accessing
them with `getX()` and `getY()` is needlessly verbose.
These calls have been removed, though the trait impl is
still presen for the time being, since the OneDReader
proc macro requires it.
@Asha20
Copy link
Contributor Author

Asha20 commented Feb 16, 2023

@hschimke I think this might be a good point for a review and maybe a merge if everything looks good. There's still more that can be worked on, but I feel like there's enough improvement here that it's worth merging it and adding more with another PR.

Here's what this PR includes:

  • RXingResultPoint is renamed to Point
  • MathUtils is completely gone
  • Some other duplicated point math functions are gone
  • ResultPoint usage across the repo is completely removed (the only reason it's still there is because the proc macro wants it implemented for Point)

@hschimke
Copy link
Collaborator

I'll review this evening. For reference: it's just now morning for me.

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.

Reviewed the code and everything looks good. CI passed.

@hschimke
Copy link
Collaborator

This looks great, I passed review, I will merge in the morning. Thanks for your hard work!

@hschimke hschimke merged commit 01e4f4a into rxing-core:main Feb 17, 2023
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.

Refactoring RXingResultPoint usage
2 participants