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

Restructure widget initialization #2937

Open
HalfWhitt opened this issue Oct 30, 2024 · 4 comments · May be fixed by #2942
Open

Restructure widget initialization #2937

HalfWhitt opened this issue Oct 30, 2024 · 4 comments · May be fixed by #2942
Labels
enhancement New features, or improvements to existing features.

Comments

@HalfWhitt
Copy link
Contributor

What is the problem or limitation you are having?

The order in which widgets currently initialize means they try (and fail) to apply their styles before their implementations are available. The style isn't actually applied until the end of the constructor for the implementation, meaning it's repeated in every single implementation class. The creation and assignment of the implementation also has common logic that can be pushed from the individual widget subclasses into the base.

Describe the solution you'd like

Rearranging things as described in beeware/travertino#224 (comment)

Describe alternatives you've considered

Do nothing. Everything currently works as-is, it's just not as sensible and efficient as it could be.

Additional context

Originally stemming from the mysterious errors I saw in #2916 (which had previously been there, but were always suppressed until I made a change in Travertino), I went down the rabbit hole. The Travertino half of the restructuring is in beeware/travertino#224, which is backwards compatible with the current development and stable versions of Toga.

I plan to tackle this, but I think I'd like to finish Travertino's composite property first, so I thought it'd be a good idea to post this as an issue for now.

@HalfWhitt HalfWhitt added the enhancement New features, or improvements to existing features. label Oct 30, 2024
@mhsmith
Copy link
Member

mhsmith commented Oct 31, 2024

I haven't been following this discussion too closely, but is it possiible this could be fixed in a way that also fixes the "redundant rendering pass" issue mentioned #1794 (comment)? Except for Winforms, none of the native platforms make any changes visible until the next main loop iteration. So if we delay actually applying the styles until the end of the current iteration, then:

  • Multiple assignments to the same style property within a single main loop iteration would only have the final value passed to the native widget.
  • More importantly, multiple changes to style or content within a single main loop iteration would only require a single layout pass. Right now, if you insert N items into the layout, or you have N labels and you change the font or text on all of them, then we'll perform N layout passes, which will cost O(N ** 2) time.

@HalfWhitt
Copy link
Contributor Author

Hm. My off-the-cuff guess is that fixing that would better be done within the apply and/or refresh methods. Perhaps instead of performing their work immediately, calls could place themselves as a task in a one-slot queue of sorts, or staging area, and at a defined point the end of the event loop, only the most recent request is submitted to the interface.

However, I haven't even read those methods yet and I'm currently on my phone, so who knows!

@freakboy3742
Copy link
Member

Yeah - my inclination is that this is a separate problem to what @mhsmith is describing. The immediate issue this ticket is describing is one about the instantiation of the widget and the assignment of the applicator - something that is highlighted by @HalfWhitt's; recent changes in Travertino. The issue @mhsmith is describing is an ongoing issue, where each individual style assignment causes a recompute. Batching/suspending updates on an ongoing basis is a much more complex issue, and it's not immediately clear to me whether this is an issue on the Travertino side or the Toga side (or both).

@mhsmith
Copy link
Member

mhsmith commented Nov 1, 2024

OK, then I'll create a separate issue for that – #2938.

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.

3 participants