Updated Pack property names to match CSS #3033

HalfWhitt · 2024-12-09T03:46:10Z

@mhsmith pointed out in #3011 that the padding (et al) and alignment Pack properties would be better named margin and align_items, to match their corresponding CSS properties. I've renamed them throughout the codebase, and set up an aliasing system for backwards compatibility (with tests). I've also taken the liberty of clarifying the name of text alignment whenever I encountered it in my searching-and-replacing.

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

freakboy3742

That's a whole lot of changes, but it's mostly a mechanical search and replace.

A couple of minor typos in docs flagged inline.

The only issue of substance is the set of allowed values for align-items. If we're going to the effort to ensure that we're using a "true" CSS subset, it seems like we should also be correcting the inconsistency in the allowed values. This would have the added benefit of removing the ambiguity around the interpretation with direction.

freakboy3742 · 2024-12-09T06:58:41Z

core/src/toga/style/pack.py

@@ -56,12 +60,12 @@
 DISPLAY_CHOICES = Choices(PACK, NONE)
 VISIBILITY_CHOICES = Choices(VISIBLE, HIDDEN)
 DIRECTION_CHOICES = Choices(ROW, COLUMN)
-ALIGNMENT_CHOICES = Choices(LEFT, RIGHT, TOP, BOTTOM, CENTER)
+ALIGN_ITEMS_CHOICES = Choices(LEFT, RIGHT, TOP, BOTTOM, CENTER)


It's worth noting that these aren't valid values for CSS3-flexbox align_items.

Can we implement this by simply making left and top aliases for start, and right and bottom aliases for end? This would be a change in the current documented behavior:

If a value is provided, but the value isn’t honored, the alignment reverts to the default for the direction.

But any code that's setting invalid alignment options is broken anyway.

There's an edge case if the direction is RTL as well (left and start aren't analogs if direction=RTL) - but I think I could live with both of those inconsistencies as long as using the old names raises an appropriate warning on first use.

I haven't had time to get to it yet, but for maximum compatibility, what I envisioned was only storing whichever version was set last — alignment with directions, or align_items with start/center/end. Then if the same one is requested, return that, but if the other is requested, compute it based on the set value and the current row/column direction and RTL/LTR direction.

I think it's possible to make align_items the single source of truth, while keeping full compatibility with the current alignment behavior:

In the alignment setter, check the current direction and text_direction, and transform top, bottom, left or right to start or end according to the current behavior. In particular, this means that setting alignment="bottom" when direction="column" will result in align_items="start", which is "the default for the direction".

In the alignment getter, do the reverse transformation, which is simpler.

As discussed in #1194, there are some bugs in the layout algorithm. For example, the actual default alignment is sometimes start and sometimes what CSS calls stretch, and center doesn't always work. I'm working on fixing this in another branch which I'll stack on top of this one, so please don't make any structural changes to the layout algorithm beyond renaming the attributes.

In the alignment setter, check the current direction and text_direction

I guess this also means that when setting multiple properties at once, either via the constructor or via update, we must make sure to set direction and text_direction before alignment. Code that sets the properties individually may experience a change in behavior, but I think that should be rare enough that we can accept it, especially since we're planning a version number bump to 0.5.0.

But what if existing code sets alignment and then changes direction or text_direction? This is why I think it's safer to just preserve the existing behavior for the old name. And if align_items is requested while alignment is currently an invalid value for the current direction/text_direction, then align_items can read as not having been set.

As discussed in #1194, there are some bugs in the layout algorithm. For example, the actual default alignment is sometimes start and sometimes what CSS calls stretch, and center doesn't always work. I'm working on fixing this in another branch which I'll stack on top of this one, so please don't make any structural changes to the layout algorithm beyond renaming the attributes.

Thanks for the heads up! I was looking at potentially merging the nearly-identical row and column layout methods, either here or in a second PR, but will avoid mucking about with any of that for now.

for maximum compatibility, what I envisioned was only storing whichever version was set last. [...] Then if the same one is requested, return that, but if the other is requested, compute it

It would be more complex that way, but I guess it's the only way to achieve 100% backward compatibility. Either approach is OK with me.

docs/reference/api/containers/box.rst

docs/tutorial/tutorial-0.rst

mhsmith · 2024-12-10T22:23:51Z

I've also taken the liberty of clarifying the name of text alignment whenever I encountered it in my searching-and-replacing.

If we're going to all this trouble, we might as well match the CSS name of text_align rather than text_alignment.

HalfWhitt · 2024-12-10T22:55:06Z

If we're going to all this trouble, we might as well match the CSS name of text_align rather than text_alignment.

The Pack property is already named text_align... My thinking for using "alignment" in a number of other places was that it's a noun, and a thing we control, test, and verify, regardless of what the property that controls it is named. Also "align" can be misleading in a function name; toga_align sounds like it actually aligns something, whereas toga_alignment is more grokkable (to me, at least) as what it is: a converter that returns a Toga-compatible alignment value.

That said, I do realize that's an inconsistency, and I'm not against switching it.

mhsmith · 2024-12-11T13:23:28Z

I agree the grammar is questionable, but I think that's outweighed by the advantage of using the CSS name everywhere. It would be one less thing we need to remember, and it might be useful for some form of automation in the future.

mhsmith · 2024-12-11T14:22:34Z

docs/reference/style/pack.rst

 The amount of space to allocate between the edge of the box, and the edge of the content
 in the box, in :ref:`CSS pixels <css-units>`.


The current wording describes interior padding, which we have never supported, regardless of what the property was called.

Suggested change

The amount of space to allocate between the edge of the box, and the edge of the content

in the box, in :ref:`CSS pixels <css-units>`.

The amount of space to allocate outside the edge of the widget, in :ref:`CSS pixels

<css-units>`.

Only thing I amended here is keeping "box" instead of "widget", to be consistent with how the page discusses geometry.

…values

HalfWhitt

align_items now coexists with alignment. It's a pretty convoluted backwards-compatibility section, but it does seem to work as expected.

I've also made the requested tweaks to the docs, rewritten the part about align_items, and applied a consistent text_align name everywhere.

HalfWhitt · 2024-12-12T03:00:39Z

core/src/toga/style/pack.py

@@ -867,33 +996,19 @@ def __css__(self) -> str:
        if self.height != NONE:
            css.append(f"height: {self.height}px;")

-        # alignment


This CSS had its main and cross axes reversed, but it wasn't being caught because the tests were also flipped. Should be a moot point now.

HalfWhitt · 2024-12-12T03:02:06Z

android/tests_backend/widgets/properties.py

@@ -22,7 +22,7 @@ def toga_color(color_int):
        )


-def toga_alignment(gravity, justification_mode=None):
+def to_toga_text_align(gravity, justification_mode=None):


While changing all instances of text_alignment to text_align, I've also added to_ to the beginning of converters like this, to help avoid the ambiguity of "align" being a verb.

HalfWhitt added 7 commits December 8, 2024 22:40

Changed padding to margin and alignment to align_items

b8d0efb

Added changenote

52ecf6a

Fixes to coverage and testbed

e71b659

More testbed fixes

a919c45

More GTK testbed fixes

7edbccd

Yet more GTK testbed fixes

97f8d6c

Hopefully last GTK fix

e2f3628

HalfWhitt marked this pull request as ready for review December 9, 2024 06:19

freakboy3742 requested changes Dec 9, 2024

View reviewed changes

mhsmith reviewed Dec 11, 2024

View reviewed changes

mhsmith mentioned this pull request Dec 11, 2024

Add gap style #3036

Draft

4 tasks

HalfWhitt added 3 commits December 11, 2024 18:28

Documentation fixes

e84de89

Fixed align_items values, added backwards compatibilty for alignment …

5e1af52

…values

Consistently name text_align everywhere

a185b13

HalfWhitt commented Dec 12, 2024

View reviewed changes

Slightly easier-to-read __getattribute__

097db43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Pack property names to match CSS #3033

Updated Pack property names to match CSS #3033

HalfWhitt commented Dec 9, 2024 •

edited

Loading

freakboy3742 left a comment

freakboy3742 Dec 9, 2024

mhsmith Dec 10, 2024

freakboy3742 Dec 10, 2024

HalfWhitt Dec 10, 2024

mhsmith Dec 11, 2024 •

edited

Loading

mhsmith Dec 11, 2024 •

edited

Loading

HalfWhitt Dec 11, 2024

mhsmith Dec 11, 2024

mhsmith commented Dec 10, 2024

HalfWhitt commented Dec 10, 2024 •

edited

Loading

mhsmith commented Dec 11, 2024 •

edited

Loading

mhsmith Dec 11, 2024

HalfWhitt Dec 12, 2024

HalfWhitt left a comment

HalfWhitt Dec 12, 2024

HalfWhitt Dec 12, 2024

		The amount of space to allocate between the edge of the box, and the edge of the content
		in the box, in :ref:`CSS pixels <css-units>`.

Updated Pack property names to match CSS #3033

Are you sure you want to change the base?

Updated Pack property names to match CSS #3033

Conversation

HalfWhitt commented Dec 9, 2024 • edited Loading

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

mhsmith Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith commented Dec 10, 2024

HalfWhitt commented Dec 10, 2024 • edited Loading

mhsmith commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HalfWhitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HalfWhitt commented Dec 9, 2024 •

edited

Loading

mhsmith Dec 11, 2024 •

edited

Loading

mhsmith Dec 11, 2024 •

edited

Loading

HalfWhitt commented Dec 10, 2024 •

edited

Loading

mhsmith commented Dec 11, 2024 •

edited

Loading