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: Choice between classic and MLkit scanner with a system of dimensions to disable binary blobs for F-Droid #3836

Merged
merged 14 commits into from
Mar 9, 2021
Merged

feat: Choice between classic and MLkit scanner with a system of dimensions to disable binary blobs for F-Droid #3836

merged 14 commits into from
Mar 9, 2021

Conversation

kartikaysharma01
Copy link
Contributor

@kartikaysharma01 kartikaysharma01 commented Feb 16, 2021 •
edited
Loading

Description

Implemented mlKit scanner using Android Camera API to support minSDK 16.

Fixed Trouble Scanning button.

Implemented "Stop the barcode scanner from scanning barcodes on product addition" . The Camera preview now freezes on barcode detection and can be resumed by clicking on the screen or dragging down the bottom sheet .

Related issues

Fixes #3400
https://drive.google.com/file/d/1h8Bk-ZFOYoqkRvf_mtsy4Kj_0Gcnuv4p/view?usp=sharing

Fixes #3015
https://drive.google.com/file/d/1glTAN_u6muBowP1EIc6wWmUx6Y0vGDVC/view?usp=sharing

Fixes #3834
https://drive.google.com/file/d/1gmbLzyCerfdylGP3w7mGd30VJBHbsJTF/view?usp=sharing

@kartikaysharma01
Copy link
Contributor Author

I have disabled the "Scan Sound" option for the New Scanner for now, and maybe we can implement it later in a new issue.
Also, I will need some help with the F-droid build.
@teolemon @VaiTon Can you please guide me.

@VaiTon
Copy link
Member

VaiTon commented Feb 17, 2021

@teolemon so we are replacing? Don't we want to make them both available?

@teolemon
Copy link
Member

@kartikaysharma01 @VaiTon is a check like
if package-name = org.open(x)facts.scanner then mlkit
else: classic scanner
something doable ?
We shouldn't include MLKIT at build time for F-Droid

@teolemon
Copy link
Member

Also related: we should disable Sentry for F-Droid, since it bugs with AUTH at build time.

@teolemon teolemon changed the title Mlkit scanner Implemented MLkit scanner Implemented Feb 18, 2021
@kartikaysharma01
Copy link
Contributor Author

@teolemon so we are replacing? Don't we want to make them both available?

@VaiTon If you are referring to the scanners, no the user can switch between the two scanners anytime in settings.

@kartikaysharma01
Copy link
Contributor Author

@teolemon Can you please provide some documentation on that.

@kartikaysharma01
Copy link
Contributor Author

@teolemon @VaiTon
The build worked using fdroid along with MLKit Scanner. Is the non-free classification assigned later? There were no such logs during build.

Logs https://slack-files.com/T02KVRT1Q-F01NQ5F698D-d5e39bb5d8

Signed APK generated from Fdroid: https://slack-files.com/T02KVRT1Q-F01NT80RR18-e0534022b1

@teolemon
Copy link
Member

yes, it might work in local, but it will be reviewed and removed if we include any non-free binaries.
The switch mechanism is cool for the Play Store version but for fdroid (the package name will contain scrachx instead of being org.openfoodfacts.scanner) we need to get rid of any calls to the binary library from google othewise it will be rejected as non-free

@kartikaysharma01
Copy link
Contributor Author

@teolemon @VaiTon This is complete and extensively tested. Ready for merge.
Will raise a PR for fdroid-data repo too. @teolemon

@kartikaysharma01
Copy link
Contributor Author

Some info about the PR:
I have added separate build dimensions for fdroid and playstore.

In playstore, the camera package contains the classes used for implementing the Camera API and the scanner package contains the classes implementing MLKit Scanner.
image

In fdroid, we have the older version of continuous scan activity without MLKit implementation.
image

@teolemon teolemon requested review from VaiTon and a team February 27, 2021 09:53
@teolemon teolemon changed the title MLkit scanner Implemented Choice between classic and MLkit scanner with a system of dimensions to disable binary blobs for F-Droid Feb 27, 2021
@teolemon teolemon changed the title Choice between classic and MLkit scanner with a system of dimensions to disable binary blobs for F-Droid feat: Choice between classic and MLkit scanner with a system of dimensions to disable binary blobs for F-Droid Feb 27, 2021
@kartikaysharma01
Copy link
Contributor Author

@VaiTon pushed the changes

@teolemon
Copy link
Member

teolemon commented Mar 4, 2021

The screenshot test is failing, possibly because of the additional dimension:
https://github.com/openfoodfacts/openfoodfacts-androidapp/pull/3836/checks?check_run_id=2017171362

FAILURE: Build failed with an exception.

* What went wrong:
Task 'compileOffScreenshotsAndroidTestSources' not found in root project 'openfoodfacts-androidapp'.

@kartikaysharma01
Copy link
Contributor Author

The screenshot test is failing, possibly because of the additional dimension:
https://github.com/openfoodfacts/openfoodfacts-androidapp/pull/3836/checks?check_run_id=2017171362

@teolemon Yes, the new dimension is the reason.

@kartikaysharma01
Copy link
Contributor Author

@teolemon I think the failing test can be rectified after review process is complete.

VaiTon
VaiTon requested changes Mar 6, 2021
View reviewed changes
...rc/fdroid/java/openfoodfacts/github/scrachx/openfood/features/scan/ContinuousScanActivity.kt Show resolved Hide resolved
...rc/fdroid/java/openfoodfacts/github/scrachx/openfood/features/scan/ContinuousScanActivity.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/camera/CameraReticleAnimator.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/camera/CameraReticleAnimator.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/camera/CameraReticleAnimator.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/utils/CameraUtils.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/utils/CameraUtils.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/utils/CameraUtils.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/utils/CameraUtils.kt Outdated Show resolved Hide resolved
app/src/playstore/java/openfoodfacts/github/scrachx/openfood/utils/InputInfo.kt Outdated

@Synchronized
override fun getBitmap(): Bitmap {
return bitmap ?: let {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a lazy property for this?

@kartikaysharma01
Copy link
Contributor Author

@VaiTon I have pushed the changes. Will resolve the merge conflicts once the PR is approved.

VaiTon
VaiTon approved these changes Mar 9, 2021
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.

@kartikaysharma01 please rebase/merge and then we're good to go!

@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
25.9% 25.9% Duplication

@kartikaysharma01
Copy link
Contributor Author

@VaiTon Have merged develop into this branch. Ready for merge.

@VaiTon VaiTon merged commit 4594db1 into openfoodfacts:develop Mar 9, 2021
@VaiTon
Copy link
Member

VaiTon commented Mar 9, 2021

@kartikaysharma01 thank you! 🎉

@teolemon teolemon mentioned this pull request Mar 9, 2021
Closed
@teolemon teolemon mentioned this pull request May 18, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VaiTon VaiTon VaiTon approved these changes

Assignees
No one assigned
Labels
scanning
Projects
None yet
Milestone
No milestone
3 participants
@kartikaysharma01 @VaiTon @teolemon