Skip to content
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.

Sign up for GitHub

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

FFIgen doc updates #4478

Closed
wants to merge 9 commits into from
Closed

FFIgen doc updates #4478

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f0362c1
Adding finding iOS header files
leighajarett Jan 4, 2023
5cc5484
Adding bit about compiler flags
leighajarett Jan 4, 2023
06f7a70
Adding steps / examples for handling the callbacks
leighajarett Jan 5, 2023
7f86dc6
Fixing bullets
leighajarett Jan 5, 2023
d9af166
Merge branch 'main' into ffigen-doc-updates
domesticmouse Jan 5, 2023
88d9f59
Merge branch 'main' into ffigen-doc-updates
atsansone Jan 13, 2023
b0ffd7e
Merge branch 'main' into ffigen-doc-updates
atsansone Apr 3, 2023
4183237
Merge branch 'main' into ffigen-doc-updates
atsansone Apr 5, 2023
658b48d
Merge branch 'main' into ffigen-doc-updates
atsansone May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/_guides/libraries/objective-c-interop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ The `output` is the path of the Dart file that ffigen will create.
The entry point is the header file containing the API.
In this example, it is the internal `AVAudioPlayer.h` header.

If you are creating an iOS plugin, you may need to reference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reference here, does it mean as an entry-point in the above config file?

Also, is this a common situation or could we put this in an aside/note?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the above config file we show a header file for a specific library on MacOS. But many developers will use this for iOS plugins. I had trouble finding the header files for commonly used frameworks on iOS, so I thought it would be helpful to point in that direction. We could certainly include it as an aside or a note though, since its a different use case then the example

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a great idea for a note then :)

Perhaps a tip?: {{site.alert.tip}}

header files from the iPhoneOS SDK framework directory:
`/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks/`

Another import thing you'll see,
if you look at the [example config]({{page.example}}/pubspec.yaml),
is the exclude and include options.
Expand Down Expand Up @@ -156,6 +160,15 @@ to insert some linter ignore rules at the top of the generated file:
// ignore_for_file: camel_case_types, non_constant_identifier_names, unused_element, unused_field, return_of_invalid_type, void_checks, annotate_overrides, no_leading_underscores_for_local_identifiers, library_private_types_in_public_api
```

In some cases you may need to specify compiler flags
to include the framework dependencies, or specify the minimum platform version:

```yaml
compiler-opts:
- "-F/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/System/Library/Frameworks"
- "-mios-version-min=13.0"
```

See the [ffigen readme]({{page.ffigen}}#configurations)
for a full list of configuration options.

Expand Down Expand Up @@ -328,6 +341,34 @@ to the correct isolate.
For an example of this,
see the implementation of [`package:cupertino_http`][].

There are a few steps required to handle callbacks:
Copy link
Member

@parlough parlough Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this should be its own section or subsection (which could be cross-linked to) or even if it's own page if you think that's better (since this seems to be temporary). Primarily because the text after these additions still refer to the above bulleted list. This large break in that discussion may cause confusion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. I am open to creating a new section or page (and perhaps go into a bit more detail) - do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really does seem like the instructions would make more sense and be easier to follow with inline code snippets (and get less out of date).

You mention the links "should only be a temporary solution until callbacks are more easily handled". Is there a rough timeline for that or an issue to track (maybe we could link to it)? If it's going to be a while and this is a common use case, it might be best to make a very simple example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liamappelbe can comment more on timeline, but I don't think we have an exact timeline at the moment. I do think it will be at least a few months.

The demo we created for Flutter Forward is pretty simple so maybe it makes sense to hold off until we've published that demo and then use it as a code sample (I don't think it will be updated so maintenance is less of an issue). What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a temporary solution / experimental package, is there a way to create a page that makes that clear? Maybe it makes more sense to have a Medium blog post or something and then just link to it in this page?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely going to be O(months). I think making it a separate page and using inline snippets from the flutter forward demo, rather than linking to cupertino_http makes the most sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like we all agree on the separate page then. And if there's a demo with snippets, that would be fantastic to use instead :)


1. Create an Objective-C function, or method,
that calls the method of interest. Your helper function or method
must include a callback that transforms the returned data
into Dart C Objects and sends the data, or a pointer to the object,
to the Dart Port. You can see an [example here](https://github.com/dart-lang/http/blob/2f8205c5254886e088aa5ebd52a2e4dc4ec18afb/pkgs/cupertino_http/src/CUPHTTPClientDelegate.m#L63).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit queasy about linking a random git hash version. Does dart-lang/http tag releases, or should we link to master/main with the understanding there will be maintenance overhead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will let @brianquinlan chime in on this question


- You should copy the Dart API classes from the [`dart-sdk` directory](https://github.com/dart-lang/http/tree/master/pkgs/cupertino_http/src/dart-sdk).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth explaining why this is necessary?

Also, is this also just a temporary necessity? Sounds like this may get out of date and may cause other concerns for users, such as properly handling the license requirements of the code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that this whole section is just temporary but can't say for sure how long. If we break it out into its own page then I think it makes sense to go into more details on why its necessary and what the classes do.

As for licensing requirements, I am not really sure. Ideally there's a better way to do this (like importing a library or something), but since this is meant to be a temporary solution I don't think the team is looking to do much more work. @liamappelbe any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Google has a policy of putting code that's under a different license in a third_party folder.

https://opensource.google/documentation/reference/thirdparty/non-google3

Yes, I'd also prefer a separate page.


- If you will reference an Objective-C object in your Dart code,
use [`retain`](https://developer.apple.com/documentation/objectivec/1418956-nsobject/1571946-retain), to ensure the object is not garbage collected.
You can see an [example here](https://github.com/dart-lang/http/blob/2f8205c5254886e088aa5ebd52a2e4dc4ec18afb/pkgs/cupertino_http/src/CUPHTTPForwardedDelegate.m#L16).

1. Include the source files in the `ios/Classes` directory,
[as shown here](https://github.com/dart-lang/http/tree/master/pkgs/cupertino_http/ios/Classes).
Comment on lines +358 to +359
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I read this, it at first seems like I should copy the files shown here, rather than add includes for my specific files. I'm not familiar with what this step is for, but if a little context is added on why, maybe it'll be more clear.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, you need to actually add the files for the classes you've created and import the source files. I can be more specific!


1. Include the header files for your helper functions, or classes,
in the `ffigen` config [as shown here](https://github.com/dart-lang/http/blob/master/pkgs/cupertino_http/ffigen.yaml#L23).

1. On the Dart side, open up a Dart port to receive the data from
the callback as [shown here](https://github.com/dart-lang/http/blob/047d6ed015d397be169a7fb892d75141d9bfd58f/pkgs/cupertino_http/lib/cupertino_http.dart#L823).

1. To leverage the Objective-C object in Dart, cast the pointer that
was passed to the Dart port. You can see an [example here](https://github.com/dart-lang/http/blob/047d6ed015d397be169a7fb892d75141d9bfd58f/pkgs/cupertino_http/lib/cupertino_http.dart#L830). `release: true`
ensures that the object is released.


The third point means that directly calling some Apple APIs
using the generated Dart bindings might be thread unsafe.
This could crash your app, or cause other unpredictable behavior.
Expand Down