packages icon indicating copy to clipboard operation
packages copied to clipboard

[pigeon] Adds Kotlin support for Pigeon

Open ailtonvivaz opened this issue 2 years ago • 3 comments

This PR adds Kotlin support for Pigeon. This code was based on the Java and Swift generator.

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • [ ] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

ailtonvivaz avatar Mar 02 '22 00:03 ailtonvivaz

Any update on it? Looking forward to the kotlin support

PandaGeek1024 avatar Mar 23 '22 22:03 PandaGeek1024

Any update on it? Looking forward to the kotlin support

I am waiting https://github.com/flutter/packages/pull/976 to be release to continue with the tests, once there are a lot of changes which would be useful here.

ailtonvivaz avatar May 03 '22 18:05 ailtonvivaz

@ailtonvivaz Apologies for the delay in assigning reviewers; I missed during the last triage that this had moved out of Draft :(

stuartmorgan avatar Jul 19 '22 19:07 stuartmorgan

@ailtonvivaz If you can get this up to date with main again, I can probably get it pushed through in the next day or so! Thanks for the hard work you put into this!

tarrinneal avatar Sep 19 '22 23:09 tarrinneal

Hi folks. I'm working on change requests today to make it ready to merge.

ailtonvivaz avatar Sep 20 '22 13:09 ailtonvivaz

In the generator_tools file, there is a line stating the version number of pigeon, it needs to be updated to match the pubspec file.

tarrinneal avatar Sep 20 '22 17:09 tarrinneal

Looks like the macOS bots don't have a new version of Java; can we use a slightly older version of Gradle?

stuartmorgan avatar Sep 20 '22 19:09 stuartmorgan

Looks like the macOS bots don't have a new version of Java; can we use a slightly older version of Gradle?

I'll change that and test downgrading.

ailtonvivaz avatar Sep 20 '22 19:09 ailtonvivaz

Seems like there is still a (or a new) failing test on macos @ailtonvivaz hopefully the last piece of the puzzle

tarrinneal avatar Sep 20 '22 22:09 tarrinneal

Seems like there is still a (or a new) failing test on macos @ailtonvivaz hopefully the last piece of the puzzle

@tarrinneal I hope so. I'm working on that to understanding whats happening

ailtonvivaz avatar Sep 20 '22 22:09 ailtonvivaz

@tarrinneal This error happens just in flutter master version. I am looking for what could be the cause of this error ('writeValue' overrides nothing), once the function signature had no changes.

ailtonvivaz avatar Sep 21 '22 00:09 ailtonvivaz

@tarrinneal Edit: The function signature of StandardMessageCodec has changed in flutter master

- protected void writeValue(@NonNull ByteArrayOutputStream stream, @NonNull Object value)
+ protected void writeValue(@NonNull ByteArrayOutputStream stream, @Nullable Object value)

How proceed in this case? Should we wait for this change in stable too? And for the developer, how code will be generated?

ailtonvivaz avatar Sep 21 '22 00:09 ailtonvivaz

It changed in https://github.com/flutter/engine/pull/32706 which is on stable as well, so you can just update to the new version. The reason the stable tests passed here is that we're currently (accidentally) not running Pigeon tests on stable.

stuartmorgan avatar Sep 21 '22 14:09 stuartmorgan

Now all checks have passed. Would be better add some information that this generator only works in Flutter 3.3.0 and later?

ailtonvivaz avatar Sep 21 '22 16:09 ailtonvivaz

Yes, that seems like something that's worth calling out both the CHANGELOG and the README.

Actually, I hadn't registered that there's no README update here. We should definitely document the support (and that it's currently experimental, so could change) in the README. Looks like we didn't notice for Swift either, so you can include that here or we can do it separately.

stuartmorgan avatar Sep 21 '22 16:09 stuartmorgan

I could include in this PR both references (Kotlin and Swift). But, in other case, I could create a new PR to adds them.

ailtonvivaz avatar Sep 21 '22 16:09 ailtonvivaz

Either is fine for the README. (I wanted to separate out Swift unit testing because that could end up snowballing if, e.g., there were latent issues with CI due the tests not having been run there before, but a README should be pretty safe.)

stuartmorgan avatar Sep 21 '22 17:09 stuartmorgan

@stuartmorgan README updated

ailtonvivaz avatar Sep 21 '22 17:09 ailtonvivaz

I'm very glad to contribute to this project. This package has helped me a lot at work and I'm very proud in be part of some of it.

I hope it keeps helping a lot of developers.

🚀

ailtonvivaz avatar Sep 21 '22 21:09 ailtonvivaz

I tried to resolve the (trivial) conflicts created by the other Pigeon PR I just landed, but for some reason the GitHub UI resolver UI keeps failing to commit the fix.

stuartmorgan avatar Sep 22 '22 14:09 stuartmorgan

I tried to resolve the (trivial) conflicts created by the other Pigeon PR I just landed, but for some reason the GitHub UI resolver UI keeps failing to commit the fix.

I'm gonna fix this locally.

ailtonvivaz avatar Sep 22 '22 14:09 ailtonvivaz

Thanks! I should have landed the two PRs in the other order, I just wasn't thinking about the fact that there were two when I was going through email.

stuartmorgan avatar Sep 22 '22 14:09 stuartmorgan