Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[camera] Add CameraSelector class to CameraX plugin #6348

Merged
merged 23 commits into from
Sep 26, 2022

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Aug 31, 2022

Adds CameraSelector class wrapper to CameraX plugin.

Part of flutter/flutter#111124.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@camsim99 camsim99 force-pushed the camx_camera_selector branch from e3cf18f to 28a9247 Compare September 13, 2022 20:51
@camsim99 camsim99 marked this pull request as ready for review September 13, 2022 21:51
}

List<CameraInfo> filteredCameraInfos = cameraSelector.filter(cameraInfosForFilter);
final CameraInfoFlutterApiImpl cameraInfoFlutterApiImpl =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this class is used in multiple methods, you could make it a private field in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other method uses CameraSelectorFlutterApiImpl. I don't plan on implementing any other methods in this class for now (flutter/flutter#111124), so I'l leave them as local variables, but can definitely change that if I end up reusing either!

}

@Override
public Long requireLensFacing(@NonNull Long lensDirection) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better pattern would be to change this to a create method with this value as a parameter. And then the Dart class would have an additional normal constructor that sets this field. e.g.

class CameraSelector extends JavaObject {
  CameraSelector({
    int lensFacing,
    super.binaryMessenger,
    super.instanceManager,
  })  : _api = CameraSelectorHostApiImpl(
          binaryMessenger: binaryMessenger,
          instanceManager: instanceManager,
        ),
        super.detached() {
    AndroidCameraXCameraFlutterApis.instance.ensureSetUp();
    _api.createFromInstances(this, lensFacing);
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the create method

  1. Create the builder
  2. set the lens facing.
  3. Build the selector
  4. add the selector to the instancemanager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the pattern you've typically followed for the builder pattern? I think this makes sense but just wondering about the context on moving away from the Android Api.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in most situations, we do want to follow the Java API because it makes our wrapper API more resistant to changes of the Java API. However, the pattern explained above is often considered, unofficially, as the "Dart equivalent" to the Java Builder Pattern. The main reason you never see the builder pattern in Dart is because Dart has language features that makes it obsolete.

So in situations like this, where a superior Dart equivalent pattern exists, we can go with the "Dartier" pattern. It should also probably be considered case by case. As far as I can tell, I don't see any downsides to doing it for CameraSelector.

@github-actions github-actions bot added the p: webview_flutter Edits files for a webview_flutter plugin label Sep 14, 2022
@camsim99 camsim99 removed the p: webview_flutter Edits files for a webview_flutter plugin label Sep 14, 2022
static const int LENS_FACING_BACK = 1;

/// Selector for default front facing camera.
static final Future<CameraSelector> defaultFrontCamera =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is guaranteed to be correct that the Java:

DEFAULT_BACK_CAMERA == new CameraSelector.Builder().requireLensFacing(LENS_FACING_BACK).build()

I think this method could use the attach pattern from the Wrapping Native Apis doc. It would also make it synchronous. e.g.

static CameraSelector defaultFrontCamera({
  BinaryMessenger? binaryMessenger,
  InstanceManager? instanceManager,
}) {
  final CameraSelector cameraSelector = CameraSelector.detached();
  CameraSelectorHostApiImpl(
    binaryMessenger: binaryMessenger,
    instanceManager: instanceManager,
  ).attachDefaultFrontCameraFromInstances(cameraSelector);
  return cameraSelector;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


@Override
public List<Long> filter(@NonNull Long identifier, @NonNull List<Long> cameraInfoIds) {
CameraSelector cameraSelector = (CameraSelector) instanceManager.getInstance(identifier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method implementation is a good sign for me to make improvements to this design. Nothing needs to be done for this comment, I'm just letting you know that I'm going to work on helper methods for InstanceManager to improve working with Lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I agree that would be super useful!

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

private final BinaryMessenger binaryMessenger;
private final InstanceManager instanceManager;

@VisibleForTesting public CameraSelector.Builder cameraSelectorBuilder;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

So having a settable parameter like this is pretty typical for testing the create methods in host apis. However, most classes don't use builders and you will have to test the constructor. It's worth considering using a proxy class to handle all your class instantiations and static methods. For example

class CameraXProxy {
  CameraSelector createCameraSelector(lensFacing) {
   // build and return
  }

  ACameraXClass createACameraXClass() {
    return new ACameraXClass();
  }

  void staticMethodForACameraXClass() {
    ACameraXClass.staticMethod();
  }
}

And then you can add this value to any HostApi that has a create method. And mock it in tests.

You could also create a separate proxy for each host api. (e.g. CameraSelectorProxy).

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022
@auto-submit auto-submit bot merged commit fe3b542 into flutter:main Sep 26, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 26, 2022
adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants