implement `keyboard-shortcuts-inhibit` and `wlr-virtual-pointer` #630

sodiboo · 2024-08-26T20:56:40Z

My motivation for this is the use of KVM switching software, in particular i noticed that lan-mouse requires them (hence my branch name).

Two things need to be done:

Please someone check that what i am doing with pointer axis frames is sane. This was painful. It works. Smithay docs refer to a nonexistent function so i had to check 2-year-old git blames to find out what replaced it. We just ignore the axis source if it's done before an arbitrary other thing?? Help.
Need to implement a keyboard-shortcuts-inhibit release bind. The current implementation already doesn't inhibit the ChangeVt binds ever.

You can try the functionality of both of these protocols using lan-mouse --test-emulation (will cause the pointer to move in a lemniscate) or lan-mouse --test-capture (WARNING: YOUR FOCUS WILL BE UNRECOVERABLE FROM WITHIN NIRI. use Ctrl+Alt+F2 and then pkill -f test-capture to recover. the Ctrl+Alt+{F1..F12} binds are the ChangeVt binds i've mentioned already)

Another thing i've noticed is that virtual-keyboard inputs aren't ever handled by the compositor (i.e. Super+Left is passed to whatever client, instead of focusing the column to the left)? This is irrelevant to this PR, but something i noticed while using lan-mouse. This is probably a Smithay issue? Since it's uhh mostly implemented in Smithay.

YaLTeR

Smithay docs refer to a nonexistent function so i had to check 2-year-old git blames to find out what replaced it. We just ignore the axis source if it's done before an arbitrary other thing??

I haven't looked in detail but if it involves axis v120 events then yeah it's like this pretty much because of legacy, v120 events replaced the old ones: https://wayland.freedesktop.org/libinput/doc/latest/wheel-api.html

YaLTeR · 2024-08-27T13:42:57Z

src/input/mod.rs

@@ -2221,6 +2247,18 @@ fn should_intercept_key(
        }
    }

+    if is_inhibiting_shortcuts


I have a feeling that this should interact in a more complex manner with the match below because starting to inhibit while some modifier is held (and in suppressed_keys) then releasing the modifier will currently leave it in suppressed_keys, which does not seem right.

This behavior should also be tested in the unit tests below.

I've since updated the handling in a way that i think works with supressed_keys properly (and i've commented the behaviour), but to be honest, thinking about the order of events kinda makes my head hurt and i'm dreading writing the requested tests. I'm also not entirely confident that my intuition of how it should behave exactly aligns with what you think it should do? Do you wanna take a shot at writing tests for this? 🥺👉👈

Seems fine to me. For the tests, just add a few cases with the shortcuts inhibited = true. For pressing and releasing allow-inhibit, !allow-inhibit, which also check suppress keys. Basically check that a shortcut that would return Intercept() returns instead Forward if inhibited = true. And maybe also something which first does a press with inhibited = true, then release with inhibited = false in case that logic would change later.

src/input/mod.rs

sodiboo · 2024-09-07T20:56:56Z

I've implemented the allow-inhibiting property for binds, and the toggle-keyboard-shortcuts-inhibit action, which weren't present in the initial draft of this PR. This is basically the only thing that was missing from making this PR "complete". keyboard-shortcuts-inhibit is now fully implemented (should probably have some more tests).

I've also made sure you can't bind toggle-keyboard-shortcuts-inhibit but allow that bind to be inhibited. the allow-inhibiting property is ignored for this action

wlr-virtual-pointer worked flawlessly right off the bat for my use case, and you wouldn't be able to tell from using, but the underlying code is definitely janky!!! I'd like someone to verify that this is how the Axis* events are suppposed to work. Basically if we get more than one timestamp in a single frame, all but the first are ignored. And... AxisDiscrete overrides the values set by Axis? It feels very dirty, but judging by the AxisFrame struct in Smithay, it feels like the protocol should not really be allowing any of these requests to be sent twice in one frame. It feels very jank, and i know little about how axis events actually work, i just plugged together the parts of Smithay that "looked" about right to me.

The uhh... AxisSource request is optional (i think?) but PointerAxisEvent::source() does not allow a None value, so this is definitely wrong. The AxisFrame struct cannot be constructed without a timestamp, so for the AxisSource event, which doesn't include a timestamp, it's just dropped because no AxisFrame can be started; a different Axis* event must be sent first for the AxisSource to amend it.

YaLTeR

I'd like someone to verify that this is how the Axis* events are suppposed to work. Basically if we get more than one timestamp in a single frame, all but the first are ignored. And... AxisDiscrete overrides the values set by Axis? It feels very dirty, but judging by the AxisFrame struct in Smithay, it feels like the protocol should not really be allowing any of these requests to be sent twice in one frame. It feels very jank, and i know little about how axis events actually work, i just plugged together the parts of Smithay that "looked" about right to me.

The uhh... AxisSource request is optional (i think?) but PointerAxisEvent::source() does not allow a None value, so this is definitely wrong. The AxisFrame struct cannot be constructed without a timestamp, so for the AxisSource event, which doesn't include a timestamp, it's just dropped because no AxisFrame can be started; a different Axis* event must be sent first for the AxisSource to amend it.

Yeah idk tbh, I would just test with several wlr-virtual-pointer clients to see that they work. And if there's something weird, check in WAYLAND_DEBUG=1 what events they send.

(btw, what wlr-virtual-pointer clients are there apart from lan-mouse? I'll need a few to test this I guess)

YaLTeR · 2024-10-06T07:14:03Z

resources/default-config.kdl

+    //
+    // The allow-inhibiting=false property can be applied to other binds as well,
+    // which ensures niri always processes them, even when an inhibitor is active.
+    Mod+Shift+Escape allow-inhibiting=false { toggle-keyboard-shortcuts-inhibit; }


I think we should bind this to Mod+Escape by default because Super+Escape is the default in GNOME: https://gitlab.gnome.org/GNOME/mutter/-/blob/d7d92c68bd5c76525752feab65c44b3157deb7f1/data/org.gnome.mutter.wayland.gschema.xml.in#L58-61

YaLTeR · 2024-10-06T07:18:53Z

src/handlers/mod.rs

+    }
+
+    fn new_inhibitor(&mut self, inhibitor: KeyboardShortcutsInhibitor) {
+        inhibitor.activate();


Suggested change

inhibitor.activate();

// FIXME: show a confirmation dialog with a "remember for this application" kind of toggle.

inhibitor.activate();

YaLTeR · 2024-10-06T07:22:37Z

src/input/mod.rs

+        if let KeyboardFocus::LayerShell { surface }
+        | KeyboardFocus::LockScreen {
+            surface: Some(surface),
+        }
+        | KeyboardFocus::Layout {
+            surface: Some(surface),
+        } = &self.niri.keyboard_focus


If only there was a convenient KeyboardFocus::surface() method that did exactly this :p

yeah. if only.

YaLTeR · 2024-10-06T07:27:29Z

niri-config/src/lib.rs

                Mod+T allow-when-locked=true { spawn "alacritty"; }
                Mod+Q { close-window; }
                Mod+Shift+H { focus-monitor-left; }
                Mod+Ctrl+Shift+L { move-window-to-monitor-right; }
                Mod+Comma { consume-window-into-column; }
                Mod+1 { focus-workspace 1; }
                Mod+Shift+1 { focus-workspace "workspace-1"; }
-                Mod+Shift+E { quit skip-confirmation=true; }
+                Mod+Shift+E allow-inhibiting=false { quit skip-confirmation=true; }


I wonder if allow-inhibiting=false is a bit hard to wrap one's head around? Maybe something along prevent-inhibiting=true would be better, human config file reading and writing wise?

src/input/mod.rs

YaLTeR · 2024-10-06T07:32:56Z

src/input/mod.rs

+            // shortcuts inhibition here, because if it wasn't inhibited on press, it wouldn't be
+            // suppressed, so it would already have been forwarded at the start of this function.


Suggested change

// shortcuts inhibition here, because if it wasn't inhibited on press, it wouldn't be

// suppressed, so it would already have been forwarded at the start of this function.

// shortcuts inhibition here, because if it was inhibited on press (forwarded to the client), it wouldn't be

// suppressed, so it would already have been forwarded at the start of this function.

Is this right?

yeah. that seems correct. i think i wrote that thinking "if it wasn't intercepted" (== YES inhibited). or thinking that the keypress itself is inhhibited; but what that really means here is that the compositor's handling of the keypress is inhibited.

YaLTeR · 2024-10-06T07:40:40Z

src/protocols/virtual_pointer.rs

+#[derive(Debug)]
+pub struct VirtualPointerUserData {
+    seat: Option<WlSeat>,
+    output: Option<WlOutput>,


What is this supposed to do?

Uh,, it stores the parameters the virtual pointer was created with? I don't think i used it for anything (and looking at references, i just noticed that i kept in #[allow(dead_code)] for the getters); but this is like, "generic protocol code" that could (should?) be part of Smithay or copied to other compositors, so i figured it would be weird to not keep it in.

Something we should be doing, though, is properly handling absolute pointer events when output is Some(_), which i don't think i did. This would just happen to work by chance on single-monitor layouts, but i think on multi-monitor it would look janky without special care taken (as is done for tablets' and touchscreens' map-to-output)

i just noticed that i kept in #[allow(dead_code)] for the getters

fixed this now by making them pub instead, which also silences the warnings. since i never ended up using them, ig i forgot that i should've made them pub. lol.

I guess it's for absolute motion, yeah. How hard would that be to implement?

From a glance at the input handling, it looks like it would be nontrivial to make this track the correct output. We could fairly easily just bypass the existing input handling; but I think it'd be best to have some trait we implement on each input backend to allow us to query further device information about each event, like which output this absolute position event logically refers to, if any. This should apply equally to all devices, and would probably imply the ability to configure individual devices in the config, and virtual pointers are just one kind of device that is configured via wlr-virtual-pointer instead.

This is... maybe too much just for this PR? should I open another one? Alternatively, maybe Smithay should be able to handle providing an output for an absolute motion event? At the very least, that Wayland backend I've been working on does require the ability to specify which output a motion event belongs to, if we're ever gonna hope to see it use multiple outputs mapped as multiple toplevels. That could make more sense to implement in Smithay than in niri.

Likewise, it makes a lot of sense for individual keyboard devices to carry their xkb states and keymaps, for e.g. virtual keyboard, which currently Smithay just sneaks past the compositor and directly into its surface tree, but ideally they should go through the normal input backend system, and propagate their keymaps properly. The same goes for my Wayland backend, which can have multiple keyboards in different outer seats with different layouts. A lot of that could/should make its way into Smithay, while at the same time it wouldn't really be too difficult to implement such extension traits in niri.

There's already a map-to-output system in niri for tablets and touchscreens, and there's a downcast to libinput event to figure out what device the event comes from. Maybe you could similarly downcast to a virtual pointer event, and then get the output from it?

There's already a map-to-output system in niri for tablets and touchscreens

Yeah, I know. But these are applied unilaterally to all touch and tablet position events, based on the event kind; it's not very useful to apply that to virtual pointers since there isn't a single output they all should map to. What I'm suggesting is we replace this system with one that can associate the mapped output with the device or the event data.

Yeah but if you downcast the absolute motion pointer event to its virtual absolute motion pointer event type, then you can grab the right output from that event. Should be simple enough until proper systems are in place in Smithay or wherever.

src/protocols/virtual_pointer.rs

YaLTeR · 2024-10-06T07:43:17Z

src/handlers/mod.rs

+        self.process_input_event(InputEvent::<VirtualPointerInputBackend>::PointerAxis { event });
+    }
+}
+


Suggested change

YaLTeR · 2024-10-06T07:51:08Z

src/input/mod.rs

@@ -2221,6 +2247,18 @@ fn should_intercept_key(
        }
    }

+    if is_inhibiting_shortcuts


Seems fine to me. For the tests, just add a few cases with the shortcuts inhibited = true. For pressing and releasing allow-inhibit, !allow-inhibit, which also check suppress keys. Basically check that a shortcut that would return Intercept() returns instead Forward if inhibited = true. And maybe also something which first does a press with inhibited = true, then release with inhibited = false in case that logic would change later.

sodiboo · 2024-10-06T12:34:26Z

(btw, what wlr-virtual-pointer clients are there apart from lan-mouse? I'll need a few to test this I guess)

just about any KVM or RDP software. Something like deskflow or input leap should do the same thing as lan-mouse. I also noticed that WayVNC works just fine with niri as of this PR, as it has several capture protocols including wlr-screencopy, so you can connect to it from wlvncc.

Most of these should probably also be using keyboard shortcuts inhibit? Unsure. I know lan-mouse does, at least.

This was referenced Aug 26, 2024

idle inhibit on unfocused computers feschber/lan-mouse#181

Open

binds are not processed for virtual-keyboard events #631

Closed

YaLTeR reviewed Aug 27, 2024

View reviewed changes

sodiboo changed the title ~~implement wlr-keyboard-shortcuts-inhibit and wlr-virtual-pointer~~ implement keyboard-shortcuts-inhibit and wlr-virtual-pointer Aug 27, 2024

YaLTeR force-pushed the main branch from 62f2af1 to a2c8553 Compare September 4, 2024 09:39

sodiboo force-pushed the support-lan-mouse branch 3 times, most recently from bfd9d78 to 62982c4 Compare September 7, 2024 19:53

sodiboo marked this pull request as ready for review September 7, 2024 21:08

YaLTeR reviewed Oct 6, 2024

View reviewed changes

sodiboo added 5 commits October 6, 2024 15:09

stub keyboard-shortcuts-inhibit and virtual-pointer impls

326685b

implement keyboard-shortcuts-inhibit

60ce71d

implement virtual-pointer

b82c171

deal with supressed key release edge-case; add allow-inhibiting property

d4396ed

add toggle-keyboard-shortcuts-inhibit bind

234b6b5

sodiboo force-pushed the support-lan-mouse branch from 62982c4 to 234b6b5 Compare October 6, 2024 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `keyboard-shortcuts-inhibit` and `wlr-virtual-pointer` #630

implement `keyboard-shortcuts-inhibit` and `wlr-virtual-pointer` #630

sodiboo commented Aug 26, 2024

YaLTeR left a comment

YaLTeR Aug 27, 2024

sodiboo Sep 7, 2024

YaLTeR Oct 6, 2024

sodiboo commented Sep 7, 2024

YaLTeR left a comment

YaLTeR Oct 6, 2024

YaLTeR Oct 6, 2024

YaLTeR Oct 6, 2024

sodiboo Oct 7, 2024

YaLTeR Oct 6, 2024

YaLTeR Oct 6, 2024

sodiboo Oct 6, 2024

YaLTeR Oct 6, 2024

sodiboo Oct 6, 2024

sodiboo Oct 6, 2024

YaLTeR Oct 6, 2024

sodiboo Oct 7, 2024

YaLTeR Oct 7, 2024

sodiboo Oct 7, 2024

YaLTeR Oct 7, 2024

YaLTeR Oct 6, 2024

YaLTeR Oct 6, 2024

sodiboo commented Oct 6, 2024

	inhibitor.activate();
	// FIXME: show a confirmation dialog with a "remember for this application" kind of toggle.
	inhibitor.activate();

		// shortcuts inhibition here, because if it wasn't inhibited on press, it wouldn't be
		// suppressed, so it would already have been forwarded at the start of this function.

implement keyboard-shortcuts-inhibit and wlr-virtual-pointer #630

Are you sure you want to change the base?

implement keyboard-shortcuts-inhibit and wlr-virtual-pointer #630

Conversation

sodiboo commented Aug 26, 2024

YaLTeR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodiboo commented Sep 7, 2024

YaLTeR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodiboo commented Oct 6, 2024

implement `keyboard-shortcuts-inhibit` and `wlr-virtual-pointer` #630

implement `keyboard-shortcuts-inhibit` and `wlr-virtual-pointer` #630