Skip to content

Conversation

@matejdro
Copy link
Contributor

@matejdro matejdro commented Oct 5, 2020

Made a bunch of minor improvements to the Java generation part of the pigeon. Those should make usage of pigeon from java side a bit more convenient.

Please see commit messages for full list of improvements.

@goderbauer goderbauer requested a review from gaaclarke October 7, 2020 21:53
Comment on lines +17 to +18
Copy link
Member

Choose a reason for hiding this comment

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

ArrayList and HashMap are directly lifted from the platform channels documentation as the datatypes the standard message codec is creating. I don't imagine it's safe or possible just to cast from one to the other:

https://flutter.dev/docs/development/platform-integration/platform-channels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I don't see scenario where this conversion code would suddenly start caring about type of Lists. They are just iterating through it and storing it into transfer format.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let me give it a closer look today. It looks like I'm going to have some time set aside for Pigeon work today.

@matejdro
Copy link
Contributor Author

@gaaclarke What should I do to move this forward? Any changes I need to make to the code?

@gaaclarke
Copy link
Member

@matejdro no, thank you. I have an outstanding PR for unit tests for generated java code that I wanted to land first. If i keep getting pulled away from that I'd like to test it personally at least. Hopefully I can get around to it this week, thanks for your patience.

@matejdro
Copy link
Contributor Author

@gaaclarke Anything new?

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Sorry, I was hoping to have some extra testing capabilities for this PR. It got stalled longer than I hoped. This looks good. I might have to tweak it since I'm running into weird null safety CI issues locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants