session: cursor_pos dedup, mouse_press bail, slot-ID stability, env-var test serial (Grok round 3 #7, #8, #10, #14)

#7: broadcast_mirror_click called hypr::cursor_pos().await?.0 then
.1 -- two hyprctl roundtrips. Replaced with a single
hypr::cursor_pos().await? returning (cx, cy).

#8: mouse_press always spawned the per-button repeat loop, even
when neither Mode::Mirror nor mouse_broadcast was on (a no-op press).
Added an early bail: when both are off, return Ok(()) without
firing the initial click or spawning the repeat task.

#10: refresh_slots re-enumerated the matched vec every 400 ms with
enumerate().map(|(i, c)| ((i + 1) as u32, c)), which shuffled slot
IDs mid-flight whenever one slot briefly hid -- regressing Bug #11.
Reuse the previous slot ID for any client whose wl_address is still
present in the new matched set; only assign fresh IDs (1..=n) to
genuinely new clients; drop disappeared clients.

#14: mouse_repeat_ms_clamps_env_var set ENBOXER_MOUSE_REPEAT_MS
without a serial guard. cargo test runs unit tests in parallel; a
concurrent test touching the same env var would race our reads.
Added a std::sync::Mutex<()> static to serialise the test.

cargo test 99+/0; clippy clean.
This commit is contained in:
en 2026-09-16 17:33:25 +02:00
parent 3f59c09ce8
commit 0906e86594

View File

@ -203,12 +203,29 @@ async fn refresh_slots(
matched.sort_by_key(|c| (c.at[1], c.at[0], c.pid));
let n = g.engine.profile.slots as usize;
let new_slots: Vec<(u32, Client)> = matched
.into_iter()
.take(n)
.enumerate()
.map(|(i, c)| ((i as u32) + 1, c))
// #10: stable slot IDs across refresh ticks. The previous
// enumerate-based re-numbering shuffled slot IDs every 400 ms
// whenever one slot briefly hid, breaking per-character
// assist/follow keys (Bug #11 regression). Reuse the previous
// slot ID for any client whose wl_address is still present in
// the new matched set; only assign fresh IDs to genuinely new
// clients; disappeared clients are dropped.
let old_by_addr: std::collections::HashMap<&str, u32> = g
.slots
.iter()
.map(|(s, c)| (c.address.as_str(), *s))
.collect();
let mut used: std::collections::HashSet<u32> = std::collections::HashSet::new();
let mut new_slots: Vec<(u32, Client)> = Vec::with_capacity(n);
for c in matched.into_iter().take(n) {
let id = old_by_addr
.get(c.address.as_str())
.copied()
.filter(|id| !used.contains(id))
.unwrap_or_else(|| (1..=(n as u32)).find(|i| !used.contains(i)).unwrap_or(1));
used.insert(id);
new_slots.push((id, c));
}
let old_keys: Vec<(u32, &str)> = g
.slots
.iter()
@ -832,22 +849,25 @@ pub fn others_clients(slots: &[(u32, Client)], leader: u32) -> Vec<Client> {
/// a per-button repeat loop. Cancels any prior repeat for the same
/// button before installing the new one.
pub async fn mouse_press(session: &Arc<Mutex<Session>>, button: u32) -> Result<()> {
// Fire the first click synchronously through whichever path the
// session is in (mouse-broadcast or mirror mode). mirror-click
// no-ops outside of Mode::Mirror so the dispatch is harmless.
{
// #8: bail early when neither mode is active. Don't burn a tokio
// task + the Session mutex on a no-op repeat loop. The bind was
// installed regardless of state (so the operator can toggle
// mouse_broadcast mid-hold) but the per-button repeat is only
// useful while something would fire on each tick.
let (mode, mouse_broadcast) = {
let g = session.lock().await;
let mode = g.engine.mode;
let mouse_broadcast = g.mouse_broadcast;
drop(g);
(g.engine.mode, g.mouse_broadcast)
};
if mode != Mode::Mirror && !mouse_broadcast {
return Ok(());
}
// Fire the first click synchronously through whichever path the
// session is in.
{
let r = if mode == Mode::Mirror {
broadcast_mirror_click(session, button).await
} else if mouse_broadcast {
broadcast_click(session, button).await
} else {
// Neither mode nor mouse_broadcast is on; the bind was
// installed anyway. Just no-op.
Ok(())
broadcast_click(session, button).await
};
if let Err(e) = r {
tracing::warn!("mouse-press initial click failed: {e}");
@ -1042,8 +1062,10 @@ async fn broadcast_mirror_click(session: &Arc<Mutex<Session>>, button: u32) -> R
let Some((_, primary)) = g.slots.iter().find(|(s, _)| *s == g.engine.leader_slot) else {
return Ok(());
};
let cx = hypr::cursor_pos().await?.0;
let cy = hypr::cursor_pos().await?.1;
// #7: one cursor_pos call. The previous two-call form (.0, .1)
// doubled the hyprctl IPC traffic and could read a stale
// second value if the cursor moved between calls.
let (cx, cy) = hypr::cursor_pos().await?;
let pw = primary.size[0].max(1) as f64;
let ph = primary.size[1].max(1) as f64;
let nx = (cx - primary.at[0]) as f64 / pw;
@ -1254,6 +1276,13 @@ mod tests {
#[test]
fn mouse_repeat_ms_clamps_env_var() {
// #14: serial guard. cargo test runs unit tests in parallel
// by default; without this mutex a concurrent test that
// also touches ENBOXER_MOUSE_REPEAT_MS would race our
// reads. A static Mutex<()> is the standard way to
// serialise env-var access in unit tests.
static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
let _guard = ENV_LOCK.lock().unwrap_or_else(|p| p.into_inner());
// The default cadence is 50 ms when the env var is unset
// or malformed; explicit values in [1, 2000] pass through;
// out-of-range values fall back to 50. We assert the bounds