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

NativeJavaClass: Add ".class" to access the associated Java Class #1679

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camnwalter
Copy link
Contributor

@camnwalter camnwalter commented Oct 4, 2024

First time making tests so let me know if there is some case that isn't covered or whatever else I should change.

This PR adds support for class literal syntax for NativeJavaClasses. Before this, an example would be trying to get the Class associated with java.lang.Object, where the user would have to do

java.lang.Object.__javaObject__

or

var objectClass = org.mozilla.javascript.Context.jsToJava(java.lang.Object, java.lang.Class)

This makes it so that

java.lang.Object.class

works as well, just like it looks like in Java. The work around I had to add was this, due to how Rhino automatically creates bean properties associated with the getters and setters of the same name. If there is a class that has a static setClass() method, a class field is created, which would conflict with the goal of this PR. Instead, we just ignore that case if it exists, since there can't be a static class getter due to getClass() being defined as an instance method inside Object, so there is no static class getter associated to the bean property.

Closes #757

@camnwalter camnwalter marked this pull request as draft October 5, 2024 06:48
@p-bakker
Copy link
Collaborator

p-bakker commented Oct 5, 2024

Looks like at least one of the testcases you added fails

@camnwalter
Copy link
Contributor Author

I think that should be fixed now. I guess it was running it multiple times and so the static count variable was higher than expected

@camnwalter camnwalter marked this pull request as ready for review October 5, 2024 15:05
@gbrail
Copy link
Collaborator

gbrail commented Oct 5, 2024

The code looks OK and I see the issue, but will people understand what we changed here? I think that at the very least the PR or commit should describe what we did and what people can do now -- since I'm not sure where the LiveConnect "spec" or docs are, I'm not sure how people will understand what is or isn't possible unless we update our docs.

@camnwalter camnwalter changed the title NativeJavaClass: Add class literal support NativeJavaClass: Add ".class" access to access the associated Java Class Oct 5, 2024
@camnwalter camnwalter changed the title NativeJavaClass: Add ".class" access to access the associated Java Class NativeJavaClass: Add ".class" to access the associated Java Class Oct 5, 2024
@camnwalter
Copy link
Contributor Author

camnwalter commented Oct 5, 2024

Renamed the PR and added a more detailed description on what was changed. Not sure where else to put this (other than the commit description).

@gbrail
Copy link
Collaborator

gbrail commented Oct 7, 2024

IMO, this is basically adding a new feature to LiveConnect, which looks useful to me, but I could use some more opinions.

What do regular users of the Java interop think about this?

@p-bakker
Copy link
Collaborator

p-bakker commented Oct 7, 2024

Did you see the discussion on #757?

This makes sense to me, to simplify access to the class

@rPraml
Copy link
Contributor

rPraml commented Oct 7, 2024

Hello, we use LC a lot. But until now I can't remember that we were missing this feature. If a Java method expected a class as parameter, you could just call myJavaFunc(java.lang.String)

Nevertheless, I can imagine use cases where it would be necessary.
But I also think we should just rename this __javaObject__ to class because

  • it is relatively undocumented
  • and therefore unintuitive.
  • There is no test.
  • it is probably hardly used (Google search does not really find something, expect these GH issues or rhino source code.)

So, I would have nothing against simply replacing __javaObject__ with the more intuitive class. But perhaps others can give their opinion on whether we can afford this breaking change or if we should impmement the same functionality twice?

BTW: It was added 17 years ago with this commit d623313 and then changed from __javaClass__ to __javaObject__ c782d03

@camnwalter
Copy link
Contributor Author

Nevertheless, I can imagine use cases where it would be necessary.

Yeah the main reason I see it being useful is for reflection.

@gbrail
Copy link
Collaborator

gbrail commented Oct 10, 2024

This seems useful but given that so much of this stuff isn't documented we'll I'm kind of waiting to see if we have more opinions.

I am trying to resurrect some of the old docs, once I do that perhaps you can all take a look and see if that's a good place to update.

@p-bakker
Copy link
Collaborator

@gbrail which old docs are you referring to? Thought we did resurrect all (relevant) docs from the Mozilla website into rhino.github.io already

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.

please support java class access, eg: java.lang.Object.class
4 participants