-
Notifications
You must be signed in to change notification settings - Fork 3.5k
refactor(video_player): replace audio track string ID with typed indices #10379
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
refactor(video_player): replace audio track string ID with typed indices #10379
Conversation
- Changed VideoAudioTrack.id (String) to separate groupIndex and trackIndex (int) properties for better type safety - Updated selectAudioTrack() to accept VideoAudioTrack object instead of separate trackId parameter - This is a breaking change (v7.0.0) that provides a more intuitive API for audio track selection
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 is a well-executed refactoring of the audio track selection API. It replaces the string-based track ID with typed indices (groupIndex and trackIndex) on the VideoAudioTrack object, improving type safety and the developer experience. The changes are consistently applied across the platform interface, including the selectAudioTrack method, the VideoAudioTrack class, and the corresponding tests. The version bump to 7.0.0 for this breaking change is appropriate, and the changelog is updated accordingly. I have one minor suggestion to improve the clarity of the changelog entry.
|
|
||
| * **BREAKING CHANGE**: Refactors audio track identification system: | ||
| * `VideoAudioTrack.id` (String) replaced with separate `groupIndex` (int) and `trackIndex` (int) properties. | ||
| * `selectAudioTrack(int playerId, String trackId)` changed to `selectAudioTrack(int playerId, VideoAudioTrack track)` - now accepts the full track object instead of separate parameters. |
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.
For improved clarity in the release notes, consider rephrasing this point. The playerId is still a separate parameter, so stating it accepts the full track object 'instead of separate parameters' is slightly inaccurate. The main change is replacing the trackId string with the VideoAudioTrack object.
| * `selectAudioTrack(int playerId, String trackId)` changed to `selectAudioTrack(int playerId, VideoAudioTrack track)` - now accepts the full track object instead of separate parameters. | |
| * `selectAudioTrack(int playerId, String trackId)` changed to `selectAudioTrack(int playerId, VideoAudioTrack track)` - now accepts the full track object instead of a separate track ID. |
|
What |
That would have needed to be considered before you posted #10171, had it go through review, and got it landed. We don't do rapid iteration in production with breaking changes of platform interfaces; please read this comment. I don't see a sufficiently compelling reason here to make a breaking change. You are welcome to file an issue tracking the desire to change this the next time we are making a breaking change for other reasons.
At this point, the best option to mitigate it is probably to have an internal structure on each platform, and a single method to convert between that structure and the string form, used at the interface boundary of each platform.
Most developers never need to interact with the platform interface, so this is not sufficient reason for a platform interface breaking change. We're likely talking about a single-digit number of developers who will need to interact with this.
If different platform handle things differently, that's an argument for a flexible system—either strings, or some subclassable structure. So even if we did want to make a breaking change for this, hard-coding a specific set of fields seems like the wrong solution given this, and likely to cause yet another round of breaking changes in the future.
Clients of |
|
got it @stuartmorgan-g have moved the encoding/decoding logic to dart side of the platform interface and updated the main #9925 PR. The reason i raised this updated interface PR was based on comment by @ash2moon in #10312 PR |
This is why we generally wait until the full combined PR has passed all reviews before landing any sub-PRs. It looks like this comment was premature, since the Android review wasn't complete. (@tarrinneal FYI for future federated PRs.) It's not ideal that it turned out this way, but given that it did we'll need to work with the string form. |
Summary
Refactors the audio track identification and selection API to use separate
groupIndexandtrackIndexintegers instead of encoded string IDs, and updates selectAudioTrack to accept aVideoAudioTrackobject for improved type safety and developer experience.Breaking Changes
Version: 7.0.0 (major version bump)
1. VideoAudioTrack Structure Change
Before:
After:
Motivation :
The previous string-based track ID system had several issues:
Lack of Type Safety: String encoding/decoding of track indices was error-prone
Poor Developer Experience: Required developers to manually construct or parse track IDs
Platform Inconsistency: Different platforms (ExoPlayer vs AVFoundation) handle track groups differently, but this was hidden in string encoding
Unintuitive API: Users had to remember the format and construction of track IDs
Interface Breakout PR From : #9925
Pre-Review Checklist
[shared_preferences]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].///).