[video_player] Updated Pigeon version #4726

gaaclarke · 2022-02-03T00:40:45Z

Updated video_player_android and video_player_avfoundation to use the latest version of pigeon.

Test exempt: Refactor, no new features.

Changes:

Pigeon now 1.0.x.
Constructors are now used in the Dart code.
Some generated files have the ".g." segment in their name (the java generator has bug where I couldn't add it, it would generate the class "Messages.g")

Pre-launch Checklist

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

flutter-dashboard · 2022-02-03T02:39:02Z

This pull request is not mergeable in its current state, likely because of a merge conflict. Pre-submit CI jobs were not triggered. Pushing a new commit to this branch that resolves the issue will result in pre-submit jobs being scheduled.

gaaclarke · 2022-02-03T02:41:48Z

I didn't look into the async or null safety usage, just getting everything updated. There were a few things that were had tweaked that we should fix in pigeon:

unnecessary_parenthesis linter failure in dart test file
import in dart test file is relative which messes the compiler up. it thinks there are 2 instances of the types.
generating a file named "Foo.g.java" makes class called "Foo.g"

stuartmorgan · 2022-02-03T03:49:25Z

packages/video_player/video_player_android/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.0
+
+* Update Pigeon to ^1.0.16.


Nit: Updates

Per repo CHANGELOG style guide.

fyi: The style guide just says it should be the present tense, it doesn't specify which conjugation should be used (ie 3rd person singular, neutral).

It doesn't just say it should be in present tense, it says it should be "present tense indicative". I assumed that the fact the package was the implied subject was obvious, but I'll add that.

It also specifically says:

For example, "Adds cool new feature.", not "Add" or "Added"

So I'm not clear how it's ambiguous that this should be "Updates" rather than "Update". But I'll add even more specific grammar notes there if you like.

packages/video_player/video_player_android/CHANGELOG.md

packages/video_player/video_player_android/pigeons/messages.dart

stuartmorgan · 2022-02-03T03:52:15Z

packages/video_player/video_player_avfoundation/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.0
+
+* Update Pigeon to 1.0.x.


Same version and verb form comments here.

done (conjugation)

packages/video_player/video_player_avfoundation/ios/Classes/messages.g.h

stuartmorgan · 2022-02-03T04:01:56Z

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

+ (FLTCreateMessage *)fromMap:(NSDictionary *)dict {
+  FLTCreateMessage *pigeonResult = [[FLTCreateMessage alloc] init];
+  pigeonResult.asset = dict[@"asset"];
+  if ((NSNull *)pigeonResult.asset == [NSNull null]) {


Pigeon fix (so, not for this PR): a simpler solution occurs to me for this type issue than local variables. You can generate a static helper C function at the top of this file and use it for the lookups:

NSObject *GetNullableObjectFromDictionary(NSDictionary *dict, NSString *key) { NSObject *value = dict[key]; return value == [NSNull null] ? nil : value; }

Then fromMap is just a series of:

pigeonResult.foo = GetNullableObjectFromDictionary(@"foo"); ...

statements. Shorter, and no type abuse.

Good call. I'll try to do some cleanup on the few pigeon things I saw today. I'll add that to the list.

stuartmorgan · 2022-02-03T04:03:34Z

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

+
+NSObject<FlutterMessageCodec> *FLTVideoPlayerApiGetCodec() {
+  static dispatch_once_t s_pred = 0;
+  static FlutterStandardMessageCodec *s_sharedObject = nil;


Pigeon fix: These names violate style; it should be sPred and sSharedObject. Hungarian-style prefixes (s, k) in ObjC still use normal naming rules.

gaaclarke · 2022-02-04T20:04:46Z

@stuartmorgan friendly ping, I think we are good here. I talked about updating pigeon but we've landed the fix in pigeon and we'll grab it the next time. The differences are trivial.

stuartmorgan · 2022-02-04T20:23:20Z

packages/video_player/video_player_android/pigeons/messages.dart

 class TextureMessage {
-  int textureId;
+  int? textureId;


Shouldn't textureId be non-null in all of these, now that that's supported?

I mentioned that in my first comment I didn't investigate async or nullability changes. I just want this PR to be an upgrade of pigeon, no semantics changes.

- // @dart = 2.9

This is a semantics change though.

It shouldn't be. In Pigeon if you switch to NNBD then make all types nullable the semantics are the same, the syntax is changed only. Pigeon is special since it doesn't allow logic, just declarations.

stuartmorgan · 2022-02-04T20:23:43Z

packages/video_player/video_player_android/pigeons/messages.dart

-  int textureId;
-  bool isLooping;
+  int? textureId;
+  bool? isLooping;


In fact, shouldn't all of the values in these classes be non-null?

stuartmorgan · 2022-02-04T20:24:19Z

packages/video_player/video_player_android/pigeons/messages.dart

+  String? uri;
+  String? packageName;
+  String? formatHint;
+  Map<String?, String?>? httpHeaders;


This should definitely be non-null so that we don't have the nil-vs-empty confusion.

You've marked this as nullable. It should be non-nullable (default empty) like the class it is mirroring.

packages/video_player/video_player_android/pigeons/messages.dart

packages/video_player/video_player_avfoundation/pigeons/messages.dart

gaaclarke · 2022-02-04T21:18:36Z

Hey @stuartmorgan , I wanted to address nullability and async changes in a different PR in order to separate the big pigeon upgrade from functionality changes.

stuartmorgan · 2022-02-04T21:40:28Z

I understand why async changes would be separate since they involve rewriting logic, but I don't understand why nullability would be separate. A core part of this PR is that you are upgrading messages.dart from pre-NNBD to post-NNBD. When we do NNBD updates, actually using the correct nullability semantics is a core part of that.

In fact, I would expect that not adding a bunch of unwanted nullability annotations would make this diff smaller.

gaaclarke · 2022-02-04T23:32:02Z

In fact, I would expect that not adding a bunch of unwanted nullability annotations would make this diff smaller.

Yea, probably. It's just that was explicitly listed as a non-goal of this PR in order to change less variables. Let me queue up another PR for you.

gaaclarke · 2022-02-04T23:54:14Z

@stuartmorgan I have a draft prepared for updating the nullability: #4742 I don't know if it's easy to see but there was code that had to be changed to accommodate the new semantics that didn't have to get changed in this PR since they were the same.

stuartmorgan · 2022-02-07T17:42:55Z

It's just that was explicitly listed as a non-goal of this PR in order to change less variables.

I understand that you wrote it down; that doesn't mean that it's actually desirable to have done so. Every single NNBD upgrade we did across flutter/plugin and flutter/packages involved auditing the desired nullability of each item and changing the code accordingly, not blindly adding ?s everywhere to preserve semantics. Preserving semantics is a non-goal of an NNBD migration.

There is no reason to make a Pigeon update different in that regard. We should not migrate a file from non-NNBD to NNBD without giving it the desired nullability semantics.

I don't know if it's easy to see but there was code that had to be changed to accommodate the new semantics

Yes, that's a normal part of an NNBD upgrade. (And it appears that they were incredibly minor, and primarily consistent of removing one ! in each package which is entirely positive and trivial to reason about.)

stuartmorgan

(Updating review status to reflect comments above, since I forgot to do that as a review)

gaaclarke · 2022-02-07T19:18:11Z

@stuartmorgan This isn't an NNBD upgrade, I know it looks like one but it isn't. We are only using Dart for the front-end compiler, not the backend. I've written the whole backend, this is not changing the semantics or the generated code. It's just changing how the Dart front-end works. We really should separate the change in semantics into a different PR.

stuartmorgan · 2022-02-07T19:35:17Z

This isn't an NNBD upgrade

I'm not talking about the package; I know that the package has already been converted. I'm talking about messages.dart, where you are making the following change:

-// @dart = 2.9

this is not changing the semantics or the generated code

I am aware that it is not changing the generated code, but it is changing the semantics of everything in that file from "we aren't making any claims about the intended nullability of these values" to "we are explicitly claiming that all of these things are intended to be nullable". Those new semantics are wrong, because those things are not all intended to be nullable.

I'm not going to approve a PR that explicitly puts the wrong (as defined by the desired behavior of the API layer) annotations in messages.dart just to preserve behavior that we don't want, any more than I would have approved a package-level NNBD update that did the same thing to the rest of the code.

gaaclarke · 2022-02-07T19:47:25Z

"we aren't making any claims about the intended nullability of these values"

I think that's where our disconnect is. That isn't what things meant before NNBD. It meant "all of my values are nullable". So the smallest change for upgrading would have been to keep those semantics "all of my values are nullable".

gaaclarke · 2022-02-08T22:17:48Z

I talked offline with stuart. Moving the fields from implicitly nullable to explicitly nullable means null values should be accommodated in the handlers. Instead of going through the work to make that happen it's easier just to do the migration to nonnull values now when that was their intended usage. So, while this is 2 big changes in one PR, it's less contradictory and more clear to land it in one PR.

gaaclarke · 2022-02-08T22:24:43Z

@stuartmorgan I've adopted the null-safety annotations that match the usage. Everything is non-null except for the fields in CreateMessage because that's how they are created in the dart side:

plugins/packages/video_player/video_player_android/lib/src/android_video_player.dart

Line 36 in 9d32c83

final CreateMessage message = CreateMessage();

and how they are used on the host side:

plugins/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java

Line 84 in 9d32c83

if (httpHeaders != null && !httpHeaders.isEmpty()) {

stuartmorgan · 2022-02-09T19:57:00Z

packages/video_player/video_player_android/pigeons/messages.dart

+  String? uri;
+  String? packageName;
+  String? formatHint;
+  Map<String?, String?>? httpHeaders;


You've marked this as nullable. It should be non-nullable (default empty) like the class it is mirroring.

stuartmorgan · 2022-02-09T20:00:13Z

packages/video_player/video_player_avfoundation/pigeons/messages.dart

+  String? uri;
+  String? packageName;
+  String? formatHint;
+  Map<String?, String?>? httpHeaders;


Done. FYI they were nonnull in the DataSource class, but in both the android implementation and the ios implementation they were nullable.

Yes, but the fact that it's traditionally been hard to reason about nullability across the platform channel boundary—leading to code that is either over-cautious like in this case, or not cautious enough and buggy as in other cases—is exactly why I think Pigeon NNBD support is a substantial opportunity.

stuartmorgan · 2022-02-14T20:07:43Z

packages/video_player/video_player_android/pigeons/messages.dart

@@ -43,11 +43,12 @@ class PositionMessage {
 }

 class CreateMessage {
+  CreateMessage({required this.httpHeaders});


Why is this required, rather than defaulting to an empty collection?

Pigeon demands there be no logic here, it doesn't generate code for initialization.

Filed flutter/flutter#98448

stuartmorgan · 2022-02-14T20:08:41Z

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

@@ -177,7 +177,7 @@ - (void)createVideoOutputAndDisplayLink:(FLTFrameUpdater *)frameUpdater {

 - (instancetype)initWithURL:(NSURL *)url
               frameUpdater:(FLTFrameUpdater *)frameUpdater
-                httpHeaders:(NSDictionary<NSString *, NSString *> *)headers {
+                httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers {
  NSDictionary<NSString *, id> *options = nil;
  if (headers != nil && [headers count] != 0) {


The headers != nil can be removed.

(Technically it could have been removed either way since [nil count] is zero, but it should definitely be removed now.)

stuartmorgan

LGTM given the current Pigeon limitations.

* google/master: (340 commits) [camera]remove "selfRef" for SavePhotoDelegate and ensure thread safety (flutter#4780) Roll Flutter from 919d205 to adafd66 (5 revisions) (flutter#4876) [google_sign_in] Update platform interface analysis options (flutter#4872) Roll Flutter from b623279 to 919d205 (2 revisions) (flutter#4871) Roll Flutter from 14a2b13 to b623279 (5 revisions) (flutter#4870) [image_picker] Update platform interface analysis options (flutter#4837) [ci] Re-enable stable webview Android tests (flutter#4867) Roll Flutter from 286c975 to 14a2b13 (1 revision) (flutter#4865) [local_auth] support localizedFallbackTitle in IOSAuthMessages (flutter#3806) [image_picker] Update app-facing and web analysis options (flutter#4838) Roll Flutter from 8386344 to 286c975 (4 revisions) (flutter#4864) Roll Flutter from e9f83cf to 8386344 (6 revisions) (flutter#4862) [camera] Switch web package to new analysis options (flutter#4834) [flutter_plugin_tool] Fix iOS/macOS naming (flutter#4861) [webview_flutter] Fix debuggingEnabled on Android (flutter#4859) Roll Flutter from ca2a751 to e9f83cf (10 revisions) (flutter#4857) fix license (flutter#4858) [video_player] add allowBackgroundPlayback option platform interface changes (flutter#4807) [video_player] Updated Pigeon version (flutter#4726) Roll Flutter from 381cb28 to ca2a751 (5 revisions) (flutter#4850) ... # Conflicts: # packages/camera/camera/CHANGELOG.md # packages/camera/camera/android/build.gradle # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java # packages/camera/camera/android/src/main/java/io/flutter/plugins/camera/MethodCallHandlerImpl.java # packages/camera/camera/ios/Classes/CameraPlugin.m # packages/camera/camera/ios/camera.podspec # packages/camera/camera/lib/camera.dart # packages/camera/camera/lib/src/camera_controller.dart # packages/camera/camera/pubspec.yaml

github-actions bot added p: video_player platform-ios labels Feb 3, 2022

gaaclarke force-pushed the upgrade-pigeon branch 6 times, most recently from acc1d24 to 35e22a9 Compare February 3, 2022 01:17

github-actions bot added the platform-android label Feb 3, 2022

gaaclarke force-pushed the upgrade-pigeon branch 2 times, most recently from 697b1b8 to a7e195b Compare February 3, 2022 02:28

gaaclarke marked this pull request as ready for review February 3, 2022 02:39

gaaclarke requested a review from stuartmorgan February 3, 2022 02:39

gaaclarke requested a review from bparrishMines February 3, 2022 02:42

stuartmorgan suggested changes Feb 3, 2022

View reviewed changes

gaaclarke force-pushed the upgrade-pigeon branch from 86c87bc to 76476fe Compare February 3, 2022 21:43

gaaclarke requested a review from stuartmorgan February 3, 2022 21:45

gaaclarke mentioned this pull request Feb 3, 2022

[pigeon] a few tweaks to the output after having upgraded video_player flutter/packages#663

Merged

11 tasks

gaaclarke force-pushed the upgrade-pigeon branch from 76476fe to 0145c0c Compare February 4, 2022 20:03

stuartmorgan mentioned this pull request Feb 4, 2022

[pigeon] Add correctly named convenience constructors for ObjC classes flutter/flutter#97832

Open

stuartmorgan suggested changes Feb 4, 2022

View reviewed changes

gaaclarke requested a review from stuartmorgan February 4, 2022 21:18

stuartmorgan suggested changes Feb 7, 2022

View reviewed changes

stuartmorgan mentioned this pull request Feb 8, 2022

[video_player] upgraded pigeon to 1.0 #4277

Closed

11 tasks

gaaclarke force-pushed the upgrade-pigeon branch 5 times, most recently from e3125dd to 0be5dc9 Compare February 8, 2022 22:07

gaaclarke requested a review from stuartmorgan February 8, 2022 22:25

stuartmorgan suggested changes Feb 9, 2022

View reviewed changes

gaaclarke added 4 commits February 14, 2022 11:24

[video_player] Updated Pigeon version

84f0aad

updated android

ee7ce00

updated the changelog messages

d9a9307

[video_player] updated nullability in pigeon files

c702665

gaaclarke force-pushed the upgrade-pigeon branch from 0be5dc9 to c702665 Compare February 14, 2022 19:24

made httpheaders not null

32bb3f8

gaaclarke requested a review from stuartmorgan February 14, 2022 19:50

stuartmorgan suggested changes Feb 14, 2022

View reviewed changes

gaaclarke requested a review from stuartmorgan February 14, 2022 22:06

removed nil check and switched away from init

df6a66a

gaaclarke force-pushed the upgrade-pigeon branch from 399cc68 to df6a66a Compare February 14, 2022 22:18

stuartmorgan approved these changes Feb 15, 2022

View reviewed changes

gaaclarke merged commit 10a59af into flutter:main Feb 15, 2022

engine-flutter-autoroll mentioned this pull request Feb 16, 2022

Roll Plugins from c48a5f87757d to 136f49b3e8ec (9 revisions) flutter/flutter#98586

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2022

10a59af20 [video_player] Updated Pigeon version (flutter/plugins#4726)

6d11a86

[video_player] Updated Pigeon version #4726

[video_player] Updated Pigeon version #4726

Conversation

gaaclarke commented Feb 3, 2022 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Feb 3, 2022

gaaclarke commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke commented Feb 4, 2022

stuartmorgan commented Feb 4, 2022

gaaclarke commented Feb 4, 2022

gaaclarke commented Feb 4, 2022

stuartmorgan commented Feb 7, 2022

stuartmorgan left a comment

Choose a reason for hiding this comment

gaaclarke commented Feb 7, 2022

stuartmorgan commented Feb 7, 2022

gaaclarke commented Feb 7, 2022

gaaclarke commented Feb 8, 2022

gaaclarke commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

gaaclarke commented Feb 3, 2022 •

edited

Loading

stuartmorgan Feb 4, 2022 •

edited

Loading