-
-
Notifications
You must be signed in to change notification settings - Fork 533
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: Fix the issue of incorrect QR code position detection when Boxfit is … #1303
base: develop
Are you sure you want to change the base?
Conversation
example test video: |
/// Calculate the scaling ratios for width and height to fit the small box (cameraPreviewSize) | ||
/// into the large box (size) based on the specified BoxFit mode. | ||
/// Returns a record containing the width and height scaling ratios. | ||
({double widthRatio, double heightRatio}) calculateBoxFitRatio( |
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.
Can we add unit tests for this function? Then we can assure that the math is right.
test/scan_window_test.dart
Outdated
const cameraPreviewSize = Size(480.0, 640.0); | ||
const size = Size(432.0, 256.0); | ||
|
||
test('scp p: BoxFit.fill', () { |
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.
What is the meaning of the scp p:
in the labels?
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.
I don't know what the naming convention for test functions is, I just saw that the previous tests used abbreviations.
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.
I don't know the reasoning behind the existing abbreviations either. I would rather avoid introducing abbreviations altogether. It makes the code harder to reason about.
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.
Just a comment about fixing the test names & a small concern about the test results being duplicated.
size, | ||
); | ||
|
||
expect(ratio.widthRatio, 0.4); |
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.
Can we use several different sizes, to have the numbers be different for each test?
I'd like to avoid introducing a bug because one test group accidentally had 0.9 / 0.9
several times in tests within the group. If we use different sizes, then the outcomes will also be unique among tests in a test group.
When you define the extra sizes, you can reuse them for landscape as well (since that is just flipping the width/height)
const cameraPreviewSize = Size(480.0, 640.0); | ||
const size = Size(432.0, 256.0); | ||
|
||
test('scpsip: BoxFit.fill', () { |
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.
Maybe you can change the inner test group name to already mention portrait / landscape? Then that does not need to be repeated in the tests themselves and the abbreviations can be removed?
group('calculateBoxFitRatio', () {
group('smaller portrait camera preview size', () {
test('fit: BoxFit.fill', () { /* ... */ });
// more tests
});
group('smaller landscape camera preview size', () {
test('fit: BoxFit.fill', () { /* ... */ });
// more tests
});
})
my qrcode scan code:
before:
![3db30c82481a125934ffc29ecc2eaf2](https://private-user-images.githubusercontent.com/178561373/405931153-dc11e51d-d6df-45c9-9e23-eb00bb0304c1.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0MDc2OTMsIm5iZiI6MTczOTQwNzM5MywicGF0aCI6Ii8xNzg1NjEzNzMvNDA1OTMxMTUzLWRjMTFlNTFkLWQ2ZGYtNDVjOS05ZTIzLWViMDBiYjAzMDRjMS5qcGc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxM1QwMDQzMTNaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0zZGQzMmM2Yjk3ODdlYzNmZGUwNzY4YTUzNTUwNWMwNzI3ZTZmMDU2NTY2ZWY5MWQ1OWYxYjkyYTIzMTQ2MGIzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.fWjL6WXaVeOimsG2_i3Uquj-cXS-7g9hZs1ddumAKEY)
fixed after:
![5f1fa23a7a5791334c0f760070f3b48](https://private-user-images.githubusercontent.com/178561373/405931215-3bc96412-d127-40b1-ad6f-ca4056baa537.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0MDc2OTMsIm5iZiI6MTczOTQwNzM5MywicGF0aCI6Ii8xNzg1NjEzNzMvNDA1OTMxMjE1LTNiYzk2NDEyLWQxMjctNDBiMS1hZDZmLWNhNDA1NmJhYTUzNy5qcGc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxM1QwMDQzMTNaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iNjZmNmI0MTA4M2E4YThhNDI3NzA1YjNiY2YzYTIzMjYyN2QyMDhhY2NjMzY4YzA2Njg5ZmMwNWEwN2ExMjFjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.XEzDe6D9IuepF2AVlTf3hIAoQaxMNsrKKFX4sbbwycg)