Repository navigation
Update the plugin template to use Pigeon - #192886
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request migrates the Flutter plugin template from using MethodChannels to Pigeon for cross-platform communication. It includes updates to the plugin creation logic, adds support for generating Pigeon-based code across multiple platforms (Android, iOS, macOS, Linux, Windows), and updates the corresponding tests to use the new Pigeon API. A high-severity issue was identified regarding the swiftOut configuration in PigeonOptions, which currently uses an invalid list format instead of a single string path.
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
#192890 will fix one of the test failures. |
02a8b71 to
b3e5fd3
Compare
|
Failing tests are all fixed now; this is actually ready for review. |
There was a problem hiding this comment.
Code Review
This pull request transitions the default Flutter plugin template from using manual MethodChannels to using Pigeon for type-safe host-platform communication across Android, iOS, macOS, Windows, and Linux. It also updates iOS and macOS runner tests to use Swift Testing instead of XCTest, and adjusts the linter and tests to accommodate Pigeon-generated files. Feedback on the changes identifies a high-severity issue in the Pigeon configuration template (messages.dart.tmpl), where swiftOut is incorrectly defined as a list of strings instead of a single string, which will cause Dart static analysis and compilation errors.
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| ) : RuntimeException() | ||
| private open class MessagesPigeonCodec : StandardMessageCodec() { | ||
| override fun readValueOfType(type: Byte, buffer: ByteBuffer): Any? { | ||
| return super.readValueOfType(type, buffer) |
There was a problem hiding this comment.
Extra whitespace here?
There was a problem hiding this comment.
Pigeon doesn't always produce nicely formatted code, and I explicitly left all the generated code unformatted to avoid developers getting tons of diffs the first time they update the Pigeon definition file. That does mean some of the generated code is a bit ugly. (We do fix things like this in Pigeon sometimes, but it's not a priority since we assume clients who are concerned about formatting will likely run their own formatter, like we do in our plugins.)
There was a problem hiding this comment.
Might be time for me to do another formatting pass on the actual output
robert-ancell
left a comment
There was a problem hiding this comment.
Looks good - I was mostly focusing on the Linux code
tarrinneal
left a comment
There was a problem hiding this comment.
This all looks reasonable to me. I'm not sure about the formatting stuff, but I guess these default files are all relatively fragile to pigeon changes anyway...
Are we planning to include anything about ffi/jni pigeon next? instructions or at least a mention that it can be used?
Yes, you'll want to update all of these files (and fix any incompatibilities in the callers, although hopefully that will be rare since they only use a single non-async Flutter API method) basically any time you release a major version of Pigeon. |
I believe the plan is to have one of the docs pages have step-by-step instructions for converting this (Once that page is live, you could always add a comment in the |
Flutter's plugin development documentation is being updated to describe Pigeon as the standard method of plugin development, rather than raw method channel use, and as part of that we want the
flutter createplugin template to be set up using Pigeon, so that the starting point matches the documentation and the recommended development path.This takes the approach of checking in a templatized version of all of the Pigeon-generated files; this has the advantage of not needing to run any additional commands, but does mean that updating Pigeon in the templates will require updating all of the generated files with the diffs (which can be done by snapshotting a fresh
flutter create -t plugin, updating Pigeon, running generation, then porting those diffs back into the templates). If this turns out to be onerous in practice, this could be replaced later with updating thecreateflow in thefluttertool to automatically rundart run pigeon --input pigeon/messages.dartafter creation (although this will makecreateslightly slower).Also updates the iOS/macOS tests from XCTest to Swift Testing while I was in the files, to reflect current testing practices.
Fixes #63284
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.