282595 - Patch for BeanProperties to work with several setters for one BeanProperty.

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Juerg lehni]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; de-de) AppleWebKit/125.5.6 (KHTML, like Gecko) Safari/125.12
Build Identifier: 

I've modified the way BeanProperties are handled by org.mozilla.javascript.JavaMembers:
Instead of only one setter, the class can define several setters with different parameters. All the defined 
setters for one property are uninfied in one NativeJavaMethod. In addition to that, the setter with the 
same parameter type as the return type of the getter is selected as the main setter. The main setter is 
responsible for setting a BeanProperty to the value null. Otherwise, the call to the NativeJavaMethod 
that unifies several setters would be ambigous (e.g. a setter with parameter of class String and one of 
class Object).


Reproducible: Always

Steps to Reproduce:

Comment 1

[Juerg lehni]

Attachment: BeanProperties patch

Comment 2

[Igor Bukanov]

Patch comments:

1. Cosmetics: please remove all tabs and try to fit lines to 80 chars limit.

2. I would suggest to keep the old implementation if there is only one setter.
The idea of using NativeJavaObject for multiple setters is a nice one, but for
the common case of single setter it would create 2 additional objects per setter
and that is not good.

Comment 3

[Juerg lehni]

1. I'm sorry for the tabs and the 80 chars limit, that's my default setting here and I didn't notice. I'll 
adapt to the Rhino style.

2. How do you mean it would generate two objects for one setter? If there's only one setter, mainSetter 
and setter actually point to the same object, so only one NativeJavaMethod is used. And I would prefer 
allways use this approach, as otherwise one cannot guarantee that the behavior for how parameters are 
converted will allways be exactly the same (or at least I'm not sure about this, I'm not deeply enough 
into Rhino...). But one thing I wonder: The NativeJavaMethod that wrapps all setters in one should 
already exist somewhere anyway. So wouldn't it make more sense to reuse that one instead of creating 
a new one?

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> 2. How do you mean it would generate two objects for one setter? If there's
only one setter, mainSetter 
> and setter actually point to the same object, so only one NativeJavaMethod is
used. 

In the case of the single setter BeanProperty stored only MemberBox. The patch
changes that to NativeJavaMethod containing the one-element array of MemberBox
with the original MemberBox. It is 2 additional objects.

> And I would prefer 
> allways use this approach, as otherwise one cannot guarantee that the behavior
for how parameters are 
> converted will allways be exactly the same (or at least I'm not sure about
this, I'm not deeply enough 
> into Rhino...). But one thing I wonder: The NativeJavaMethod that wrapps all
setters in one should 
> already exist somewhere anyway. So wouldn't it make more sense to reuse that
one instead of creating 
> a new one?

This is a very good idea :). And then there is no need to create the main setter
if NativeJavaMethod.call would be split into part serach part and a static
method to apply Java call to MemberBox. Then the static method can be applied to
the main setter directly.

Comment 5

[Juerg lehni]

Funny I didn't notice this before. I changed my code, it became much simpler and closer to what was 
there before. The formating should be alright this time too.

Comment 6

[Juerg lehni]

Attachment: Updated Patch

The new patch that includes the discussed changes.

Comment 7

[Igor Bukanov]

Marking as fixed: the last changes are in Rhino CVS now.