feat: On iOS, the camera was never stopped after being resumed in some edge cases

[g123k]

Woo that one, was tricky another time.

So let me explain everything:

• By default the camera is opened when its build method is called.
  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

[monsieurtanuki]

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

[g123k]

Yes it's only with MLKit, because we use the autostart feature.
Maybe we could simplify the implementation by doing it manually, but the previous implementation was using this mindset and it wasn't better.

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 build method is called as needed
✅ When we open a new screen/change the tab, the process to pause the camera is OK
✅ When we reopen the app and we are in the camera, there is no issue
🔴 However, if we are on a screen on top of it (eg: a product page), the build method is called, because the Widget is in the tree forcing the camera to autostart.

But at this point, we have different scenarios to catch:

• When the app is minimized/reopened for the first time, the build is called multiple times (at least 5), which is enough to determine if it's visible or not
• However, when we do the same operation a second time (minimize -> reopen -> minimize -> reopen), the build method is only called once. That's why I have to rely on Timers and not the redraw process.

And that's for this second issue, that I have to implement all this stuff.
And furthermore, in debug mode, it can't be reproduced! So it was really hard to discover it.

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 😞

[monsieurtanuki]

Yes it's only with MLKit, because we use the autostart feature. Maybe we could simplify the implementation by doing it manually, but the previous implementation was using this mindset and it wasn't better.

Would autostart: false work, as for ZXing?

that's basically the issue of dealing with native code, where we don't have the full access on Dart

I would then suggest a new issue on mobile_scanner, in order to have full access and to let the whole open source community enjoy the fixes.

[g123k]

Yes it's only with MLKit, because we use the autostart feature. Maybe we could simplify the implementation by doing it manually, but the previous implementation was using this mindset and it wasn't better.

Would autostart: false work, as for ZXing?

Yes, it would. But as explained, with the previous implementation (with the fork), we all this can of worms.

that's basically the issue of dealing with native code, where we don't have the full access on Dart

I would then suggest a new issue on mobile_scanner, in order to have full access and to let the whole open source community enjoy the fixes.

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.
With Dart, where we have this messenger between, we have to deal with that kind of ugly stuff unfortunately.

[monsieurtanuki]

@g123k I cannot dedicate too much brain bandwith to that PR. This is what I suggest (please pick your favorite):

1. refactor that problematic and less and less relevant navigation bottom bar, that caused issues for more than a year, including that camera open/close issue
2. find a different clean way to fix the issue - it's hard to believe that all flutter apps that use mobile_scanner have the same issue
3. add an explicit comment in the code stating that it is an ugly and almost impossible to maintain code and linking to the current PR - I'll have no problem approving it

[g123k]

@g123k I cannot dedicate too much brain bandwith to that PR. This is what I suggest (please pick your favorite):

1. refactor that problematic and less and less relevant navigation bottom bar, that caused issues for more than a year, including that camera open/close issue
2. find a different clean way to fix the issue - it's hard to believe that all flutter apps that use mobile_scanner have the same issue
3. add an explicit comment in the code stating that it is an ugly and almost impossible to maintain code and linking to the current PR - I'll have no problem approving it

For the first point, I know there are many discussions about this. So it's out of scope for now.
For the second one, I think we represent a really few percent of the user base, as using a camera as a homepage isn't the most common pattern. Furthermore, as the maintainer of mobile_scanner has inconsistent availabilities, I'm not sure we will receive an answer anytime soon.

So I will take the third road, and tonight I was able to reproduce it on Android.

[codecov-commenter]

Codecov Report

Merging #4292 (43d3840) into develop (965ee71) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             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

[monsieurtanuki]

Thank you @g123k!

[g123k]

I'm sorry for this ugly code, let's hope we can improve it

[monsieurtanuki]

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.
Probably with a different management of the navigation bar and its 3 widgets open at the same time. By letting flutter cleanly open and close the camera when we open and close the widget.
We'll work on it next time we have a problem with the open camera or the navigation bar, whichever comes first.
But if the bug is fixed now, that's good enough for me for the moment!