feat: 3712 - new mlkit and zxing scanners on flutter 3.7 #3767

monsieurtanuki · 2023-03-15T09:42:57Z

New files:

smooth_barcode_scanner_mlkit.dart: Barcode scanner based on MLKit.
smooth_barcode_scanner_mockup.dart: Fake barcode scanner, for tests.
smooth_barcode_scanner_type.dart: Barcode scanner types.
smooth_barcode_scanner_zxing.dart: Barcode scanner based on ZXing.

Deleted files:

scanner: removed folder
camera_controller.dart
camera_image_cropper.dart
camera_full_getter.dart
camera_image_preview.dart
camera_modes.dart
lifecycle_aware_widget.dart
lifecycle_manager.dart
scan_flash_toggle.dart
scan_visor.dart
scanner_overlay.dart
user_preferences_dialog_editor.dart

Impacted files:

app_test.dart: now uses new enum SmoothBarcodeScannerType
background_task_badge.dart: now uses flutter 3.7 badge
basic_test.dart: now uses new enum SmoothBarcodeScannerType
build.gradle: upgraded kotlin to 1.8.0
camera_helper.dart: simplified
camera_scan_page.dart: simplified
constant_icons.dart: added an adaptive "flip camera" icon
file_cache_manager_impl.dart: minor 3.7 refactoring
goldens.dart: minor 3.7 refactoring
labeler.yml: removed references to deleted files
main_fdroid.dart: now uses new enum SmoothBarcodeScannerType
main_google_play.dart: now uses new enum SmoothBarcodeScannerType
main_ios.dart: now uses new enum SmoothBarcodeScannerType
network_config.dart: minor 3.7 refactoring
new_product_page.dart: minor 3.7 refactoring
onboarding_bottom_bar.dart: minor 3.7 refactoring
pubspec.lock: wtf
apple_app_store/pubspec.yaml: minor 3.7 refactoring
google_play/pubspec.yaml: minor 3.7 refactoring
shared/pubspec.yaml: minor 3.7 refactoring
uri_store/pubspec.yaml: minor 3.7 refactoring
data_importer/pubspec.yaml: minor 3.7 refactoring
data_importer_shared/pubspec.yaml: minor 3.7 refactoring
smooth_app/pubspec.yaml: now includes mlkit and zxing scanners; minor 3.7 refactoring; removed now redundant badge; minor upgrades
scan_page.dart: simplified
user_preferences.dart: removed now useless methods
user_preferences_dev_mode.dart: removed duration and scan parameter settings
user_preferences_settings.dart: removed now useless parameter; minor 3.7 refactoring

What

Heavy refactoring of the barcode scanners.
Basically, now we rely on existing barcode scanners: mobile_scanner for mlkit and qr_code_scanner for zxing.
En passant, upgraded to flutter 3.7.

Screenshot

mlkit	zxing

Fixes bug(s)

New files: * `smooth_barcode_scanner_mlkit.dart`: Barcode scanner based on MLKit. * `smooth_barcode_scanner_mockup.dart`: Fake barcode scanner, for tests. * `smooth_barcode_scanner_type.dart`: Barcode scanner types. * `smooth_barcode_scanner_zxing.dart`: Barcode scanner based on ZXing. Deleted files: * `scanner`: removed folder * `camera_controller.dart` * `camera_image_cropper.dart` * `camera_full_getter.dart` * `camera_image_preview.dart` * `camera_modes.dart` * `lifecycle_aware_widget.dart` * `lifecycle_manager.dart` * `scan_flash_toggle.dart` * `scan_visor.dart` * `scanner_overlay.dart` * `user_preferences_dialog_editor.dart` Impacted files: * `app_test.dart`: now uses new enum `SmoothBarcodeScannerType` * `background_task_badge.dart`: now uses flutter 3.7 badge * `basic_test.dart`: now uses new enum `SmoothBarcodeScannerType` * `build.gradle`: upgraded kotlin to 1.8.0 * `camera_helper.dart`: simplified * `camera_scan_page.dart`: simplified * `constant_icons.dart`: added an adaptive "flip camera" icon * `file_cache_manager_impl.dart`: minor 3.7 refactoring * `goldens.dart`: minor 3.7 refactoring * `labeler.yml`: removed references to delete files * `main_fdroid.dart`: now uses new enum `SmoothBarcodeScannerType` * `main_google_play.dart`: now uses new enum `SmoothBarcodeScannerType` * `main_ios.dart`: now uses new enum `SmoothBarcodeScannerType` * `network_config.dart`: minor 3.7 refactoring * `new_product_page.dart`: minor 3.7 refactoring * `onboarding_bottom_bar.dart`: minor 3.7 refactoring * `pubspec.lock`: wtf * `apple_app_store/pubspec.yaml`: minor 3.7 refactoring * `google_play/pubspec.yaml`: minor 3.7 refactoring * `shared/pubspec.yaml`: minor 3.7 refactoring * `uri_store/pubspec.yaml`: minor 3.7 refactoring * `data_importer/pubspec.yaml`: minor 3.7 refactoring * `data_importer_shared/pubspec.yaml`: minor 3.7 refactoring * `smooth_app/pubspec.yaml`: now includes mlkit and zxing scanners; minor 3.7 refactoring; removed now redundant `badge`; minor upgrades * `scan_page.dart`: simplified * `user_preferences.dart`: removed now useless methods * `user_preferences_dev_mode.dart`: removed duration and scan parameter settings * `user_preferences_settings.dart`: removed now useless parameter; minor 3.7 refactoring

monsieurtanuki · 2023-03-15T09:53:52Z

The code was written in flutter 3.7, as it was the main focus of the issue. The barcode scan refactoring was "only" a side-effect.
Therefore, the CI has to be upgraded to 3.7 / 2.19 for the pre-tests to work.
Meanwhile, we would be better off with a new official version of Smoothie - hopefully the last flutter 3.0.5 version.

M123-dev · 2023-03-15T10:47:58Z

Should I push the ci changes on your branch

monsieurtanuki · 2023-03-15T11:19:19Z

Should I push the ci changes on your branch

Please do, thanks!

teolemon · 2023-03-15T14:03:41Z

I'm making slow progress on pushing out the release to the Apple app store, we had an issue with the CI, which is not solved yet.

teolemon · 2023-03-15T16:03:35Z

Ok, I've fixed the build, and the iOS build has been proposed to Apple.

adilwahla

LOOKS GOOD

M123-dev

First of all thanks for your work @monsieurtanuki I can't try to test this out.
Added some comments but no dealbreaker in it.

Some other things I want to have written down, do not forget (not for this PR):

We should implement some kind of on phone testing*
Ci can be fixed to the new version with no problem, though cd is going to be harder, we likely can't build if we remove the ml or zxing dependencie, but let this be my worry in a follow-up pr.
We need extensive analytics

When someone first starts the app (also after an update?) we hand the currently selected engine a mock image which a barcode, as close to how the scanner does it as possible. If it does not work, we report it to sentry/matomo? and switch the engine. I'd suggest to at first push both to play store to now break anything on edge devices.

On some devices, it seemed that ml kit only works when we store the image on the storage and let ml kit read it from there.

M123-dev · 2023-03-15T17:32:54Z

packages/smooth_app/lib/helpers/camera_helper.dart

-  /// Returns if the device has at least one camera
-  static bool get hasACamera {
+  /// Returns if the device has more than one camera
+  static bool get hasMoreThanOneCamera {
    if (_cameras == null) {
      throw Exception('Please call [init] before!');


Suggested change

throw Exception('Please call [init] before!');

throw Exception('Please call [init] before, no cameras found!');

No no, if you haven't called init before it's null. If you have called init before and found no camera, it's empty.

M123-dev · 2023-03-16T11:13:12Z

packages/smooth_app/android/build.gradle

-    ext.kotlin_version = '1.6.10'
+    ext.kotlin_version = '1.8.0'


Was kind of impact has this

No idea. It's just that I couldn't build locally without upgrading.
Maybe flutter 3.7 related?

@M123-dev Please try to build apk locally.
To me it's a nightmare with that stupid kotlin and that stupid gradle, that I was supposed to freely despise using flutter. But in fact I'm trapped.
On top of that, stupid Android Studio cannot downgrade the kotlin version, so nothing is compatible anymore between those 3 stupid moronic tools.

I had some problems at first with some invalid caches which I had to delete myself and a lot of warnings about the kotlin version but it turned out to work eventually

M123-dev · 2023-03-16T16:20:26Z

packages/smooth_app/lib/pages/scan/camera_scan_page.dart

-        ),
-      );
+    if (!CameraHelper.hasACamera) {
+      return EMPTY_WIDGET;


Maybe log something here, or show a dedicated message on the page. Unlikely for real phones, but great for debugging.

(If someone see's a "We've found no camera" text they know what to report in contrast to just a blank screen)

(If someone see's a "We've found no camera" text they know what to report in contrast to just a blank screen)

Fair enough.

Just pushed:

@M123-dev Tell me if that's enough, and I localize the message.

We could offer common solutions if there are any (like restarting the app, rebooting device, attaching external camera, getting your device fixed)

@teolemon I wish there were a "getting your device fixed" button :)
I don't think there are any implementable common solution.
The best thing I can do, at least for this PR, is to change the "No camera found" label. Perhaps in another PR an additional "try to detect a camera" button?

M123-dev · 2023-03-16T16:24:04Z

packages/smooth_app/lib/pages/scan/smooth_barcode_scanner_mlkit.dart

+    try {
+      await _controller.stop();
+      _isStarted = false;
+    } on Exception catch (e) {


We should log to sentry

Eventually. For the moment I'm still not 100% sure about the visibility/invisibility steps.

M123-dev · 2023-03-16T16:25:09Z

packages/smooth_app/lib/pages/scan/smooth_barcode_scanner_mlkit.dart

+                ),
+                Container(
+                  decoration: ShapeDecoration(
+                    shape: zxing.QrScannerOverlayShape(


zxing mentioned on the mlkit page?

Indeed:

zxing comes with a visor, mlkit does not, so if we use zxing's visor in mlkit we have the same UI

of course we could create a custom visor, different from zxing's, and use it in zxing and mlkit

to be discussed - I'm not sure it's worth to hassle to "xor" zxing and mlkit, code wise. We can just embed both, while using the same "flavors" as now and use only one barcode scanner, depending on the entry point parameter

What about the one we had before

What about the one we had before

To be honest it doesn't look that different.
But if needed, please create an issue asking for the old painter from scan_visor.dart to be implemented, instead of the default zxing visor.

monsieurtanuki · 2023-03-16T17:51:08Z

First of all thanks for your work @monsieurtanuki

You're welcome!

We should implement some kind of on phone testing

I'm not sure how we could do that, if you mean automated tests.

Ci can be fixed to the new version with no problem, though cd is going to be harder, we likely can't build if we remove the ml or zxing dependencie, but let this be my worry in a follow-up pr.

Not sure what you mean, but as I said in a comment for this PR we may not have to select either zxing or mlkit, build wise: we just embed both and the only different is the entry point that selects which one.

We need extensive analytics

I'm not sure that's relevant anymore, as most of the work is done by the external packages.
Which analytics would you like to add?

When someone first starts the app (also after an update?) we hand the currently selected engine a mock image which a barcode, as close to how the scanner does it as possible. If it does not work, we report it to sentry/matomo? and switch the engine. I'd suggest to at first push both to play store to now break anything on edge devices.

Is that a fact? I believed that there was only one embedded scanner in the previous version.
In addition to that, that would make sense only for mlkit downgraded to zxing, as - for some reason - on FDroid mlkit is "forbidden".
I don't think we can provide an image to the engine; it works with the camera.

On some devices, it seemed that ml kit only works when we store the image on the storage and let ml kit read it from there.

Again, that's supposed to be dealt with the external mlkit package. Anyway there's no related parameter in the package. If you could find the related issue, we should contact the "problematic" users for additional tests.

@teolemon Additional thought: now we can easily create automated screenshots of the home page of the app, if we just create a specific "scanner type" called "screenshot" that displays a saved picture, with the zxing visor on top.

Impacted file: * `camera_scan_page.dart`: new "no camera found" message

teolemon · 2023-03-17T11:13:24Z

I was thinking on how to make the carousel feel immersive. So far, I only have half-convincing/bad ideas.

get rid of the carousel and loose the compare feature, and move to a card based UI at the bottom of the screen
make the carousel edge to edge, and get rid of the card metaphor
some kind of very low resolution background for the carousel, based on the dominant colors of the camera stream
Find a way to move the focus to the bottom up of the screen
Your idea here

monsieurtanuki · 2023-03-17T11:37:56Z

I was thinking on how to make the carousel feel immersive. So far, I only have half-convincing/bad ideas.

I've just created #3777.
In the current PR we change flutter version and 78 files are created/deleted/changed, therefore I suggest to do things step by step, issue by issue, PR by PR.

M123-dev

You are right, we are somewhat restricted on testing the scanner, don't know if that's good or not. I'd say let's merge iterate further

Impacted files: * `app_en.arb`: new label for "no camera found" * `app_fr.arb`: new label for "no camera found" * `camera_scan_page.dart`: localized "no camera found" message

monsieurtanuki · 2023-03-17T14:36:10Z

Thank you @adilwahla @M123-dev @teolemon for your reviews, feedbacks and commits!
It's not that I'm shy but I'm not sure when I can merge this PR, regarding the 4.6.0 version?

M123-dev · 2023-03-17T14:39:11Z

Feel free to merge, the release is done

monsieurtanuki requested a review from a team as a code owner March 15, 2023 09:42

github-actions bot assigned monsieurtanuki Mar 15, 2023

monsieurtanuki requested review from AshAman999 and M123-dev March 15, 2023 09:48

monsieurtanuki changed the title ~~feat/3712 - new mlkit and zxing scanners on flutter 3.7~~ feat: 3712 - new mlkit and zxing scanners on flutter 3.7 Mar 15, 2023

adilwahla approved these changes Mar 15, 2023

View reviewed changes

M123-dev reviewed Mar 16, 2023

View reviewed changes

github-actions bot added the PR: needs rebase label Mar 16, 2023

Merge branch 'develop' into feat/3712

4110c17

github-actions bot removed the PR: needs rebase label Mar 16, 2023

feat/3712 - new "no camera found" message

16c9595

Impacted file: * `camera_scan_page.dart`: new "no camera found" message

monsieurtanuki mentioned this pull request Mar 17, 2023

Make the carousel feel immersive #3777

Open

fix github actions flutter version

c1fd493

M123-dev approved these changes Mar 17, 2023

View reviewed changes

M123-dev and others added 2 commits March 17, 2023 14:42

Merge branch 'develop' into feat/3712

19e2e3a

feat/3712 - localized "no camera found" message

bd16f2f

Impacted files: * `app_en.arb`: new label for "no camera found" * `app_fr.arb`: new label for "no camera found" * `camera_scan_page.dart`: localized "no camera found" message

github-actions bot added the 🌐 l10n label Mar 17, 2023

monsieurtanuki merged commit 87d9280 into openfoodfacts:develop Mar 17, 2023

github-actions bot mentioned this pull request Mar 17, 2023

chore(develop): release 4.7.0 #3778

Merged

M123-dev mentioned this pull request Mar 18, 2023

Bring back the old scan visor #3783

Closed

monsieurtanuki mentioned this pull request Mar 19, 2023

feat: 3783 - brought back the old visor #3785

Closed

monsieurtanuki mentioned this pull request Mar 29, 2023

fix: 3816 - commented camerawesome #3824

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 3712 - new mlkit and zxing scanners on flutter 3.7 #3767

feat: 3712 - new mlkit and zxing scanners on flutter 3.7 #3767

monsieurtanuki commented Mar 15, 2023

monsieurtanuki commented Mar 15, 2023

M123-dev commented Mar 15, 2023

monsieurtanuki commented Mar 15, 2023

teolemon commented Mar 15, 2023

teolemon commented Mar 15, 2023

adilwahla left a comment

M123-dev left a comment •

edited

Loading

M123-dev Mar 15, 2023

monsieurtanuki Mar 16, 2023

M123-dev Mar 16, 2023

monsieurtanuki Mar 16, 2023

monsieurtanuki Mar 17, 2023

M123-dev Mar 18, 2023

M123-dev Mar 16, 2023

monsieurtanuki Mar 16, 2023

monsieurtanuki Mar 16, 2023

teolemon Mar 17, 2023

monsieurtanuki Mar 17, 2023

M123-dev Mar 16, 2023

monsieurtanuki Mar 16, 2023

M123-dev Mar 16, 2023

monsieurtanuki Mar 16, 2023

M123-dev Mar 17, 2023

monsieurtanuki Mar 17, 2023

monsieurtanuki commented Mar 16, 2023

teolemon commented Mar 17, 2023

monsieurtanuki commented Mar 17, 2023

M123-dev left a comment

monsieurtanuki commented Mar 17, 2023

M123-dev commented Mar 17, 2023

	throw Exception('Please call [init] before!');
	throw Exception('Please call [init] before, no cameras found!');

feat: 3712 - new mlkit and zxing scanners on flutter 3.7 #3767

feat: 3712 - new mlkit and zxing scanners on flutter 3.7 #3767

Conversation

monsieurtanuki commented Mar 15, 2023

What

Screenshot

Fixes bug(s)

monsieurtanuki commented Mar 15, 2023

M123-dev commented Mar 15, 2023

monsieurtanuki commented Mar 15, 2023

teolemon commented Mar 15, 2023

teolemon commented Mar 15, 2023

adilwahla left a comment

Choose a reason for hiding this comment

M123-dev left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monsieurtanuki commented Mar 16, 2023

teolemon commented Mar 17, 2023

monsieurtanuki commented Mar 17, 2023

M123-dev left a comment

Choose a reason for hiding this comment

monsieurtanuki commented Mar 17, 2023

M123-dev commented Mar 17, 2023

M123-dev left a comment •

edited

Loading