Re: ItemRenderer musings (was Re: "has" vs "is": sharing code, swapping out subsystems, and more...)

2020-01-27 Thread Carlos Rovira
Hi Alex,

I'm happy with Item renderers, maybe we can do some refactors, but if it
was my time, I'll try to prioritize the actual few problems we have that
can be cumbersome for users and that use to make us code things that should
not be that way. I think as long as we have some problems that make us
(users of Royale) code things that should not be that way, that's a big
problem, since if some they the problems are fixed, is a lot of code in
user bases to update, and bad practices are consolidated in how people code.

Actual problems:

1.- Blog Example about List and Item Renderers shows a problem in the
renderer [1]. We need to check existence of iconList or binding fails. In
Flex we just to {iconList.label} or {iconList.icon}

 

2.- This issue opened by Piotr [2], about bindings in renderers.

Maybe most problems are in the use of renderers with bindings. As I said in
other occasions, binding issues in Royale are (IMHO) critical, since is
what could make the use of Royale in real world very problematic.

Far beyond, I think the optimizations you propose are good as always, but
in may case, I think "index" is for me a core property, and function calls
can not be a real problem for what I read [3]. ItemRendererParent should be
more easy to define, or give access to the main component that holds the
renderer (List?, DataGrid?) since some time it's not, and we need to need
searching that real parent (or the one you really need) from view beads.

In the, in real world apps, default renders like StringItemRender or
DataItemRender is used maybe 0 to 5%. Most of the times people need to
define its own itemrenderer that shows a check and a label, or a image or
icon and a label, or a dropdown,  to say some few cases.

Thanks

Carlos




[1]
https://github.com/apache/royale-asjs/blob/develop/examples/blog/BE0009_Using_an_Item_%20Renderer_with_a_List/src/main/royale/itemRenderers/IconListItemRenderer.mxml
[2] https://github.com/apache/royale-asjs/issues/639
[3]
https://www.codereadability.com/performance-cost-javascript-function-call-and-foreach/

El sáb., 25 ene. 2020 a las 9:03, Alex Harui ()
escribió:

> Hi,
>
> I had a window of time to try to do the "has" vs "is" refactor for
> ItemRenderers.
>
> However, I didn't get very far because as I started looking through the
> ItemRenderer code, I realized that there are other things that could use a
> refactor as well.  IMO, there is too much just-in-case code in current
> renderers.  IOW, too many if statements.  For example: getLabelFromData is
> full of just-in-case code
>
> Also:
> -Not all renderers should need an "index" property
> -There are both dataField vs labelField properties and I'm not sure anyone
> is using dataField
> -The renderer factories are setting labelFIeld on each renderer.
>
> Also, there are also a ton of DataItemRendererFactory variants, many
> containing duplicate code.
>
> And while I'm thinking about it, we should document why "data" set after
> parenting, which is different from our recommended lifecycle (normally we
> set properties then parent).  The reason is that the setting of the data
> property triggers bindings once, which can be evaluated against a known CSS
> tree, which is more efficient.
>
> Then there is the itemRendererParent property which is poorly named and I
> don't think it is being used properly.
>
> So, I'm leaning towards a more impactful refactor.  "Has" is not fast
> right now and I don't want to build a faster "has" capability until we see
> whether it works at all, but we also don't want to skew the data by asking
> too many unnecessary "has" questions.
>
> Some ideas for the refactor:
>
> A) The renderers should pull additional information (labelField,
> labelFunction), although there is a concern that could be expensive to find
> the additional info.  On the other hand, fewer pieces need to touch the
> additional information so maybe it is more PAYG and better encapsulated
>
> So, If each renderer got a 'data' and the itemRendererParent) it would get
> additional information via:
>
> myLabelFunction = itemRendererParent.strand.labelFunction
> or maybe
> myLabelFunction = itemRendererParent.strand.model.labelFunction
>
> B) One of the reasons there are so many DataItemRendererFactory classes is
> because creating renderers is a loop and needs to be fast so the variations
> are effectively inlined.  However, I've noticed that Harbs has been
> refactoring code into shared functions (like sendEvent).  Do we have data
> that function call overhead is not as big a factor as it was in Flash?  Or
> that we get gains back from the browser optimizing (maybe via JIT) hot code
> paths?
>
> If we're better off sharing more code, then all of the
> DataItemRendererFactory variants could extend some base class and have
> overridable methods or a few more composited beads to abstract the
> differences in the variants.  I think the 4 steps in the Factory are:
>
> 1) get next item
> 2) create the renderer
> 3) parent it
> 

ItemRenderer musings (was Re: "has" vs "is": sharing code, swapping out subsystems, and more...)

2020-01-25 Thread Alex Harui
Hi,

I had a window of time to try to do the "has" vs "is" refactor for 
ItemRenderers.

However, I didn't get very far because as I started looking through the 
ItemRenderer code, I realized that there are other things that could use a 
refactor as well.  IMO, there is too much just-in-case code in current 
renderers.  IOW, too many if statements.  For example: getLabelFromData is full 
of just-in-case code

Also:
-Not all renderers should need an "index" property
-There are both dataField vs labelField properties and I'm not sure anyone is 
using dataField
-The renderer factories are setting labelFIeld on each renderer.

Also, there are also a ton of DataItemRendererFactory variants, many containing 
duplicate code.

And while I'm thinking about it, we should document why "data" set after 
parenting, which is different from our recommended lifecycle (normally we set 
properties then parent).  The reason is that the setting of the data property 
triggers bindings once, which can be evaluated against a known CSS tree, which 
is more efficient.

Then there is the itemRendererParent property which is poorly named and I don't 
think it is being used properly.

So, I'm leaning towards a more impactful refactor.  "Has" is not fast right now 
and I don't want to build a faster "has" capability until we see whether it 
works at all, but we also don't want to skew the data by asking too many 
unnecessary "has" questions.

Some ideas for the refactor:

A) The renderers should pull additional information (labelField, 
labelFunction), although there is a concern that could be expensive to find the 
additional info.  On the other hand, fewer pieces need to touch the additional 
information so maybe it is more PAYG and better encapsulated

So, If each renderer got a 'data' and the itemRendererParent) it would get 
additional information via:

myLabelFunction = itemRendererParent.strand.labelFunction 
or maybe
myLabelFunction = itemRendererParent.strand.model.labelFunction

B) One of the reasons there are so many DataItemRendererFactory classes is 
because creating renderers is a loop and needs to be fast so the variations are 
effectively inlined.  However, I've noticed that Harbs has been refactoring 
code into shared functions (like sendEvent).  Do we have data that function 
call overhead is not as big a factor as it was in Flash?  Or that we get gains 
back from the browser optimizing (maybe via JIT) hot code paths?

If we're better off sharing more code, then all of the DataItemRendererFactory 
variants could extend some base class and have overridable methods or a few 
more composited beads to abstract the differences in the variants.  I think the 
4 steps in the Factory are:

1) get next item
2) create the renderer
3) parent it
4) set data on the renderer

Currently only #2 is done with a bead.  #1 and #4 probably could be as well (or 
by overriding a method in the base class).

The challenge is for #4 to try to allow different sets of properties to be 
passed to the renderer.  The index should be passed if the renderer wants it, 
and the data and the itemrendererview/parent.  

C) The renderers may also need a refactor.  StringItemRenderer should presume 
that data is a String.  It should not use getLabelFromData.  DataItemRenderer 
should assume there is a data object and a dataField/labelField.  That's why 
there is a TextItemRendererFactory and a DataItemRendererFactory.  The former 
assumes the data is a String, the latter assumes the data is an instance of an 
object.  

A renderer in the Express package can have more if statements and check if 
labelField is being used or not.

D) We might look at abstracting the computation of the label for a renderer.  
That feels like it would be too heavy in many cases, but right now we call 
getLabelFromData anyway.

Thoughts?
-Alex