Skip to content
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

[video_player_platform_interface] Platform view support #8453

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

FirentisTFW
Copy link
Contributor

@FirentisTFW FirentisTFW commented Jan 17, 2025

This PR is a subset of #8237 - it only adds platform interface changes.

Partially resolves 86613.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@FirentisTFW
Copy link
Contributor Author

FirentisTFW commented Jan 17, 2025

@stuartmorgan This PR changes the perception a little bit - we now should use player ID in app-facing package, and have no knowledge about texture ID there. How should I go about renaming textureId to playerId? textureId is currently used for parameter names in many platform interface methods (e.g. play, pause, setVolume). Then, it's also used for helper variables, in tests, and in other files.

What would be best way to rename it in all packages without breaking the static analysis in the meantime? What kind of changes should I add to this PR? Rename it in platform interface? Rename it everywhere? We're currently in an ugly state where we allow using platform views, but we still use textureId for parameter names everywhere.

@stuartmorgan
Copy link
Contributor

I think the best way to do this is probably:

  1. In this PR, rename it throughout the platform interface
  2. In follow-up PRs (for iOS and Android it should just be part of the PRs that add platform view support), rename it in the implementation packages.
  3. When updating app-facing package calls, like create and buildView, to the new versions, update the name there.

In order to pass CI you'll need to add // ignore directives to all the implementation packages as part of this PR, and then remove them as the implementations are updated in step 2.

@FirentisTFW FirentisTFW force-pushed the feature/video-player-platform-view-support-platform-interface branch 2 times, most recently from af483a1 to 118447c Compare January 20, 2025 09:29
@FirentisTFW
Copy link
Contributor Author

@stuartmorgan As suggested, in this PR I renamed parameters in the platform interface and added ignores to the package implementations. The checks on the CI now fail, but I assume that this could be resolved with override: no versioning needed label?

@FirentisTFW FirentisTFW marked this pull request as ready for review January 20, 2025 09:46
@stuartmorgan
Copy link
Contributor

The version failures we can override, but the other failure is (by design) not overridable. In this case it's a tool bug, so #8468 will need to land and then this rebased on that; sorry about that.

@stuartmorgan stuartmorgan added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Jan 21, 2025
@stuartmorgan
Copy link
Contributor

Version/changelog override: changes to packages that don't have a version bump are local-dev-only, so are exempt.

@FirentisTFW FirentisTFW force-pushed the feature/video-player-platform-view-support-platform-interface branch from 118447c to ca5dfb5 Compare January 22, 2025 08:20
@FirentisTFW
Copy link
Contributor Author

@stuartmorgan I see that the other PR was already merged, so I rebased this one. One check failed, but looks like it's infra failure, so I t think it can be re-run

@@ -44,67 +44,81 @@ abstract class VideoPlayerPlatform extends PlatformInterface {
}

/// Clears one video.
Future<void> dispose(int textureId) {
Future<void> dispose(int playerId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is playerId used for both textureId (for texture based implementation) and playerId (for platform view implementation)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
override: no changelog needed Override the check requiring CHANGELOG updates for most changes override: no versioning needed Override the check requiring version bumps for most changes p: video_player platform-android platform-ios platform-macos platform-web
Projects
None yet
3 participants