feat(keyboard_locks): add Caps/Num/Scroll lock indicators - #755
feat(keyboard_locks): add Caps/Num/Scroll lock indicators#755noirbizarre wants to merge 1 commit into
Conversation
MalpenZibo
left a comment
There was a problem hiding this comment.
Sorry, I'm a bit late, but this PR is quite complex, so I decided to postpone it after the release.
Anyway, if I understand correctly, this PR can retrieve the current status of the lock indicators and react to a change by watching the lock indicator status.
For example, if ashell starts after the user clicks the Caps Lock, we're able to understand that the CAPS lock is active.
The only downside is the permissions that you have documented. A nice addition.
I will take some time to complete the review because the keyboard_locks is more complex than I thought!
There was a problem hiding this comment.
Ok, the PR seems good. I only find the service part a little bit confusing.
If I understand correctly, we're spawning a tokio task for each keyboard, then we use an mpsc channel and track the join handle in a HashMap. Maybe we can handle everything with a simple SelectAll.
fn open_lock_stream(path: &Path) -> Option<(KeyboardLocksData, EventStream)> {
let device = match open_device_with_retry(path) {
Ok(Some(device)) => device,
Ok(None) => return None,
Err(err) => {
debug!("failed to open {}: {err}", path.display());
return None;
}
};
if !has_lock_leds(&device) {
return None;
}
let initial = read_lock_state(&device)
.map_err(|err| debug!("failed to read LED state for {}: {err}", path.display()))
.ok()?;
let stream = device
.into_event_stream()
.map_err(|err| debug!("failed to start event stream for {}: {err}", path.display()))
.ok()?;
Some((initial, stream))
}
fn device_lock_stream(
path: PathBuf,
initial: KeyboardLocksData,
stream: EventStream,
) -> BoxStream<'static, (PathBuf, Option<KeyboardLocksData>)> {
stream::unfold(
(stream, initial, path, false),
|(mut stream, mut current, path, ended)| async move {
if ended {
return None;
}
loop {
match stream.next_event().await {
Ok(event) => {
let EventSummary::Led(_, code, value) = event.destructure() else {
continue;
};
let on = value != 0;
let changed = match code {
LedCode::LED_CAPSL if current.caps_lock != on => {
current.caps_lock = on;
true
}
LedCode::LED_NUML if current.num_lock != on => {
current.num_lock = on;
true
}
LedCode::LED_SCROLLL if current.scroll_lock != on => {
current.scroll_lock = on;
true
}
_ => false,
};
if changed {
return Some(((path.clone(), Some(current)), (stream, current, path, false)));
}
}
Err(err) => {
debug!("event stream error for {}: {err}; dropping device", path.display());
return Some(((path.clone(), None), (stream, current, path, true)));
}
}
}
},
)
.boxed()
}
fn add_device(
path: PathBuf,
per_device: &mut HashMap<PathBuf, KeyboardLocksData>,
streams: &mut SelectAll<BoxStream<'static, (PathBuf, Option<KeyboardLocksData>)>>,
) {
if per_device.contains_key(&path) {
return;
}
if let Some((initial, stream)) = open_lock_stream(&path) {
per_device.insert(path.clone(), initial);
streams.push(device_lock_stream(path, initial, stream));
}
}
async fn supervise(
output: &mut Sender<ServiceEvent<KeyboardLocksService>>,
mut commands: UnboundedReceiver<KeyboardLocksCommand>,
) {
let mut monitor = match input_subsystem_monitor() {
Ok(monitor) => monitor,
Err(err) => {
warn!("Failed to set up input udev monitor: {err}");
return;
}
};
let mut per_device: HashMap<PathBuf, KeyboardLocksData> = HashMap::new();
let mut streams: SelectAll<BoxStream<'static, (PathBuf, Option<KeyboardLocksData>)>> =
SelectAll::new();
let mut current_aggregate = KeyboardLocksData::default();
let mut emitter = ToggleEmitter::new();
if let Ok(paths) = enumerate_event_devices() {
for path in paths {
add_device(path, &mut per_device, &mut streams);
}
}
info!(
"Keyboard locks service listening (tracking {} devices)",
per_device.len()
);
loop {
tokio::select! {
Some((path, update)) = streams.next() => {
match update {
Some(state) => { per_device.insert(path, state); }
None => { per_device.remove(&path); }
}
let new_aggregate = aggregate(&per_device);
if new_aggregate != current_aggregate {
current_aggregate = new_aggregate;
if output
.send(ServiceEvent::Update(KeyboardLocksEvent(current_aggregate)))
.await
.is_err()
{
break;
}
}
}
cmd = commands.recv() => {
let Some(cmd) = cmd else { break };
match cmd {
KeyboardLocksCommand::Toggle(kind) => emitter.toggle(kind),
}
}
guard = monitor.writable_mut() => {
match guard {
Ok(mut guard) => {
let events: Vec<_> = guard.get_inner().iter().collect();
for evt in events {
handle_udev_event(&evt, &mut per_device, &mut streams);
}
guard.clear_ready();
}
Err(err) => {
warn!("input udev monitor failed: {err}");
break;
}
}
}
}
}
}
fn handle_udev_event(
evt: &udev::Event,
per_device: &mut HashMap<PathBuf, KeyboardLocksData>,
streams: &mut SelectAll<BoxStream<'static, (PathBuf, Option<KeyboardLocksData>)>>,
) {
let Some(devnode) = evt.device().devnode().map(|p| p.to_path_buf()) else {
return;
};
let Some(name) = devnode.file_name().and_then(|n| n.to_str()) else {
return;
};
if !name.starts_with("event") {
return;
}
match evt.event_type() {
udev::EventType::Add => add_device(devnode, per_device, streams),
// On Remove the device's stream surfaces a read error and self-removes,
// emitting the `None` that clears `per_device` — nothing to abort here.
udev::EventType::Remove => {}
_ => {}
}
}There was a problem hiding this comment.
Thanks, that's a much better shape — applied in the latest push.
The service now keeps a single SelectAll<BoxStream<'static, (PathBuf, Option<KeyboardLocksData>)>> instead of one tokio task + mpsc + JoinHandle per keyboard:
open_lock_streamopens/filters the device and returns the initial state plus theEventStream.device_lock_streamwraps it instream::unfold, yielding only on an actual state change and a finalNonepayload when the underlying stream errors out, after which the stream terminates andSelectAlldrops it automatically.add_deviceis shared between the initial enumeration and the udevAddpath.handle_udev_eventno longer needs aRemovebranch — the stream self-removes and emits theNonethat clearsper_device, so the explicit abort/removal signalling is gone.
That also removed the tasks: HashMap<PathBuf, JoinHandle<()>>, the internal DeviceUpdate struct, and the best-effort abort loop at the end of supervise. Net -24 lines and no more tokio::spawn in the module.
One small deviation from your snippet: I kept the PermissionDenied arm in open_lock_stream so an inaccessible device is logged distinctly from a generic open failure — it's still just a debug! + None.
Also rebased on current main (the Message enum moved to src/app/message.rs in the meantime). make check is clean.
1c5752d to
9948874
Compare
|
Hi 👋🏼 I addressed the review comment. To answer your question, yes, it displays the locks (CapsLock, NumLock) as a widget. It offload the state and event/update responsibility to udev, explaining the permissions requirement. And yes, udev allows us to properly catch-up the status on late startup |
MalpenZibo
left a comment
There was a problem hiding this comment.
I have some security concerns about this
|
|
||
| let mut per_device: HashMap<PathBuf, KeyboardLocksData> = HashMap::new(); | ||
| let mut streams = DeviceStreams::new(); | ||
| let mut current_aggregate = KeyboardLocksData::default(); |
There was a problem hiding this comment.
current_aggregate is define with default and per_device is seeded with real data. What happen if the Num lock is on at boot? Turning it off should produce an aggregate identical to the starting one and no Update is sent
| return; | ||
| } | ||
|
|
||
| // On `Remove` the device's stream surfaces a read error and self-removes, |
There was a problem hiding this comment.
We are sure that the stream errors out before udev reports a re-Add on the same event?
| Err(err) => { | ||
| last_err = Some(err); | ||
| if attempt < 2 { | ||
| std::thread::sleep(Duration::from_millis(50)); |
There was a problem hiding this comment.
It's better to use tokio::time::sleep
| KeyboardLocksCommand::Toggle(kind) => emitter.toggle(kind), | ||
| } | ||
| } | ||
| guard = monitor.writable_mut() => { |
There was a problem hiding this comment.
Can you try with?
| guard = monitor.writable_mut() => { | |
| guard = monitor.readable_mut() => { |
| Both nodes are owned by the `input` group on most distributions, so the user | ||
| running ashell just needs to be a member of that group: |
There was a problem hiding this comment.
Here we should explicitly state that this will give any process running as that user read access to all keystrokes, not just ashell.
Also, I have some concern about this. I'm not sure if I really want to suggest to people to add their user to the input group. It's not a trivial security concern.
Summary
Adds a new bar module displaying Caps/Num/Scroll Lock state, with click-to-toggle support.
/dev/input/event*via evdev (compositor-agnostic — works on both Hyprland and Niri).inputsubsystem./dev/uinputvirtual keyboard. If/dev/uinputcannot be opened, the display still works and clicks become no-ops with a single warning.enabled,visibility(ActiveOnlydefault /AlwaysVisible), and an optionaliconoverride (Nerd Font glyph or plain text). Default glyphs bundled (no extra font install required).Permissions
Both
/dev/input/event*and/dev/uinputare typically owned by theinputgroup, so a singlesudo usermod -aG input \"\$USER\"is enough. The user docs cover the manual udev-rule fallback for systems where/dev/uinputis not in theinputgroup by default.Testing
make check(fmt + cargo check + clippy-D warnings) is clean.bar_style = \"Islands\"and all three locks set toAlwaysVisible: glyphs render flush inside one island, click toggles the corresponding lock, LED state updates after the kernel synthesizes the press./dev/uinputaccess the display still updates from external lock presses; clicks log a single warning and no-op.Docs
website/docs/configuration/modules/keyboard.mdadds a new "Keyboard Locks" section. Versioned docs underwebsite/versioned_docs/are untouched per the project policy.Screenshots
Notes