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

[camera] Export VideoCaptureOptions so that implementers can use it #6666

Merged
merged 2 commits into from
Nov 21, 2022
Merged

[camera] Export VideoCaptureOptions so that implementers can use it #6666

merged 2 commits into from
Nov 21, 2022

Conversation

adam-harwood
Copy link
Contributor

This should have been done in #6550 but was missed.

Related to flutter/flutter#83634

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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@adam-harwood adam-harwood changed the title Export VideoCaptureOptions so that implementers can use it [camera] Export VideoCaptureOptions so that implementers can use it Nov 3, 2022
@adam-harwood adam-harwood mentioned this pull request Nov 9, 2022
Merged
11 tasks
bparrishMines
bparrishMines approved these changes Nov 15, 2022
View reviewed changes
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

@stuartmorgan for secondary review.

stuartmorgan
stuartmorgan reviewed Nov 15, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

We should be able to add a test for this by having a test file that imports only the primary package import, and uses all the classes we expect to be publicly available.

@Hixie
Copy link
Contributor

Hixie commented Nov 15, 2022

Yeah I was gonna say, the implication of there not being a test here is that we're not actually testing the stuff we're exporting, which is concerning. :-)

@bparrishMines
Copy link
Contributor

Yea, that's a good point. I suppose I haven't been testing exports because it often gets overlooked by other changes in a PR.

@adam-harwood
Copy link
Contributor Author

I agree some tests here would be useful. @stuartmorgan are you saying that adding those tests needs to be part of this PR, or was it a forward-looking statement?

@stuartmorgan
Copy link
Contributor

Yes, they should be part of this PR since the PR isn't test-exempt.

@adam-harwood
Copy link
Contributor Author

Yes, they should be part of this PR since the PR isn't test-exempt.

Got it @stuartmorgan, I've added a test class to verify that the classes exported from types.dart can be referenced when only importing the root package. Anything else needed?

stuartmorgan
stuartmorgan suggested changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Looks good other than a comment about test location.

You'll also need to merge in master to fix the CI failures (due to credential rotation in the repository).

packages/camera/camera_platform_interface/test/types/type_export_test.dart Outdated
@@ -0,0 +1,48 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
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 is really testing that the main interface file exports types, not the types themselves, these tests should be a new group('exports' ... in test/camera_platform_interface_test.dart, rather than a new test file here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Export VideoCaptureOptions so that it is accessible to other packages
826145b 
Also added tests so that missing exports can be more easily tested in the future.
@adam-harwood adam-harwood force-pushed the aharwood/expose_video_capture_options branch from 2f8f895 to 826145b Compare November 21, 2022 05:32
@adam-harwood adam-harwood requested review from stuartmorgan and removed request for jmagman, ditman, hellohuanlin and camsim99 November 21, 2022 05:44
@stuartmorgan stuartmorgan added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Nov 21, 2022
@auto-submit auto-submit bot merged commit 1aabcd6 into flutter:main Nov 21, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 21, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 21, 2022
@engine-flutter-autoroll
1aabcd62a [camera] Export VideoCaptureOptions so that implementers ca…
6515595 
…n use it (flutter/plugins#6666)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 21, 2022
@engine-flutter-autoroll
Roll Plugins from b2fe01bc0f29 to 475caa00130d (5 revisions) (#115761)
a9858ec 
* 017e776f9 Revert "[ci]Temporarily disable starqlteue on firebase device lab (#6657)" (flutter/plugins#6710)

* d6c0bcbf2 [ci] Manually Roll Flutter from 61e927d to d2e6dfe (143 revisions) (flutter/plugins#6723)

* 283122b1f Roll Flutter from d2e6dfe to 46d868c (5 revisions) (flutter/plugins#6724)

* 1aabcd62a [camera] Export VideoCaptureOptions so that implementers can use it (flutter/plugins#6666)

* 475caa001 Roll Flutter from 46d868c to 633d7ef (8 revisions) (flutter/plugins#6725)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@engine-flutter-autoroll @shogohida
Roll Plugins from b2fe01bc0f29 to 475caa00130d (5 revisions) (flutter…
8cedca2 
…#115761)

* 017e776f9 Revert "[ci]Temporarily disable starqlteue on firebase device lab (flutter#6657)" (flutter/plugins#6710)

* d6c0bcbf2 [ci] Manually Roll Flutter from 61e927d to d2e6dfe (143 revisions) (flutter/plugins#6723)

* 283122b1f Roll Flutter from d2e6dfe to 46d868c (5 revisions) (flutter/plugins#6724)

* 1aabcd62a [camera] Export VideoCaptureOptions so that implementers can use it (flutter/plugins#6666)

* 475caa001 Roll Flutter from 46d868c to 633d7ef (8 revisions) (flutter/plugins#6725)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Plugins from b2fe01bc0f29 to 475caa00130d (5 revisions) (flutter…
9fc1d98 
…#115761)

* 017e776f9 Revert "[ci]Temporarily disable starqlteue on firebase device lab (flutter#6657)" (flutter/plugins#6710)

* d6c0bcbf2 [ci] Manually Roll Flutter from 61e927d to d2e6dfe (143 revisions) (flutter/plugins#6723)

* 283122b1f Roll Flutter from d2e6dfe to 46d868c (5 revisions) (flutter/plugins#6724)

* 1aabcd62a [camera] Export VideoCaptureOptions so that implementers can use it (flutter/plugins#6666)

* 475caa001 Roll Flutter from 46d868c to 633d7ef (8 revisions) (flutter/plugins#6725)
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@adam-harwood @mauricioluz
[camera] Export VideoCaptureOptions so that implementers can use it (f…
00dda9e 
…lutter#6666)

* Export VideoCaptureOptions so that it is accessible to other packages

Also added tests so that missing exports can be more easily tested in the future.

* Fix dart analyze warnings
@rjansen rjansen mentioned this pull request Mar 24, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bparrishMines bparrishMines bparrishMines approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adam-harwood @Hixie @bparrishMines @stuartmorgan