Repository navigation
[video_player] Convert event channel to Swift and Pigeon - #13375
auto-submit[bot] merged 19 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request converts the internal event channel implementation of the AVFoundation video player plugin to Swift using Pigeon, replacing the Objective-C FVPEventBridge with a Swift EventBridge class and introducing structured event classes. Feedback on the changes highlights a potential memory leak where the EventBridge is registered as a stream handler but never unregistered upon disposal, creating a strong retain cycle. Suggestions are provided to store references to the messenger and channel suffix to properly unregister the stream handler when the player is disposed.
f5f60d8 to
cd29985
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the event channel implementation of the video_player_avfoundation package from Objective-C to Swift using Pigeon's event channel generation, and updates video_player_android to handle event channel unregistration upon player disposal. A misplaced and duplicate changelog entry for version 2.12.3 was identified at the bottom of the AVFoundation CHANGELOG.md file and should be removed.
|
@jmagman I folded the Android unregistration change into this PR since I was making the same change on the iOS side as part of updating this PR; let me know if you'd prefer I split that into a separate PR. |
There was a problem hiding this comment.
Code Review
This pull request updates the video_player_android and video_player_avfoundation packages to utilize Pigeon-generated event channels for video events. In video_player_android, the event channel is now properly cleaned up on player disposal to prevent native object leaks. In video_player_avfoundation, the internal event channel implementation is migrated from Objective-C to Swift. Feedback recommends marking the conforming test stub StubEventListener with @MainActor to maintain consistent actor isolation with the VideoEventListener protocol.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| if let queuedEvents = queuedEvents { | ||
| for event in queuedEvents { |
There was a problem hiding this comment.
Minor nit, the queuedEvents shadow is a bit weird, could do something like:
queuedEvents?.forEach { event inThere was a problem hiding this comment.
That gives an analysis warning (ReplaceForEachWithForLoop). But I recently learned that we've reaching a minimum Xcode version where we can now safely use the if let foo shorthand instead of the old, weirder-looking standard of if let foo = foo, so I shortened it here (and throughout the plugin where I had been using the older form while converting).
| public void dispose() { | ||
| isWaitingForValidDuration = false; | ||
| mainHandler.removeCallbacks(initializationFallback); | ||
| events.onDisposed(); |
There was a problem hiding this comment.
Did you add a test for this new leak fix?
There was a problem hiding this comment.
I had forgotten, but now I have 🙂 I don't think there's a way to directly test for the leak itself in Java, but I now test that it unregisters (via a mock).
stuartmorgan
added
the
autosubmit
…lutter#196558) Manually rolls flutter/packages from 14f9933a8cd6 to 015df97cbc3f (5 revisions). This is the same one-line change the [flutter/packages → flutter/flutter autoroller](https://autoroll.skia.org/r/flutter-packages-flutter-autoroll) makes. Today's two automatic rolls, flutter#196527 and flutter#196547, failed on presubmit jobs that don't use the packages pin (`Windows tool_integration_tests_5` and `Linux_android_emu android_engine_opengles_tests`). The roller closed the second one after its roll window ended. It only rolls on weekdays and doesn't support manual rolls, so its next roll would be on Monday. flutter/packages@14f9933...015df97 2026-11-13 [camera_windows] Fall back to temp dir for capture path. (flutter/packages#12755) 2026-11-13 [video_player] Convert event channel to Swift and Pigeon (flutter/packages#13375) 2026-11-13 [material_ui] Document and demonstrate safe MaterialBanner dismissal across routes (flutter/packages#13267) 2026-11-12 [flutter_svg] Add a byte budget for the compiled SVG cache (flutter/packages#13596) 2026-11-12 Roll Flutter from b98afb5 to 9c427b5 (29 revisions) (flutter/packages#13636)
Converts
FVPVideoEventListenerand theFVPEventBridgeimplementation from Obj-C to Swift, and now that it uses Swift, adopts Pigeon's Swift event channel support to eliminate the use of stringly-typed data.The overhauled Dart unit test code is taken directly from the rewrite that I did previously for
video_player_android's Kotlin Pigeon event channel conversion, with minimal changes to match the iOS test cases.Also includes a minor adjustment to
video_player_androidto add missing unregistration code to avoid a minor native object leak, matching changes made on the iOS side during review. This requires a Pigeon update to pick up the functionality needed to unregister.Part of flutter/flutter#119105
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2