Refactor JObject structure

[HosseinYousefi]

This PR adds changes so that APIs use JReference objects instead of Pointer<Void>s.

Closes #549. Closes #985.

[github-actions]

PR Health

Changelog Entry ✔️

Details

Package	Changed Files

Changes to files need to be accounted for in their respective changelogs.

Coverage ⚠️

Details

File	Coverage
pkgs/jni/example/lib/main.dart	💔 Not covered
pkgs/jni/lib/internal_helpers_for_jnigen.dart	💔 Not covered
pkgs/jni/lib/jni.dart	💔 Not covered
pkgs/jni/lib/src/accessors.dart	💔 Not covered
pkgs/jni/lib/src/errors.dart	💔 Not covered
pkgs/jni/lib/src/jarray.dart	💔 Not covered
pkgs/jni/lib/src/jclass.dart	💔 Not covered
pkgs/jni/lib/src/jni.dart	💔 Not covered
pkgs/jni/lib/src/jobject.dart	💔 Not covered
pkgs/jni/lib/src/jprimitives.dart	💔 Not covered
pkgs/jni/lib/src/jreference.dart	💔 Not covered
pkgs/jni/lib/src/jvalues.dart	💔 Not covered
pkgs/jni/lib/src/lang/jboolean.dart	💔 Not covered
pkgs/jni/lib/src/lang/jbyte.dart	💔 Not covered
pkgs/jni/lib/src/lang/jcharacter.dart	💔 Not covered
pkgs/jni/lib/src/lang/jdouble.dart	💔 Not covered
pkgs/jni/lib/src/lang/jfloat.dart	💔 Not covered
pkgs/jni/lib/src/lang/jinteger.dart	💔 Not covered
pkgs/jni/lib/src/lang/jlong.dart	💔 Not covered
pkgs/jni/lib/src/lang/jnumber.dart	💔 Not covered
pkgs/jni/lib/src/lang/jshort.dart	💔 Not covered
pkgs/jni/lib/src/lang/jstring.dart	💔 Not covered
pkgs/jni/lib/src/method_invocation.dart	💔 Not covered
pkgs/jni/lib/src/nio/jbuffer.dart	💔 Not covered
pkgs/jni/lib/src/nio/jbyte_buffer.dart	💔 Not covered
pkgs/jni/lib/src/third_party/global_env_extensions.dart	💔 Not covered
pkgs/jni/lib/src/types.dart	💔 Not covered
pkgs/jni/lib/src/util/jiterator.dart	💔 Not covered
pkgs/jni/lib/src/util/jlist.dart	💔 Not covered
pkgs/jni/lib/src/util/jmap.dart	💔 Not covered
pkgs/jni/lib/src/util/jset.dart	💔 Not covered
pkgs/jni/tool/wrapper_generators/generate_dart_extensions.dart	💔 Not covered
pkgs/jnigen/example/in_app_java/lib/android_utils.dart	💔 Not covered
pkgs/jnigen/example/in_app_java/lib/main.dart	💔 Not covered
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart	💔 Not covered
pkgs/jnigen/example/notification_plugin/example/lib/main.dart	💔 Not covered
pkgs/jnigen/example/notification_plugin/lib/notifications.dart	💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/dart_example/bin/pdf_info.dart	💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/example/lib/main.dart	💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart	💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart	💔 Not covered
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart	💔 Not covered
pkgs/jnigen/lib/src/bindings/dart_generator.dart	💔 Not covered
pkgs/jnigen/lib/src/bindings/renamer.dart	💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check

License Headers ⚠️

Details

// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

Files
pkgs/jni/lib/src/lang/jcharacter.dart
pkgs/jni/lib/src/third_party/global_env_extensions.dart
pkgs/jnigen/example/in_app_java/lib/android_utils.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart

All source files should start with a license header.

Unrelated files missing license headers

Files
pkgs/ffi/example/main.dart
pkgs/ffigen/example/libclang-example/generated_bindings.dart
pkgs/ffigen/example/objective_c/avf_audio_bindings.dart
pkgs/ffigen/example/shared_bindings/generate.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart
pkgs/ffigen/example/simple/generated_bindings.dart
pkgs/ffigen/example/swift/swift_api_bindings.dart
pkgs/ffigen/lib/src/config_provider/config_spec.dart
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart
pkgs/jnigen/android_test_runner/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/_init.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart
pkgs/jnigen/lib/src/bindings/descriptor.dart
pkgs/jnigen/lib/src/elements/elements.g.dart
pkgs/jnigen/test/jackson_core_test/third_party/c_based/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/test/jackson_core_test/third_party/dart_only/dart_bindings/com/fasterxml/jackson/core/_package.dart
pkgs/jnigen/tool/command_runner.dart

This check can be disabled by tagging the PR with skip-license-check

Package publish validation ✔️

Details

Package	Version	Status
package:ffi	2.1.2	already published at pub.dev
package:ffigen	12.0.0-wip	WIP (no publish necessary)
package:jni	0.8.0-wip	WIP (no publish necessary)
package:jnigen	0.8.0-wip	WIP (no publish necessary)
package:native_assets_builder	0.5.0	already published at pub.dev
package:native_assets_cli	0.4.3-wip	WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

[dcharkes]

I like the new JClass!

Initial round of comments

[dcharkes]

Do we need a JFunctionType to be able to call .signature on it?

[dcharkes]

static final _name = r"java/lang/Boolean";

And then in JBooleanType the signature as 'L${JBoolean._name}'?

Not sure if _name is the right name for the field, but if we're moving towards constructing signatures from smaller components we might as well do it nicely.

Ditto for the other types.

(Feel free to punt to another PR.)

[HosseinYousefi]

We could also do _class = type.jClass here but that means type is created. I treat every binding available in package:jni to be generated by package:jnigen and it will soon be this way.

[HosseinYousefi]

Consider making this an actual object so the jClass doesn't have to be passed again.

It's actually somewhat interesting why JNI spec needs this jClass. And Android implementation doesn't even use it, which makes sense since jmethodID has the class information. So at least for android, we could pass null but I can't go with one implementation detail instead of spec of course.

Back to the original point: This makes it so that we create an extra object (instead of an extension type which uses the same underlying JMethodIDPtr). The general question to ask is whether we want to optimize for use in package:jnigen or for direct use. The reason that I removed a bunch of sugar from JObject has been this as well. Users can always create their extensions like

extension CallMethod on JObject {
  /// This is fine if you're calling something once, but it is not efficient for 
  /// calling the same method multiple times, as both the class and the methodID
  /// would need to be fetched on each call.
  DartT callMethodByName<JavaT, DartT>(String name, String signature,
      JCallable<JavaT, DartT> type, List<dynamic> args) {
    // ...
  }
}

I chose not to do this myself since it increases the amount of code we have to maintain, and as far as I know, no one is using package:jni directly. This nicer to use package:jni could even be a separate package.

An alternative we can consider is to have an object version for package:jni/jni.dart and an extension type version for package:jni/internal_helpers_for_jnigen.dart. This allows us to make it even nicer, for example passing a String signature for both fields and methods is not ideal for users to do. Instead we could get a JFunctionType and a JType and find the signature from these. But again, these all optimize the experience for the non-existent (so far at least) package:jni user, as opposed to package:jnigen as we have access to the exact signature there.

To allow the possibility of this change in the future, I could change the naming of instanceMethod to something like instanceMethodId and JInstanceMethod to JInstanceMethodId, this way we can add instanceMethod that returns an object as opposed to an extension type and that is more powerful later on, wdyt?

One might argue that since most type classes are not generic anyways, even the fact that we're first creating the type class and then creating the object by typeClass.fromReference(...) is doing extra work. Instead when generating bindings, since we know the name of the class, it can simply be written like ActualClass.fromReference(...). That's why I have a internal type called referenceType that just returns JReference instead. This could be used to reduce the amount of object creation in JNIgen generated bindings.

Do we need a JFunctionType to be able to call .signature on it?

Great minds think alike! But this means maintaining more code, maybe something we want to add if users ask us. We have an issue for it too: #770.

cc/ @mkustermann I would like to know your input as well.

[dcharkes]

To allow the possibility of this change in the future, I could change the naming of instanceMethod to something like instanceMethodId and JInstanceMethod to JInstanceMethodId, this way we can add instanceMethod that returns an object as opposed to an extension type and that is more powerful later on, wdyt?

Since we don't have many users of package:jni, I wouldn't worry too much about having to do a breaking change to it later. However, once we do a breaking change every package having generated bindings needs to update at the same time. So, if with something minor we can avoid a potential breaking change later we should probably do it. 👍

[coveralls]

coverage: 88.892%. first build
when pulling e63c6ac on refactor-jreference-2
into 3555b62 on main.

[HosseinYousefi]

@dcharkes Thanks for the initial round of review. I changed the method names to allow us to add more powerful abstractions for package:jni standalone users later on.

The coverage seems to be low since it's not adding the tests in package:jnigen that also cover key functionalities of package:jni. I'll try to fix this problem in a follow up PR.

I'm still not done with all the changes but the PR is large as is, so let's land these ones for now.