The current value of 200ms is too long and makes river feel very slow
when it it hit due to an application being very slow/frozen.
The new timeout of 50ms seems to be rarely hit in practice even on
slower hardware such as my old Thinkpad x220. When it is hit the system
feels much more responsive than when the 200ms timeout is hitdespite the
guilty application window potentially being visible in the wrong
location/size a bit longer.
These are by default only enabled for debug builds but give a higher
chance of getting a usable stack trace out of bug reports as Zig's
builtin stack trace dumping code doesn't handle their absence well in
all cases yet. The cost should be negligible as river is not CPU-bound.
I assume that I'm not the only one who has changed/will change their
email over the years of river development, so this seems like a
reasonable thing to have.
It seems like the wlr_scene implementation of sending per-surface
feedback is a bit too spammy and can lead to resource exhaustion in
clients in at least some reported cases.
Outputs using the wlr_output_layout auto-layout feature may have their
coordinates update any time an output is added/removed to the layout or
the position of another output in the layout is set.
River currently doesn't keep the scene node coordinates of such outputs
in sync with their position in the output layout, which leads to bugs
with e.g. the cursor not working properly for such outputs.
Currently we update output-management clients based on changes in the
wlr_output_layout struct. However, this is obviously wrong on closer
inspection due to the fact that not all outputs are tracked by the
wlr_output_layout at all times. I think this aproach was originally
cargo-culted from some other output-management implementation and it
needs to go.
Luckily, the solution is quite simple since the only way to configure
outputs using river is through the wlr-output-management protocol. This
means we need to send a new configuration to the output-management
client in 3 cases:
1. An output is created
2. An output is destroyed
3. A wlr-output-management config from a client is applied
Unfortunately, clients in the wild sometimes get the
configure/ack_configure/commit sequence wrong, committing the configured
surface with a size different than the one they ack'd only to later in
separate commit adpat the surface size to that requested by the
compositor.
Improve river's handling of such buggy clients to keep river's ssd
borders and internal state consistent with whatever the clients actually
commit regardless of the correctness of the clients. Log a shame message
for such clients to make me feel better about working around their bugs.
If a view that is currently being destroyed is matched by a newly added
rule river crashes due to an assertion failure.
Fix this and add another assertion to make this precondition more
visible to the users of RuleList.match().
If there is a transaction inflight when an output is disabled then we
must call View.commitTransaction() for any views evacuated from the
disabled output to ensure transaction state remains consistent.
Root.commitTransaction() will not call View.commitTransaction()
for these evacuated views as their output is no longer around.
If a new transaction is started that calls configure() on an xdg
toplevel with a timed_out/timed_out_acked configure_state we currently
leave the configure_state untouched if no new configure is needed.
This can cause an assertion failure in View.commitTransaction() if the
xdg toplevel still hasn't acked/committed by the time the new
transaction completes.
To fix this, update the configure_state as necessary.
All of these API calls checking if the device supports a given option
and checking if the value would be changed are effectively useless.
A quick peek inside the libinput source code shows us that all of these
"setter" functions validate their arguments and return an error if they
are invalid.
Since we don't do anything with the information of whether or not a
config option has been changed or if a config option is even supported
for a given device, all these apply() functions can be simplified to a
single libinput function call.
wlroots implements this behavior with its key press tracking but
continues to forward the events to the compositor. Matching the wlroots
behavior here seems like the best way to avoid strange edge cases and
this is unlikely to ever be an annoying limit in practice.
Also take this oppurtunity to finally refactor away the hasMapping()
function in a way that doesn't sacrifice correctness even when hitting
this 32 key press limit.
It didn't occur to me that this is a completely valid case when a key
release event is received even though there was no press event known
to river.
(I've also just had this same crash on something else, but i don't
understand what the cause could be.)
The old eat/pass-on point of view was good when there was only the
focused client to send the key to. But where does the input method
stand? Instead, we now want to know where the key goes, treating river
and clients all equally.
Thanks to ifreund for pointing me to std.BoundedArray which simplifies
some of the logic.
Currently configure timeouts hit the "client is buggy and initiated size
change while tiled or fullscreen" code path even if the client is not in
fact buggy. This causes state to get out of sync between river and the
client, and is highly visible as the borders drawn by river no longer
align with the buffer dimensions committed by the client.
This commit fixes this by tracking acks/commits in response to
configures even after a timeout and properly integrating them with the
transaction system.
There are some cases in which a view can end up with a size/position
that places some part of it outside its output. For example, a window
with a large minimum size in a tiled layout that is placed near the
right or bottom edge of the output may extend past the output bounds.
The problem with this is that part of the view outside the output bounds
may be rendered on another output where it does not belong in a multi-
monitor setup.
To fix this, clip the surfaces of the view and the borders to the output
bounds.
Focus may not actually change here so seat.focus() may not automatically
warp the cursor. Nevertheless, a cursor warp seems to be what users
expect with `set-cursor-warp on-focus` configured, especially in
combination with focus-follows-cursor.
To see why this is needed, compare the following flows:
- user: press key 'j' with Super already pressed
- Keyboard: handle mapping, focusing next view
- Seat: send wl_keyboard.enter with keys Super and 'j'
- Keyboard: eat key 'j'
versus:
- user: press key 'j' with Super already pressed
- Keyboard: eat key 'j'
- Keyboard: handle mapping, focusing next view
- Seat: send wl_keyboard.enter with key Super.
The necessity of this was already mentioned in 1e3b8ed1; however,
without a comment in code, it was removed in 393bfb42 as superfluous.
Hopefully, the newly added comment will prevent such mistakes in the
future.
Fixes https://github.com/riverwm/river/issues/978
As noticed by leon-p, last refactorings made river send a release event
to the client even if the press event has been eaten. In addition, the
introduction of input method support means that we need to remember
*why* we've eaten the key.
Also make KeycodeSet more strict: i am not aware of any case when a
keyboard could have the same key pressed twice (specifically, keyboard
groups have this handled in wlroots), so make the behavior follow a
smaller set of possible scenarios.
It's unclear if this is technically a violation of the protocol or not,
but it makes little sense to do this and many clients in the wild crash
if wl_keyboard.enter is sent before wl_keyboard.keymap.
If our current approch without xkbcommon translation does not match any
mapping on a key event attempt to match the translated keysym as well.
This makes e.g. the keypad number keys (e.g. KP_1) work intuitively as
they may require translation with numlock active.
The reason we stopped doing this in I7c02ebcbc was due to layout where
e.g. Super+Shift+Space is translated as Space with the Shift modifier
consumed, thereby conflicting with a separate mapping for Super+Space.
This should not be a issue anymore though as we now only run a maximum
of one mapping per key event and we attemt to match mappings without
xkbcommon translation before attempting with translation.
