osu-framework icon indicating copy to clipboard operation
osu-framework copied to clipboard

Improve readability and handling of fill directions in `FillFlowContainer`

Open Flutterish opened this issue 4 years ago • 12 comments

This PR introduces a concept used by flexboxes (and probably some other UI design components) called main and cross axes. This is basically a Vector2 except rather than having X and Y, it has Main and Cross which can be interpreted depending on orientation. For example for FillDirection.{Full, Horizontal} the main axis is Axes.X and the cross axis is Axes.Y, and the opposite is true for the other FillDirections.

This allows us to ignore the actual direction we are using (only converting at the beginning and end) and operate as if we were handling just Full or Horizontal with Main and Cross rather than X and Y respectively. Due to this this PR also introduces a new FillDirection - FullVertical.

This PR makes some cases, especially new row handling, easier to read. Speaking of rows, (namingwise) I replaced them with "spans" as they, just like cross axes, lack orientation. FillFlowContainers have a ForceNewRow method, which I believe originally forced a row, even for the Vertical orientation, which did effectively nothing. Now, ForceNewRow was made obsolete and replaced with ForceNewSpan which would instead create a new column in such case. This is only a rename, as FFCs now use spans rather than rows and columns. [request: add when the obsolete member can be removed]

You might notice that ForceNewSpan creates a new span even if the direction should not create new spans (the not "Full" fill directions). This is how it worked originally too (for horizontal orientations, since it did nothing for vertical), but I think it should be discussed whether this is correct behaviour.

This PR also refactored child validation into its own method, rather than ifs scattered across the method.


Edit: The naming has changed to "Flow" and "Line" for position and "Main" and "Cross" for size. Spans have been also renamed to lines, which is the reason sizes need different names from positions, as without them a situation like "the line's line size" could arise, which is confusing.

Flutterish avatar Nov 19 '21 12:11 Flutterish

Idea: we could use "line" instead of "span". I think that would be more straightforward and it also implies that its a 1-dimensional line rather than a span which could imply more dimensions, even though in this context there is no space for such conflict.

Flutterish avatar Nov 20 '21 11:11 Flutterish

In general I disagree that this improves code quality or readability. It is a new useful feature, but abstracting away concrete axes makes this code less understandable, not more.

As a point of opinion, I find the Main/Cross distinction confusing. Especially so that I gather that the content primarily flows in the "cross" direction, until there is no more space, and only then the "main" direction is used. So in a sense the "main" direction of flow is... not the primary one?

I would suggest FlowingAxis and LineAxis or something similar as an alternative. That makes the distinction of which axis is used to flow content and which one is used to break content into lines clearer.

bdach avatar Nov 20 '21 12:11 bdach

As a point of opinion, I find the Main/Cross distinction confusing. Especially so that I gather that the content primarily flows in the "cross" direction, until there is no more space, and only then the "main" direction is used. So in a sense the "main" direction of flow is... not the primary one?

The "Main" axis is used first, then new rows/columns are added along the "Cross" axis. You should be able to see this from these lines alone: https://github.com/Flutterish/osu-framework/blob/7055441b9ba33ce03ce8af91680e70344e810143/osu.Framework/Graphics/Containers/FillFlowContainer.cs#L149-L160

Flutterish avatar Nov 20 '21 12:11 Flutterish

I think the name "Main" should stay, but changing the "Cross" to something else might indeed be useful. "Line" something, "Side" or "Orth"/"Perp" could do?

Flutterish avatar Nov 20 '21 12:11 Flutterish

The reason why I suggested what I did is that I would like to be able to tell at a glance which is the flowing direction and which is the line-breaking direction. For instance, the reason why I was confused with the current setup is that the outermost-scoped code starts off using Cross and then only uses Main in a super-nested block.

Those are the defining features of those axes: across one content flows, and across one content is broken up into lines. I feel that names like "cross", "main", "side", "orth" are all too generic to convey that clearly.

bdach avatar Nov 20 '21 12:11 bdach

Would FlowVector with Main or Flow as primary and Line as orthogonal do? I dont think they should have the word "Axis" in them. I would then rename the ToCross to ToFlowVector, I think that would be clear.

Flutterish avatar Nov 20 '21 12:11 Flutterish

I could work with that, yeah.

bdach avatar Nov 20 '21 13:11 bdach

Ah, one more thing. I am working on a container that implements the full flexbox spec and the concept of a flow vector is used there too. I was thinking I should perhaps move it out to its own file somewhere (since right now its a protected nested type) so it could use the same type?

Flutterish avatar Nov 20 '21 15:11 Flutterish

One type per file is the preferred style anyhow.

bdach avatar Nov 20 '21 15:11 bdach

There is one more thing that I could address in this PR - it is currently impossible to both reverse and center a flow at the same time. When children are centered they prefer left-right and top-down. I am not sure how to call the member that would indicate which direction if preferred, but I think it could be an enum that could either reverse the flow in all cases, or define a preferred direction for the center case alone, since the non-center cases depend on child anchors moving them to the appropriate side of the container.

Flutterish avatar Nov 21 '21 10:11 Flutterish

That sounds like a new feature and as such a separate pull. I would ask to keep pulls as small and self-contained as possible.

bdach avatar Nov 21 '21 10:11 bdach

Due to the confusion caused by a line being able to have a "line size" due to how the flow vector names are, I propose this naming: Frame 1 (1) The change only for the names of sizes on a given axis. The axes are still "flow" and "line", spans placed on the line axis are still "lines", but the line axis size is now a "cross size" and the flow axis size is now a "main size". I would also be fine with the main size being called a "flow size" but I think that might be confused with it being the 2d size. I might consider either making a copy of FlowVector - FlowSize with the names changed to Main and Cross, or giving the FlowVector alias names for the axes to be used when referring to sizes.

Flutterish avatar Nov 22 '21 06:11 Flutterish