Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Add platform interface #2273

Merged

Conversation

cbenhagen
Copy link
Contributor

@cbenhagen cbenhagen commented Nov 13, 2019

Description

Adds platform interface in preparation for federated plugins. Second part of this is here: #2276

Related Issues

#2041

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@cbenhagen
Copy link
Contributor Author

@hterkelsen PTAL

@cbenhagen cbenhagen force-pushed the video-player-platform-interface branch from f84205b to 074afdb Compare November 13, 2019 19:20
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

Thanks, Ben!

@harryterkelsen
Copy link
Contributor

Also, I don't see anything here about the stream of VideoPlayerValues. Shouldn't that be a part of the platform interface? Otherwise all implementations will be forced to use EventChannel which is basically the same as locking them into the MethodChannelVideoPlayer platform

@cbenhagen
Copy link
Contributor Author

True, I guess it should expose a stream?

Also there are some other things I am not too happy about:

  • the create method currently gets a Map. Would it be nicer if it would get a DataSourceDescription class?
  • There is a seekTo method which accepts int milliseconds. But a getPosition method which returns a Duration. What if I change seekTo to setPosition which also gets a Duration?

@harryterkelsen
Copy link
Contributor

Yeah, I think you should expose a Stream<VideoPlayerValue>. Maybe in the platform interface you can have something like Stream<VideoPlayerValue> playerValuesFor(int textureId).

As for changing the Map parameter of create to a DataSource class, I like that idea. I see the maps as basically an implementation detail of the MethodChannel platform implementation.

I think changing seekTo to take a Duration makes sense because that maps closer to how it is called from video_player, but I don't think we need to change the name since 'seekTo' is the name of the method we call on the MethodChannel implementation

@cbenhagen
Copy link
Contributor Author

cbenhagen commented Nov 13, 2019

Exposing a Stream<VideoEvent> would be much closer to what we have now. I will implement it this way first. Tell me if you really want me to change this to a Stream<VideoPlayerValue>.

@harryterkelsen
Copy link
Contributor

Stream<VideoEvent> SGTM. We can let the video_player package turn those into VideoPlayerValues

@cbenhagen cbenhagen force-pushed the video-player-platform-interface branch from c37aec4 to d6b75e7 Compare November 13, 2019 23:52
@cbenhagen cbenhagen force-pushed the video-player-platform-interface branch from d6b75e7 to 1ec7ca2 Compare November 13, 2019 23:52
@cbenhagen
Copy link
Contributor Author

cbenhagen commented Nov 13, 2019

Stream<VideoEvent> has been added. If the tests succeed I think this could be merged and the platform interface published. Improving the create method could also be done in the second PR. Thank you very much for your help so far @hterkelsen!

Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@cbenhagen
Copy link
Contributor Author

Let's make sure we add buildView to the interface before this gets merged.

@cbenhagen
Copy link
Contributor Author

Any ideas why the builds are failing now? The e2e tests are not touching anything I changed.

@harryterkelsen
Copy link
Contributor

Yeah, very odd. It seems like some of the shards are running that same test and passing... I'll try rerunning them and see if they deflake or if there's another problem

@cbenhagen
Copy link
Contributor Author

Some more of them went green. Very odd.. Can this be merged without them passing?

@cbenhagen
Copy link
Contributor Author

@hterkelsen Can you restart the missing checks please? Is there anything else missing from my side to get this merged?

@harryterkelsen
Copy link
Contributor

I restarted them. If more pass and some are still flaky I think we should still submit. I don't think this ought to block you

@cbenhagen
Copy link
Contributor Author

Only 2 left. I guess it could be merged. Can you do this or do we need another review?

@harryterkelsen
Copy link
Contributor

Sorry, I should have just merged this in earlier. It looks like there are new merge conflicts. If you resolve them and push the changes I think we can just land this.

@harryterkelsen
Copy link
Contributor

BTW I added this tracking issue for video_player: flutter/flutter#45290

I also created this project for tracking the state of implementation for the various plugins: https://github.com/flutter/flutter/projects/69

# Conflicts:
#	packages/video_player/video_player/CHANGELOG.md
#	packages/video_player/video_player/pubspec.yaml
@cbenhagen
Copy link
Contributor Author

No worries. I merged master. There haven't been any big conflicts fortunately.

@harryterkelsen
Copy link
Contributor

Thanks! Are you able to merge? If not I can merge once the tests run (and after double checking any failures for if they are those iOS flaky tests)

@cbenhagen
Copy link
Contributor Author

No I don't have write access to this repo.

@cbenhagen
Copy link
Contributor Author

@hterkelsen "All checks have passed" :)

@harryterkelsen harryterkelsen merged commit cba3932 into flutter:master Nov 21, 2019
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
* Move plugin to subdir

* Add video_player_platform_interface

* Make analyzer happy

* Improve documentation

* Bump version and update changelog

* Add stream of VideoEvent

* Ignore deprecated_member_use

* Improve changelog message and version

* Use DataSource class

* Add duration, size and buffering to VideoEvent

* Seek to Duration

* Fix buffering update

* Adapt docstring

* Add buildView
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants