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

Conversation

@bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Jul 9, 2019

Description

Beginning of refactor of the camera plugin. This only implements the camera interface and shared classes (e.g. camera mixins, CameraChannel & CameraTesting )

This also adds the publish_to: none to the pubspec.yaml until the refactor is finished.

nomnoml

Breaking Changes

The new interface will avoid making many breaking changes, but will change a few things to follow dart style guide and follow other plugin designs:

  • availableCameras() -> CameraController.availableCameras()
  • enum CameraLensDirection -> enum LensDirection
  • CameraController constructor no longer takes a ResolutionPreset. CameraController({CameraDescription description, ResolutionPreset preset}) -> CameraController({CameraDescription description}). Undecided if we should keep ResolutionPreset in the new interface.
  • CameraDescription.name -> CameraDescription.id

Related Issues

flutter/flutter#31225

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

@bparrishMines bparrishMines requested a review from mklim as a code owner July 9, 2019 18:09
@mklim mklim self-assigned this Jul 9, 2019
@bparrishMines bparrishMines changed the title [WIP] [camera] Just the support dart interface [camera] New Camera Dart Interface Jul 9, 2019
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Thanks for breaking this up! It was really quick to review.

This is being set to no_publish so there won't be immediate impact, but the breaking changes parts of this should still go through the Breaking Changes process.

I'm also not sure if setting the plugin to no_publish and breaking the code while this is worked on is a good idea. Ideally we would be changing it in functional pieces that are gradual and easy to revert. The danger with this is that it makes it extremely difficult to actually change the camera plugin at all if needed until this is finished, and reverting any one part of this would also be extremely difficult since it wouldn't be one small piece landing. I think landing this refactor in incremental functional pieces would probably mean throwing up a bunch of dead code and then calling it at the very end like we talked about offline, which has its own drawbacks. @amirh WDYT?

///
/// This class is used as a simple interface that works for Android and iOS.
///
/// Depending on device/API, you may only be able to open one CameraController
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if somebody tried to open multiple ones? Is there any way for them to know whether or not it's safe to?

Copy link
Contributor Author

@bparrishMines bparrishMines Jul 10, 2019

Choose a reason for hiding this comment

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

So this is a good question.

On iOS, there would only be a crash if you called startRunning on two sessions, but not for instantiating them.

However, Android doesn't have a clear answer. It just depends on the device.

Their docs say Your application should only have one Camera object active at a time for a particular hardware camera. But, there are devices where it works and doesn't work: https://stackoverflow.com/questions/12382322/is-it-possible-to-use-front-and-back-camera-at-same-time-in-android.

Maybe this has better clarity?

/// When using iOS, simultaneously calling [start] on two [CameraController]s
/// will throw a [PlatformException].
///
/// When using Android, simultaneously calling [start] on two
/// [CameraController]s may throw a [PlatformException] depending on the
/// hardware resources of the device.

}

/// Starts processing for the camera.
Future<void> start() => configurator.start();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "processing" mean here exactly?

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 explanation is slightly different for each api. But one that could incorporate all of them could be?:

/// Begins the flow of data between the inputs and outputs connected the camera instance.

}

/// Location of the camera on the device.
enum LensDirection { front, back, external }
Copy link
Contributor

Choose a reason for hiding this comment

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

external is a keyword. It looks like it can be used as an identifier if we need it to be, but there's some caveats around it. Is there anything other word that would make sense? unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Android uses external and iOS uses Unspecified. unknown could work as well. I don't really have a preference.


import 'camera_channel.dart';

mixin NativeMethodCallHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Dartdocs on these?

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 developer won't have access to the mixin, only their variables/methods. I created these because many classes needed them on Android and iOS, but I couldn't find a way to make them private and separate their libraries. I may just remove them from the common directory.

@bparrishMines
Copy link
Contributor Author

bparrishMines commented Jul 10, 2019

Thanks for breaking this up! It was really quick to review.

This is being set to no_publish so there won't be immediate impact, but the breaking changes parts of this should still go through the Breaking Changes process.

I'm also not sure if setting the plugin to no_publish and breaking the code while this is worked on is a good idea. Ideally we would be changing it in functional pieces that are gradual and easy to revert. The danger with this is that it makes it extremely difficult to actually change the camera plugin at all if needed until this is finished, and reverting any one part of this would also be extremely difficult since it wouldn't be one small piece landing. I think landing this refactor in incremental functional pieces would probably mean throwing up a bunch of dead code and then calling it at the very end like we talked about offline, which has its own drawbacks. @amirh WDYT?

I added the breaking changes to the CHANGELOG and also kept the don't publish tag. Since the plugin is still pre 1.0, I don't think anything more is necessary. Although, I'll also talk to @amirh to see what he thinks is the best way to do the refactor.

@bparrishMines
Copy link
Contributor Author

Closed in favor of #1832

@bparrishMines bparrishMines deleted the just_the_support_dart_interface branch October 8, 2020 20:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants