juliansteenbakker / mobile_scanner

A universal scanner for Flutter based on MLKit. Uses CameraX on Android and AVFoundation on iOS.
BSD 3-Clause "New" or "Revised" License
756 stars 446 forks source link

new version can use better documentation #1046

Open mauricev opened 1 month ago

mauricev commented 1 month ago

The old version, 4.x, was super simple. The new version is way more complex and is crying out for better documentation.

Consider my use of the old way

Widget displayCameraForReadingQrCode(BuildContext context) {
    final MobileScannerController mobileScannerController =
        MobileScannerController(
      formats: [BarcodeFormat.qrCode],
      detectionSpeed: DetectionSpeed.noDuplicates,
      facing: CameraFacing.back,
      torchEnabled: true,
    );

    return MobileScanner(
      controller: mobileScannerController,
      onDetect: (capture) {
        transferFromQrCodeToTank(context, mobileScannerController, capture);
      },
    );
  }

onDetect is now gone. It's not clear how to revise this to accommodate the new version. Would the function transferFromQrCodeToTank even not work any longer? All it did was stop the processing of incoming codes (after the first one was detected) and the process what was captured.

The new documentation is quite cryptic. It refers to a generic stateful widget that doesn't seem to do anything. It also refers to an example page with no actual examples.

navaronbracke commented 1 month ago

Did you read our updated documentation in the README? The old version had a bug with widget reparenting that caused a lot of issues for users. You should be able to do the exact same thing using a StreamSubscription.

See https://github.com/juliansteenbakker/mobile_scanner?tab=readme-ov-file#usage for updated usage information.

mauricev commented 1 month ago

I am literally commenting on that page. I suggest having someone unfamiliar with plugin rewrite it. Right now, it's just a jumble of words and code with little direction as how to use the plugin.

navaronbracke commented 1 month ago

I see. I guess we can combine the code samples into the full StatefulWidget (currently it is only the relevant parts of said widget)

We can indeed update the link at the bottom to point to https://github.com/juliansteenbakker/mobile_scanner/tree/master/example instead of the README of said example.

As for the switch from version 4.0.0 to 5.0.0, we can probably include a migration guide markdown file and link to that on the main README.

Would that be sufficient?

mauricev commented 1 month ago

It's not clear where the stateful widget in your example comes from. Is it an existing widget in the user's code or does v5 requires its own dedicated stateful widget? It's also not clear why we need to track the lifecycle of said widget. The stream reference seems to be a global variable, and that is not clear.

Your example doesn't demonstrate how to use MobileScanner in v5.

Consider my code using v4 below. My code brings up a dialog with the camera where a user can scan a QR code and they're done. I would show this code and then code that achieves the same functionality using v5, so users can see how the code exactly needs to change to work with v5.

Widget displayCameraForReadingQrCode(BuildContext context) {
final MobileScannerController mobileScannerController =
    MobileScannerController(
  formats: [BarcodeFormat.qrCode],
  detectionSpeed: DetectionSpeed.noDuplicates,
  facing: CameraFacing.back,
  torchEnabled: true,
);

return MobileScanner(
  controller: mobileScannerController,
  onDetect: (capture) {
    transferFromQrCodeToTank(context, mobileScannerController, capture);
  },
);
}

void transferFromQrCodeToTank(BuildContext context2,
  MobileScannerController mobileScannerController, BarcodeCapture capture) {
final List<Barcode> barcodes = capture.barcodes;

mobileScannerController.stop().then((value) {
  // launch and replace navigation screen (can this go back to the screen before this one?)
  String? rawValue = barcodes[0].rawValue;

  if (rawValue != null) {
    String tankDocumentId = rawValue;

    jumpToTheTank(context2, tankDocumentId);
  } else {
    Navigator.pop(context);
  }
});
}
navaronbracke commented 1 month ago

It's not clear where the stateful widget in your example comes from. Is it an existing widget in the user's code

It would always be a widget in the user's code. The only widget that the user does not provide themselves is the MobileScanner widget.

It's also not clear why we need to track the lifecycle of said widget.

This is because if the MobileScanner widget does it, you get problems with widget reparenting or the notorious "Called start when already started" bug. Giving the user control over the lifecycle avoids those problems.

The stream reference seems to be a global variable

Agreed. This will become clearer when I merge the existing code samples into a single StatefulWidget example.

Your example doesn't demonstrate how to use MobileScanner in v5.

The migration guide from v4 to v5 should help with that, as would the updated link to the example app.

Consider my code using v4 below. My code brings up a dialog with the camera where a user can scan a QR code and they're done. I would show this code and then code that achieves the same functionality using v5, so users can see how the code exactly needs to change to work with v5.

The migration guide or samples can definitely help with a use case like this. Although, creating the controller in a local function is an anti-pattern, since it manages the state of the camera. It should be declared in the State class of a StatefulWidget instead.

juliansteenbakker commented 1 month ago

Hi All,

I'd like to step in and clarify a few things. I have reinstated a few breaking changes that were causing some confusion.

The onDetect function and autoStart on the controller are back. I think by not removing these we will keep this package simple and easy. The user can still opt-in into the new lifecycle system by passing null to the onDetect function. Due to this the controller is also not required anymore, so the scanner can be initiated by just these parameters:

          MobileScanner(
            onDetect: _handleBarcode,
          ),

All the other changes will stay, and i think by highlighting these on the readme page, it will be clear enough for the user to adopt to the new syntax.

I have updated the code on the master branch and will release it after some people validated it.

juliansteenbakker commented 4 weeks ago

I have released v5.1.0 which brings back some features. Please see the readme for an overview of the changes.

mauricev commented 3 weeks ago

Thank you for making the program work as before and thanks for letting me know about the anti-pattern.