Responses in-line
On 10/22/19, 1:52 AM, "Piotr Zarzycki" <[email protected]> wrote:
Comments inline.
pon., 21 paź 2019 o 18:25 Alex Harui <[email protected]> napisał(a):
> Then you will need to explain why the event handling code that uses
> royale_wrapper is not going to be invoked. Maybe there is no way to
> actually click on the icon (as opposed to the label next to the icon)?
>
>
I just checked CheckBox and nothing has changed. I can click on every
element in CheckBox and it's being selected.
Did you step through it in the debugger to make sure the target when clicking
on the icon element was the icon?
> If you look through the code, I believe we set royale_wrapper on just
> about every child element in every other component. Creating an exception
> must be justified. The actual solution (some sort of stack overflow)
might
> be better prevented by detecting the 'loop' of calls. We do that in
> several places already, like the layouts.
>
>
Yes we are, but why in case o checkbox royale_wrapper is being set to
children component (Icon in this case), as "this" ?
royale_wrapper is used on all kinds of child elements in other components to
point to the top-level component (TLC). There is code that tries to determine
that the "CheckBox" component is the dispatcher of some event even though the
browser is going to say it was some HTMLElement. It is effectively the
back-reference from HTMLElements to the TLC. In Flash, you could use
mouseChildren and mouseTransparent to affect the event.target. In Royale, we
have APIs like Event.isSameTarget(). I'm having trouble believing that
isSameTarget is going to work if you pass in the icon.
I think I've said this before, but IMO, there is a difference between
application coding and framework coding. When developing an application, it is
often ok for something to "just work" because that code only has to work in
that one application. In framework development, it can't just work for the one
application you happen to be working on, it has to work for everyone else's
application as well, so decision have to be based on fundamental logic and
there has to be much less (although probably not zero) code that "just works".
When I saw your change, my first thought was: "(almost) all child elements
should have a royale_wrapper set to the TLC, so why is this an exception?"
Somehow, we need framework developers to learn the common patterns in the
framework or go searching for them because having an exception to the rules is
usually a sign that the code isn't right, even if it appears to work for some
scenario. It will be a lot easier to teach other folks to be framework
developers and for them to learn how to debug the framework if there is a
consistent set of rules like "all child elements have royale_wrapper set".
I also looked at RadioButton and saw that it had its icon.royale_wrapper set to
the TLC. Is it working correctly? I grep'd the code for royale_wrapper and
saw how it was being used in lots of places and who was reading it. And that
got me thinking it was a definite pattern and your change was an exception and
while it may look like it "just works" it may not later if there is some event
dispatched off the icon and no royale_wrapper to use to determine the TLC.
Maybe if someone calls isSameTarget on the event it will fail.
My 2 cents,
-Alex
