-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player] Pause video when it completes #3727
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for the contribution! Per the checklist and Flutter's PR policies, this will need tests covering the change(s) you are making here as part of the PR. Once those are in place, we can move forward with a review. |
@@ -429,7 +431,37 @@ void main() { | |||
await tester.pumpAndSettle(); | |||
|
|||
expect(controller.value.isPlaying, isFalse); | |||
expect(controller.value.position, controller.value.duration); | |||
expect(controller.value.position, nonzeroDuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, duration
was zero so this assertion would pass even if position
never changed.
fakeVideoPlayerPlatform | ||
.calls[fakeVideoPlayerPlatform.calls.length - 2], | ||
'pause'); | ||
expect(fakeVideoPlayerPlatform.calls.last, 'seekTo'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to check that the underlying platform actually stopped playing (instead of testing implementation details), but it wasn't clear to me how to do that using the existing test fakes.
If it's worth the effort, I'll dig more into how I might access the underlying platform in unit tests or set up a golden file test or device lab tests for video_player
. (Those feel rather heavy-handed for this change, but the setup seems like it would be generally useful for video_player
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integration tests in packages/video_player/video_player/example/integration_test/video_player_test.dart looks like it may be a good place to test this. I'll look into it more now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll plan to wait until a reviewer has bandwidth to provide guidance.
Here are some things I found in the rabbit hole:
- Running tests in packages/video_player/video_player/example/integration_test/video_player_test.dart with
flutter test
fails (maybe there's a different way to run them?). - @cyanglaz's TODO to un-skip the test in
example/test_driver/video_player_test.dart
may be resolved (video_player driver test failing on Cirrus CI iOS simulator. flutter#43012 was marked fixed in Oct).flutter drive test_driver/video_player_test.dart
passed once on my machine, but failed with timeouts about 4 times. The issue had notes about running these tests on Firebase device lab, so I'm not sure if or where these tests are currently intended to be run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick note on this question (I haven't looked at the PR in general):
- Running tests in packages/video_player/video_player/example/integration_test/video_player_test.dart with
flutter test
fails (maybe there's a different way to run them?).
Things in integration_test
folders are intended to be run via flutter drive
, using the test driver in test_driver
; see https://flutter.dev/docs/testing/integration-tests for discussion of the directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Stuart! I added 2 integration tests. This PR is ready for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change worth mentioning in CHANGELOG.md and updating version in pubspec.yaml?
@@ -129,6 +129,51 @@ void main() { | |||
}, | |||
); | |||
|
|||
testWidgets( | |||
'stay paused when seeking after video completed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue (flutter/flutter#77674)
); | ||
|
||
testWidgets( | ||
'do not exceed duration on play after video completed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional symptom I observed in flutter/flutter#77674 (comment)
@@ -277,6 +277,23 @@ void main() { | |||
expect(fakeVideoPlayerPlatform.calls.last, 'setPlaybackSpeed'); | |||
}); | |||
|
|||
test('play restarts from beginning if video is at end', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new behavior I added to make Android and iOS consistent with web.
Re-opening this PR. (I accidentally closed it while I was working on renaming git branches.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this was in my queue for so long; I lost track of the notification :(
Overall it looks great, just a couple small nits.
- One inline
- The version in pubspec.yaml and CHANGELOG.md need to be updated to the change will be published
I'm not sure what's going on with the CI failures there; I suspect merging in master will resolve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry again for losing the review!
If I play video when it is not initialized, I will get an error. |
Please use the issue tracker, not comments on submitted PRs, to report issues. |
* upstream_master: (40 commits) [image_picker] Image picker fix camera device (flutter#3898) [flutter_plugin_tools] Improve license-check output (flutter#4154) [webview_flutter] Fix broken keyboard issue link (flutter#3266) [flutter_plugin_tools] Support format on Windows (flutter#4150) [flutter_plugin_tools] Make unit tests pass on Windows (flutter#4149) [image_picker_for_web] Migrate image_picker to package:cross_file (flutter#4083) [various] Prepare plugin repo for binding API improvements (flutter#4148) [quick_actions] Add const constructor (flutter#4131) [in_app_purchase] Add iOS currency symbol to ProductDetails (flutter#4144) [in_app_purchase] Added priceCurrencySymbol to SkuDetailsWrapper (flutter#4114) [image_picker_platform_interface] Add methods that return package:cross_file (flutter#4072) [flutter_plugin_tools] Improve and test 'format' (flutter#4145) [flutter_plugin_tools] Only check target packages in analyze (flutter#4146) [in_app_purchase] Fix crash when retrieveReceiptWithError gives an error. (flutter#4138) [video_player] Pause video when it completes (flutter#3727) [in_app_purchase] Add currencySymbol to ProductDetails (flutter#4115) [in_app_purchase] Add documentation for price change confirmations (flutter#4092) [camera] android-rework part 8: Supporting modules for final implementation (flutter#4054) [plugin_platform_interface] Fix README broken link (flutter#4143) [various] Prepare plugin repo for binding API improvements (flutter#4137) ...
Fixes a regression introduced by flutter#3727 where scrubbing a progress indicator to the end of the video while playing would unexpectedly restart playback from the start of the video. Restarting is an intentional behavior under the assumption that `play` is called for a deliberate user action to start playback, but in the case of scrubbing it is a programatic pause/play pair where the decision to call `play` again should account for scrubbing to the end. Fixes flutter/flutter#92658
Instead of just updating the
VideoPlayerValue
isPlaying
andposition
fields when a video completes, have the platform pause and seek to the last frame of the video.Fixes flutter/flutter#77674
Pre-launch Checklist
[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.