-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[experiment] Part 2 of refactor c-interop guides to support hooks. #12839
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
Conversation
Summary of ChangesHello @antfitch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the C-interop documentation for Android, iOS, and macOS. The primary goal is to provide clearer guidance on integrating native code by distinguishing between the newer 'build hooks' approach and the 'legacy FFI plugin' method. The changes aim to help developers choose the appropriate method based on their needs and provide detailed instructions for each, improving the overall clarity and usability of the documentation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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 refactors the C-interop guides for Android, iOS, and macOS to better distinguish between the modern 'build hooks' approach and the 'legacy FFI plugin' approach. The changes significantly improve the structure and clarity of the documentation. I've found a few minor issues, such as typos and some content that could be better placed, and one more significant issue in the macOS guide where important sections have been accidentally commented out. Overall, this is a great improvement to the documentation.
|
|
||
| ## Overview | ||
|
|
||
| Flutter mobile and desktop apps can use [build hooks][] or the the [dart:ffi][] |
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.
|
|
||
| :::note | ||
| If you are using the legacy `plugin_ffi`, | ||
| You should also add attributes to indicate that the |
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.
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 left the same comment, not seeing that the bot had already done so. Though, to be honest, I'd just delete the "You should also" entirely. :)
|
|
||
| ## Overview | ||
|
|
||
| Flutter mobile and desktop apps can use [build hooks][] or the the [dart:ffi][] |
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.
| Before your library or program can use the FFI library | ||
| to bind to native code, you must ensure that the | ||
| native code is loaded and its symbols are visible to Dart. | ||
| This page focuses on compiling, packaging, | ||
| and loading iOS native code within a Flutter plugin or app. |
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.
|
|
||
| :::note | ||
| If you are using the legacy `plugin_ffi`, | ||
| You should also add attributes to indicate that the |
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.
|
|
||
| ## Overview | ||
|
|
||
| Flutter mobile and desktop apps can use [build hooks][] or the the [dart:ffi][] |
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.
| Before your library or program can use the FFI library | ||
| to bind to native code, you must ensure that the | ||
| native code is loaded and its symbols are visible to Dart. | ||
| This page focuses on compiling, packaging, | ||
| and loading macOS native code within a Flutter plugin or app. |
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.
|
|
||
| :::note | ||
| If you are using the legacy `plugin_ffi`, | ||
| You should also add attributes to indicate that the |
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.
|
Visit the preview URL for this PR (updated for commit 0927c58): https://flutter-docs-prod--pr12839-hooks-update-2-h8cqy3rp.web.app |
dcharkes
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.
This is still super messy.
I think the pages should flow as follows:
- general intro explaining the two major approaches: build hooks&code assets and legacy FFI plugins
- Everything related to build hooks - should have 0 references to legacy FFI plugins
- Everything related to legacy FFI plugins - should have 1 references to code hooks.
Currently it is all mixed and matched, which makes it very confusing for users. It causes a lot of cognitive overload.
| [Binding to native iOS code using dart:ffi][ios-ffi]. | ||
| For information in macOS, see | ||
| [Binding to native macOS code using dart:ffi][macos-ffi]. | ||
| This feature is not yet supported for web plugins. |
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 note belongs to legacy FFI plugins. Ditto on the other pages.
|
|
||
| Flutter mobile and desktop apps can use [build hooks][] or the the [dart:ffi][] | ||
| library to call native C APIs. FFI stands for | ||
| [_foreign function interface._][FFI]. Other terms for similar functionality |
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.
double period
|
|
||
| ## Overview | ||
|
|
||
| Flutter mobile and desktop apps can use [build hooks][] or the the [dart:ffi][] |
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.
build hooks also use dart:ffi, maybe it's better to
Flutter mobile and desktop apps can use the
[dart:ffi][] library to call native C APIs.
FFI stands for [foreign function interface.][FFI]
Other terms for similar functionality include
native interface and language bindings.
The FFI can be used in two ways:
- via code assets reported in build hooks, in which Flutter takes care of bundling the native code, and
- via legacy FFI plugins where you have to write the build files yourself.
(new bullet points)
Prefer using build hooks.
| so in C++ these symbols are marked `extern "C"`. | ||
|
|
||
| :::note | ||
| If you are using the legacy `plugin_ffi`, |
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 makes no sense under the header for build hooks. This note should be under the header legacy FFI plugins.
| [dart:ffi]: {{site.dart.api}}/dart-ffi/dart-ffi-library.html | ||
| [FFI]: https://en.wikipedia.org/wiki/Foreign_function_interface | ||
|
|
||
| ### Accessing native code (build hooks) |
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.
Maybe this section makes more sense as a subsection under "create an FFI package"
| your Dart code won't need to change. | ||
|
|
||
| ## Dynamic vs static linking | ||
| ### Dynamic vs static linking (legacy FFI plugin) |
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.
And this section makes more sense under the FFI plugin section
sfshaza2
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.
Yeah, I'd like to see tabs used in this page: one for legacy, another for build hooks, and whatever else makes sense if you need more.
I'm going to go ahead and approve, but consider making that change.
|
|
||
| :::note | ||
| If you are using the legacy `plugin_ffi`, | ||
| You should also add attributes to indicate that the |
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 left the same comment, not seeing that the bot had already done so. Though, to be honest, I'd just delete the "You should also" entirely. :)
| ### Open-source third-party support | ||
|
|
||
| For open source, build the C/C++ sources in your build hook. Or build the | ||
| sources on GitHub actions and download the binary from github artifacts on |
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.
github=>GitHub
Though I am not sure what "GitHub artifacts" are. Does it just mean things that are built by/on GitHub?
| ## Other use cases | ||
|
|
||
| ### Platform library | ||
| ### Platform library (legacy FFI plugin) |
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.
Do we need this parenthetical in a header?
I understand that this page shows the new (hooks) vs the old (legacy). But what I think would be much cleaner would be to use tabs in the page rather than a potentially confusing set of "legacy" headers. A tab for build hooks and a tab for legacy.
Description of what this PR is changing or adding, and why:
Refactor the layout of the c-interop guides to make it clear what hooks support and what they don't.
Required reviewer: @dcharkes