Add support for Label "for" attribute (like in HTML) (BZ#587)

[qx-bug-importer]

Stefan Kloiber (stefan.kloiber) wrote:

Unlike the HTML element label, the qx.ui.basic.Label has no for attribute.

assigned to Martin Wittemann (@wittemann)

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Marked as enhancement.

[qx-bug-importer]

Fabian Jakobs (@fjakobs) wrote:

to sw

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Good idea. Basically a focus redirection.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

to none

[qx-bug-importer]

Fabian Jakobs (@fjakobs) wrote:

to martin

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Assigned!

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Added the feature in revision 18872. The property is called buddy (like in Qt) and can be found on every label. http://doc.trolltech.com/4.5/qlabel.html

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Wow, never knew that this is named "buddy" in other frameworks. Neat.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

You missed the dispose section for: "buddyEnabledBinding" and the name of "clickListenerId" is to generic in my opinion. Interesting that you use data binding. The events and bindings should also be cleared in dispose.

BTW: You don't need the closure at all there. Why not just attach "value.focus" with the context "value" to the listener?

Just wonder why you think that HTML's "for" is the same as "buddy" in Qt. When I read their documentation they only mention keyboard support for the underlined character (mnemonics - currently not supported in qooxdoo). Nothing regarding mouse click support. Do I overlook something or may there be another property available for this? Can you point me to your source?

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Thanks for reminding me. I really missed the dispose section. But its done now (revision 18877). I have left the name __clickListenerId like it is because its a private member in a label and therefore does have no impact on other classes.

Sure i dont need the closure here. But the code is much more readable with it.

We have tested the buddy property in the Designer of Qt. The buddy property in Qt supports the click (not on OSX) for sure. But I don't know if it supports the enabled propagation.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

The closure is against coding guidelines of qooxdoo. We don't use closures in this way :)

At least you - if you really think it's more readable - put the function outside.

But it's definitely simpler to just attach the target function directly. And it would save you the event ID storage.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Cool, that you tested it out in the designer. At least this behavior is not covered in their API documentation.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

BTW: The binding should be cleared as well. It makes no sense to keep the binding active in an disposed object.

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

I have removed the binding in the dispose.

But before i change the closure stuff, could you show me the coding guidelines? I haven't found them in the wiki and even if, I dont agree with them. So perhaps that is a topic we should change in the guidelines if there are some in written from.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Closures in general are expensive. Not sure where something like this is mentioned in the documentation. Maybe it's more like a public consensus. Just look out for code like the one you have written there. The framework basically don't use closures for these things. Keep in mind that the function is (re-)created for every call of the function. What a bloat - especially in bad garbage collectors like IE6.

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

To Andreas.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

Closures are ok even in IE6 if you just take care not to create circular references. See http://www.jibbering.com/faq/faq_notes/closures.html#clMem and http://www.codeproject.com/KB/scripting/leakpatterns.aspx.

Crockford uses closures to implement private and protected class members: http://www.crockford.com/javascript/private.html

Here is an article that warns about pitfalls of closures, but is generally positive about them when they are used considerately (avoiding unnecessary closure creation, immediate consumption of the closure, no cyclic references, etc.): https://developer.mozilla.org/en/Core_JavaScript_1.5_Guide/Working_with_Closures#Performance_considerations

A newer blog entry that compared performance of closures in certain context (but mind comment about IE): http://joose-js.blogspot.com/2009/03/closures-vs-properties-argumentscallee.html

I also found this article that compares binding and closures (not sure if it might be helpful here): http://www.alistapart.com/articles/getoutbindingsituations

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Thomas, not really anything new. Would be interesting what your occasion was to answer in such a longly comment which is mainly unrelated to the bug topic. Are you our new JavaScript performance guru? ;)

To keep this producitve: Hopefully nobody want to disagree with:

1. Variable lookup in closures is slower (so the execution of their content, was reported some time ago by the webkit developers)
2. Re-creation of functions/objects is expensive (triggers garbage collectore more often - and this is an issue in IE6)
3. That Crockford uses closures says nothing. It's more interesting that his module pattern (which you mean, by the way) is slower than prototypal inheritance. For the same reason like point #2. For John Resig this insight was one reason why jQuery now uses less closures and function calls in critical areas and that's why jQuery 1.3 is so much faster than 1.2. Another example: Atlas moved two years ago from the module pattern to prototypal inheritance. In fact all large frameworks I know off do not depend on the module pattern: qooxdoo, Bindows, Backbase, ExtJS, ...
4. Prototype-based members are shared between instances and method calls. This is good because it means less memory fluctuation, memory usage and performance.

Thomas, we can keep this going but until you throw anything new into the discussion, I would say this is useless. Especially regarding that this bug is not regarding pro/cons of closures. It was just about that this is not a common pattern in qooxdoo - and IMO for good reason. I still wonder why at all this is in question now. So, if we need further discussion on this, the mailing list might be the better channel.

BTW: In the case mentioned here, the closure is completely useless. So when we are following the Mozilla article the closure should be removed from the function.

[qx-bug-importer]

Thomas Herchenroeder (@thron7) wrote:

> Thomas, not really anything new. Would be interesting what your occasion was to > answer in such a longly comment

You don't have a monopole on this, have you?! ;)

> which is mainly unrelated to the bug topic. Are

But related to the current discussion in the bug.

> you our new JavaScript performance guru? ;)

Certainly not. What I wanted to bring to the discussion was factual information and evidence, in contrast to piling up claims nobody can verify. I think such a discussion style is more productive.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Thomas, do you really think that it's more productive when every bug re-discusses best-practises? These are there to improve productivity. It is OK to question these from time to time, but it makes no sense to do it bug-related IMHO.

[qx-bug-importer]

Sebastian Werner (@swernerx) wrote:

Sure, some kind of own documentation in the wiki would be helpful. Maybe even for more things regarded as best-practise in the qooxdoo world. Some of these are already covered by this article:

http://qooxdoo.org/documentation/general/javascript_best_practises

but closures are really still missing.

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

This must be some sort of a bad joke. :-( Or has this become a place to publicly disallow people from providing technical input to a discussion? Looks like it was completely misunderstood (?) that the list to articles about closures in comment #17 was just another neutral input, and not suggesting the use of closures within this specific bug. Even if it were, what should be wrong with taking into account developers' different point-of-views within a technical discussion? Every developer expects to be able to provide his/her individual input and insight, so this holds for all other devs as well.

That being said I encourage everyone to return to a technical level again and to add real value to this topic (which obviously is important). Thanks for pointing out here there seems to be a lack of information about closures (also the aspects that affect qooxdoo as a framework), so let us nail things down on a wiki page. Thomas' links as well as Sebastian's arguments are an excellent starting point, so I added them to http://qooxdoo.org/documentation/general/javascript_best_practises#using_or_not_using_closures Please feel free to add more info that you think is relevant.

Regarding this bug-specific implementation detail that sparkled such a fierce general discussion over the weekend, I already added a suggestion on Friday evening to http://qooxdoo.org/documentation/general/coding_style that I wanted to discuss with Martin today anyway. While each of the different implementation possibilities may have their pros/cons, when looking into the existing code base I realize quite a consistent use of externalized protected event handlers in such scenarios, right? So (Martin) I suggest to continue with that implementation style here as well, unless the more general discussion about closures may lead to a clearly superior implementation that we'd have to address separately, consistently and in large.

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

If i had known that this topic is so religious I would have just removed the closure and thats it! Have done it now (revision 18908) and I hope now is everybody pleased with it. I guess it does not make any difference because its not really a performance critical piece of code anyway. So I think it really not worth the effort and the whole discussion!

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

mass renaming of 0.9 target to 1.0-beta1

[qx-bug-importer]

Andreas Ecker (@ecker) wrote:

corrected wrong milestone (was 0.9, but was fixed for 0.8.3 already)

[qx-bug-importer]

Martin Wittemann (@wittemann) wrote:

Closed all bugs already shipped with a release.