Hi Harbs, I'm adding Royale dev list.
I just checked your changes with more complex app and apart of many conflicts during merge with my custom SDK I don't see any problems. Take a look into my comments to one of your commit. Once you do that from my sight is green light to merge it to develop. I'm sorry for a long delay! Thanks, Piotr 2017-09-26 21:01 GMT+02:00 Piotr Zarzycki <piotrzarzyck...@gmail.com>: > Harbs, > > Give me couple of days and I will pick up that branch and try it out. I > will also review those changes and give the feedback. > > Thanks! > > 2017-09-26 20:50 GMT+02:00 Harbs <harbs.li...@gmail.com>: > >> I think I’m done. Any reason to not merge into develop? >> >> > On Sep 26, 2017, at 7:01 PM, Piotr Zarzycki <piotrzarzyck...@gmail.com> >> wrote: >> > >> > Harbs, >> > >> > Please push those changes into separate branch "feature/" no matter how >> non >> > serious it look. I hope your changes will simplify things. >> > >> > Thank you! >> > >> > >> > 2017-09-26 17:54 GMT+02:00 Harbs <harbs.li...@gmail.com>: >> > >> >> I’m working on refactoring this. >> >> >> >> Is there a reason for the null check in UIBase.createElement()? >> >> >> >> Why would createElement be called if the element is already created? >> None >> >> of the subclasses have this null check. >> >> >> >> if (element == null) >> >> element = document.createElement('div') as WrappedHTMLElement; >> >> >> >> Do you think it’s safe to remove the check? >> >> >> >>> On Sep 26, 2017, at 6:42 PM, Alex Harui <aha...@adobe.com.INVALID> >> >> wrote: >> >>> >> >>> I believe there are components where more than one HTMLElement is >> created >> >>> but only one is (and can be) assigned as "element" but all have >> >>> flexjs_wrapper assigned to the wrapping IUIBase. >> >>> >> >>> If in fact no components need a separate positioner, it is fine to >> remove >> >>> it. But if we keep it, even as a getter returning element, we have to >> >>> make sure our code that positions things uses positioner and not >> element >> >>> in case someone does try to override positioner some day. >> >>> >> >>> As Peter mentioned, the original thinking was that element would be >> the >> >>> HTMLElement that defines the node in the DOM that dispatches >> interaction >> >>> events, but positioner might be some parent of the element like a Div >> >> used >> >>> to give the element appropriate visuals, chrome, accessory widgets, >> etc, >> >>> like NumericStepper, ComboBox, DateField, and maybe more sophisticated >> >>> components like a RichTextEditor where the "element" is a Div that >> gets >> >>> focus and holds the text lines, but is a child of a positioner Div >> that >> >>> also contains child buttons for bold/italic/underline. Another >> example >> >>> may be VideoPlayback. The element might be some sort of video widget >> but >> >>> the positioner might be a div that also contains child buttons for >> >>> stop/pause/rewind/forward. >> >>> >> >>> Of course, I could be wrong... >> >>> -Alex >> >>> >> >>> On 9/26/17, 6:27 AM, "Peter Ent" <p...@adobe.com.INVALID> wrote: >> >>> >> >>>> @Harbs: yes on get positioner returning element. This way someone >> could >> >>>> override the getter and return something else if it suited their >> needs. >> >>>> >> >>>> —peter >> >>>> >> >>>> On 9/26/17, 9:25 AM, "Harbs" <harbs.li...@gmail.com> wrote: >> >>>> >> >>>>> I looked at MDL and I don’t see any problem there. >> >>>>> >> >>>>> I’m talking about simplifying things across the board. I don’t see >> how >> >> it >> >>>>> could effect anything. >> >>>>> >> >>>>> @Peter I think removing positioner might not be a bad idea, but >> keeping >> >>>>> it and using it as a pointer to element is basically just as cheap. >> >>>>> >> >>>>>> On Sep 26, 2017, at 4:12 PM, Piotr Zarzycki < >> >> piotrzarzyck...@gmail.com> >> >>>>>> wrote: >> >>>>>> >> >>>>>> Hi Harbs, >> >>>>>> >> >>>>>> If you will do such changes like moving to set flexjs_wrapper in >> the >> >>>>>> setter >> >>>>>> of element - please make it on the separate branch. Let me test >> with >> >> my >> >>>>>> app >> >>>>>> whether MDL will not breaking up. I hope that we could avoid this >> one, >> >>>>>> even >> >>>>>> if I think that it seems to be quite reasonable to do that. >> >>>>>> >> >>>>>> Can you for example do this only for your custom component not for >> the >> >>>>>> global IUIBase class ? >> >>>>>> >> >>>>>> Let see what Peter say. >> >>>>>> >> >>>>>> Thanks, Piotr >> >>>>>> >> >>>>>> >> >>>>>> 2017-09-26 15:02 GMT+02:00 Harbs <harbs.li...@gmail.com>: >> >>>>>> >> >>>>>>> Yishay and I were working on drag/drop today and we were modifying >> >> one >> >>>>>>> of >> >>>>>>> the classes you wrote for generating the drag image. >> >>>>>>> >> >>>>>>> The code can be simplified by using cloneNode() and stuffing the >> >>>>>>> results >> >>>>>>> into the element. The thing is, it does not work until you assign >> the >> >>>>>>> flexjs_wrapper to the element. IMO, calling the element setter >> should >> >>>>>>> do >> >>>>>>> that automatically. >> >>>>>>> >> >>>>>>> On a similar note, Every IUIBase object has a positioner set. I >> don’t >> >>>>>>> know >> >>>>>>> of a single class which has a different positioner than the >> element. >> >>>>>>> It >> >>>>>>> seems to me that positioner should be a getter (which normally >> >> returns >> >>>>>>> the >> >>>>>>> element) that’s overridden for classes which need a different one. >> >>>>>>> That >> >>>>>>> will save memory for every IUIBase created. >> >>>>>>> >> >>>>>>> Harbs >> >>>>>>> >> >>>>>>>> On Sep 26, 2017, at 3:23 PM, Peter Ent <p...@adobe.com.INVALID> >> >>>>>>>> wrote: >> >>>>>>>> >> >>>>>>>> The setter for element is in HTMLElementWrapper, the super class >> for >> >>>>>>>> UIBase. The setter for flexes_wrapper is in UIBase. So if the >> >> element >> >>>>>>>> setter were to also set the flexjs_wrapper, it would have to be >> an >> >>>>>>>> override in UIBase to do it. At least that¹s how I understand it. >> >>>>>>>> >> >>>>>>>> Could you elaborate a little more on the issue that is raising >> this >> >>>>>>>> concern? >> >>>>>>>> >> >>>>>>>> Your question made me scan through these classes. Looking at this >> >>>>>>>> code >> >>>>>>> now >> >>>>>>>> makes me think we can do a better and more consistent job >> organizing >> >>>>>>>> it >> >>>>>>>> for Royale. After all, having code that can be quickly understood >> >> and >> >>>>>>>> modified is important. >> >>>>>>>> >> >>>>>>>> ‹peter >> >>>>>>>> >> >>>>>>>> On 9/26/17, 7:13 AM, "Harbs" <harbs.li...@gmail.com> wrote: >> >>>>>>>> >> >>>>>>>>> Currently, setting the element of a IUIBase will not work >> correctly >> >>>>>>>>> because the flexjs_wrapper is not set. This makes it error prone >> >>>>>>>>> when >> >>>>>>>>> there¹s a need to work with the underlying DOM elements for HTML >> >>>>>>>>> output. >> >>>>>>>>> >> >>>>>>>>> I cannot think of a reason why the wrapper should not be set >> when >> >>>>>>> calling >> >>>>>>>>> the element setter. Instead of setting the flexjs_wrapper in >> >>>>>>>>> createElement(), it seems to me that it should be set in the >> >> element >> >>>>>>>>> setter in HTMLElementWrapper. >> >>>>>>>>> >> >>>>>>>>> Anyone have a reason not to do this? >> >>>>>>>>> >> >>>>>>>>> Harbs >> >>>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>> >> >>>>>> >> >>>>>> -- >> >>>>>> >> >>>>>> Piotr Zarzycki >> >>>>>> >> >>>>>> mobile: +48 880 859 557 >> >>>>>> skype: zarzycki10 >> >>>>>> >> >>>>>> LinkedIn: >> >>>>>> https://na01.safelinks.protection.outlook.com/?url= >> >> http%3A%2F%2Fwww.link >> >>>>>> e >> >>>>>> din.com%2Fpiotrzarzycki&data=02%7C01%7C% >> >> 7C6716901213624a0e804708d504e203 >> >>>>>> 9 >> >>>>>> f%7Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0% >> >> 7C636420291136295544&sdata= >> >>>>>> f >> >>>>>> Q1CjLGkCpNKxSQBmZn%2BnKZEplQpGl25XACOqq0gO2o%3D&reserved=0 >> >>>>>> >> >>>>>> <https://na01.safelinks.protection.outlook.com/?url= >> >> https%3A%2F%2Fpl.lin >> >>>>>> k >> >>>>>> edin.com%2Fin%2Fpiotr-zarzycki-92a53552&data=02% >> >> 7C01%7C%7C6716901213624a >> >>>>>> 0 >> >>>>>> e804708d504e2039f%7Cfa7b1b5a7b34438794aed2c178de >> >> cee1%7C0%7C0%7C636420291 >> >>>>>> 1 >> >>>>>> 36295544&sdata=LzIej2n6WVnm9AG1Hi4NqIZjOQS% >> >> 2Byo4w%2BPYTX0Rq8Gc%3D&reserv >> >>>>>> e >> >>>>>> d=0> >> >>>>>> >> >>>>>> GitHub: >> >>>>>> https://na01.safelinks.protection.outlook.com/?url= >> >> https%3A%2F%2Fgithub. >> >>>>>> c >> >>>>>> om%2Fpiotrzarzycki21&data=02%7C01%7C%7C6716901213624a0e80470 >> 8d504e2 >> >> 039f% >> >>>>>> 7 >> >>>>>> Cfa7b1b5a7b34438794aed2c178decee1%7C0%7C0% >> >> 7C636420291136295544&sdata=WeI >> >>>>>> l >> >>>>>> LzVsJzRKniD1r9F2xb%2BwljhCLHnurBnl03kBM9E%3D&reserved=0 >> >>>>> >> >>>> >> >>> >> >> >> >> >> > >> > >> > -- >> > >> > Piotr Zarzycki >> > >> > mobile: +48 880 859 557 <+48%20880%20859%20557> >> > skype: zarzycki10 >> > >> > LinkedIn: http://www.linkedin.com/piotrzarzycki >> > <https://pl.linkedin.com/in/piotr-zarzycki-92a53552> >> > >> > GitHub: https://github.com/piotrzarzycki21 >> >> > > > -- > > Piotr Zarzycki > > mobile: +48 880 859 557 <+48%20880%20859%20557> > skype: zarzycki10 > > LinkedIn: http://www.linkedin.com/piotrzarzycki > <https://pl.linkedin.com/in/piotr-zarzycki-92a53552> > > GitHub: https://github.com/piotrzarzycki21 > -- Piotr Zarzycki mobile: +48 880 859 557 skype: zarzycki10 LinkedIn: http://www.linkedin.com/piotrzarzycki <https://pl.linkedin.com/in/piotr-zarzycki-92a53552> GitHub: https://github.com/piotrzarzycki21