Android: Fix `JavaClassWrapper` so it actually works #96182

dsnopek · 2024-08-27T19:30:30Z

Currently, JavaClassWrapper has most of the code in order to work, but is missing a bunch of the pieces to connect it all together, and, in fact, doesn't actually do anything. :-)

This PR aims to get it minimally working, so that you can create Java objects and call their methods.

It also allows JavaObjects to be passed as arguments to methods on JNISingletons registered by Godot Android Plugins, and for any unknown objects that are returned from such methods to be turned into JavaObjects.

Here's some example GDScript code:

var LocalDateTime = JavaClassWrapper.wrap("java.time.LocalDateTime")
var DateTimeFormatter = JavaClassWrapper.wrap("java.time.format.DateTimeFormatter")

var datetime = LocalDateTime.now()
var formatter = DateTimeFormatter.ofPattern("dd-MM-yyyy HH:mm:ss")

print(datetime.format(formatter))

There's still a bunch of things that don't work, namely accessing fields and constants. Also, this code could use some refactoring and clean-up. There's also some amount of code duplication between JavaClassWrapper and JNISingleton.

But this PR doesn't address those things - just getting it minimally working. Further work can be done in follow-up PRs.

Mickeon · 2024-08-27T20:23:35Z

So it's true. I have been ASKING everyone. I have had people reach out to me, asking what these classes actually do because I used to be the only one brave enough to figure it out and you're telling me they did NOT work for who knows how long, potentially since their conception.

dsnopek · 2024-08-27T20:50:15Z

you're telling me they did NOT work for who knows how long, potentially since their conception.

From the look of the code, I get the impression that these classes were partially ported from somewhere else where they were originally working, but the port was never fully finished. And, yeah, looking at the git blame, it's possible they haven't really worked for as long as Godot has been Open Source.

That said, the changes to get them working weren't really all that big :-)

devloglogan

Have not been able to test this yet, but the code looks good to me!

doc/classes/JavaClass.xml

doc/classes/JavaObject.xml

m4gr3d · 2024-09-01T19:01:08Z

platform/android/jni_utils.cpp

@@ -395,10 +409,10 @@ Variant::Type get_jni_type(const String &p_type) {
 		idx++;
 	}

-	return Variant::NIL;
+	return Variant::OBJECT;


So this assumes we have an object if we don't match any of the other types, correct?
Should it also checks if the type starts with L?

So this assumes we have an object if we don't match any of the other types, correct?

Yes

Should it also checks if the type starts with L?

This particular function gets class names (for non-arrays) as plain strings without the L prefix and ; suffix. So, for example, I made a JNISingleton in a Godot Android plugin that takes a java.time.format.DateTimeFormatter as an argument, and this function gets exactly the string "java.time.format.DateTimeFormatter".

akien-mga · 2024-09-03T09:55:08Z

Thanks!

Mickeon · 2024-09-03T11:07:12Z

Just as a note, it would be nice to document in prior versions that these classes literally do not work.

dsnopek · 2024-09-03T21:24:50Z

Just as a note, it would be nice to document in prior versions that these classes literally do not work.

What form should that take? It could be a "note" in the description of the classes, although, it seems like that could potentially be easy to miss amid the description of what the classes and functions should be doing. Or should we remove the existing descriptions and only have the note? Only Godot 4.3 has full documentation of those classes - in Godot 4.2 and below they are undocumented - so returning to a largely undocumented state for 4.3 could make sense?

akien-mga · 2024-09-03T21:30:27Z

Well if they really don't work in 4.3, this is a bugfix and should likely be cherry-picked, instead of documenting that they're useless.

Mickeon · 2024-09-03T22:35:05Z

They haven't worked since their conception. So "this" should be cherrypicked to 3.x as well, which I'm not sure is easier than noting it down.
Also this "bugfix" adds new methods to the API to actually have it work. I thought that for minor releases doing so is not exactly nice practice.

dsnopek · 2024-09-03T22:57:23Z

Well if they really don't work in 4.3, this is a bugfix and should likely be cherry-picked, instead of documenting that they're useless.

My first inclination would be to not cherry-pick this. Even though the classes existed, the fact that they didn't really work means this is basically totally new functionality. I feel like it'd be best to go through the usual dev -> beta -> rc process to get testing and shake out bugs.