diff --git a/crates/openlogi-agent-core/src/device_order.rs b/crates/openlogi-agent-core/src/device_order.rs index 8412b89c..6eae0338 100644 --- a/crates/openlogi-agent-core/src/device_order.rs +++ b/crates/openlogi-agent-core/src/device_order.rs @@ -9,10 +9,17 @@ use openlogi_hid::DeviceRoute; -/// A stable, route-derived identity used to order devices deterministically. -/// Distinct devices never share one (a Bolt receiver UID + slot, a direct -/// vendor/product + serial/unit, or a slot + serial/unit are each unique), so -/// it alone fixes the sort order regardless of secondary tiebreakers. +/// A configuration key backed by enough information to identify one physical +/// device across inventory snapshots and process restarts. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct PhysicalDeviceKey(String); + +/// A route-derived identity used to order devices deterministically. +/// +/// Receiver UID + slot and direct serial/non-zero unit identities are stable +/// and unique. A direct all-zero unit identity is deliberately retained here +/// only so a transient inventory record can still be ordered; it cannot become +/// a [`PhysicalDeviceKey`]. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub enum DeviceStableId { Bolt { @@ -41,9 +48,11 @@ impl DeviceIdentity { /// Prefer the serial number (case-folded) when present, else the unit id. #[must_use] pub fn from_parts(serial: Option<&str>, unit_id: [u8; 4]) -> Self { - serial.map_or(Self::Unit(unit_id), |s| { - Self::Serial(s.to_ascii_lowercase()) - }) + serial + .filter(|serial| !serial.is_empty()) + .map_or(Self::Unit(unit_id), |serial| { + Self::Serial(serial.to_ascii_lowercase()) + }) } } @@ -81,13 +90,18 @@ impl DeviceStableId { } } - /// Stable key for persisted per-physical-device configuration. + /// Route-derived key used while a device is present in a runtime snapshot. + /// + /// Unlike [`Self::physical_key`], this also represents a direct or + /// routeless device whose only reported identity is an all-zero unit id. + /// Such a key is suitable for short-lived UI bookkeeping only and must not + /// be written to configuration. /// /// This intentionally keys receiver-connected devices by receiver UID + /// pairing slot rather than model id, so two identical mice paired to the /// same receiver can carry different settings. #[must_use] - pub fn config_key(&self) -> String { + pub fn runtime_key(&self) -> String { match self { Self::Bolt { receiver_uid, slot } => format!("receiver:{receiver_uid}:slot:{slot}"), Self::Direct { @@ -98,9 +112,32 @@ impl DeviceStableId { Self::Unknown { slot, identity } => format!("unknown:slot:{slot}:{}", identity.key()), } } + + /// Stable key for persisted per-physical-device configuration. + /// + /// Receiver-connected devices are identified by receiver UID + pairing + /// slot. Direct and routeless devices require either a non-empty serial + /// number or a non-zero unit id; an all-zero unit id is a transient probe + /// result, not a physical identity. + #[must_use] + pub fn physical_key(&self) -> Option { + match self { + Self::Bolt { .. } => Some(PhysicalDeviceKey(self.runtime_key())), + Self::Direct { identity, .. } | Self::Unknown { identity, .. } => identity + .is_physical() + .then(|| PhysicalDeviceKey(self.runtime_key())), + } + } } impl DeviceIdentity { + fn is_physical(&self) -> bool { + match self { + Self::Serial(serial) => !serial.is_empty(), + Self::Unit(unit) => *unit != [0; 4], + } + } + fn key(&self) -> String { match self { Self::Serial(serial) => format!("serial:{serial}"), @@ -109,6 +146,81 @@ impl DeviceIdentity { } } +impl PhysicalDeviceKey { + /// Parse a key emitted by [`DeviceStableId::physical_key`]. + /// + /// Legacy model-scoped configuration keys intentionally return `None`; + /// callers can use that distinction when applying compatibility behavior + /// without treating a model identifier as a physical identity. + #[must_use] + pub fn parse(value: &str) -> Option { + if receiver_key_is_valid(value) + || direct_identity_fragment(value).is_some_and(identity_fragment_is_physical) + || unknown_identity_fragment(value).is_some_and(identity_fragment_is_physical) + { + Some(Self(value.to_string())) + } else { + None + } + } + + /// Whether `value` is a structurally valid runtime key whose only device + /// identity is the all-zero unit id. + #[must_use] + pub fn is_transient(value: &str) -> bool { + direct_identity_fragment(value) + .or_else(|| unknown_identity_fragment(value)) + .is_some_and(|identity| identity == "unit:00000000") + } + + /// Borrow the serialized configuration key. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } + + /// Consume the wrapper and return its serialized configuration key. + #[must_use] + pub fn into_string(self) -> String { + self.0 + } +} + +fn receiver_key_is_valid(value: &str) -> bool { + value + .strip_prefix("receiver:") + .and_then(|rest| rest.rsplit_once(":slot:")) + .is_some_and(|(receiver_uid, slot)| !receiver_uid.is_empty() && slot.parse::().is_ok()) +} + +fn direct_identity_fragment(value: &str) -> Option<&str> { + let mut parts = value.strip_prefix("direct:")?.splitn(3, ':'); + let vendor_id = parts.next()?; + let product_id = parts.next()?; + let identity = parts.next()?; + (is_hex_word(vendor_id) && is_hex_word(product_id)).then_some(identity) +} + +fn unknown_identity_fragment(value: &str) -> Option<&str> { + let (slot, identity) = value.strip_prefix("unknown:slot:")?.split_once(':')?; + slot.parse::().ok().map(|_| identity) +} + +fn identity_fragment_is_physical(value: &str) -> bool { + value + .strip_prefix("serial:") + .is_some_and(|serial| !serial.is_empty()) + || value.strip_prefix("unit:").is_some_and(|unit| { + unit.len() == 8 + && unit.bytes().all(|byte| byte.is_ascii_hexdigit()) + && unit != "00000000" + }) +} + +fn is_hex_word(value: &str) -> bool { + value.len() == 4 && value.bytes().all(|byte| byte.is_ascii_hexdigit()) +} + fn hex_unit(unit: [u8; 4]) -> String { format!( "{:02x}{:02x}{:02x}{:02x}", @@ -120,7 +232,7 @@ fn hex_unit(unit: [u8; 4]) -> String { mod tests { use openlogi_hid::DeviceRoute; - use super::DeviceStableId; + use super::{DeviceStableId, PhysicalDeviceKey}; #[test] fn unifying_route_maps_to_bolt_stable_id() { @@ -162,8 +274,57 @@ mod tests { }; assert_eq!( - DeviceStableId::from_parts(Some(&route), 2, Some("SERIAL"), [1, 2, 3, 4]).config_key(), - "receiver:aabb:slot:2" + DeviceStableId::from_parts(Some(&route), 2, Some("SERIAL"), [1, 2, 3, 4]) + .physical_key() + .map(PhysicalDeviceKey::into_string), + Some("receiver:aabb:slot:2".to_string()) ); } + + #[test] + fn zero_unit_direct_identity_is_transient() { + let route = DeviceRoute::Direct { + vendor_id: 0x046d, + product_id: 0xb023, + }; + let id = DeviceStableId::from_parts(Some(&route), 0xff, None, [0; 4]); + + assert!(id.physical_key().is_none()); + assert_eq!(id.runtime_key(), "direct:046d:b023:unit:00000000"); + assert!(PhysicalDeviceKey::is_transient(&id.runtime_key())); + assert!(PhysicalDeviceKey::parse(&id.runtime_key()).is_none()); + } + + #[test] + fn serial_identity_is_physical_when_unit_is_zero() { + let route = DeviceRoute::Direct { + vendor_id: 0x046d, + product_id: 0xb023, + }; + let key = DeviceStableId::from_parts(Some(&route), 0xff, Some("ABCDEF"), [0; 4]) + .physical_key() + .map(PhysicalDeviceKey::into_string); + + assert_eq!(key, Some("direct:046d:b023:serial:abcdef".to_string())); + } + + #[test] + fn nonzero_unit_identity_is_physical_without_serial() { + let route = DeviceRoute::Direct { + vendor_id: 0x046d, + product_id: 0xb023, + }; + let key = DeviceStableId::from_parts(Some(&route), 0xff, None, [0xa3, 0x93, 0xca, 0xe0]) + .physical_key() + .map(PhysicalDeviceKey::into_string); + + assert_eq!(key, Some("direct:046d:b023:unit:a393cae0".to_string())); + } + + #[test] + fn parser_distinguishes_physical_keys_from_legacy_model_keys() { + assert!(PhysicalDeviceKey::parse("receiver:d0289db2:slot:1").is_some()); + assert!(PhysicalDeviceKey::parse("direct:046d:b023:unit:a393cae0").is_some()); + assert!(PhysicalDeviceKey::parse("2b034").is_none()); + } } diff --git a/crates/openlogi-agent-core/src/orchestrator.rs b/crates/openlogi-agent-core/src/orchestrator.rs index a3c26fdd..eb30ed6e 100644 --- a/crates/openlogi-agent-core/src/orchestrator.rs +++ b/crates/openlogi-agent-core/src/orchestrator.rs @@ -401,8 +401,11 @@ fn build_devices(inventories: &[DeviceInventory]) -> Vec { model.serial_number.as_deref(), model.unit_id, ); + let Some(config_key) = stable_id.physical_key() else { + continue; + }; devices.push(AgentDevice { - config_key: stable_id.config_key(), + config_key: config_key.into_string(), model_key: model.config_key(), route, slot: paired.slot, @@ -518,12 +521,15 @@ fn write_value(lock: &RwLock, value: T, name: &str) { #[cfg(test)] mod tests { use super::{ - AgentDevice, InventoryHealth, Orchestrator, configured_wheel_mode, plan_reapply, - reapply_targets, + AgentDevice, InventoryHealth, Orchestrator, build_devices, configured_wheel_mode, + plan_reapply, reapply_targets, }; use openlogi_core::config::{Config, ScrollResolution}; - use openlogi_core::device::Capabilities; - use openlogi_hid::DeviceRoute; + use openlogi_core::device::{ + Capabilities, DeviceInventory, DeviceKind, DeviceModelInfo, DeviceTransports, PairedDevice, + ReceiverInfo, + }; + use openlogi_hid::{DIRECT_DEVICE_INDEX, DeviceRoute}; fn dev(key: &str, slot: u8, online: bool) -> AgentDevice { AgentDevice { @@ -541,6 +547,43 @@ mod tests { } } + fn direct_inventory(serial_number: Option<&str>, unit_id: [u8; 4]) -> DeviceInventory { + DeviceInventory { + receiver: ReceiverInfo { + name: "MX Master 3S".to_string(), + vendor_id: 0x046d, + product_id: 0xb023, + unique_id: None, + }, + paired: vec![PairedDevice { + slot: DIRECT_DEVICE_INDEX, + codename: Some("MX Master 3S".to_string()), + wpid: None, + kind: DeviceKind::Mouse, + online: true, + battery: None, + model_info: Some(DeviceModelInfo { + entity_count: 1, + serial_number: serial_number.map(str::to_string), + unit_id, + transports: DeviceTransports::default(), + model_ids: [0xb034, 0, 0], + extended_model_id: 2, + }), + capabilities: Some(Capabilities::presumed_from_kind(DeviceKind::Mouse)), + }], + } + } + + #[test] + fn build_devices_skips_transient_zero_unit_direct_identity() { + assert!(build_devices(&[direct_inventory(None, [0; 4])]).is_empty()); + + let devices = build_devices(&[direct_inventory(Some("ABC123"), [0; 4])]); + assert_eq!(devices.len(), 1); + assert_eq!(devices[0].config_key, "direct:046d:b023:serial:abc123"); + } + #[test] fn configured_wheel_mode_gates_resolution_and_inversion_independently() { let mut config = Config::default(); diff --git a/crates/openlogi-gui/src/app.rs b/crates/openlogi-gui/src/app.rs index bd6d2e2a..aa0fcdbf 100644 --- a/crates/openlogi-gui/src/app.rs +++ b/crates/openlogi-gui/src/app.rs @@ -554,6 +554,7 @@ mod tests { fn record(kind: DeviceKind, capabilities: Option) -> DeviceRecord { DeviceRecord { config_key: "test".to_string(), + persistent: true, model_key: "test".to_string(), display_name: "Test".to_string(), asset: None, diff --git a/crates/openlogi-gui/src/state.rs b/crates/openlogi-gui/src/state.rs index c6a74711..6440fe3b 100644 --- a/crates/openlogi-gui/src/state.rs +++ b/crates/openlogi-gui/src/state.rs @@ -34,6 +34,7 @@ use crate::asset::AssetResolver; use crate::data::mouse_buttons::{Action, Binding, ButtonId, GestureDirection}; use crate::state::devices::{build_device_list, pick_initial_device, sort_device_list}; use openlogi_agent_core::bindings::{bindings_for, gesture_bindings_for}; +use openlogi_agent_core::device_order::PhysicalDeviceKey; /// Default DPI value applied to a fresh AppState. Matches a common Logitech /// mid-range mouse and keeps the dot-preview visually obvious from frame one. @@ -452,6 +453,14 @@ impl AppState { continue; } + // An all-zero direct unit id is only a transient probe result. If + // the next snapshot resolves a physical serial/unit key, retaining + // this record through the normal miss grace would show both cards. + if !previous.is_persistent() { + self.inventory_misses.remove(&previous.config_key); + continue; + } + let misses = self .inventory_misses .entry(previous.config_key.clone()) @@ -506,8 +515,15 @@ impl AppState { self.dpi = self.dpi_for_current(); self.button_bindings = self.bindings_for_current(); self.gesture_bindings = self.gesture_bindings_for_current(); - let key = self.current_record().map(|r| r.config_key.clone()); - self.config.set_selected_device(key); + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { + debug!("transient device selection not persisted"); + return; + }; + self.config.set_selected_device(Some(key)); // The agent owns the hook + device I/O; have it switch devices too. self.persist_and_reload("selected device"); } @@ -521,8 +537,12 @@ impl AppState { /// No-op when no device is selected (binding panel won't expose the /// editor in that state). pub fn commit_dpi_presets(&mut self, presets: Vec) { - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { - debug!("no active device key — DPI presets kept in memory only"); + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { + debug!("no persistent device key — DPI presets kept in memory only"); return; }; self.config.set_dpi_presets(&key, presets); @@ -534,7 +554,8 @@ impl AppState { #[must_use] pub fn dpi_presets(&self) -> Vec { self.current_record() - .map(|r| self.config.dpi_presets(&r.config_key)) + .and_then(DeviceRecord::persistent_config_key) + .map(|key| self.config.dpi_presets(key)) .unwrap_or_default() } @@ -742,6 +763,7 @@ impl AppState { return; }; let key = record.config_key.clone(); + let persistent_key = record.persistent_config_key().map(str::to_string); let route = record.route.clone(); if let Some(route) = route { self.send_ipc(crate::ipc_client::Command::SetSmartShift( @@ -751,15 +773,17 @@ impl AppState { tunable_torque, )); } - self.config.set_smartshift( - &key, - openlogi_core::config::SmartShift { - mode: mode.into(), - auto_disengage, - tunable_torque, - }, - ); - self.persist_and_reload("SmartShift"); + if let Some(persistent_key) = persistent_key { + self.config.set_smartshift( + &persistent_key, + openlogi_core::config::SmartShift { + mode: mode.into(), + auto_disengage, + tunable_torque, + }, + ); + self.persist_and_reload("SmartShift"); + } // Reflect the write immediately so the panel doesn't flicker back to // the previous value before a re-read lands, but queue a confirming // re-read: the write is fire-and-forget, so a sleeping device that @@ -781,7 +805,8 @@ impl AppState { #[must_use] pub fn current_invert_scroll(&self) -> bool { self.current_record() - .is_some_and(|r| self.config.invert_scroll(&r.config_key)) + .and_then(DeviceRecord::persistent_config_key) + .is_some_and(|key| self.config.invert_scroll(key)) } /// Whether the active device reports native HID++ wheel inversion support. @@ -800,8 +825,12 @@ impl AppState { debug!("active device does not support native scroll inversion"); return; } - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { - debug!("no active device — invert-scroll change ignored"); + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { + debug!("no persistent device key — invert-scroll change ignored"); return; }; self.config.set_invert_scroll(&key, invert); @@ -813,7 +842,8 @@ impl AppState { #[must_use] pub fn current_scroll_resolution(&self) -> Option { self.current_record() - .and_then(|record| self.config.scroll_resolution(&record.config_key)) + .and_then(DeviceRecord::persistent_config_key) + .and_then(|key| self.config.scroll_resolution(key)) } /// Whether the active device exposes HID++ `0x2121 HiResWheel`. @@ -831,15 +861,16 @@ impl AppState { &mut self, resolution: Option, ) { - let Some((key, supported)) = self.current_record().map(|record| { - ( - record.config_key.clone(), + let Some((key, supported)) = self.current_record().and_then(|record| { + let key = record.persistent_config_key()?.to_string(); + Some(( + key, record .capabilities .is_some_and(|capabilities| capabilities.hires_wheel), - ) + )) }) else { - debug!("no active device — wheel-resolution change ignored"); + debug!("no persistent device key — wheel-resolution change ignored"); return; }; if !set_scroll_resolution_if_supported(&mut self.config, &key, supported, resolution) { @@ -866,30 +897,44 @@ impl AppState { #[must_use] pub fn lighting(&self) -> Lighting { self.current_record() - .and_then(|r| self.config.lighting(&r.config_key)) + .and_then(DeviceRecord::persistent_config_key) + .and_then(|key| self.config.lighting(key)) .unwrap_or_default() } /// The stored lighting config for `key`, or `None` when unset. #[must_use] pub fn lighting_for(&self, key: &str) -> Option { + if PhysicalDeviceKey::is_transient(key) + || self + .device_list + .iter() + .any(|record| record.config_key == key && !record.is_persistent()) + { + return None; + } self.config.lighting(key) } /// Persist a new lighting config for the active device and push it to the /// hardware (best-effort). No-op when no device is selected. pub fn commit_lighting(&mut self, lighting: Lighting) { - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { - debug!("no active device key — lighting kept in memory only"); + let Some(record) = self.current_record() else { + debug!("no active device — lighting change ignored"); return; }; - let target = self.current_record().and_then(|r| r.route.clone()); + let key = record.persistent_config_key().map(str::to_string); + let target = record.route.clone(); if let Some(route) = target { self.send_ipc(crate::ipc_client::Command::SetLighting( route, lighting.clone(), )); } + let Some(key) = key else { + debug!("transient device lighting applied without persistence"); + return; + }; self.config.set_lighting(&key, lighting); // Keep the agent's config copy fresh: it re-applies the saved colour // when the keyboard reconnects, and without the reload it would @@ -908,12 +953,17 @@ impl AppState { return; }; let key = record.config_key.clone(); + let persistent_key = record.persistent_config_key().map(str::to_string); let route = record.route.clone(); if let Some(route) = route { self.send_ipc(crate::ipc_client::Command::SetDpi(route, dpi)); } - self.config.set_dpi(&key, dpi); - self.persist_and_reload("DPI"); + if let Some(persistent_key) = persistent_key { + self.config.set_dpi(&persistent_key, dpi); + self.persist_and_reload("DPI"); + } else { + debug!(key, "transient device DPI applied without persistence"); + } } /// App-wide settings backing the Settings window (launch-at-login, @@ -1109,10 +1159,14 @@ impl AppState { pub fn commit_binding(&mut self, button: ButtonId, action: Action) { self.button_bindings.insert(button, action.clone()); - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { debug!( ?button, - "no active device key — binding kept in memory only" + "no persistent device key — binding kept in memory only" ); return; }; @@ -1125,13 +1179,17 @@ impl AppState { fn bindings_for_current(&self) -> BTreeMap { bindings_for( &self.config, - self.current_record().map(|r| r.config_key.as_str()), + self.current_record() + .and_then(DeviceRecord::persistent_config_key), self.current_app_bundle.as_deref(), ) } fn gesture_bindings_for_current(&self) -> BTreeMap { - let Some(key) = self.current_record().map(|r| r.config_key.as_str()) else { + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + else { return BTreeMap::new(); }; match self.config.gesture_owner(key) { @@ -1153,7 +1211,7 @@ impl AppState { /// the gesture menu rather than the single-action picker. #[must_use] pub fn current_gesture_owner(&self) -> Option { - let key = self.current_record()?.config_key.as_str(); + let key = self.current_record()?.persistent_config_key()?; self.config.gesture_owner(key) } @@ -1161,7 +1219,11 @@ impl AppState { /// `None`), enforcing the one-gesture-button-per-device lock. Persists, tells /// the agent to rebuild, and refreshes the projected maps the UI reads. pub fn commit_gesture_owner(&mut self, button: Option) { - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { return; }; match button { @@ -1181,10 +1243,14 @@ impl AppState { /// Update a single gesture-button sub-binding in memory, on disk, and in the /// shared gesture map the watcher thread reads. pub fn commit_gesture_binding(&mut self, direction: GestureDirection, action: Action) { - let Some(key) = self.current_record().map(|r| r.config_key.clone()) else { + let Some(key) = self + .current_record() + .and_then(DeviceRecord::persistent_config_key) + .map(str::to_string) + else { debug!( ?direction, - "no active device key — gesture binding edit ignored" + "no persistent device key — gesture binding edit ignored" ); return; }; @@ -1223,6 +1289,9 @@ fn persist_identities(config: &mut Config, list: &[DeviceRecord]) { if !record.online { continue; } + let Some(config_key) = record.persistent_config_key() else { + continue; + }; let Some(capabilities) = record.capabilities else { continue; }; @@ -1237,8 +1306,8 @@ fn persist_identities(config: &mut Config, list: &[DeviceRecord]) { }), codename: record.codename.clone(), }; - if config.device_identity(&record.config_key) != Some(&identity) { - config.set_device_identity(&record.config_key, identity); + if config.device_identity(config_key) != Some(&identity) { + config.set_device_identity(config_key, identity); changed = true; } } @@ -1281,12 +1350,82 @@ impl Global for AppState {} #[cfg(test)] mod tests { - use openlogi_core::config::{Config, DeviceIdentity, ScrollResolution}; - use openlogi_core::device::{Capabilities, DeviceKind, DeviceModelInfo, DeviceTransports}; + use openlogi_core::config::{Config, DeviceIdentity, Lighting, ScrollResolution}; + use openlogi_core::device::{ + Capabilities, DeviceInventory, DeviceKind, DeviceModelInfo, DeviceTransports, PairedDevice, + ReceiverInfo, + }; use crate::asset::AssetResolver; - use super::{AppState, set_scroll_resolution_if_supported}; + use super::{AppState, build_device_list, set_scroll_resolution_if_supported}; + + fn direct_inventory(unit_id: [u8; 4]) -> DeviceInventory { + DeviceInventory { + receiver: ReceiverInfo { + name: "MX Master 3S".to_string(), + vendor_id: 0x046d, + product_id: 0xb023, + unique_id: None, + }, + paired: vec![PairedDevice { + slot: openlogi_hid::DIRECT_DEVICE_INDEX, + codename: Some("MX Master 3S".to_string()), + wpid: None, + kind: DeviceKind::Mouse, + online: true, + battery: None, + model_info: Some(DeviceModelInfo { + entity_count: 1, + serial_number: None, + unit_id, + transports: DeviceTransports::default(), + model_ids: [0xb034, 0, 0], + extended_model_id: 2, + }), + capabilities: Some(Capabilities::presumed_from_kind(DeviceKind::Mouse)), + }], + } + } + + #[test] + fn transient_identity_is_not_persisted_or_retained_after_resolution() { + let cache = AssetResolver::new(); + let transient_inventory = direct_inventory([0; 4]); + let (commands, _receiver) = tokio::sync::mpsc::unbounded_channel(); + let mut state = + AppState::with_runtime(Config::default(), &[transient_inventory], &cache, commands); + let transient_key = "direct:046d:b023:unit:00000000"; + + assert_eq!(state.device_list.len(), 1); + assert!(state.config.device_identity(transient_key).is_none()); + state.commit_dpi(2400); + assert!(state.config.dpi(transient_key).is_none()); + + let stable_list = build_device_list( + &[direct_inventory([0xa3, 0x93, 0xca, 0xe0])], + &cache, + &state.config, + ); + let merged = state.merge_inventory_snapshot(stable_list); + + assert_eq!(merged.len(), 1); + assert_eq!(merged[0].config_key, "direct:046d:b023:unit:a393cae0"); + assert!(merged[0].is_persistent()); + } + + #[test] + fn historical_transient_lighting_is_not_exposed_without_a_live_record() { + let transient_key = "direct:046d:b023:unit:00000000"; + let mut config = Config::default(); + config.set_lighting(transient_key, Lighting::default()); + assert!(config.lighting(transient_key).is_some()); + let (commands, _receiver) = tokio::sync::mpsc::unbounded_channel(); + let state = AppState::with_runtime(config, &[], &AssetResolver::new(), commands); + + assert!(state.device_list.is_empty()); + assert!(state.lighting_for(transient_key).is_none()); + } #[test] fn known_offline_device_is_an_asset_sync_target() { diff --git a/crates/openlogi-gui/src/state/devices.rs b/crates/openlogi-gui/src/state/devices.rs index 09fd68dc..d67cfac3 100644 --- a/crates/openlogi-gui/src/state/devices.rs +++ b/crates/openlogi-gui/src/state/devices.rs @@ -2,7 +2,7 @@ use std::collections::HashSet; -use openlogi_agent_core::device_order::DeviceStableId; +use openlogi_agent_core::device_order::{DeviceStableId, PhysicalDeviceKey}; use openlogi_core::config::{Config, DeviceIdentity}; use openlogi_core::device::{ BatteryInfo, Capabilities, DeviceInventory, DeviceKind, DeviceModelInfo, DeviceTransports, @@ -24,8 +24,12 @@ use crate::asset::{AssetResolver, ResolvedAsset}; /// with [`super::AppState::current_device`]. #[derive(Debug, Clone)] pub struct DeviceRecord { - /// Stable physical-device key used for persisted settings. + /// Route-derived key used for runtime state and, when [`Self::persistent`] + /// is true, persisted settings. pub config_key: String, + /// Whether `config_key` identifies one physical device and may be written + /// to configuration. False for a direct/routeless all-zero unit identity. + pub(crate) persistent: bool, /// Stable model key used only for asset/model lookup and diagnostics. pub model_key: String, pub display_name: String, @@ -47,6 +51,19 @@ pub struct DeviceRecord { pub battery: Option, } +impl DeviceRecord { + /// Return the configuration key only when it identifies one physical + /// device and is therefore safe to persist. + pub(super) fn persistent_config_key(&self) -> Option<&str> { + self.persistent.then_some(self.config_key.as_str()) + } + + /// Whether this record may participate in persistent configuration. + pub(super) fn is_persistent(&self) -> bool { + self.persistent + } +} + /// Build the carousel's device list as the **union** of the live inventory and /// the persisted set of devices we've seen before. /// @@ -58,8 +75,9 @@ pub struct DeviceRecord { /// known device (with its Pointer/Buttons panels) is always shown, and the live /// probe only *enriches* it (online state, battery, asset photo) rather than /// *gating* whether it appears at all. See issue #159. Placeholders that are -/// unreachable (their receiver is unplugged) or duplicate a visible same-model -/// card are suppressed — see [`append_offline_known`] (#271/#280). +/// unreachable (their receiver is unplugged), structurally transient, or a +/// legacy same-model duplicate are suppressed — see [`append_offline_known`] +/// (#271/#280/#387). pub(super) fn build_device_list( inventories: &[DeviceInventory], cache: &AssetResolver, @@ -91,13 +109,16 @@ pub(super) fn build_device_list( ); (key, None, None, paired.codename.clone(), None, [0u8; 4]) }; - let config_key = DeviceStableId::from_parts( + let stable_id = DeviceStableId::from_parts( route.as_ref(), paired.slot, serial_number.as_deref(), unit_id, - ) - .config_key(); + ); + let (config_key, persistent) = stable_id.physical_key().map_or_else( + || (stable_id.runtime_key(), false), + |key| (key.into_string(), true), + ); let display_name = asset .as_ref() @@ -107,6 +128,7 @@ pub(super) fn build_device_list( let kind = effective_kind(paired.kind, asset.as_ref().map(|a| a.kind)); list.push(DeviceRecord { config_key, + persistent, model_key, display_name, asset, @@ -143,51 +165,69 @@ pub(super) fn build_device_list( } /// Append an offline placeholder for every known device not already present in -/// `list`, skipping unreachable devices and duplicates of a visible one. +/// `list`, skipping unreachable devices and invalid transient identities. /// -/// Three gates keep phantom cards out (#271/#280): -/// - an exact key/model match against a live record — the device is already +/// The gates keep phantom cards out without conflating model identity with +/// physical identity: +/// - an exact physical key match against a live record — the device is already /// in the list; /// - a `receiver:` key whose receiver is not plugged in — its paired devices /// are unreachable until that receiver returns (e.g. the work receiver's /// mouse while at home); -/// - a wire PID already visible live or as an earlier placeholder — two units -/// of one model render as identical cards, so a second one only confuses. -/// The PID comparison also absorbs the flaky extended-model byte that made -/// `0b034` and `2b034` read as different models in #271. +/// - a historical direct/routeless all-zero unit key, which never identified a +/// physical device; +/// - for legacy model-scoped keys only, a model/PID already visible live or as +/// an earlier placeholder. This preserves the #271/#280 compatibility fix +/// without hiding a second physical device of the same model. fn append_offline_known<'a>( list: &mut Vec, known: impl Iterator, cache: &AssetResolver, present_receivers: &HashSet, ) { - let mut blocked_keys: HashSet = list + let mut present_keys: HashSet = list .iter() - .flat_map(|r| [r.config_key.clone(), r.model_key.clone()]) + .map(|record| record.config_key.clone()) .collect(); - let mut blocked_pids: HashSet = list.iter().filter_map(record_wire_pid).collect(); + let mut blocked_legacy_models: HashSet = + list.iter().map(|record| record.model_key.clone()).collect(); + let mut blocked_legacy_pids: HashSet = + list.iter().filter_map(record_wire_pid).collect(); let mut known = known.collect::>(); known.sort_by_key(|(key, identity)| (identity.model_info.is_none(), (*key).to_string())); for (key, identity) in known { + if PhysicalDeviceKey::is_transient(key) { + continue; + } if receiver_uid_of(key).is_some_and(|uid| !present_receivers.contains(&uid)) { continue; } + if present_keys.contains(key) { + continue; + } + let is_legacy_model_key = PhysicalDeviceKey::parse(key).is_none(); let model_key = identity .model_info .as_ref() .map_or_else(|| key.to_string(), DeviceModelInfo::config_key); - if blocked_keys.contains(key) || blocked_keys.contains(&model_key) { + if is_legacy_model_key && blocked_legacy_models.contains(&model_key) { continue; } let record = offline_record(key, identity, cache); - if let Some(pid) = record_wire_pid(&record) - && !blocked_pids.insert(pid) + let wire_pid = record_wire_pid(&record); + if is_legacy_model_key + && wire_pid + .as_ref() + .is_some_and(|pid| blocked_legacy_pids.contains(pid)) { continue; } - blocked_keys.insert(record.config_key.clone()); - blocked_keys.insert(record.model_key.clone()); + present_keys.insert(record.config_key.clone()); + blocked_legacy_models.insert(record.model_key.clone()); + if let Some(pid) = wire_pid { + blocked_legacy_pids.insert(pid); + } list.push(record); } } @@ -199,7 +239,8 @@ fn receiver_uid_of(key: &str) -> Option { .map(str::to_ascii_lowercase) } -/// The record's wire product id, used to suppress same-model duplicate cards. +/// The record's wire product id, used to suppress legacy same-model duplicate +/// cards without conflating physical device keys. fn record_wire_pid(record: &DeviceRecord) -> Option { match record.model_info.as_ref().map(|m| m.model_ids[0]) { Some(pid) if pid != 0 => Some(format!("{pid:04x}")), @@ -238,6 +279,7 @@ fn offline_record( .map_or_else(|| config_key.to_string(), DeviceModelInfo::config_key); DeviceRecord { config_key: config_key.to_string(), + persistent: true, model_key, display_name: identity.display_name.clone(), asset, @@ -300,6 +342,7 @@ fn device_order_key(record: &DeviceRecord) -> (DeviceStableId, String, String) { fn demo_keyboard() -> DeviceRecord { DeviceRecord { config_key: "demo-g513".to_string(), + persistent: true, model_key: "demo-g513".to_string(), display_name: "Logitech G513".to_string(), asset: None, @@ -355,7 +398,10 @@ fn effective_kind(hid_kind: DeviceKind, asset_kind: Option) -> Devic pub(super) fn pick_initial_device(list: &[DeviceRecord], saved: Option<&str>) -> usize { saved - .and_then(|key| list.iter().position(|r| r.config_key == key)) + .and_then(|key| { + list.iter() + .position(|record| record.is_persistent() && record.config_key == key) + }) .unwrap_or(0) } @@ -396,6 +442,7 @@ mod tests { use super::{ Capabilities, DeviceIdentity, DeviceKind, DeviceModelInfo, DeviceRecord, DeviceTransports, append_offline_known, build_device_list, effective_kind, offline_record, + pick_initial_device, }; fn paired_device_no_model_info(slot: u8, wpid: Option) -> PairedDevice { @@ -423,9 +470,31 @@ mod tests { } } + fn direct_inventory(model_info: DeviceModelInfo) -> DeviceInventory { + DeviceInventory { + receiver: ReceiverInfo { + name: "MX Master 3S".into(), + vendor_id: 0x046d, + product_id: 0xb023, + unique_id: None, + }, + paired: vec![PairedDevice { + slot: openlogi_hid::DIRECT_DEVICE_INDEX, + codename: Some("MX Master 3S".into()), + wpid: None, + kind: DeviceKind::Mouse, + online: true, + battery: None, + model_info: Some(model_info), + capabilities: Some(Capabilities::presumed_from_kind(DeviceKind::Mouse)), + }], + } + } + fn online_record(key: &str) -> DeviceRecord { DeviceRecord { config_key: key.to_string(), + persistent: true, model_key: key.to_string(), display_name: format!("live {key}"), asset: None, @@ -541,6 +610,72 @@ mod tests { } } + #[test] + fn zero_unit_direct_inventory_is_transient() { + let cache = AssetResolver::new(); + let list = build_device_list( + &[direct_inventory(model_info(2, 0xb034))], + &cache, + &Config::default(), + ); + + assert_eq!(list.len(), 1); + assert_eq!(list[0].config_key, "direct:046d:b023:unit:00000000"); + assert!(!list[0].is_persistent()); + assert!(list[0].persistent_config_key().is_none()); + } + + #[test] + fn historical_zero_unit_identity_does_not_create_offline_card() { + let id = mouse_identity("MX Master 3S"); + let cache = AssetResolver::new(); + let mut list = Vec::new(); + + append_offline_known( + &mut list, + [("direct:046d:b023:unit:00000000", &id)].into_iter(), + &cache, + &HashSet::new(), + ); + + assert!(list.is_empty()); + } + + #[test] + fn same_model_physical_bluetooth_devices_remain_distinct() { + let mut id_a = mouse_identity("MX Master 3S"); + id_a.model_info = Some(model_info(2, 0xb034)); + let id_b = id_a.clone(); + let cache = AssetResolver::new(); + let mut list = Vec::new(); + + append_offline_known( + &mut list, + [ + ("direct:046d:b023:unit:01020304", &id_a), + ("direct:046d:b023:unit:05060708", &id_b), + ] + .into_iter(), + &cache, + &HashSet::new(), + ); + + assert_eq!(list.len(), 2); + } + + #[test] + fn persisted_selection_does_not_target_transient_identity() { + let stable = online_record("receiver:aabb:slot:1"); + let mut transient = online_record("direct:046d:b023:unit:00000000"); + transient.persistent = false; + let list = vec![stable, transient]; + + assert_eq!( + pick_initial_device(&list, Some("direct:046d:b023:unit:00000000")), + 0 + ); + } + #[test] fn placeholders_for_absent_receivers_are_hidden() { // The work receiver's mouse must not haunt the list at home: with its @@ -585,7 +720,7 @@ mod tests { } #[test] - fn same_model_placeholders_collapse_to_one_card() { + fn legacy_same_model_placeholders_collapse_to_one_card() { // Two persisted identities of one model render identically — a second // offline card carries no information, only confusion. let id_a = mouse_identity("MX Master 3S");