Bug 360615

Comment 1 Huang Ji Yong 2011-10-12 03:27:03 EDT

This is a JSGen Bug.
The EGL code in line 1030 of AddClass function in DataGrid.egl is not correctly generated.
EGL code:
widget.class = widget.class + " " + class;
JS Code:
widget.setClass(((((egl.checkNull(widget).ezekw$$class) + " ")) + ezekw$$class));

The second widget.class should be generated as widget.getClass();

Comment 2 Huang Ji Yong 2011-10-13 02:51:00 EDT

Created attachment 205087 [details]
Temporary fix

Comment 3 Huang Ji Yong 2011-10-13 02:51:34 EDT

A simpler test case
handler testGetField type RUIhandler{onConstructionFunction = start}
	w Widget{class="abc"};
		
    function start()
   		w.class = w.class + "a";
    end
end

Comment 4 Huang Ji Yong 2011-10-13 03:00:18 EDT

Hi Scott,
I attach a fix for this problem, please review.

I change the NamedElementTemplate.genAccessor function
if (((ctx.getAttribute(element, Constants.EXPR_LHS) == null) || (ctx.getAttribute(element, Constants.EXPR_LHS) == Boolean.FALSE)) // return false in this test case
			&& (propertyFunction != null) && !CommonUtilities.isCurrentFunction(ctx, propertyFunction)) {


The ctx.getAttribute(element, Constants.EXPR_LHS) is false in this test case. Because when running AssignmentTemplate (line 32), it is put into the context. And the assignment lhs happens to be the same as the LHS of the "+" expression..

I find you the 1st line of this method is
String propertyFunction = CommonUtilities.getPropertyFunction( element, false, ctx );
The second attribute assumes this namedElement is not a setter function. So I remove the if condition to judge if it is namedElement. Is this assumption OK?
With this testcase and my other handlers in my workspace, this fix does not break anything now.

Comment 5 Scott Greer 2011-10-18 20:45:37 EDT

Ji Yong and I reviewed his fix together and concluded that it's incorrect;  however, I think he's isolated the problem -- we just need to identify the fix.

Comment 6 Huang Ji Yong 2011-10-19 03:57:07 EDT

Another simpler test case
a Widget{};
b Widget{};
function start()
  a.x = b.x;
end

The generated js for a.x = b.x is wrong as
this.a.setX(egl.checkNull(this.b).x);

Should be
this.a.setX(egl.checkNull(this.b).getX());

Comment 7 Huang Ji Yong 2011-10-20 03:51:04 EDT

(In reply to comment #5)
> Ji Yong and I reviewed his fix together and concluded that it's incorrect; 
> however, I think he's isolated the problem -- we just need to identify the fix.

Scott,
The previous fix can resolve this problem. Can you check if it has regression or any side effects.
Thanks.

Comment 8 Scott Greer 2011-10-20 18:58:18 EDT

Ji Yong,

Do you mean the fix in comment #2?  I thought we reviewed that and concluded that it would break other things, so as a result you were no longer confident in the change?  Or are you talking about a different fix?

Comment 9 Huang Ji Yong 2011-10-20 22:52:58 EDT

Created attachment 205678 [details]
Updated patch

Comment 10 Huang Ji Yong 2011-10-20 22:53:33 EDT

(In reply to comment #8)
> Ji Yong,
> 
> Do you mean the fix in comment #2?  I thought we reviewed that and concluded
> that it would break other things, so as a result you were no longer confident
> in the change?  Or are you talking about a different fix?

Sorry, I forgot to attach the patch.
Here it is.

Comment 11 Huang Ji Yong 2011-10-21 04:06:15 EDT

Have committed the code because many defects are caused by this one.
Still need Scott's review.

Comment 12 Huang Ji Yong 2011-10-25 01:11:38 EDT

Scott,
Please mark this as resolved if you don't see any problem.
Thanks.

Comment 13 Scott Greer 2011-10-25 09:44:40 EDT

Ji Yong,

Reviewed and marking resolved.   Thanks!

Comment 14 Yu Hao 2011-11-02 01:45:47 EDT

Verified in 201111012101