dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
102 stars 35 forks source link

Improving usability of ObjC protocol listener methods #1231

Closed liamappelbe closed 1 day ago

liamappelbe commented 1 week ago

Implementing a protocol using listener methods is a bit of a pain atm. For our examples, we'll be using MyProtocol from protocol_test.m:

@protocol SuperProtocol<NSObject>
@required
- (NSString*)instanceMethod:(NSString*)s withDouble:(double)x;
@end

@protocol MyProtocol<SuperProtocol>
@optional
- (int32_t)optionalMethod:(SomeStruct)s;
@optional
- (void)voidMethod:(int32_t)x;
@end

The generated bindings for MyProtocol look like this:

/// MyProtocol
abstract final class MyProtocol {
  /// Builds an object that implements the MyProtocol protocol. To implement
  /// multiple protocols, use [addToBuilder] or [objc.ObjCProtocolBuilder] directly.
  static objc.ObjCObjectBase implement(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    final builder = objc.ObjCProtocolBuilder();
    builder.implementMethod(MyProtocol.optionalMethod_, optionalMethod_);
    builder.implementMethod(MyProtocol.voidMethod_, voidMethod_);
    builder.implementMethod(
        MyProtocol.instanceMethod_withDouble_, instanceMethod_withDouble_);
    return builder.build();
  }

  /// Adds the implementation of the MyProtocol protocol to an existing
  /// [objc.ObjCProtocolBuilder].
  static void addToBuilder(objc.ObjCProtocolBuilder builder,
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    builder.implementMethod(MyProtocol.optionalMethod_, optionalMethod_);
    builder.implementMethod(MyProtocol.voidMethod_, voidMethod_);
    builder.implementMethod(
        MyProtocol.instanceMethod_withDouble_, instanceMethod_withDouble_);
  }

  /// optionalMethod:
  static final optionalMethod_ = objc.ObjCProtocolMethod(...);

  /// voidMethod:
  static final voidMethod_ = objc.ObjCProtocolListenableMethod(...);

  /// instanceMethod:withDouble:
  static final instanceMethod_withDouble_ = objc.ObjCProtocolMethod(...);
}

For ordinary (isolate local, blocking) methods implement and addToBuilder are ergonomic enough. But for listener methods users have to use ObjCProtocolBuilder.implementMethodAsListener directly. There are a few options to improve the situation.

1. Status quo

final protocolBuilder = ObjCProtocolBuilder();
protocolBuilder.implementMethodAsListener(MyProtocol.voidMethod_,
    (int x) {
  ...
});

The main downside of the status quo approach is that implementMethodAsListener takes a Function with no type information. So the type of the arguments can't be inferred, and must be given explicitly. For non-trivial method signatures this can be verbose.

MyProtocol.implement/addToBuilder doesn't have this problem because the named params are explicitly typed.

2. Default to listeners for void methods

  static objc.ObjCObjectBase implement(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {
    final builder = objc.ObjCProtocolBuilder();
    builder.implementMethod(MyProtocol.optionalMethod_, optionalMethod_);
    builder.implementMethodAsListener(MyProtocol.voidMethod_, voidMethod_);  // <- Here
    builder.implementMethod(
        MyProtocol.instanceMethod_withDouble_, instanceMethod_withDouble_);
    return builder.build();
  }

MyProtocol.implement(...
  voidMethod: (x) {...});

In Apple APIs, it's pretty much always the case that we don't know what thread a delegate's method is called on, and users will need to use listeners. So maybe it would be reasonable to default to listeners rather that isolate local callbacks? The downside of this is that the behavior would be confusing if you aren't aware of it.

3. Give the implement method 2 variants of the named param for listeners

  static objc.ObjCObjectBase implement(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      void Function(int)? voidMethod_asListener,  // <- Add this
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {

MyProtocol.implement(...
  voidMethod_asListener: (x) {...});

If users want to implement voidMethod: using an isolate local callback, they can use voidMethod_, and if they want to implement it as a listener they can use voidMethod_asListener.

The main downside of this is it makes the implement method a bit noisy and harder to read. There's also not a 1:1 relationship between the params and the methods anymore, so it's a bit more confusing. Another downside is that there's a fair chance that we will add more types of callback in the future, and supporting those in this way will mean we have O(M*C) params (where M=num methods, C=num callback types).

An alternative that at least solves the O(M*C) problem is to add an enum param instead:

  static objc.ObjCObjectBase implement(
      {int Function(SomeStruct)? optionalMethod_,
      void Function(int)? voidMethod_,
      CallbackType voidMethod_type = CallbackType.isolateLocal,  // <- Add this
      required objc.NSString Function(objc.NSString, double)
          instanceMethod_withDouble_}) {

MyProtocol.implement(...
  voidMethod_: (x) {...},
  voidMethod_type: CallbackType.listener);

4. Add an implement/implementAsListener method to the protocol bindings for each method.

  static void implement_voidMethod_(objc.ObjCProtocolBuilder builder,
      void Function(int)? voidMethod_) {
    builder.implementMethod(MyProtocol.voidMethod_, voidMethod_);
  }

  static void implementAsListener_voidMethod_(objc.ObjCProtocolBuilder builder,
      void Function(int)? voidMethod_) {
    builder.implementMethodAsListener(MyProtocol.voidMethod_, voidMethod_);
  }

MyProtocol.implementAsListener_voidMethod_(builder, (x) { ... });

Under this approach we could also make the method objects private, and construct the blocks in these implement methods instead of in the ObjCProtocolBuilder, which would be cleaner.

The downside is it makes the protocol bindings a lot more verbose. This is also a case where we'll have O(M*C) methods if we add more callback types.

5. The method object has a correctly typed implement/implementAsListener method

If we pass the function type as a generic param to ObjCProtocolMethod/ ObjCProtocolListenableMethod, we can add correctly typed implement and implementAsListener methods.

  static final voidMethod_ = objc.ObjCProtocolListenableMethod<void Function(int)>(...);

MyProtocol.voidMethod_.implementAsListener(builder, (x) { ... });
brianquinlan commented 1 week ago

(2) is too magical. Would it be possible to do (3) by just having two implement methods i.e. implement and implementAsListener?

liamappelbe commented 1 week ago

Would it be possible to do (3) by just having two implement methods i.e. implement and implementAsListener?

The idea being that if a user wants one listener method, the probably want all the methods to be listeners? Sure, we can do that.

I think implementing (5) would be a good idea anyway as it will let me clean things up a bit, but there's no reason I can't also do your suggestion.

dcharkes commented 1 week ago

My hunch is that (5) is the cleanest, as we can expose the right implementation strategies per method. (We'll end up with a bunch of sibbling types to ObjCProtocolListenableMethod for the valid cross product of NativeCallable's but that might actually be a very sparse one. E.g. NativeCallable.blocking and NativeCallable.isolateLocal will always both be available I think.)

And layering (3) version B

An alternative that at least solves the O(M*C) problem is to add an enum param instead:

or version C

Would it be possible to do (3) by just having two implement methods i.e. implement and implementAsListener?

on top of that.