Skip to content

[camera_avfoundation] Tests backfilling - part 4 #8854

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 19, 2025
Merged

[camera_avfoundation] Tests backfilling - part 4 #8854

merged 4 commits into from
Mar 19, 2025

Conversation

RobertOdrowaz
Copy link
Contributor

@RobertOdrowaz RobertOdrowaz commented Mar 12, 2025 •
edited
Loading

Backfills tests for the FLTCam class as part of flutter/flutter#119109

Adds tests for the setDeviceOrientation method of the FLTCam class. I had to add FLTCaptureVideoDataOutput and FLTCaptureOutput wrappers around AVFoundation classes to make it mockable (similar to the existing FLTCapturePhotoOutputwrapper)

Pre-Review Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene 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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I linked to 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 I have commented below to indicate which version change exemption this PR falls under.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under.
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under.
  • All existing and new tests are passing.

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

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-tests-backfilling-part4 branch 2 times, most recently from ace2d95 to 0710a0f Compare March 12, 2025 19:01
hellohuanlin
hellohuanlin reviewed Mar 13, 2025
View reviewed changes
Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

overall looks good

...undation/Sources/camera_avfoundation/include/camera_avfoundation/FLTCaptureVideoDataOutput.h
/// The underlying instance of `AVCaptureVideoDataOutput`.
@property(nonatomic, readonly) AVCaptureVideoDataOutput *avOutput;

@property(nonatomic) BOOL alwaysDiscardsLateVideoFrames;
Copy link
Contributor

Choose a reason for hiding this comment

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

these all need doc comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of comment do you have in mind? More "Corresponds to the alwaysDiscardsLateVideoFrames property of the wrapped AVCaptureVideoDataOutput" or a copy of the comment from the AVCaptureVideoDataOutput so that docs are quick to access despite the additional wrapper?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the former. IMO those comments are mostly pointless. What else can a wrapper method do besides calling the equivalent in the wrapped instance? The latter would make more sense but I'm not sure how copyright/licensing around apple documentation works in this case so I don't want to copy it

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO those comments are mostly pointless.

FWIW, I agree. I'm a stickler for enforcing the style guide's requirements for declaration comments in my reviews, but for protocols/classes that are documented at the class level explicitly as being direct passthroughs I have been making an exception since in that specific context I don't think they are adding anything.

The latter would make more sense but I'm not sure how copyright/licensing around apple documentation works in this case so I don't want to copy it

If we are going to put comments, maybe they should just be links to the actual Apple docs for the method.

@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-tests-backfilling-part4 branch from 0710a0f to c515c6f Compare March 14, 2025 09:37
@RobertOdrowaz RobertOdrowaz force-pushed the feature/camera-tests-backfilling-part4 branch from c515c6f to db239d1 Compare March 19, 2025 07:29
@RobertOdrowaz RobertOdrowaz mentioned this pull request Mar 19, 2025
Open
@RobertOdrowaz RobertOdrowaz added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2025
@auto-submit auto-submit bot merged commit dd781d4 into flutter:main Mar 19, 2025
82 checks passed
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2025
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Mar 19, 2025

auto label is removed for flutter/packages/8854, Failed to merge flutter/packages/8854 with Pull request flutter/packages/8854 could not be merged: Merge already in progress.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 19, 2025
@engine-flutter-autoroll
dd781d4bc [camera_avfoundation] Tests backfilling - part 4 (flutter/p…
38bb0bf 
…ackages#8854)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 19, 2025
Merged
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Mar 19, 2025
@engine-flutter-autoroll
Roll Packages from d450e1bfa713 to dd781d4bcb7f (5 revisions) (#165489)
20e7f8d 
flutter/packages@d450e1b...dd781d4

2025-03-19 [email protected] [camera_avfoundation] Tests
backfilling - part 4 (flutter/packages#8854)
2025-03-18 [email protected] [camera_avfoundation] Tests
backfilling - part 5 (flutter/packages#8873)
2025-03-18 [email protected] [video_player]: reduce video player
position update interval from 500ms to 100ms (flutter/packages#8346)
2025-03-18 [email protected] [google_maps_flutter] Support
for Ground Overlay (flutter/packages#8432)
2025-03-18 [email protected] [google_maps_flutter] Ground
overlay support - platform impls (flutter/packages#8563)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hellohuanlin hellohuanlin hellohuanlin approved these changes

@stuartmorgan-g stuartmorgan-g Awaiting requested review from stuartmorgan-g

Assignees
No one assigned
Labels
p: camera platform-ios platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RobertOdrowaz @stuartmorgan-g @hellohuanlin