- Notifications
You must be signed in to change notification settings - Fork3.6k
[pigeon] Fix kotlin warning about calling bridge method#10632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
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 thecompanion 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 theCHANGELOG.md file.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| } | ||
| } | ||
| // Implement methods from EventChannelMessagesPigeonEventChannelWrapper | ||
| overrideopenfunonListen(p0:Any?,sink:PigeonEventSink<PlatformEvent>) {} |
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.
Does making theseabstract 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I repro'd locally and tried it, and this does indeed seem to work:
overrideopenabstractfunonListen(p0:Any?,sink:PigeonEventSink<PlatformVideoEvent>)overrideopenabstractfunonCancel(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
modifying the pr to do this instead. My goal was not to change the semantics.
…warning on bridged methods
tarrinneal left a comment
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.
lgtm once changes are made as discussed.
| eventSink= sink | ||
| } | ||
| overridefunonCancel(p0:Any?) {} |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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!
stuartmorgan-g left a comment
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.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is the "without implementing all of its methods" part right? The video_player codewas implementing both of these methods.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, I see. Since we don't expect Pigeon users to directly subclass theWrapper class, let's just say "... warning when implementing an event stream handler."
tarrinneal left a comment
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.
looks good, thanks Reid
Uh oh!
There was an error while loading.Please reload this page.
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].///).