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

Refactor: Extracted camera overlays into their own widget with visibility management #949

Merged
merged 6 commits into from
Jan 15, 2022

Conversation

M123-dev
Copy link
Member

@M123-dev M123-dev commented Jan 13, 2022

What

  • Extracted the camera overlay widgets into their own stateless widget
  • Updated ML Kit camera lifecycle

FYK

The camera plugin has a section about Handling Lifecycle states, for short minimizing and then again opening the app can maybe cause some unexpected behaviors. What do you think we need to have some extra logic for that?

},
child: Stack(
children: <Widget>[
Container(
Copy link
Contributor

Choose a reason for hiding this comment

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

The question I have is why do we need this container ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This container is helpful for a kind of scanner starting animation, the camera preview takes a little time to show up, and until then just a black screen with a scanner logo is shown until the preview animated before it. It's not from me but I'd leave it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks, that's very usedul information, could you please document it

import 'package:smooth_ui_library/widgets/smooth_view_finder.dart';
import 'package:visibility_detector/visibility_detector.dart';

class ScannerOverlay extends StatelessWidget {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you document that this has the qr overlay widget and SmoothProductCarousel

@jasmeet0817
Copy link
Contributor

What

  • Extracted the camera overlay widgets into their own stateless widget
  • Updated ML Kit camera lifecycle

FYK

The camera plugin has a section about Handling Lifecycle states, for short minimizing and then again opening the app can maybe cause some unexpected behaviors. What do you think we need to have some extra logic for that?

Yes I think we should handle those cases

Copy link
Contributor

@jasmeet0817 jasmeet0817 left a comment

Choose a reason for hiding this comment

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

Yess, the code looks great, I have a feeling this is going to fix all the camera issues

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.

Hi @M123-dev! Looks better now.

Comment on lines 35 to 36
final double carouselHeight =
constraints.maxHeight / 1.81; // roughly 55% of the available height
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange to see this 1.81 again, after continuous_scan_page.dart.
I don't know what if refers to. I suggest to use a constant instead.
And maybe to use 0.55 instead of 1.81, that would save the comment // roughly 55% of the available height.

@teolemon teolemon changed the title Refactor: Extracted camera overlays into their own widget with visbility mangement Refactor: Extracted camera overlays into their own widget with visibility management Jan 14, 2022
@teolemon teolemon removed the scan label Jan 15, 2022
@M123-dev M123-dev linked an issue Jan 15, 2022 that may be closed by this pull request
1 task
@github-actions github-actions bot added the 🤳🥫 Scan We need to be able to scan on low-end, old devices, even with a bad camera, connexion… label Jan 15, 2022
@sentry-io
Copy link

sentry-io bot commented Jan 15, 2022

Sentry issue: SMOOTHIE-S7

@M123-dev
Copy link
Member Author

Thanks for your review @monsieurtanuki @jasmeet0817, I added some further documentation and the state management and maybe fixed a sentry but, I'm going to merge so that we can test this

@teolemon teolemon merged commit e30ab6b into develop Jan 15, 2022
@teolemon teolemon deleted the camera-refactoring branch January 15, 2022 15:30
stopCamera: _stopImageStream,
model: _model,
scannerWidget: Transform.scale(
scale: scale,
Copy link
Contributor

Choose a reason for hiding this comment

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

this scale causes camera to not take up the whole screen, is it really needed ?

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…
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ML Kit scanner is not asking for camera permission
4 participants