-
Notifications
You must be signed in to change notification settings - Fork 607
[macOS] Align FLEPlugin with iOS Flutter #182
[macOS] Align FLEPlugin with iOS Flutter #182
Conversation
Changes FLEPlugin to be a subset of iOS's FlutterPlugin's API, including changing the plugin creation and registration system. Plugins now create and manage their own channels, instead of relying on FLEViewController for high-level message APIs. This is a breaking change for any FLEPlugin implementation, as well as application-level code that was calling addPlugin:. (The later may require additional changes in the future as noted in the comment on registerPlugin:, but the plugin implemention API should now be stable.) This essentially completes issue google#102 for macOS.
krisgiesing
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.
So overall this change LGTM, but I have to admit I don't understand why MethodChannel and MessageChannel are asymmetric (e.g. there's an addMethodCallDelegate but no addMessageCallDelegate).
| - (id<FLEPluginRegistrar>)registrarForPlugin:(NSString *)pluginName { | ||
| // Currently, the view controller acts as the registrar for all plugins, so the | ||
| // name is ignored. It is part of the API to reduce churn in the future when | ||
| // aligning more closely with the Flutter registrar system. |
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.
Out of curiosity, what does the Flutter registrar system do with the name?
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.
Registrars are one-per-plugin in Flutter, so the name is used as a map key for finding the correct registrar.
| * from existing Flutter platforms, which default to the standard codec; this is to preserve | ||
| * backwards compatibility for FLEPlugin until the breaking change for the platform messages API. | ||
| */ | ||
| @property(nonnull, readonly) id<FLEMethodCodec> codec; |
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.
I guess this is no longer needed because the type of codec is now private to the plugin and doesn't need to be viewed by anything outside?
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.
More than that, there's no longer necessarily such thing as "the type of codec" for a plugin. A plugin can have any number of channels it wants (including zero, theoretically), and they don't all necessarily have to use the same codec.
stuartmorgan-g
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.
I don't understand why MethodChannel and MessageChannel are asymmetric (e.g. there's an addMethodCallDelegate but no addMessageCallDelegate).
I'm not sure either (beyond the not-very-illuminating answer of "it matches the iOS Flutter API"). Perhaps just an expectation that MethodChannel is much more common.
(addMethodCallDelegate: is kind of an odd method in my opinion, because it doesn't actually do anything with the registrar. It's purely a convenience method that creates a block with a specific callback. That code could just as easily be inline in the plugins, and addMethodCallDelegate: removed.)
| * from existing Flutter platforms, which default to the standard codec; this is to preserve | ||
| * backwards compatibility for FLEPlugin until the breaking change for the platform messages API. | ||
| */ | ||
| @property(nonnull, readonly) id<FLEMethodCodec> codec; |
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.
More than that, there's no longer necessarily such thing as "the type of codec" for a plugin. A plugin can have any number of channels it wants (including zero, theoretically), and they don't all necessarily have to use the same codec.
| - (id<FLEPluginRegistrar>)registrarForPlugin:(NSString *)pluginName { | ||
| // Currently, the view controller acts as the registrar for all plugins, so the | ||
| // name is ignored. It is part of the API to reduce churn in the future when | ||
| // aligning more closely with the Flutter registrar system. |
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.
Registrars are one-per-plugin in Flutter, so the name is used as a map key for finding the correct registrar.
| flutterViewController.add(FLEMenubarPlugin()) | ||
| FLEColorPanelPlugin.register( | ||
| with: flutterViewController.registrar(forPlugin: "FLEColorPanelPlugin")) | ||
| FLEFileChooserPlugin.register( |
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.
the wiki seem to contain only the usage in ObjectiveC, this I guess this code shows correctly how to use the file chooser plugin for instance
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.
the wiki seem to contain only the usage in ObjectiveC
Calling plugin APIs from Swift is exactly the same as calling any other ObjC code from Swift.
Changes FLEPlugin to be a subset of iOS's FlutterPlugin's API, including
changing the plugin creation and registration system. Plugins now create
and manage their own channels, instead of relying on FLEViewController
for high-level message APIs.
This is a breaking change for any FLEPlugin implementation, as well as
application-level code that was calling addPlugin:.
This essentially completes issue #102 for macOS.