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 (<aha...@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