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

Batch application of style properties #3273

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HalfWhitt
Copy link
Contributor

@HalfWhitt HalfWhitt commented Mar 21, 2025

This consists of a few related changes that cut down on the number of calls to Widget.refresh() and Font(). By my count, when running the core test suite, font creation dropped from 110 to 42, and widget refreshes from 129 to 17.

  • BaseStyle gains a batch_apply context manager in which all calls to apply() are aggregated. This is used by:
    • BaseStyle.update()
    • Setting or deleting a directional_property (and eventually composite_property)
    • It doesn't need to be invoked during __init__; there's never an applicator present then, so apply() is a no-op anyway.
  • Apply is split into two methods, an outer apply and an inner _apply.
    • The outer apply is defined in BaseStyle, and handles the logic of checking for an applicator and deciding whether to batch the requested name or call _apply.
    • Pack (and any other style subclass) now overrides the inner one, and does so accepting a set rather than one or more names. The logic then checks for membership of names in this set, rather than iterating through a list. Thus it will never create more than one Font, or call refresh() more than once per call.
      • This makes the biggest difference when the assignment of a style and an applicator triggers a bare apply(), applying all possible style properties.
      • Taking the argument as a set means that the stored _batched_names set doesn't need to be expanded into multiple positional arguments and then recreated as a set within the method.
  • While setting up the above, I noticed that validated_property always calls apply on delete, even if the value being deleted is equal to the initial value, so I added a check there (and tested it).

I also combined/parametrized the existing tests for:

  • test_property_with_explicit_const
  • test_property_with_explicit_value
  • test_property_with_explicit_none
  • test_property_with_implicit_default

I realize this changes the API of Travertino, in terms of what its subclass is supposed to override — but that's the only change. (Except that apply() goes back to accepting only a single name, but nothing was written yet that called it with multiple.) And it's still backwards compatible with Toga < 0.5, because existing Toga defines its own Pack.apply() method and will completely ignore this; styles that it uses will simply never enter batched mode.

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 HalfWhitt marked this pull request as ready for review March 21, 2025 06:02
@HalfWhitt
Copy link
Contributor Author

I also wasn't sure if this would count as a feature or should just be misc.

@HalfWhitt HalfWhitt requested a review from freakboy3742 March 25, 2025 19:45
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.

1 participant