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

Allow style properties to be accessed directly on the widget #3107

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Jan 14, 2025

Closes #3011.

See design discussion starting at #3011 (comment).

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

_MIN_WIDTH = 100
_MIN_HEIGHT = 100

def __init__(
self,
id: str | None = None,
style: StyleT | None = None,
**kwargs,
Copy link
Member Author

@mhsmith mhsmith Jan 14, 2025

Choose a reason for hiding this comment

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

Although this argument is currently only used for styles, I called it kwargs rather than anything style-specific, because at some point I’d like to have a larger cleanup of constructor arguments in which non-style properties could be included. I'll write this up in a separate issue (#3134).



class Widget(Node):
class Widget(Node, PackMixin):
Copy link
Member Author

Choose a reason for hiding this comment

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

I put the style properties into a separate base class to avoid them cluttering the documentation.

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.

This all looks fairly straightforward. I've flagged a couple of testing issues, and pushed some tests to cover those cases.

The only other concern I have is that this binds Toga's widgets to Pack as a style implementation. However, for the sake of practicality, I can live with this. There's no really viable alternative to Pack on the immediate horizon, and with the recent Pack name changes to align with CSS, all the descriptors that have been added here will be valid for Colosseum/CSS users anyway. If other style engines become a realistic possibility, we can always revisit how the descriptors are bound, and the clear separation of mixin from widget means introducing a new approach should be relatively clean.

I'm happy to approve as is; the only other thought I've had is whether it might be worth to not install descriptors for deprecated style properties. There isn't currently any formal annotation of deprecation status that we could use for this purpose... but it shouldn't be too hard to add that sort of annotation. I'll leave it up to you if you think that's worthwhile.

@mhsmith
Copy link
Member Author

mhsmith commented Jan 15, 2025

It looks like the only deprecated property that has a descriptor is alignment, as the padding to margin rename was handled via __getattr__ and __setattr__ methods in the Pack class. Accessing it via Widget will give the same deprecation warning as accessing it via Pack, so I don't think it's worth any extra effort to exclude it completely.

@mhsmith mhsmith merged commit fbedd3f into beeware:main Jan 15, 2025
41 checks passed
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.

Style property redesign
2 participants