[video_player] upgraded video_player to use pigeon #2544

gaaclarke · 2020-02-20T20:13:40Z

Description

Upgraded to use pigeon, parent commit: #2617

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.

xster

Some notes but this is looking great!!

xster · 2020-03-17T22:28:01Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+public class Messages {
+
+  /** Generated class from Pigeon that represents data sent in messages. */
+  public static class VoidMessage {


ya, supporting null would be great

xster · 2020-03-17T22:53:17Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+  }
+
+  /** Generated class from Pigeon that represents data sent in messages. */
+  public static class TextureMessage {


This is more of a comment for https://github.com/flutter/plugins/pull/2544/files#diff-eba6bc6c27f777f5309e5a219983ec11R34 but I'm here now. This seems like a good place to demonstrate "primitives" since the original implementation for java is just a long too. Could the IDL just be int create(CreateMessage msg);?

xster · 2020-03-17T23:56:09Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+    public Long getTextureId() { return textureId; }
+    public void setTextureId(Long setterArg) { this.textureId = setterArg; }
+
+    HashMap toMap() {


Meta-context for the comment above, one of the struggles big apps tend to get into is once you unpack the APK, 1/3 of the APK size is from the proto generated outputs with a bunch of helper functions. I don't think we'll run into it any time soon but it's good to let primitives be primitives where possible.

xster · 2020-03-17T23:58:54Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+  public static class CreateMessage {
+    private String asset;
+    public String getAsset() { return asset; }
+    public void setAsset(String setterArg) { this.asset = setterArg; }


I think the java part wasn't in the original doc but looking at this again, could we default to immutables only (fields can only be set via constructor (fromMap in this case) unless specifically annotated?

xster · 2020-03-18T00:01:27Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+    public String getFormatHint() { return formatHint; }
+    public void setFormatHint(String setterArg) { this.formatHint = setterArg; }
+
+    HashMap toMap() {


I'm thinking, since the APIs are directional in the IDL, this should theoretically never be needed right? Could we omit this by default unless we annotate it somehow?

xster · 2020-03-18T00:36:25Z

packages/video_player/video_player/pigeons/messages.dart

+  VoidMessage pause(TextureMessage msg);
+}
+
+void setupPigeon(PigeonOptions opts) {


So in this whole file, only the import line and this function are required, everything else is freeform right? I wish we can somehow make this distinction clearer. Maybe we need a pub run to generate the dart template and this is pre-filled with a big dartdoc saying you need to fill this.

This function isn't required. All of these flags can be supplied at command-line. I added this function so people can save arguments.

right, understood.

1- I just meant how can we provide a clearer affordance to this thing? Maybe just a doc in the readme is ok. But since we're telling people to create this file from scratch, it would be good to have some template available.

2- kinda unrelated, can we tweak the name to not have setup in there? The generated java/objc code has the word setup in the static functions too and they mean pretty different things than this setup. Could be confusing if people cognitively mixed those things. Maybe just configurePigeon or some such.

xster · 2020-03-18T00:37:15Z

packages/video_player/video_player/pubspec.yaml

-  video_player_platform_interface: ^1.0.1
+  video_player_platform_interface:
+    path: ../video_player_platform_interface
  # The design on https://flutter.dev/go/federated-plugins was to leave


your change invalidates this comment right?

xster · 2020-03-18T00:37:35Z

packages/video_player/video_player/pubspec.yaml

    sdk: flutter
  pedantic: ^1.8.0
+  pigeon:
+    path: ../../../../packages/packages/pigeon


We can probably just reference a version here right? If we were to actually check it in.

I'll change that when i switch from being a draft.

xster · 2020-03-18T00:39:15Z

packages/video_player/video_player_platform_interface/lib/messages.dart

+    } else {
+      return VoidMessage._fromMap(replyMap['result']);
+    }
+


nit: some whitespace unbalance here

xster · 2020-03-18T00:41:06Z

packages/video_player/video_player_platform_interface/lib/messages.dart

+class VideoPlayerApi {
+  Future<VoidMessage> initialize(VoidMessage arg) async {
+    Map requestMap = arg._toMap();
+    BasicMessageChannel channel =


I guess this part is a bit different from the receiving side. I get the ergonomics of it but I wonder what's the cost (if a function is invoked on each frame). Is it reasonable to save it somewhere?

gaaclarke · 2020-03-19T22:31:21Z

@xster the Android and iOS example apps both work now, yay.

xster

LGTM

xster · 2020-05-05T19:48:04Z

...video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

+      return asset;
+    }
+
+    public void setAsset(String setterArg) {


forgot if we talked about this before. Perhaps for a v1.1. Can't all these be construction arguments rather than setters? Since the "receiver" can theoretically take this object as is and just pass it around deeper into their application as immutable data. It's harder to do if it's mutable.

Good idea I've considered it i n the past, just haven't had the time: flutter/flutter#56496

Since java doesn't have named parameters i've never prioritized it to keep the generated code similar across platforms.

xster · 2020-05-05T19:56:24Z

packages/video_player/video_player/pigeons/messages.dart

@@ -0,0 +1,50 @@
+import 'package:pigeon/pigeon_lib.dart';


nit, the convention is to have the main dart file be the same name as the package, e.g. package:pigeon/pigeon.dart

flutter/flutter#56495

cyanglaz · 2020-05-06T17:51:10Z

packages/video_player/video_player/ios/Classes/FLTVideoPlayerPlugin.m

  FLTVideoPlayerPlugin* instance = [[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];
-  [registrar addMethodCallDelegate:instance channel:channel];
  [registrar publish:instance];
+  FLTVideoPlayerApiSetup(registrar.messenger, instance);


I wonder if we can make the requirement of calling this function more explicit.
What is the current failure message if the user didn't call this before using the pigeon generated code?

It's the same error then if someone sent a message to a channel that hasn't had a callback set for it. It doesn't say anything specifically about Pigeon. Thats good feedback. I'll look into that for future versions.

Sure. I guess documentations on the method would also help in some patch release.

flutter/flutter#56497

cyanglaz · 2020-05-06T17:57:54Z

packages/video_player/video_player/ios/Classes/messages.m

+ (FLTTextureMessage *)fromMap:(NSDictionary *)dict {
+  FLTTextureMessage *result = [[FLTTextureMessage alloc] init];
+  result.textureId = dict[@"textureId"];
+  if ((NSNull *)result.textureId == [NSNull null]) {


nits:
use if (![result.textureId iSKindOfClass:[NSNumber class]])
would also check the the type.
Or if we want to be more explicit about plugin passing the wrong type, we can add

if (![result.textureId iSKindOfClass:[NSNumber class]]) { throw... }

cyanglaz · 2020-05-06T17:59:20Z

packages/video_player/video_player/ios/Classes/messages.m

+}
+- (NSDictionary *)toMap {
+  return
+      [NSDictionary dictionaryWithObjectsAndKeys:(self.textureId ? self.textureId : [NSNull null]),


nits: I personally prefer

@{ @"textureId" : self.textureId ? self.textureId : [NSNull null]) };

Good call. I would update this but since it is part of the generated code it is really an issue with pigeon, not this PR. That requires republishing pigeon, since it's a nit I'll address it later if you don't mind.

packages/video_player/video_player/ios/Classes/messages.m

cyanglaz · 2020-05-06T23:50:17Z

packages/video_player/video_player/pubspec.yaml

 description: Flutter plugin for displaying inline video with other Flutter
  widgets on Android and iOS.
-version: 0.10.9+1
+version: 0.10.10


Does this PR change the public api of the plugin?
We usually only update minors when there are non-breaking public api changes.

It doesn't change the API. It isn't a breaking change in that sense. It adds no new functionality from the users perspective.

if it doesn't add anything from users perspective, we should change it to 0.10.9+2 so users
who depends on video_player: ^0.10.9^ can get this update.

My understanding is that the "+N" nomenclature is for changes that don't change the binary. This doesn't change the intention of the code, but the binary is definitely changed.

cyanglaz

LGTM

…afterward.

gaaclarke · 2020-05-08T05:51:54Z

Landing on red, the only breaking CI is submit-queue, this should fix it. @cyanglaz I had to make a few changes after your LGTM, but they were all fixing tests in case you want to review them.

* master: (96 commits) Update README.md (flutter#2768) [url_launcher_web] Launch mailto urls in same window in Safari (flutter#2740) update README with enableJavaScript info (flutter#2766) Run publish ci check on master (flutter#2764) [image_picker] Add documentation for Android external storage permissions (flutter#2765) [package_info] add support for macos to package_info plugin (flutter#2618) fixed detach from engine logic (flutter#2759) [url_launcher] Initialize previousAutomaticSystemUiAdjustment in launch (flutter#2757) [google_maps_flutter] add todo on skipped test. (flutter#2752) [google_maps_flutter] use `WaitUntilTouchesEndedPolicy` to fix the cameraIdle not called issue on iOS (flutter#2746) Use Xvfb for Linux desktop tests (flutter#2750) update lower dart bound to 2.1.0 (flutter#2751) [camera] Update lower bound of dart dependency to 2.1.0. (flutter#2749) [battery] update dart deps lower bound to 2.1.0 (flutter#2748) [android_alarm_manager] update dart deps lower bound to 2.1.0 (flutter#2747) [url_launcher] Add web to example app. (flutter#2736) [in_app_purchase] update docs to warn about `completePurchase` (flutter#2739) [video_player] upgraded video_player to use pigeon (flutter#2544) [video_player]: fixed platform_interface unit tests (flutter#2745) [video_player]: added test class to fix video_player unit tests (flutter#2744) ... # Conflicts: # packages/quick_actions/ios/Classes/FLTQuickActionsPlugin.m

googlebot added the cla: yes label Feb 20, 2020

gaaclarke force-pushed the video-player-pigeon branch 4 times, most recently from 8c4c695 to 0f60ba4 Compare February 21, 2020 18:47

gaaclarke force-pushed the video-player-pigeon branch 2 times, most recently from 0a689b7 to e728df8 Compare March 17, 2020 21:29

xster reviewed Mar 18, 2020

View reviewed changes

gaaclarke mentioned this pull request Mar 23, 2020

[video-player] upgraded video_player_interface to use pigeon #2617

Merged

13 tasks

gaaclarke force-pushed the video-player-pigeon branch 2 times, most recently from fc5eddf to d79ff02 Compare March 23, 2020 22:11

gaaclarke force-pushed the video-player-pigeon branch 4 times, most recently from 99e6b0b to ed4b42b Compare April 28, 2020 22:01

xster reviewed May 5, 2020

View reviewed changes

cyanglaz reviewed May 6, 2020

View reviewed changes

cyanglaz approved these changes May 6, 2020

View reviewed changes

gaaclarke force-pushed the video-player-pigeon branch 2 times, most recently from a29a04b to d3ce457 Compare May 7, 2020 17:58

gaaclarke marked this pull request as ready for review May 7, 2020 17:58

gaaclarke mentioned this pull request May 7, 2020

[video_player]: updated video_player_web to use the new video_player_platform_interface #2741

Merged

13 tasks

gaaclarke force-pushed the video-player-pigeon branch from 5722e2a to 4d3751d Compare May 7, 2020 21:28

gaaclarke mentioned this pull request May 8, 2020

[video_player]: added test class to fix video_player unit tests #2744

Merged

13 tasks

gaaclarke force-pushed the video-player-pigeon branch from 32f5b25 to dab2092 Compare May 8, 2020 04:37

gaaclarke mentioned this pull request May 8, 2020

[video_player]: fixed platform_interface unit tests #2745

Merged

13 tasks

gaaclarke added 2 commits May 7, 2020 22:37

[video_player] upgraded video_player to use pigeon

696ac24

Hand massaged the generated code to pass linting, i'll update pigeon …

f80aa8c

…afterward.

gaaclarke added 4 commits May 7, 2020 22:37

ran formatter

9b1792d

updated dart version

f73b903

fixed the controller tests

421aa34

removed accidental diff

d431937

gaaclarke force-pushed the video-player-pigeon branch from dab2092 to d431937 Compare May 8, 2020 05:37

gaaclarke merged commit c65bdd0 into flutter:master May 8, 2020

stuartmorgan-g pushed a commit to stuartmorgan-g/plugins that referenced this pull request May 11, 2020

[video_player] upgraded video_player to use pigeon (flutter#2544)

52e301b

EdwinRomelta pushed a commit to EdwinRomelta/plugins that referenced this pull request Jun 11, 2020

[video_player] upgraded video_player to use pigeon (flutter#2544)

e3607ce

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[video_player] upgraded video_player to use pigeon (flutter#2544)

36c7c15

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[video_player] upgraded video_player to use pigeon (flutter#2544)

07adeca

[video_player] upgraded video_player to use pigeon #2544

[video_player] upgraded video_player to use pigeon #2544

Uh oh!

Conversation

gaaclarke commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

xster 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!

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!

gaaclarke commented Mar 19, 2020

Uh oh!

xster 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!

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!

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!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented May 8, 2020

gaaclarke commented Feb 20, 2020 •

edited

Loading