Our current rendering code constructs and returns complex
`-> impl Iterator<Item = SomeRenderElement>` types that are collected
into a vector at the top level Niri::render(). This causes some
problems:
- It's hard to write logic around returning iterators. Especially things
like conditions, since the returned iterator must have a single type,
you can't branch and return different iterators. This will be solved
by gen fn but alas it's not here yet.
- In many cases, the returned `-> impl Iterator` will borrow from &self
leading to complex lifetimes. In certain cases, it is also desirable
for it to borrow the &mut NiriRenderer, which causes a lot of issues
because it's exclusive (&mut).
- Sometimes those issues are too hard to deal with, leading to the
escape hatch of allocating and returning a temporary
Vec<SomeRenderElement>, like in
Scrolling/FloatingSpace::render_elements(). These allocations are
unfortunate because they are not really necessary.
- It's impossible to use some downstream combinators with this
`-> impl Iterator` approach, leading to functions like Smithay's
render_elements_from_surface_tree() returning a Vec. This is extra
unfortunate because it results in a temporary allocation per Wayland
toplevel/popup.
- It's hard to properly create profiling spans for the rendering
functions since the spans are dropped when the (lazy) iterator is
returned and not when all the code actually completes.
- The code compiles down to complex state machines in generated iterator
types with logic located in Iterator::next(), which makes it annoying
to follow in debuggers and profiling tools.
This refactor changes the code to push-based iteration: rendering
functions receive a push() closure that they call to push their render
elements. It solves all of the aforementioned problems:
- The logic becomes simpler. Just use conditionals and loops as normal.
- No borrowing and lifetimes since we're not returning anything.
- All temporary Vecs are removed because the problems they worked around
no longer exist.
- The new push_elements_from_surface_tree() helper is the same as
render_elements_from_surface_tree() but doesn't allocate a temporary
Vec since it's not necessary; the push() closure can be passed down.
- Profiling spans work normally since the function returns when it ran
all of the logic.
- The code compiles down to normal functions and calls as expected.
Generally, the iterator approach gives these advantages:
- You can wrap the returned items in the upstream logic. This is
possible in exactly the same way with the push closure.
- You can decide to cut the iterator short in the upstream logic. This
is not possible with push-based iteration, but we don't actually use
it anywhere.
I chose the push closure type to be &mut dyn FnMut(SomeRenderElement).
It's deliberately not a generic impl FnMut() to avoid duplicating the
rendering logic when it's called from several different places. But it's
still a normal closure that can capture the outside context.
While my original idea for this refactor was to simplify the logic while
getting rid of temporary Vecs, it also appears to have brought a
consistent 2-3x speedup to the whole render list construction. On an old
Eee PC laptop I even observed a 8x speedup.
The refactor also results in smaller binary size, presumably due to
removing many iterator combinators and state tracking.
This design makes more sense spatially, and is required for the
Overview. Gaps also make it clear how clipping windows to workspace
bounds works.
Background and bottom layer-shell surfaces get duplicated for each
workspace, while top and overlay stay "on top".
This both avoids sending frame callbacks to surfaces invisible on the offscreen
(fixing Firefox with subsurface compositing in the process), and fixes
searching for split popups during the resize animation.
Leave the Workspace to do the workspace parts, and extract the scrolling parts
into a new file. This is a pre-requisite for things like the floating layer
(which will live in a workspace alongside the scrolling layer).
As part of this huge refactor, I found and fixed at least these issues:
- Wrong horizontal popup unconstraining for a smaller window in an
always-centered column.
- Wrong workspace switch in focus_up_or_right().
most of the time the activation token is passed
while the window is still unmapped. in this case
store the intend to activate the window for
later retrieval on map.
Ivan Molodetskikh