[video_player_avfoundation] Implement preventsDisplaySleepDuringVideoPlayback#11547
[video_player_avfoundation] Implement preventsDisplaySleepDuringVideoPlayback#11547shrabanti722 wants to merge 5 commits into
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request implements the setPreventsDisplaySleepDuringVideoPlayback feature for the AVFoundation video player, enabling control over display sleep during playback. The changes include updates to Pigeon message definitions, the Dart platform implementation, and the native Objective-C code. Feedback indicates that the implementation lacks required unit or integration tests as specified in the repository style guide.
…Playback Implements the platform interface method `setPreventsDisplaySleepDuringVideoPlayback` using `AVPlayer.preventsDisplaySleepDuringVideoPlayback` on iOS and macOS. AVFoundation implementation breakout PR for flutter#11225. Depends on the platform interface breakout PR (6.7.0). Made-with: Cursor
431a14a to
b53cddc
Compare
There was a problem hiding this comment.
Code Review
This pull request implements setPreventsDisplaySleepDuringVideoPlayback for the AVFoundation video player implementation, allowing control over display sleep during video playback. The changes include Pigeon-generated message updates, Dart and Objective-C platform code additions, and corresponding unit tests. Feedback on the changes suggests removing a redundant @available check in FVPVideoPlayer.m since the package's minimum supported versions are already higher than the check specifies.
| - (void)setPreventsDisplaySleepDuringVideoPlayback:(BOOL)preventsDisplaySleepDuringVideoPlayback error:(FlutterError *_Nullable *_Nonnull)error { | ||
| if (@available(iOS 12.0, macOS 10.14, *)) { | ||
| self.player.preventsDisplaySleepDuringVideoPlayback = preventsDisplaySleepDuringVideoPlayback; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since the minimum supported versions for this package are iOS 13.0 and macOS 10.15 (as of version 2.8.5), the @available(iOS 12.0, macOS 10.14, *) check is redundant. We can safely remove it to simplify the code.
- (void)setPreventsDisplaySleepDuringVideoPlayback:(BOOL)preventsDisplaySleepDuringVideoPlayback error:(FlutterError *_Nullable *_Nonnull)error {
self.player.preventsDisplaySleepDuringVideoPlayback = preventsDisplaySleepDuringVideoPlayback;
}…tion Resolves conflicts caused by the avfoundation Pigeon and source-directory restructure on main: - Drops stale pigeons/messages.dart, lib/src/messages.g.dart, and the old messages.g.h (the file split into instance/plugin pigeons under video_player_avfoundation_objc/). - Re-applies setPreventsDisplaySleepDuringVideoPlayback to pigeons/video_player_instance_messages.dart. - Accepts upstream copies of regenerated files (VideoPlayerInstanceMessages.g.m, avfoundation_video_player_test.mocks.dart) — will be regenerated next. - Bumps version 2.10.0 -> 2.11.0 and platform_interface dep ^6.7.0 -> ^6.8.0.
- Regenerates VideoPlayerInstanceMessages.g.{h,m,dart} so the new
setPreventsDisplaySleepDuringVideoPlayback method is plumbed through.
- Regenerates avfoundation_video_player_test.mocks.dart to include the new
method's mock entry.
- clang-formats the regenerated ObjC files.
- dart format --page-width 100 across lib/, test/, pigeons/.
- Adds the new pigeon method to video_player_instance_messages.dart.
|
@stuartmorgan-g @tarrinneal this is the AVFoundation child PR for the screen auto-lock change. |
hellohuanlin
left a comment
There was a problem hiding this comment.
Seems to be a straight forward change, but it's missing unit tests on native side.
|
@hellohuanlin In the future, please try to check for missing tests when reviewing the combo PR. Part of the goal of getting all necessary approvals on the combo PR before splitting out the sub PRs is to have the sub PRs all land very quickly so that we don't end up in a limbo where features are partially landed for extended periods of time. |
|
|
||
| - (void)setPreventsDisplaySleepDuringVideoPlayback:(BOOL)preventsDisplaySleepDuringVideoPlayback | ||
| error:(FlutterError *_Nullable *_Nonnull)error { | ||
| if (@available(iOS 12.0, macOS 10.14, *)) { |
There was a problem hiding this comment.
Why are you doing runtime checks for OS versions that are lower than our minimum compilation targets? This condition can never be false.
- Remove the redundant @available(iOS 12.0, macOS 10.14, *) check in setPreventsDisplaySleepDuringVideoPlayback; the package minimums are iOS 13 / macOS 10.15, so the condition can never be false. - Add a native Swift unit test verifying the setter updates the underlying AVPlayer in both directions.
|
@shrabanti722 It looks like you either didn't run the repo |
Yeah @stuartmorgan-g I actually came on a vacation and don’t have my Mac with me, so couldn’t run it locally, I used a windows laptop for this since it was getting delayed. But looks like I will be able to fix it only after I come back :( which will be 5 - 6 days. Sorry for that. |
Description
Implements the
setPreventsDisplaySleepDuringVideoPlaybackplatform interface method usingAVPlayer.preventsDisplaySleepDuringVideoPlaybackon iOS and macOS.AVFoundation implementation breakout PR for #11225.
Dependencies
video_player_platform_interface6.7.0 breakout PR. CI will fail until that PR lands and the new interface version is published. Keeping this PR in draft until then.Changes
setPreventsDisplaySleepDuringVideoPlaybackinFVPVideoPlayer.mand wires it through the Dart platform implementation.video_player_avfoundationto 2.10.0 and updates thevideo_player_platform_interfaceconstraint to ^6.7.0.Related