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

placeholderBuilder and errorBuilder callbacks for MobileScanner have an extra argument which serves no purpose. #1256

Open
Tracked by #1225
remonke opened this issue Dec 8, 2024 · 2 comments
Assignees

Comments

@remonke
Copy link

remonke commented Dec 8, 2024

This is what the build method of the MobileScanner widget looks like:

@override
Widget build(BuildContext context) {
  return ValueListenableBuilder<MobileScannerState>(
    valueListenable: controller,
    builder: (BuildContext context, MobileScannerState value, Widget? child) {
      if (!value.isInitialized) {
        const Widget defaultPlaceholder = ColoredBox(color: Colors.black);

        return widget.placeholderBuilder?.call(context, child) ??
            defaultPlaceholder;
      }

      final MobileScannerException? error = value.error;

      if (error != null) {
        const Widget defaultError = ColoredBox(
          color: Colors.black,
          child: Center(child: Icon(Icons.error, color: Colors.white)),
        );

        return widget.errorBuilder?.call(context, error, child) ??
            defaultError;
      }

      return LayoutBuilder(
        builder: (context, constraints) {
          _maybeUpdateScanWindow(value, constraints);

          final Widget? overlay =
              widget.overlayBuilder?.call(context, constraints);
          final Size cameraPreviewSize = value.size;

          final Widget scannerWidget = ClipRect(
            child: SizedBox.fromSize(
              size: constraints.biggest,
              child: FittedBox(
                fit: widget.fit,
                child: SizedBox(
                  width: cameraPreviewSize.width,
                  height: cameraPreviewSize.height,
                  child: MobileScannerPlatform.instance.buildCameraView(),
                ),
              ),
            ),
          );

          if (overlay == null) {
            return scannerWidget;
          }

          return Stack(
            alignment: Alignment.center,
            children: <Widget>[
              scannerWidget,
              overlay,
            ],
          );
        },
      );
    },
  );
}

Since child is not passed to the ValueListenableBuilder, it will always be null. There is no point in making placeholderBuilder and errorBuilder callbacks take an extra argument.

@navaronbracke
Copy link
Collaborator

navaronbracke commented Dec 9, 2024

Good catch! Should we provide the child to the ValueListenable builder? Or should we omit the argument?

I would go with adding the child to the value listenable builder, since then you can optimize rebuilds for that child, if desired.
The child argument of ValueListenableBuilder is there for performance reasons, so I think we should use it.

@navaronbracke navaronbracke self-assigned this Dec 9, 2024
@remonke
Copy link
Author

remonke commented Dec 10, 2024

I think it's the best option as it wouldn't be a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants