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

[Carlos Rovira <ca...@apache.org>]

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}

<js:FontIcon text="{iconList ? iconList.icon : ''}" material="true" visible=
"{iconList ? iconList.icon != null : false}"/> <html:Span text="{iconList ?
iconList.label : ''}"/>

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 (<ah...@adobe.com.invalid>)
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
> 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
>
>
>
>
>

-- 
Carlos Rovira
http://about.me/carlosrovira