Skip to content

Conversation

@reidbaker
Copy link
Contributor

  • *Modify genrated code to implement methods from PigeonEventChannelWrapper
  • Bump tooling/yaml version, update changelog
  • update kotlin generated tests

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

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran [the auto-formatter].
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with 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].
  • I updated CHANGELOG.md to 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].
  • I updated/added any relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under[^1].
  • All existing and new tests are passing.

Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines 1236 to 1242
// 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.

reidbaker and others added 2 commits December 15, 2025 13:19
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>) {}
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@tarrinneal tarrinneal left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants