[video_player] Add v2 embedding support #2226

mklim · 2019-10-21T18:16:10Z

Description

Adds support in the plugin
Adds a v2 embedding to the example app
Fixes a broken remote example in the example app
Increments the Flutter SDK dependency
Increments the version
Adds e2e tests for some simple use cases of the plugin

Related Issues

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.

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.

cyanglaz

LGTM module nits

cyanglaz · 2019-10-22T16:08:14Z

...ges/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java

-      return exoPlayer.getCurrentPosition();
-    }
+  /** Register this with the v2 embedding for the plugin to respond to lifecycle callbacks. */
+  public VideoPlayerPlugin() {}


Seems not necessary

Yeah, it's redundant. I think I'm fine having this just as a place to hang the documentation on though.

cyanglaz · 2019-10-22T16:19:23Z

packages/video_player/example/android/app/src/main/AndroidManifest.xml

+    <activity
+      android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection"
+      android:hardwareAccelerated="true"
+      android:launchMode="singleTop"


not necessary per Matt's suggestion. #2164 (comment)

cyanglaz · 2019-10-22T16:46:42Z

packages/video_player/example/pubspec.yaml

@@ -8,7 +8,9 @@ dependencies:
 dev_dependencies:
  flutter_test:
    sdk: flutter
-
+  flutter_driver:


missing flutter version constraints for example app.

Not needed here since it matches the Flutter SDK constraints of the plugin itself.

xster · 2019-10-29T20:19:24Z

Can you sync your PR to head again?

mklim

Can you sync your PR to head again?

Done.

mklim · 2019-10-29T22:06:05Z

packages/video_player/example/android/app/src/main/AndroidManifest.xml

+    <activity
+      android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale|layoutDirection"
+      android:hardwareAccelerated="true"
+      android:launchMode="singleTop"


mklim · 2019-10-29T22:06:23Z

packages/video_player/example/pubspec.yaml

@@ -8,7 +8,9 @@ dependencies:
 dev_dependencies:
  flutter_test:
    sdk: flutter
-
+  flutter_driver:


Not needed here since it matches the Flutter SDK constraints of the plugin itself.

mklim · 2019-10-29T22:07:20Z

...ges/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java

-      return exoPlayer.getCurrentPosition();
-    }
+  /** Register this with the v2 embedding for the plugin to respond to lifecycle callbacks. */
+  public VideoPlayerPlugin() {}


Yeah, it's redundant. I think I'm fine having this just as a place to hang the documentation on though.

xster

LGTM

xster · 2019-11-11T20:50:01Z

Is it just broken tests waiting for this?

mklim · 2019-11-11T20:56:42Z

@xster yeah. The plugin code itself LG, but something about the driver tests are failing on iOS devices in CI. I don't usually develop on macOS devices so I haven't taken the time to triage whether it's a CI specific issue or an iOS specific issue.

It's possible that the tests are either catching a real bug or the tests themselves are just not written well. I think the latter is more likely. I wrote them to hit "play" and then wait for a few hundred milliseconds and then hit "pause" and make sure that their current video position isn't 0:00, so it could be that the tests aren't waiting long "enough." A better way to write the tests would be to actually have a future that waits for the current video position to increment past 0:00 and then immediately stops (with a generous upper bound timeout, so that the test doesn't completely hang).

- Migrates the plugin - Adds a v2 embedding to the example app - Fixes a broken remote example in the example app - Increments the Flutter SDK dependency - Increments the version - Adds e2e tests for some simple use cases of the plugin

mklim · 2019-11-12T21:18:14Z

These tests pass locally on iOS for me. I'm not sure why they're failing in CI. I'm tempted to skip them on iOS with a TODO for now but I don't think that's a great solution.

@collinjackson do you know any reasons why iOS tests around playing videos would fail in CI?

@xster do you have opinions with skipping the driver tests on iOS until we root cause why they're failing in CI?

mklim · 2019-11-13T18:54:53Z

I'm tentatively skipping the problem tests on iOS for now.

- Adds support in the plugin - Adds a v2 embedding to the example app - Fixes a broken remote example in the example app - Increments the Flutter SDK dependency - Increments the version - Adds e2e tests for some simple use cases of the plugin

* newCache/blackVideos: (105 commits) [connectivity] Fix reachability stream for iOS (flutter#2281) [google_sign_in] Port plugin to use the federated Platform Interface (flutter#2266) [webview_flutter] Add async NavigationDelegates (flutter#2257) Update cirrus to create IOS simulator on 13.2 an xCode 11 (flutter#2275) [battery] Update and migrate iOS example project (flutter#2089) [url_launcher] DartDoc and test improvements (flutter#2274) [google_maps_flutter] Define clang module for iOS, fix analyzer warnings (flutter#2182) [video_player] Add v2 embedding support (flutter#2226) [path_provider] Update and migrate iOS example project (flutter#2099) [sensors] Documentation and test improvements (flutter#2272) [image_picker] Lint for public DartDocs (flutter#2270) [connectivity] Lint for public DartDocs (flutter#2269) [shared_preferences] Update and migrate iOS example project (flutter#2103) [quick_actions] Update and migrate iOS example project (flutter#2100) [infra] Ignore analyzer issues in CI (flutter#2271) [android_intent] Add missing DartDocs (flutter#2268) Bump google_maps_flutter pubspec to match CHANGELOG (flutter#2267) [google_sign_in] Handle new style URLs in GoogleUserCircleAvatar (flutter#2252) [google_sign_in] Move plugin to its subdir to allow for federated implementations. Add platform interface. (flutter#2244) Make setMockInitialValues handle non-prefixed keys (flutter#2260) ... # Conflicts: # packages/video_player/CHANGELOG.md # packages/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java # packages/video_player/ios/video_player.podspec # packages/video_player/pubspec.yaml # packages/video_player/test/video_player_test.dart

- Adds support in the plugin - Adds a v2 embedding to the example app - Fixes a broken remote example in the example app - Increments the Flutter SDK dependency - Increments the version - Adds e2e tests for some simple use cases of the plugin

mklim requested a review from cyanglaz October 21, 2019 18:16

mklim requested a review from iskakaushik as a code owner October 21, 2019 18:16

googlebot added the cla: yes label Oct 21, 2019

cyanglaz approved these changes Oct 22, 2019

View reviewed changes

mklim commented Oct 29, 2019

View reviewed changes

xster reviewed Oct 30, 2019

View reviewed changes

Michael Klimushyn added 6 commits November 12, 2019 12:24

Move VideoPlayer into its own top level class

208060e

[video_player] Migrate to the v2 embedding

0bfed5b

- Migrates the plugin - Adds a v2 embedding to the example app - Fixes a broken remote example in the example app - Increments the Flutter SDK dependency - Increments the version - Adds e2e tests for some simple use cases of the plugin

Review feedback and time updates

b54566e

Improve test

b351f13

Speculative test fix

5297968

Await driver.close()

88c174f

Skip tests that involve playing on iOS

267812f

mklim merged commit a336b28 into flutter:master Nov 13, 2019

mklim deleted the migrate_video_player branch November 13, 2019 20:12

[video_player] Add v2 embedding support #2226

[video_player] Add v2 embedding support #2226

Uh oh!

Conversation

mklim commented Oct 21, 2019

Description

Related Issues

Checklist

Breaking Change

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster commented Oct 29, 2019

Uh oh!

mklim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

xster commented Nov 11, 2019

Uh oh!

mklim commented Nov 11, 2019

Uh oh!

mklim commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mklim commented Nov 13, 2019

Uh oh!

Uh oh!

mklim commented Nov 12, 2019 •

edited

Loading