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.

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

Conversation

leighajarett
Copy link

I have added a few things to the FFIgen guide, based on the friction log I put together while working on a demo.

Quick note that I added links to the appropriate lines in the cupertino_http package, as opposed to writing an entirely new example out, since this should only be a temporary solution until callbacks are more easily handled. I also added the links in-line in the markdown because I was linking on terms like "as shown here" -- but if there's a better solution feel free to propose changes.

I'm not an engineer, so it would be great to get some eyes on the language used to make sure everything looks correct:
@brianquinlan + @liamappelbe + @dcharkes :)

@parlough parlough added review.copy Awaiting Copy Review review.tech Awaiting Technical Review labels Jan 5, 2023
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

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! It's always nice to have more details rather than just link to a complex example :)

I'm not really familiar with iOS development or FFI, but I have some initial questions and comments.

@@ -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

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?

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).

- 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.

Comment on lines +358 to +359
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).
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!

@@ -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

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 :)

@@ -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}}

@parlough
Copy link
Member

parlough commented Jan 5, 2023

Thanks for answering my questions and providing some extra context :)

Sounds like it may be better to add a new page in this same directory as this and only link to it here, with a note somewhere near the top of the page.

In this format:

{{site.alert.note}}
  TODO text here
{{site.alert.end}}

This seems like a better solution as it's a pretty specific, temporary workaround. Then we can redirect that page to the new functionality once that is released and documented.

@leighajarett
Copy link
Author

Sounds good to me, I will work on that and resubmit shortly :)

@parlough
Copy link
Member

Thanks again for this Leigha!

Some of the changes here were addressed in #5336 and we'll be working on some larger improvements and updates to the page as part of #5475. I'll make sure anything still relevant here gets accounted for!

@parlough parlough closed this Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review.copy Awaiting Copy Review review.tech Awaiting Technical Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants