[video_player] Adds Android options for [video_player_platform_interface]
Platform Interface PR for Video Player Android Options (https://github.com/flutter/packages/pull/5900).
Adds Android options to add support for enabling exoplayer extensions like ffmpeg.
Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
- [x] I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the [CLA].
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I [linked to at least one issue that this PR fixes] in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes]. - [x] I updated
CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style]. - [x] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is [test-exempt].
- [x] All existing and new tests are passing.
I didn't comment on all of them, but you should read through the style guide for comments/documentation and redo the formatting on most of your doc comments.
I have updated the doc comments and the formatting. Is there any other changes needed for this pr?
@stuartmorgan I know this isn't the preferred style of platform options, but seeing as web is already there as well, do you think it's better to match that style, or move this code to the android package?
In general I think we should use our newer patterns when possible, and since this case is extremely niche I would definitely prefer not to expand the cross-platform API surface. Can this be a platform-package-specific subclass of VideoPlayerOptions instead?
The place to have this conversation would be a combination PR though; @basemosama please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins. It's too early in the design+review process for a platform-interface-specific PR to exist yet.
@stuartmorgan
In general I think we should use our newer patterns when possible, and since this case is extremely niche I would definitely prefer not to expand the cross-platform API surface.
I think it's fine to add custom Android options to VideoPlayerOptions, especially since it already includes webOptions. This way, we're prepared for future platform-specific additionitional options for android or any other platform.
Can this be a platform-package-specific subclass of
VideoPlayerOptionsinstead?
We could make a platform-specific subclass of VideoPlayerOptions, but what about situations where we need to set options for multiple platforms like Android and web?
We might consider incorporating a platformOptions field within VideoPlayerOptions to manage platform-specific configurations. Then platformOptions class can contain distinct classes such as VideoPlayerAndroidOptions and VideoPlayerWebOptions to handle the unique requirements of each platform.
The place to have this conversation would be a combination PR though; @basemosama please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins. It's too early in the design+review process for a platform-interface-specific PR to exist yet.
Appreciate the direction. I've reviewed the provided link. We can have conversation on the original pr #5900 If you would like.
However should I continue with implementing the remaining parts as planned, or would you recommend any adjustments to the APIs before proceeding further?
I think it's fine to add custom Android options to VideoPlayerOptions, especially since it already includes webOptions.
Already having some code that uses a pattern that we've found doesn't scale well isn't an argument for doing more of the same thing rather than doing new things using newer patterns based on more experience with federated plugins.
This way, we're prepared for future platform-specific additionitional options for android or any other platform.
Adding Android-specific options doesn't prepare us for adding other platform-specific options for other platforms, it just makes it even more likely that the next case would continue putting more platform-specific things into the platform-agnostic interface, which we now consider an anti-pattern.
Can this be a platform-package-specific subclass of
VideoPlayerOptionsinstead?We could make a platform-specific subclass of VideoPlayerOptions, but what about situations where we need to set options for multiple platforms like Android and web?
See https://github.com/flutter/packages/blob/a2f4ce0a50570a623e54788901314b5f8352c0cc/packages/webview_flutter/webview_flutter/lib/src/webview_controller.dart#L66-L96 and https://github.com/flutter/packages/blob/a2f4ce0a50570a623e54788901314b5f8352c0cc/packages/webview_flutter/webview_flutter_android/lib/src/android_webview_controller.dart#L37-L51 for an example of the pattern.
We might consider incorporating a
platformOptionsfield withinVideoPlayerOptionsto manage platform-specific configurations. ThenplatformOptionsclass can contain distinct classes such asVideoPlayerAndroidOptionsandVideoPlayerWebOptionsto handle the unique requirements of each platform.
This does not address issues like third-party federated implementations for new platforms.
However should I continue with implementing the remaining parts as planned, or would you recommend any adjustments to the APIs before proceeding further?
I did suggest adjustments to the API, and suggested that further conversation about it happen in the other issue.
Closing for now, per the comment above, in favor of discussion in https://github.com/flutter/packages/pull/5900.