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

feat: Zxing implementation #3252

Merged
merged 5 commits into from
Nov 2, 2022
Merged

feat: Zxing implementation #3252

merged 5 commits into from
Nov 2, 2022

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Nov 1, 2022 •
edited
Loading

Zxing is used with:

  • The Amazon AppStore
  • Fdroid
  • Huawei App Gallery

Other variants (iOS / Google Play / Samsung Gallery) still relies on MLKit

⚠️ One thing to know with Zxing is the scan performance is not as advanced/powerful as MLKit. For example, the barcode needs to be in the same direction as the camera.

@g123k g123k added the F-Droid label Nov 1, 2022
@g123k g123k requested a review from a team as a code owner November 1, 2022 14:13
@g123k g123k self-assigned this Nov 1, 2022
@g123k
Copy link
Collaborator Author

g123k commented Nov 1, 2022 •
edited by teolemon
Loading

Weirdly I can't link this PR to :

monsieurtanuki
monsieurtanuki approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

I cannot say that I've understood everything but that looks correct.

@teolemon teolemon linked an issue Nov 1, 2022 that may be closed by this pull request
Create a ZXing (MLKit-free) build for F-Droid #1882
Closed
3 tasks
VaiTon
VaiTon reviewed Nov 1, 2022
View reviewed changes
Copy link
Member

@VaiTon VaiTon left a comment

Choose a reason for hiding this comment

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

I left a few comments. Thank you for your work!

packages/scanner/shared/lib/src/scanner.dart
Comment on lines -17 to +19
if (image is CameraImage) {
if (image is CameraImage && supportCameraImage) {
return onNewCameraImage(image);
} else if (image is String) {
} else if (image is String && supportCameraFile) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A plugin may support file and/or byte stream.
We ensure this here

packages/scanner/zxing/.gitignore Outdated Show resolved Hide resolved
packages/scanner/zxing/android/build.gradle
@@ -0,0 +1,50 @@
group 'com.example.scanner_zxing'
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to change this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is relevant, as this dependency will always be independant.

.../scanner/zxing/android/src/main/kotlin/org/openfoodfacts/scanner_zxing/ScannerZxingPlugin.kt Outdated Show resolved Hide resolved
.../scanner/zxing/android/src/main/kotlin/org/openfoodfacts/scanner_zxing/ScannerZxingPlugin.kt
Comment on lines +25 to +26
call.argument("path") as String?,
call.argument("orientation") as Int?,
Copy link
Member

Choose a reason for hiding this comment

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

If any of these two are mandatory, I would remove the nullable and make it crash if we pass something wrong. It could be subdle to find issues related to the scanner not working just to find out we're passing null somewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, the check is done in the calling method, hence the nullable accepted here

packages/scanner/zxing/android/src/main/kotlin/org/openfoodfacts/scanner_zxing/ZXingUtils.kt

object ZXingUtils {

fun extractBarcodeFromImage(path: String?, orientation: Int?): String? {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you have a look at the content of this method, I return a null string in the case of null args

packages/scanner/zxing/android/src/main/kotlin/org/openfoodfacts/scanner_zxing/ZXingUtils.kt Outdated Show resolved Hide resolved
packages/scanner/zxing/lib/src/scanner_zxing_method_channel.dart
Future<List<String?>?> onNewCameraFile(String path) async {
assert(path.isNotEmpty);

return methodChannel.invokeMethod<String>('scanFile', <String, dynamic>{
Copy link
Member

Choose a reason for hiding this comment

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

I would put an await here and then return the ternary operator below

packages/scanner/zxing/lib/src/scanner_zxing_method_channel.dart

@override
Future<List<String?>?> onNewCameraImage(CameraImage image) async {
throw UnimplementedError();
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only files are supported by Zxing -> so, yes

g123k and others added 3 commits November 1, 2022 15:50
@g123k @VaiTon
Update packages/scanner/zxing/android/src/main/kotlin/org/openfoodfac…
890f55d 
…ts/scanner_zxing/ZXingUtils.kt

Co-authored-by: VaiTon <eyadlorenzo@gmail.com>
@g123k @VaiTon
Update packages/scanner/zxing/.gitignore
2d84c2b 
Co-authored-by: VaiTon <eyadlorenzo@gmail.com>
@g123k @VaiTon
Update packages/scanner/zxing/android/src/main/kotlin/org/openfoodfac…
48494ee 
…ts/scanner_zxing/ScannerZxingPlugin.kt

Co-authored-by: VaiTon <eyadlorenzo@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #3252 (2d84c2b) into develop (4b945eb) will not change coverage.
The diff coverage is n/a.

❗ Current head 2d84c2b differs from pull request most recent head 48494ee. Consider uploading reports for the commit 48494ee to get more accurate results

@@           Coverage Diff           @@
##           develop   #3252   +/-   ##
=======================================
  Coverage     7.86%   7.86%           
=======================================
  Files          250     250           
  Lines        12315   12315           
=======================================
  Hits           968     968           
  Misses       11347   11347

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@M123-dev
Copy link
Member

M123-dev commented Nov 2, 2022 •
edited
Loading

I have no expierence in Kotlin so I can't add any value here, but I'm sure it will work

@M123-dev M123-dev merged commit c72242a into openfoodfacts:develop Nov 2, 2022
@github-actions github-actions bot mentioned this pull request Nov 2, 2022
Merged
@Altonss Altonss mentioned this pull request Nov 4, 2022
Closed
4 tasks
@M123-dev M123-dev added this to the v4 milestone Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VaiTon VaiTon VaiTon left review comments

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@g123k g123k

Labels
F-Droid 🤳 ZXing
Projects
None yet
Milestone
v4.1
Development

Successfully merging this pull request may close these issues.

Create a ZXing (MLKit-free) build for F-Droid
6 participants
@g123k @codecov-commenter @M123-dev @monsieurtanuki @VaiTon @teolemon