Currently if we disable an output due to a wlr-output-power-management
protocol request we do not update Output.lock_render_state properly.
This is fine if the output is also re-enabled using the
wlr-output-power-management protocol but causes an assertion failure
if it is re-enabled using wlr-output-management instead.
Also clean up this code a bit, it's no longer necessary to split these
one line functions out into separate files as Zig's conditional
compilation support has improved since these functions were originally
written.
This field being nullable at all is code smell. I think what needs to
happen here long term is for a proper separation of "window management
output" and "physical output" as concepts and integration outputs into
the transaction system.
That's a much larger change and I don't want to cause that amount of
code churn just before a release though.
I have managed to crash river (because the relevant assertion was wrong)
on this using fcitx5 (5.1.8) and anthy with the following sequence of
key events on a physical keyboard (every line followed by releasing
everything):
super-; (my shortcut to enable anthy)
shift-letter (fcitx doesn't release shift on its virtual keyboard)
escape
shift (fcitx sends another press event)
The failure to release shift is not the only weirdness happening, but
it's what eventually lead to the assertion failure.
This makes tablet tool cursors visually distinct from pointer cursors by
default. Client may of course continue to set custom cursors for tablet
tools if they have focus.
Also fixes a custom cursor set by a client persisting after the tablet
exits the client's surface until proximity out.
This was dropped from 200ms to 50ms in 4a65af66. However 50ms seems
to be a bit too short in practice. I often hit ugly frame imperfection
do to timeouts when toggling mpv between a small floating window and
fullscreen for example, even on a relatively beefy desktop computer.
This only happens while the video is playing in mpv, not while it is
paused. I believe this is due to mpv ignoring the compositor's hints for
when to render a new frame entirely while playing a video. It instead
renders at the framerate of the video being played, even if the
compositor requests a change in size. This isn't great but seems
unlikely to change [1].
Overall, hitting 100ms timeouts subjectively doesn't feel anywhere near
as sluggish as hitting 200ms timeouts and offers better frame perfection
than 50ms timeouts in at least this one example, there are bound to be
others.
[1]: https://dudemanguy.github.io/blog/posts/2022-06-10-wayland-xorg/wayland-xorg.html
Currently we can hit an assertion failure in the putNoClobber() call in
response to the down event since we fail to handle the cancel event.
This commit fixes that issue.
Currently we only support interactive move/resize with the pointer,
touch and tablet tool support are TODO.
Validate the serial here to ensure we don't start a pointer move/resize
in response to the client attempting to start a move/resize with
touch/tablet tool.
This fixes an assertion failure that the pointer's cursor is not hidden
during move/resize, which is how the issue was discovered. Another win
for assertions :)
Sensitive Wayland protocols such as wlr_screencopy and wlr_data_control
(clipboard managment) are now blocked by default inside security
contexts (e.g. flatpak 1.15.6 or later).
User configuration of the allowlist/blocklist is TODO.
If multiple consecutive transaction timeouts occur it is possible for
the SSD borders to become temporarily desynchronized with the size of
the rendered surface.
This is especially noticeable during interactive resize of mpv.
For details on cause/fix, see the new comment in the code.
This fixes a possible assertion failure in Cursor.updateState() when
trying to start move/resize of a xdg toplevel with the timed_out or
timed_out_acked configure_state.
This also generally improves the UX of transaction timeouts as all
state except for the size change is now applied immediately.
There is not any pointer emulation for tablet tool input. This means
that only clients implementing the tablet-v2 protocol will be able to
process tablet tool input.
Tablet pad support is TODO
This assertion does not consider that a view may be visible on an output
despite no view on that output having focus due to a differnt output
currently having focus.
I don't see any clean way to fix the assert, so just remove it.
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.
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.
