diff --git a/src/api.rs b/src/api.rs index cf70ed8..9b12a8b 100644 --- a/src/api.rs +++ b/src/api.rs @@ -307,8 +307,32 @@ async fn set_custom_game_image( Ok(Json(ApiResult { ok: true })) } -async fn local_applications(State(service): State) -> Json> { - Json(service.local_applications().await) +/// A saved local application as the API serves it, with the id its icon is +/// served under. The id is never read back from a saved list. +#[derive(Debug, Serialize)] +#[serde(rename_all = "camelCase")] +struct LocalApplicationEntry { + #[serde(flatten)] + application: LocalApplication, + icon_id: String, +} + +fn local_application_entries(applications: Vec) -> Vec { + applications + .into_iter() + .map(|application| LocalApplicationEntry { + icon_id: crate::applications::icon_id(&application.path), + application, + }) + .collect() +} + +async fn local_applications( + State(service): State, +) -> Json> { + Json(local_application_entries( + service.local_applications().await, + )) } async fn replace_local_applications( @@ -321,7 +345,7 @@ async fn replace_local_applications( .await .map_err(profile_error)?; Ok(Json( - serde_json::json!({"ok":true,"applications":applications}), + serde_json::json!({"ok":true,"applications":local_application_entries(applications)}), )) } @@ -510,6 +534,43 @@ mod tests { (status, bytes, kind) } + #[tokio::test] + async fn saved_local_applications_carry_their_icon_id() { + let (app, dir) = app(); + let path = r"C:\Apps\Editor.exe"; + let icon_id = crate::applications::icon_id(path); + let entry = serde_json::json!({"id":"local:editor","name":"Editor","executable":"Editor.exe","path":path,"kind":"app"}); + let (status, saved, _) = call( + &app, + Method::PUT, + "/v1/local-applications", + "application/json", + serde_json::to_vec(&serde_json::json!({"applications":[entry]})).unwrap(), + ) + .await; + assert_eq!(status, StatusCode::OK); + let saved: serde_json::Value = serde_json::from_slice(&saved).unwrap(); + assert_eq!(saved["applications"][0]["iconId"], icon_id.as_str()); + + let (status, listed, _) = + call(&app, Method::GET, "/v1/local-applications", "", vec![]).await; + assert_eq!(status, StatusCode::OK); + let listed: serde_json::Value = serde_json::from_slice(&listed).unwrap(); + assert_eq!(listed[0]["iconId"], icon_id.as_str()); + + // A client may send back the list it was served, icon ids included. + let (status, ..) = call( + &app, + Method::PUT, + "/v1/local-applications", + "application/json", + serde_json::to_vec(&serde_json::json!({"applications":listed})).unwrap(), + ) + .await; + assert_eq!(status, StatusCode::OK); + std::fs::remove_dir_all(dir).expect("cleanup"); + } + #[tokio::test] async fn a_custom_game_keeps_the_image_saved_for_it() { let (app, dir) = app(); diff --git a/src/applications.rs b/src/applications.rs index 848697b..6e476b3 100644 --- a/src/applications.rs +++ b/src/applications.rs @@ -15,11 +15,7 @@ pub struct ApplicationInfo { pub icon_id: String, } -#[cfg_attr( - not(any(target_os = "windows", target_os = "macos", target_os = "linux")), - allow(dead_code) -)] -fn icon_id(path: &str) -> String { +pub fn icon_id(path: &str) -> String { let mut hasher = DefaultHasher::new(); crate::games::path_key(path).hash(&mut hasher); format!("{:016x}", hasher.finish()) diff --git a/src/hid.rs b/src/hid.rs index 7858658..0e01322 100644 --- a/src/hid.rs +++ b/src/hid.rs @@ -843,6 +843,9 @@ impl HidSession { .get_mut(key) .ok_or_else(|| "the HID interface must be open before listening".to_owned())?; let stop = Arc::new(AtomicBool::new(false)); + // Stored before any reader starts, so a later spawn failure can stop and + // join the readers already running instead of leaving them to block cleanup. + open.stop = Some(Arc::clone(&stop)); for (index, (device, buffer_len, uses_report_ids)) in reader_specs.into_iter().enumerate() { let thread_stop = Arc::clone(&stop); let tx = self.input_tx.clone(); @@ -884,12 +887,16 @@ impl HidSession { } } } - }) - .map_err(|error| format!("could not start the HID input reader: {error}"))?; - open.readers.push(reader); + }); + match reader { + Ok(reader) => open.readers.push(reader), + Err(error) => { + open.stop_reader(); + return Err(format!("could not start the HID input reader: {error}")); + } + } } open.listening = true; - open.stop = Some(stop); Ok(()) } @@ -915,35 +922,20 @@ impl HidSession { .open_devices .get(key) .ok_or_else(|| "the HID interface is not open".to_owned())?; - if open.paths.len() != 1 { - return Err("HID physical identity is ambiguous; refusing report fan-out".into()); + // An open device has several paths only when `inspect_candidate` verified + // they share one serial. A report still goes to exactly one of them. + if open.paths.is_empty() { + return Err("the HID interface has no open path".into()); } let _access = crate::drivers::manual_report_access(open.vendor_id) .map_err(|error| error.to_string())?; let mut frame = Vec::with_capacity(data.len() + 1); frame.push(report_id); frame.extend_from_slice(&data); - let mut sent = false; let mut last_error = None; - let declared_path_exists = open.paths.iter().any(|path| { - let reports = if feature { - &path.layout.feature_bits - } else { - &path.layout.output_bits - }; - reports.contains_key(&report_id) - }); - for path in &open.paths { - if declared_path_exists { - let reports = if feature { - &path.layout.feature_bits - } else { - &path.layout.output_bits - }; - if !reports.contains_key(&report_id) { - continue; - } - } + let layouts: Vec<&ReportLayout> = open.paths.iter().map(|path| &path.layout).collect(); + for index in report_paths(&layouts, report_id, feature) { + let path = &open.paths[index]; let result = path .device .lock() @@ -956,20 +948,18 @@ impl HidSession { } .map_err(|error| error.to_string()) }); + // The first path that takes the report is the only one written, so a + // command never reaches the device twice. match result { - Ok(()) => sent = true, + Ok(()) => return Ok(()), Err(error) => last_error = Some(error), } } - if sent { - Ok(()) - } else { - let operation = if feature { "feature" } else { "output" }; - Err(format!( - "could not send {operation} report {report_id}: {}", - last_error.unwrap_or_else(|| "no HID path was open".to_owned()) - )) - } + let operation = if feature { "feature" } else { "output" }; + Err(format!( + "could not send {operation} report {report_id}: {}", + last_error.unwrap_or_else(|| "no HID path was open".to_owned()) + )) } fn receive_input_report(&self, key: &str, report_id: u8) -> Result, String> { @@ -1113,6 +1103,35 @@ fn device_group_key(info: &hidapi::DeviceInfo) -> Vec { ) } +/// The paths to try for a report, in order: those whose descriptor declares it, +/// or every path when none does (Razer descriptors are not parsed). The sender +/// stops at the first path that takes the report. +fn report_paths(layouts: &[&ReportLayout], report_id: u8, feature: bool) -> Vec { + let declares = |layout: &ReportLayout| { + if feature { + layout.feature_bits.contains_key(&report_id) + } else { + layout.output_bits.contains_key(&report_id) + } + }; + let declared: Vec = (0..layouts.len()) + .filter(|&index| declares(layouts[index])) + .collect(); + if declared.is_empty() { + (0..layouts.len()).collect() + } else { + declared + } +} + +/// True when every interface reports the same non-empty serial. +fn shares_verified_serial<'a>(mut serials: impl Iterator>) -> bool { + match serials.next().flatten().filter(|serial| !serial.is_empty()) { + Some(first) => serials.all(|serial| serial == Some(first)), + None => false, + } +} + fn inspect_candidate( api: &HidApi, key: String, @@ -1129,7 +1148,11 @@ fn inspect_candidate( } } - if unique_paths.len() != 1 { + // Several paths are the collections of one device only when they share a + // verified serial; that is the one case `grouped_device_key` merges them. + if unique_paths.len() > 1 + && !shares_verified_serial(infos.iter().map(hidapi::DeviceInfo::serial_number)) + { return Err("HID physical identity is ambiguous; refusing multi-path access".into()); } @@ -1655,6 +1678,38 @@ mod tests { ); } + #[test] + fn several_paths_open_only_under_one_verified_serial() { + assert!(shares_verified_serial( + [Some("receiver-123"), Some("receiver-123")].into_iter() + )); + assert!(!shares_verified_serial( + [Some("receiver-123"), Some("receiver-456")].into_iter() + )); + assert!(!shares_verified_serial( + [Some("receiver-123"), None].into_iter() + )); + assert!(!shares_verified_serial([None, None].into_iter())); + assert!(!shares_verified_serial([Some(""), Some("")].into_iter())); + assert!(!shares_verified_serial(std::iter::empty())); + } + + #[test] + fn a_report_goes_to_the_paths_that_declare_it_first() { + let mut vendor = ReportLayout::default(); + vendor.feature_bits.insert(0x11, 160); + vendor.output_bits.insert(0x10, 48); + let mouse = ReportLayout::default(); + let mut second = ReportLayout::default(); + second.feature_bits.insert(0x11, 160); + let layouts = [&mouse, &vendor, &second]; + assert_eq!(report_paths(&layouts, 0x11, true), vec![1, 2]); + assert_eq!(report_paths(&layouts, 0x10, false), vec![1]); + // Undeclared (a Razer path without parsed descriptors): every path is a candidate, tried in order. + assert_eq!(report_paths(&layouts, 0x00, true), vec![0, 1, 2]); + assert!(report_paths(&[], 0x11, true).is_empty()); + } + #[test] fn report_payload_never_reads_past_the_buffer() { // A full unnumbered Razer read: hidapi on Windows reports 92 bytes. diff --git a/src/service.rs b/src/service.rs index 6d356c5..d3718eb 100644 --- a/src/service.rs +++ b/src/service.rs @@ -80,14 +80,20 @@ fn profile_applications( /// Drops cached icons for applications that are no longer running, so the /// cache stays the size of the running list instead of every app ever seen. +/// Icons of saved local applications stay, since they are served whether or +/// not the application is running. fn prune_application_icons( icons: &mut HashMap>>, running: &[ApplicationInfo], + local_applications: &[LocalApplication], ) { icons.retain(|icon_id, _| { running .iter() .any(|application| &application.icon_id == icon_id) + || local_applications + .iter() + .any(|target| &applications::icon_id(&target.path) == icon_id) }); } @@ -882,14 +888,43 @@ impl BridgeService { self.inner.read().await.running_applications.clone() } + /// Serves a cached icon, extracting a saved local application's icon from + /// its path the first time it is asked for, even when it is not running. pub async fn application_icon(&self, icon_id: &str) -> Option> { - self.inner - .read() + let path = { + let state = self.inner.read().await; + if let Some(Some(icon)) = state.application_icons.get(icon_id) { + return Some(icon.clone()); + } + state + .config + .local_applications + .iter() + .find(|target| applications::icon_id(&target.path) == icon_id)? + .path + .clone() + }; + let icon = tokio::task::spawn_blocking(move || applications::application_icon(&path)) .await - .application_icons - .get(icon_id) - .cloned() - .flatten() + .ok() + .flatten(); + // A failed extraction is not cached, so a path that comes back later gets + // its icon. The saved list may have changed while the icon was read, so it + // is cached only while a saved application still owns this id. + if let Some(icon) = &icon { + let mut state = self.inner.write().await; + let owned = state + .config + .local_applications + .iter() + .any(|target| applications::icon_id(&target.path) == icon_id); + if owned { + state + .application_icons + .insert(icon_id.to_owned(), Some(icon.clone())); + } + } + icon } pub async fn profiles(&self) -> Vec { @@ -1017,6 +1052,14 @@ impl BridgeService { Ok(()) }) .await?; + // A removed application's icon is not served any more. + let mut guard = self.inner.write().await; + let state = &mut *guard; + prune_application_icons( + &mut state.application_icons, + &state.running_applications, + &state.config.local_applications, + ); Ok(canonical) } @@ -1336,7 +1379,12 @@ impl BridgeService { state.processes = processes; state.applications = applications; state.application_icons.extend(icons); - prune_application_icons(&mut state.application_icons, &running); + let inner = &mut *state; + prune_application_icons( + &mut inner.application_icons, + &running, + &inner.config.local_applications, + ); state.running_applications = running; // Detect a debounced active-profile switch, push the new @@ -2562,12 +2610,90 @@ mod tests { ("closed".to_string(), None), ]); - prune_application_icons(&mut icons, &running); + prune_application_icons(&mut icons, &running, &[]); assert_eq!(icons.len(), 1); assert!(icons.contains_key("running")); } + #[test] + fn a_saved_local_application_keeps_its_icon_while_closed() { + let saved: LocalApplication = serde_json::from_value(serde_json::json!({"id":"local:editor","name":"Editor","executable":"Editor.exe","path":"C:\\Apps\\Editor.exe","kind":"app"})).unwrap(); + let saved_icon = applications::icon_id(&saved.path); + let mut icons = HashMap::from([ + (saved_icon.clone(), Some(vec![1])), + ("closed".to_string(), Some(vec![2])), + ]); + + prune_application_icons(&mut icons, &[], &[saved]); + + assert_eq!(icons.len(), 1); + assert!(icons.contains_key(&saved_icon)); + } + + fn saved_editor() -> LocalApplication { + serde_json::from_value(serde_json::json!({"id":"local:editor","name":"Editor","executable":"Editor.exe","path":"C:\\Missing\\Editor.exe","kind":"app"})).unwrap() + } + + #[tokio::test] + async fn removing_a_saved_application_drops_its_cached_icon() { + let saved = saved_editor(); + let icon_id = applications::icon_id(&saved.path); + let path = + std::env::temp_dir().join(format!("openmouse-icon-test-{}.json", std::process::id())); + let bridge = BridgeService::new( + BridgeConfig { + local_applications: vec![saved], + ..BridgeConfig::default() + }, + path.clone(), + ); + bridge + .inner + .write() + .await + .application_icons + .insert(icon_id.clone(), Some(vec![1])); + assert_eq!(bridge.application_icon(&icon_id).await, Some(vec![1])); + + bridge.replace_local_applications(Vec::new()).await.unwrap(); + let _ = std::fs::remove_file(&path); + + assert!( + !bridge + .inner + .read() + .await + .application_icons + .contains_key(&icon_id) + ); + assert_eq!(bridge.application_icon(&icon_id).await, None); + } + + #[tokio::test] + async fn a_failed_icon_extraction_is_not_cached() { + let saved = saved_editor(); + let icon_id = applications::icon_id(&saved.path); + let bridge = service(BridgeConfig { + local_applications: vec![saved], + ..BridgeConfig::default() + }); + + assert_eq!( + bridge.application_icon(&icon_id).await, + None, + "the path does not exist" + ); + assert!( + !bridge + .inner + .read() + .await + .application_icons + .contains_key(&icon_id) + ); + } + #[tokio::test] async fn client_is_connected_only_after_a_heartbeat() { let bridge = service(BridgeConfig::default()); diff --git a/tests/backend_local_profiles.rs b/tests/backend_local_profiles.rs index 396956b..fc00a26 100644 --- a/tests/backend_local_profiles.rs +++ b/tests/backend_local_profiles.rs @@ -55,6 +55,13 @@ async fn server_with_config(storage_failure: bool, config: BridgeConfig) -> Test } } +/// A saved target as the local-applications endpoints serve it. +fn served(mut value: serde_json::Value) -> serde_json::Value { + let icon_id = openmouse_bridge::applications::icon_id(value["path"].as_str().unwrap()); + value["iconId"] = icon_id.into(); + value +} + fn target() -> serde_json::Value { serde_json::json!({"id":"local:editor","name":"Editor","executable":"editor.exe","path":"C:/Tools/editor.exe","kind":"app"}) } @@ -74,7 +81,7 @@ async fn activation_metadata_roundtrips_into_canonical_profiles() { assert_eq!(response.status(), 200); assert_eq!( response.json::().await.unwrap(), - serde_json::json!({"ok":true,"applications":[value.clone()]}) + serde_json::json!({"ok":true,"applications":[served(value.clone())]}) ); let stored = server.service.local_applications().await; assert_eq!(serde_json::to_value(&stored[0]).unwrap(), value); @@ -163,7 +170,7 @@ async fn saved_missing_canonical_bundle_survives_batch_crud_and_profile_readback .unwrap(); assert_eq!(response.status(), 200); let readback: serde_json::Value = response.json().await.unwrap(); - assert_eq!(readback["applications"][0], bundle); + assert_eq!(readback["applications"][0], served(bundle.clone())); let profiles = server.service.profiles().await; assert_eq!(profiles.len(), 1); assert_eq!( @@ -201,7 +208,7 @@ async fn local_http_roundtrip_reconciles_id_linked_profiles_and_restarts() { assert_eq!(response.status(), 200); assert_eq!( response.json::().await.unwrap(), - serde_json::json!({"ok":true,"applications":[target()]}) + serde_json::json!({"ok":true,"applications":[served(target())]}) ); let local = serde_json::json!({"application":{"localId":"local:editor","name":"Wrong","executable":"wrong.exe","path":""},"device":{"id":"d","name":"Mouse"},"settings":{"dpi":1600},"enabled":false}); let preset = serde_json::json!({"application":{"name":"Editor","executable":"editor.exe","path":""},"device":{"id":"d","name":"Mouse"},"settings":{"dpi":800}}); @@ -303,7 +310,7 @@ async fn local_http_validation_is_400_and_never_changes_state() { assert_eq!(std::fs::read(&server.path).unwrap(), original); let targets: serde_json::Value = client.get(&url).send().await.unwrap().json().await.unwrap(); - assert_eq!(targets, serde_json::json!([target()])); + assert_eq!(targets, serde_json::json!([served(target())])); } assert_eq!( client