-
-
Notifications
You must be signed in to change notification settings - Fork 529
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: Ignore additional calls to start() when the controller is starting #1086
fix: Ignore additional calls to start() when the controller is starting #1086
Conversation
Nice! You probably need to update the examples folder as well, as I had to now manually call start() after setting the controller in initState(). |
@simplenotezy Ah right, I'll probably have to tweak the automatic start in the samples. Just to be clear, you want automatic start to be enabled for the samples? Or not? |
Sounds good! After trying your version, I needed to call start manually. Otherwise the example code would not work. class _TicketScannerScreenState extends State<TicketScannerScreen> {
late MobileScannerController controller;
@override
void initState() {
super.initState();
controller = MobileScannerController(
formats: const [BarcodeFormat.qrCode],
);
+ controller.start();
}
@override
Future<void> dispose() async {
super.dispose();
await controller.dispose();
}
@override
Widget build(BuildContext context) {
final scanWindow = Rect.fromCenter(
center: MediaQuery.sizeOf(context).center(Offset.zero),
width: 400,
height: 200,
); |
Hmm, what device is this on? Do you see anything in XCode / Logcat / browser logs? |
And it works initially. If I start a new debug session it will work just fine. If I then hot reload/restart the app, and go to the route, it won't work. Which smells like something is off in the underlying APIs. |
Hmm, it's precisely that error that we should ignore when already starting. Is Also, do you have a stack trace? I can test with the sample above to see where it occurs, though. |
The error is this: when the widget is disposed*, the underlying service is not being stopped. When you restart your application, it is technically still running, so when you go to the route that starts the controller, it won't work. |
Here's a video showing the issue: RPReplay_Final1718473315.mov |
Could you clarify that? We should be disposing the underlying camera. Perhaps there is a case where this isn't true? |
@navaronbracke Not sure how I can clarify further apart from the video I sent above. The code won't work if you use flutters hot restart functionality (note: hot reload works, but not hot restart) |
So this exception only happens when you press I'll have to specifically address the use case for hot reload then, perhaps by just short circuiting a return value instead of an error, if we do not have hot reload callbacks. Another idea would be to see if stopping the controller in |
4fead48
to
a7e67ba
Compare
analysis_options.yaml
Outdated
- unnecessary_library_directive | ||
- prefer_single_quotes |
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.
We had one violation for this rule. I do personally prefer single quotes, so I turned this on.
@@ -25,7 +21,7 @@ class MobileScannerPermissions { | |||
} | |||
|
|||
interface ResultCallback { | |||
fun onResult(errorCode: String?, errorDescription: String?) | |||
fun onResult(errorCode: String?) |
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.
The second argument was unused, since we return a bool to the interface
@@ -28,7 +29,7 @@ class MobileScannerHandler( | |||
|
|||
private val analyzeImageErrorCallback: AnalyzerErrorCallback = { | |||
Handler(Looper.getMainLooper()).post { | |||
analyzerResult?.error("MobileScanner", it, null) | |||
analyzerResult?.error(MobileScannerErrorCodes.BARCODE_ERROR, it, null) |
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.
To be consistent with the results from the scanning stream, I used the same barcode error constant
@@ -103,21 +104,21 @@ class MobileScannerHandler( | |||
|
|||
@ExperimentalGetImage | |||
override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { | |||
if (mobileScanner == null) { |
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.
This check isn't needed. The scanner is set up when attached to the activity
when (call.method) { | ||
"state" -> result.success(permissions.hasCameraPermission(activity)) | ||
"request" -> permissions.requestPermission( | ||
activity, | ||
addPermissionListener, | ||
object: MobileScannerPermissions.ResultCallback { | ||
override fun onResult(errorCode: String?, errorDescription: String?) { | ||
override fun onResult(errorCode: String?) { |
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.
Plumbing through the new error codes
ios/Classes/MobileScanner.swift
Outdated
@@ -127,7 +124,6 @@ public class MobileScanner: NSObject, AVCaptureVideoDataOutputSampleBufferDelega | |||
/// Gets called when a new image is added to the buffer | |||
public func captureOutput(_ output: AVCaptureOutput, didOutput sampleBuffer: CMSampleBuffer, from connection: AVCaptureConnection) { | |||
guard let imageBuffer = CMSampleBufferGetImageBuffer(sampleBuffer) else { | |||
print("Failed to get image buffer from sample buffer.") |
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.
The print isn't helpful to users :)
ios/Classes/MobileScanner.swift
Outdated
@@ -25,9 +25,6 @@ public class MobileScanner: NSObject, AVCaptureVideoDataOutputSampleBufferDelega | |||
/// Barcode scanner for results | |||
var scanner = BarcodeScanner.barcodeScanner() | |||
|
|||
/// Return image buffer with the Barcode event | |||
var returnImage: Bool = false |
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.
This field was moved to fix a bug
e48a689
to
b50b565
Compare
/// | ||
/// This exception type is only used internally, | ||
/// and is not part of the public API. | ||
class PermissionRequestPendingException implements Exception {} |
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.
This class is no longer used, as the workaround is removed.
return nil | ||
} | ||
} else { | ||
if (error != nil) { |
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.
Small refactor to use early returns for readability + plumbing of the new error codes
barcodeMap.add(barcode.data) | ||
} | ||
} else { | ||
if (scanWindow == null) { |
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.
Refactoring with early returns
3fdae25
to
4d92d5d
Compare
As I've found multiple github links related to this issue and was now able to solve it thanks to the sequence of your posts, trying to sum everything up here, if that may be beneficial for further debuggers: @navaronbracke the behaviour @simplenotezy presents in his video (thanks for that @simplenotezy) happens for me in the exact same way! Flutter 3.24.2 • channel stable
Tools when it happens, the screen showing up is simply plain white (Scaffold's background screen, which in @simplenotezy most likely is set to black), with nothing inside it. That is, visually. in the widget tree, all the widgets are built normally, which is somewhat weird (The QR-Code Scanner / Camera simply does not show up and the entire screen stays in the single color of the Scaffold background). The pace at which we open and close the screen multiple times quickly or slowly one after the other (waiting a few seconds before re-opening the screen after closing it) did not seem to have an impact on the outcome. Regarding the controller, I copy-pasted your methods provided in the example https://github.com/juliansteenbakker/mobile_scanner/blob/master/example/lib/barcode_scanner_controller.dart. Interestingly, when we tried to never call
comepletely fixed the issue for me (@simplenotezy, are you sure you've set that to But your proposed additional permission check still makes absolute sense to me. Without it, when the mobile scanner is opened for the first time and permission was not granted, there's another problem: The app keeps endlessly running into the error:
until the permission dialog is dismissed. To act against that, I'm additionally adding your permission check within initState too, even though disabling the Related to your proposed permission check, if I may suggest:
@navaronbracke you may want to change the example's lifecycle handler init check, as there is no |
That is a new getter that I added in 1ab7f5a, which does exactly this check. |
@@ -24,6 +24,12 @@ class BarcodeHandler(binaryMessenger: BinaryMessenger) : EventChannel.StreamHand | |||
} | |||
} | |||
|
|||
fun publishError(errorCode: String, errorMessage: String, errorDetails: Any?) { |
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.
Like publishEvent
above, but for result.error()
"name" to "error", | ||
"data" to error, | ||
)) | ||
barcodeHandler.publishError(MobileScannerErrorCodes.BARCODE_ERROR, error, null) |
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.
This is an actual error event now.
@@ -42,7 +42,10 @@ public class MobileScannerPlugin: NSObject, FlutterPlugin { | |||
init(barcodeHandler: BarcodeHandler, registry: FlutterTextureRegistry) { | |||
self.mobileScanner = MobileScanner(registry: registry, mobileScannerCallback: { barcodes, error, image in | |||
if error != nil { | |||
barcodeHandler.publishEvent(["name": "error", "data": error!.localizedDescription]) | |||
barcodeHandler.publishError( |
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.
This is an actual error event now
/// The JS static interop class for the Result class in the ZXing library. | ||
/// | ||
/// See also: https://github.com/zxing-js/library/blob/master/src/core/Exception.ts | ||
@JS('ZXing.Exception') |
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.
ZXing had an error type, so I prefer to use it, so we can forward the message.
I have started testing this patch and it seems to fix the underlying issue. I still need to test it on iOS and I'm having a bit of trouble testing MacOS hot reload/hot restart due to a tool bug, |
.buildlog/ | ||
.history | ||
.svn/ | ||
.swiftpm/ |
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.
This was added by a project migrator when running on the master branch of Flutter. This is probably needed for SwiftPM stuff.
@@ -21,7 +21,7 @@ | |||
<meta name="description" content="Demonstrates how to use the mobile_scanner plugin."> | |||
|
|||
<!-- iOS meta tags & icons --> | |||
<meta name="apple-mobile-web-app-capable" content="yes"> | |||
<meta name="mobile-web-app-capable" content="yes"> |
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.
The old tag is deprecated
), | ||
); | ||
// Skip the event if no code was detected. | ||
if (error != null && error.message != kNoCodeDetectedErrorMessage) { |
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.
When introducing the forwarding of the errors on web, this was apparently breaking our usage. Added a check to fix it.
@@ -10,5 +10,7 @@ | |||
<true/> | |||
<key>com.apple.security.network.server</key> | |||
<true/> | |||
<key>com.apple.security.files.user-selected.read-only</key> |
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.
Not having this broke the example app on MacOS for analyze image.
This was documented in image_picker
@@ -6,4 +6,8 @@ class AppDelegate: FlutterAppDelegate { | |||
override func applicationShouldTerminateAfterLastWindowClosed(_ sender: NSApplication) -> Bool { | |||
return true | |||
} | |||
|
|||
override func applicationSupportsSecureRestorableState(_ app: NSApplication) -> Bool { |
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.
This fixes a warning for MacOS
@@ -488,14 +488,14 @@ | |||
CODE_SIGN_IDENTITY = "Apple Development"; | |||
CODE_SIGN_STYLE = Automatic; | |||
CURRENT_PROJECT_VERSION = "$(FLUTTER_BUILD_NUMBER)"; | |||
DEVELOPMENT_TEAM = 75Y2P2WSQQ; | |||
DEVELOPMENT_TEAM = ""; |
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.
When trying to test the iOS example app, XCode kept complaining about the development team & bundle identifier.
Presumably the old development team was Julian's personal team or something.
Likewise, the bundle identifier was already registered (which really should not be the case for an exampel app, that isn't intended for publishing)
I updated the bundle identifier to something that is not registered, by adding the -example
string at the end.
I changed the development team to None
in XCode, which is better than a team that isn't yours, in my opinion.
This fix was now verified for iOS, MacOS, Android and the web. With this fix, during hot restart, the app restarts fully (as expected) and with hot reload the camera continues to work as if the hot reload never happened (since hot reload is only for Dart code on the client, not the plugin) This fix will be available for the next release, which I plan to release later today. |
Fixes: #1078
Fixes: #502