-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
feat: On iOS, the camera was never stopped after being resumed in some edge cases #4292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @g123k!
I would have minor comments but let me focus on the most important first:
- you coded for MLKit - doesn't that impact ZXing too? You could say that the bug was so far only detected on iOS that works only with MLKit, but the bug may be present in android too
- that's really a pity to see all that hardly maintainable tap dancing around the camera, and I would suggest to refactor the whole bottom navigation bar thing now that the navigation bar is only present in limited pages
Yes it's only with MLKit, because we use the I will try to explain to you the behavior of the error here, but without any drawing, it's a bit complicated: ✅ When the camera is visible, the But at this point, we have different scenarios to catch:
And that's for this second issue, that I have to implement all this stuff. I know the code is ugly, but that's basically the issue of dealing with native code, where we don't have the full access on Dart 😞 |
Would
I would then suggest a new issue on |
Yes, it would. But as explained, with the previous implementation (with the fork), we all this can of worms.
Basically, we can't, what I mean by that, is if we could be directly in the native code, which would be supplier to catch. |
@g123k I cannot dedicate too much brain bandwith to that PR. This is what I suggest (please pick your favorite):
|
For the first point, I know there are many discussions about this. So it's out of scope for now. So I will take the third road, and tonight I was able to reproduce it on Android. |
Codecov Report
@@ Coverage Diff @@
## develop #4292 +/- ##
===========================================
- Coverage 10.87% 10.80% -0.08%
===========================================
Files 285 287 +2
Lines 14172 14202 +30
===========================================
- Hits 1541 1534 -7
- Misses 12631 12668 +37 see 32 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @g123k!
I'm sorry for this ugly code, let's hope we can improve it |
The best way to improve it would be to get rid of it. |
Woo that one, was tricky another time.
So let me explain everything:
Since this Widget is always in the tree, it's restarted even if it's not even in foreground
That's why, my initial code was checking if the app was still visible after being resumed.
But the check was not strong enough, not on the first resume, but the second one!
By tweaking a little bit my implementation, I am not 100% sure, that it will force close the camera, if it's not visible.
Here is a test scenario (that's working now): https://youtu.be/mCUF5mSw83o