Combined row and column layout methods

[HalfWhitt]

Except for two spots that handle RTL text direction, the _layout_row_children and _layout_column_children methods of Pack are identical, swapping out the relevant names of width for height, top/bottom for left/right, etc. I'm impressed by the almost total lack of drift between two methods of 200+ lines each, but it would be easier to update and modify the layout mechanism if changes could be made in just one place. This PR combines both methods into one, _layout_children, that accepts a direction parameter uses node.style.direction to determine the main and cross axes.

My first pass at this involved a lot of ungainly getattr. To make the spellings clearer, I've added some convenience methods to Box and IntrinsicSize that accept the dimension name as a string, like so:

• node.layout.content_height -> node.layout.content("height")
• node.intrinsic.height -> node.intrinsic.dim("height")

I'm not at all married to these spellings, they just seemed intuitive.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

This will definitely conflict with #3054, but does make it and similar changes simpler.

[HalfWhitt]

I noticed an inconsistency that bothered me: the layout methods are all normal methods, so they receive the style object as self. But they're also passed the node, and node.style is the same style. Both references are then used, in various places. If we make these all class methods, then there's only one source of truth — and you can't accidentally get a mismatch either, if you incorrectly call a layout method on one style but somehow pass it the wrong node.

(Honestly they could just as easily be static methods, were it not for the calls to _debug, which then would have to become a free function and _depth a global variable.)

(Edit: unless of course those calls were then hard-coded to Pack._debug, but that kinda bothers me even in commented-out debug code...)

[freakboy3742]

I agree that the algorithm should (other than RTL handling) be identical between column and row layout - so I definitely support the general direction of this consolidation.

There's a lot of changes here, but my comments at this point mostly relate to two high level concepts, rather than specific line-by-line issues.

Firstly, I'm not sure I'm convinced by the move to class methods. I agree that having both node and self as arguments is (or should be) redundant. However, my inclination would be to lean into the use of self, rather than moving to class methods.

A class method is really just a standalone function, but namespaced, and I'm not sure that's what we actually want here. To my mind, the layout of a node really is a property of that node, not an abstract mutation of the node. If the method took a node and spat out a layout, then I might support the class method interpretation, but as it is, it's more of a "re-compute" process.

Secondly, I'm not convinced WIDTH and HEIGHT should be external constants like this - or, if they are, that they're public constants. The public constants in Pack are intended as symbols end-users could/should use to avoid typos in using strings. At best, these should be _WIDTH and _HEIGHT - but even then, I'm not 100% sure that's what is needed here.

The same goes for the utility methods for dim, min_content etc. These become symbols that are visible in the public namespace, entirely so that we can avoid getattr(self, f"content_{main_axis}") calls in the code. I'd even argue that the full getattr() call reads better in the context where it's being used. Interestingly, the getattr call should be identical to self[f"content_{name}"]... which came up recently as a "why does this even exist" question.

I'm sure I'll have additional comments on a deeper review, but at present, these high level issues are the bulk of the changes, so it's difficult to identify which changes are independent of the "broad strokes" changes that have been made.

[HalfWhitt]

Good call on things being private variables/names, that does make sense. I think I only made WIDTH and HEIGHT constants because it seemed weird defining them as strings in-place alongside the constants of START, COLUMN, etc. But functionally, they don't need to be named anything at all.

I can put all the getattrs back... It gets pretty ugly to call it with f-strings when it's embedded in debugging f-strings, though. How would you feel about keeping the methods and underscoring them? Edit: I put them back, they're fine, I'm not entirely sure what I was worried about.

Conceptually, keeping self and ditching node makes way more sense, and it's what I started to try first... I only went with class methods because the style doesn't keep a direct link to its node. Thinking about it now while more awake, however, it does have an indirect link through its applicator, so yeah, we could use that.

[HalfWhitt]

Interestingly, the getattr call should be identical to self[f"content_{name}"]... which came up recently as a "why does this even exist" question.

I don't think that works... self is a style/Pack object; the thing with a content_height is that style object's node's layout, in other words self._applicator.node.layout.content_height.

And even for styles, as currently implemented, bracket notation enforces valid property names, so it doesn't allow access to arbitrary attributes. style._applicator and style.layout() exist, but aren't accessible via style["_applicator"] and style["layout"]().

(It's really easy to get the styles and nodes mixed up — I keep doing it myself, including when I first wrote this comment and was thinking the layout object lived on the style instead of on the node.)

[HalfWhitt]

Eh. That's not too painful I suppose.

[HalfWhitt]

Aw crap... didn't think about the deprecation warning messing up the test suite. That's a lot of places to have to put pytest.warns... Since it's almost certainly only Travertino that will ever be doing this, I would be okay skipping a deprecation warning for this if you are.

[freakboy3742]

Interestingly, the getattr call should be identical to self[f"content_{name}"]... which came up recently as a "why does this even exist" question.

I don't think that works... self is a style/Pack object; the thing with a content_height is that style object's node's layout, in other words self._applicator.node.layout.content_height.

🤦 You are, of course, correct.

[freakboy3742]

This looks pretty good; the only question I've got at this point is around the migration path for the new layout() argument list. I agree that adding a warning would be exceedingly noisy; but we need to be clear on how modifying the layout() call in Travertino fits into the whole Toga 0.5/Travertino 0.4 upgrade path.

The other hesitation to merging is that @mhsmith was looking at some of the cross-axis alignment features (See #3054 and #1194); I'm not sure how "in flight" that work is, and whether this PR would be a big disruption to that work. We definitely want to merge this; it's just a question of ordering.

[HalfWhitt]

If it's just a matter of #3054, I'm fine with either order; it would probably be easier for @mhsmith to merge that first, then I'll merge main into here and do the necessary "translating".

If he's already working on the cross-axis alignment, this could wait till that's done; if not, merging this PR makes changes to the layout algorithm simpler to do.

[mhsmith]

#3054 should be ready to merge now, pending a review, and I don't have anything else in flight.

[freakboy3742]

#3054 should be ready to merge now, pending a review, and I don't have anything else in flight.

OK - I've just reviewed and merged that PR; happy conflict resolution @HalfWhitt :-)

[HalfWhitt]

(_initial_offset, that is, not _initial_content)

[freakboy3742]

All looks good, except for one edge case in RTL handling flagged inline.

[freakboy3742]

Looks good - thanks for the cleanup!