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

fix: 3674 - simplified the barcode scan with native solution #3694

Closed
wants to merge 2 commits into from

Conversation

monsieurtanuki
Copy link
Contributor

@monsieurtanuki monsieurtanuki commented Feb 11, 2023

Deleted files:

  • tons of them

New file:

  • smooth_qr_view.dart: QR View.

Impacted files:

  • app_test.dart: removed reference to specific scanner
  • basic_test.dart: removed reference to specific scanner
  • camera_helper.dart: removed now useless code
  • camera_scan_page.dart: HIGHLY simplified the code with the use of new class SmoothQRView
  • constant_icons.dart: added a "camera flip" icon
  • labeler.yml: removed reference to now removed files
  • main.dart: removed reference to specific scanner
  • main_fdroid.dart: removed reference to specific scanner
  • main_google_play.dart: removed reference to specific scanner
  • main_ios.dart: removed reference to specific scanner
  • pubspec.lock: wtf
  • pubspec.yaml: added qr_code_scanner; removed reference to specific camera and specific scanner
  • scan_page.dart: simplified the display - camera on top, carousel on bottom
  • user_preferences_dev_mode.dart: removed reference to camera post frame callback duration and scan area settings
  • user_preferences_settings.dart: removed reference to file/byte camera mode computation setting

What

  • The goal is to get rid of the flutter camera package (which does not give entire satisfaction) for the barcode scanning.
  • The solution here is to use native packages.
  • The existing flutter package with native code I found (qr_code_scanner) is based on ZXing.
  • Very interesting side-effects:
    • the app finally works on my smartphone
    • the code is hugely simplified
    • the code runs much faster - at least for developers waiting much too long during the flutter run
  • The UI is slightly different: the camera preview is now restricted to the top part of the screen.
  • If later we need to implement MLKit as well, we need to implement a solution similar to whatever qr_code_scanner provides: a single widget, with camera based on native libraries.
  • Of course this PR should be tested on a large number of devices.

Screenshot

dark mode light mode
Screenshot_2023-02-11-15-44-35 Screenshot_2023-02-11-15-43-43

Fixes bug(s)

Part of

Deleted files:
* tons of them

New file:
* `smooth_qr_view.dart`: QR View.

Impacted files:
* `app_test.dart`: removed reference to specific scanner
* `basic_test.dart`: removed reference to specific scanner
* `camera_helper.dart`: removed now useless code
* `camera_scan_page.dart`: HIGHLY simplified the code with the use of new class `SmoothQRView`
* `constant_icons.dart`: added a "camera flip" icon
* `labeler.yml`: removed reference to now removed files
* `main.dart`: removed reference to specific scanner
* `main_fdroid.dart`: removed reference to specific scanner
* `main_google_play.dart`: removed reference to specific scanner
* `main_ios.dart`: removed reference to specific scanner
* `pubspec.lock`: wtf
* `pubspec.yaml`: added `qr_code_scanner`; removed reference to specific camera and specific scanner
* `scan_page.dart`: simplified the display - camera on top, carousel on bottom
* `user_preferences_dev_mode.dart`: removed reference to camera post frame callback duration and scan area settings
* `user_preferences_settings.dart`: removed reference to file/byte camera mode computation setting
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner February 11, 2023 14:56
@github-actions github-actions bot added dependencies GitHub 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… labels Feb 11, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3694 (07f41af) into develop (f2ede98) will increase coverage by 0.44%.
The diff coverage is 1.14%.

@@            Coverage Diff             @@
##           develop   #3694      +/-   ##
==========================================
+ Coverage     9.34%   9.79%   +0.44%     
==========================================
  Files          272     264       -8     
  Lines        13722   13010     -712     
==========================================
- Hits          1283    1274       -9     
+ Misses       12439   11736     -703     
Impacted Files Coverage Δ
packages/smooth_app/lib/helpers/camera_helper.dart 0.00% <0.00%> (ø)
...b/pages/preferences/user_preferences_dev_mode.dart 0.00% <ø> (ø)
...b/pages/preferences/user_preferences_settings.dart 10.30% <ø> (+0.46%) ⬆️
...es/smooth_app/lib/pages/scan/camera_scan_page.dart 2.56% <0.00%> (+2.11%) ⬆️
packages/smooth_app/lib/pages/scan/scan_page.dart 3.07% <0.00%> (-1.83%) ⬇️
...ages/smooth_app/lib/pages/scan/smooth_qr_view.dart 0.00% <0.00%> (ø)
packages/smooth_app/lib/themes/constant_icons.dart 53.84% <0.00%> (-9.80%) ⬇️
packages/smooth_app/lib/main.dart 14.16% <25.00%> (+0.45%) ⬆️

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

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

I'd really like to have a thorough discussion about this, it's the 3rd time we change scanner, but now we're in production. I've added elements to the Barcode Scanning document.
https://docs.google.com/document/d/1nxjyZD8NKM28w1J5LPhMybIRiw1t-31gB3mq39XtFxw/edit#
Can we schedule a discussion sometime this week ?

@monsieurtanuki
Copy link
Contributor Author

@teolemon Please set an appointment and tell me a day in advance.

Additional thoughts...

The previous MLKit implementation:

  • was too embedded in the Smoothie code; it should be a black box from Smoothie's perspective, just a widget with a callback
  • was partly hidden in another github repository; as a dev with limited understanding of github I have no idea where the code is. As a consequence, I have no way to challenge it or to work on it.
  • was not stable and provoked tons of sentry logs, which made sentry reports useless
  • never worked on my smartphone, and more generally was notoriously problematic with low-end devices because of memory

To me, those are only nice-to-haves:

  • immersive fullscreen
  • best decoder
  • in all directions

My recent work on the barcode scanner provided me with food for thought.
The most practical way would be to build our own "scan barcode" widgets, away from Smoothie:

  • based on a native, camera or cameraX visual layer
  • calling either zxing or mlkit
  • and in the end a callback for each new barcode

With those widgets we'd be then able to say something like that in Smoothie:

  • for ios, native and mlkit
  • for google play, native (then cameraX) and mlkit
  • for fdroid, native (then cameraX) and zxing

If I started this PR it was because:

  • there were recent pending issues (a dozen would be closed, including several P0s)
  • I had no idea how to fix MLKit
  • I had nothing to lose as it never worked on my smartphone anyway

If someone else could fix the P0 issues I referred to while keeping MLKit I'd be more than happy.

@natrius
Copy link

natrius commented Feb 24, 2023

Because its mentioned above - i installed 4.5.0 now and tried it: #3427 is not fixed unfortunately. The camera seems to refocus every 2-3 seconds but thats it. Thanks for your efforts!

@monsieurtanuki
Copy link
Contributor Author

@natrius This PR was not merged eventually.
Thank you for your comment: we definitely need to fix that barcode scan / camera preview issue.

@M123-dev
Copy link
Member

Because its mentioned above - i installed 4.5.0 now and tried it: #3427 is not fixed unfortunately. The camera seems to refocus every 2-3 seconds but thats it. Thanks for your efforts!

What I am wondering right now, is this refocusing a "big" problem, because we use the same camera implementation on ever store only the scanning engine is changed

@monsieurtanuki
Copy link
Contributor Author

@M123-dev That's the whole idea: being able to split in OOP a generic widget, camera preview and barcode scan algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment