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

Implement window refresh lock #2438

Closed
wants to merge 3 commits into from

Conversation

cvwillegen
Copy link
Contributor

Added functionality to stop window content from refreshing when a lot of boxes are added programmatically to a window, because refresh would kick in after every add, leading to a quadratic increase of refresh() calls.

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.

Thanks for the PR. I definitely acknowledge that there is an opportunity to optimise here, so thanks for taking this on. I have one minor suggestion, and two design questions about the specifics you're presented here.

Firstly, the suggestion: the test case you've provided exercises the simple case of changing a window's content, but doesn't validate the more complex case of changing the child of a window (which I would have thought would be the more likely case in practice), or the case of refreshes on content that hasn't been attached to a window yet. I suspect that both of these aren't actually issues, but we should verify that in the test, since it's the core of the thing we're trying to optimise.

The design questions are larger scale issues.

Firstly, is this something we could optimise generally, rather than requiring an explicit lock? Is it something we could use as a decorator in Toga's code to remove the need for a user to apply the lock except in extreme situations? After all, the best API is one you don't need to use at all.

One area to explore here is that layouts can only be altered in user-space code, and there are constraints on when user-space code can be entered - it's only ever going to be as part of a method like startup(), or in an event handler. With that in mind: could we make this a universal optimisation that doesn't require the user to explicitly invoke it? There's some complications around async handlers (i.e., if I await in the middle of a long running handler, when does the refresh take effect?) but I think it might be worth exploring how this could be implemented without needing an explicit public API.

Secondly, this approach optimises the call to refresh() at the core level, which is definitely a computationally expensive part of the process - but are there other parts of the refresh process that could/should be optimised? Does this approach work on all backends? The particular source of concern here is GTK. The other backends essentially update "live" - if you add a widget to a tree, the layout is recomputed, and the widget is added or moves as appropriate. However, GTK's layout process is slightly different - making a layout change marks the container as "dirty", and the refresh doesn't happen until GTK decides to do a repaint addressing the "dirty" status. It's not immediately clear to me that this approach will work with GTK's approach, especially in the context of async handlers where control is released to the event loop, potentially for a long time, and a lock might be in effect.

At the very least, I suspect there's a need for a testbed test that validates the number of refresh calls that occur when a lock is imposed.

@freakboy3742
Copy link
Member

Closing due to lack of response. If you intend to continue this work, let us know an we can re-open the PR.

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