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

Added composite property (and some nearby cleanups) #222

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

HalfWhitt
Copy link
Contributor

@HalfWhitt HalfWhitt commented Oct 15, 2024

I do, in fact, still exist! Sorry to disappear so abruptly in the middle of working on this.

Here's composite_property, the next piece of the puzzle after list_property in #148. It's able to incorporate a list_property as a constituent aliased property, and with reset_value set to NORMAL, can provide the basis for the font shorthand property (and background, much more simply). Some shared functionality with directional_property has been factored out into a property_alias base class.

I've also fleshed out ImmutableString's dunders and typing a little (with tests), and tidied up some other things in the file. (I think those are mostly self-explanatory, except perhaps the cut down code in validated_property.__init__. I think the extra catch was only there initially to provide different error wording when name wasn't set yet. With the existence of _name_if_set, that's no longer necessary.)

I have a question about future direction. Is the idea for this to eventually be the base layer for something in either Travertino or Colosseum that can parse a CSS-style text declaration into its constituent parts and pass it down as a properly portioned-out list, effectly splitting what travertino.fonts.font does into layers? I'm not sure how cleanly the two layers could ever be separated, since as far as I can tell, validation against individual fields will have to be done in the first place as part of figuring out how to properly split up the string.

(Also, based on the specs, I don't think the existing font function works correctly, since it enforces order of optional values that are set to normal. It accepts "italic normal" but chokes on "normal italic", for instance.)

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

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly speaking, this looks like it's headed in the right direction. I've flagged some small things inline; my bigger questions are mostly around the edge cases.

In particular, the example property doesn't seem rich enough to expose the edge cases where this could fall down - for example, having more than 2 optional properties, where a provided value is allowed for more than 1 of those properties (I'm thinking here of NORMAL when used for font-size, font-weight, and font-variant. I think the implementation here will likely work - but it's not validated that it will work.

It's also not clear to me how a reset value will be interpreted for more complex example (especially one that provides a default); and the test case only tests the "reset value provided" case; there's no example of a "not provided" reset value.

@@ -209,7 +209,22 @@ def validate(self, value):
return ImmutableList(result)


class directional_property:
class property_alias:
"""A base class for list / composite properties. Not designed to be instantiated."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion, this should be marked as an ABC, with any property requirements either documented, or defines as @abstractproperty

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!



class composite_property(property_alias):
def __init__(self, optional, required, reset_value=NOT_PROVIDED):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a reset value differ from (and interact with) a default value on the underlying property?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That... is a very good point. In my first pass the "parser" checked explicitly for NORMAL, and I think I generalized in the wrong direction. Of course in the real font property, that will just be the default values of the three optional properties, which I can check programmatically.

It's an unusual exercise, generalizing from something there only two specific (and pretty different) instances of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the reset parameter and overhauled the assignment of optional properties; more below.

@freakboy3742
Copy link
Member

I have a question about future direction. Is the idea for this to eventually be the base layer for something in either Travertino or Colosseum that can parse a CSS-style text declaration into its constituent parts and pass it down as a properly portioned-out list, effectly splitting what travertino.fonts.font does into layers? I'm not sure how cleanly the two layers could ever be separated, since as far as I can tell, validation against individual fields will have to be done in the first place as part of figuring out how to properly split up the string.

This is indeed the intention. Font() is a bit of an edge case; while there is an explicit parser, that isn't actually used anywhere that I can think of. It should be handled as a composite property, and the font() parser deprecated.

To that end - one thing I'd like to see around the design of CompositeProperty is a proof-by-usage that it will be sufficient for CSS purposes. A handful of examples of "real" declarations would be very helpful. What does a "real" Font, Background etc declaration look like? Are there any other interesting cases that we've missed?

(Also, based on the specs, I don't think the existing font function works correctly, since it enforces order of optional values that are set to normal. It accepts "italic normal" but chokes on "normal italic", for instance.)

That seems entirely possible.

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Oct 16, 2024

Font() is a bit of an edge case; while there is an explicit parser, that isn't actually used anywhere that I can think of. It should be handled as a composite property, and the font() parser deprecated.

Gotcha. Yeah, the font parser isn't used anywhere except in the tests for it. I can add a deprecation warning to it.

To that end - one thing I'd like to see around the design of CompositeProperty is a proof-by-usage that it will be sufficient for CSS purposes. A handful of examples of "real" declarations would be very helpful. What does a "real" Font, Background etc declaration look like? Are there any other interesting cases that we've missed?

That'll be the next thing I put in the API update for Toga, although yes, I'll also expand the tests here around the factors you've mentioned. Are there any CSS shorthands besides font and background this would be used for? I believe all the others are directional — let me know if you know of one I'm overlooking.

@HalfWhitt
Copy link
Contributor Author

That is to say, I'll implement and test font in the Toga API update, but I can make a background test here. Unless you're planning to add support for widget backgrounds tiled with animated gifs? Would really help for a Geocities app I'm working on. 😉

@HalfWhitt
Copy link
Contributor Author

HalfWhitt commented Oct 16, 2024

I'm going to fully rework Travertino's tests of this — that's not done yet. And I'm going to test the actual font property from Toga once I can get past whatever's going on in beeware/toga#2916. So this code is completely untested, but I wanted to commit its initial (quite possibly buggy) version and document my thoughts on the assignment logic of optional properties.

Font's a very special case in that each of its optional properties have non-overlapping possible values, except for their defaults, which are all the same. So if you're only handling font, the algorithm can be pretty simple: you can completely disregard any "normal"s, and then assign anything that remains to the only place each will fit, resetting the rest. (Assuming the values you've been given are valid, of course.)

To generalize it across different defaults, overlapping non-defaults, and any combination, I'm taking the approach of first seeing, for each value, which properties could accept it. Then, assign them in order of specificity: that is, take the value that could apply to the fewest properties first, and assign it to the earliest of its possible properties. Then for each next value, assign it to its earliest possible property that hasn't yet been assigned. (If there any ties in specificity, they're assigned in the order supplied.) Anything left out is still unset / set to its default, of course.

I think this should correctly specify to font and background, but obviously the proof (or disproof) will be in the testing pudding.

(Oh, and I also added a parse_str parameter for interpreting string assignment, which by default just splits on whitespace, but which can be replaced by a slightly smarter splitter for font.)

@freakboy3742
Copy link
Member

That'll be the next thing I put in the API update for Toga, although yes, I'll also expand the tests here around the factors you've mentioned. Are there any CSS shorthands besides font and background this would be used for? I believe all the others are directional — let me know if you know of one I'm overlooking.

font and background are the only ones that Pack uses, but CSS has lots of others - the MDN docs list them all.

Worthy of note - they can even be multi-level: CSS3 Grid grid contains grid-template, grid-template contains grid-colums, grid-rows and grid-areas (although the formal specification is a bit complex; it also allows grid-template-rows and grid-template-columns, which are also targeted by grid-template).

@freakboy3742
Copy link
Member

That is to say, I'll implement and test font in the Toga API update, but I can make a background test here. Unless you're planning to add support for widget backgrounds tiled with animated gifs? Would really help for a Geocities app I'm working on. 😉

Darn - you've accurately predicted the project that was going to be top of the Q1 roadmap... 🤣

Font is the immediate source of concern. While Pack supports background, we're not pushing it very hard, so I don't think we actually need the composite there.

@HalfWhitt
Copy link
Contributor Author

Worthy of note - they can even be multi-level: CSS3 Grid grid contains grid-template, grid-template contains grid-colums, grid-rows and grid-areas (although the formal specification is a bit complex; it also allows grid-template-rows and grid-template-columns, which are also targeted by grid-template).

Now I know how Lovecraft protagonists feel when they glimpse the impossible geometry.

While Pack supports background, we're not pushing it very hard, so I don't think we actually need the composite there.

It only supports background color though, right? Are there plans to add attachment, clip, image, origin, position, and/or size?

@freakboy3742
Copy link
Member

While Pack supports background, we're not pushing it very hard, so I don't think we actually need the composite there.

It only supports background color though, right? Are there plans to add attachment, clip, image, origin, position, and/or size?

In Pack? Highly unlikely. That set of features is very much in the "what you need is CSS" bucket, and I'm deliberately avoiding making Pack a fully-featured replacement for CSS so that I don't take the wind out of any efforts to advance Colosseum.

The only notable feature I foresee adding to Pack in the near future is a really bare bones implementation of Grid layout (because grid-based widget layouts is a really common use case that we're going to need for any sort of generic form/settings API) - but beyond that, I'd rather any effort into layout mechanisms be directed at Colosseum.

@HalfWhitt HalfWhitt marked this pull request as draft October 18, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants