Hi Andrew, Good point about users adding their own ValueChangeEvent listeners. If we can detect a private variable is bindable and not rename it, that sounds like the solution that will work in the most places.
My 2 cents, -Alex On 12/10/18, 3:13 AM, "Frost, Andrew" <[email protected]> wrote: Hi all Thanks for the inputs.. just getting round to going through this. So I think we have two different approaches: 1) Ensure we correctly generate the property name to be used in the Watcher object. This would be a fix in the emitter code as per Greg's suggestions, so that the Watcher would be correctly checking for a valueChange event coming from the correct property. Greg you mentioned about the ValueChangeEvent being dispatched with the undecorated name of the property, but that's still fine as we're not listening out for that at this particular point: instead we're listening to changes that are being made on the object that's assigned to this property. However, I'm not sure what would happen if that property starts off as undefined/null and is then assigned at a later point, as in that case the watcher should be adding a listener to the parent object to spot when the property is set.. ah, having just checked it, yes you're right, this higher-level assignment is missed unless the property setter's event is also updated to the decorated property name... So the changes required are: a) in our list of bindings, we need to use the longer name b) in our dispatching of ValueChangeEvents, we need to use the longer name Although this (b) then also means we can't manually write code to listen to the changes, it only works within the binding framework.. 2) For bindable variables, don't use the decorated name for private variables. This is roughly what Alex suggests due to the longer strings that would then be generated throughout the binding code.. My slight amendment to this is that we can do it automatically within the compiler (I believe - I'll check, but the bound properties I believe are flagged by the IBindableVariableDefinition interface). I think this second approach is actually going to be needed, rather than just trying to use the decorated names everywhere, because of the potential issues with people trying to add listeners for changes in their bound private variables. Maybe that's rare, but #2 does seem a simple and more optimal approach.. thanks Andrew -----Original Message----- From: Alex Harui [mailto:[email protected]] Sent: 06 December 2018 21:15 To: [email protected] Subject: [EXTERNAL] Re: Binding to private variables [Bindable] Private/public/protected var someVar; Always results in generated code to convert the var into a getters/setter so we can detect writes. A JS setter results in: Object.defineProperty("someVar",... // if private it would be "MyClass_someVar" And that prevents the getter/setter from being renamed. I will note that if we generate decorated names for private variables we are generating a longer string in the final output that will not be minified. This is will make binding to private variables more expensive than binding to public variables. And what are the odds that the classes that use private binding are subclassing or will be subclassed with conflicting private names? I'd guess that it would be close to zero. So maybe the cheap answer is to tell folks to turn off private variable name decoration in the classes that want to use private binding. Then our old code-gen will work. HTH, -Alex On 12/6/18, 12:52 PM, "Greg Dove" <[email protected]> wrote: I did a sweep on some of the less common bindings a couple of years ago. I think a few things that were working then may no longer be working after various changes. I need to check the test project for that. I'm happy to revisit that if people send me their binding bugs (better still add an issue wtih example and assign to me). But I want to get some other things in first, so probably won't get to that until end of next week. -Greg On Fri, Dec 7, 2018 at 9:43 AM Harbs <[email protected]> wrote: > What I’m not sure is how any of this worked in minified code. > > I’ve used [Bindable]private quite a lot… > > I’m not sure how to best fix this, but it does seem like a problem which > needs fixing. > > > On Dec 6, 2018, at 9:59 PM, Greg Dove <[email protected]> wrote: > > > > The other thing to consider is the ValueChangeEvent that is being > > dispatched for automatically generated private [Bindable] vars > > > > It keeps the undecorated name in terms of what it dispatches. > > > > so for > > [Bindable] > > private var test:String > > > > you get > > > this.dispatchEvent(org.apache.royale.events.ValueChangeEvent.createUpdateEvent( > > this, "test", oldValue, value)); > > > > in the generated setter > > but the test with the local name is > > if (value != this.com_foo_bar_test_) > > or similar > > > > I did not use private bindings very often. I wonder what would happen in > > legacy flex code if a parent class had a bindable private var and a > > subclass had a bindable private var with the same name. I expect they > both > > might dispatch the same property change events. And of course they could > be > > completely different types. I'm not going to test this :). > > > > The point is that I assume the dispatched event should still have the > > original (undecorated) name. So maybe this is more tricky and should > always > > be with a getter/setter like Alex suggests. > > > > Just sounding off some quick thoughts. I have not thought too deeply > about > > this, I am sure you will! > > > > > > > > > > > > > > > > > > On Fri, Dec 7, 2018 at 8:16 AM Greg Dove <[email protected]> wrote: > > > >> Hi Andrew, I just had a quick go at doing this locally. I *think* it > >> should be able to be done in MXMLRoyaleEmitter with minor changes. > >> > >> I have not tested extensively, so I will just describe what I did and > you > >> can see if it works, and if it is universally applicable. > >> > >> The following is what I did: > >> > >> change the > >> encodeWatcher method signature to this: > >> private void encodeWatcher(WatcherInfoBase watcherInfoBase, IDefinition > >> sourceDefinition) > >> > >> and make 2 changes inside that method: > >> > >> #1. > >> for the part that is: > >> writeNewline(ASEmitterTokens.DOUBLE_QUOTE.getToken() + > >> propertyWatcherInfo.getPropertyName() + > >> ASEmitterTokens.DOUBLE_QUOTE.getToken() + > >> ASEmitterTokens.COMMA.getToken()); > >> > >> replace with this: > >> > >> String outputName = propertyWatcherInfo.getPropertyName(); > >> if (sourceDefinition != null && sourceDefinition.isPrivate() > >> && getMXMLWalker().getProject().getAllowPrivateNameConflicts()) { > >> JSRoyaleEmitter fjs = (JSRoyaleEmitter) ((IMXMLBlockWalker) > >> getMXMLWalker()) > >> .getASEmitter(); > >> outputName = > >> fjs.formatPrivateName(sourceDefinition.getParent().getQualifiedName(), > >> outputName); > >> } > >> writeNewline(ASEmitterTokens.DOUBLE_QUOTE.getToken() + > >> outputName + > >> ASEmitterTokens.DOUBLE_QUOTE.getToken() + > >> ASEmitterTokens.COMMA.getToken()); > >> > >> > >> #2 > >> further down, comply with the new method signature: > >> replace: > >> encodeWatcher(ent.getValue()); > >> > >> with: > >> IDefinition childSourceDefinition = (IDefinition) ent.getKey(); > >> encodeWatcher(ent.getValue(), childSourceDefinition); > >> > >> > >> > >> And also in > >> outputBindingInfoAsData method: > >> comply with the new method signature for encodeWatcher > >> > >> IDefinition sourceDefinition = (IDefinition) entry.getKey(); > >> encodeWatcher(watcherInfoBase, sourceDefinition); > >> > >> > >> I think that might work for the general case, and maybe it can be > >> simplified. I only tested quickly and just looked at the output, did not > >> actually try the resulting app. If this does not work, it might give > you a > >> lead. > >> -Greg > >> > >> On Fri, Dec 7, 2018 at 7:58 AM Alex Harui <[email protected]> > >> wrote: > >> > >>> Hi Andrew, > >>> > >>> Thanks for looking into this. > >>> > >>> One of the things to keep in mind about the compiler that isn't > >>> immediately obvious is that the JS compiler can be thought of almost > as a > >>> subclass of the SWF compiler. It isn't truly that way in the code, > but the > >>> way it is now, even if you don't want SWF output, we actually run the > most > >>> of the SWF compiler tasks on the source code before generating the JS > >>> because the transpiler's AST walk (that uses the Emitters to generate > the > >>> JS) does not have the smarts for semantic analysis and similar things > like > >>> binding and watcher analysis. > >>> > >>> The clue to that is which Eclipse projects or jars the various source > >>> files belong to. We will try not to introduce any files from the > >>> compiler-jx project/jar into the compiler and compiler-common > >>> project/jars. Doing so could create build order/dependency issues. > And > >>> thus, we don't want to access the Emitters from the WatcherAnalyzer, > nor > >>> have the WatcherAnalyzer know that it is talking to a RoyaleJSProject > >>> instead of a RoyaleProject (the latter which is designed for SWF > output). > >>> > >>> And, even if you did find a way to access the Emitter, I think the same > >>> Watcher database is used for SWF output as well, so you don't always > want > >>> to decorate the name of private variables in the data structure. > >>> > >>> After a quick think, I would suggest doing a bit more digging. I > noticed > >>> that in the snippet you posted, a getter function appeared to be > correctly > >>> generated. Is that part of the Watcher data or the Binding data? In > the > >>> ActionScript code (DataBindingBase and subclasses) the Binding data is > >>> supposed to discriminate between "simple" property chains that it can > >>> create more efficient Bindings for vs GenericBinding that can handle > more > >>> complex cases. The idea is that in some cases, we can't just access > the > >>> value by an instance.propName lookup and a getterFunction is generated > >>> instead. This is absolutely required in SWF code since external > classes > >>> like the Binding classes can't access private members of the class it > is > >>> watching. It has always felt like a "cheat" to me that Binding pokes > holes > >>> in the private access protection for Bindings, but that's pretty much > the > >>> only way we can do it in SWF code (which also sort of means you lose > the > >>> advantage of binding to private variables in the first place). In > fact, I > >>> think one of the optimizations for Royale binding will be that bindings > >>> will be faster/smaller on public variables than private variables. > >>> > >>> So, having said all that, one possible solution is to have the > >>> ActionScript code use the getter function instead of the propertyName > for > >>> private variables. I thought that the transpiler did not always > output a > >>> getter function if public property chains could access the value. The > AS > >>> code could then know to use a function instead of the property name. > If > >>> some future version of JS actually does provide private access > protection > >>> someday, we'll be glad we did this. Right now, the Royale JS binding > code > >>> has been relying on direct access to private variables, which is also a > >>> "cheat", IMO. > >>> > >>> Another possible approach is to change what is stored in the Watcher > >>> database. Add new fields to store the fact that the property is > private. > >>> Then in MXMLRoyaleEmitter, the watcher output code should have access > to an > >>> emitter, and there is no need to access compiler-jx classes from the > >>> WatcherAnalyzer. This approach (just adding more info in the shared > >>> Analyzer code instead of generating code) is probably more > >>> platform/output-independent. > >>> > >>> Or maybe you'll need both. > >>> > >>> Hope that made sense. Good luck, > >>> -Alex > >>> > >>> > >>> On 12/6/18, 6:30 AM, "Frost, Andrew" <[email protected]> wrote: > >>> > >>> Hi all > >>> > >>> I found recently that binding had stopped working when we were using > >>> private variables, for example: > >>> <test:TestComponent id="testing" width="100" height="20" > >>> boundValue="{_model.value}"> > >>> where we declare: > >>> private var _model : Model = Model.getModel(); > >>> > >>> However it works with protected/public variables.. > >>> > >>> The reason is clear when you look at the generated _bindings array, > >>> which contains: > >>> function() { return this.test_MainView__model.value; }, > >>> and then > >>> "_model", > >>> "valueChange", > >>> > >>> So basically the watcher list is using "_model" whereas the property > >>> is actually generated as "test_MainView__model". > >>> > >>> Looking at the compiler code, there's a function on the emitter > which > >>> does this translation: > >>> public String formatPrivateName(String className, String > name) { > >>> return className.replace(".", "_") + "_" + name; > >>> } > >>> > >>> However I can't work out how to get the watcher analyser class to > >>> access the emitter.. I think this is where the logic should go, because > >>> within "WatcherAnalyzer. analyzeIdentifierNode" we're getting the name > as a > >>> string and creating the watcher object based on this string. So I want > to > >>> do: > >>> name = def.getBaseName(); > >>> if (def.isPrivate() && > >>> project.getAllowPrivateNameConflicts()) > >>> { > >>> name = > >>> getEmitter().formatPrivateName(def.getParent().getQualifiedName(), > name); > >>> } > >>> > >>> My issue is that there's no access that I can see to the Emitter. > >>> > >>> So I thought I'd ask: > >>> > >>> 1. does anyone know how I could actually get a valid reference to > >>> the emitter from this point in the code? i.e. just through the > >>> JSRoyaleProject object from what I can see.. > >>> 2. or is there a better place where this should be? e.g. within > >>> getBaseName itself, or where the base name is set up on the > definition..? > >>> 3. or could we move the "formatPrivateName" method to somewhere > >>> more accessible? > >>> 4. or can I just reproduce the "formatPrivateName" functionality > >>> here, and add comments so that future maintainers know to update two > >>> locations in case this changes? > >>> > >>> The last option is simplest but very hacky.. but would appreciate > >>> thoughts on the best approach. > >>> > >>> > >>> thanks > >>> > >>> Andrew > >>> > >>> > >>> > >>> > >>> > >>> > >
