You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@royale.apache.org by Greg Dove <gr...@gmail.com> on 2020/05/28 12:00:04 UTC
Change in MXRoyale
Alex,
I hope you are ok with this change:
https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
It makes a big difference in a large codebase I am working on, many
components with bindings that were not working suddenly started working.
In the absence of 'this is IBindingClient' I think the '_bindings in this'
check is as specific as we can get (and very likely faster to execute in
any case).
Let me know if you have any objections to that, in case I did not consider
something else that is obvious to you.
thanks
Greg
Re: Change in MXRoyale
Posted by Harbs <ha...@gmail.com>.
OK. Cool.
> On May 28, 2020, at 3:39 PM, Greg Dove <gr...@gmail.com> wrote:
>
> I think it is because we are using
> _strand["_bindings"]
> in some places it keeps it without renaming
>
>
> On Fri, May 29, 2020 at 12:38 AM Greg Dove <gr...@gmail.com> wrote:
>
>>
>> Actually maybe I need to double double-check this more, thanks for the
>> prompt. I will do that now.
>>
>>
>> On Fri, May 29, 2020 at 12:36 AM Greg Dove <gr...@gmail.com> wrote:
>>
>>>
>>> There are a few things like this where it does not. I forget exactly the
>>> rules for that. We are already using this exact same check elsewhere in
>>> bindings code, possibly since its inception.
>>>
>>> On Fri, May 29, 2020 at 12:33 AM Harbs <ha...@gmail.com> wrote:
>>>
>>>> I’m surprised that Closure Compiler renaming doesn’t break this.
>>>>
>>>>> On May 28, 2020, at 3:00 PM, Greg Dove <gr...@gmail.com> wrote:
>>>>>
>>>>> Alex,
>>>>>
>>>>> I hope you are ok with this change:
>>>>>
>>>> https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
>>>>>
>>>>>
>>>>> It makes a big difference in a large codebase I am working on, many
>>>>> components with bindings that were not working suddenly started
>>>> working.
>>>>> In the absence of 'this is IBindingClient' I think the '_bindings in
>>>> this'
>>>>> check is as specific as we can get (and very likely faster to execute
>>>> in
>>>>> any case).
>>>>>
>>>>> Let me know if you have any objections to that, in case I did not
>>>> consider
>>>>> something else that is obvious to you.
>>>>>
>>>>> thanks
>>>>> Greg
>>>>
>>>>
Re: Change in MXRoyale
Posted by Greg Dove <gr...@gmail.com>.
I think it is because we are using
_strand["_bindings"]
in some places it keeps it without renaming
On Fri, May 29, 2020 at 12:38 AM Greg Dove <gr...@gmail.com> wrote:
>
> Actually maybe I need to double double-check this more, thanks for the
> prompt. I will do that now.
>
>
> On Fri, May 29, 2020 at 12:36 AM Greg Dove <gr...@gmail.com> wrote:
>
>>
>> There are a few things like this where it does not. I forget exactly the
>> rules for that. We are already using this exact same check elsewhere in
>> bindings code, possibly since its inception.
>>
>> On Fri, May 29, 2020 at 12:33 AM Harbs <ha...@gmail.com> wrote:
>>
>>> I’m surprised that Closure Compiler renaming doesn’t break this.
>>>
>>> > On May 28, 2020, at 3:00 PM, Greg Dove <gr...@gmail.com> wrote:
>>> >
>>> > Alex,
>>> >
>>> > I hope you are ok with this change:
>>> >
>>> https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
>>> >
>>> >
>>> > It makes a big difference in a large codebase I am working on, many
>>> > components with bindings that were not working suddenly started
>>> working.
>>> > In the absence of 'this is IBindingClient' I think the '_bindings in
>>> this'
>>> > check is as specific as we can get (and very likely faster to execute
>>> in
>>> > any case).
>>> >
>>> > Let me know if you have any objections to that, in case I did not
>>> consider
>>> > something else that is obvious to you.
>>> >
>>> > thanks
>>> > Greg
>>>
>>>
Re: Change in MXRoyale
Posted by Greg Dove <gr...@gmail.com>.
Actually maybe I need to double double-check this more, thanks for the
prompt. I will do that now.
On Fri, May 29, 2020 at 12:36 AM Greg Dove <gr...@gmail.com> wrote:
>
> There are a few things like this where it does not. I forget exactly the
> rules for that. We are already using this exact same check elsewhere in
> bindings code, possibly since its inception.
>
> On Fri, May 29, 2020 at 12:33 AM Harbs <ha...@gmail.com> wrote:
>
>> I’m surprised that Closure Compiler renaming doesn’t break this.
>>
>> > On May 28, 2020, at 3:00 PM, Greg Dove <gr...@gmail.com> wrote:
>> >
>> > Alex,
>> >
>> > I hope you are ok with this change:
>> >
>> https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
>> >
>> >
>> > It makes a big difference in a large codebase I am working on, many
>> > components with bindings that were not working suddenly started working.
>> > In the absence of 'this is IBindingClient' I think the '_bindings in
>> this'
>> > check is as specific as we can get (and very likely faster to execute in
>> > any case).
>> >
>> > Let me know if you have any objections to that, in case I did not
>> consider
>> > something else that is obvious to you.
>> >
>> > thanks
>> > Greg
>>
>>
Re: Change in MXRoyale
Posted by Greg Dove <gr...@gmail.com>.
There are a few things like this where it does not. I forget exactly the
rules for that. We are already using this exact same check elsewhere in
bindings code, possibly since its inception.
On Fri, May 29, 2020 at 12:33 AM Harbs <ha...@gmail.com> wrote:
> I’m surprised that Closure Compiler renaming doesn’t break this.
>
> > On May 28, 2020, at 3:00 PM, Greg Dove <gr...@gmail.com> wrote:
> >
> > Alex,
> >
> > I hope you are ok with this change:
> >
> https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
> >
> >
> > It makes a big difference in a large codebase I am working on, many
> > components with bindings that were not working suddenly started working.
> > In the absence of 'this is IBindingClient' I think the '_bindings in
> this'
> > check is as specific as we can get (and very likely faster to execute in
> > any case).
> >
> > Let me know if you have any objections to that, in case I did not
> consider
> > something else that is obvious to you.
> >
> > thanks
> > Greg
>
>
Re: Change in MXRoyale
Posted by Harbs <ha...@gmail.com>.
I’m surprised that Closure Compiler renaming doesn’t break this.
> On May 28, 2020, at 3:00 PM, Greg Dove <gr...@gmail.com> wrote:
>
> Alex,
>
> I hope you are ok with this change:
> https://github.com/apache/royale-asjs/commit/26d4b9cf7c1968160278754a01bd612946ddf41d
>
>
> It makes a big difference in a large codebase I am working on, many
> components with bindings that were not working suddenly started working.
> In the absence of 'this is IBindingClient' I think the '_bindings in this'
> check is as specific as we can get (and very likely faster to execute in
> any case).
>
> Let me know if you have any objections to that, in case I did not consider
> something else that is obvious to you.
>
> thanks
> Greg
Re: Change in MXRoyale
Posted by Alex Harui <ah...@adobe.com.INVALID>.
Maybe just check for either MXMLDescriptor or _bindings. Might some containers not have _bindings?
Thanks for looking into it.
-Alex
On 5/29/20, 12:43 AM, "Greg Dove" <gr...@gmail.com> wrote:
OK I think I can see the issue.
The getter for mxmlDocument checks for MXMLDescriptor not being null (in
which case assume mxmlDocument is 'this') before mx_internal::addingChild
sets it to something else.
But if an mxml document (the only one in the ancestry) just has bindings
and no simple properties being set, and no child instances via mxml, then
there is no MXMLDescriptor getter generated. (and no generateMXMLAttributes
call either). But there are still bindings added.
Mostly the use of these mxml classes is to include bindings in the ancestry
of the actionscript subclasses, so the mxml is often just an extension of
an actionscript class that has [Bindable]'s and the mxml is the next link
in the chain that wires up the bindings (and that mxml version is used as
the base class for a bunch of other stuff).
I'm not sure what to do about the accuracy of mxmlDocument in these cases
yet, but I think '_bindings' in this is good so far in terms of whether a
Container needs the bindings support added or not.
I will try to give mxmlDocument a bit more thought over coming days. I will
also check what Flex does for those same classes with the document getter,
just in case it has the same result (for document property) in these cases.
On Fri, May 29, 2020 at 9:46 AM Greg Dove <gr...@gmail.com> wrote:
>
> This is not really about itemRenderers. More to do with regular mxml
> components I think, but tbh that is 'gut feel' and I didn't try to check a
> lot of the details so far, just was happy to make more things work.
> I will try to investigate/understand the potential mxmlDocument mismatch
> today and either report back here or fix it if I think it is obvious.
> Using
> '_bindings' in this
> might ultimately be a more performant option, and (although probably
> relatively rare) could perhaps avoid adding default binding support in some
> cases where it is not needed, I think.
>
>
> On Fri, May 29, 2020 at 4:11 AM Alex Harui <ah...@adobe.com.invalid>
> wrote:
>
>> Not opposed if it works, but do you know why it is needed? It makes me
>> worry that the mxmlDocument chain isn't set up properly and we'll end up
>> with other related bugs. Is the problem in mx:Container-based
>> itemrenderers? If so, the factories may need to propagate mxmlDocument
>> across the gaps in the DOM where non-UIComponents exist.
>>
>> HTH,
>> -Alex
>>
>> On 5/28/20, 5:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> Alex,
>>
>> I hope you are ok with this change:
>>
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fcommit%2F26d4b9cf7c1968160278754a01bd612946ddf41d&data=02%7C01%7Caharui%40adobe.com%7Cd670931e49854a897c8d08d803a40477%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637263350278484740&sdata=Gn%2FqKH5jqMMzBV2JU%2FCyOJV1H0N4%2BsjFsTM6OfXhdzY%3D&reserved=0
>>
>>
>> It makes a big difference in a large codebase I am working on, many
>> components with bindings that were not working suddenly started
>> working.
>> In the absence of 'this is IBindingClient' I think the '_bindings in
>> this'
>> check is as specific as we can get (and very likely faster to execute
>> in
>> any case).
>>
>> Let me know if you have any objections to that, in case I did not
>> consider
>> something else that is obvious to you.
>>
>> thanks
>> Greg
>>
>>
>>
Re: Change in MXRoyale
Posted by Greg Dove <gr...@gmail.com>.
OK I think I can see the issue.
The getter for mxmlDocument checks for MXMLDescriptor not being null (in
which case assume mxmlDocument is 'this') before mx_internal::addingChild
sets it to something else.
But if an mxml document (the only one in the ancestry) just has bindings
and no simple properties being set, and no child instances via mxml, then
there is no MXMLDescriptor getter generated. (and no generateMXMLAttributes
call either). But there are still bindings added.
Mostly the use of these mxml classes is to include bindings in the ancestry
of the actionscript subclasses, so the mxml is often just an extension of
an actionscript class that has [Bindable]'s and the mxml is the next link
in the chain that wires up the bindings (and that mxml version is used as
the base class for a bunch of other stuff).
I'm not sure what to do about the accuracy of mxmlDocument in these cases
yet, but I think '_bindings' in this is good so far in terms of whether a
Container needs the bindings support added or not.
I will try to give mxmlDocument a bit more thought over coming days. I will
also check what Flex does for those same classes with the document getter,
just in case it has the same result (for document property) in these cases.
On Fri, May 29, 2020 at 9:46 AM Greg Dove <gr...@gmail.com> wrote:
>
> This is not really about itemRenderers. More to do with regular mxml
> components I think, but tbh that is 'gut feel' and I didn't try to check a
> lot of the details so far, just was happy to make more things work.
> I will try to investigate/understand the potential mxmlDocument mismatch
> today and either report back here or fix it if I think it is obvious.
> Using
> '_bindings' in this
> might ultimately be a more performant option, and (although probably
> relatively rare) could perhaps avoid adding default binding support in some
> cases where it is not needed, I think.
>
>
> On Fri, May 29, 2020 at 4:11 AM Alex Harui <ah...@adobe.com.invalid>
> wrote:
>
>> Not opposed if it works, but do you know why it is needed? It makes me
>> worry that the mxmlDocument chain isn't set up properly and we'll end up
>> with other related bugs. Is the problem in mx:Container-based
>> itemrenderers? If so, the factories may need to propagate mxmlDocument
>> across the gaps in the DOM where non-UIComponents exist.
>>
>> HTH,
>> -Alex
>>
>> On 5/28/20, 5:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> Alex,
>>
>> I hope you are ok with this change:
>>
>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fcommit%2F26d4b9cf7c1968160278754a01bd612946ddf41d&data=02%7C01%7Caharui%40adobe.com%7C7034bdf00e5c47b6fa4708d802feb42c%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637262640254601272&sdata=UV%2F7%2FuX8nBNIHWJXp0YKCycjR0PkfvstUJv0lexZ%2B0s%3D&reserved=0
>>
>>
>> It makes a big difference in a large codebase I am working on, many
>> components with bindings that were not working suddenly started
>> working.
>> In the absence of 'this is IBindingClient' I think the '_bindings in
>> this'
>> check is as specific as we can get (and very likely faster to execute
>> in
>> any case).
>>
>> Let me know if you have any objections to that, in case I did not
>> consider
>> something else that is obvious to you.
>>
>> thanks
>> Greg
>>
>>
>>
Re: Change in MXRoyale
Posted by Greg Dove <gr...@gmail.com>.
This is not really about itemRenderers. More to do with regular mxml
components I think, but tbh that is 'gut feel' and I didn't try to check a
lot of the details so far, just was happy to make more things work.
I will try to investigate/understand the potential mxmlDocument mismatch
today and either report back here or fix it if I think it is obvious.
Using
'_bindings' in this
might ultimately be a more performant option, and (although probably
relatively rare) could perhaps avoid adding default binding support in some
cases where it is not needed, I think.
On Fri, May 29, 2020 at 4:11 AM Alex Harui <ah...@adobe.com.invalid> wrote:
> Not opposed if it works, but do you know why it is needed? It makes me
> worry that the mxmlDocument chain isn't set up properly and we'll end up
> with other related bugs. Is the problem in mx:Container-based
> itemrenderers? If so, the factories may need to propagate mxmlDocument
> across the gaps in the DOM where non-UIComponents exist.
>
> HTH,
> -Alex
>
> On 5/28/20, 5:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
>
> Alex,
>
> I hope you are ok with this change:
>
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fcommit%2F26d4b9cf7c1968160278754a01bd612946ddf41d&data=02%7C01%7Caharui%40adobe.com%7C7034bdf00e5c47b6fa4708d802feb42c%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637262640254601272&sdata=UV%2F7%2FuX8nBNIHWJXp0YKCycjR0PkfvstUJv0lexZ%2B0s%3D&reserved=0
>
>
> It makes a big difference in a large codebase I am working on, many
> components with bindings that were not working suddenly started
> working.
> In the absence of 'this is IBindingClient' I think the '_bindings in
> this'
> check is as specific as we can get (and very likely faster to execute
> in
> any case).
>
> Let me know if you have any objections to that, in case I did not
> consider
> something else that is obvious to you.
>
> thanks
> Greg
>
>
>
Re: Change in MXRoyale
Posted by Alex Harui <ah...@adobe.com.INVALID>.
Not opposed if it works, but do you know why it is needed? It makes me worry that the mxmlDocument chain isn't set up properly and we'll end up with other related bugs. Is the problem in mx:Container-based itemrenderers? If so, the factories may need to propagate mxmlDocument across the gaps in the DOM where non-UIComponents exist.
HTH,
-Alex
On 5/28/20, 5:00 AM, "Greg Dove" <gr...@gmail.com> wrote:
Alex,
I hope you are ok with this change:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Froyale-asjs%2Fcommit%2F26d4b9cf7c1968160278754a01bd612946ddf41d&data=02%7C01%7Caharui%40adobe.com%7C7034bdf00e5c47b6fa4708d802feb42c%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0%7C637262640254601272&sdata=UV%2F7%2FuX8nBNIHWJXp0YKCycjR0PkfvstUJv0lexZ%2B0s%3D&reserved=0
It makes a big difference in a large codebase I am working on, many
components with bindings that were not working suddenly started working.
In the absence of 'this is IBindingClient' I think the '_bindings in this'
check is as specific as we can get (and very likely faster to execute in
any case).
Let me know if you have any objections to that, in case I did not consider
something else that is obvious to you.
thanks
Greg