Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
49 changes: 48 additions & 1 deletion niri-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,7 @@ pub struct Bind {
pub repeat: bool,
pub cooldown: Option<Duration>,
pub allow_when_locked: bool,
pub allow_inhibiting: bool,
}

#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)]
Expand Down Expand Up @@ -1195,6 +1196,7 @@ pub enum Action {
ScreenshotWindow,
#[knuffel(skip)]
ScreenshotWindowById(u64),
ToggleKeyboardShortcutsInhibit,
CloseWindow,
#[knuffel(skip)]
CloseWindowById(u64),
Expand Down Expand Up @@ -2863,6 +2865,7 @@ where
let mut cooldown = None;
let mut allow_when_locked = false;
let mut allow_when_locked_node = None;
let mut allow_inhibiting = true;
for (name, val) in &node.properties {
match &***name {
"repeat" => {
Expand All @@ -2877,6 +2880,9 @@ where
allow_when_locked = knuffel::traits::DecodeScalar::decode(val, ctx)?;
allow_when_locked_node = Some(name);
}
"allow-inhibiting" => {
allow_inhibiting = knuffel::traits::DecodeScalar::decode(val, ctx)?;
}
name_str => {
ctx.emit_error(DecodeError::unexpected(
name,
Expand All @@ -2898,6 +2904,7 @@ where
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
};

if let Some(child) = children.next() {
Expand All @@ -2920,12 +2927,19 @@ where
}
}

// The toggle-inhibit action must always be uninhibitable.
// Otherwise, it would be impossible to trigger it.
if matches!(action, Action::ToggleKeyboardShortcutsInhibit) {
allow_inhibiting = false;
}

Ok(Self {
key,
action,
repeat,
cooldown,
allow_when_locked,
allow_inhibiting,
})
}
Err(e) => {
Expand Down Expand Up @@ -3307,14 +3321,16 @@ mod tests {
}

binds {
Mod+Escape { toggle-keyboard-shortcuts-inhibit; }
Mod+Shift+Escape allow-inhibiting=true { toggle-keyboard-shortcuts-inhibit; }
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; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't feel like that change is an improvement, because prevent-inhibiting=true is a double negative. but also, i agree with your concern, and i think it's slightly confusing because allow-inhibiting=false is also a double negative, because the value it will be set to is false.

is there a way to name it that isn't a double negative? always-available=true, but more descriptive? borrow an idiom from CSS and call it important=true? maybe with the exclamation point to drive home the correlation? !important=true. though it reads as "not".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, difficult question indeed. I like the word "inhibit" being there because it's the technical term for this (unless it isn't always? do some apps re-phrase it?).

If we drop inhibit, maybe something like "always-accessible" is better than "always-available"? That's kind of still not very descriptive, while it does control a very specific thing, so it would be good if it were descriptive...

Mod+WheelScrollDown cooldown-ms=150 { focus-workspace-down; }
}

Expand Down Expand Up @@ -3616,6 +3632,28 @@ mod tests {
},
],
binds: Binds(vec![
Bind {
key: Key {
trigger: Trigger::Keysym(Keysym::Escape),
modifiers: Modifiers::COMPOSITOR,
},
action: Action::ToggleKeyboardShortcutsInhibit,
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: false,
},
Bind {
key: Key {
trigger: Trigger::Keysym(Keysym::Escape),
modifiers: Modifiers::COMPOSITOR | Modifiers::SHIFT,
},
action: Action::ToggleKeyboardShortcutsInhibit,
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: false,
},
Bind {
key: Key {
trigger: Trigger::Keysym(Keysym::t),
Expand All @@ -3625,6 +3663,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: true,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3635,6 +3674,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3645,6 +3685,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3655,6 +3696,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3665,6 +3707,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3675,6 +3718,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3687,6 +3731,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: true,
},
Bind {
key: Key {
Expand All @@ -3697,6 +3742,7 @@ mod tests {
repeat: true,
cooldown: None,
allow_when_locked: false,
allow_inhibiting: false,
},
Bind {
key: Key {
Expand All @@ -3707,6 +3753,7 @@ mod tests {
repeat: true,
cooldown: Some(Duration::from_millis(150)),
allow_when_locked: false,
allow_inhibiting: true,
},
]),
switch_events: SwitchBinds {
Expand Down
10 changes: 10 additions & 0 deletions resources/default-config.kdl
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,16 @@ binds {
Ctrl+Print { screenshot-screen; }
Alt+Print { screenshot-window; }

// Applications such as remote-desktop clients and software KVM switches may
// request that niri stops processing the keyboard shortcuts defined here
// so they may, for example, forward the key presses as-is to a remote machine.
// It's a good idea to bind an escape hatch to toggle the inhibitor,
// so a buggy application can't hold your session hostage.
//
// 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+Escape allow-inhibiting=false { toggle-keyboard-shortcuts-inhibit; }

// The quit action will show a confirmation dialog to avoid accidental exits.
Mod+Shift+E { quit; }
Ctrl+Alt+Delete { quit; }
Expand Down
5 changes: 5 additions & 0 deletions src/backend/winit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,4 +266,9 @@ impl Winit {
pub fn ipc_outputs(&self) -> Arc<Mutex<IpcOutputMap>> {
self.ipc_outputs.clone()
}

// FIXME: If/when the winit backend supports multiple outputs, then this method makes no sense.
pub fn single_output(&self) -> &Output {
&self.output
}
}
69 changes: 62 additions & 7 deletions src/handlers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::time::Duration;

use smithay::backend::allocator::dmabuf::Dmabuf;
use smithay::backend::drm::DrmNode;
use smithay::backend::input::TabletToolDescriptor;
use smithay::backend::input::{InputEvent, TabletToolDescriptor};
use smithay::desktop::{PopupKind, PopupManager};
use smithay::input::pointer::{
CursorIcon, CursorImageStatus, CursorImageSurfaceData, PointerHandle,
Expand All @@ -35,6 +35,9 @@ use smithay::wayland::fractional_scale::FractionalScaleHandler;
use smithay::wayland::idle_inhibit::IdleInhibitHandler;
use smithay::wayland::idle_notify::{IdleNotifierHandler, IdleNotifierState};
use smithay::wayland::input_method::{InputMethodHandler, PopupSurface};
use smithay::wayland::keyboard_shortcuts_inhibit::{
KeyboardShortcutsInhibitHandler, KeyboardShortcutsInhibitState, KeyboardShortcutsInhibitor,
};
use smithay::wayland::output::OutputHandler;
use smithay::wayland::pointer_constraints::{with_pointer_constraint, PointerConstraintsHandler};
use smithay::wayland::security_context::{
Expand All @@ -59,11 +62,12 @@ use smithay::wayland::xdg_activation::{
use smithay::{
delegate_cursor_shape, delegate_data_control, delegate_data_device, delegate_dmabuf,
delegate_drm_lease, delegate_fractional_scale, delegate_idle_inhibit, delegate_idle_notify,
delegate_input_method_manager, delegate_output, delegate_pointer_constraints,
delegate_pointer_gestures, delegate_presentation, delegate_primary_selection,
delegate_relative_pointer, delegate_seat, delegate_security_context, delegate_session_lock,
delegate_single_pixel_buffer, delegate_tablet_manager, delegate_text_input_manager,
delegate_viewporter, delegate_virtual_keyboard_manager, delegate_xdg_activation,
delegate_input_method_manager, delegate_keyboard_shortcuts_inhibit, delegate_output,
delegate_pointer_constraints, delegate_pointer_gestures, delegate_presentation,
delegate_primary_selection, delegate_relative_pointer, delegate_seat,
delegate_security_context, delegate_session_lock, delegate_single_pixel_buffer,
delegate_tablet_manager, delegate_text_input_manager, delegate_viewporter,
delegate_virtual_keyboard_manager, delegate_xdg_activation,
};

pub use crate::handlers::xdg_shell::KdeDecorationsModeState;
Expand All @@ -75,10 +79,15 @@ use crate::protocols::gamma_control::{GammaControlHandler, GammaControlManagerSt
use crate::protocols::mutter_x11_interop::MutterX11InteropHandler;
use crate::protocols::output_management::{OutputManagementHandler, OutputManagementManagerState};
use crate::protocols::screencopy::{Screencopy, ScreencopyHandler, ScreencopyManagerState};
use crate::protocols::virtual_pointer::{
VirtualPointerAxisEvent, VirtualPointerButtonEvent, VirtualPointerHandler,
VirtualPointerInputBackend, VirtualPointerManagerState, VirtualPointerMotionAbsoluteEvent,
VirtualPointerMotionEvent,
};
use crate::utils::{output_size, send_scale_transform, with_toplevel_role};
use crate::{
delegate_foreign_toplevel, delegate_gamma_control, delegate_mutter_x11_interop,
delegate_output_management, delegate_screencopy,
delegate_output_management, delegate_screencopy, delegate_virtual_pointer,
};

pub const XDG_ACTIVATION_TOKEN_TIMEOUT: Duration = Duration::from_secs(10);
Expand Down Expand Up @@ -243,7 +252,28 @@ impl InputMethodHandler for State {
}
}

impl KeyboardShortcutsInhibitHandler for State {
fn keyboard_shortcuts_inhibit_state(&mut self) -> &mut KeyboardShortcutsInhibitState {
&mut self.niri.keyboard_shortcuts_inhibit_state
}

fn new_inhibitor(&mut self, inhibitor: KeyboardShortcutsInhibitor) {
// FIXME: show a confirmation dialog with a "remember for this application" kind of toggle.
inhibitor.activate();
sodiboo marked this conversation as resolved.
Show resolved Hide resolved
self.niri
.keyboard_shortcuts_inhibiting_surfaces
.insert(inhibitor.wl_surface().clone(), inhibitor);
}

fn inhibitor_destroyed(&mut self, inhibitor: KeyboardShortcutsInhibitor) {
self.niri
.keyboard_shortcuts_inhibiting_surfaces
.remove(&inhibitor.wl_surface().clone());
}
}

delegate_input_method_manager!(State);
delegate_keyboard_shortcuts_inhibit!(State);
delegate_virtual_keyboard_manager!(State);

impl SelectionHandler for State {
Expand Down Expand Up @@ -562,6 +592,31 @@ impl ScreencopyHandler for State {
}
delegate_screencopy!(State);

impl VirtualPointerHandler for State {
fn virtual_pointer_manager_state(&mut self) -> &mut VirtualPointerManagerState {
&mut self.niri.virtual_pointer_state
}

fn on_virtual_pointer_motion(&mut self, event: VirtualPointerMotionEvent) {
self.process_input_event(InputEvent::<VirtualPointerInputBackend>::PointerMotion { event });
}

fn on_virtual_pointer_motion_absolute(&mut self, event: VirtualPointerMotionAbsoluteEvent) {
self.process_input_event(
InputEvent::<VirtualPointerInputBackend>::PointerMotionAbsolute { event },
);
}

fn on_virtual_pointer_button(&mut self, event: VirtualPointerButtonEvent) {
self.process_input_event(InputEvent::<VirtualPointerInputBackend>::PointerButton { event });
}

fn on_virtual_pointer_axis(&mut self, event: VirtualPointerAxisEvent) {
self.process_input_event(InputEvent::<VirtualPointerInputBackend>::PointerAxis { event });
}
}
delegate_virtual_pointer!(State);

impl DrmLeaseHandler for State {
fn drm_lease_state(&mut self, node: DrmNode) -> &mut DrmLeaseState {
self.backend
Expand Down
50 changes: 50 additions & 0 deletions src/input/backend_ext.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
use ::input as libinput;
use smithay::backend::input;
use smithay::backend::winit::WinitVirtualDevice;
use smithay::output::Output;

use crate::backend::Backend;
use crate::niri::State;
use crate::protocols::virtual_pointer::VirtualPointer;

pub trait NiriInputBackend: input::InputBackend<Device = Self::NiriDevice> {
type NiriDevice: NiriInputDevice;
}
impl<T: input::InputBackend> NiriInputBackend for T
where
Self::Device: NiriInputDevice,
{
type NiriDevice = Self::Device;
}

pub trait NiriInputDevice: input::Device {
// FIXME: this should maybe be per-event, not per-device,
// but it's not clear that this matters in practice?
// it might be more obvious once we implement it for libinput
fn output(&self, state: &State) -> Option<Output>;
}

impl NiriInputDevice for libinput::Device {
fn output(&self, _state: &State) -> Option<Output> {
// FIXME: Allow specifying the output per-device?
None
}
}

impl NiriInputDevice for WinitVirtualDevice {
fn output(&self, state: &State) -> Option<Output> {
match state.backend {
Backend::Winit(ref winit) => Some(winit.single_output().clone()),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually do anything? Maybe just replace the whole thing with None and remove the single_output() function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, it asserts that the mouse position on the winit backend will break if/when it ever adds another output. Like, it'd try to scale it relative to the global extents of all monitors instead of just the one. That's obviously like, super wrong, for everything on the winit backend.


I guess it might also correctly transform the tablet position? If we return None, we hit that code path:

(geo, false, 1. / scale, Transform::Normal)

(the Transform::Normal here looks suspicious to me)


in general, it just makes sure the more relevant known-output code paths are hit. It feels more correct, if nothing else.

// returning None over panicking here because it's not worth panicking over
// and also, foreseeably, someone might want to, at some point, use `WinitInputBackend`
// for dirty hacks or mocking or whatever, in which case this will be useful.
_ => None,
}
}
}

impl NiriInputDevice for VirtualPointer {
fn output(&self, _: &State) -> Option<Output> {
self.output().cloned()
}
}
Loading