From 0906e86594410de715f5f39bf80fa61c40b987d9 Mon Sep 17 00:00:00 2001 From: en Date: Wed, 16 Sep 2026 17:33:25 +0200 Subject: [PATCH] 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. --- src/session.rs | 67 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/src/session.rs b/src/session.rs index 545dab1..39fb1b4 100644 --- a/src/session.rs +++ b/src/session.rs @@ -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 = 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 { /// 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>, 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>, 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