-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[pigeon] Adds Quick start guide to README #8098
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.
I started leaving specific comments, but the further I got into it the more I think we need to start from a discussion about what the intended audience is, and where the content should live. Overall, this seems much more like something I would expect to see in a codelab or on an updated version of https://docs.flutter.dev/platform-integration/platform-channels than in the README; our package READMEs are generally not structured as tutorials.
Internally, the generated code uses `MethodChannel`s to communicate across Flutter's | ||
UI thread and the Platform thread where your host app initially launched. The value | ||
in Pigeon comes from automatically keeping this unpleasant boilerplate in sync and | ||
efficiently marshalling data between languages. |
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.
This is an internal implementation detail, not something people generally need to know.
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.
FWIW, Our existing documentation does mention efficiency gains which personally made me wonder, "O RLY? Like what?"
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.
These changes don't describe the efficiency gains, so I'm not sure how that related to adding this paragraph.
If we want to add a technical discussion somewhere of how Pigeon improves over the common pattern of using string-keyed dictionaries to send multiple parameters we could, but that would be completely orthogonal to making it easier to understand how to use Pigeon (and IMO wouldn't be a great use of README space).
a `.dart` configuration file. This guide will place all such configurations within | ||
`/pigeons/messages.dart`, but you are free to choose any file name(s) you like. | ||
|
||
Begin by instantiating a `PigeonOptions` object, wrapped in the `@ConfigurePigeon` |
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.
This seems like a long-form tutorial rather than a quick-start.
Maybe a codelab would be a better place for some of this content?
in a `pigeons/*.dart` ensures Pigeon will generate matching Dart and native | ||
implementations. | ||
|
||
### Define which methods to expose |
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.
It's not clear to me how this step is different from the previous step.
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.
Define your messages
intends to describe defining the data structures that will be passed back and forth; whereas Define which methods to expose
is about the HostApi()
and FlutterApi()
decorators which expose a class with methods.
A function and its parameters are definitely heavily linked, but given the context of Pigeon jargon, they felt distinct to me.
WDYT?
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.
Define your messages
intends to describe defining the data structures that will be passed back and forth; whereasDefine which methods to expose
is about theHostApi()
andFlutterApi()
decorators which expose a class with methods.
Many uses of Pigeon don't even require data structures; I don't see these are separate steps (and if they were, certainly wouldn't put defining data structures before figuring out what the methods are going to be that would need them).
The examples here will cover basic usage. For a more thorough set of examples, | ||
check the [core_tests pigeon file](../pigeons/core_tests.dart) and | ||
[platform test folder](../platform_tests/) ([shared_test_plugin_code](../platform_tests/shared_test_plugin_code/) and [alternate_language_test_plugin](../platform_tests/alternate_language_test_plugin/) especially). | ||
|
||
## Invocation |
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.
This removed sections seems like a much quicker quick-start than the new section that's called a quick start.
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.
It was shorter, that's true, but at least for me it was definitely not quicker. I also said this in an earlier comment, but in my first experience using Pigeon, I found myself searching through sample code to fill in large mental gaps in how everything was supposed to work.
See the [language-specific guides](example/README.md#HostApi-Example) for help instantiating the native classes | ||
generated by Pigeon. | ||
|
||
In Dart, typical use within a `StatefulWidget` might look like this: |
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 don't think we have any code in our repo that looks like this, so I would not say this is typical use. I'm not sure why we need an example for this at all.
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.
This code (and on down) is what inspired it.
FWIW, this whole PR was inspired by me reading Pigeon's documentation, still being quite confused about the high level picture, and then having to piece it together from sample source code. My goal is to elevate the high level picture with snippets to the top-level README.
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'm not sure why we need an example for this at all.
Pigeon is great, but I thinks fundamental API shape has a decent learning curve (writing a pseudo-class, generating an abstract class off that, defining a concrete class and registering it with the generated abstract class). It's all fairly unusual, so my general sense here is that we're currently short on context and perspective explainers around code samples.
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.
This code (and on down) is what inspired it.
That code is pretty artificial, due to the constraints of trying to fit an example of Pigeon usage into a single-file application.
thinks fundamental API shape has a decent learning curve (writing a pseudo-class, generating an abstract class off that, defining a concrete class and registering it with the generated abstract class). It's all fairly unusual, so my general sense here is that we're currently short on context and perspective explainers around code samples.
Sure, but none of that is related to StatefulWidget
. I would rather the readme focus on important concepts that are fundamental to using Pigeon, rather than spending space on the unrealistic example of embedding usage directly into a StatefulWidget
.
If we are going to overhaul the in-package docs, I think the first step is to decide what should be in |
### Define which methods to expose | ||
|
||
The point of Pigeon and the `MethodChannel`s it utilizes is to call native functions | ||
living on the Platform thread from Dart, or to call Dart functions living on the UI |
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.
This language seems very confusing to me; functions don't live on threads, calls are on threads. There is no inherent binding between a function and a thread.
Adds a single "Quick start" guide to using Pigeon to the main README. Increases concept / explanation of Pigeon concepts alongside code samples.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.