-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[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
| } | ||
|
|
||
| - (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_interfacethat 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.