Skip to content

Fix issue 504 #512

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

Merged
merged 12 commits into from
Apr 12, 2022
Merged

Fix issue 504 #512

merged 12 commits into from
Apr 12, 2022

Conversation

moljac
Copy link
Contributor

@moljac moljac commented Mar 16, 2022

Does this change any of the generated binding API's?

Yes. Adds overloaded CreateIntent method with string parameter.

Fix for #504

Describe your contribution

CreateIntent parameter fix

  • copied generated code + added necessary cast
  • removed method with remove-node

@moljac
Copy link
Contributor Author

moljac commented Mar 16, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jpobst
Copy link
Contributor

jpobst commented Mar 17, 2022

Do we know why the string version isn't getting bound?

Does this need to be virtual since #504 mentions overridding the method is the issue?

@moljac
Copy link
Contributor Author

moljac commented Mar 27, 2022

Do we know why the string version isn't getting bound?

From what I saw - generics. IIRC something in the form SomeJavaType<string>, but not 100% sure. I would need to go back. I begun fixing issues while waiting to be unblocked with other problems.

Does this need to be virtual since #504 mentions overridding the method is the issue?

No. It was parameter type erasure.

@jpobst
Copy link
Contributor

jpobst commented Apr 5, 2022

I don't think I understand how this will solve the reported issue. The user wants to override this method:

public Intent createIntent(Context context, String input) { ... }

The method this PR adds is not virtual, thus it cannot be overridden.

Additionally, even making this method virtual will not work, because it is not hooked into the Java infrastructure, so Java will never call it.

Given that the existing method does not appear to work, I think we should look at fixing it instead of adding this additional overload.

@jpobst
Copy link
Contributor

jpobst commented Apr 5, 2022

I think the reported Java error is because the type was modified using type instead of managedType:
https://github.com/xamarin/AndroidX/blob/main/source/androidx.activity/activity/Transforms/Metadata.xml#L218-L223

Using managedType would allow the JLO version to work. It still would not be a perfect match for the documented API, but using it should work.

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

After discussing with @jonpryor, we aren't entirely sure if this fix will work at runtime. I'm going to do some additional testing to see. Please hold off on merging this while I'll investigate. Thanks!

Copy link
Contributor

@jpobst jpobst left a comment

Choose a reason for hiding this comment

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

Testing looks good, so this is fine to commit now:
dotnet/java-interop#967 (comment)

@moljac moljac merged commit 9525eb5 into main Apr 12, 2022
@moljac moljac deleted the fix-issue-504 branch April 12, 2022 06:30
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Apr 19, 2022
Fixes: #967

Context: dotnet/android-libraries#504
Context: #424
Context: #586
Context: #918

Java generics continues to be a "difficulty" in creating bindings.
Consider [`ActivityResultContracts.RequestPermission`][0]:

	// Java
	public abstract /* partial */ class ActivityResultContract<I, O> {
	    public abstract Intent createIntent(Context context, I input);
	}
	public /* partial */ class /* ActivityResultContracts. */ RequestPermission
	    extends ActivityResultContract<String, Boolean>
	{
	    @OverRide public Intent createIntent(Context context, String input) {…}
	}

The JNI Signature for
`ActivityResultContracts.RequestPermission.createIntent()` is
`(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;`,
i.e. `class-parse` & `generator` believe that the `input` parameter
is of type `String`, which we bind by default as `string`.  Thus:

	// C#
	public abstract partial class ActivityResultContract {
	    public abstract Intent CreateIntent (Context? context, Java.Lang.Object? input) => …
	}
	public partial class /* ActivityResultContracts. */ RequestPermission {
	    public override Intent CreateIntent (Context? context, string? input) => …
	}

This fails to compile with a [CS0115][1]:

	'RequestPermission.CreateIntent(Context?, string?)': no suitable method found to override

as the `input` parameter of `RequestPermission.CreateIntent()` changes
from `Java.Lang.Object?` to `string?`.

We can attempt to address this via Metadata:

	<attr
	  path="/api/package[@name='androidx.activity.result.contract']/class[@name='ActivityResultContracts.RequestPermission']/method[@name='createIntent' and count(parameter)=2 and parameter[1][@type='android.content.Context'] and parameter[2][@type='java.lang.String']]"
	  name="managedType"
	>Java.Lang.Object</attr>

This fixes one error, as `generator` now emits:

	public partial class /* ActivityResultContracts. */ RequestPermission {
	    [Register ("createIntent", "(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;", "")]
	    public override unsafe global::Android.Content.Intent CreateIntent (global::Android.Content.Context context, global::Java.Lang.Object input)
	    {
	        const string __id = "createIntent.(Landroid/content/Context;Ljava/lang/String;)Landroid/content/Intent;";
	        IntPtr native_input = JNIEnv.NewString (input);
	        try {
	            JniArgumentValue* __args = stackalloc JniArgumentValue [2];
	            __args [0] = new JniArgumentValue ((context == null) ? IntPtr.Zero : ((global::Java.Lang.Object) context).Handle);
	            __args [1] = new JniArgumentValue (native_input);
	            var __rm = _members.InstanceMethods.InvokeAbstractObjectMethod (__id, this, __args);
	            return global::Java.Lang.Object.GetObject<global::Android.Content.Intent> (__rm.Handle, JniHandleOwnership.TransferLocalRef);
	        } finally {
	            JNIEnv.DeleteLocalRef (native_input);
	            global::System.GC.KeepAlive (context);
	            global::System.GC.KeepAlive (input);
	        }
	    }
	}

The `override` method declaration is correct.

However, this introduces a [CS1503][2] error:

	IntPtr native_input = JNIEnv.NewString (input);
	// Error CS1503 Argument 1: cannot convert from 'Java.Lang.Object' to 'string?'

The workaround is to remove `createIntent()` ~entirely, and manually
bind it, as done in dotnet/android-libraries#512.

Fix this issue by always emitting a cast to `(string)` as
part of the `JNIEnv.NewString()` invocation, instead emitting:

	IntPtr native_input = JNIEnv.NewString ((string?) input);

This works because `Java.Lang.Object` defines an
[explicit conversion to `string?`][3], and if a `Java.Lang.String`
instance is provided to the `input` parameter, it's equivalent to
calling `.ToString()`.

This fix allows the original suggested Metadata solution to work.

[0]: https://developer.android.com/reference/androidx/activity/result/contract/ActivityResultContracts.RequestPermission
[1]: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/cs0115
[2]: https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs1503
[3]: https://github.com/xamarin/xamarin-android/blob/a58d4e9706455227eabb6e5b5103b25da716688b/src/Mono.Android/Java.Lang/Object.cs#L434-L439
@jpobst jpobst mentioned this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActivityResultContracts.CreateDocument class implementation error
2 participants