Rework macOS plugin API #56
Conversation
There was a problem hiding this comment.
Neat! This makes the plugin code much cleaner. I'll let @awdavies take a look, but LGTM.
| - (nonnull instancetype)init NS_UNAVAILABLE; | ||
|
|
||
| /** | ||
| * The error code, as a string. |
There was a problem hiding this comment.
What determines the namespace of error codes?
There was a problem hiding this comment.
I haven't seen any guidance about this. Given that it's a string I suspect it doesn't really matter much, especially since the location where the exception is raised on the Flutter side should prevent any potential confusion if two very different plugins happened to use the same string.
macos/library/FLEChannels.h
Outdated
| /** | ||
| * The arguments to the method being called, if any. | ||
| * | ||
| * This object must be serializable to JSON. |
There was a problem hiding this comment.
You might want to note somewhere that NSJSONSerialization is what does the transformation.
There was a problem hiding this comment.
I added a link to https://docs.flutter.io/flutter/services/JSONMessageCodec-class.html since that explicitly lists the supported types (and the goal is to move to having an FLEJSONMessageCodec, as well as versions of the others).
| * Returns the serialized JSON data that should be sent to the engine for the given | ||
| * FLEMethodResult argument. | ||
| * | ||
| * // TODO: Move this logic into method codecs. |
There was a problem hiding this comment.
What's the benefit of moving this logic? I'm not disagreeing, just curious.
There was a problem hiding this comment.
Consistency with the iOS API, and flexibility to use something other than JSON.
| */ | ||
| - (BOOL)sendPlatformMessage:(nonnull NSData *)message onChannel:(nonnull NSString *)channel; | ||
| - (void)invokeMethod:(nonnull NSString *)method | ||
| arguments:(nullable id)arguments |
There was a problem hiding this comment.
nit: "withArguments:(nullable id)arguments" might be more idiomatic?
There was a problem hiding this comment.
I agree... but the corresponding iOS API on the method channel object (which I plan to introduce at some point here as well for more consistency) is invokeMethod:arguments:.
I struggled with this a fair amount in the patch. For instance, why is this two parameters at all, as opposed to taking the existing MethodCall object that exists to abstract them, and is used in calls going the other way? Another example is the MethodCall object itself, which has properties called methodName (why prefix a property on a method object with 'method'?) and arguments (why is this inconsistent about the prefix?).
Ultimately, I feel that mimicking the existing API as closely as possible is the more important consideration, even when it means going against idiomatic ObjC naming, to minimize friction in porting plugins (or even sharing them, with some #defines for class names).
| FLEMethodCall *methodCall = | ||
| messageObject ? [FLEMethodCall methodCallFromMessage:messageObject] : nil; | ||
|
|
||
| __block const FlutterPlatformMessageResponseHandle *responseHandle = message->response_handle; |
There was a problem hiding this comment.
It might be worth a comment explaining that responseHandle isn't just a convenient intermediate variable but is captured in the block below. (Maybe this is obvious to people who write Objective-C more regularly though.)
There was a problem hiding this comment.
I had a comment at first, but once I added the __block it seemed redundant.
|
|
||
| id<FLEPlugin> plugin = _plugins[channel]; | ||
| if (!plugin) { | ||
| resultHandler(FLEMethodNotImplemented); |
There was a problem hiding this comment.
It seems like this would warrant a more specific error than FLEMethodNotImplemented? Is this the same in the iOS implementation?
There was a problem hiding this comment.
Is this the same in the iOS implementation?
I believe so, yes:
https://github.com/flutter/engine/blob/58e84c8bf0b6304649340e6eaad988602961d7f6/shell/platform/darwin/ios/framework/Source/platform_message_router.mm#L38
An empty reply is what the higher-level code sends for FLEMethodNotImplemented.
Replaces the synchronous plugin response using raw data with an asynchronous response using a much higher-level abstraction that makes sending non-empty responses much easier. The API changes are based very closely on the iOS Flutter plugin API, so that it will be familiar to Flutter plugin developers, and make code sharing easier. This includes using that APIs naming and terminology whenever possible. This is an incremental change, in that some aspects of the iOS API (most notably message channel registration and method codecs) are still not present in this API. A future change will further align the macOS API with iOS, but this portion makes most of the changes that would significantly change the structure of a plugin (allowing for rich, async responses). This addresses the macOS portion of issue 45.
stuartmorgan
force-pushed
the
plugin-macos-rework
branch
from
047c255 to
884924a
Compare
Replaces the synchronous plugin response using raw data with an
asynchronous response using a much higher-level abstraction that makes
sending non-empty responses much easier.
The API changes are based very closely on the iOS Flutter plugin API,
so that it will be familiar to Flutter plugin developers, and make code
sharing easier. This includes using that APIs naming and terminology
whenever possible.
This is an incremental change, in that some aspects of the iOS API (most
notably message channel registration and method codecs) are still not
present in this API. A future change will further align the macOS API
with iOS, but this portion makes most of the changes that would
significantly change the structure of a plugin (allowing for rich, async
responses).
This addresses the macOS portion of issue #45.
Also fixes issue #13 on macOS.