From 53c772702f49d4f7cbed2b5f086024772c08eb92 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 27 Jul 2026 19:33:05 +0200 Subject: [PATCH] feat(host): list-monitors CLI, and two bugs it caught on-glass An operator configuring an unattended host has to learn the connector names from somewhere, and "curl the management API before the host is configured" is not it. `punktfunk-host list-monitors` prints them with geometry and flags the pinned one. Running it under a real sway immediately caught two defects the unit tests could not: - the query used the `swaymsg` command helper, which inserts `--`, so `-t get_outputs` came back as "Unknown/invalid command '-t'". Queries now go through a `swaymsg_query` helper that cannot make that mistake. - compositors write the literal "Unknown" rather than leaving make/model empty, so the picker label read "Unknown Unknown". One `describe` helper now treats that as absent, for all four backends. Verified on a headless two-output sway: names, modes, positions, scale and the case-insensitive pin all resolve. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/vdisplay/linux/hyprland.rs | 13 ++--- .../src/vdisplay/linux/kwin_output_mgmt.rs | 12 ++--- .../pf-vdisplay/src/vdisplay/linux/mutter.rs | 7 +-- .../pf-vdisplay/src/vdisplay/linux/wlroots.rs | 33 +++++++----- crates/pf-vdisplay/src/vdisplay/monitors.rs | 35 +++++++++++++ crates/punktfunk-host/src/main.rs | 50 +++++++++++++++++++ 6 files changed, 118 insertions(+), 32 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs b/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs index 88baea51..656e6381 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/hyprland.rs @@ -289,12 +289,13 @@ pub(crate) fn list_monitors() -> Result> { .filter_map(|m| { let connector = m.get("name")?.as_str()?.to_string(); let num = |k: &str| m.get(k).and_then(|v| v.as_i64()).unwrap_or(0); - let description = m - .get("description") - .and_then(|v| v.as_str()) - .filter(|s| !s.trim().is_empty()) - .unwrap_or(&connector) - .to_string(); + // Hyprland's `description` is already a "make model (connector)" string; treat it as + // the make and let the shared helper drop it when it is empty/Unknown. + let description = crate::monitors::describe( + m.get("description").and_then(|v| v.as_str()).unwrap_or(""), + "", + &connector, + ); Some(crate::monitors::PhysicalMonitor { connector, description, diff --git a/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs b/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs index c64a47f4..44f62c30 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs @@ -493,15 +493,13 @@ pub(crate) fn list_monitors() -> anyhow::Result { - format!("{make} {model}").trim().to_string() - } - _ => connector.clone(), - }; Some(crate::monitors::PhysicalMonitor { managed: connector.starts_with(MANAGED_PREFIX), - description: label, + description: crate::monitors::describe( + d.make.as_deref().unwrap_or(""), + d.model.as_deref().unwrap_or(""), + &connector, + ), // A disabled output has no current mode — report 0s rather than inventing one. width: dims.map(|d| d.0).unwrap_or(0), height: dims.map(|d| d.1).unwrap_or(0), diff --git a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs index 32d02143..58c9b997 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/mutter.rs @@ -840,14 +840,9 @@ pub(crate) fn list_monitors() -> Result> { let (w, h, refresh) = current_mode_full(&state, connector) .map(|(_id, w, h, hz)| (w.max(0) as u32, h.max(0) as u32, (hz * 1000.0) as u32)) .unwrap_or((0, 0, 0)); - let label = format!("{vendor} {product}"); crate::monitors::PhysicalMonitor { connector: connector.clone(), - description: if label.trim().is_empty() { - connector.clone() - } else { - label.trim().to_string() - }, + description: crate::monitors::describe(vendor, product, connector), width: w, height: h, refresh_mhz: refresh, diff --git a/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs b/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs index 232627a5..dde6482b 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/wlroots.rs @@ -207,20 +207,29 @@ fn swaymsg(args: &[&str]) -> Result { Ok(String::from_utf8_lossy(&out.stdout).into_owned()) } -/// Current output names from `swaymsg -t get_outputs` (JSON). -fn output_names() -> Result> { +/// Run a swaymsg **query** (`-t --raw`) and parse its JSON. +/// +/// ⚠️ Deliberately NOT [`swaymsg`]: that helper inserts `--` so its arguments are read as a sway +/// *command*, which is right for `create_output` and wrong for a query — `-t` after `--` comes back +/// as `Unknown/invalid command '-t'` (caught on-glass writing the monitor enumeration). +fn swaymsg_query(kind: &str) -> Result { let out = Command::new("swaymsg") - .args(["-t", "get_outputs", "--raw"]) + .args(["-t", kind, "--raw"]) .output() .context("run swaymsg (is sway installed?)")?; if !out.status.success() { bail!( - "swaymsg -t get_outputs failed: {}", + "swaymsg -t {kind} failed: {}", String::from_utf8_lossy(&out.stderr).trim() ); } let raw = String::from_utf8_lossy(&out.stdout).into_owned(); - let outputs: serde_json::Value = serde_json::from_str(&raw).context("parse get_outputs")?; + serde_json::from_str(&raw).with_context(|| format!("parse {kind}")) +} + +/// Current output names from `swaymsg -t get_outputs` (JSON). +fn output_names() -> Result> { + let outputs = swaymsg_query("get_outputs")?; Ok(outputs .as_array() .context("get_outputs: not an array")? @@ -235,8 +244,7 @@ fn output_names() -> Result> { /// post-transform) — what `crate::monitors` documents. An inactive output has no `current_mode`, so /// its mode reads as zeros rather than a guess. pub(crate) fn list_monitors() -> Result> { - let raw = swaymsg(&["-t", "get_outputs", "--raw"])?; - let parsed: serde_json::Value = serde_json::from_str(&raw).context("parse get_outputs")?; + let parsed = swaymsg_query("get_outputs")?; let mut out: Vec<_> = parsed .as_array() .context("get_outputs: not an array")? @@ -256,13 +264,12 @@ pub(crate) fn list_monitors() -> Result> { .unwrap_or(0) }; let str_field = |k: &str| o.get(k).and_then(|v| v.as_str()).unwrap_or("").trim(); - let label = format!("{} {}", str_field("make"), str_field("model")); Some(crate::monitors::PhysicalMonitor { - description: if label.trim().is_empty() { - connector.clone() - } else { - label.trim().to_string() - }, + description: crate::monitors::describe( + str_field("make"), + str_field("model"), + &connector, + ), width: mode("width").max(0) as u32, height: mode("height").max(0) as u32, // sway reports `refresh` in mHz already. diff --git a/crates/pf-vdisplay/src/vdisplay/monitors.rs b/crates/pf-vdisplay/src/vdisplay/monitors.rs index 0a677b88..54252979 100644 --- a/crates/pf-vdisplay/src/vdisplay/monitors.rs +++ b/crates/pf-vdisplay/src/vdisplay/monitors.rs @@ -47,6 +47,29 @@ pub struct PhysicalMonitor { pub managed: bool, } +/// Build the picker label from a compositor's make/model, falling back to the connector. +/// +/// Compositors fill unknown fields with the literal string `"Unknown"` rather than leaving them +/// empty (seen on-glass: sway reports `"Unknown Unknown"` for a headless output), so treat that as +/// absent too — a picker row reading "Unknown Unknown" is worse than one reading "DP-1". +pub(crate) fn describe(make: &str, model: &str, connector: &str) -> String { + let known = |s: &str| { + let s = s.trim(); + !s.is_empty() && !s.eq_ignore_ascii_case("unknown") + }; + let label = [make, model] + .iter() + .map(|s| s.trim()) + .filter(|s| known(s)) + .collect::>() + .join(" "); + if label.is_empty() { + connector.to_string() + } else { + label + } +} + impl PhysicalMonitor { /// `1920x1080@60` — for logs and pickers. pub fn mode_label(&self) -> String { @@ -163,6 +186,18 @@ mod tests { assert!(err.contains("no monitors at all"), "{err}"); } + /// Compositors write the literal "Unknown" instead of leaving make/model empty (sway does it + /// for headless outputs), so a picker must not end up showing "Unknown Unknown". + #[test] + fn describe_falls_back_to_the_connector_for_empty_or_unknown_fields() { + assert_eq!(describe("ACME", "U2720Q", "DP-1"), "ACME U2720Q"); + assert_eq!(describe("Unknown", "Unknown", "HEADLESS-1"), "HEADLESS-1"); + assert_eq!(describe("", "", "DP-1"), "DP-1"); + // A half-known pair keeps the half that means something. + assert_eq!(describe("Unknown", "U2720Q", "DP-1"), "U2720Q"); + assert_eq!(describe(" ", "unknown", "DP-2"), "DP-2"); + } + #[test] fn mode_label_drops_an_unknown_refresh() { let mut m = mon("DP-1"); diff --git a/crates/punktfunk-host/src/main.rs b/crates/punktfunk-host/src/main.rs index 42e95114..1cb77da6 100644 --- a/crates/punktfunk-host/src/main.rs +++ b/crates/punktfunk-host/src/main.rs @@ -199,6 +199,9 @@ fn is_management_cli(args: &[String]) -> bool { | Some("openapi") | Some("library") | Some("detect-conflicts") + // Reads the compositor's output list and exits — none of the host-startup work applies, + // and it must not re-run the pin's own startup report while printing that same list. + | Some("list-monitors") | Some("-h") | Some("--help") | Some("help") @@ -458,6 +461,51 @@ fn real_main() -> Result<()> { println!("{compositor:?} ready"); Ok(()) } + // List the host's physical monitors — the connector names `PUNKTFUNK_CAPTURE_MONITOR` + // takes. An operator configuring an unattended host has to learn those names from + // somewhere, and "curl the management API before the host is configured" is not it. + #[cfg(target_os = "linux")] + Some("list-monitors") => { + let compositor = vdisplay::detect()?; + let monitors = vdisplay::monitors::list(compositor) + .with_context(|| format!("enumerate monitors on {compositor:?}"))?; + if monitors.is_empty() { + println!("{compositor:?}: no monitors"); + return Ok(()); + } + let pinned = pf_host_config::config().capture_monitor.as_deref(); + println!("{compositor:?}:"); + for m in &monitors { + let mut tags = Vec::new(); + if m.primary { + tags.push("primary"); + } + if !m.enabled { + tags.push("disabled"); + } + if m.managed { + tags.push("punktfunk virtual display"); + } + if pinned.is_some_and(|p| p.eq_ignore_ascii_case(&m.connector)) { + tags.push("PINNED (PUNKTFUNK_CAPTURE_MONITOR)"); + } + println!( + " {:<12} {:>13} at +{},+{} scale {} {}{}", + m.connector, + m.mode_label(), + m.x, + m.y, + m.scale, + m.description, + if tags.is_empty() { + String::new() + } else { + format!(" [{}]", tags.join(", ")) + } + ); + } + Ok(()) + } // Create a virtual DualSense via UHID and exercise it (validation, no streaming session). #[cfg(target_os = "linux")] Some("dualsense-test") => devtest::dualsense_test(&args), @@ -780,6 +828,8 @@ USAGE: punktfunk-host openapi print the management API's OpenAPI document (codegen) punktfunk-host punktfunk1-host [OPTIONS] native punktfunk/1 host (QUIC control + UDP data plane) punktfunk-host probe-compositor exit 0 iff the compositor is up + ready (bringup gate) + punktfunk-host list-monitors list the host's physical monitors (Linux) — the + connector names PUNKTFUNK_CAPTURE_MONITOR takes punktfunk-host spike [OPTIONS] capture→encode→file pipeline spike (dev tool) SERVE OPTIONS: