You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by Alex Harui <ah...@adobe.com> on 2016/09/01 04:23:44 UTC
Re: [FlexJX][Falcon] Binding support fixes/improvements
Before we try to patch Closure Library, I would still like to examine a
simple test case. I think it would help me understand the problem. I
thought that folks who created custom IEventDispatchers without extending
EventDispatcher wrapped an EventDispatcher and passed in a reference to
the instance to that EventDispatcher. Are you saying it didn't work at
all in JS? Or just in some scenario?
Thanks,
-Alex
On 8/31/16, 4:47 PM, "Greg Dove" <gr...@gmail.com> wrote:
>"I guess we need something on goog's EventTarget that is more
>setExternalTarget and that is only used for the event.target , not for the
>fireListeners call. "
>
>This type of change to their eventtarget.js works [1] and all that is
>needed is a change to setTargetForTesting(target) to be
>setTargetForTesting(target,true) in our constructor. Then that hack I
>added
>is no longer needed.
>
>I have not issued any request against closure lib there yet, but can do so
>if you think it is best (I don't know how it will be received). I'm not
>entirely comfortable doing this on behalf of Apache Flex, so would prefer
>if someone on the team did it.
>
>I can't see another simpler way around this (which doesn't mean there
>isn't
>one!). I definitely didn't want to put that method in the interface
>because
>it would deviate from flash.
>
>
>
>1.
>https://github.com/greg-dove/closure-library/commit/6be3161f02cf327a0dc1a3
>3f085f0531d2993b4e
>
>
>
>On Thu, Sep 1, 2016 at 10:03 AM, Greg Dove <gr...@gmail.com> wrote:
>
>> Alex, I just issued a PR that lets you easily see the problem (along
>>with
>> a fix for ant script for the manual test).
>>
>> I had a quick look inside eventtarget.js and it seems pretty clear to
>>me:
>>
>>
>>https://github.com/google/closure-library/blob/master/closure/goog/events
>>/
>> eventtarget.js#L349
>>
>> it is calling currentTarget.fireListeners and in the most basic case for
>> proxy event dispatching, this is the same as the 'targetForTesting'
>>which
>> requires that fireListeners is implemented on the alternate target.
>> when a subclass inherits EventDispatcher, currentTarget will always
>> be 'this' and so the same instance EventTarget's fireListeners method is
>> called directly.
>>
>> all the other variables and functions are referenced via 'this' which
>> would correctly resolve inside the proxy EventDispatcher instance,
>>although
>> they would all obviously be inherited if the class extends the flexjs
>> EventDispatcher.
>>
>> I guess we need something on goog's EventTarget that is more
>> setExternalTarget and that is only used for the event.target , not for
>>the
>> fireListeners call.
>>
>>
>>
>> On Thu, Sep 1, 2016 at 3:51 AM, Alex Harui <ah...@adobe.com> wrote:
>>
>>>
>>>
>>> On 8/31/16, 12:43 AM, "Greg Dove" <gr...@gmail.com> wrote:
>>>
>>> >I observed that issue when implementing IEventdispatcher i.e. when
>>>the
>>> >EventDispatcher constructor has an IEventDispatcher argument - so not
>>>for
>>> >statics. This is not seen when extending EventDispatcher, but in that
>>> case
>>> >the subclass would presumably inherit that method from goog
>>>EventTarget.
>>> >So
>>> >that setCurrentTarget method in the constructor did not seem to be the
>>> >complete answer here... that extra hack in the constructor just got
>>> things
>>> >working for now....it works as is but I don't like it.
>>>
>>> Interesting. Do you have a simple test case we can look at? Looking
>>>at
>>> the EventTarget code, I would think there would be other functions and
>>> variables that need propagation.
>>>
>>> Thanks,
>>> -Alex
>>>
>>>
>>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
sankar wrote
>
> PKumar wrote
>> @Sankar, FlexJS binding support is not similar to regular Flex SDK
>> binding and FlexJS DataGrid. FlexJS having the supported classes for
>> runtime data update in DataGrid and dataprovider. If you need the sample
>> code. i will check in my demo list & share with you.
> I'd love to see how this working, thanks PKumar!
@PKumar, is any input available for me? Please, suggest.
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p57168.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
PKumar wrote
> @Sankar, FlexJS binding support is not similar to regular Flex SDK
> binding and FlexJS DataGrid. FlexJS having the supported classes for
> runtime data update in DataGrid and dataprovider. If you need the sample
> code. i will check in my demo list & share with you.
I'd love to see how this working, thanks PKumar!
Peter Ent wrote
> I have assigned the JIRA issue to myself and will be looking into this.
That's great!
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p57044.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by PKumar <pr...@gmail.com>.
@Sankar, FlexJS binding support is not similar to regular Flex SDK binding
and FlexJS DataGrid. FlexJS having the supported classes for runtime data
update in DataGrid and dataprovider. If you need the sample code. i will
check in my demo list & share with you.
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p57041.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Peter Ent <pe...@adobe.com>.
I have assigned the JIRA issue to myself and will be looking into this.
‹peter
On 12/6/16, 1:11 AM, "sankar" <sa...@gmail.com> wrote:
>Can Peter or anyone from Apache dev suggest where the present development
>stays for runtime data update to DataGrid component, or even any specific
>beads way already available, as discussed in earlier comments/queries?
>
>I think it's very much essential to me as a Flex developer to have the
>runtime update feature available to DataGrid component. I understood
>FlexJS
>works per pay-as-you-go ideology. But yet it's a critical feature that
>many
>of us developers would want in their project, and I am sure about it.
>
>I have raised a JIRA question on this:
>https://issues.apache.org/jira/browse/FLEX-35197.
>
>It'd be great to hear from the devs on this.
>
>Thanks!
>
>
>
>--
>View this message in context:
>http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding
>-support-fixes-improvements-tp54632p57023.html
>Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
Can Peter or anyone from Apache dev suggest where the present development
stays for runtime data update to DataGrid component, or even any specific
beads way already available, as discussed in earlier comments/queries?
I think it's very much essential to me as a Flex developer to have the
runtime update feature available to DataGrid component. I understood FlexJS
works per pay-as-you-go ideology. But yet it's a critical feature that many
of us developers would want in their project, and I am sure about it.
I have raised a JIRA question on this:
https://issues.apache.org/jira/browse/FLEX-35197.
It'd be great to hear from the devs on this.
Thanks!
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p57023.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 12/1/16, 8:21 PM, "sankar" <sa...@gmail.com> wrote:
>
>> Alex Harui wrote
>> Peter knows this example better, so I'll ask him to answer.
>
>That sounds good!
>
>
>> Alex Harui wrote
>> I envision several kinds of view,
>> controller and model beads to manage this complexity. I'm not sure
>>which
>> ones have been built yet.
>
>
>
>I also wondering such level of process may requires addition of different
>DataGrid specific beads (i.e. data-binding etc.), but are they available
>any, to listen the collection change event from it's dataProvider.
>
>
I don't know which ones have been built yet. Peter may know better. I
expect that someday there will be a feature rich DG with the the most
complex beads that folks will use because it will have everything they
want in it, and will re-compose with lighter weight beads if they need it.
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p56908.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
Peter knows this example better, so I'll ask him to answer. Keep in mind
that with pay-as-you-go, every level of functionality adds code. The
smallest DG would not expect any changes: it would just display the data
in its columns. The next level of change support is adding and removing
whole rows. After that comes replacing a row, then we get into changing
fields in the rows. Each level adds more code and complexity in handling
things like selectedItem/selectedIndex. I envision several kinds of view,
controller and model beads to manage this complexity. I'm not sure which
ones have been built yet.
-Alex
On 12/1/16, 2:35 AM, "sankar" <sa...@gmail.com> wrote:
>Alex Harui wrote
>> But I think there isn't any code that will send itemUpdated if a field
>>in
>> an item
>> changes, only if an item changes.
>
>Does the above means that if I updates a whole item, it suppose to reflect
>to the row UI? Does DataGrid has necessary codes to listen itemUpdate
>event
>at all, or what we need to do if it can't?
>
>I tried to add a new item to the ArrayList; I also noticed the ArrayList
>has
>/'propertyChange'/ event, but don't know if that suppose to work with
>DataGrid:
>
>> var tmp:Product = new
>> Product("ps220","Weejets",35,190,"assets/smallorangerect.jpg")
>> ProductsModel(applicationModel).productList.addItem(tmp);
>>
>> ...
>> // inside model
>> [Bindable(event="propertyChange")]
>> public function get productList():ArrayList
>> {
>> return _productList;
>> }
>>
>> public function set productList(value:ArrayList):void
>> {
>> if (value != _productList)
>> {
>> _productList = value;
>> dispatchEvent(new Event("propertyChange"));
>> }
>> }
>
>
>
>
>
>--
>View this message in context:
>http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding
>-support-fixes-improvements-tp54632p56868.html
>Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
Alex Harui wrote
> But I think there isn't any code that will send itemUpdated if a field in
> an item
> changes, only if an item changes.
Does the above means that if I updates a whole item, it suppose to reflect
to the row UI? Does DataGrid has necessary codes to listen itemUpdate event
at all, or what we need to do if it can't?
I tried to add a new item to the ArrayList; I also noticed the ArrayList has
/'propertyChange'/ event, but don't know if that suppose to work with
DataGrid:
> var tmp:Product = new
> Product("ps220","Weejets",35,190,"assets/smallorangerect.jpg")
> ProductsModel(applicationModel).productList.addItem(tmp);
>
> ...
> // inside model
> [Bindable(event="propertyChange")]
> public function get productList():ArrayList
> {
> return _productList;
> }
>
> public function set productList(value:ArrayList):void
> {
> if (value != _productList)
> {
> _productList = value;
> dispatchEvent(new Event("propertyChange"));
> }
> }
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p56868.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 11/30/16, 9:25 PM, "sankar" <sa...@gmail.com> wrote:
>Alex Harui wrote
>> You should be able to convert ProductItemRenderer to MXML and use
>> data-binding, or just have the AS version listen for the appropriate
>>event
>> fired from the data item.
>
>I've tried to create a new MXML component based on DataItemRenderer, but I
>couldn't even build compiler starts showing this error:
>
>/1551: Internal error in ABC generator subsystem, when generating code
>for:
>E:\apache-flex-flexjs-0.8.0-bin\examples\flexjs\DataGridExample\src\produc
>ts\ProductItemRendererMXML.mxml:
>java.lang.NullPointerException
>/
>Is that class supposed to be extended as MXML component?
If you get an internal error, that's probably a bug in the compiler. File
a JIRA issue with the renderer. There is probably some pattern you are
using that is fooling the compiler.
>
>
>Alex Harui wrote
>> Because of the pay-as-you-go philosophy, the data
>> provider used in the example does not dispatch collection change events,
>> and the DataGrid and renderers aren't listening for it.
>
>What alteration do I need to make the example works to dispatch collection
>change event and DataGrid/renderers listens to it?
>
It looks like DataGridExample is already using ArrayList. But I think
there isn't any code that will send itemUpdated if a field in an item
changes, only if an item changes. If you can get itemUpdated to dispatch
you can then see if any code is listening and will do the right thing, but
it might be less work to have a custom renderer with binding like you
tried above.
HTH,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
Alex Harui wrote
> You should be able to convert ProductItemRenderer to MXML and use
> data-binding, or just have the AS version listen for the appropriate event
> fired from the data item.
I've tried to create a new MXML component based on DataItemRenderer, but I
couldn't even build compiler starts showing this error:
/1551: Internal error in ABC generator subsystem, when generating code for:
E:\apache-flex-flexjs-0.8.0-bin\examples\flexjs\DataGridExample\src\products\ProductItemRendererMXML.mxml:
java.lang.NullPointerException
/
Is that class supposed to be extended as MXML component?
Alex Harui wrote
> Because of the pay-as-you-go philosophy, the data
> provider used in the example does not dispatch collection change events,
> and the DataGrid and renderers aren't listening for it.
What alteration do I need to make the example works to dispatch collection
change event and DataGrid/renderers listens to it?
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p56850.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 11/30/16, 12:44 AM, "sankar" <sa...@gmail.com> wrote:
>I was recently testing 0.8.0 FlexJS nightly build, and I must say binding
>is
>more responsive here than 0.7.0 version. I was testing by the example
>project 'DataGridExample', and my interest was to see if data binding is
>working in grid item renderer as well. Is this doable with present nightly
>build?
>
>At runtime upon a button click I tried to modify an item row in
>/MyInitialView.mxml/ as this:
>
>> private function onButtonClicked():void
>> {
>> ProductsModel(applicationModel).productList.source[1].image =
>> "assets/smallorangerect.jpg";
>> }
>
>I tit-bit tried to dispatch event upon change here and there too, but that
>didn't effect DataGrid UI change, get/set method in DataItemRenderer also
>not being called.
>
>Is there any way in current nightly build that we can meet the above
>requirement?
I assume you are still using the ProductItemRenderer in the example? It
is written in ActionScript, so there is no compiler-generated data-binding
in the renderer. Because of the pay-as-you-go philosophy, the data
provider used in the example does not dispatch collection change events,
and the DataGrid and renderers aren't listening for it.
You should be able to convert ProductItemRenderer to MXML and use
data-binding, or just have the AS version listen for the appropriate event
fired from the data item.
HTH,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by sankar <sa...@gmail.com>.
I was recently testing 0.8.0 FlexJS nightly build, and I must say binding is
more responsive here than 0.7.0 version. I was testing by the example
project 'DataGridExample', and my interest was to see if data binding is
working in grid item renderer as well. Is this doable with present nightly
build?
My testing requirement was simple, changing any row of item's property at
runtime and see it's effect in dataGrid row. I tried to modify codes in
DataGridExample project per my understanding, to see if a runtime change to
the Product (model object) object can apply changes to DataGrid row. The
example also shown usage of DataItemRenderer. But I couldn't able to
implement the requirement I had.
I modified the Product model object to this:
> package products
> {
> import org.apache.flex.events.Event;
> import org.apache.flex.events.EventDispatcher;
>
> [Bindable] public class Product extends EventDispatcher
> {
> public function
> Product(id:String,title:String,detail:Number,sales:Number,image:String)
> {
> this.id = id;
> this.title = title;
> this.detail = detail;
> this.sales = sales;
> this.image = image;
> }
>
> public var id:String;
> public var title:String;
> public var detail:Number;
> public var sales:Number;
>
> private var _image:String;
>
> public function toString():String
> {
> return title;
> }
>
> [Bindable(event="propertyChange")]
> public function get image():String
> {
> return _image;
> }
>
> public function set image(value:String):void
> {
> if (value != _image)
> {
> _image = value;
> dispatchEvent(new Event("propertyChange"));
> }
> }
> }
> }
At runtime upon a button click I tried to modify an item row in
/MyInitialView.mxml/ as this:
> private function onButtonClicked():void
> {
> ProductsModel(applicationModel).productList.source[1].image =
> "assets/smallorangerect.jpg";
> }
I tit-bit tried to dispatch event upon change here and there too, but that
didn't effect DataGrid UI change, get/set method in DataItemRenderer also
not being called.
Is there any way in current nightly build that we can meet the above
requirement?
Thanks.
--
View this message in context: http://apache-flex-development.2333347.n4.nabble.com/FlexJX-Falcon-Binding-support-fixes-improvements-tp54632p56813.html
Sent from the Apache Flex Development mailing list archive at Nabble.com.
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
>For the patch mentioned below, will you apply that to develop? Or do you
>want me to do that sometime tomorrow?
Now that you are a committer, you can do it yourself!
I will! Will do that later today. :)
What do you mean by "pure". That it is a direct result of the parsing?
We don't have to finish this discussion now, I just want to stick in the
back of my head for a while.
I guess it is easier to describe it in terms of my personal analogy (which
as I said, may not apply in compiler-land in general, or specifically here)
If the Typed defintion representation of the AST is like the vector data in
a displayobject, then, I am thinking of [Bindable]'s implicit
implementation as possibly more like a Matrix or ColorTransform
Perhaps there is a way to do something like this already in the compiler
code and I just have not discovered it yet. iirc I think I did see some
'isImplicit()' methods.
Vaguely, I am thinking sort of like:
IDefintion {
extras:
Boolean isTransformed() //might be useful to know that this is 'virtual'
Boolean getHasTransforms()
IDefintion getTransformedDefintion()
set/clear transforms support methods etc... IDefinitionTransformer
}
IDefinitionTransformer { //e.g. [Bindable] on class or [Bindable] on var
Boolean isApplicableFor(IDefinition dev); //then add it to the collection
above (e.g. check if the class definition has 'Bindable')
void applyTo(IDefintion proxyDefinition); //called in sequence inside
getTransformedDefintion()
above
}
example:
definitionForOutput = class_defintion.getHasTransforms() ?
class_defintion. getTransformedDefintion()
: class_defintion;
getHasTransforms would create a proxy clone of the original definition and
then apply the transforms and cache the result for subsequent requests.
If the doc emitters are also driven from the type definitions (I think they
are?), then the above approach might permit docgen from the transformed
definitions as well.
But probably none of this really makes sense unless:
a) we can conceive of a scenario where one pass of the AST can result in
multiple outputs with different variations of transformed output
(transforms would be changed between outputs)
and/or
b) we want to make metadata-driven (or possibly other types of) output
transformations extensible
As I think you pointed out, Bindable (convenience) implementations would
only be an easy candidate for 'swappable' (e.g. for as3 signal based
implementation etc) if there were no explicit implementations of
getters/setters elsewhere in the project.
None of this is something I specifically 'want', it's just my mind
wandering. I should stop my ponderings here :) None of this may be relevant
- if you think it is useful to revisit, as you said, we can do so later.
On Thu, Sep 8, 2016 at 4:37 AM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 9/7/16, 1:11 AM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >I can see advantages either way, but had assumed that longer term it may
> >be
> >advantageous to keep the AST/Typed AST more 'pure'
>
> What do you mean by "pure". That it is a direct result of the parsing?
> We don't have to finish this discussion now, I just want to stick in the
> back of my head for a while.
>
> >
> >Re : "It is a convenience feature: the compiler could just report an error
> >saying you can't use [Bindable] on Objects."
> >I guess that is always another option. :)
> >[Bindable] implicit implementation was many times overused by some Flex
> >developers in the past, I was probably guilty of this myself in the early
> >days. But it is quite handy and quick sometimes, so long as you know the
> >implications of its use/abuse. And if it is taken away now I am pretty
> >sure
> >sdk users will ask for it back ;)
>
> I wouldn't take away the [Bindable] on Objects feature, I was just
> pointing out that it really is a shortcut to writing the proper source
> code. If I could, I would actually have the compiler alter the source
> file. Otherwise, imagine how surprised you are when you see a call to
> super() or debug into super() and it doesn't go to Object.
>
> IMO, implementing [Bindable] by hacking the AST is sort of like having a
> preprocessor phase (which the former Falcon engineers were very much
> against). I still ponder the notion of metadata-as-macros and having
> [Bindable] be one of them.
>
> >
> >For the patch mentioned below, will you apply that to develop? Or do you
> >want me to do that sometime tomorrow?
>
> Now that you are a committer, you can do it yourself!
>
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/7/16, 1:11 AM, "Greg Dove" <gr...@gmail.com> wrote:
>I can see advantages either way, but had assumed that longer term it may
>be
>advantageous to keep the AST/Typed AST more 'pure'
What do you mean by "pure". That it is a direct result of the parsing?
We don't have to finish this discussion now, I just want to stick in the
back of my head for a while.
>
>Re : "It is a convenience feature: the compiler could just report an error
>saying you can't use [Bindable] on Objects."
>I guess that is always another option. :)
>[Bindable] implicit implementation was many times overused by some Flex
>developers in the past, I was probably guilty of this myself in the early
>days. But it is quite handy and quick sometimes, so long as you know the
>implications of its use/abuse. And if it is taken away now I am pretty
>sure
>sdk users will ask for it back ;)
I wouldn't take away the [Bindable] on Objects feature, I was just
pointing out that it really is a shortcut to writing the proper source
code. If I could, I would actually have the compiler alter the source
file. Otherwise, imagine how surprised you are when you see a call to
super() or debug into super() and it doesn't go to Object.
IMO, implementing [Bindable] by hacking the AST is sort of like having a
preprocessor phase (which the former Falcon engineers were very much
against). I still ponder the notion of metadata-as-macros and having
[Bindable] be one of them.
>
>For the patch mentioned below, will you apply that to develop? Or do you
>want me to do that sometime tomorrow?
Now that you are a committer, you can do it yourself!
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Just circling back to this: sure, I understand your point, Alex. I also am
usually all for addressing things in a central location.
I can see advantages either way, but had assumed that longer term it may be
advantageous to keep the AST/Typed AST more 'pure' and have 'Bindable' (and
possibly other forms of future metadata-driven) output variations be some
form of transformation at output. I am not only not a compiler expert, I'd
consider myself only slightly more than a novice (I did get my head into
the haxe compiler at one point a few years back, but ). So the way I think
about things is more in terms of analogies of things I do understand, which
may perhaps not be appropriate here.
I agree with what you pointed out for code implications and making future
maintenance/enhancements easier. I will give some thought to this in the
coming weeks, as maybe there is a way to refactor this to make things
easier all round, but still keep the AST 'pure'-ish. I can discuss with you
closer to the time, if so.
Re : "It is a convenience feature: the compiler could just report an error
saying you can't use [Bindable] on Objects."
I guess that is always another option. :)
[Bindable] implicit implementation was many times overused by some Flex
developers in the past, I was probably guilty of this myself in the early
days. But it is quite handy and quick sometimes, so long as you know the
implications of its use/abuse. And if it is taken away now I am pretty sure
sdk users will ask for it back ;)
For the patch mentioned below, will you apply that to develop? Or do you
want me to do that sometime tomorrow?
It sounded like you were ok with it, although had not tested it yet - fyi I
did run it by a few of the example apps and made sure the unit tests still
passed etc. I also made sure it correctly generated a swf with
flash.events.EventDispatcher instead of
org.apache.flex.events.EventDispatcher configured - not for a FlexJS
actual binding test which I think may not work in some cases because of
coercion issues in the framework classes, but just to check that it handled
the config changes correctly and the swf output was valid when the base
class for the event dispatcher reverted to a 'native' player class.
cheers,
Greg
On Tue, Sep 6, 2016 at 4:49 PM, Alex Harui <ah...@adobe.com> wrote:
> I haven't tested the patch, looks reasonable.
>
> That said, the reason I tried hacking the AST in ASCompilationUnit was
> because I thought the "best" way would be to effectively fix up the source
> code as if it had been written properly in the first place. IOW, this
> code effectively looks for source code that does:
>
> [Bindable]
> SomeClass extends Object
> {
> }
>
> And makes it look like:
>
> [Bindable]
> SomeClass extends EventDispatcher
> {
> }
>
> As you know, there are other patterns for static binding. Or another way
> to think of it: if you do take the time to re-base your [Bindable] source
> code on EventDispatcher, none of this code in the compiler executes. It
> is a convenience feature: the compiler could just report an error saying
> you can't use [Bindable] on Objects.
>
> I'm not a compiler expert, but back when Falcon only produced SWFs, I
> guess it seemed reasonable to leave the AST alone and generate modified
> output, but with FalconJX leveraging the AST, I'm concerned about work
> that means that future compiler developers will have to know that the AST
> doesn't go straight to JS or whatever the output is. As we've seen, the
> JSDoc annotation was driven off the AST. What else might we someday want
> to change in the AST "conversion" that will require us knowing about this
> code in ClassDirectiveProcessor?
>
> Anyway, again, I don't see any hurry to go back and look into
> re-implementing this [Bindable] handling in ASCompilationUnit. At least
> we've recorded in the archives that there might be other ways to solve
> this problem.
>
> Thanks,
> -Alex
>
> On 9/5/16, 6:44 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >I think I found a solution for the dependency issue I mentioned.
> >The inclusion of "ValueChangeEvent" when not otherwise specified in the
> >project provided a useful clue here! (Otherwise I suspect I would not have
> >found this)
> >I need to do a bit more testing, but I expect to issue a related PR for a
> >change in ClassDirectiveProcessor before too long.
> >
> >On Mon, Sep 5, 2016 at 10:05 PM, Greg Dove <gr...@gmail.com> wrote:
> >
> >> Alex, I did some testing with a minimal project, I did not even have to
> >> try as-only, as SimpleApplication was sufficient. And yes, there is an
> >> issue if the dependency is not explicitly added somewhere. I will try to
> >> find a way to resolve this, otherwise the ASCompilationUnit code will
> >>need
> >> to go back.
> >> I had thought it might be useful to have the bindable implementation be
> >> more of any output variation than a manipulation of the AST. I had even
> >> wondered about longer term possibilities with such an approach - e.g.
> >> perhaps different binding implementation variations (e.g. as3 Signals
> >> instead of events or something like that). I realize that this is not
> >> something to think about seriously now. Plenty to do with things as they
> >> are... was just thinking of possibilities for the future.
> >>
> >> Anyhow, if I can't find a quick solution to add a specific dependency
> >>for
> >> output in the swf without the ASCompilationUnit AST manipulation
> >>approach,
> >> then that will need to go back (at least for now, anyway).
> >> Perhaps this was the reason you did this in the first place? Was it when
> >> the EventDispatcher base class for binding became non-native?
> >> If you think its best to try and fix the original problems with the
> >> original code in ASCompilationUnit , then perhaps that is the simplest
> >> route for now. If so, I'd be keen to leave the other stuff I added for a
> >> short time, but will happily remove the unnecessary parts once we can be
> >> sure they never get branched into as a fallback (i.e. we can be sure the
> >> ASCompilationUnit code catches all the 'extends' candidates). Let me
> >>know
> >> what you think.
> >>
> >> fyi I will be focusing on reflection work in whatever spare time I can
> >> find this week, following on from where you got to earlier this year
> >>with
> >> that. I will let you know where I get to later this week, I'm not sure
> >>if I
> >> will have it finished this week or not.
> >>
> >> cheers
> >> Greg
> >>
> >>
> >> On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <gr...@gmail.com> wrote:
> >>
> >>> Sound great. Yes, switching it back on at any point should remain a
> >>>quick
> >>> fix option because it would simply make the other code act as a
> >>>backstop.
> >>> I will take a quick look on Monday to see if I can find any problems in
> >>> small as-only projects where maybe the order of class definitions in
> >>>the
> >>> swf could be important with the current approach. Sounds a bit like a
> >>> theoretical edge case but you made me think about it.
> >>> Have a nice weekend.
> >>>
> >>> -Greg
> >>> [sent from my phone]
> >>>
> >>> On 3/09/2016 12:41 PM, "Alex Harui" <ah...@adobe.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
> >>>>
> >>>> >Yes, that was one of the major changes. I moved it alongside the
> >>>>other
> >>>> >implementation inside ClassDirectiveProcessor and was able to get
> >>>>more
> >>>> >consistent results.
> >>>> >I initially commented it out so I could get everything working using
> >>>>the
> >>>> >implements IEventDispatcher approach - which is the more general
> >>>> approach
> >>>> >that would work for all cases- in javascript, then added it back for
> >>>> both
> >>>> >targets as a 2nd output variation in a later commit, and it went to
> >>>> >ClassDirectorProcessor for swf.
> >>>> >
> >>>> >I was seeing 2 issues from the ASCompilationUnit implementation:
> >>>> >1. It was not always applying where it should have been (so in cases
> >>>> where
> >>>> >it could have been 'extends EventDispatcher' it would sometimes fall
> >>>> >through to the implements IEventDispatcher approach in
> >>>> >ClassDIrectiveProcessor anyway). I suspect this was
> >>>>threading-related,
> >>>> but
> >>>> >I am not so familiar with threads.
> >>>> >
> >>>> >2. Less of an issue, it could also apply an EventDispatcher base
> >>>>class
> >>>> in
> >>>> >static-only bindable cases, which is unnecessary, this I expect could
> >>>> have
> >>>> >been more easily fixed. I tried checking with hasModifier in the
> >>>> original
> >>>> >code, but I think that might not be available yet, iirc that caused
> >>>>an
> >>>> >error.
> >>>> >
> >>>> >So I moved everything to the ClassDirectiveProcessor, which had the
> >>>> other
> >>>> >implementation of bindable support in any case. Sorry, I thought I
> >>>>had
> >>>> >been
> >>>> >clear about that.
> >>>>
> >>>> Well, you probably did explain it, but it probably didn't stick in my
> >>>> head
> >>>> until we hit this last issue and I went looking for the code where I
> >>>>had
> >>>> a
> >>>> vague memory of hacking a base class. I still don't know the compiler
> >>>> code so well that it is clear in my head what work should be done
> >>>>where.
> >>>> It looks like the original attempt to deal with [Bindable] for SWF was
> >>>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
> >>>> FalconJX I decided to fix it by hacking the AST since that drives
> >>>> everything in FalconJX. The right answer may have been for me to move
> >>>> the
> >>>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
> >>>> existing tests for needsEventDispatcher, then maybe we wouldn't have
> >>>>had
> >>>> the two issues above.
> >>>>
> >>>> Anyway, I think we have recorded the possibility that AST hacking
> >>>>might
> >>>> be
> >>>> a solution if we hit other problems. I'm ok with leaving the code as
> >>>>is
> >>>> until we hit another problem but feel free to keep digging if you
> >>>>want.
> >>>>
> >>>> Thanks,
> >>>> -Alex
> >>>>
> >>>>
> >>
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
I haven't tested the patch, looks reasonable.
That said, the reason I tried hacking the AST in ASCompilationUnit was
because I thought the "best" way would be to effectively fix up the source
code as if it had been written properly in the first place. IOW, this
code effectively looks for source code that does:
[Bindable]
SomeClass extends Object
{
}
And makes it look like:
[Bindable]
SomeClass extends EventDispatcher
{
}
As you know, there are other patterns for static binding. Or another way
to think of it: if you do take the time to re-base your [Bindable] source
code on EventDispatcher, none of this code in the compiler executes. It
is a convenience feature: the compiler could just report an error saying
you can't use [Bindable] on Objects.
I'm not a compiler expert, but back when Falcon only produced SWFs, I
guess it seemed reasonable to leave the AST alone and generate modified
output, but with FalconJX leveraging the AST, I'm concerned about work
that means that future compiler developers will have to know that the AST
doesn't go straight to JS or whatever the output is. As we've seen, the
JSDoc annotation was driven off the AST. What else might we someday want
to change in the AST "conversion" that will require us knowing about this
code in ClassDirectiveProcessor?
Anyway, again, I don't see any hurry to go back and look into
re-implementing this [Bindable] handling in ASCompilationUnit. At least
we've recorded in the archives that there might be other ways to solve
this problem.
Thanks,
-Alex
On 9/5/16, 6:44 PM, "Greg Dove" <gr...@gmail.com> wrote:
>I think I found a solution for the dependency issue I mentioned.
>The inclusion of "ValueChangeEvent" when not otherwise specified in the
>project provided a useful clue here! (Otherwise I suspect I would not have
>found this)
>I need to do a bit more testing, but I expect to issue a related PR for a
>change in ClassDirectiveProcessor before too long.
>
>On Mon, Sep 5, 2016 at 10:05 PM, Greg Dove <gr...@gmail.com> wrote:
>
>> Alex, I did some testing with a minimal project, I did not even have to
>> try as-only, as SimpleApplication was sufficient. And yes, there is an
>> issue if the dependency is not explicitly added somewhere. I will try to
>> find a way to resolve this, otherwise the ASCompilationUnit code will
>>need
>> to go back.
>> I had thought it might be useful to have the bindable implementation be
>> more of any output variation than a manipulation of the AST. I had even
>> wondered about longer term possibilities with such an approach - e.g.
>> perhaps different binding implementation variations (e.g. as3 Signals
>> instead of events or something like that). I realize that this is not
>> something to think about seriously now. Plenty to do with things as they
>> are... was just thinking of possibilities for the future.
>>
>> Anyhow, if I can't find a quick solution to add a specific dependency
>>for
>> output in the swf without the ASCompilationUnit AST manipulation
>>approach,
>> then that will need to go back (at least for now, anyway).
>> Perhaps this was the reason you did this in the first place? Was it when
>> the EventDispatcher base class for binding became non-native?
>> If you think its best to try and fix the original problems with the
>> original code in ASCompilationUnit , then perhaps that is the simplest
>> route for now. If so, I'd be keen to leave the other stuff I added for a
>> short time, but will happily remove the unnecessary parts once we can be
>> sure they never get branched into as a fallback (i.e. we can be sure the
>> ASCompilationUnit code catches all the 'extends' candidates). Let me
>>know
>> what you think.
>>
>> fyi I will be focusing on reflection work in whatever spare time I can
>> find this week, following on from where you got to earlier this year
>>with
>> that. I will let you know where I get to later this week, I'm not sure
>>if I
>> will have it finished this week or not.
>>
>> cheers
>> Greg
>>
>>
>> On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <gr...@gmail.com> wrote:
>>
>>> Sound great. Yes, switching it back on at any point should remain a
>>>quick
>>> fix option because it would simply make the other code act as a
>>>backstop.
>>> I will take a quick look on Monday to see if I can find any problems in
>>> small as-only projects where maybe the order of class definitions in
>>>the
>>> swf could be important with the current approach. Sounds a bit like a
>>> theoretical edge case but you made me think about it.
>>> Have a nice weekend.
>>>
>>> -Greg
>>> [sent from my phone]
>>>
>>> On 3/09/2016 12:41 PM, "Alex Harui" <ah...@adobe.com> wrote:
>>>
>>>>
>>>>
>>>> On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>>>
>>>> >Yes, that was one of the major changes. I moved it alongside the
>>>>other
>>>> >implementation inside ClassDirectiveProcessor and was able to get
>>>>more
>>>> >consistent results.
>>>> >I initially commented it out so I could get everything working using
>>>>the
>>>> >implements IEventDispatcher approach - which is the more general
>>>> approach
>>>> >that would work for all cases- in javascript, then added it back for
>>>> both
>>>> >targets as a 2nd output variation in a later commit, and it went to
>>>> >ClassDirectorProcessor for swf.
>>>> >
>>>> >I was seeing 2 issues from the ASCompilationUnit implementation:
>>>> >1. It was not always applying where it should have been (so in cases
>>>> where
>>>> >it could have been 'extends EventDispatcher' it would sometimes fall
>>>> >through to the implements IEventDispatcher approach in
>>>> >ClassDIrectiveProcessor anyway). I suspect this was
>>>>threading-related,
>>>> but
>>>> >I am not so familiar with threads.
>>>> >
>>>> >2. Less of an issue, it could also apply an EventDispatcher base
>>>>class
>>>> in
>>>> >static-only bindable cases, which is unnecessary, this I expect could
>>>> have
>>>> >been more easily fixed. I tried checking with hasModifier in the
>>>> original
>>>> >code, but I think that might not be available yet, iirc that caused
>>>>an
>>>> >error.
>>>> >
>>>> >So I moved everything to the ClassDirectiveProcessor, which had the
>>>> other
>>>> >implementation of bindable support in any case. Sorry, I thought I
>>>>had
>>>> >been
>>>> >clear about that.
>>>>
>>>> Well, you probably did explain it, but it probably didn't stick in my
>>>> head
>>>> until we hit this last issue and I went looking for the code where I
>>>>had
>>>> a
>>>> vague memory of hacking a base class. I still don't know the compiler
>>>> code so well that it is clear in my head what work should be done
>>>>where.
>>>> It looks like the original attempt to deal with [Bindable] for SWF was
>>>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
>>>> FalconJX I decided to fix it by hacking the AST since that drives
>>>> everything in FalconJX. The right answer may have been for me to move
>>>> the
>>>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
>>>> existing tests for needsEventDispatcher, then maybe we wouldn't have
>>>>had
>>>> the two issues above.
>>>>
>>>> Anyway, I think we have recorded the possibility that AST hacking
>>>>might
>>>> be
>>>> a solution if we hit other problems. I'm ok with leaving the code as
>>>>is
>>>> until we hit another problem but feel free to keep digging if you
>>>>want.
>>>>
>>>> Thanks,
>>>> -Alex
>>>>
>>>>
>>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
I think I found a solution for the dependency issue I mentioned.
The inclusion of "ValueChangeEvent" when not otherwise specified in the
project provided a useful clue here! (Otherwise I suspect I would not have
found this)
I need to do a bit more testing, but I expect to issue a related PR for a
change in ClassDirectiveProcessor before too long.
On Mon, Sep 5, 2016 at 10:05 PM, Greg Dove <gr...@gmail.com> wrote:
> Alex, I did some testing with a minimal project, I did not even have to
> try as-only, as SimpleApplication was sufficient. And yes, there is an
> issue if the dependency is not explicitly added somewhere. I will try to
> find a way to resolve this, otherwise the ASCompilationUnit code will need
> to go back.
> I had thought it might be useful to have the bindable implementation be
> more of any output variation than a manipulation of the AST. I had even
> wondered about longer term possibilities with such an approach - e.g.
> perhaps different binding implementation variations (e.g. as3 Signals
> instead of events or something like that). I realize that this is not
> something to think about seriously now. Plenty to do with things as they
> are... was just thinking of possibilities for the future.
>
> Anyhow, if I can't find a quick solution to add a specific dependency for
> output in the swf without the ASCompilationUnit AST manipulation approach,
> then that will need to go back (at least for now, anyway).
> Perhaps this was the reason you did this in the first place? Was it when
> the EventDispatcher base class for binding became non-native?
> If you think its best to try and fix the original problems with the
> original code in ASCompilationUnit , then perhaps that is the simplest
> route for now. If so, I'd be keen to leave the other stuff I added for a
> short time, but will happily remove the unnecessary parts once we can be
> sure they never get branched into as a fallback (i.e. we can be sure the
> ASCompilationUnit code catches all the 'extends' candidates). Let me know
> what you think.
>
> fyi I will be focusing on reflection work in whatever spare time I can
> find this week, following on from where you got to earlier this year with
> that. I will let you know where I get to later this week, I'm not sure if I
> will have it finished this week or not.
>
> cheers
> Greg
>
>
> On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <gr...@gmail.com> wrote:
>
>> Sound great. Yes, switching it back on at any point should remain a quick
>> fix option because it would simply make the other code act as a backstop.
>> I will take a quick look on Monday to see if I can find any problems in
>> small as-only projects where maybe the order of class definitions in the
>> swf could be important with the current approach. Sounds a bit like a
>> theoretical edge case but you made me think about it.
>> Have a nice weekend.
>>
>> -Greg
>> [sent from my phone]
>>
>> On 3/09/2016 12:41 PM, "Alex Harui" <ah...@adobe.com> wrote:
>>
>>>
>>>
>>> On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>>
>>> >Yes, that was one of the major changes. I moved it alongside the other
>>> >implementation inside ClassDirectiveProcessor and was able to get more
>>> >consistent results.
>>> >I initially commented it out so I could get everything working using the
>>> >implements IEventDispatcher approach - which is the more general
>>> approach
>>> >that would work for all cases- in javascript, then added it back for
>>> both
>>> >targets as a 2nd output variation in a later commit, and it went to
>>> >ClassDirectorProcessor for swf.
>>> >
>>> >I was seeing 2 issues from the ASCompilationUnit implementation:
>>> >1. It was not always applying where it should have been (so in cases
>>> where
>>> >it could have been 'extends EventDispatcher' it would sometimes fall
>>> >through to the implements IEventDispatcher approach in
>>> >ClassDIrectiveProcessor anyway). I suspect this was threading-related,
>>> but
>>> >I am not so familiar with threads.
>>> >
>>> >2. Less of an issue, it could also apply an EventDispatcher base class
>>> in
>>> >static-only bindable cases, which is unnecessary, this I expect could
>>> have
>>> >been more easily fixed. I tried checking with hasModifier in the
>>> original
>>> >code, but I think that might not be available yet, iirc that caused an
>>> >error.
>>> >
>>> >So I moved everything to the ClassDirectiveProcessor, which had the
>>> other
>>> >implementation of bindable support in any case. Sorry, I thought I had
>>> >been
>>> >clear about that.
>>>
>>> Well, you probably did explain it, but it probably didn't stick in my
>>> head
>>> until we hit this last issue and I went looking for the code where I had
>>> a
>>> vague memory of hacking a base class. I still don't know the compiler
>>> code so well that it is clear in my head what work should be done where.
>>> It looks like the original attempt to deal with [Bindable] for SWF was
>>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
>>> FalconJX I decided to fix it by hacking the AST since that drives
>>> everything in FalconJX. The right answer may have been for me to move
>>> the
>>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
>>> existing tests for needsEventDispatcher, then maybe we wouldn't have had
>>> the two issues above.
>>>
>>> Anyway, I think we have recorded the possibility that AST hacking might
>>> be
>>> a solution if we hit other problems. I'm ok with leaving the code as is
>>> until we hit another problem but feel free to keep digging if you want.
>>>
>>> Thanks,
>>> -Alex
>>>
>>>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Alex, I did some testing with a minimal project, I did not even have to try
as-only, as SimpleApplication was sufficient. And yes, there is an issue if
the dependency is not explicitly added somewhere. I will try to find a way
to resolve this, otherwise the ASCompilationUnit code will need to go back.
I had thought it might be useful to have the bindable implementation be
more of any output variation than a manipulation of the AST. I had even
wondered about longer term possibilities with such an approach - e.g.
perhaps different binding implementation variations (e.g. as3 Signals
instead of events or something like that). I realize that this is not
something to think about seriously now. Plenty to do with things as they
are... was just thinking of possibilities for the future.
Anyhow, if I can't find a quick solution to add a specific dependency for
output in the swf without the ASCompilationUnit AST manipulation approach,
then that will need to go back (at least for now, anyway).
Perhaps this was the reason you did this in the first place? Was it when
the EventDispatcher base class for binding became non-native?
If you think its best to try and fix the original problems with the
original code in ASCompilationUnit , then perhaps that is the simplest
route for now. If so, I'd be keen to leave the other stuff I added for a
short time, but will happily remove the unnecessary parts once we can be
sure they never get branched into as a fallback (i.e. we can be sure the
ASCompilationUnit code catches all the 'extends' candidates). Let me know
what you think.
fyi I will be focusing on reflection work in whatever spare time I can find
this week, following on from where you got to earlier this year with that.
I will let you know where I get to later this week, I'm not sure if I will
have it finished this week or not.
cheers
Greg
On Sat, Sep 3, 2016 at 1:41 PM, Greg Dove <gr...@gmail.com> wrote:
> Sound great. Yes, switching it back on at any point should remain a quick
> fix option because it would simply make the other code act as a backstop.
> I will take a quick look on Monday to see if I can find any problems in
> small as-only projects where maybe the order of class definitions in the
> swf could be important with the current approach. Sounds a bit like a
> theoretical edge case but you made me think about it.
> Have a nice weekend.
>
> -Greg
> [sent from my phone]
>
> On 3/09/2016 12:41 PM, "Alex Harui" <ah...@adobe.com> wrote:
>
>>
>>
>> On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> >Yes, that was one of the major changes. I moved it alongside the other
>> >implementation inside ClassDirectiveProcessor and was able to get more
>> >consistent results.
>> >I initially commented it out so I could get everything working using the
>> >implements IEventDispatcher approach - which is the more general approach
>> >that would work for all cases- in javascript, then added it back for both
>> >targets as a 2nd output variation in a later commit, and it went to
>> >ClassDirectorProcessor for swf.
>> >
>> >I was seeing 2 issues from the ASCompilationUnit implementation:
>> >1. It was not always applying where it should have been (so in cases
>> where
>> >it could have been 'extends EventDispatcher' it would sometimes fall
>> >through to the implements IEventDispatcher approach in
>> >ClassDIrectiveProcessor anyway). I suspect this was threading-related,
>> but
>> >I am not so familiar with threads.
>> >
>> >2. Less of an issue, it could also apply an EventDispatcher base class in
>> >static-only bindable cases, which is unnecessary, this I expect could
>> have
>> >been more easily fixed. I tried checking with hasModifier in the original
>> >code, but I think that might not be available yet, iirc that caused an
>> >error.
>> >
>> >So I moved everything to the ClassDirectiveProcessor, which had the other
>> >implementation of bindable support in any case. Sorry, I thought I had
>> >been
>> >clear about that.
>>
>> Well, you probably did explain it, but it probably didn't stick in my head
>> until we hit this last issue and I went looking for the code where I had a
>> vague memory of hacking a base class. I still don't know the compiler
>> code so well that it is clear in my head what work should be done where.
>> It looks like the original attempt to deal with [Bindable] for SWF was
>> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
>> FalconJX I decided to fix it by hacking the AST since that drives
>> everything in FalconJX. The right answer may have been for me to move the
>> code from ClassDirectiveProcessor into ASCompilationUnit and use the
>> existing tests for needsEventDispatcher, then maybe we wouldn't have had
>> the two issues above.
>>
>> Anyway, I think we have recorded the possibility that AST hacking might be
>> a solution if we hit other problems. I'm ok with leaving the code as is
>> until we hit another problem but feel free to keep digging if you want.
>>
>> Thanks,
>> -Alex
>>
>>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Sound great. Yes, switching it back on at any point should remain a quick
fix option because it would simply make the other code act as a backstop.
I will take a quick look on Monday to see if I can find any problems in
small as-only projects where maybe the order of class definitions in the
swf could be important with the current approach. Sounds a bit like a
theoretical edge case but you made me think about it.
Have a nice weekend.
-Greg
[sent from my phone]
On 3/09/2016 12:41 PM, "Alex Harui" <ah...@adobe.com> wrote:
>
>
> On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >Yes, that was one of the major changes. I moved it alongside the other
> >implementation inside ClassDirectiveProcessor and was able to get more
> >consistent results.
> >I initially commented it out so I could get everything working using the
> >implements IEventDispatcher approach - which is the more general approach
> >that would work for all cases- in javascript, then added it back for both
> >targets as a 2nd output variation in a later commit, and it went to
> >ClassDirectorProcessor for swf.
> >
> >I was seeing 2 issues from the ASCompilationUnit implementation:
> >1. It was not always applying where it should have been (so in cases where
> >it could have been 'extends EventDispatcher' it would sometimes fall
> >through to the implements IEventDispatcher approach in
> >ClassDIrectiveProcessor anyway). I suspect this was threading-related, but
> >I am not so familiar with threads.
> >
> >2. Less of an issue, it could also apply an EventDispatcher base class in
> >static-only bindable cases, which is unnecessary, this I expect could have
> >been more easily fixed. I tried checking with hasModifier in the original
> >code, but I think that might not be available yet, iirc that caused an
> >error.
> >
> >So I moved everything to the ClassDirectiveProcessor, which had the other
> >implementation of bindable support in any case. Sorry, I thought I had
> >been
> >clear about that.
>
> Well, you probably did explain it, but it probably didn't stick in my head
> until we hit this last issue and I went looking for the code where I had a
> vague memory of hacking a base class. I still don't know the compiler
> code so well that it is clear in my head what work should be done where.
> It looks like the original attempt to deal with [Bindable] for SWF was
> done in ClassDirectiveProcessor, but when I wanted to fix a bug in
> FalconJX I decided to fix it by hacking the AST since that drives
> everything in FalconJX. The right answer may have been for me to move the
> code from ClassDirectiveProcessor into ASCompilationUnit and use the
> existing tests for needsEventDispatcher, then maybe we wouldn't have had
> the two issues above.
>
> Anyway, I think we have recorded the possibility that AST hacking might be
> a solution if we hit other problems. I'm ok with leaving the code as is
> until we hit another problem but feel free to keep digging if you want.
>
> Thanks,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/2/16, 5:10 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Yes, that was one of the major changes. I moved it alongside the other
>implementation inside ClassDirectiveProcessor and was able to get more
>consistent results.
>I initially commented it out so I could get everything working using the
>implements IEventDispatcher approach - which is the more general approach
>that would work for all cases- in javascript, then added it back for both
>targets as a 2nd output variation in a later commit, and it went to
>ClassDirectorProcessor for swf.
>
>I was seeing 2 issues from the ASCompilationUnit implementation:
>1. It was not always applying where it should have been (so in cases where
>it could have been 'extends EventDispatcher' it would sometimes fall
>through to the implements IEventDispatcher approach in
>ClassDIrectiveProcessor anyway). I suspect this was threading-related, but
>I am not so familiar with threads.
>
>2. Less of an issue, it could also apply an EventDispatcher base class in
>static-only bindable cases, which is unnecessary, this I expect could have
>been more easily fixed. I tried checking with hasModifier in the original
>code, but I think that might not be available yet, iirc that caused an
>error.
>
>So I moved everything to the ClassDirectiveProcessor, which had the other
>implementation of bindable support in any case. Sorry, I thought I had
>been
>clear about that.
Well, you probably did explain it, but it probably didn't stick in my head
until we hit this last issue and I went looking for the code where I had a
vague memory of hacking a base class. I still don't know the compiler
code so well that it is clear in my head what work should be done where.
It looks like the original attempt to deal with [Bindable] for SWF was
done in ClassDirectiveProcessor, but when I wanted to fix a bug in
FalconJX I decided to fix it by hacking the AST since that drives
everything in FalconJX. The right answer may have been for me to move the
code from ClassDirectiveProcessor into ASCompilationUnit and use the
existing tests for needsEventDispatcher, then maybe we wouldn't have had
the two issues above.
Anyway, I think we have recorded the possibility that AST hacking might be
a solution if we hit other problems. I'm ok with leaving the code as is
until we hit another problem but feel free to keep digging if you want.
Thanks,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Yes, that was one of the major changes. I moved it alongside the other
implementation inside ClassDirectiveProcessor and was able to get more
consistent results.
I initially commented it out so I could get everything working using the
implements IEventDispatcher approach - which is the more general approach
that would work for all cases- in javascript, then added it back for both
targets as a 2nd output variation in a later commit, and it went to
ClassDirectorProcessor for swf.
I was seeing 2 issues from the ASCompilationUnit implementation:
1. It was not always applying where it should have been (so in cases where
it could have been 'extends EventDispatcher' it would sometimes fall
through to the implements IEventDispatcher approach in
ClassDIrectiveProcessor anyway). I suspect this was threading-related, but
I am not so familiar with threads.
2. Less of an issue, it could also apply an EventDispatcher base class in
static-only bindable cases, which is unnecessary, this I expect could have
been more easily fixed. I tried checking with hasModifier in the original
code, but I think that might not be available yet, iirc that caused an
error.
So I moved everything to the ClassDirectiveProcessor, which had the other
implementation of bindable support in any case. Sorry, I thought I had been
clear about that.
cheers
Greg
On Sat, Sep 3, 2016 at 11:55 AM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 9/2/16, 4:30 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >"In looking at the change in Falcon, it appears that the patch was
> >required
> >because the resolveBaseClass does not resolve to EventDispatcher. Did you
> >look into trying to get resolveBaseClass to resolve to EventDispatcher.
> >I'm wondering if there will be other issues related to that."
> >
> >The change is more at the output stage rather than hacking the original
> >AST
> >to do that now. You know this far better than I do, but I don't *think*
> >this should cause an issue in js because of the way the framework classes
> >are always avalalable, but just thinking about it, perhaps for a simple AS
> >project with a couple of Bindable classes, if the implicit implementation
> >is created this may cause an issue in swf, because the baseclass
> >EventDispatcher is now non-native - if the dependency is not addressed
> >explicitly elsewhere in the swf - is that what you mean? I haven't tried
> >anything like this, but I can check this first thing on Monday my time.
>
> In looking more closely at these changes, one of the changes removed code
> from ASCompilationUnit that set the base class to EventDispatcher. I
> hadn't realized that code wasn't replaced elsewhere, but it seems like
> hacking the AST to set the base class would have also fixed the last two
> issues (the missing goog.require and the missing @extends). So now I'm
> more curious about why you chose to move the logic to
> ClassDirectiveProcessor, and that further makes me wonder if we just
> haven't noticed some other ramification of not hacking the AST.
>
> We can continue this on Monday, no hurry.
>
> Thanks,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/2/16, 4:30 PM, "Greg Dove" <gr...@gmail.com> wrote:
>"In looking at the change in Falcon, it appears that the patch was
>required
>because the resolveBaseClass does not resolve to EventDispatcher. Did you
>look into trying to get resolveBaseClass to resolve to EventDispatcher.
>I'm wondering if there will be other issues related to that."
>
>The change is more at the output stage rather than hacking the original
>AST
>to do that now. You know this far better than I do, but I don't *think*
>this should cause an issue in js because of the way the framework classes
>are always avalalable, but just thinking about it, perhaps for a simple AS
>project with a couple of Bindable classes, if the implicit implementation
>is created this may cause an issue in swf, because the baseclass
>EventDispatcher is now non-native - if the dependency is not addressed
>explicitly elsewhere in the swf - is that what you mean? I haven't tried
>anything like this, but I can check this first thing on Monday my time.
In looking more closely at these changes, one of the changes removed code
from ASCompilationUnit that set the base class to EventDispatcher. I
hadn't realized that code wasn't replaced elsewhere, but it seems like
hacking the AST to set the base class would have also fixed the last two
issues (the missing goog.require and the missing @extends). So now I'm
more curious about why you chose to move the logic to
ClassDirectiveProcessor, and that further makes me wonder if we just
haven't noticed some other ramification of not hacking the AST.
We can continue this on Monday, no hurry.
Thanks,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
"In looking at the change in Falcon, it appears that the patch was required
because the resolveBaseClass does not resolve to EventDispatcher. Did you
look into trying to get resolveBaseClass to resolve to EventDispatcher.
I'm wondering if there will be other issues related to that."
The change is more at the output stage rather than hacking the original AST
to do that now. You know this far better than I do, but I don't *think*
this should cause an issue in js because of the way the framework classes
are always avalalable, but just thinking about it, perhaps for a simple AS
project with a couple of Bindable classes, if the implicit implementation
is created this may cause an issue in swf, because the baseclass
EventDispatcher is now non-native - if the dependency is not addressed
explicitly elsewhere in the swf - is that what you mean? I haven't tried
anything like this, but I can check this first thing on Monday my time.
"I'll see if I can reproduce."
I think there was an example of this in the manualtests commit history of
MyIntiialView
On Sat, Sep 3, 2016 at 11:14 AM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 9/2/16, 3:26 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >Alex, I believe the last PRs I made across falcon and asjs address the
> >various recent issues we discussed.
>
> Hi Greg, thanks for sticking with it.
>
> In looking at the change in Falcon, it appears that the patch was required
> because the resolveBaseClass does not resolve to EventDispatcher. Did you
> look into trying to get resolveBaseClass to resolve to EventDispatcher.
> I'm wondering if there will be other issues related to that.
>
> >
> >One additional (strange) thing I observed (partly because I was using a
> >plain text editor at one point, without xml hinting) is that you can get a
> >swf with invalid bytecode (stack overflow or underflow) by having bad xml
> >in the mxml.
>
> I'll see if I can reproduce.
>
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/2/16, 3:26 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Alex, I believe the last PRs I made across falcon and asjs address the
>various recent issues we discussed.
Hi Greg, thanks for sticking with it.
In looking at the change in Falcon, it appears that the patch was required
because the resolveBaseClass does not resolve to EventDispatcher. Did you
look into trying to get resolveBaseClass to resolve to EventDispatcher.
I'm wondering if there will be other issues related to that.
>
>One additional (strange) thing I observed (partly because I was using a
>plain text editor at one point, without xml hinting) is that you can get a
>swf with invalid bytecode (stack overflow or underflow) by having bad xml
>in the mxml.
I'll see if I can reproduce.
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Alex, I believe the last PRs I made across falcon and asjs address the
various recent issues we discussed.
One additional (strange) thing I observed (partly because I was using a
plain text editor at one point, without xml hinting) is that you can get a
swf with invalid bytecode (stack overflow or underflow) by having bad xml
in the mxml.
I had a stray '-->' at one point in the mxml without a matching open
comment tag and there was no compiler error. The resulting swf was bad (but
the js output still seemed fine). The regular flex 4.x compiler gives an
error in this case, so this is something else that needs to be looked at -
I can come back to see if I can figure that out it if no-one else does, but
I am more keen to work on some reflection stuff next over the coming week
(I have something in mind that I think will help make framework development
work easier in general).
cheers
Greg
On Sat, Sep 3, 2016 at 5:15 AM, Greg Dove <gr...@gmail.com> wrote:
> Thanks! I was hoping it was something like that. I will add that now and
> test.
> I did look through the google closure docs for the annotations with the
> various outputs, but mostly followed whatever leads were already in the
> code and I did not look at the class level stuff I guess.
>
> (I actually had a lot of head-scratching on release output at one point
> when I was working on the earlier stuff with the static bindable var output
> - until I found Josh's comment elsewhere in the code about using the
> 'deprecated' @expose instead of @export for static accessors)
>
>
> On Sat, Sep 3, 2016 at 2:52 AM, Alex Harui <ah...@adobe.com> wrote:
>
>>
>>
>> On 9/1/16, 11:40 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> >Took me a little while to get to this...
>> >
>> >Actually, I am really confused here. I don't have a real 'fix', but I
>> >understand what is causing it after chasing a few wild geese for a while.
>> >It seems the remove-circulars setting is the primary cause of this
>> problem
>> >- removing that setting restores all the goog.requires stuff (which I
>> >*thought* I had already supported in the output), and after this js
>> >release
>> >mode seems fine. Is there any known bug here with remove-circulars? Or do
>> >I
>> >need to do something extra in the output to make it compatible with
>> >remove-circulars ?
>>
>> I forgot that it may not be as simple as adding goog.require. I just took
>> another look and saw that the constructor jsdoc is missing the @extends
>> (and/or @implements) directive which I think Google Closure Compiler uses,
>> but also the remove-circulars uses it to make sure that goog.requires for
>> base classes do not get removed.
>>
>> Hope that gets things working,
>> -Alex
>>
>>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Thanks! I was hoping it was something like that. I will add that now and
test.
I did look through the google closure docs for the annotations with the
various outputs, but mostly followed whatever leads were already in the
code and I did not look at the class level stuff I guess.
(I actually had a lot of head-scratching on release output at one point
when I was working on the earlier stuff with the static bindable var output
- until I found Josh's comment elsewhere in the code about using the
'deprecated' @expose instead of @export for static accessors)
On Sat, Sep 3, 2016 at 2:52 AM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 9/1/16, 11:40 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >Took me a little while to get to this...
> >
> >Actually, I am really confused here. I don't have a real 'fix', but I
> >understand what is causing it after chasing a few wild geese for a while.
> >It seems the remove-circulars setting is the primary cause of this problem
> >- removing that setting restores all the goog.requires stuff (which I
> >*thought* I had already supported in the output), and after this js
> >release
> >mode seems fine. Is there any known bug here with remove-circulars? Or do
> >I
> >need to do something extra in the output to make it compatible with
> >remove-circulars ?
>
> I forgot that it may not be as simple as adding goog.require. I just took
> another look and saw that the constructor jsdoc is missing the @extends
> (and/or @implements) directive which I think Google Closure Compiler uses,
> but also the remove-circulars uses it to make sure that goog.requires for
> base classes do not get removed.
>
> Hope that gets things working,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/1/16, 11:40 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Took me a little while to get to this...
>
>Actually, I am really confused here. I don't have a real 'fix', but I
>understand what is causing it after chasing a few wild geese for a while.
>It seems the remove-circulars setting is the primary cause of this problem
>- removing that setting restores all the goog.requires stuff (which I
>*thought* I had already supported in the output), and after this js
>release
>mode seems fine. Is there any known bug here with remove-circulars? Or do
>I
>need to do something extra in the output to make it compatible with
>remove-circulars ?
I forgot that it may not be as simple as adding goog.require. I just took
another look and saw that the constructor jsdoc is missing the @extends
(and/or @implements) directive which I think Google Closure Compiler uses,
but also the remove-circulars uses it to make sure that goog.requires for
base classes do not get removed.
Hope that gets things working,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Took me a little while to get to this...
Actually, I am really confused here. I don't have a real 'fix', but I
understand what is causing it after chasing a few wild geese for a while.
It seems the remove-circulars setting is the primary cause of this problem
- removing that setting restores all the goog.requires stuff (which I
*thought* I had already supported in the output), and after this js release
mode seems fine. Is there any known bug here with remove-circulars? Or do I
need to do something extra in the output to make it compatible with
remove-circulars ?
On Fri, Sep 2, 2016 at 2:25 PM, Greg Dove <gr...@gmail.com> wrote:
> ah. I was just about to circle back to this stuff. Thanks for picking that
> up... yes I will fix that, my mistake!
>
>
> On Fri, Sep 2, 2016 at 2:23 PM, Alex Harui <ah...@adobe.com> wrote:
>
>>
>>
>> On 9/1/16, 8:15 AM, "Alex Harui" <ah...@adobe.com> wrote:
>>
>> >I will take a look.
>> >
>> >Thanks for trying it.
>>
>> OK, I figured out why the release version is not working. The output for
>> InstanceTimer (for example) which got re-based to inherit from
>> EventDispatcher, is missing the goog.require for EventDispatcher. This
>> fools the Google Closure Compiler and it does not emit EventDispatcher
>> before InstanceTimer so the prototype never gets set properly and thus
>> dispatchEvent is missing. It works in js-debug because other code happens
>> to load EventDispatcher first.
>>
>> Do you want to make the fix to the compiler?
>>
>> Thanks,
>> -Alex
>>
>>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
ah. I was just about to circle back to this stuff. Thanks for picking that
up... yes I will fix that, my mistake!
On Fri, Sep 2, 2016 at 2:23 PM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 9/1/16, 8:15 AM, "Alex Harui" <ah...@adobe.com> wrote:
>
> >I will take a look.
> >
> >Thanks for trying it.
>
> OK, I figured out why the release version is not working. The output for
> InstanceTimer (for example) which got re-based to inherit from
> EventDispatcher, is missing the goog.require for EventDispatcher. This
> fools the Google Closure Compiler and it does not emit EventDispatcher
> before InstanceTimer so the prototype never gets set properly and thus
> dispatchEvent is missing. It works in js-debug because other code happens
> to load EventDispatcher first.
>
> Do you want to make the fix to the compiler?
>
> Thanks,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 9/1/16, 8:15 AM, "Alex Harui" <ah...@adobe.com> wrote:
>I will take a look.
>
>Thanks for trying it.
OK, I figured out why the release version is not working. The output for
InstanceTimer (for example) which got re-based to inherit from
EventDispatcher, is missing the goog.require for EventDispatcher. This
fools the Google Closure Compiler and it does not emit EventDispatcher
before InstanceTimer so the prototype never gets set properly and thus
dispatchEvent is missing. It works in js-debug because other code happens
to load EventDispatcher first.
Do you want to make the fix to the compiler?
Thanks,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
I will take a look.
Thanks for trying it.
-Alex
On 9/1/16, 1:51 AM, "Greg Dove" <gr...@gmail.com> wrote:
>Alex, here's what I tried:
>
> COMPILE::JS
>public class EventDispatcher extends goog.events.EventTarget implements
>IEventDispatcher
>{
>private var _target:IEventDispatcher;
> public function EventDispatcher(target:IEventDispatcher = null)
> {
>_target = target || this;
> }
>
> public function hasEventListener(type:String):Boolean
> {
> return goog.events.hasListener(this, type);
> }
>override public function dispatchEvent(event:Object):Boolean
>{
>try
>{
>//we get quite a few string events here, "initialize" etc
>//so this general approach doesn't work:
>//event.target = _target;
>if (event is String) event = new Event(event as String,_target);
>else if (event.target != undefined) event.target = _target;
>return super.dispatchEvent(event);
>}
>catch (e:Error)
>{
>if (e.name != "stopImmediatePropagation")
>throw e;
>}
>return false;
>}
>}
>
>
>This seemed to work in js-debug, but I saw issues (see below) in some code
>in release mode, I didn't get to the bottom of that yet, sorry.
>I had not seen these with the original version:
>Uncaught TypeError: this.dispatchEvent is not a function
>
>
>On Thu, Sep 1, 2016 at 6:20 PM, Greg Dove <gr...@gmail.com> wrote:
>
>> yeah, it's 6pm here now, I am breaking for dinner. Will try this evening
>> and report back here
>>
>> On Thu, Sep 1, 2016 at 6:16 PM, Alex Harui <ah...@adobe.com> wrote:
>>
>>>
>>>
>>> On 8/31/16, 11:03 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>>
>>> >In looking at the Google code, it appears that all we need to do in
>>>our
>>> >EventDispatcher.dispatchEvent override is set the event.target on the
>>> >event object to the wrapper. Do you see the same thing?
>>> >
>>> >I didn't try that, but yes I do see that now. well spotted! :)
>>> >that sounds good if all our events are descendants of
>>>goog.events.Event
>>>
>>> Do you have time to try it? I can try tomorrow (for me) if you don't
>>>have
>>> time.
>>>
>>> Thanks,
>>> -Alex
>>>
>>>
>>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Alex, here's what I tried:
COMPILE::JS
public class EventDispatcher extends goog.events.EventTarget implements
IEventDispatcher
{
private var _target:IEventDispatcher;
public function EventDispatcher(target:IEventDispatcher = null)
{
_target = target || this;
}
public function hasEventListener(type:String):Boolean
{
return goog.events.hasListener(this, type);
}
override public function dispatchEvent(event:Object):Boolean
{
try
{
//we get quite a few string events here, "initialize" etc
//so this general approach doesn't work:
//event.target = _target;
if (event is String) event = new Event(event as String,_target);
else if (event.target != undefined) event.target = _target;
return super.dispatchEvent(event);
}
catch (e:Error)
{
if (e.name != "stopImmediatePropagation")
throw e;
}
return false;
}
}
This seemed to work in js-debug, but I saw issues (see below) in some code
in release mode, I didn't get to the bottom of that yet, sorry.
I had not seen these with the original version:
Uncaught TypeError: this.dispatchEvent is not a function
On Thu, Sep 1, 2016 at 6:20 PM, Greg Dove <gr...@gmail.com> wrote:
> yeah, it's 6pm here now, I am breaking for dinner. Will try this evening
> and report back here
>
> On Thu, Sep 1, 2016 at 6:16 PM, Alex Harui <ah...@adobe.com> wrote:
>
>>
>>
>> On 8/31/16, 11:03 PM, "Greg Dove" <gr...@gmail.com> wrote:
>>
>> >In looking at the Google code, it appears that all we need to do in our
>> >EventDispatcher.dispatchEvent override is set the event.target on the
>> >event object to the wrapper. Do you see the same thing?
>> >
>> >I didn't try that, but yes I do see that now. well spotted! :)
>> >that sounds good if all our events are descendants of goog.events.Event
>>
>> Do you have time to try it? I can try tomorrow (for me) if you don't have
>> time.
>>
>> Thanks,
>> -Alex
>>
>>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
yeah, it's 6pm here now, I am breaking for dinner. Will try this evening
and report back here
On Thu, Sep 1, 2016 at 6:16 PM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 8/31/16, 11:03 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >In looking at the Google code, it appears that all we need to do in our
> >EventDispatcher.dispatchEvent override is set the event.target on the
> >event object to the wrapper. Do you see the same thing?
> >
> >I didn't try that, but yes I do see that now. well spotted! :)
> >that sounds good if all our events are descendants of goog.events.Event
>
> Do you have time to try it? I can try tomorrow (for me) if you don't have
> time.
>
> Thanks,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 8/31/16, 11:03 PM, "Greg Dove" <gr...@gmail.com> wrote:
>In looking at the Google code, it appears that all we need to do in our
>EventDispatcher.dispatchEvent override is set the event.target on the
>event object to the wrapper. Do you see the same thing?
>
>I didn't try that, but yes I do see that now. well spotted! :)
>that sounds good if all our events are descendants of goog.events.Event
Do you have time to try it? I can try tomorrow (for me) if you don't have
time.
Thanks,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
In looking at the Google code, it appears that all we need to do in our
EventDispatcher.dispatchEvent override is set the event.target on the
event object to the wrapper. Do you see the same thing?
I didn't try that, but yes I do see that now. well spotted! :)
that sounds good if all our events are descendants of goog.events.Event
I saw all the other parts except for this:
e.target = e.target || target;
On Thu, Sep 1, 2016 at 5:44 PM, Alex Harui <ah...@adobe.com> wrote:
>
>
> On 8/31/16, 9:51 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
> >Yeah, I guess that is not 'simple' in terms of an example but it was
> >easier
> >to show it with what was already there.
> >
> > "I thought that folks who created custom IEventDispatchers without
> >extending
> >EventDispatcher wrapped an EventDispatcher and passed in a reference to
> >the instance to that EventDispatcher."
> >
> >yep, I am definitely one of those folks too :). That's what I am doing.
> >
> >"Are you saying it didn't work atall in JS?"
> >
> >I did not see it work, and based on the eventtarget code, I can't see how
> >it can.
>
> Hmm, looking at this case again, I agree that it couldn't work. I must
> have had my head on sideways that day.
>
> The exception we hit in the test case is because we've set the
> actualEventTarget_ to the wrapper instead of the wrapped EventDispatcher,
> which causes the code to look in the wrong place for the fireListeners
> method. But we don't really want to do that at all. It doesn't matter
> which object manages the list of listeners to call. t think all we really
> want is to set the event.target to be the wrapper instead of the wrapped
> EventDispatcher.
>
> In looking at the Google code, it appears that all we need to do in our
> EventDispatcher.dispatchEvent override is set the event.target on the
> event object to the wrapper. Do you see the same thing? Then we don't
> need the other patches to Google's code you have proposed.
>
> Of course, I could be wrong.
>
> Thanks,
> -Alex
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
On 8/31/16, 9:51 PM, "Greg Dove" <gr...@gmail.com> wrote:
>Yeah, I guess that is not 'simple' in terms of an example but it was
>easier
>to show it with what was already there.
>
> "I thought that folks who created custom IEventDispatchers without
>extending
>EventDispatcher wrapped an EventDispatcher and passed in a reference to
>the instance to that EventDispatcher."
>
>yep, I am definitely one of those folks too :). That's what I am doing.
>
>"Are you saying it didn't work atall in JS?"
>
>I did not see it work, and based on the eventtarget code, I can't see how
>it can.
Hmm, looking at this case again, I agree that it couldn't work. I must
have had my head on sideways that day.
The exception we hit in the test case is because we've set the
actualEventTarget_ to the wrapper instead of the wrapped EventDispatcher,
which causes the code to look in the wrong place for the fireListeners
method. But we don't really want to do that at all. It doesn't matter
which object manages the list of listeners to call. t think all we really
want is to set the event.target to be the wrapper instead of the wrapped
EventDispatcher.
In looking at the Google code, it appears that all we need to do in our
EventDispatcher.dispatchEvent override is set the event.target on the
event object to the wrapper. Do you see the same thing? Then we don't
need the other patches to Google's code you have proposed.
Of course, I could be wrong.
Thanks,
-Alex
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Greg Dove <gr...@gmail.com>.
Yeah, I guess that is not 'simple' in terms of an example but it was easier
to show it with what was already there.
"I thought that folks who created custom IEventDispatchers without
extending
EventDispatcher wrapped an EventDispatcher and passed in a reference to
the instance to that EventDispatcher."
yep, I am definitely one of those folks too :). That's what I am doing.
"Are you saying it didn't work atall in JS?"
I did not see it work, and based on the eventtarget code, I can't see how
it can.
I don't actually know what the intent of their setTargetForTesting is so I
am not sure if it is a bug
or whether they only ever intended one EventTarget to be manipulating
another. The typing in their
code is Object, not goog.events.EventTarget for that method's argument, so
it might even be a bug on their side.
I made another minor change to what I had against eventtarget, which avoids
a little branching - if you replace your library copy of eventtarget.js
with this [1] you should
be able to change the setTargetForTesting call to
setTargetForTesting(target,true) and the hack can be avoided.
1.
https://raw.githubusercontent.com/greg-dove/closure-library/342fa762984b10e53ebe9fcb906b7af813edb739/closure/goog/events/eventtarget.js
On Thu, Sep 1, 2016 at 4:41 PM, Alex Harui <ah...@adobe.com> wrote:
> Nevermind, didn't see the PR. Thanks for creating it. Looking into it.
>
> On 8/31/16, 9:23 PM, "Alex Harui" <ah...@adobe.com> wrote:
>
> >Before we try to patch Closure Library, I would still like to examine a
> >simple test case. I think it would help me understand the problem. I
> >thought that folks who created custom IEventDispatchers without extending
> >EventDispatcher wrapped an EventDispatcher and passed in a reference to
> >the instance to that EventDispatcher. Are you saying it didn't work at
> >all in JS? Or just in some scenario?
> >
> >Thanks,
> >-Alex
> >
> >On 8/31/16, 4:47 PM, "Greg Dove" <gr...@gmail.com> wrote:
> >
> >>"I guess we need something on goog's EventTarget that is more
> >>setExternalTarget and that is only used for the event.target , not for
> >>the
> >>fireListeners call. "
> >>
> >>This type of change to their eventtarget.js works [1] and all that is
> >>needed is a change to setTargetForTesting(target) to be
> >>setTargetForTesting(target,true) in our constructor. Then that hack I
> >>added
> >>is no longer needed.
> >>
> >>I have not issued any request against closure lib there yet, but can do
> >>so
> >>if you think it is best (I don't know how it will be received). I'm not
> >>entirely comfortable doing this on behalf of Apache Flex, so would prefer
> >>if someone on the team did it.
> >>
> >>I can't see another simpler way around this (which doesn't mean there
> >>isn't
> >>one!). I definitely didn't want to put that method in the interface
> >>because
> >>it would deviate from flash.
> >>
> >>
> >>
> >>1.
> >>https://github.com/greg-dove/closure-library/commit/
> 6be3161f02cf327a0dc1a
> >>3
> >>3f085f0531d2993b4e
> >>
> >>
> >>
> >>On Thu, Sep 1, 2016 at 10:03 AM, Greg Dove <gr...@gmail.com> wrote:
> >>
> >>> Alex, I just issued a PR that lets you easily see the problem (along
> >>>with
> >>> a fix for ant script for the manual test).
> >>>
> >>> I had a quick look inside eventtarget.js and it seems pretty clear to
> >>>me:
> >>>
> >>>
> >>>https://github.com/google/closure-library/blob/master/
> closure/goog/event
> >>>s
> >>>/
> >>> eventtarget.js#L349
> >>>
> >>> it is calling currentTarget.fireListeners and in the most basic case
> >>>for
> >>> proxy event dispatching, this is the same as the 'targetForTesting'
> >>>which
> >>> requires that fireListeners is implemented on the alternate target.
> >>> when a subclass inherits EventDispatcher, currentTarget will always
> >>> be 'this' and so the same instance EventTarget's fireListeners method
> >>>is
> >>> called directly.
> >>>
> >>> all the other variables and functions are referenced via 'this' which
> >>> would correctly resolve inside the proxy EventDispatcher instance,
> >>>although
> >>> they would all obviously be inherited if the class extends the flexjs
> >>> EventDispatcher.
> >>>
> >>> I guess we need something on goog's EventTarget that is more
> >>> setExternalTarget and that is only used for the event.target , not for
> >>>the
> >>> fireListeners call.
> >>>
> >>>
> >>>
> >>> On Thu, Sep 1, 2016 at 3:51 AM, Alex Harui <ah...@adobe.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 8/31/16, 12:43 AM, "Greg Dove" <gr...@gmail.com> wrote:
> >>>>
> >>>> >I observed that issue when implementing IEventdispatcher i.e. when
> >>>>the
> >>>> >EventDispatcher constructor has an IEventDispatcher argument - so not
> >>>>for
> >>>> >statics. This is not seen when extending EventDispatcher, but in that
> >>>> case
> >>>> >the subclass would presumably inherit that method from goog
> >>>>EventTarget.
> >>>> >So
> >>>> >that setCurrentTarget method in the constructor did not seem to be
> >>>>the
> >>>> >complete answer here... that extra hack in the constructor just got
> >>>> things
> >>>> >working for now....it works as is but I don't like it.
> >>>>
> >>>> Interesting. Do you have a simple test case we can look at? Looking
> >>>>at
> >>>> the EventTarget code, I would think there would be other functions and
> >>>> variables that need propagation.
> >>>>
> >>>> Thanks,
> >>>> -Alex
> >>>>
> >>>>
> >>>
> >
>
>
Re: [FlexJX][Falcon] Binding support fixes/improvements
Posted by Alex Harui <ah...@adobe.com>.
Nevermind, didn't see the PR. Thanks for creating it. Looking into it.
On 8/31/16, 9:23 PM, "Alex Harui" <ah...@adobe.com> wrote:
>Before we try to patch Closure Library, I would still like to examine a
>simple test case. I think it would help me understand the problem. I
>thought that folks who created custom IEventDispatchers without extending
>EventDispatcher wrapped an EventDispatcher and passed in a reference to
>the instance to that EventDispatcher. Are you saying it didn't work at
>all in JS? Or just in some scenario?
>
>Thanks,
>-Alex
>
>On 8/31/16, 4:47 PM, "Greg Dove" <gr...@gmail.com> wrote:
>
>>"I guess we need something on goog's EventTarget that is more
>>setExternalTarget and that is only used for the event.target , not for
>>the
>>fireListeners call. "
>>
>>This type of change to their eventtarget.js works [1] and all that is
>>needed is a change to setTargetForTesting(target) to be
>>setTargetForTesting(target,true) in our constructor. Then that hack I
>>added
>>is no longer needed.
>>
>>I have not issued any request against closure lib there yet, but can do
>>so
>>if you think it is best (I don't know how it will be received). I'm not
>>entirely comfortable doing this on behalf of Apache Flex, so would prefer
>>if someone on the team did it.
>>
>>I can't see another simpler way around this (which doesn't mean there
>>isn't
>>one!). I definitely didn't want to put that method in the interface
>>because
>>it would deviate from flash.
>>
>>
>>
>>1.
>>https://github.com/greg-dove/closure-library/commit/6be3161f02cf327a0dc1a
>>3
>>3f085f0531d2993b4e
>>
>>
>>
>>On Thu, Sep 1, 2016 at 10:03 AM, Greg Dove <gr...@gmail.com> wrote:
>>
>>> Alex, I just issued a PR that lets you easily see the problem (along
>>>with
>>> a fix for ant script for the manual test).
>>>
>>> I had a quick look inside eventtarget.js and it seems pretty clear to
>>>me:
>>>
>>>
>>>https://github.com/google/closure-library/blob/master/closure/goog/event
>>>s
>>>/
>>> eventtarget.js#L349
>>>
>>> it is calling currentTarget.fireListeners and in the most basic case
>>>for
>>> proxy event dispatching, this is the same as the 'targetForTesting'
>>>which
>>> requires that fireListeners is implemented on the alternate target.
>>> when a subclass inherits EventDispatcher, currentTarget will always
>>> be 'this' and so the same instance EventTarget's fireListeners method
>>>is
>>> called directly.
>>>
>>> all the other variables and functions are referenced via 'this' which
>>> would correctly resolve inside the proxy EventDispatcher instance,
>>>although
>>> they would all obviously be inherited if the class extends the flexjs
>>> EventDispatcher.
>>>
>>> I guess we need something on goog's EventTarget that is more
>>> setExternalTarget and that is only used for the event.target , not for
>>>the
>>> fireListeners call.
>>>
>>>
>>>
>>> On Thu, Sep 1, 2016 at 3:51 AM, Alex Harui <ah...@adobe.com> wrote:
>>>
>>>>
>>>>
>>>> On 8/31/16, 12:43 AM, "Greg Dove" <gr...@gmail.com> wrote:
>>>>
>>>> >I observed that issue when implementing IEventdispatcher i.e. when
>>>>the
>>>> >EventDispatcher constructor has an IEventDispatcher argument - so not
>>>>for
>>>> >statics. This is not seen when extending EventDispatcher, but in that
>>>> case
>>>> >the subclass would presumably inherit that method from goog
>>>>EventTarget.
>>>> >So
>>>> >that setCurrentTarget method in the constructor did not seem to be
>>>>the
>>>> >complete answer here... that extra hack in the constructor just got
>>>> things
>>>> >working for now....it works as is but I don't like it.
>>>>
>>>> Interesting. Do you have a simple test case we can look at? Looking
>>>>at
>>>> the EventTarget code, I would think there would be other functions and
>>>> variables that need propagation.
>>>>
>>>> Thanks,
>>>> -Alex
>>>>
>>>>
>>>
>