-
Notifications
You must be signed in to change notification settings - Fork 81
[jnigen] revamp readme #2670
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
[jnigen] revamp readme #2670
Conversation
PR HealthBreaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with |
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.
You have auto-line-break on, but it's not at 80 but 120? Is that intentional?
Should we have some standard for markdown files in this repo and enforce it with a ci check?
If it's not intentional, prefer not adding line-breaks to keep the git diff smaller and easier to review.
pkgs/jnigen/README.md
Outdated
## Migrating to 0.14.2 | ||
|
||
When regenerating bindings after an upgrade to JNIgen in the one file per class mode, you will need to delete the old bindings once to fix `A file that was not generated by JNIgen was found in ...` error. | ||
When regenerating bindings after an upgrade to JNIgen in the one file per class mode, you will need to delete the old |
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.
Should this be in the changelog instead?
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 already in there. I'll add some highlight to it and remove it from here.
pkgs/jnigen/README.md
Outdated
Experimental bindings generator for Java bindings through dart:ffi and JNI. | ||
|
||
`jnigen` scans compiled JAR files or Java source code to generate a description of the API, then uses it to generate Dart bindings. The Dart bindings call the C bindings, which in-turn call the Java functions through JNI. Shared functionality and base classes are provided through the support library, `package:jni`. | ||
Bindings generator to simplify calling Java APIs from Dart code via `dart:ffi` and JNI. |
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.
to simply calling -> to call
pkgs/jnigen/README.md
Outdated
Bindings are generated for methods, constructors, and fields. Exceptions thrown in Java are rethrown in Dart with stack | ||
trace from Java. | ||
|
||
More advanced features such as callbacks are not supported yet. Support for these features is tracked in 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.
Isn't this supported by now?
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.
Removed
pkgs/jnigen/README.md
Outdated
Currently basic features of the Java language are supported in the bindings. Each Java class is mapped to a Dart class. Bindings are generated for methods, constructors and fields. Exceptions thrown in Java are rethrown in Dart with stack trace from Java. | ||
|
||
More advanced features such as callbacks are not supported yet. Support for these features is tracked in the [issue tracker](https://github.com/dart-lang/native/issues?q=is%3Aopen+is%3Aissue+label%3Apackage%3Ajni%2Cpackage%3Ajnigen). | ||
Currently basic features of the Java language are supported in the bindings. Each Java class is mapped to a Dart class. |
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 we should rephrase this. We're no longer interested in mapping features 1-to-1 due to impedance mismatches. JNIgen is a layer-1 tool.
Side note: We should add the 3-layer markdown file on the repo here somewhere so that we can refer to it.
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 you have a suggested phrasing?
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 just: Bindings are generated for classes, methods, constructors, and fields. (leaving out how they are mapped or generated.)
pkgs/jnigen/README.md
Outdated
|
||
Dart standalone target is supported, but due to some problems with pubspec format, the `dart` command must be from Flutter SDK and not Dart SDK. See [dart-lang/pub#3563](https://github.com/dart-lang/pub/issues/3563). | ||
Dart standalone target is supported, but due to some problems with pubspec format, the `dart` command must be from the | ||
Flutter SDK and not Dart SDK. See [dart-lang/pub#3563](https://github.com/dart-lang/pub/issues/3563). |
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 think we might be able to make it Dart standalone when we have native assets in stable, and when we split out the android context into package:jni_flutter
. cc @HosseinYousefi will know for sure.
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.
For now let's leave the readme like this.
LGTM to overall flow! 🙏 |
pkgs/jnigen/README.md
Outdated
to the pubspec of your app by running: | ||
|
||
Check out the [examples](example/) to see some sample configurations. | ||
```shell |
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.
Now it's possible to do flutter pub add jni dev:jnigen
.
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.
Nice!
pkgs/jnigen/README.md
Outdated
Bindings are generated for methods, constructors, and fields. Exceptions thrown in Java are rethrown in Dart with stack | ||
trace from Java. | ||
|
||
More advanced features such as callbacks are not supported yet. Support for these features is tracked in 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.
Actually they are supported, I forgot to update this section!
pkgs/jnigen/README.md
Outdated
|
||
Dart standalone target is supported, but due to some problems with pubspec format, the `dart` command must be from Flutter SDK and not Dart SDK. See [dart-lang/pub#3563](https://github.com/dart-lang/pub/issues/3563). | ||
Dart standalone target is supported, but due to some problems with pubspec format, the `dart` command must be from the | ||
Flutter SDK and not Dart SDK. See [dart-lang/pub#3563](https://github.com/dart-lang/pub/issues/3563). |
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.
For now let's leave the readme like this.
The reflow was intentional since most other markdowns in this repo also seem to implicitly follow a limit and it does make the file easier to read in environments that don't render the markdown. Not intentional was the 120 limit, should have been 80. That's fixed now. |
cc @stuartmorgan-g Are you interested in taking a look at the revamped readme to incorporate your feedback? |
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 new intro is great! This definitely addresses the core of #1640 from my perspective. I think the notes in that issue about the Requirements section are probably still valid, but it also makes sense to address those separately, so we can split them into a new issue.
I flagged a couple of nits that I happened to notice while re-reading the whole doc, but they are for pre-existing sections so feel free to ignore them here.
pkgs/jnigen/README.md
Outdated
| Android | n/a | Supported | | ||
| Linux | Supported | Supported | | ||
| Windows | Supported | Supported | | ||
| MacOS | Supported | Not Yet | |
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.
Nit while the file is being overhauled: the name of the OS is macOS
, not MacOS
. (It's confusing because it used to be Mac OS X
.)
pkgs/jnigen/README.md
Outdated
## Note on Dart (standalone) target | ||
`package:jni` is an FFI plugin containing native code, and any bindings generated from jnigen contains native code too. | ||
`package:jni` is an FFI plugin containing native code, and any bindings | ||
generated from jnigen contains native code too. |
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.
Nit: contain
pkgs/jnigen/README.md
Outdated
application targeting Android. It assumes that Flutter has been set up to build | ||
apps for Android | ||
([instructions](https://docs.flutter.dev/platform-integration/android/setup)) | ||
and that the Flutter app was created via `flutter create in_app_java`. |
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 might be worth adding a forward-link to the Requirements section here? Maybe something like "If you encounter issues running the commands below, check [the detailed setup requirements](...)
".
Fixes #1640.
This adds a simple "getting started" section to the readme to guide newcomers through setting up jnigen for a Flutter app. It also shuffles some sections around and reflows the text.