-
Notifications
You must be signed in to change notification settings - Fork 3.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 our terms of service and privacy 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?
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 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.
| // Implement methods from ${generatorOptions.fileSpecificClassNameComponent}PigeonEventChannelWrapper | ||
| override | ||
| open fun onListen(p0: Any?, sink: PigeonEventSink<${_kotlinTypeForDartType(func.returnType)}>) {} | ||
| override | ||
| open fun onCancel(p0: Any?) {} | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
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.
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.
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].///).