-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player] upgraded pigeon to 1.0 #4277
Conversation
60ffbb4
to
b916bb9
Compare
9db485a
to
ff0e42d
Compare
36f4bd7
to
daf130e
Compare
Locally testing is working on iOS. |
@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? |
There is one issue with the Android generated code:
Looks like the template parameters didn't get brought over, the generated code has that method returning |
There was a problem hiding this 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.
packages/video_player/video_player/example/ios/Flutter/AppFrameworkInfo.plist
Outdated
Show resolved
Hide resolved
packages/video_player/video_player/example/ios/Runner.xcodeproj/project.pbxproj
Outdated
Show resolved
Hide resolved
@@ -511,7 +511,7 @@ - (FLTTextureMessage*)onPlayerSetup:(FLTVideoPlayer*)player | |||
return result; | |||
} | |||
|
|||
- (void)initialize:(FlutterError* __autoreleasing*)error { | |||
- (void)initialize:(FlutterError* _Nullable __autoreleasing*)error { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
daf130e
to
fb21de0
Compare
I've tested the example app locally on android now too and it works. |
fb21de0
to
ea8425d
Compare
ea8425d
to
b8d3c7c
Compare
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. |
Closing as obsolete (in favor of #4726) since this copy can't be updated without a breaking change. |
change to
video_player_platform_interface
that must merge first: #4291Pre-launch Checklist
dart format
.)[shared_preferences]
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.