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

[video_player] upgraded pigeon to 1.0 #4277

Closed
wants to merge 2 commits into from

Conversation

gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Aug 26, 2021

change to video_player_platform_interface that must merge first: #4291

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • 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 updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

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

@google-cla google-cla bot added the cla: yes label Aug 26, 2021
@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch 5 times, most recently from 60ffbb4 to b916bb9 Compare August 27, 2021 18:29
@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch 2 times, most recently from 9db485a to ff0e42d Compare August 27, 2021 22:15
@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch 3 times, most recently from 36f4bd7 to daf130e Compare August 27, 2021 22:50
@gaaclarke
Copy link
Member Author

Locally testing is working on iOS.

@gaaclarke
Copy link
Member Author

@stuartmorgan Am I able to land this update atomically or am I going to have to separate the video_player_platform_interface changes into a separate PR that I land first?

@gaaclarke
Copy link
Member Author

There is one issue with the Android generated code:

/Users/aaclarke/dev/plugins/packages/video_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java:146: error: incompatible types: Map<Object,Object> cannot be converted to Map<String,String>
      Map<String, String> httpHeaders = arg.getHttpHeaders();
                                                          ^
1 error

Looks like the template parameters didn't get brought over, the generated code has that method returning Map<Object, Object>.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Am I able to land this update atomically or am I going to have to separate the video_player_platform_interface changes into a separate PR that I land first?

The latter; there is no such thing as an atomic change to multiple packages because they have to be published individually. We also have a repo policy against publishing any untested configuration, and it's impossible to have the dependent packages go through CI as they would be published until the dependencies are published.

@@ -511,7 +511,7 @@ - (FLTTextureMessage*)onPlayerSetup:(FLTVideoPlayer*)player
return result;
}

- (void)initialize:(FlutterError* __autoreleasing*)error {
- (void)initialize:(FlutterError* _Nullable __autoreleasing*)error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use nullable in ObjC method signatures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, why is this method named incorrectly? This isn't Pigeon-generated presumably.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you have to use _Nullable since we are passing in a handle to a FlutterError. It's not named incorrectly, I've used @ObjcSelector to make all the selectors backwards compatible with how the code previously worked.

@gaaclarke
Copy link
Member Author

@stuartmorgan This is a WIP. I'll ping you when it's ready. I'm waiting for 1.0 to land before cleaning it up for review.

@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch from daf130e to fb21de0 Compare August 30, 2021 21:08
@gaaclarke
Copy link
Member Author

I've tested the example app locally on android now too and it works.

@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch from fb21de0 to ea8425d Compare August 30, 2021 21:55
@gaaclarke gaaclarke force-pushed the video_player_upgrade_pigeon branch from ea8425d to b8d3c7c Compare August 30, 2021 21:57
@stuartmorgan-g
Copy link
Contributor

This will need to be updated to move the platform channel into the implementation, per the new platform channel plan. That's probably best done a precursor PR to move in a copy of the existing implementation, then updating this PR to update that version.

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 23:37
@stuartmorgan-g
Copy link
Contributor

Closing as obsolete (in favor of #4726) since this copy can't be updated without a breaking change.

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

Successfully merging this pull request may close these issues.

2 participants