[video_player_platform_interface] Add preventsDisplaySleepDuringVideoPlayback option#11546
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the video_player_platform_interface to version 6.7.0, adding preventsDisplaySleepDuringVideoPlayback to VideoPlayerOptions and a corresponding method to VideoPlayerPlatform. Review feedback suggests restoring a removed CHANGELOG entry regarding SDK versions and ensuring the new interface method throws an UnimplementedError for consistency with existing methods.
| Future<void> setPreventsDisplaySleepDuringVideoPlayback( | ||
| int playerId, | ||
| bool preventsDisplaySleepDuringVideoPlayback, | ||
| ) { | ||
| return Future<void>.value(); | ||
| } |
There was a problem hiding this comment.
For consistency with other methods in this class (such as setMixWithOthers, setAllowBackgroundPlayback, and setWebOptions), this method should throw an UnimplementedError instead of returning a no-op Future. This ensures that platform implementations are explicitly notified if they haven't implemented the method when it's called, rather than failing silently.
| Future<void> setPreventsDisplaySleepDuringVideoPlayback( | |
| int playerId, | |
| bool preventsDisplaySleepDuringVideoPlayback, | |
| ) { | |
| return Future<void>.value(); | |
| } | |
| Future<void> setPreventsDisplaySleepDuringVideoPlayback( | |
| int playerId, | |
| bool preventsDisplaySleepDuringVideoPlayback, | |
| ) { | |
| throw UnimplementedError( | |
| 'setPreventsDisplaySleepDuringVideoPlayback() has not been implemented.', | |
| ); | |
| } |
…Playback option Adds the `preventsDisplaySleepDuringVideoPlayback` field to `VideoPlayerOptions` and a `setPreventsDisplaySleepDuringVideoPlayback` method to `VideoPlayerPlatform`, allowing platform implementations to control whether the display sleeps during video playback. Platform interface breakout PR for flutter#11225. Made-with: Cursor
shrabanti722
force-pushed
the
video-player-prevents-display-sleep-interface
branch
from
fbf1314 to
c52b4b6
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
@stuartmorgan-g 2nd rev
stuartmorgan-g
added
the
federated: partial_changes
| /// | ||
| /// Defaults to `true`. | ||
| /// | ||
| /// This option is currently only supported on iOS and macOS. |
There was a problem hiding this comment.
This comment should not be here; a platform interface cannot make claims about implementations, because they are part of another package that this doesn't control the version of.
It's fine to say that something may not be supported on all platforms, because that's a generic statement that won't become inaccurate if other packages change, but not to say specific things about which ones do and don't.
Replace the preventsDisplaySleepDuringVideoPlayback dartdoc sentence that named specific platforms with generic wording, per platform interface conventions. CHANGELOG: note the clarification under 6.7.0.
|
@stuartmorgan-g are there any more changes needed from my side ? |
|
@shrabanti722 I can't review the PR in its current state, as you have uploaded commits with a co-author that doesn't have a signed CLA entry. We can only review PRs that pass the CLA check. |
shrabanti722
force-pushed
the
video-player-prevents-display-sleep-interface
branch
from
72b79dc to
0459c0f
Compare
|
@stuartmorgan-g fixed it. |
stuartmorgan-g
added
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/packages/11546, because - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Unfortunately, due to the delay the code in this PR will need to be re-autoformatted, as the line length for the repo changed. Sorry about that. Once it's updated we can land it. |
Re-run `dart format` after the repo's analysis_options.yaml page_width changed from 80 to 100 in flutter#11692.
|
@stuartmorgan-g reformatted. |
| int playerId, | ||
| bool preventsDisplaySleepDuringVideoPlayback, | ||
| ) { | ||
| throw UnimplementedError( |
There was a problem hiding this comment.
Sorry, I missed this in the previous review. It looks like the main PR plans to only implement this for some platforms.
A feature cannot both throw an error when not implemented, and not have a support query, because otherwise there is no way for a client to know whether or not it is safe to call the method.
Is the desired behavior for this feature that it silently no-ops if not implemented, or do clients need to know whether it is going to work?
/cc @tarrinneal
There was a problem hiding this comment.
switched the default to a silent no-op.
Per review feedback: a method cannot both throw UnimplementedError and lack a support query, since clients have no way to know whether it is safe to call. Switch the default to a silent no-op so platforms that do not support controlling display sleep gracefully fall back to their default behavior.
3 participants
Description
Adds the
preventsDisplaySleepDuringVideoPlaybackfield toVideoPlayerOptionsand asetPreventsDisplaySleepDuringVideoPlaybackmethod toVideoPlayerPlatform, allowing platform implementations to control whether the display sleeps during video playback.Platform interface breakout PR for #11225.
Changes
preventsDisplaySleepDuringVideoPlaybackfield toVideoPlayerOptions(defaults totrueto preserve existing behavior).setPreventsDisplaySleepDuringVideoPlayback(int playerId, bool preventsDisplaySleepDuringVideoPlayback)method toVideoPlayerPlatformwith a default no-op implementation so existing platform implementations continue to compile without changes.preventsDisplaySleepDuringVideoPlayback.Related