-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[template] [video_player] Add playback speed support on all platforms #3031
[template] [video_player] Add playback speed support on all platforms #3031
Conversation
c52dc75
to
2428843
Compare
9ae25cf
to
58062c1
Compare
In a discussion with @stuartmorgan, I just now realized that this PR needs to be split up. I will iteratively roll out the smaller PRs in order to make this work. The start is the platform interface: #3032 The tests failing here is intentional. |
Was this merged? |
@jpiabrantes No, I am still waiting for review. I will update the merge conflicts today and hope for a review soon. |
(I am bumping the Dart SDK constraint to |
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.
This looks good to me, I didn't find anything blocking, mostly nitpicking!
As you've already done, this needs to be split in 3 separate PRs so we can publish it in order.
packages/video_player/video_player_platform_interface/lib/method_channel_video_player.dart
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_web/lib/video_player_web.dart
Outdated
Show resolved
Hide resolved
packages/video_player/video_player_web/test/video_player_web_test.dart
Outdated
Show resolved
Hide resolved
Let's close this PR since it's been split up in individual PRs now. |
Published video_player v0.11.0, which contains the playback speed feature! |
Description
Playback speed on videos is a very important feature to us and seems to be highly requested as well.
There have been earlier attempts to implement this feature into the
video_player
plugin, however, the PRs for those have remained stale.Consequently, I decided to pick up this task and implement playback speed support into the latest version of the
video_player
plugin.What this PR accomplishes
This PR adds the playback speed feature on Android, iOS, and web.
I did not copy code from existing PRs (though I did use them as a reference for the Android and iOS implementations). Instead, I designed the API in the way that I figured was most sensible having the learnings from the other PRs in mind. (I actually later figured out just how similar the interface implementations were, even though they were created separately. I suppose it happened because we all followed the existing interface).
Related Issues
Pigeon
I upgraded
pigeon
to0.1.7
. You might say that this makes the PR harder to review because the generation changed, but I thought about it and I have to disagree:pigeon
version.Testing
Manual testing
I tested this both on Android and on iOS in a production app.
Additionally, I added the feature to the example app and tested that on web and mobile.
Unit (and integration) tests
Integration testing for this feature seems not only very difficult, but most importantly not sensible. I can test the platform calls, but the underlying native libraries and Flutter functionality should do the integration testing.
Having said that, I did include unit tests.
Checklist
///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
Barely breaking
I think formally this is a breaking change, but realistically it is just a hotfix because the only breaking change is
toString
forVideoPlayerValue
, so 🤷🏽♀️