-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[pigeon] Improve java generation #214
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
Conversation
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.
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:
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.
From what I can see in Flutter's code, they don't care about specific type anywhere:
- https://github.com/flutter/engine/blob/36769af3103819b6e977b71f80761091955fa5d9/shell/platform/android/io/flutter/plugin/common/StandardMessageCodec.java#L260
- https://github.com/flutter/engine/blob/36769af3103819b6e977b71f80761091955fa5d9/shell/platform/android/io/flutter/plugin/common/JSONUtil.java#L71
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.
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.
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.
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.
|
@gaaclarke What should I do to move this forward? Any changes I need to make to the code? |
|
@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. |
|
@gaaclarke Anything new? |
This resolves java side of the flutter/flutter#58913
gaaclarke
left a comment
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.
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.
e49ef9d to
27fc2de
Compare
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.