Bugzilla – Full Text Bug Listing
| Summary: | Cannot set event handler field with an array, must use ::= | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Will Smythe <smythew> | ||||||||||||||
| Component: | EDT | Assignee: | Huang Ji Yong <hjiyong> | ||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | hjiyong, mayunf, mheitz, svihovec, wxwu | ||||||||||||||
| Version: | unspecified | ||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows XP | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | 368575, 368577 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Hi Will,
This problem is not related to dynamically creation.
It is about event setting.
Change to onClick::= will resolve the problem.
link HyperLink{text = "Hey", href = "www.ibm.com", onClick ::= linkClicked};
This problem also exists in RBD. The event setting always require the concated operator ::=.
I think we can ignore this problem in 0.7.0 because it has a alternative method to bypass the problem.
How would a developer know this? It may be obvious to 'us' what the issue is, but someone new to EGL is going to stumble over stuff like this because 1) it should work, and 2) the error we report to the developer is completely useless. deferred - too complex to fix this late. People are living with it in RBD. Created attachment 208366 [details]
Fix for the widget event setting
This fix is in widget runtime (widget.js). The old setXXEvent function just throw an error so that user can not assign events. In this fix, the setXXevent functions are implemented. When the events have been installed, just assign the handlers to the new one; otherwise, install the events. The risks 1. This fix have a lot of impacts in widgets event setting. So it will need a high coverage of event testing. 2. There are some jsgen code fix before to deal with widget event specifically, I think they should be reverted. (bug 362155 and one more Jimmy did). Brian and other guys, please review this patch. Thomas, please take events into consideration when making testing plan. Would the following code work for this patch?
var0 EventHandler[] = [func1];
var1 Button{ onClick = var0 };
...
function doSomething()
var0.appendElement(func2);
end
When onClick is fired after doSomething is invoked, would func2 be run?
(In reply to comment #6) > Would the following code work for this patch? > > var0 EventHandler[] = [func1]; > var1 Button{ onClick = var0 }; > ... > function doSomething() > var0.appendElement(func2); > end > > When onClick is fired after doSomething is invoked, would func2 be run? Yes, both functions will run. If I am reading the attached patch correctly, setting onClick = var0 does an assignment of the elements in var0 to the internal onClick array, and does not replace the onClick array to the var0 array. When func2 is appended to the var0 array, what causes the function to be added to the onClick array? "setOnClick" : function(handlers) { if(this.onClick) this.onClick.assign(handlers); else this.installEventHandlers(this, 'click', this.onClick = handlers); },
When invoking onClick = var0
the else part will be executed, which pass the reference of var0 to onClick. So the onClick is the same array with var0.
Is it better to not pass reference to onClick instead clone it.
"setOnClick" : function(handlers) {
if(this.onClick == null){
this.installEventHandlers(this, 'click', this.onClick = []);
}
this.onClick.assign(handlers);
},
After change to this,
var0.appendElement(func2); // won't work
should be
Button.onClick.appendElement(func2);
(In reply to comment #8)
> If I am reading the attached patch correctly, setting onClick = var0 does an
> assignment of the elements in var0 to the internal onClick array, and does not
> replace the onClick array to the var0 array. When func2 is appended to the
> var0 array, what causes the function to be added to the onClick array?
I talked with Tim about this, and we came up with the following information:
The 'onClick' field of a widget is an array of EventHandler delegates. This field also contains the @Property annotation, which extends the default getter and setter for this field, but the field should still behave like a regular array when the getter and setter is invoked. For example, in the Box widget we have the following:
myButton Button{};
myChildren Widget[] = [myButton];
var1 Box{ children = myChildren};
function func1()
myOtherButton Button{};
var1.children.appendElement(myOtherButton);
Syslib.writetdout(myChildren.getSize()); // writes "2"
end
In the example above, the myChildren array changes when the var1.children array changes, which is the expected behavior for arrays in the EGL language. When myOtherButton is appended to var1.children, the browser document does not change, which is expected, because the setter method of the 'children' field in a Box extends the standard assignment to include re-rendering the box widget in the document.
Currently, the event handler fields in a Widget are defined with both a Getter and a Setter, but the Setter throws a runtime exception indicating that it is not supported. Our current options for fixing this issue involve:
1) Fixing the Setter as described by comment 9, where a single array is managed for the event handler, and assigning a new array copies values into the managed array.
2) Fixing the Setter by always assigning a new array over the old array, which matches the expected behavior of the EGL language.
3) Removing the Setter function from the @Property annotation so that "onCLick = [ a ]" is marked as invalid by the compiler.
I believe we should not implement option 1, because it violates the standard behavior of array assignment in the EGL language. For example:
var1 EventHandler[] = [func1];
myButton Button{onClick ::= var1};
function func1(event Event in)
end
function func2(event Event in)
end
function func3()
myButton.onClick.appendElement(func2);
Syslib.writetdout(var1.getSize()); // writes "1"
end
I tried to implement option 2, but it does not work because the original event handler array is 'pinned' in our event handling closure, and cannot be overridden once it is set.
For these reasons, I believe we should implement option 3, which requires the following defects are resolved as well, to provide the appropriate validation errors at compile time:
Bug 368575
Bug 368577
Thanks Brian, I will defer this defect because of the blocking issues. To remind myself, the remaining work including: 1. Remove the setXX from @property in widget definition. 2. There are several places in js gen to deal with the set/get event of widgets, need to reconsider them. Can you provide more details about #2 in comment 11 when you get a chance? (In reply to comment #12) > Can you provide more details about #2 in comment 11 when you get a chance? I misunderstood the final statement of comment 10. I believe we won't do anything except the validations. So close this defect as won't fixed. Ji Yong, was the 'setMethod' removed (or just the 'getmethod' specified) for the event handler arrays in the Widget External Type(s)? If not, we still won't see an error message when these other defects are resolved. Created attachment 210837 [details]
Patch to remove the setMethod of event properties
Patch to remove the setMethod of event properties
Created attachment 210852 [details]
Patch to remove the setMethod of event properties
Commit the patch and the rebuilt eglar
I do not think that the event handler fields of Document should have been changed from having only having a 'set' method to only having a 'get' method. How will users be able to set event handlers on the document? Also, since these event handlers are not arrays, it is ok for them to have a traditional 'set' operation. Out of curiosity, why does DojoCheckBox have it's own onWidgetLoad event, when it should extend from DojoBase, which has the same event handler? Do the Dojo event handling arrays work like our EGL widget handling arrays, where a Set has not been supported in the past? If a Set has been supported, and it worked, I wonder if changing this API is going to break someone. (In reply to comment #17) > I do not think that the event handler fields of Document should have been > changed from having only having a 'set' method to only having a 'get' method. > How will users be able to set event handlers on the document? Also, since > these event handlers are not arrays, it is ok for them to have a traditional > 'set' operation. > > Out of curiosity, why does DojoCheckBox have it's own onWidgetLoad event, when > it should extend from DojoBase, which has the same event handler? > > Do the Dojo event handling arrays work like our EGL widget handling arrays, > where a Set has not been supported in the past? If a Set has been supported, > and it worked, I wonder if changing this API is going to break someone. OK. I revert the change to Document section. DojoCheckBox is actually a RUIWidget containing two child widget label and a dojo widget(DojoCheckBoxWithoutLabel). So it is not a real Dojo widget which won't inherit the API from DojoBase. Dojo event handling works the same as EGL widget. Created attachment 210897 [details]
Revert the change in Document section
Revert the changes made to Document event setting section in ExternalTypes.egl So, two issues here: 1) It's still not clear to me what should work and what shouldn't. 2) The first thing I tried ( ... onClick = [ a ] ) failed with a massive error being displayed (see attached screen shot 'Error when setting array on event handler Feb 2012.png') Can someone net out what should work and what should not? And, in the cases that shouldn't work, we need a much simpler, easier to understand error message. Created attachment 211336 [details]
Error message shown when setting a new array on the event handler field
Content assist should also be in sync with whatever is supported. For example, I pressed CTRL+SPACE after typing this:
f DojoTextField { onKey
I selected "onKeyDown" and content assist added the following:
f DojoTextField { onKeyDown =
I pressed CTRL+SPACE again and the first proposal was [], which I believe is invalid.
The set method should be removed from the Property annotation for the Event handling fields, which would result in an EGL compile error for the code that you provided:
link HyperLink{text = "Hey", href = "www.ibm.com", onClick =[linkClicked ]};
This code should not get into the runtime.
I re-read the comments, and in comment 10 it states:
For these reasons, I believe we should implement option 3, which requires the
following defects are resolved as well, to provide the appropriate validation
errors at compile time:
Bug 368575
Bug 368577
I have marked this bug as depending on those other two, and this bug should not be closed until those have been resolved.
Will, go ahead and open a UI defect for comment 23. That is a general defect for content assist and fields with the EGLProperty and Property annotation where a set method is not specified. Verified in 0.8.0.v201202272101-1Co-FjuJ6QMNSOJRWsc_GC8C Open another defect for comment 21 The IDE bug is Bug 372695 - Content assist try to invoke setter function which is not defined in @property |
Created attachment 206899 [details] Handler demonstrating the error Running the attached handler results in the following error: Could not render UI unknown error [CRRUI2094E] Here are the EGL function calls leading to this error: org/eclipse/edt/rui/widgets/HyperLink.egl() at line 48 client/HyperlinkErrorExample.egl() at line 19 HyperlinkErrorExample.<init>() [native JavaScript] undefined undefined:0 [CRRUI2095E] Could not find the EGL function calls leading to this error The code is really simple - it's simply creating a Hyperlink widget on-the-fly on start and setting the text, href, and onClick. Based on the line number in the exception, the code is failing when href is being set. Opening this MAJOR because this really simple code that should work.