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

Fix bug in JavaAdapter.java of cannot override method in multi-layer interface #857

Merged
merged 9 commits into from
Apr 15, 2021

Conversation

mulingLHY
Copy link
Contributor

@mulingLHY mulingLHY commented Apr 6, 2021

Supposing that the program has a java abstract class as follows

interface C{
  public int methodInterfaceC(String str)
//...
}

interface B extends C{
//...
}

class A implements B{
//...
}

This PR fixes a bug in JavaAdapter that the method public int methodInterfaceC(String str) cannot be overrided correctly in javascript byJavaAdapter(Packages.A,{methodInterfaceC:function(){}},null)

@tuchida
Copy link
Contributor

tuchida commented Apr 6, 2021

Could you write a unit test that shows what you are able to do?

This seems to be the test for JavaAdapter.
https://github.com/mozilla/rhino/blob/db5ce1665d06a3afb9cac9d556811fb6fc1fdc05/testsrc/doctests/javaadapter.doctest
Or you may want to add the file here.
https://github.com/mozilla/rhino/tree/db5ce1665d06a3afb9cac9d556811fb6fc1fdc05/testsrc/org/mozilla/javascript/tests

@mulingLHY
Copy link
Contributor Author

Could you write a unit test that shows what you are able to do?

This seems to be the test for JavaAdapter.
https://github.com/mozilla/rhino/blob/db5ce1665d06a3afb9cac9d556811fb6fc1fdc05/testsrc/doctests/javaadapter.doctest
Or you may want to add the file here.
https://github.com/mozilla/rhino/tree/db5ce1665d06a3afb9cac9d556811fb6fc1fdc05/testsrc/org/mozilla/javascript/tests

Supposing that the program has a java abstract class as follows and I want to extend class A and override the method methodInInterfaceC in interface C, the current method getOverridableMethods cannot obtain such method. Considering that the methods in the interface are all public, I It can be solved by changing getDeclaredMethods to getMethods for interface.

interface C{
  public void methodInterfaceC()
//...
}

interface B extends C{
//...
}

Class A implements B{
//...
}

I found this probem when I was about to extend android.widget.BaseAdapter and override methods getCount,getView... which is declared in interfce android .widget.Adapter
I am an amateur from China and my English is not very good. If the expression is inaccurate or inappropriate, please forgive me.

@tuchida
Copy link
Contributor

tuchida commented Apr 7, 2021

Thanks.
Perhaps you may want to create JavaAdapterTest.java. This will be helpful for you.
https://github.com/mozilla/rhino/blob/8442718d94434e4e1c054b6228d0d4b1603b2878/testsrc/org/mozilla/javascript/tests/InterfaceAdapterTest.java

Can you write test?

I am an amateur from China and my English is not very good. If the expression is inaccurate or inappropriate, please forgive me.

I don't understand English either, so I use a translation tool.

mulingLHY added a commit to mulingLHY/rhino that referenced this pull request Apr 7, 2021
@mulingLHY
Copy link
Contributor Author

Can you write test?

I still can’t use github proficiently, so I don’t know whether you mean this.
https://github.com/mulingLHY/rhino/blob/master/testsrc/org/mozilla/javascript/tests/TestJavaAdapter.java

@tuchida
Copy link
Contributor

tuchida commented Apr 7, 2021

It looks good.
I think you commit to the wrong branch. Try to commit to patch-1 instead of master.

@tuchida
Copy link
Contributor

tuchida commented Apr 7, 2021

Good! Please wait a moment for the owner to confirm.

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I have a few specific comments.

Also, could you update the PR description with a bit longer explanation of what changed, combined with a small example like the one in your test? That will help a lot in writing the release notes.

src/org/mozilla/javascript/JavaAdapter.java Outdated Show resolved Hide resolved
testsrc/org/mozilla/javascript/tests/TestJavaAdapter.java Outdated Show resolved Hide resolved
modify the code style to match the original project
@mulingLHY mulingLHY changed the title Update JavaAdapter.java Fix bug in JavaAdapter.java of cannot override method in multi-layer interface Apr 9, 2021
change assert to throws
rename PullRequest857Test.java to JavaAdapterTest.java
for pull reqeuest mozilla#857
@gbrail
Copy link
Collaborator

gbrail commented Apr 15, 2021

OK, you have answered lots of our questions. Thanks!

@gbrail gbrail merged commit f1fe03b into mozilla:master Apr 15, 2021
@p-bakker p-bakker added enhancement Java Interop Issues related to the interaction between Java and JavaScript labels Oct 13, 2021
@p-bakker p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Java Interop Issues related to the interaction between Java and JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants