Skip to content
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

[pigeon] Kotlin implementation for ProxyApis #6371

Merged
merged 115 commits into from
Aug 30, 2024

Conversation

bparrishMines
Copy link
Contributor

@bparrishMines bparrishMines commented Mar 21, 2024

Kotlin portion of flutter/flutter#134777

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@bparrishMines bparrishMines changed the title Pigeon kotlin split [pigeon] Initial kotlin implementation for ProxyApis Mar 22, 2024
@bparrishMines bparrishMines marked this pull request as ready for review March 22, 2024 19:23
Comment on lines 370 to 426
abstract class PigeonProxyApiBaseCodec(
val binaryMessenger: BinaryMessenger,
val instanceManager: PigeonInstanceManager
) : StandardMessageCodec() {
/**
* An implementation of [PigeonApiProxyApiTestClass] used to add a new Dart instance of
* `ProxyApiTestClass` to the Dart `InstanceManager`.
*/
abstract fun getPigeonApiProxyApiTestClass(): PigeonApiProxyApiTestClass

/**
* An implementation of [PigeonApiProxyApiSuperClass] used to add a new Dart instance of
* `ProxyApiSuperClass` to the Dart `InstanceManager`.
*/
abstract fun getPigeonApiProxyApiSuperClass(): PigeonApiProxyApiSuperClass

/**
* An implementation of [PigeonApiProxyApiInterface] used to add a new Dart instance of
* `ProxyApiInterface` to the Dart `InstanceManager`.
*/
abstract fun getPigeonApiProxyApiInterface(): PigeonApiProxyApiInterface

fun setUpMessageHandlers() {
PigeonApiProxyApiTestClass.setUpMessageHandlers(
binaryMessenger, getPigeonApiProxyApiTestClass())
PigeonApiProxyApiSuperClass.setUpMessageHandlers(
binaryMessenger, getPigeonApiProxyApiSuperClass())
}

override fun readValueOfType(type: Byte, buffer: ByteBuffer): Any? {
return when (type) {
128.toByte() -> {
return instanceManager.getInstance(
readValue(buffer).let { if (it is Int) it.toLong() else it as Long })
}
else -> super.readValueOfType(type, buffer)
}
}

override fun writeValue(stream: ByteArrayOutputStream, value: Any?) {
if (value is ProxyApiTestClass) {
getPigeonApiProxyApiTestClass().pigeon_newInstance(value) {}
} else if (value is com.example.test_plugin.ProxyApiSuperClass) {
getPigeonApiProxyApiSuperClass().pigeon_newInstance(value) {}
} else if (value is ProxyApiInterface) {
getPigeonApiProxyApiInterface().pigeon_newInstance(value) {}
}

when {
instanceManager.containsInstance(value) -> {
stream.write(128)
writeValue(stream, instanceManager.getIdentifierForStrongReference(value))
}
else -> super.writeValue(stream, value)
}
}
}
Copy link
Contributor Author

@bparrishMines bparrishMines Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camsim99 FYI

@stuartmorgan @tarrinneal This is one of the places where the design changes from the implementation in the doc. I wanted to improve the scenario of when a method returns a ProxyApi or a list/map of a ProxyApi:

class Basket {
  Apple giveMeAnApple();
  List<Apple> giveMeApples();
}

Typically we solve this by manually handling adding a new instance when we implement the api:

class PigeonApiBasketImpl {
  Apple giveMeAnApple(Basket instance) {
    final Apple apple = instance.giveMeAnApple();
    if (!instanceManager.containsInstance(apple)) {
      instanceManager.addHostCreatedInstance(apple);
    }

    return apple;
  }

  List<Apple> giveMeApples(Basket instance) {
    final List<Apple> apples = instance.giveMeApples();
    for (Apple apple in apples) {
      if (!instanceManager.containsInstance(apple)) {
        instanceManager.addHostCreatedInstance(apple);
      }
    }

    return apples;
  }
}

This change makes the codec the central class that contains all the proxy api implementations, so the writeValue can handle creating new dart instances automatically by calling pigeon_newInstance. And pigeon_newInstance would handle the logic shown in the methods above.

This also:

  • Creates a registrar like class that ensures a user implements each proxy api.
  • Provides access to a central class with every api implementation which prevents the need for multiple instances of the same api implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may end up overlapping with some of the codec work I'm about to do as well. I think it's generally a good idea to create a central codec though. We may end up with multiple codecs implementing each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern at this point is that the number of custom inputs is limited to around 120, so if there are a lot of proxy apis, enums, and or data classes, we may not be able to accommodate them all.

Copy link
Contributor Author

@bparrishMines bparrishMines Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of right now, the generation of the ProxyApi codec is separate from Host/Flutter APIs codecs. Mainly because:

  1. ProxyApis don't support data classes
  2. ProxyApis support other ProxyApis in the codec and Host/Flutter APIs don't.

So they currently don't really share anything. If you create a central custom codec for all the APIs, then ProxyApis should only need one dedicated spot that uses the InstanceManager: https:/flutter/packages/blob/main/packages/pigeon/platform_tests/shared_test_plugin_code/lib/src/generated/proxy_api_tests.gen.dart#L374.

Also, as a side note, since all data classes have an encode/decode could you create a base data class for all the data classes and only use one spot. I realized this wouldn't work since the reader wouldn't know what class to call this on.

@tarrinneal
Copy link
Contributor

Are you planning to add the full implementation and test suite to this pr?

@bparrishMines
Copy link
Contributor Author

Are you planning to add the full implementation and test suite to this pr?

I wasn't planning on doing that for this PR, but I can if you prefer the full implementation. This PR just adds the Kotlin ProxyApi utility classes, including the InstanceManager and the tests for it. And also the bare minimum ProxyApi class declaration.

@bparrishMines bparrishMines changed the title [pigeon] Initial kotlin implementation for ProxyApis [pigeon] Kotlin implementation for ProxyApis Apr 16, 2024
@tarrinneal
Copy link
Contributor

@bparrishMines I think there needs to be some plumbing added to have the kotlin files generate using the tool/generate script

@bparrishMines
Copy link
Contributor Author

@tarrinneal What do you mean? I have been using dart run tool/generate.dart to generate the Kotlin code successfully. Is this the script you are referring to?

@tarrinneal
Copy link
Contributor

@tarrinneal What do you mean? I have been using dart run tool/generate.dart to generate the Kotlin code successfully. Is this the script you are referring to?

yep, If it's working then don't worry about it :)

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more things here, should be easy.

packages/pigeon/lib/generator_tools.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/kotlin_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/kotlin_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/kotlin_generator.dart Outdated Show resolved Hide resolved
packages/pigeon/lib/kotlin_generator.dart Outdated Show resolved Hide resolved
return ProxyApiSuperClass()
}

override fun aBool(pigeon_instance: ProxyApiTestClass): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this change necessary?

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's at a place where we can land it and start making incremental changes as needed. Good job with it all, and thanks for your patience!

@bparrishMines bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 30, 2024
@auto-submit auto-submit bot merged commit c9c0004 into flutter:main Aug 30, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 30, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 30, 2024
flutter/packages@2a0f254...c9c0004

2024-08-30 [email protected] [pigeon] Kotlin implementation for ProxyApis  (flutter/packages#6371)
2024-08-29 [email protected] [in_app_purchase_storekit] Add storekit 2 support for canMakePayments and products (flutter/packages#7473)
2024-08-29 [email protected] [flutter_adaptive_scaffold] Add correct material spacing and panes (flutter/packages#7428)
2024-08-29 [email protected] [many] Upgrade example apps to AGP 8.5.2 (if they were below 8.1.0) (flutter/packages#7521)
2024-08-29 [email protected] [pigeon] allow gen of unused classes (flutter/packages#7529)
2024-08-29 [email protected] Increase deprecation check minimum to iOS 14 and macOS 13 (flutter/packages#7431)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https:/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
vasilich6107 pushed a commit to artflutter/flutter that referenced this pull request Aug 30, 2024
flutter/packages@2a0f254...c9c0004

2024-08-30 [email protected] [pigeon] Kotlin implementation for ProxyApis  (flutter/packages#6371)
2024-08-29 [email protected] [in_app_purchase_storekit] Add storekit 2 support for canMakePayments and products (flutter/packages#7473)
2024-08-29 [email protected] [flutter_adaptive_scaffold] Add correct material spacing and panes (flutter/packages#7428)
2024-08-29 [email protected] [many] Upgrade example apps to AGP 8.5.2 (if they were below 8.1.0) (flutter/packages#7521)
2024-08-29 [email protected] [pigeon] allow gen of unused classes (flutter/packages#7529)
2024-08-29 [email protected] Increase deprecation check minimum to iOS 14 and macOS 13 (flutter/packages#7431)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https:/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
flutter/packages@2a0f254...c9c0004

2024-08-30 [email protected] [pigeon] Kotlin implementation for ProxyApis  (flutter/packages#6371)
2024-08-29 [email protected] [in_app_purchase_storekit] Add storekit 2 support for canMakePayments and products (flutter/packages#7473)
2024-08-29 [email protected] [flutter_adaptive_scaffold] Add correct material spacing and panes (flutter/packages#7428)
2024-08-29 [email protected] [many] Upgrade example apps to AGP 8.5.2 (if they were below 8.1.0) (flutter/packages#7521)
2024-08-29 [email protected] [pigeon] allow gen of unused classes (flutter/packages#7529)
2024-08-29 [email protected] Increase deprecation check minimum to iOS 14 and macOS 13 (flutter/packages#7431)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https:/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon platform-android triage-android Should be looked at in Android triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants