Rebuild widget tree only after an application update#597
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves the implementation of the event loop of an
Applicationiniced_winitandiced_glutinto callApplication::viewonly when strictly necessary.Rust and The Elm Architecture
As you may know,
icedis heavily inspired by Elm and its architecture.In short, The Elm Architecture consists in initializing an application, obtaining its widget tree, and using it to process user interactions in a loop.
The widget tree may produce messages after a user interaction. In that case, the application reacts to these messages and changes state accordingly. When the application changes state, the widget tree needs to be reconstructed in order to stay up-to-date.
We can express this in a few lines of Rust:
This seems simple enough! And still,
iceddoes not currently implement its event loop like this. Instead,icedrebuilds the widget tree on every iteration, right before processing user interactions. Let me explain why.Widgets and mutable references
The
Applicationtrait inicedis defined as follows:In other words,
icedapplications produce widget trees that may contain mutable references to different parts of the application state.As a consequence, after calling
Application::view, Rust's borrow checker will forbid any further references to the application until the resulting widget tree is dropped.But this should be fine! In our event loop, we only reference the application again when an update is necessary. And in that case, we are rebuilding the widget tree afterwards anyways!
Additionally, since non-lexical lifetimes landed, the borrow checker should be smart enough to deal with the borrows in the previous event loop.
So, what's the problem? Basically,
Elementrepresents aBox<dyn Widget>and, therefore, needs to free memory when dropped. Thus, if we tried to implement the previous event loop as-is, the compiler would complain:Dropping widgets manually
Once I first hit the previous error, I assumed it would take some unsafe code to please the borrow checker. So, for the time being, I worked around it by rebuilding the widget tree at every iteration.
Earlier this week, I was playing with the idea of a lazy widget to tackle the problem and I brought up this issue in the Zulip server. Eventually, we realized that we could leverage
ManuallyDropto aid the compiler and fix the issue (thanks @twitchyliquid64!).Specifically, we can use
ManuallyDropto inhibit the compiler from automatically calling the destructor of the widget tree. This requires no unsafe code since potentially leaking memory is safe in Rust:Great! This event loop compiles. That should be it, right? No, not so fast!
iced_winitcannot use aloopto implement the event loop!Closures and lifetimes
For very acceptable reasons,
EventLoop::runinwinittakes control of the calling thread and asks for a closure which is run on every iteration.This is equivalent to replacing our previous
loopwith a closure:Unfortunately, this no longer compiles. Rust is not able to properly desugar the closure and keep track of the appropriate (non-lexical) lifetimes.
We were so close! All would be good if we could somehow turn the closure calls into a loop of events...
Futures as closures
Futures are data structures that hold both state and execution progress. Similar to how closures can be called, futures can be polled to resume their execution. Could we maybe leverage them to turn a closure into a loop of calls?
Yes! We can rewrite our event loop using an
mpsc::Receiverandawaitsyntax:Then, we wire it together with the closure:
And that's it! This is the strategy that this PR uses to leverage the borrow checker to avoid calling
Application::viewunnecessarily without any unsafe code.The main issue with this approach is that we are breaking the
Futurecontract, asrun_applicationblocks very often (when drawing, for instance). However, thisFutureis an implementation detail, it's not exposed publicly, and we are manually polling it. We may be able to get rid of it once closures can handle non-lexical lifetimes.In any case, if anyone is aware of a better way to do this, please let me know!
Fixes #579.