Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style property redesign #3011

Closed
mhsmith opened this issue Dec 2, 2024 · 11 comments · Fixed by #3107
Closed

Style property redesign #3011

mhsmith opened this issue Dec 2, 2024 · 11 comments · Fixed by #3107
Labels
enhancement New features, or improvements to existing features.

Comments

@mhsmith
Copy link
Member

mhsmith commented Dec 2, 2024

Toga currently supports two kinds of property on a widget:

  • Properties set directly on a widget, controlling its data and behavior
  • Properties of the widget's style object, controlling its appearance, position and size

There are a few problems with this arrangement:

  • There's no documentation of which styles are applicable to which widgets.
    • There's no runtime validation either – inapplicable styles are silently ignored with no warnings or errors, so the developer doesn't know what they've done wrong.
    • Because of this, any interactive GUI builder would need to offer all styles on all widgets, cluttering the UI.
  • The set of style properties is centralized and fixed. There's no way for a third-party widget to declare styles applicable to itself, or for a third-party container to declare styles that are applicable to its children.

I'd like to discuss switching to the design used by Invent, which instead divides properties according to who's responsible for defining and implementing them:

  • Properties set directly on a widget, controlling its data, behavior and appearance – defined by the widget's own class
  • Properties of the widget's layout object, controlling its position and size – defined by the class of the widget's parent

This allows both sets of properties to be extensible, and even if the widgets and containers come from different developers, there's no risk of name clashes.

Implementation details

The first group of properties are implemented directly on the widget's class, or inherited from a base class. For example, this would include background_color and font_family. Mixin classes could be used to add multiple properties at once, such as all the font properties.

The layout group is more complex because it depends on the widget's parent. Invent implements this as follows:

  • Every container widget has a corresponding Layout class which defines the layout properties applicable to the container's children. So for the Toga Box, there would be a BoxLayout class with properties width, height, padding_... and flex. However, direction and alignment are set on the container itself, so they would be ordinary properties defined on Box.

  • Container classes are linked to their corresponding layout class by a layout_class attribute, i.e. Box.layout_class is BoxLayout.

    • To avoid any need to import the layout class separately, I initially tried making it a nested class, e.g. Box.Layout. The problem with this is that error messages only display the inner class name, so if you saw "Layout object has no attribute x", you wouldn't know what Layout class you were dealing with. Maybe we could get the best of both worlds by renaming layout_class to Layout.
  • Layout classes all have the constructor signature (widget=None, **kwargs), where:

    • widget is the widget the layout properties will apply to, i.e. the child.
    • kwargs are initial values of properties.
  • Layout classes can be instantiated directly, but for conciseness a dict is also accepted by the the widget constructor. So both of the following are equivalent:

    toga.Label(layout=BoxLayout(flex=1))
    toga.Label(layout=dict(flex=1))
    
  • A widget's constructor doesn't know what its container will be, so the validation of the layout property is deferred until the widget has a parent. At that time, a new Layout object of the correct type is created and associated with the widget, and intialized from whatever was passed into the constructor:

    • If the constructor received a Layout object of the correct type, its properties are copied into the new object.
    • If the constructor received a dict, its keys are copied into the new object. Unknown properties will raise an error.
    • Otherwise, raise an error.
  • If a widget is assigned to a different parent, or the layout property is itself assigned, the same validation takes place. This allows a widget to be moved to another container of the same class with its layout properties intact. Moving to a container of a different class currently requires an awkward dance, so in this case maybe the layout properties should be reset to the new container's defaults.

Backward compatibility

The existing style syntax can be kept as a deprecated alternative. It maps onto the new system in a 1-1 manner, with each style property corresponding to either a direct widget property or a layout property.

However, the new syntax would be a good opportunity to rename the properties to more closely match CSS, e.g. padding to margin, and alignment to align_items. This will help developers who are familiar with CSS, and even if they're not, it will allow them to search for the terms in general CSS documentation such as MDN.

@mhsmith mhsmith added the enhancement New features, or improvements to existing features. label Dec 2, 2024
@freakboy3742
Copy link
Member

There are a few problems with this arrangement:

  • There's no documentation of which styles are applicable to which widgets.

    • There's no runtime validation either – inapplicable styles are silently ignored with no warnings or errors, so the developer doesn't know what they've done wrong.
    • Because of this, any interactive GUI builder would need to offer all styles on all widgets, cluttering the UI.

This is definitely true of the current Toga implementation - but I don't agree that it's fundamentally unresolvable. It strikes me as a gap of metadata, not a limitation of Toga's existing design.

The manual solution is to annotate the "applicable" styles on each class. This doesn't strike me as an especially onerous task to add - certainly a lot less invasive than a full redesign of Toga's style implementation.

However, my initial impression is that all this metadata could be largely (if not entirely) determined by inspection Applicator implementations - if a widget doesn't declare an implementation of the set_font Applicator interface, you can infer that it won't honor font_face, font_size, etc.

It's also worth nothing that CSS and HTML doesn't contain any runtime validation either - and that hasn't stopped GUI HTML/CSS builders from having widget-specific UIs in their design interface. Whether this is a strength or a weakness of the design of HTML and CSS is a debatable topic - but whatever the flaws, the HTML and CSS approach is undeniably successful, and I'd argue it's the single most prevalent UI API design in use today.

  • The set of style properties is centralized and fixed. There's no way for a third-party widget to declare styles applicable to itself, or for a third-party container to declare styles that are applicable to its children.

This is true - but I'm also not clear on why it's a problem. Can you provide an example of a style property that we would require extension in this form but couldn't be satisfied by a existing CSS3-compliant style property?

The only examples I can think of relate to the styling of "complex" widgets like table, where you may want to (for example) set the text color of a particular column, or similar (see #1478) - but in that case, I'd argue the issue isn't that we need new style properties - it's that we don't have the ability to apply the existing style properties to "sub elements" of a large widget.

I'd like to discuss switching to the design used by Invent, which instead divides properties according to who's responsible for defining and implementing them:

  • Properties set directly on a widget, controlling its data, behavior and appearance – defined by the widget's own class
  • Properties of the widget's layout object, controlling its position and size – defined by the class of the widget's parent

This allows both sets of properties to be extensible, and even if the widgets and containers come from different developers, there's no risk of name clashes.

My fundamental objection here is that I'm not sure I agree styling properties should be extensible - or, at least, if we make them extensible, we need to be careful about that extension mechanism.

The exiting style property makes a very clear distinction between the function and appearance of a widget. A Toga app with no style definitions will be 100% functionally identical to the same app with any random collection of style properties. This is, to my mind, a strength of the Toga design - and one that has been deliberately borrowed from the philosophical underpinning of HTML and CSS - that the markup defines the content, and the stylesheet defines the appearance.

It's also consistent with the long term goal of Toga to replace Pack with Colosseum - a true CSS implementation, with everything that entails. Ultimately, it should be possible to have a "mobile optimised app" in the same way that you have a "mobile optimised website" - the same widgets, but with a different stylesheet (or a single stylesheet with media specifiers) that accounts for the size and properties of the device where it is being used. The same could also be done for "dark mode" stylesheets.

On top of that - CSS doesn't require arbitrary extension of style properties to render websites of almost arbitrary complexity... so why is it needed for Toga? What is the use case?

It's also worth noting that with some a couple of very specific exceptions (mostly in the ImageView class - and I consider these exceptions to be bugs), a user could provide their own style class right now, with no impact on the operation of Toga. In practice, I don't consider that this is something most users will do - but the API separation is at the core of how a transition to Colosseum would work in future - replace any instance of style=Pack(...) with style=CSS(...) and the two should be able to co-exist.

Implementation details

I feel like I'm missing something, because I see very little here that would be a net gain for Toga, and a lot that we would lose.

  • It removes the form/function separation that currently exists
  • It would require a massive redesign of the implementation of styles and widgets
  • It would be a huge code churn for existing example code
  • It binds us to one specific interpretation of style

I agree that Toga (as currently implemented) has a metadata gap that would make it difficult to automatically generate GUI tooling. However, to me, the solution to this problem is to plug the metadata gap, not to completely redesign Toga.

The only other benefit that I can see is that this design is (presumably) more closely aligned with Invent's API design, which might simplify writing a "Invent on Toga". However, if API adaptation is required, it's not clear to me why Toga's API should be changing to map to Invent's design, rather than the other way around - especially given the fact that (a) Toga's design mirrors explicitly the design of an established standard in HTML and CSS, and (b) Toga has (to the best of my knowledge) a bigger pre-existing user base, so the "principle of least harm" would dictate Toga's API is the one that is preserved.

If there is some pedagogical reason why it's preferable for Invent to define background_color on the API of a widget rather than a style, then it's also not clear to me why Invent's API can't map a keyword argument/property setter to an underlying Toga implementation that is defined on a stylesheet - or, circumventing the Toga Core API entirely, write against the toga-impl backend API.

@HalfWhitt
Copy link
Contributor

My two cents are with Russel on this one, for what they're worth. However...

However, the new syntax would be a good opportunity to rename the properties to more closely match CSS, e.g. padding to margin, and alignment to align_items. This will help developers who are familiar with CSS, and even if they're not, it will allow them to search for the terms in general CSS documentation such as MDN.

I somehow hadn't even noticed these, but yeah, it seems like it would be good to rename them to match. They can be aliased for backwards compatibility.

@martinklein

This comment was marked as off-topic.

@HalfWhitt

This comment was marked as off-topic.

@martinklein

This comment was marked as off-topic.

@HalfWhitt

This comment was marked as off-topic.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 13, 2025

There's no documentation of which styles are applicable to which widgets. There's no runtime validation either.

This [...] strikes me as a gap of metadata, not a limitation of Toga's existing design. The manual solution is to annotate the "applicable" styles on each class. This doesn't strike me as an especially onerous task to add - certainly a lot less invasive than a full redesign of Toga's style implementation.

However, my initial impression is that all this metadata could be largely (if not entirely) determined by inspection Applicator implementations - if a widget doesn't declare an implementation of the set_font Applicator interface, you can infer that it won't honor font_face, font_size, etc.

That's a good point. Let's come back to it later once we've resolved the larger issues.

The set of style properties is centralized and fixed. There's no way for a third-party widget to declare styles applicable to itself, or for a third-party container to declare styles that are applicable to its children.

This is true - but I'm also not clear on why it's a problem. Can you provide an example of a style property that we would require extension in this form but couldn't be satisfied by a existing CSS3-compliant style property?

CSS certainly has everything we need, but we don't support all of CSS, and CSS itself is constantly changing. For example, if you wanted to implement a a third-party Grid container, even one that was only supported on web platforms via CSS generation, then you'd need to create a new style class to define the grid-related styles, and apps would have to use that class within grids instead of the Pack class.

But maybe it's premature to worry about this now. For the moment, the only third-party widgets that are likely to need this are those from Invent, and we're working closely enough with them that we can add more CSS properties to Pack as necessary, even if Toga itself doesn't use them.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 13, 2025

If there is some pedagogical reason why it's preferable for Invent to define background_color on the API of a widget rather than a style

After discussing this with @ntoll and @freakboy3742, we agreed that we should allow style attributes to be set directly on the widget, as this not only saves beginners from needing to understand the style/content distinction, but also helps developers of all levels by significantly reducing the amount of typing required to define a layout.

In fact, in invent-framework/invent#130, @ntoll has already removed the separate layout namespace described above.

For Toga, the necessary changes to support this would be:

  • Add a **kwargs argument to every widget, which will be passed through to the base Widget class and used to set style properties.
  • Automatically generate descriptors on the base Widget class for each style property, so they can be accessed using dotted syntax.

This is much less disruptive than what I previously proposed, as it doesn't make any major structural changes, just adds a simpler syntax on top of the existing one. The existing style constructor argument and namespace would not be deprecated, and could still be used for advanced purposes such as copying an entire set of styles from one widget to another.

@HalfWhitt
Copy link
Contributor

  • Automatically generate descriptors on the base Widget class for each style property, so they can be accessed using dotted syntax.

Would this best be done with auto-generated descriptors, or with a custom setattr/getattr that checks against what's supported by the widget's associated style? There might be a performance difference, but it should still be pretty fast since it's checking membership in a set. And style properties aren't usually being set as rapidly as something like layout calculations.

The main advantage I'm thinking of is that were we to get to a point where Pack is the default, but CSS or Grid or something else is a fledgling but opt-in alternative, the user's selection of engine wouldn't have to be baked in when the Widget class is initialized. Granted, that's hypothetical at this point, but if we can accommodate it now without harming anything else, that'd be a bonus.

@freakboy3742
Copy link
Member

  • Automatically generate descriptors on the base Widget class for each style property, so they can be accessed using dotted syntax.

Would this best be done with auto-generated descriptors, or with a custom setattr/getattr that checks against what's supported by the widget's associated style?

Agreed that a setattr/getattr based implementation should be simpler to implement; but there's a second consideration - automated code assistance tools. A setattr/getattr solution wouldn't be able to provide meaningful hints to Pylance et al. And part of the "et al" would be a GUI app designer tool that needs to know what style properties it can set.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 14, 2025

Agreed that a setattr/getattr based implementation should be simpler to implement

I thought so too, but when I started implementing it, I found that wasn't the case. Because of the "intentional asymmetry between __getattr__() and __setattr__()", a custom __setattr__ method is always called when an attribute is set. It therefore has to walk the class hierarchy to see whether a regular property exists, then check for a style property, and finally assign to self.__dict__. If we use descriptors, we get this behavior automatically.

were we to get to a point where Pack is the default, but CSS or Grid or something else is a fledgling but opt-in alternative, the user's selection of engine wouldn't have to be baked in when the Widget class is initialized

Descriptors can be changed dynamically by modifying the class __dict__, so if this ever becomes an issue, we can always add a method to switch the default style class and generate a new set of descriptors. For now, alternative style classes can still be opted into via the style syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants