The Cast struct fields were ordered such that `stream` was dropped before
`_listener`. In Rust, struct fields are dropped in declaration order.
Because `StreamListener` attempts to unregister itself from the stream on
drop, and `StreamRc` destroys the underlying PipeWire stream on drop, the
previous order caused `_listener` to access the stream after it had
already been freed.
This reorders the fields so `_listener` is declared before `stream`,
ensuring the listener unregisters itself while the stream is still valid.
* Apply suggestion from @YaLTeR
---------
Co-authored-by: Ivan Molodetskikh <yalterz@gmail.com>
* Improve dinit service files and niri-session
Two main changes were made:
- After a discussion in davmac314/dinit#496, 2 dinit services are now
provided. The first one is 'niri', which runs niri itself, and the
second one is 'niri.target' which brings up all the dependences from
user configuration.
- Made the behaviour of 'niri-session' when running under dinit closer
to the behaviour when running under systemd. In particular, now the
script wait for service completion, because some login managers shut
the session down the moment the startup script completes.
* Update paths in docs
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.
Fixes menu in Telegram. Some weird behavior is still possible e.g. with
gtk4-widget-factory and dropdowns on entries, but things seem to be
slightly less broken this way.
The wording in the deleted comment still stands: Smithay doesn't handle
overlapping grabs. However, in this case things appear to more or less
work themselves out. IME seems to re-request its grab every time an
input field is focused, replacing the popup keyboard grab. And the popup
keyboard grab doesn't seem to mind being replaced this way.
When the column was added immediately to the left of the current column
and activated, the new idx would be equal to active_column_idx, which
would skip activate_column() with its variable resets.
Ivan Molodetskikh