[video_player] Add new option for allowBackgroundPlayback #5013
Conversation
# Conflicts: # packages/video_player/video_player_platform_interface/CHANGELOG.md
- Add _ambiguate to address an analyzer error - Increase the required version for video_player_platform_interface to 5.1.0, the first version where allowBackgroundPlayback appears
stuartmorgan-g
changed the title
- always_specify_types - avoid_bool_literal_in_conditional_expressions - avoid_function_literal_in_foreach_calls - terminating new line
ghost
mentioned this pull request
| video_player_android: ^2.2.17 | ||
| video_player_avfoundation: ^2.2.17 | ||
| video_player_platform_interface: ">=4.2.0 <6.0.0" | ||
| video_player_platform_interface: ">=5.1.0 <6.0.0" |
There was a problem hiding this comment.
Nit: This should just be ^5.1.0. The other syntax was specifically to allow two major versions of the platform interface for a while.
| } | ||
|
|
||
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( |
There was a problem hiding this comment.
Nit: This should be private (_ verifyPlayingWhenAppLifecyclePaused)
| } | ||
|
|
||
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( |
There was a problem hiding this comment.
The name here seems confusing. How about _verifyPlayStateRespondsToLifecycle?
| void main() { | ||
| void verifyPlayingWhenAppLifecyclePaused( | ||
| VideoPlayerController controller, { | ||
| required bool isObserving, |
There was a problem hiding this comment.
s/isObsvering/shouldPauseInBackground/ to make what is being validated (rather than the implementation details of how that is achieved) clearer.
| VideoPlayerController controller, { | ||
| required bool isObserving, | ||
| }) { | ||
| final bool wasPlayingBeforePause = controller.value.isPlaying; |
There was a problem hiding this comment.
It doesn't make sense for a test to call this method when it's not playing already, so I would eliminate this and just start with expect(controller.value.isPlaying, true); as a starting condition. That should make the method easier to understand.
|
@gaaclarke for second review |
|
Thanks for the the review @stuartmorgan! |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I made one minor formatting tweak to pubspec.yaml; LGTM with that.
|
You'll need to merge master in again; there was an authentication change for Firebase Test Lab that required a commit, which is why this last CI run failed. |
| expect(controller.videoPlayerOptions!.mixWithOthers, true); | ||
| }); | ||
|
|
||
| for (final bool allowBackgroundPlayback in <bool>[true, false]) { |
There was a problem hiding this comment.
nit: instead of looping and asserting 2 different things, this should be 2 different independent tests (factor out shared code if needed). It's much easier to grok tests that are asserting absolutes instead of having the extra layer of indirection.
stuartmorgan-g
added
the
waiting for tree to go green
5 participants
This PR picks up and continues #4908 to support background play
Fixes flutter/flutter#63836