Thanks for looking it over. Merged.
> On Oct 1, 2017, at 7:07 PM, Piotr Zarzycki <piotrzarzyck...@gmail.com> wrote: > > 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 > <mailto: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 >>>> <http://www.linkedin.com/piotrzarzycki> >>>> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552 >>>> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>> >>>> >>>> GitHub: https://github.com/piotrzarzycki21 >>>> <https://github.com/piotrzarzycki21> >>> >>> >> >> >> -- >> >> Piotr Zarzycki >> >> mobile: +48 880 859 557 <+48%20880%20859%20557> >> skype: zarzycki10 >> >> LinkedIn: http://www.linkedin.com/piotrzarzycki >> <http://www.linkedin.com/piotrzarzycki> >> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552 >> <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>> >> >> GitHub: https://github.com/piotrzarzycki21 >> <https://github.com/piotrzarzycki21> >> > > > > -- > > Piotr Zarzycki > > mobile: +48 880 859 557 > skype: zarzycki10 > > LinkedIn: http://www.linkedin.com/piotrzarzycki > <http://www.linkedin.com/piotrzarzycki> > <https://pl.linkedin.com/in/piotr-zarzycki-92a53552 > <https://pl.linkedin.com/in/piotr-zarzycki-92a53552>> > > GitHub: https://github.com/piotrzarzycki21 > <https://github.com/piotrzarzycki21>
