[Camera] Made CameraController.isDisposed publicly accessible. Added unit Tests for the new implementation. #3193

Sempakonka · 2020-10-23T13:31:59Z

Description

Currently when you call CameraController.Dispose() there is no way of knowing whether you disposed that CameraController.
With this PR, the user can check if that CameraController has been disposed, by getting CameraController.isDisposed.

Since the current test/camera_test.dart file contains test for the CameraController class located in the lib/new/src/camera_controller.dart I have moved it into the test/new/camera_test.dart sub folder.

I have added a new test/camera_test.dart file which contains the unit tests covering the new isDisposed property which was added to the CameraController class located in the current lib/camera.dart implementation.

Additionally I have added unit tests to the test/new/camera_test.dart file to ensure the isDisposed property (already) implemented in the CameraController class located in the lib/new/src/camera_controller.dart file is working properly.

Related Issues

Fixes flutter/flutter#51630

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

packages/camera/test/camera_test.dart

packages/camera/test/new/camera_test.dart

google-cla · 2020-11-05T08:23:41Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-11-05T08:47:03Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-11-05T08:48:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mvanbeusekom · 2020-11-05T08:49:14Z

@googlebot I consent.

dnfield · 2020-11-05T17:02:28Z

packages/camera/lib/camera.dart

+  /// True after [CameraController.dispose] has completed successfully.
+  bool get isDisposed => _isDisposed;


we typically expose things like this guarded by asserts, so they're only valid in debug mode.

This is to discourage people from adding checks like this to release mode code, where it should never really be necessary.

IOW, making it easier to assert program correctness is great. Allowing someone to bog down code with 1000s of small checks that really should only be done during development is not as preferable.

thank you for providing feedback. I agree with the idea on only allowing this during during development. However I am not really sure how to "expose things like this guarded by asserts"?

Does this imply we don't expose a property but rather add assert statements to the methods? I feel a bit stupid here :P, but I just cannot wrap my head around it.

Suggested change

/// True after [CameraController.dispose] has completed successfully.

bool get isDisposed => _isDisposed;

/// Checks whether [CameraController.dispose] has completed successfully.

///

/// This is a no-op when asserts are disabled.

void debugDisposed() => assert(_isDisposed);

Or maybe debugCheckDisposed or something.

@bparrishMines what do you think?

In general it would be bad to have any code that can potentially access the CameraController when it is already disposed. This means having the debugCheckIsDisposed around should be helpful for developers trying to debug their application, but should not be used in production. This will also prevent code constructs like this, which has high risk on state conflicts:

var cameraController = CameraController(); // do stuff with the camera controller ... if (cameraController.isDisposed) { cameraController = CameraController(); } // continue doing stuff with the camera controller

Yea, that makes sense to avoid that type of pattern. I'm fine with going with a debugCheckIsDisposed instead.

@dnfield thank you for clarifying the coding structure that was very helpful. I took your recommendations and updates the pull request.

Note that I had to change:

void debugCheckIsDisposed() => assert(_isDisposed);

to:

void debugCheckIsDisposed() { assert(_isDisposed); }

otherwise we would receive a compile error.

I would appreciate it if you could have another look.

dnfield

This should be guarded by asserts to discourage use in release mode.

dnfield

LGTM if LG to @bparrishMines

bparrishMines

Everything looks good! Just one small comment on the test.

bparrishMines · 2020-11-09T18:43:40Z

packages/camera/test/camera_test.dart

+
+      controller.dispose();
+
+      try {


I believe returnsNormally is the standard way to make sure no Exception is thrown.

expect(() => controller.debugCheckIsDisposed(), returnsNormally);

bparrishMines

LGTM

I can merge for you once tests pass again.

mvanbeusekom · 2020-11-09T20:36:05Z

Noticed that flutter format failed. I pushed a fix and will monitor the CI and merge when it is all green. Thank you for reviewing so quickly!

…unit Tests for the new implementation. (flutter#3193) * Update feedback from Flutter team * Removed obsolete dependency on mockito * Apply feedback on pull request * Bump version number * Update CHANGELOG description to reflect changes * Fix formatting * Refactor try..catch to returnsNormally * Fix formatting issues Co-authored-by: Maurits van Beusekom <[email protected]>

Sempakonka requested a review from bparrishMines as a code owner October 23, 2020 13:31

google-cla bot added the cla: yes label Oct 23, 2020

bparrishMines reviewed Oct 30, 2020

View reviewed changes

packages/camera/test/camera_test.dart Outdated Show resolved Hide resolved

packages/camera/test/new/camera_test.dart Outdated Show resolved Hide resolved

Sempakonka requested review from amirh, bkonyi, cyanglaz, dnfield, iskakaushik, LHLL, matthew-carroll, mehmetf and mklim as code owners November 5, 2020 08:23

google-cla bot added cla: no and removed cla: yes labels Nov 5, 2020

mvanbeusekom closed this Nov 5, 2020

mvanbeusekom force-pushed the issue-51630 branch from 209cbde to 27f5cba Compare November 5, 2020 08:35

mvanbeusekom reopened this Nov 5, 2020

google-cla bot added cla: yes and removed cla: no labels Nov 5, 2020

dnfield reviewed Nov 5, 2020

View reviewed changes

dnfield suggested changes Nov 5, 2020

View reviewed changes

mvanbeusekom added 3 commits November 9, 2020 09:58

Update feedback from Flutter team

00daf54

Removed obsolete dependency on mockito

f4f9291

Apply feedback on pull request

bb7d4f4

mvanbeusekom force-pushed the issue-51630 branch from 37c0d92 to bb7d4f4 Compare November 9, 2020 11:51

Bump version number

e77fd8c

mvanbeusekom added 2 commits November 9, 2020 12:55

Update CHANGELOG description to reflect changes

3ea8491

Fix formatting

3b13fe9

dnfield approved these changes Nov 9, 2020

View reviewed changes

bparrishMines reviewed Nov 9, 2020

View reviewed changes

Refactor try..catch to returnsNormally

e14f7ee

mvanbeusekom requested a review from bparrishMines November 9, 2020 20:25

bparrishMines approved these changes Nov 9, 2020

View reviewed changes

Fix formatting issues

252a727

mvanbeusekom merged commit ee1dbfa into flutter:master Nov 10, 2020

mvanbeusekom deleted the issue-51630 branch November 10, 2020 06:50

darshankawar mentioned this pull request Mar 13, 2025

[camera] CameraController should expose isDisposed flutter/flutter#165097

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Camera] Made CameraController.isDisposed publicly accessible. Added unit Tests for the new implementation. #3193

[Camera] Made CameraController.isDisposed publicly accessible. Added unit Tests for the new implementation. #3193

Sempakonka commented Oct 23, 2020 •

edited by bparrishMines

Loading

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

mvanbeusekom commented Nov 5, 2020

dnfield Nov 5, 2020

mvanbeusekom Nov 5, 2020

dnfield Nov 5, 2020

dnfield Nov 5, 2020

mvanbeusekom Nov 6, 2020

bparrishMines Nov 6, 2020

mvanbeusekom Nov 9, 2020

dnfield left a comment

dnfield left a comment

bparrishMines left a comment

bparrishMines Nov 9, 2020

bparrishMines left a comment

mvanbeusekom commented Nov 9, 2020

		/// True after [CameraController.dispose] has completed successfully.
		bool get isDisposed => _isDisposed;

-  /// True after [CameraController.dispose] has completed successfully.
-  bool get isDisposed => _isDisposed;
+  /// Checks whether [CameraController.dispose] has completed successfully.
+  ///
+  /// This is a no-op when asserts are disabled.
+  void debugDisposed() => assert(_isDisposed);

[Camera] Made CameraController.isDisposed publicly accessible. Added unit Tests for the new implementation. #3193

[Camera] Made CameraController.isDisposed publicly accessible. Added unit Tests for the new implementation. #3193

Conversation

Sempakonka commented Oct 23, 2020 • edited by bparrishMines Loading

Description

Related Issues

Checklist

Breaking Change

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

mvanbeusekom commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

bparrishMines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparrishMines left a comment

Choose a reason for hiding this comment

mvanbeusekom commented Nov 9, 2020

Sempakonka commented Oct 23, 2020 •

edited by bparrishMines

Loading