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

'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])' #647

Closed
fernandovm opened this issue May 19, 2020 · 25 comments
Assignees
Labels
generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@fernandovm
Copy link

When I try compile a binding to google mediation mopub adapter I get the error 'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])'

Steps to Reproduce

  1. Try compile the attached sample project Google.Ads.Mediation.Mopub.zip

Expected Behavior

A successful build.

Actual Behavior

1>C:\Users\fernando\source\repos\Google.Ads.Mediation.Mopub\Google.Ads.Mediation.Mopub\obj\Debug\generated\src\Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync.cs(10,23,10,45): error CS0534: 'DownloadDrawablesAsync' does not implement inherited abstract member 'AsyncTask.DoInBackground(params Object[])'

Version Information

Microsoft Visual Studio Enterprise 2019
Version 16.5.5
VisualStudio.16.Release/16.5.5+30104.148
Microsoft .NET Framework
Version 4.8.03752

Installed Version: Enterprise

Xamarin 16.5.000.533 (d16-5@9152e1b)
Visual Studio extension to enable development for Xamarin.iOS and Xamarin.Android.

Xamarin Designer 16.5.0.470 (remotes/origin/d16-5@681de3fd6)
Visual Studio extension to enable Xamarin Designer tools in Visual Studio.

Xamarin Templates 16.5.49 (0904f41)
Templates for building iOS, Android, and Windows apps with Xamarin and Xamarin.Forms.

Xamarin.Android SDK 10.2.0.100 (d16-5/988c811)
Xamarin.Android Reference Assemblies and MSBuild support.
Mono: c0c5c78
Java.Interop: xamarin/java.interop/d16-5@fc18c54
ProGuard: xamarin/proguard@905836d
SQLite: xamarin/sqlite@46204c4
Xamarin.Android Tools: xamarin/xamarin-android-tools/d16-5@9f4ed4b

Xamarin.iOS and Xamarin.Mac SDK 13.16.0.13 (b75deaf)
Xamarin.iOS and Xamarin.Mac Reference Assemblies and MSBuild support.

@grendello grendello transferred this issue from dotnet/android May 19, 2020
@grendello grendello assigned moljac and unassigned jpobst May 19, 2020
@moljac
Copy link

moljac commented May 19, 2020

@fernandovm
If bindings project would be that easy... We are trying to make bindings projects easy, but it will work only in ideal case.

In your case you are inheriting a type and binding generator generates code w/o compiling (or even better semantically analysing the code), so the only way is to tweak class inheritance (metadata) with metadata transforms in metadata.xml:

  1. remove base class - NO-NO
  2. check if the method is generated and why c# has issues with it

So that method was generated:

		// Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
		[Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
		protected virtual unsafe global::System.Collections.Generic.IDictionary<string, global::Android.Graphics.Drawables.Drawable> DoInBackground (params global::Java.Lang.Object[] @params)
		{
			const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
			IntPtr native__params = JNIEnv.NewArray (@params);
			try {
				JniArgumentValue* __args = stackalloc JniArgumentValue [1];
				__args [0] = new JniArgumentValue (native__params);
				var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
				return global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
			} finally {
				if (@params != null) {
					JNIEnv.CopyArray (native__params, @params);
					JNIEnv.DeleteLocalRef (native__params);
				}
			}
		}

So why does it not build?

Because Mopub version returns System.Collections.Generic.IDictionary<string, global::Android.Graphics.Drawables.Drawable>, while AsyncTask returns Java.Lang.Object

see: https://docs.microsoft.com/en-us/dotnet/api/android.os.asynctask.doinbackground?view=xamarin-android-sdk-9#Android_OS_AsyncTask_DoInBackground_Java_Lang_Object___

So if add this to your metadata.xml:

    <attr
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
        name="managedReturn"
        >
        Java.Lang.Object
    </attr>

It should have the same signature as method in the base class, but the error persists.

@jpobst @jonpryor

This will make your code build (at least it did in my code):

    <remove-node
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground']"
        />
    <add-node
        path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']"
        >
        <!--
        java.util.HashMap&lt;java.lang.String, android.graphics.drawable.Drawable&gt;
        -->
      <method 
            visibility="protected" static="false" abstract="false" return="java.lang.Object" name="doInBackground" 
            deprecated="not deprecated" final="false" bridge="false" native="false"
            synchronized="false" synthetic="false" 
            >
        <parameter type="java.lang.Object..."  name="params" >
        </parameter>
      </method>
        
    </add-node>

I will create repro sample in the repo and post it here.

@moljac moljac added the generator Issues binding a Java library (generator, class-parse, etc.) label May 19, 2020
@moljac
Copy link

moljac commented May 19, 2020

I managed to bind v.5.4.1 without any metadata.

There is something in v.5.12.0

OK mvnrepository does not have the latest version, but bintray:

https://bintray.com/mopub/mopub-android-sdk/mopub-android-sdk/_latestVersion

5.12.0 builds w/o issues and metadata too

https://github.com/HolisticWare/HolisticWareComponents/tree/master/XPlat/Mopub

Seems that new and old style projects (and tooling) do make the difference:

https://github.com/HolisticWare/HolisticWareComponents/blob/master/XPlat/Mopub/source/Mopub.XamarinAndroid/Mopub.XamarinAndroid.csproj

@jpobst
Copy link
Contributor

jpobst commented May 19, 2020

I'm not sure why managedReturn isn't good enough. Switching it to return will build:

<attr
    path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
    name="return">
    Java.Lang.Object
</attr>

That's generally frowned upon as it messes with the type that gets sent to Java, so you will need to test it before you ship it. (This is effectively what @moljac's <remove-node>/<add-node> is doing.)

@jpobst
Copy link
Contributor

jpobst commented May 19, 2020

For internal investigation:

There's 2 issues with what gets generated for managedReturn:

[Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
protected virtual unsafe global::Java.Lang.Object DoInBackground (params global::Java.Lang.Object[] @params)
{
    const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
    IntPtr native__params = JNIEnv.NewArray (@params);
    try {
        JniArgumentValue* __args = stackalloc JniArgumentValue [1];
        __args [0] = new JniArgumentValue (native__params);
        var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
        return global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
    } finally {
        if (@params != null) {
            JNIEnv.CopyArray (native__params, @params);
            JNIEnv.DeleteLocalRef (native__params);
        }
    }
}
  1. It is virtual rather than override. (managedType parameter overrides don't influence override detection #586)
  2. The binding code is returning JavaDictionary<string, Drawable> instead of JLO.

@jonpryor
Copy link
Member

@jpobst asked:

I'm not sure why managedReturn isn't good enough.

It's not good enough because managedReturn only changes the declared return type, and nothing in the body. The problem is that JavaDictionary<T>.FromJniHandle() returns an IDictionary<TKey, TValue>, which is not implicitly convertible to Java.Lang.Object, hence the compilation error.

@jonpryor
Copy link
Member

The problem with the "just change the return type" approach, suggested by @jpobst and @moljac, is that it will break subclassing and overriding, because the return type is used to tell the Java Callable Wrapper generator what Java return type to use. By altering it to be java.lang.Object, the JCW return type will likewise be java.lang.Object, which is not the original java.util.HashMap or a subclass thereof.

The result will be Java Callable Wrapper generator errors from javac.

@moljac
Copy link

moljac commented May 19, 2020

@jonpryor @jpobst

If such metadata fails (like my attempt to change managedReturn) would it be possible to emit warning or error in such case?

@jonpryor
Copy link
Member

What we need is a (not fully specified or thought-out) generator "solution" which allows the Java & managed return types to "diverge" in more significant ways than what managedReturn currently allows.

For example, if there was an implicit conversion from IDictionary & other collection types to Java.Lang.Object, managedReturn would likely work as-is:

partial class /* Java.Lang */ Object {
    public static implicit operator Java.Lang.Object (IDictionary value) => new JavaDictionary (value);
    // ...
}

While this "works," is this something we want to support? (Though Java.Lang.Object already has a ton of implicit conversion operators, so perhaps this is "fine"…)

Alternatively, could we allow generator to detect that the types aren't implicitly convertible, e.g. that IDictionary isn't implicitly convertible to Java.Lang.Object, and in such instances insert an appropriate .JavaCast<T>?

return JavaDictionary<string, Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef)
    .JavaCast<Java.Lang.Object> ();

Though as-is, this would also require some additional "glue code" to add a new .JavaCast<T>() extension method that operates on IDictionary/etc.

@jpobst
Copy link
Contributor

jpobst commented May 19, 2020

@moljac The thing is your metadata doesn't "fail". If it couldn't match anything it would have given you an error like: <attr path=\"{0}\"/> matched no nodes..

All metadata does is modify the api.xml using XPath, it doesn't understand what that actually is, and it is successfully adding the managedReturn attribute.

It just happens to be a successful change that doesn't fix the issue. 😄

@fernandovm
Copy link
Author

fernandovm commented May 19, 2020

Hi @moljac, @jpobst and @jonpryor.. Then, I didn't go as far as you guys, but I have made some tries using the guide on https://gist.github.com/JonDouglas/dda6d8ace7d071b0e8cb before post the issue. My difficulties and points of attention were:

  1. Its not sufficient build the binding, it need works on runtime; Mainly because I think that DownloadDrawablesAsync class will be need on runtime.

  2. I tried change the base class of DownloadDrawablesAsync, I thought that using AsyncTask<> maybe it's the most correct, based in https://github.com/googleads/googleads-mobile-android-mediation/blob/master/ThirdPartyAdapters/mopub/mopub/src/main/java/com/mopub/mobileads/dfp/adapters/DownloadDrawablesAsync.java. But I was unable to adjust the generic parameters correctly. :((

  3. I also did not find ways to force the override for the DoInBackground method, it was always generated with virtual.

@jpobst
Copy link
Contributor

jpobst commented May 19, 2020

One thing to try would be use @moljac's <remove-node> above to not automatically bind this method.

Then manually bind it in an Addition:

public partial class DownloadDrawablesAsync : global::Android.OS.AsyncTask
{
	static Delegate cb_doInBackground_arrayLjava_lang_Object_;
	#pragma warning disable 0169
	static Delegate GetDoInBackground_arrayLjava_lang_Object_Handler ()
	{
		if (cb_doInBackground_arrayLjava_lang_Object_ == null)
			cb_doInBackground_arrayLjava_lang_Object_ = JNINativeWrapper.CreateDelegate ((Func<IntPtr, IntPtr, IntPtr, IntPtr>) n_DoInBackground_arrayLjava_lang_Object_);
		return cb_doInBackground_arrayLjava_lang_Object_;
	}

	static IntPtr n_DoInBackground_arrayLjava_lang_Object_ (IntPtr jnienv, IntPtr native__this, IntPtr native__params)
	{
		global::Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync __this = global::Java.Lang.Object.GetObject<global::Com.Mopub.Mobileads.Dfp.Adapters.DownloadDrawablesAsync> (jnienv, native__this, JniHandleOwnership.DoNotTransfer);
		global::Java.Lang.Object[] @params = (global::Java.Lang.Object[]) JNIEnv.GetArray (native__params, JniHandleOwnership.DoNotTransfer, typeof (global::Java.Lang.Object));
		IntPtr __ret = global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.ToLocalJniHandle ((IDictionary<string, global::Android.Graphics.Drawables.Drawable>)__this.DoInBackground (@params));
		if (@params != null)
			JNIEnv.CopyArray (@params, native__params);
		return __ret;
	}
	#pragma warning restore 0169

	// Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads.dfp.adapters']/class[@name='DownloadDrawablesAsync']/method[@name='doInBackground' and count(parameter)=1 and parameter[1][@type='java.lang.Object...']]"
	[Register ("doInBackground", "([Ljava/lang/Object;)Ljava/util/HashMap;", "GetDoInBackground_arrayLjava_lang_Object_Handler")]
	protected override unsafe Java.Lang.Object DoInBackground (params global::Java.Lang.Object[] @params)
	{
		const string __id = "doInBackground.([Ljava/lang/Object;)Ljava/util/HashMap;";
		IntPtr native__params = JNIEnv.NewArray (@params);
		try {
			JniArgumentValue* __args = stackalloc JniArgumentValue [1];
			__args [0] = new JniArgumentValue (native__params);
			var __rm = _members.InstanceMethods.InvokeVirtualObjectMethod (__id, this, __args);
			return (Java.Lang.Object)global::Android.Runtime.JavaDictionary<string, global::Android.Graphics.Drawables.Drawable>.FromJniHandle (__rm.Handle, JniHandleOwnership.TransferLocalRef);
		} finally {
			if (@params != null) {
				JNIEnv.CopyArray (native__params, @params);
				JNIEnv.DeleteLocalRef (native__params);
			}
		}
	}
}

This is the generated code, with the following changes:

  • Updated the return type to Java.Lang.Object.
  • Changed virtual to override.
  • Casts the returned IDictionary<string, global::Android.Graphics.Drawables.Drawable> to Java.Lang.Object.
  • Casts the Java.Lang.Object back into IDictionary<string, global::Android.Graphics.Drawables.Drawable> for the delegate callback.

Note this is untested, you should ensure it actually works.

@fernandovm
Copy link
Author

Hi @jpobst, I already have applied the @moljac's suggestion and I'm trying to test, but now I have another issue. The MoPub's SDK is needed, together with google mediation mopub adapter, to try the AdMob mediation. However, when I build a binding project to MoPub's SDK (https://bintray.com/mopub/mopub-android-sdk/mopub-android-sdk/5.12.0) it does not import/generate any class.. :(

@jpobst
Copy link
Contributor

jpobst commented May 20, 2020

If you run Rebuild, what messages are you seeing in the log? There's generally pretty good warnings telling you exactly why it is skipping over each type/method it doesn't bind.

@fernandovm
Copy link
Author

fernandovm commented May 20, 2020

Hi @jpobst, yes, generally, but unfortunately seems is not be this case. In normal output msbuild verbosity is generated just one: warning CS0109: The member 'BuildConfig.class_ref' does not hide an accessible member. The new keyword is not required.

The diagnostico verbosity output log: build-output.txt

@jpobst
Copy link
Contributor

jpobst commented May 20, 2020

Are the types listed in:

  • obj\Debug\api.xml.class-parse
  • obj\Debug\api.xml
  • obj\Debug\api.xml.fixed

These are the output as the process moves through in order (parsing the jar, resolving the java types, applying metadata), so if we can find where the types disappear it should tell us what step is failing.

If they aren't in any of the files, you can use a Java decompiler to see if they exist in the .jar: obj\Debug\library_project_jars\classes.jar.

@fernandovm
Copy link
Author

fernandovm commented May 30, 2020

Hi guys, excuse me by delay in reply. So @jpobst, indeed, I don't know why (maybe it is just some kind of gradle wrapper), but the mopub android sdk .aar file downloaded from bintray does not has the need classes inside the classes.jar file.

Well, I built the mopub android sdk from github sources and create a binding library from the .jar generated file. Several adjustments were needed in metadata file to build with succesfull, but okay, it worked.

However, when I add a reference to this binding library in my android app project I have new compilation errors, such as:

obj\Off\90\android\src\mono\com\mopub\mobileads\VideoDownloader_VideoDownloaderListenerImplementor.java:8: error: VideoDownloaderListener is not public in VideoDownloader; cannot be accessed from outside package com.mopub.mobileads.VideoDownloader.VideoDownloaderListener

So, the main problem is that in generated file from obj\Debug\generated\src the interface already appears as public:

// Metadata.xml XPath class reference: path="/api/package[@name='com.mopub.mobileads']/class[@name='VideoDownloader']"
[global::Android.Runtime.Register ("com/mopub/mobileads/VideoDownloader", DoNotGenerateAcw=true)]
public partial class VideoDownloader : global::Java.Lang.Object {

	// Metadata.xml XPath interface reference: path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']"
	[Register ("com/mopub/mobileads/VideoDownloader$VideoDownloaderListener", "", "Com.Mopub.Mobileads.VideoDownloader/IVideoDownloaderListenerInvoker")]
	public partial interface IVideoDownloaderListener : IJavaObject, IJavaPeerable {

		// Metadata.xml XPath method reference: path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']/method[@name='onComplete' and count(parameter)=1 and parameter[1][@type='boolean']]"
		[Register ("onComplete", "(Z)V", "GetOnComplete_ZHandler:Com.Mopub.Mobileads.VideoDownloader/IVideoDownloaderListenerInvoker, Higgx.Library.AdSource.Mopub.Sdk.Android")]
		void OnComplete (bool p0);

	}
   
   //removed code..

}

Would appreciate if you point to me what's the problem that I can't see. :((

Thank you!

@jpobst
Copy link
Contributor

jpobst commented Jun 1, 2020

That error is coming from Java, not C#. So you'll need to look at the .jar that it is trying to compile against and see if the interface is public there.

Since you do this to change the C# version I suspect it is not public in Java:

<attr path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']" name="visibility">public</attr>

I'm not sure how you're intended to implement that interface, even if you were consuming this directly from Java, if it's not public.

@fernandovm
Copy link
Author

Hi @jpobst, this change in C# was just a test (one of them), but yes, it is not public in Java (see here), it is visible only inside its package.

The problem seems me is that in binding library it is generated as public, but, even I changing that using metatada (to force the internal visibility) I get erros when compiling my app that reference this binding library.

@jpobst
Copy link
Contributor

jpobst commented Jun 4, 2020

I'm afraid I don't understand what you are saying, it is explicitly being marked public in C# by this metadata:

<attr path="/api/package[@name='com.mopub.mobileads']/interface[@name='VideoDownloader.VideoDownloaderListener']" name="visibility">public</attr>

What error do you get when you remove that?

@fernandovm
Copy link
Author

Ok, sorry, let's go.. The interface VideoDownloaderListener is not public in the java code, but I get the error described above, even without to do any visibility change in the metadata.

Then, I have made several tests changing the visibility of this interface using metadata, just attempts to try understand how the generator would can interpret the case, but, I was not successful.

You can see this problem in the updated sample project Google.Ads.Mediation.Mopub.zip.

@jpobst
Copy link
Contributor

jpobst commented Jun 5, 2020

When I compile the sample project using VS2019 16.6.0 I get:

error: MraidBridge_MraidBridgeListenerImplementor is not abstract and does not override abstract method onSetOrientationProperties(boolean,MraidOrientation) in MraidBridgeListener
\mono\com\mopub\mraid\MraidBridge_MraidBridgeListenerImplementor.java

This is because onSetOrientationProperties in the interface MraidBridgeListener requires a parameter of type MraidOrientation, which is not a public type in Java.

However this does not seem to be the error you are getting. Is this an error you have gotten previously and have a workaround for?

@fernandovm
Copy link
Author

fernandovm commented Jun 5, 2020

When I compile it I also get this error, in really, I get seven errors:

image

I didn't mentioned before because I wanted solve the other six problems, they seems me the same thing.

About MraidBridgeListener, I haven't tested, but as it requires the type MraidOrientation which is not a public, I think that there isn't to do, unless remove it using metadata. This will compile, but after will see if works in runtime.

@jpobst
Copy link
Contributor

jpobst commented Jun 5, 2020

I think those 6 will go away if you update to 16.6+, due to this change: #572.

Granted, the fix is that the listeners are going to go away, but they are package-private in Java, and shouldn't be bound.

That only leaves the error I mentioned.

@fernandovm
Copy link
Author

fernandovm commented Jun 5, 2020

I think those 6 will go away if you update to 16.6+, due to this change: #572.

Granted, the fix is that the listeners are going to go away, but they are package-private in Java, and shouldn't be bound.

Indeed, great!

That only leaves the error I mentioned.

I will remove the MraidBridge class using metadata and to test at runtime.

<remove-node path="/api/package[@name='com.mopub.mraid']/class[@name='MraidBridge']" />

@jpobst
Copy link
Contributor

jpobst commented Jul 1, 2020

Hopefully this fixed your issue! If not, please comment with updated details and we can re-open.

@jpobst jpobst closed this as completed Jul 1, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

No branches or pull requests

4 participants