royale-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Piotr Zarzycki <piotrzarzyck...@gmail.com>
Subject Re: FlexJS element setter
Date Sun, 01 Oct 2017 16:07:28 GMT
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 <piotrzarzycki21@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.lists@gmail.com>:
>
>> I think I’m done. Any reason to not merge into develop?
>>
>> > On Sep 26, 2017, at 7:01 PM, Piotr Zarzycki <piotrzarzycki21@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.lists@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 <aharui@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" <pent@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.lists@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 <
>> >> piotrzarzycki21@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.lists@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 <pent@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.lists@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message