-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[video_player] Add durationUpdate to VideoEventType #10330
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.
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
base: main
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request introduces a new durationUpdate event to allow for changes in video duration during playback, which is useful for formats like HLS live streams. The VideoPlayerController is updated to handle this event and update its duration value accordingly. A new test is added to verify this functionality. My feedback focuses on improving the clarity and correctness of the new test case.
| testWidgets('buffering status', (WidgetTester tester) async { | ||
| final VideoPlayerController controller = | ||
| VideoPlayerController.networkUrl(_localhostUri); | ||
|
|
||
| await controller.initialize(); | ||
| const Duration initDuration = Duration(milliseconds: 100); | ||
| controller.value = controller.value.copyWith(duration: initDuration); | ||
|
|
||
| final StreamController<VideoEvent> fakeVideoEventStream = | ||
| fakeVideoPlayerPlatform.streams[controller.playerId]!; | ||
|
|
||
| const Duration updatedDuration = Duration(milliseconds: 200); | ||
| fakeVideoEventStream.add( | ||
| VideoEvent(eventType: VideoEventType.durationUpdate, duration: updatedDuration), | ||
| ); | ||
|
|
||
| await tester.pumpAndSettle(); | ||
| expect(controller.value.duration, updatedDuration); | ||
| await tester.runAsync(controller.dispose); | ||
| }); |
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 test name 'buffering status' is misleading as this test verifies the duration update functionality. It's also a duplicate of another test name in this file, which can cause confusion. It should be renamed to be more descriptive. Additionally, an extra blank line can be removed for conciseness.
| testWidgets('buffering status', (WidgetTester tester) async { | |
| final VideoPlayerController controller = | |
| VideoPlayerController.networkUrl(_localhostUri); | |
| await controller.initialize(); | |
| const Duration initDuration = Duration(milliseconds: 100); | |
| controller.value = controller.value.copyWith(duration: initDuration); | |
| final StreamController<VideoEvent> fakeVideoEventStream = | |
| fakeVideoPlayerPlatform.streams[controller.playerId]!; | |
| const Duration updatedDuration = Duration(milliseconds: 200); | |
| fakeVideoEventStream.add( | |
| VideoEvent(eventType: VideoEventType.durationUpdate, duration: updatedDuration), | |
| ); | |
| await tester.pumpAndSettle(); | |
| expect(controller.value.duration, updatedDuration); | |
| await tester.runAsync(controller.dispose); | |
| }); | |
| testWidgets('duration is updated by durationUpdate event', (WidgetTester tester) async { | |
| final VideoPlayerController controller = | |
| VideoPlayerController.networkUrl(_localhostUri); | |
| await controller.initialize(); | |
| const Duration initDuration = Duration(milliseconds: 100); | |
| controller.value = controller.value.copyWith(duration: initDuration); | |
| final StreamController<VideoEvent> fakeVideoEventStream = | |
| fakeVideoPlayerPlatform.streams[controller.playerId]!; | |
| const Duration updatedDuration = Duration(milliseconds: 200); | |
| fakeVideoEventStream.add( | |
| VideoEvent(eventType: VideoEventType.durationUpdate, duration: updatedDuration), | |
| ); | |
| await tester.pumpAndSettle(); | |
| expect(controller.value.duration, updatedDuration); | |
| await tester.runAsync(controller.dispose); | |
| }); |
Some custom video_platform implementation can update a duration during playback (like media_kit) for HLS streams (if they detected as a livestream). To address this new VideoEventType added: durationUpdate.
Pre-Review Checklist
[video_player]pubspec.yamlwith 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[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).