Added getSize() to FlexibleLayout

[markmarijnissen]

Unlike for example SequentialLayout, FlexibleLayout did not have a getSize() method. This causes trouble when using a FlexibleLayout in a Scrollview or another FlexibleLayout.

[dmvaldman]

@michaelobriena

[michaelobriena]

SequentialLayout is managing a collection of things with a size and therefore should have a size. FlexibleLayout is for cutting up useable space and the size is really a property of some other component / piece of state. I don't think it should have a size personally but am willing to be further convinced.

[markmarijnissen]

Okay @michaelobriena - let me make a case for getSize()

I learn from your response that getSize() is to be understood as "get the defined size of this RenderNode".

However, throughout Famo.us, getSize() is being used as if it is the computed size. You can find this in almost any layout. (i.e. See the render function of a SequentialLayout).

To complicate things further, Surface.getSize() actually takes an optional parameter actual which makes it return its actual size (i.e. pixels in the DOM) rather than defined size (which can contain true and undefined). Unfortunately, this method is actually not used in the layouts, causing the issues described below.

It seems as if there are two different ways to understand getSize:

• get me the defined size
• get me the actual (or computed) size

This causes two issues:

• Those unaware of this issue get confused when their Scrollview is not rendering.
• Those aware of this issue have to write a lot of boilerplate code (i.e. create Modifier, wrap in RenderNode, add actual node).

These issues are all being solved by defining getSize() as the actual computed size, rather than the defined size.

However, making such change may have one unexpected side-effect. I've read somewhere that you can exploit the difference between defined and actual size to manipulate layout. For example, you could create gaps between items in a Scrollview if the defined size is larger than the (css-styled) size of the element. I would argue, however, that this is an odd way to do things and should not be possible in the first place.

So, in the bigger scope of things I would say:

• getSize() should be understood as the actual computed size.
• All renderable RenderNodes should have a render and a getSize function.
• Surface should return actual size by default, and add a new method getDefinedSize() or similar to return the defined size.

[MylesBorins]

@michaelobriena thoughts?

[michaelobriena]

@markmarijnissen Great points. We are making Surface getSize return the actual size instead of the defined size which is going to be a big improvement in terms of usability.

I still believe the issue with giving FlexibleLayout getSize is that it gives the illusion that the FlexibleLayout is in control of it's own size. It is hard for me to not put it in because I think it could be useful and is practically already there but something really gnarly about faking people out about who controls the size of a FlexibleLayout.

I think the actual vs defined is a really strong argument in favor of including it. I think my hesitations are mostly for purity reasons which is kind of weak in regards to small matters of usability. I will run it by a few people but this will probably get in.

[AdrianRossouw]

I still believe the issue with giving FlexibleLayout getSize is that it gives the illusion that the FlexibleLayout is in control of it's own size. It is hard for me to not put it in because I think it could be useful and is practically already there but something really gnarly about faking people out about who controls the size of a FlexibleLayout.

So what would it take to be explicit and add an actualSize() method, that is aliased to getSize unless otherwise needed?

It would make all the code using it a lot more readable, and be fewer things about the internals to have to keep in mind.

[AdrianRossouw]

oh man. i should have read the issue before commenting. i just saw the last response in my email. 😊

[markmarijnissen]

@michaelobriena Let's see if we can understand your hesitations better, in the hope it will improve architecture and design decisions about famous. I'll have a wild guess.

Do you make a seperation between "define-time" and "run-time"? Let me explain:

• The Rendering Tree is created at define-time. It is an abstract and pure definition of what is rendered.
• The Engine transforms the Rendering Tree into performance-optimized DOM manipulation at run-time. This logic is encapsulated in the render and commit functions.

Things get messy because getSize() is used both at define-time and run-time. Issues arise because Scrollview, at runtime, needs to know the actual size of a FlexibleLayout to layout its items.

So a broader and completely different angle to this problem might be to strictly seperate runtime logic and definition logic. In other words, constrain runtime logic to commit and render functions of RenderNodes.

In practical terms, you could create private functions for runtime logic (actual size), which is only to be used in commit or render functions. This way, the Scrollview can retrieve actual size to layout its items.

I vote for _getActualSize() as private (runtime) function, and getSize() as pubic (definition) function, returning defined size for your definition logic.

[AdrianRossouw]

This could be done by creating private functions for getting the actual size, which can be used only when needed.

Guys, could we please avoid private functions.

I think the developer community has already proven that they need this information to do what they need to do, and I think that trying to keep them away from it is missing the forrest for the trees.

I don't think it's our job to try and stop people from doing stupid things. You can warn them and tell them why not do it, but at the end of the day they are just going to fork your code and do it anyway.

Having a convention of underscore prefixed means private and unreliable, is more than enough to send the right message.

Instead we should probably think about why people need the getSize to point to the computed size, and figure out the best way to help them accomplish their goal.

related: Famous/famous#71

[markmarijnissen]

Yes I agree Vertice - I just realized it is very wrong to use private (underscored) functions for actual public API. I think underscored functions should tell: "private, unreliable, might change".

The getActualSize() is required at least for rendering (a Scrollview), but there are other use cases that are valid at "definition-time". For example: What if I wanted to change size to be twice as big? Or create a background for a true-sized image? Or an overlay?

Perhaps a more clear name would be getRenderedSize(), as this clearly shows that this function only makes sense at run-time (i.e. when rendered) and therefore can only be used when rendering things.

[MylesBorins]

hey @markmarijnissen can you move this to famous/famous referencing this issue?