[pigeon] Adds ProxyApi code generation for Dart#5544
[pigeon] Adds ProxyApi code generation for Dart#5544bparrishMines wants to merge 89 commits intoflutter:mainfrom
Conversation
stuartmorgan-g
left a comment
There was a problem hiding this comment.
I left some high-level comments; @tarrinneal should be the main reviewer for most of the details.
I didn't review the generated code since I'm assuming it's not substantially changed from the webview_flutter version. If there's anything there you do want me to look at, let me know!
One thing that wasn't immediately obvious to me without tracing all the logic: are the big blocks of generated code conditional on using them? I.e., would this be basically a no-op for people not using proxy APIs, or would they suddenly get thousands of new lines of generated code they didn't need?
| } | ||
|
|
||
| /// Represents a constructor for an API. | ||
| class Constructor extends Node { |
There was a problem hiding this comment.
Why does a constructor have to be a special type? It seems like all the properties are standard method properties.
There was a problem hiding this comment.
I think this is similar to our discussion about parameter vs field. They overlap but some of the parameters don't apply (e.g. static/isAsynchronous). However, if I consider the Method as the message call and not the AST class member, then I suppose they are essentially the same thing. I still need to distinguish the difference between Constructors for the Dart code generation, so I think I will keep the class and have it extend Method.
| }, | ||
| ); | ||
| indent.format(''' | ||
| /// Maintains instances used to communicate with the native objects they |
There was a problem hiding this comment.
I think we should probably put giant strings like this into a helper file, wrapped in a method that takes all the necessary variables, but without any logic ideally.
| ]), | ||
| ), | ||
| ]; | ||
| } |
There was a problem hiding this comment.
This is quite a method.
Maybe we could have a separate file for all the proxy-specific code? Then it would be easier to do things like extract helpers for different sections without overwhelming the rest of this file.
| }'''); | ||
| } | ||
|
|
||
| void _writeFlutterMethodMessageHandler( |
There was a problem hiding this comment.
Some of these incidental refactors could ideally be done in a prequel PR that would be trivial to review.
| }'''); | ||
| } | ||
|
|
||
| void _writeFlutterMethodMessageHandler( |
There was a problem hiding this comment.
Some of these incidental refactors could ideally be done in a prequel PR that would be trivial to review.
| message: 'Expected a named type but found "$node".', | ||
| lineNumber: _calculateLineNumber(source, node.offset))); | ||
| } | ||
| } else if (_currentApi is AstProxyApi) { |
There was a problem hiding this comment.
Can we extract all of this block to a helper?
| List? testList; | ||
| } | ||
|
|
||
| /// The core interface that each host language plugin must implement in |
There was a problem hiding this comment.
This may be a case where we do actually want to make a second file instead of putting everything into the one giant file?
|
I'm going to mark this as a draft and split it up into simpler PRs. I will still keep this PR open since it is still useful to demonstrate all the changes put together. |
Yes, the generation of the |
… in the `DartGenerator` (#5859) Separates message call code generation into separate methods in the `DartGenerator` for flutter/flutter#134777. The `ProxyApi` generator uses similar code to the `HostApi` and `FlutterApi`, so this makes the code reusable. Separated from #5544 From suggestion: #5544 (comment)
… in the KotlinGenerator (#5891) Separates message call code generation into separate methods in the KotlinGenerator for flutter/flutter#134777. The ProxyApi generator uses similar code to the HostApi and FlutterApi, so this makes the code reusable. From suggestion: #5544 (comment)
|
closing if favor of #6043 |
… in the SwiftGenerator (#5959) Separates message call code generation into separate methods in the SwiftGenerator for flutter/flutter#134777. The ProxyApi generator uses similar code to the HostApi and FlutterApi, so this makes the code reusable. From suggestion: #5544 (comment)
… in the SwiftGenerator (flutter#5959) Separates message call code generation into separate methods in the SwiftGenerator for flutter/flutter#134777. The ProxyApi generator uses similar code to the HostApi and FlutterApi, so this makes the code reusable. From suggestion: flutter#5544 (comment)
Part of flutter/flutter#134777
ApiintoAstHostApi/AstFlutterApi/AstProxyApi. I prefixed it withAstsince the annotations already had the namesHostApi, etc...;Api.locationwas moved toMethod.locationsinceAstProxyApihas methods implemented on the host and Flutter side.lib/functional.dartwith thepackage:collectionequivalent.DartGenerator.writeProxyApiusescode_builderanddart_stylewhich I added to the allowed deps config file.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.