royale-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Harbs <harbs.li...@gmail.com>
Subject Re: FlexJS element setter
Date Sun, 01 Oct 2017 17:19:38 GMT
Thanks for looking it over.

Merged.

> On Oct 1, 2017, at 7:07 PM, Piotr Zarzycki <piotrzarzycki21@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 <piotrzarzycki21@gmail.com <mailto: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 <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>

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