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: Camera fixes #2409

Conversation

g123k
Copy link
Collaborator

@g123k g123k commented Jun 28, 2022

Will fix #2394 #2399 #2398

import 'package:smooth_app/pages/scan/camera_controller.dart';

/// Forked Widget from the [camera] library, but with a simpler Widget
class CameraStreamPreview extends StatelessWidget {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class CameraStreamPreview extends StatelessWidget {
class SmoothCameraStreamPreview extends StatelessWidget {

So that it's clear that we have it locally

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will change that @M123-dev.
The PR is right now in draft to let @AshAman999 test if a fix is ok

Copy link
Member

Choose a reason for hiding this comment

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

The flash issues seemed to have resolved but the camera was going blank after I take a picture and had to restart my app to start it again, tried it multiple times faced the same issues (for ingredients picture update here in this case )
Android 11,

screen-20220628-222230.mp4

@g123k
Copy link
Collaborator Author

g123k commented Jun 28, 2022

Many changes are in place in my last commit.
The code is still not ready for a PR (so please don't review it), but could you test and let me know if it's better?

cc @AshAman999 @stephanegigandet @teolemon
EDIT: Link to a test apk -> https://appdistribution.firebase.dev/i/0b903c4d20f393a2

@g123k g123k force-pushed the camera_fixes_episode_i_dont_even_remember_the_number branch 2 times, most recently from 0aab075 to 54128e5 Compare June 28, 2022 17:30
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2022

Codecov Report

Merging #2409 (98a8f9c) into develop (2ea0da3) will decrease coverage by 1.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2409      +/-   ##
==========================================
- Coverage     8.86%   7.49%   -1.38%     
==========================================
  Files          161     206      +45     
  Lines         6623    9837    +3214     
==========================================
+ Hits           587     737     +150     
- Misses        6036    9100    +3064     
Impacted Files Coverage Δ
...kages/smooth_app/lib/widgets/attribute_button.dart 0.00% <0.00%> (-92.00%) ⬇️
...s/smooth_app/lib/data_models/user_preferences.dart 8.73% <0.00%> (-23.57%) ⬇️
packages/smooth_app/lib/themes/smooth_theme.dart 60.00% <0.00%> (-22.98%) ⬇️
...p/lib/generic_lib/dialogs/smooth_alert_dialog.dart 15.29% <0.00%> (-18.92%) ⬇️
...mooth_app/lib/data_models/product_preferences.dart 24.65% <0.00%> (-6.78%) ⬇️
packages/smooth_app/lib/main.dart 14.16% <0.00%> (-3.73%) ⬇️
.../smooth_app/lib/pages/onboarding/welcome_page.dart 0.00% <0.00%> (-3.13%) ⬇️
.../smooth_app/lib/pages/onboarding/scan_example.dart 0.00% <0.00%> (-2.28%) ⬇️
...p/lib/pages/onboarding/consent_analytics_page.dart 0.00% <0.00%> (-1.57%) ⬇️
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (-0.88%) ⬇️
... and 206 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d46da9...98a8f9c. Read the comment docs.

@AshAman999
Copy link
Member

Tried the build just now, still facing the same issues

  • as soon as I start the app the flash automatically starts
  • after a few seconds the flash stops
  • if I go to product edition try image update there, and come back to scan page there it seems like the flash now turns on and remains on

[Android 11]

@g123k
Copy link
Collaborator Author

g123k commented Jun 29, 2022

Tried the build just now, still facing the same issues

  • as soon as I start the app the flash automatically starts
  • after a few seconds the flash stops
  • if I go to product edition try image update there, and come back to scan page there it seems like the flash now turns on and remains on

[Android 11]

Just to be sure, did you do a flutter pub upgrade before? What exactly is your device?

@monsieurtanuki
Copy link
Contributor

Some suggestions:

  • @g123k That's at least the second time I see you mentioning flutter pub upgrade. Just to be sure: wouldn't it be safer to upgrade the pubspec.yaml to the latest versions, so that everybody is "forced" to share the same minimum versions (that would be temporarily the latest ones)
  • @AshAman999 Assuming that you're able to test the flash side-effect on your device, could you start a new project from scratch with the same camera packages as Smoothie, and see if a minimal test also displays the flash? Either the problem is in the camera packages or in the way we're using them specifically in Smoothie.

Sorry if my suggestions are irrelevant; I didn't follow all the camera issue threads.

@g123k
Copy link
Collaborator Author

g123k commented Jun 29, 2022

Some suggestions:

  • @g123k That's at least the second time I see you mentioning flutter pub upgrade. Just to be sure: wouldn't it be safer to upgrade the pubspec.yaml to the latest versions, so that everybody is "forced" to share the same minimum versions (that would be temporarily the latest ones)
  • @AshAman999 Assuming that you're able to test the flash side-effect on your device, could you start a new project from scratch with the same camera packages as Smoothie, and see if a minimal test also displays the flash? Either the problem is in the camera packages or in the way we're using them specifically in Smoothie.

Sorry if my suggestions are irrelevant; I didn't follow all the camera issue threads.

For the flutter upgrade thing, it's just a precaution, as the pubspeck.lock is updated

Just to be sure @AshAman999, could you test with this APK: https://appdistribution.firebase.dev/i/0b903c4d20f393a2

@AshAman999
Copy link
Member

Looks good this time,
Had no issues at all, everything working just good and smooth
I guess we can re-open this PR
👍

@g123k
Copy link
Collaborator Author

g123k commented Jun 30, 2022

As feedbacks seem positive, I will finalize the PR

@teolemon teolemon added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Jun 30, 2022
@teolemon
Copy link
Member

It seems that @stephanegigandet had regressions on Samsung devices ?

@g123k
Copy link
Collaborator Author

g123k commented Jun 30, 2022

It seems that @stephanegigandet had regressions on Samsung devices ?

No finally it was OK
https://openfoodfacts.slack.com/archives/CSV2748EB/p1656507414671039?thread_ts=1656437499.630669&cid=CSV2748EB

@g123k g123k force-pushed the camera_fixes_episode_i_dont_even_remember_the_number branch from 54128e5 to b32e385 Compare July 2, 2022 13:20
@g123k g123k changed the title fix: [WIP] Camera fixes fix: Camera fixes Jul 2, 2022
@g123k
Copy link
Collaborator Author

g123k commented Jul 2, 2022

It seems that these changes are OK.
I have tweaked a little bit the initial implementation to enhance the code, but nothing breaking.

Will it be our last episode of the scanner series? I hope so…

@g123k g123k marked this pull request as ready for review July 2, 2022 13:34
@g123k g123k requested a review from a team as a code owner July 2, 2022 13:34
@teolemon
Copy link
Member

teolemon commented Jul 4, 2022

ok, merging. Thank you @g123k 🙏

@teolemon teolemon merged commit eef4c45 into openfoodfacts:develop Jul 4, 2022
@g123k g123k deleted the camera_fixes_episode_i_dont_even_remember_the_number branch July 4, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion…
6 participants