[pigeon] Fix kotlin warning about calling bridge method #10632
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 aims to fix a Kotlin "bridge method" warning by ensuring generated stream handler classes implement all methods from their supertype. The version numbers and changelog are updated accordingly. While the intent is correct, I've identified a critical issue in the Kotlin generator that places the new override methods inside the companion object, which will cause a compilation failure. I've provided a code suggestion to move these methods to the correct location within the class body. Additionally, I've suggested a small correction for a typo in the CHANGELOG.md file.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| } | ||
| } | ||
| // Implement methods from EventChannelMessagesPigeonEventChannelWrapper | ||
| override open fun onListen(p0: Any?, sink: PigeonEventSink<PlatformEvent>) {} |
There was a problem hiding this comment.
Does making these abstract instead of no-ops ({}) work and fix the warning? That would be an explicit statement of the intent here, which is to not implement the interface at this level but require subclasses to implement the type-specified version.
There was a problem hiding this comment.
I repro'd locally and tried it, and this does indeed seem to work:
override open abstract fun onListen(p0: Any?, sink: PigeonEventSink<PlatformVideoEvent>)
override open abstract fun onCancel(p0: Any?)That keeps the same semantics of forcing the subclasses to implement both methods, without tripping the warning.
There was a problem hiding this comment.
modifying the pr to do this instead. My goal was not to change the semantics.
…warning on bridged methods
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
lgtm once changes are made as discussed.
| eventSink = sink | ||
| } | ||
|
|
||
| override fun onCancel(p0: Any?) {} |
There was a problem hiding this comment.
@stuartmorgan-g @tarrinneal pigeon_example_app failed to build without this change. Does that mean this is a breaking change? If so, does that change what solution you prefer here?
There was a problem hiding this comment.
Sorry, looking again at the top-level interface class it has default implementations. I really thought it was just abstract declarations.
So your previous version of the PR was the behavior-preserving one, and I was just confused. Sorry about that!
packages/pigeon/CHANGELOG.md
Outdated
| ## 26.1.5 | ||
|
|
||
| * [kotlin] Fixes a "bridge method" warning that occurs when a class uses `*PigeonEventChannelWrapper` as a | ||
| supertype without implementing all of its methods. |
There was a problem hiding this comment.
Is the "without implementing all of its methods" part right? The video_player code was implementing both of these methods.
There was a problem hiding this comment.
I am open to better wording but the class that implemented all the methods was an implementation of *EventsStreamHandler not the interface *PigeonEventChannelWrapper.
There was a problem hiding this comment.
Oh, I see. Since we don't expect Pigeon users to directly subclass the Wrapper class, let's just say "... warning when implementing an event stream handler."
tarrinneal
left a comment
tarrinneal
left a comment
There was a problem hiding this comment.
looks good, thanks Reid
reidbaker
added
the
autosubmit
flutter/packages@57725eb...6f392aa 2025-12-18 1063596+reidbaker@users.noreply.github.com [pigeon] Fix kotlin warning about calling bridge method (flutter/packages#10632) 2025-12-18 robert.odrowaz@leancode.pl [camera_avfoundation] Wrappers swift migration - part 4 (flutter/packages#10440) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
3 participants
Part 1/2 for #178908
Next step is to update video_player_android with this feature.
Tested by updating an example app to use a local version of video_player_android and manually modifying the generated files prior to updating the generator.
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).