From ef72d102b6c3706ef4a4af9e12f46f79090ec8fb Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 11 Aug 2026 09:22:11 +0200 Subject: [PATCH] fix(pf-vdisplay): KWin's re-enable reported success when it matched no outputs at all, leaving a physical monitor dark MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * **`reenable_outputs` returned `true` when it matched NONE of the requested outputs.** Unresolvable outputs were `continue`d and the return was the apply verdict alone — but an empty `kde_output_configuration_v2` still gets an `applied` event. So a total no-op suppressed the `reenable_outputs_kscreen` backstop and the operator's physical monitor stayed dark. Now counts staged outputs and returns `ok && matched == outputs.len()`, and refuses to apply an empty configuration at all. * **The kscreen restore logged "restored the physical/bootstrap outputs" unconditionally**, with both call results discarded — including when `kscreen_ok` returned false on its 5 s budget, which is exactly the wedged state that fallback exists for. * **`Session::open` swallowed every failure reason** — connect error, barrier timeout, missing global — and three of four callers degraded to kscreen-doctor with zero log. This is the class that hid the KWin >= 6.7 registry regression: a shipped fallback firing silently on every machine. It now logs at warn with the reason and the caller's operation name. * `last_name` was seeded with a name kscreen-doctor can never resolve (KWin's address is `Virtual-punktfunk…`), so the intended default was guarded by an `is_none()` that could never hold and `apply_position` ran against no output. `our_uuid` was never reset per `create` and only assigned under `outcome.handled`, so a supersede positioned the *previous* output and never fell back. * `probe()`'s `roundtrip` was the only unbudgeted compositor wait in the crate — every sibling path is budgeted — and it is reached from an async mgmt handler. Now bounded at 3 s. The pre-`created` dispatch loops gained deadlines and now set `stop` on the timeout arm. * Every `wl_output` global was bound for the session's life with no `GlobalRemove` arm and no `release()`, on the virtual-output path too, which never reads them: unbounded growth on a hotplugging session. * `monitors::list` was the one KWin call site with no kscreen fallback at all, despite `list_monitors` failing on exactly the condition the other four fall back for. It has one now. * `CVT_H_GRANULARITY` and `MANAGED_PREFIX` existed as two literals under prose asserting they match; the second copy now imports the first. The wider facade extraction (item 9.1) is deliberately not in this commit, but its two prerequisites are — a comment at the restore seam records why they had to come first: a fallback arm that returns a value the helper never checked re-introduces the silent success, behind a seam whose selling point is one honest log per decline. Also corrects the `PhysicalMonitor` type doc, which claimed "logical geometry throughout" while `width`/`height` are the mode's PIXELS and `x`/`y` are logical, and adds the `logical_size()` helper that is the only correct way to compare an extent against a position. --- crates/pf-vdisplay/src/vdisplay/linux/kwin.rs | 655 ++++++++++++++---- .../src/vdisplay/linux/kwin_output_mgmt.rs | 327 ++++++--- crates/pf-vdisplay/src/vdisplay/monitors.rs | 161 ++++- 3 files changed, 928 insertions(+), 215 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs index 9816bbc0..24dddd40 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs @@ -33,7 +33,8 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::Sender; use std::sync::Arc; use std::thread; -use std::time::Duration; +use std::time::{Duration, Instant}; +use wayland_client::protocol::wl_callback::{self, WlCallback}; use wayland_client::protocol::wl_output::{self, WlOutput}; use wayland_client::protocol::wl_registry::{self, WlRegistry}; use wayland_client::{Connection, Dispatch, Proxy, QueueHandle}; @@ -237,7 +238,20 @@ impl VirtualDisplay for KwinDisplay { Some(id) => format!("{VOUT_NAME}-{id}"), None => VOUT_NAME.to_string(), }; - self.last_name = Some(name.clone()); // for apply_position (registry-driven §6.2 layout) + // `apply_position`'s kscreen-doctor fallback (the registry-driven §6.2 layout) addresses + // `last_name`, so seed it with `Virtual-`: the address KWin exposes our output under + // and the ONLY spelling kscreen-doctor can resolve. The bare `name` we ask KWin for + // (`punktfunk`) matches no output at all, so seeding it with that left every position apply + // shelling out against an address that can never exist — and the `is_none()` guard that was + // supposed to correct it later could never fire, because this write is never `None`. + let our_prefix = format!("Virtual-{name}"); + self.last_name = Some(our_prefix.clone()); + // Every `create` re-resolves its own output, so the PREVIOUS one's UUID must not survive + // into this one. A supersede keeps this `KwinDisplay` and creates the replacement while the + // predecessor is still alive, so a stale UUID still RESOLVES: `set_position` would find the + // old output, position it, report success, and never reach the fallback — the new display + // silently stays where it was born. Re-set below only if the in-process path handles us. + self.our_uuid = None; let (width, height) = (mode.width, mode.height); let pointer_mode = if self.hw_cursor { POINTER_METADATA @@ -258,7 +272,15 @@ impl VirtualDisplay for KwinDisplay { match setup_rx.recv_timeout(Duration::from_secs(20)) { Ok(Ok(v)) => Ok((v, stop)), Ok(Err(e)) => bail!("KWin virtual output failed: {e}"), - Err(_) => bail!("timed out creating the KWin virtual output"), + Err(_) => { + // Nothing else will ever flip this `stop`: it is dropped with the error, and + // the `StopGuard` that normally owns it is only built on the success path. So + // the worker — which is by construction still inside `await_created` — would + // sit out its own budget holding a half-built output whose Wayland connection + // KWin keeps the output alive for. Release it here. + stop.store(true, Ordering::Relaxed); + bail!("timed out creating the KWin virtual output") + } } }; // KWin creates virtual outputs at a hardcoded 60 Hz, `stream_virtual_output` has no @@ -293,8 +315,8 @@ impl VirtualDisplay for KwinDisplay { ); // Topology + positioning address OUR output by its kde_output_management UUID (resolved // in-process in `apply_topology`, supersede-robust) — no early kscreen-doctor resolve, so - // the path never shells out. `Virtual-` is the name KWin exposes our output as. - let our_prefix = format!("Virtual-{name}"); + // the path never shells out. `our_prefix` (computed above with `last_name`) is the name + // KWin exposes our output as. let mut expect_exact_dims = false; // The size the output actually ENDS UP at — the request, unless KWin's CVT generator had to // shrink the width to the cell grain (see `CVT_H_GRANULARITY`). Reported as the output's @@ -372,12 +394,11 @@ impl VirtualDisplay for KwinDisplay { // kscreen-doctor backend; see `apply_topology`), with a kscreen-doctor fallback. `disabled` // is the physical/bootstrap outputs, each `(name, "WxH@Hz")`, to restore on teardown. let disabled = self.apply_topology(&name, &our_prefix, final_dims); - // A plain managed name is enough for apply_position's kscreen-doctor fallback when the - // in-process UUID path isn't set (single-output sessions are unambiguous; a supersede uses - // the UUID path instead). `want_high` already set `last_name` to the resolved kscreen id. - if self.last_name.is_none() { - self.last_name = Some(our_prefix); - } + // `last_name` is already the best address we have: `Virtual-` from the top of this + // function, upgraded in place to the RESOLVED numeric kscreen id by whichever of the + // `want_high` fallback or `apply_topology`'s fallback actually ran a resolve. Nothing to + // fill in here — the guard that used to sit at this spot could never fire (`last_name` is + // written unconditionally above) and only made the plain-name case look handled. // Per-group restore (§6.1): DON'T bind the re-enable to this session's keepalive (a per-session // `StopGuard` restore would re-enable the physical the moment the FIRST of several exclusive // sessions drops — under a still-live sibling). Instead stash it as a closure the registry lifts @@ -385,7 +406,16 @@ impl VirtualDisplay for KwinDisplay { // that display's output is reclaimed, so KWin never sees zero outputs). Empty ⇒ nothing to restore. self.pending_restore = (!disabled.is_empty()).then(|| { let disabled = disabled.clone(); - // In-process first; fall back to kscreen-doctor if the compositor doesn't answer in budget. + // In-process first; fall back to kscreen-doctor if the compositor doesn't answer in + // budget. **Both halves now return honest verdicts** — `reenable_outputs` reports + // `false` unless every requested output was actually staged (an empty configuration + // used to ack as `applied` and suppress this backstop), and + // `reenable_outputs_kscreen` branches on its own exit status instead of logging + // success unconditionally. Any future extraction of these hand-rolled + // in-process-then-kscreen ladders into one facade must keep that property: a fallback + // arm that returns a value the helper never checked would re-introduce exactly the + // silent-success this pair was fixed for, behind a seam that claims to have one log + // site for every decline. Box::new(move || { if !crate::kwin_output_mgmt::reenable_outputs(&disabled) { reenable_outputs_kscreen(&disabled); @@ -409,6 +439,12 @@ impl VirtualDisplay for KwinDisplay { /// closure only when the in-process path reports the compositor didn't answer. Called by the registry /// when the display group's last member is torn down (design §6.1), BEFORE that member's output is /// reclaimed — so KWin is never momentarily left with zero enabled outputs. +/// +/// **This is the last line of defence for a physical monitor**, so it reports what actually +/// happened. It used to discard both `kscreen_ok` verdicts and log restored-everything +/// unconditionally — including when the call had been killed at [`KSCREEN_BUDGET`], i.e. exactly +/// the wedged compositor this fallback exists for, with a screen left dark and a green line in the +/// log saying otherwise. fn reenable_outputs_kscreen(outputs: &[(String, String)]) { if outputs.is_empty() { return; @@ -420,20 +456,40 @@ fn reenable_outputs_kscreen(outputs: &[(String, String)]) { .iter() .map(|(name, _)| format!("output.{name}.enable")) .collect(); - let _ = kscreen_ok(&enable_args); + let enabled = kscreen_ok(&enable_args); + if !enabled { + // Nothing further to try: both the in-process path and this one have now declined, so the + // outputs stay as `exclusive` left them. Say so loudly — a dark monitor with no line in the + // log is what this whole restore chain exists to prevent. + tracing::error!( + outputs = ?outputs, + args = ?enable_args, + "KWin: could NOT re-enable the physical/bootstrap outputs (kscreen-doctor failed or hit \ + its budget after the in-process restore already declined) — a monitor may be left dark" + ); + return; + } // THEN re-assert each captured mode, best-effort — a bare re-enable lets KWin fall back to the // EDID-preferred mode (a 120 Hz panel returns at ~60 Hz); this restores the exact refresh. The - // output is enabled now, so the mode set is valid; a rejected mode just leaves KWin's default. + // output is enabled now, so the mode set is valid; a rejected mode just leaves KWin's default — + // a wrong refresh, not a dark screen, which is why only this half degrades to a warn. let mode_args: Vec = outputs .iter() .filter(|(_, mode)| !mode.is_empty()) .map(|(name, mode)| format!("output.{name}.mode.{mode}")) .collect(); - if !mode_args.is_empty() { - let _ = kscreen_ok(&mode_args); - } + let modes_restored = mode_args.is_empty() || kscreen_ok(&mode_args); std::thread::sleep(Duration::from_millis(200)); - tracing::info!(reenabled = ?outputs, "KWin: restored the physical/bootstrap outputs at their captured modes (group empty)"); + if modes_restored { + tracing::info!(reenabled = ?outputs, "KWin: restored the physical/bootstrap outputs at their captured modes (group empty)"); + } else { + tracing::warn!( + reenabled = ?outputs, + args = ?mode_args, + "KWin: re-enabled the physical/bootstrap outputs but could not re-assert their captured \ + modes — they are back at KWin's preferred refresh, not the one they were streaming at" + ); + } } /// Resolve the kscreen address of the virtual output the host JUST created: the managed-prefix @@ -511,24 +567,111 @@ fn kscreen_json() -> Option { serde_json::from_slice(&kscreen_json_bytes()?).ok() } -/// The `(width, height)` of an output's CURRENT mode from its `kscreen-doctor -j` entry. -fn output_active_size(o: &serde_json::Value) -> Option<(u32, u32)> { - let as_id = |v: &serde_json::Value| -> Option { - v.as_str() - .map(|s| s.to_string()) - .or_else(|| v.as_u64().map(|n| n.to_string())) - }; - let current = o.get("currentModeId").and_then(as_id)?; +/// The CURRENT mode of an output from its `kscreen-doctor -j` entry, as `(width, height, +/// refresh_mHz)`. `None` if the entry names no current mode or that mode carries no size; a mode +/// with no `refreshRate` reports 0 mHz, which is the "unknown" the monitor type documents. +fn output_active_mode(o: &serde_json::Value) -> Option<(u32, u32, u32)> { + let current = o.get("currentModeId").and_then(json_id)?; let mode = o .get("modes")? .as_array()? .iter() - .find(|m| m.get("id").and_then(as_id).as_deref() == Some(current.as_str()))?; + .find(|m| m.get("id").and_then(json_id).as_deref() == Some(current.as_str()))?; let size = mode.get("size")?; - Some(( - size.get("width").and_then(|v| v.as_u64())? as u32, - size.get("height").and_then(|v| v.as_u64())? as u32, - )) + let w = size.get("width").and_then(|v| v.as_u64())? as u32; + let h = size.get("height").and_then(|v| v.as_u64())? as u32; + // Hz → mHz without an intermediate round: `refreshRate` is a float (59.94, 119.92) and whole + // Hz would throw away exactly the distinction `PhysicalMonitor::refresh_mhz` exists to keep. + let mhz = mode + .get("refreshRate") + .and_then(|r| r.as_f64()) + .map(|hz| (hz * 1000.0).round().max(0.0) as u32) + .unwrap_or(0); + Some((w, h, mhz)) +} + +/// The `(width, height)` of an output's CURRENT mode from its `kscreen-doctor -j` entry. +fn output_active_size(o: &serde_json::Value) -> Option<(u32, u32)> { + output_active_mode(o).map(|(w, h, _)| (w, h)) +} + +/// Every head KWin reports, for [`crate::monitors::list`] — the in-process enumerate +/// ([`crate::kwin_output_mgmt::list_monitors`]) with a `kscreen-doctor -j` fallback. +/// +/// This was the ONE KWin call site with no fallback at all, while the in-process session it depends +/// on declines for exactly the reasons the other five fall back for: management global absent +/// (pre-6.x KWin), or a compositor that does not answer in budget. The console's monitor picker and +/// `PUNKTFUNK_CAPTURE_MONITOR`'s resolve then failed outright on a box whose `kscreen-doctor` was +/// perfectly able to answer — and a failed `list` is not "no monitors", it is a session that +/// refuses to start (`monitors::resolve` treats a miss as a hard error, deliberately). +pub(crate) fn list_monitors() -> Result> { + let declined = match crate::kwin_output_mgmt::list_monitors() { + Ok(monitors) => return Ok(monitors), + Err(e) => e, + }; + let Some(doc) = kscreen_json() else { + return Err(declined.context( + "kscreen-doctor -j did not answer either (not installed, or killed at its budget)", + )); + }; + let monitors = monitors_from_kscreen_json(&doc); + tracing::info!( + count = monitors.len(), + reason = %declined, + "KWin: enumerated monitors via kscreen-doctor (in-process output management declined)" + ); + Ok(monitors) +} + +/// Parse `kscreen-doctor -j` into the shared monitor type. Split from the process call so it can be +/// tested against captured JSON — the mapping is where a picker's identity keys come from, and +/// `x`/`y` are what make two same-sized heads distinguishable at all. +/// +/// Deliberately mirrors the in-process reader's contract: a disabled output has no current mode and +/// reports zeroed geometry rather than an invented one, `primary` accepts either the modern +/// `priority: 1` or the older `primary: true`, and the list is sorted by desktop position so it +/// reads left-to-right the way the desk looks. +fn monitors_from_kscreen_json(doc: &serde_json::Value) -> Vec { + let Some(outputs) = doc.get("outputs").and_then(|o| o.as_array()) else { + return Vec::new(); + }; + let mut out: Vec = outputs + .iter() + .filter_map(|o| { + let connector = o.get("name").and_then(|n| n.as_str())?.to_string(); + let mode = output_active_mode(o); + let coord = |k: &str| { + o.get("pos") + .and_then(|p| p.get(k)) + .and_then(|v| v.as_i64()) + .unwrap_or(0) as i32 + }; + Some(crate::monitors::PhysicalMonitor { + managed: connector.starts_with(MANAGED_PREFIX), + description: crate::monitors::describe( + o.get("vendor").and_then(|v| v.as_str()).unwrap_or(""), + o.get("model").and_then(|v| v.as_str()).unwrap_or(""), + &connector, + ), + width: mode.map(|m| m.0).unwrap_or(0), + height: mode.map(|m| m.1).unwrap_or(0), + refresh_mhz: mode.map(|m| m.2).unwrap_or(0), + x: coord("x"), + y: coord("y"), + scale: o + .get("scale") + .and_then(|v| v.as_f64()) + .filter(|s| *s > 0.0) + .unwrap_or(1.0), + primary: o.get("primary").and_then(|p| p.as_bool()).unwrap_or(false) + || o.get("priority").and_then(|p| p.as_u64()) == Some(1), + enabled: o.get("enabled").and_then(|e| e.as_bool()).unwrap_or(false), + connector, + }) + }) + .collect(); + out.sort_by_key(|m| (m.x, m.y, m.connector.clone())); + out } /// CVT's horizontal cell granularity. KWin generates every custom mode's timing with **libxcvt**, @@ -542,7 +685,11 @@ fn output_active_size(o: &serde_json::Value) -> Option<(u32, u32)> { /// birth mode, and the caller falls back to 60 Hz — while KDE's display list shows the perfectly /// good 2864x1320@119.92 mode sitting there unselected. Widths like 1920/2560/3840 are all /// multiples of 8, which is why only phone-shaped clients ever hit it. -const CVT_H_GRANULARITY: u32 = 8; +/// +/// Shared with [`crate::kwin_output_mgmt`], which matches the generated mode back the same way — +/// it used to keep its own copy under a comment claiming the two "match", which is a claim no +/// compiler was checking. +pub(crate) const CVT_H_GRANULARITY: u32 = 8; /// One row of an output's mode list, as parsed from `kscreen-doctor -j`. #[derive(Clone, Debug, PartialEq)] @@ -762,7 +909,11 @@ fn read_active_mode(output: &str) -> Option<(u32, u32, u32)> { /// The prefix EVERY managed KWin output shares — Stage 3 names them `punktfunk` / `punktfunk-`, /// which KWin exposes as `Virtual-punktfunk` / `Virtual-punktfunk-`. Group membership (§6.1) is /// recognised by this prefix, so we never have to thread the live set through the backend. -const MANAGED_PREFIX: &str = "Virtual-punktfunk"; +/// +/// Shared with [`crate::kwin_output_mgmt`] rather than copied: both halves of the ladder decide +/// "is this output one of OURS?" with it, and a drift between two copies would make the in-process +/// path disable a sibling session's output that the kscreen path deliberately spares. +pub(crate) const MANAGED_PREFIX: &str = "Virtual-punktfunk"; /// The current mode of an output as a kscreen-doctor mode setter, from its `-j` entry — preferring /// the human `WxH@Hz` form (survives a mode-id re-enumeration across disable→enable) and falling back @@ -875,14 +1026,27 @@ fn apply_virtual_primary(ours: &str) -> Vec<(String, String)> { // the group is unambiguously the desktop — never a sibling session's output (group-aware filter). // Each is captured WITH its current mode so teardown restores its real refresh, not KWin's default. let others = other_enabled_outputs(); - if !others.is_empty() { - let args: Vec = others - .iter() - .map(|(o, _mode)| format!("output.{o}.disable")) - .collect(); - let _ = kscreen(&args); + if others.is_empty() { + tracing::info!("KWin: streamed output set as the sole desktop (nothing else was enabled)"); + return others; + } + let args: Vec = others + .iter() + .map(|(o, _mode)| format!("output.{o}.disable")) + .collect(); + if kscreen(&args) { + tracing::info!(also_disabled = ?others, "KWin: streamed output set as the sole desktop"); + } else { + // Report the request, not a success: the outputs are still enabled, so the client sees the + // shell wherever KWin left it. They are returned for the restore regardless — re-enabling an + // output that was never disabled is a harmless no-op, and dropping them here would strand a + // physical dark if the disable actually landed and only the ack was lost to the budget. + tracing::warn!( + attempted_disable = ?others, + "KWin: could not disable the other outputs for the exclusive topology (kscreen-doctor \ + failed or hit its budget) — the streamed output is not the sole desktop" + ); } - tracing::info!(also_disabled = ?others, "KWin: streamed output set as the sole desktop"); others } @@ -919,10 +1083,42 @@ struct State { node_id: Option, failed: Option, closed: bool, - /// Every `wl_output` KWin advertises, keyed by the proxy, with its connector name once the - /// `name` event arrives. Only the monitor-mirror path ([`stream_existing_output`]) needs these - /// — `stream_output` takes a `wl_output` object, so the connector has to be resolved to one. - outputs: Vec<(WlOutput, Option)>, + /// Highest `wl_display.sync` serial whose `done` has arrived — the barrier [`roundtrip_within`] + /// waits on, so a compositor that accepted the connection and then stopped serving costs a + /// budget instead of the thread. + sync_done: u32, + /// Whether this connection needs `wl_output` objects at all — true ONLY on the monitor-mirror + /// path. `stream_virtual_output` names its output by string, so the virtual-output path never + /// reads [`State::outputs`]; binding them there was pure accumulation on a connection that + /// lives for the whole session, and every managed display this host creates is itself another + /// `wl_output` global. + want_outputs: bool, + /// Every `wl_output` KWin advertises, as (registry global name, proxy, connector once the + /// `name` event arrives). Only the monitor-mirror path ([`stream_existing_output`]) needs these — + /// `stream_output` takes a `wl_output` object, so the connector has to be resolved to one. The + /// global name is carried so `global_remove` can find the entry again ([`State::forget_output`]). + outputs: Vec<(u32, WlOutput, Option)>, +} + +impl State { + /// Drop the `wl_output` whose registry global just went away. + /// + /// Both halves matter. The proxy must be `release`d — wayland-rs sends no destructor when a + /// proxy is merely dropped, so an unreleased binding is a server-side object leaked for the + /// life of a connection that lasts as long as the session. And the ENTRY must go, because + /// [`run_existing`]'s connector resolve scans this vector: a stale row for an unplugged head + /// would shadow the live output that took its connector name. + fn forget_output(&mut self, global: u32) { + let Some(pos) = self.outputs.iter().position(|(n, _, _)| *n == global) else { + return; + }; + let (_, out, connector) = self.outputs.remove(pos); + // `wl_output.release` is `since 3`; below that the object simply has no destructor. + if out.version() >= 3 { + out.release(); + } + tracing::debug!(?connector, "KWin: a wl_output went away — released it"); + } } impl Dispatch for State { @@ -934,23 +1130,45 @@ impl Dispatch for State { _: &Connection, qh: &QueueHandle, ) { - if let wl_registry::Event::Global { - name, - interface, - version, - } = event - { - if interface == Screencast::interface().name { - let v = version.min(MAX_VERSION); - state.screencast = Some(registry.bind::(name, v, qh, ())); - } else if interface == WlOutput::interface().name { - // v4 is where `wl_output.name` (the connector) arrives; bind at least that when the - // compositor offers it, else bind what it has and let the resolve fail loudly - // rather than mirroring an unidentifiable head. - let v = version.min(WL_OUTPUT_MAX_VERSION); - let out = registry.bind::(name, v, qh, ()); - state.outputs.push((out, None)); + match event { + wl_registry::Event::Global { + name, + interface, + version, + } => { + if interface == Screencast::interface().name { + let v = version.min(MAX_VERSION); + state.screencast = Some(registry.bind::(name, v, qh, ())); + } else if state.want_outputs && interface == WlOutput::interface().name { + // v4 is where `wl_output.name` (the connector) arrives; bind at least that when + // the compositor offers it, else bind what it has and let the resolve fail + // loudly rather than mirroring an unidentifiable head. + let v = version.min(WL_OUTPUT_MAX_VERSION); + let out = registry.bind::(name, v, qh, ()); + state.outputs.push((name, out, None)); + } } + wl_registry::Event::GlobalRemove { name } => state.forget_output(name), + _ => {} + } + } +} + +/// The `wl_display.sync` callback: `done` releases whichever [`roundtrip_within`] is waiting on +/// this serial. A plain `roundtrip()` would do the same job in one call, but it blocks on the +/// socket with no ceiling — against a compositor that accepted the connection and then stopped +/// answering, that is the session's stream thread pinned forever. +impl Dispatch for State { + fn event( + state: &mut Self, + _: &WlCallback, + event: wl_callback::Event, + serial: &u32, + _: &Connection, + _: &QueueHandle, + ) { + if let wl_callback::Event::Done { .. } = event { + state.sync_done = state.sync_done.max(*serial); } } } @@ -969,8 +1187,8 @@ impl Dispatch for State { _: &QueueHandle, ) { if let wl_output::Event::Name { name } = event { - if let Some(slot) = state.outputs.iter_mut().find(|(o, _)| o == output) { - slot.1 = Some(name); + if let Some(slot) = state.outputs.iter_mut().find(|(_, o, _)| o == output) { + slot.2 = Some(name); } } } @@ -1055,7 +1273,13 @@ pub(crate) fn stream_existing_output( let node_id = match setup_rx.recv_timeout(Duration::from_secs(20)) { Ok(Ok(v)) => v, Ok(Err(e)) => bail!("KWin monitor mirror failed: {e}"), - Err(_) => bail!("timed out recording the KWin output {connector:?}"), + Err(_) => { + // Same leak as the virtual-output opener: `StopOnDrop` only takes ownership of `stop` + // on the success path, so without this the mirror thread keeps recording a monitor + // nobody is watching until its own budget runs out. + stop.store(true, Ordering::Relaxed); + bail!("timed out recording the KWin output {connector:?}") + } }; Ok(crate::mirror::MirrorStream { node_id, @@ -1187,7 +1411,16 @@ pub fn probe() -> Result<()> { let qh = queue.handle(); let _registry = conn.display().get_registry(&qh, ()); let mut state = State::default(); - queue.roundtrip(&mut state).context("registry roundtrip")?; + // Nothing to interrupt a probe: it is a one-shot question, bounded by the roundtrip budget. + let never = AtomicBool::new(false); + roundtrip_within( + &conn, + &mut queue, + &mut state, + &never, + 1, + "registry roundtrip", + )?; if state.screencast.is_none() { bail!( "KWin is up but does not expose zkde_screencast_unstable_v1 to this client — KWin gates \ @@ -1227,13 +1460,22 @@ fn run_existing( let qh = queue.handle(); let _registry = conn.display().get_registry(&qh, ()); - let mut state = State::default(); + // The one path that resolves a connector to a `wl_output`, so the only one that binds them. + let mut state = State { + want_outputs: true, + ..State::default() + }; // Two roundtrips: the first processes the globals (binding screencast + every wl_output), the // second drains each output's property burst — the `name` event we resolve the connector by. - queue.roundtrip(&mut state).context("registry roundtrip")?; - queue - .roundtrip(&mut state) - .context("wl_output property roundtrip")?; + roundtrip_within(&conn, &mut queue, &mut state, stop, 1, "registry roundtrip")?; + roundtrip_within( + &conn, + &mut queue, + &mut state, + stop, + 2, + "wl_output property roundtrip", + )?; let screencast = state.screencast.clone().ok_or_else(|| { anyhow!( @@ -1251,19 +1493,19 @@ fn run_existing( let named: Vec<&str> = state .outputs .iter() - .filter_map(|(_, n)| n.as_deref()) + .filter_map(|(_, _, n)| n.as_deref()) .collect(); let output = state .outputs .iter() - .find(|(_, n)| n.as_deref() == Some(connector)) + .find(|(_, _, n)| n.as_deref() == Some(connector)) .or_else(|| { - state.outputs.iter().find(|(_, n)| { + state.outputs.iter().find(|(_, _, n)| { n.as_deref() .is_some_and(|n| n.eq_ignore_ascii_case(connector)) }) }) - .map(|(o, _)| o.clone()) + .map(|(_, o, _)| o.clone()) .ok_or_else(|| { if named.is_empty() { anyhow!( @@ -1285,20 +1527,7 @@ fn run_existing( "KWin: recording an existing output; awaiting PipeWire node" ); - let node_id = loop { - queue - .blocking_dispatch(&mut state) - .context("wayland dispatch (awaiting created)")?; - if let Some(node) = state.node_id { - break node; - } - if let Some(e) = state.failed.take() { - bail!("stream_output failed: {e}"); - } - if state.closed { - bail!("KWin closed the stream before it was created"); - } - }; + let node_id = await_created(&conn, &mut queue, &mut state, stop, "stream_output")?; setup_tx .send(Ok(node_id)) .map_err(|_| anyhow!("monitor-mirror opener went away"))?; @@ -1323,8 +1552,10 @@ fn run( let qh = queue.handle(); let _registry = conn.display().get_registry(&qh, ()); + // `want_outputs` stays false: `stream_virtual_output` names its output by string, so this + // connection never needs a `wl_output` — and it lives for the whole session (see `State`). let mut state = State::default(); - queue.roundtrip(&mut state).context("registry roundtrip")?; + roundtrip_within(&conn, &mut queue, &mut state, stop, 1, "registry roundtrip")?; let screencast = state.screencast.clone().ok_or_else(|| { anyhow!( @@ -1353,21 +1584,8 @@ fn run( "KWin: requested virtual output; awaiting PipeWire node" ); - // Pump events until KWin reports the node id (or an error). - let node_id = loop { - queue - .blocking_dispatch(&mut state) - .context("wayland dispatch (awaiting created)")?; - if let Some(node) = state.node_id { - break node; - } - if let Some(e) = state.failed.take() { - bail!("stream_virtual_output failed: {e}"); - } - if state.closed { - bail!("KWin closed the stream before it was created"); - } - }; + // Pump events until KWin reports the node id (or an error, or the budget). + let node_id = await_created(&conn, &mut queue, &mut state, stop, "stream_virtual_output")?; setup_tx .send(Ok(node_id)) .map_err(|_| anyhow!("virtual-output opener went away"))?; @@ -1380,25 +1598,66 @@ fn run( Ok(()) } -/// Keep the connection (and thus the stream) alive until told to stop, observing `closed`. -/// `blocking_dispatch` can't be interrupted, so poll the connection fd with a short timeout and -/// honor `stop` within ~200 ms. Shared by the virtual-output and monitor-mirror paths — for a -/// virtual output this connection IS the output's lifetime; for a mirror it is only the -/// recording's, and the monitor itself is untouched either way. -fn park_until_stopped( +/// Poll slice while waiting on the Wayland fd — the granularity at which `stop` and a deadline are +/// observed (matches `kwin_output_mgmt`'s `POLL_MS`). +const POLL_MS: i32 = 200; + +/// Budget for one compositor roundtrip. Generous next to a healthy one (a few ms); it exists only +/// so a KWin that accepted the connection and then stopped serving cannot pin the calling thread — +/// which for [`probe`] is whatever thread the mgmt API answered a `/display/compositors` on, and +/// for [`run`] is the session's own bring-up. +const ROUNDTRIP_BUDGET: Duration = Duration::from_secs(3); + +/// Budget for the `created` handshake (the PipeWire node id). Deliberately under the opener's own +/// 20 s `recv_timeout` in [`spawn_vout`](VirtualDisplay::create) / [`stream_existing_output`], so +/// the worker returns a REASON ("KWin never created the output") rather than the opener reporting a +/// bare timeout with the worker still parked behind it. +const CREATE_BUDGET: Duration = Duration::from_secs(15); + +/// How a bounded pump ended. +enum Pumped { + /// The predicate held. + Done, + /// `stop` was set — the caller's output/recording was released while we waited. + Stopped, + /// The deadline passed first. + Expired, +} + +/// Bounded manual event loop: dispatch what's queued, then poll the connection fd for up to +/// [`POLL_MS`] and read, until `done(&state)` holds, `stop` is set, or `deadline` passes. +/// +/// This is the only way to wait on this connection. `blocking_dispatch` and `roundtrip` cannot be +/// interrupted and have no ceiling, so a compositor that stops answering turns any wait into a +/// permanently stuck thread — and on the host that thread is the session's, whose only way to end a +/// session is to return. `deadline: None` means "no ceiling", which is correct for exactly one +/// caller: [`park_until_stopped`], where the wait IS the output's lifetime. +fn pump_until( conn: &Connection, queue: &mut wayland_client::EventQueue, state: &mut State, + deadline: Option, stop: &AtomicBool, - output: &str, - node_id: u32, -) -> Result<()> { - while !stop.load(Ordering::Relaxed) { + done: impl Fn(&State) -> bool, +) -> Result { + loop { queue.dispatch_pending(state).context("dispatch_pending")?; - if state.closed { - tracing::warn!(output = %output, node_id, "KWin closed the screencast stream"); - break; + if done(state) { + return Ok(Pumped::Done); } + if stop.load(Ordering::Relaxed) { + return Ok(Pumped::Stopped); + } + let timeout = match deadline { + Some(d) => { + let remaining = d.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return Ok(Pumped::Expired); + } + (remaining.as_millis() as i64).clamp(0, i64::from(POLL_MS)) as i32 + } + None => POLL_MS, + }; conn.flush().context("wayland flush")?; let Some(guard) = conn.prepare_read() else { continue; // events already queued — loop dispatches them @@ -1411,19 +1670,99 @@ fn park_until_stopped( // SAFETY: `&mut pfd` points at a single live, fully-initialized `libc::pollfd` on the stack, and // the count `1` matches that one-element array, so `poll` reads `fd`/`events` and writes `revents` // strictly within `pfd`. `pfd.fd` is the Wayland connection's fd, valid because `conn` (and the - // `prepare_read` guard) are alive across the call. `poll` blocks up to 200 ms and writes only - // `revents`; `pfd` outlives the synchronous call and aliases nothing (a fresh local). - let r = unsafe { libc::poll(&mut pfd, 1, 200) }; + // `prepare_read` guard) are alive across the call. `poll` blocks up to `timeout` ms and writes + // only `revents`; `pfd` outlives the synchronous call and aliases nothing (a fresh local). + let r = unsafe { libc::poll(&mut pfd, 1, timeout) }; if r > 0 && (pfd.revents & libc::POLLIN) != 0 { let _ = guard.read(); - } // else: timeout or signal — drop the guard, re-check `stop` + } // else: timeout or signal — drop the guard, re-check `stop` and the deadline + } +} + +/// A `wl_display.sync` barrier bounded by [`ROUNDTRIP_BUDGET`] — the replacement for +/// `EventQueue::roundtrip`, which waits on the socket with no ceiling. `serial` must be unique per +/// connection (callers number theirs from 1); `what` names the wait in the error. +fn roundtrip_within( + conn: &Connection, + queue: &mut wayland_client::EventQueue, + state: &mut State, + stop: &AtomicBool, + serial: u32, + what: &str, +) -> Result<()> { + let qh = queue.handle(); + let _cb = conn.display().sync(&qh, serial); + let deadline = Instant::now() + ROUNDTRIP_BUDGET; + match pump_until(conn, queue, state, Some(deadline), stop, |st| { + st.sync_done >= serial + })? { + Pumped::Done => Ok(()), + Pumped::Stopped => bail!("{what} abandoned — the stream was released while we waited"), + Pumped::Expired => bail!( + "KWin accepted the Wayland connection but did not answer the {what} within \ + {ROUNDTRIP_BUDGET:?} — the compositor is not serving this client" + ), + } +} + +/// Keep the connection (and thus the stream) alive until told to stop, observing `closed`. +/// Shared by the virtual-output and monitor-mirror paths — for a virtual output this connection IS +/// the output's lifetime; for a mirror it is only the recording's, and the monitor itself is +/// untouched either way. The only deadline-free [`pump_until`] in the file, for that reason. +fn park_until_stopped( + conn: &Connection, + queue: &mut wayland_client::EventQueue, + state: &mut State, + stop: &AtomicBool, + output: &str, + node_id: u32, +) -> Result<()> { + match pump_until(conn, queue, state, None, stop, |st| st.closed)? { + Pumped::Done => { + tracing::warn!(output = %output, node_id, "KWin closed the screencast stream"); + } + // `Expired` cannot happen without a deadline; `Stopped` is the ordinary teardown. + Pumped::Stopped | Pumped::Expired => {} } Ok(()) } +/// Wait for the `created` event carrying the PipeWire node id, bounded by [`CREATE_BUDGET`] and +/// interruptible by `stop`. +/// +/// The loop this replaced was a bare `blocking_dispatch` with no deadline that never read `stop`: +/// a KWin that acknowledged `stream_virtual_output` and then never answered parked the worker +/// thread for good, and the opener's `recv_timeout` arm — which did not set `stop` either — left it +/// there holding a half-built output. `request` names the request in the error. +fn await_created( + conn: &Connection, + queue: &mut wayland_client::EventQueue, + state: &mut State, + stop: &AtomicBool, + request: &str, +) -> Result { + let deadline = Instant::now() + CREATE_BUDGET; + let settled = |st: &State| st.node_id.is_some() || st.failed.is_some() || st.closed; + match pump_until(conn, queue, state, Some(deadline), stop, settled)? { + // Node id first: a `closed` that arrives in the same burst as `created` is a stream that + // was made and then torn down, not a failure to make one. + Pumped::Done => match (state.node_id, state.failed.take()) { + (Some(node), _) => Ok(node), + (None, Some(e)) => bail!("{request} failed: {e}"), + (None, None) => bail!("KWin closed the stream before it was created"), + }, + Pumped::Stopped => bail!("{request} abandoned — released before KWin created the stream"), + Pumped::Expired => bail!( + "KWin acknowledged {request} but never sent the PipeWire node within {CREATE_BUDGET:?}" + ), + } +} + #[cfg(test)] mod tests { - use super::{modes_from_json, pick_custom_mode, KModeRow, MANAGED_PREFIX}; + use super::{ + modes_from_json, monitors_from_kscreen_json, pick_custom_mode, KModeRow, MANAGED_PREFIX, + }; fn row(id: &str, w: u32, h: u32, hz: f64) -> KModeRow { KModeRow { @@ -1506,6 +1845,80 @@ mod tests { assert!(modes_from_json(&doc, "Virtual-nope").is_empty()); } + /// The kscreen fallback for `monitors::list` must produce the same contract the in-process + /// reader promises: geometry from `pos` (the identity key), the mode in PIXELS with refresh in + /// mHz precise enough to keep 59.94 apart from 60, a DISABLED head still listed but zeroed + /// rather than invented, our own managed output flagged, and the list sorted by position. + #[test] + fn parses_a_kscreen_monitor_list() { + let doc: serde_json::Value = serde_json::from_str( + r#"{"outputs":[ + {"id":2,"name":"HDMI-A-1","enabled":true,"priority":2,"scale":1, + "pos":{"x":1920,"y":0},"vendor":"ACME","model":"U2720Q", + "currentModeId":"m9","modes":[ + {"id":"m9","size":{"width":1920,"height":1080},"refreshRate":59.94}]}, + {"id":1,"name":"eDP-1","enabled":true,"priority":1,"scale":1.5, + "pos":{"x":0,"y":0}, + "currentModeId":7,"modes":[ + {"id":7,"size":{"width":3840,"height":2160},"refreshRate":120.0}]}, + {"id":3,"name":"DP-3","enabled":false,"scale":1,"pos":{"x":0,"y":0}, + "modes":[{"id":"z","size":{"width":2560,"height":1440},"refreshRate":60.0}]}, + {"id":4,"name":"Virtual-punktfunk-7","enabled":true,"scale":1, + "pos":{"x":5760,"y":0},"currentModeId":"v1","modes":[ + {"id":"v1","size":{"width":2560,"height":1440},"refreshRate":119.98}]} + ]}"#, + ) + .expect("fixture parses"); + let mons = monitors_from_kscreen_json(&doc); + let by = |c: &str| { + mons.iter() + .find(|m| m.connector == c) + .unwrap_or_else(|| panic!("{c} missing")) + .clone() + }; + // Sorted by desktop position, not by kscreen's own order. + let order: Vec<&str> = mons.iter().map(|m| m.connector.as_str()).collect(); + assert_eq!( + order, + vec!["DP-3", "eDP-1", "HDMI-A-1", "Virtual-punktfunk-7"] + ); + let edp = by("eDP-1"); + // PIXELS, at the scale the desk actually runs — the whole point of `logical_size`. + assert_eq!((edp.width, edp.height), (3840, 2160)); + assert_eq!(edp.scale, 1.5); + assert_eq!(edp.logical_size(), (2560.0, 1440.0)); + assert!(edp.primary, "priority 1 is KWin's primary"); + assert_eq!(edp.refresh_mhz, 120_000); + // 59.94 must survive as mHz; rounding to whole Hz here is the bug this guards. + assert_eq!(by("HDMI-A-1").refresh_mhz, 59_940); + assert_eq!(by("HDMI-A-1").description, "ACME U2720Q"); + assert!(!by("HDMI-A-1").primary); + // Disabled: listed (so "why can't I pick it?" has an answer) with no invented mode. + let dark = by("DP-3"); + assert!(!dark.enabled); + assert_eq!((dark.width, dark.height, dark.refresh_mhz), (0, 0, 0)); + // Ours, and labelled by connector when the entry carries no make/model. + let ours = by("Virtual-punktfunk-7"); + assert!(ours.managed); + assert_eq!(ours.description, "Virtual-punktfunk-7"); + assert!(!by("eDP-1").managed); + } + + /// A document with no `outputs` array (an error object, or a kscreen-doctor whose schema + /// changed) is an empty list, never a panic — the caller's own error path already covers "the + /// tool did not answer". + #[test] + fn a_malformed_kscreen_document_yields_no_monitors() { + assert!(monitors_from_kscreen_json(&serde_json::json!({})).is_empty()); + assert!(monitors_from_kscreen_json(&serde_json::json!({"outputs": 7})).is_empty()); + // An output with no name cannot be pinned or resolved, so it is dropped rather than + // reported under an empty connector. + assert!( + monitors_from_kscreen_json(&serde_json::json!({"outputs": [{"enabled": true}]})) + .is_empty() + ); + } + /// Group-aware exclusive (§6.1): with two managed group members + a physical panel enabled, /// exclusive disables ONLY the non-managed panel — never a sibling session's per-slot output /// (the Stage-3 naming would otherwise make a 2nd exclusive session black out the 1st). 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 a375d57b..2a219a17 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin_output_mgmt.rs @@ -107,10 +107,13 @@ const OP_BUDGET: Duration = Duration::from_secs(3); /// Poll slice while waiting on the Wayland fd (matches the keepalive loop's cadence in `kwin.rs`). const POLL_MS: i32 = 100; -/// KWin's CVT generator aligns a custom mode's width DOWN to a multiple of this (libxcvt's cell -/// grain), so the mode it builds for a `set_custom_modes` request may be a few px narrower than -/// asked — matches `kwin::CVT_H_GRANULARITY`. Used when matching the generated mode back. -const CVT_H_GRANULARITY: u32 = 8; +// KWin's CVT generator aligns a custom mode's width DOWN to a multiple of `CVT_H_GRANULARITY` +// (libxcvt's cell grain), so the mode it builds for a `set_custom_modes` request may be a few px +// narrower than asked — used below when matching the generated mode back. IMPORTED, not re-declared: +// this and `MANAGED_PREFIX` used to be second copies of `kwin.rs`'s literals, each under prose +// asserting the two "match" — an assertion no compiler was checking, on the two values that decide +// which output is OURS and which mode is the one we asked for. +use crate::kwin::{CVT_H_GRANULARITY, MANAGED_PREFIX}; /// `kde_output_management_v2.set_replication_source` (and the device's `replication_source` event) /// arrived in v13. wayland-rs does not range-check requests, so sending one to a lower-version bind @@ -166,9 +169,19 @@ pub(crate) struct TopologyOutcome { /// One output as read from `kde_output_device_v2`. #[derive(Default, Clone)] struct DeviceState { - /// The global `name` number (higher = more recently advertised) — used to pick the newest of two - /// same-named outputs during a supersede. + /// The global `name` number (higher = more recently advertised) — the primary newest-wins + /// tie-break between two same-named outputs during a supersede. **Zero for every device on + /// KWin ≥ 6.7**, which hands outputs out through `kde_output_device_registry_v2` instead of one + /// global per output: those carry no global name at all (see [`seq`](DeviceState::seq)). global: u32, + /// Order in which THIS connection first saw the device, from 1. The tie-break of last resort + /// behind `global`: on the registry model every `global` is 0, so without this the `max_by_key` + /// below degrades to "whichever entry `HashMap` iteration happened to reach last" — and `HashMap` + /// is seeded per process, so the supersede resolve was a coin flip that could pick the + /// PREDECESSOR (same name, same size) and configure the output that is about to disappear. + /// Announce order is not a proof of newness — it is the compositor's own enumeration order — but + /// it is deterministic, which the hash order was not. + seq: u32, name: Option, uuid: Option, enabled: bool, @@ -204,6 +217,8 @@ struct State { /// the life of the session (dropping it would end the announcements). device_registry: Option, devices: HashMap, + /// Highest [`DeviceState::seq`] handed out so far — the announce counter. + next_device_seq: u32, /// mode object id → `(width, height, refresh_mHz)`. mode_dims: HashMap, /// Highest `wl_callback` serial whose `done` has arrived — the barrier the pump waits on. @@ -213,6 +228,46 @@ struct State { failure_reason: Option, } +impl State { + /// The entry for a device, stamping its announce order ([`DeviceState::seq`]) the first time we + /// see it. Every path that creates a device entry goes through here — the two announce models + /// (per-output global, and the ≥ 6.7 registry) plus the event handler, which can race ahead of + /// both — so the counter really does reflect the order the devices arrived in. + fn device_entry(&mut self, id: ObjectId) -> &mut DeviceState { + // Disjoint field borrows: `entry` holds `devices`, the closure holds only the counter. + let next = &mut self.next_device_seq; + self.devices.entry(id).or_insert_with(|| { + *next += 1; + DeviceState { + seq: *next, + ..Default::default() + } + }) + } + + /// Forget a `kde_output_device_mode_v2` the compositor has destroyed. + /// + /// The protocol's `removed` event says the compositor destroys the object *immediately after* + /// sending it — and the event is NOT marked `type="destructor"`, so wayland-rs happily keeps the + /// proxy alive locally. Anything still holding that id would later hand it back to KWin + /// (`kde_output_configuration_v2.mode`) as a request against a dead object, which is a protocol + /// error: KWin kills the connection, the apply "fails", and a >60 Hz session degrades to the + /// kscreen-doctor path with a log indistinguishable from "this KWin is too old". Reachable + /// precisely because `set_custom_modes` REPLACES the persisted custom list, so the mode a + /// previous session left behind is destroyed the moment this session installs its own. + fn forget_mode(&mut self, id: &ObjectId) { + self.mode_dims.remove(id); + for dev in self.devices.values_mut() { + dev.modes.retain(|(mid, _)| mid != id); + if dev.current_mode.as_ref() == Some(id) { + // Don't invent a size for a destroyed mode: a resolve keyed on current dims must + // miss (and fall back) rather than match on a mode that no longer exists. + dev.current_mode = None; + } + } + } +} + impl Dispatch for State { fn event( state: &mut Self, @@ -239,7 +294,7 @@ impl Dispatch for State { // handler can record it (newest-wins tie-break during a supersede). let dev = registry.bind::(name, v, qh, name); let id = dev.id(); - state.devices.entry(id).or_default().proxy = Some(dev); + state.device_entry(id).proxy = Some(dev); } else if interface == DeviceRegistry::interface().name { // KWin ≥ 6.7 (Plasma 6.7.3 verified) no longer advertises ONE // `kde_output_device_v2` global per output — it advertises this registry and @@ -260,9 +315,14 @@ impl Dispatch for State { } /// The device registry hands out one `kde_output_device_v2` per output via its `output` event -/// (a `new_id`, so the child is created by the `event_created_child!` binding below). Devices that -/// arrive this way have no global `name` number — the newest-wins supersede tie-break uses 0 for -/// them, which is fine: that tie-break only matters for the per-output-global model. +/// (a `new_id`, so the child is created by the `event_created_child!` binding below). +/// +/// Devices that arrive this way have no global `name` number — the `0u32` UserData below is stamped +/// on every one of them, so [`DeviceState::global`] is 0 across the board. That is **not** harmless, +/// and an earlier comment here claimed it was: the registry model is what CURRENT KWin uses, so the +/// newest-wins supersede tie-break is unavailable exactly where it is needed (two same-named, +/// same-sized outputs, predecessor still alive). [`DeviceState::seq`] is the deterministic +/// fallback the tie-break actually lands on there. impl Dispatch for State { fn event( state: &mut Self, @@ -274,7 +334,7 @@ impl Dispatch for State { ) { if let RegistryEvent::Output { output } = event { let id = output.id(); - state.devices.entry(id).or_default().proxy = Some(output); + state.device_entry(id).proxy = Some(output); } } @@ -319,7 +379,23 @@ impl Dispatch for State { _: &Connection, _: &QueueHandle, ) { - let entry = state.devices.entry(device.id()).or_default(); + // Before anything re-creates the entry: `removed` (device ≥ v21, and we bind up to 24) means + // this output is gone for good and no further update will arrive. Dropping it keeps a + // hot-unplugged head from being resolved, disabled or "restored" minutes later, and the XML + // asks the client to `release` the object — the only way the server-side one is ever freed, + // since wayland-rs sends no destructor when a proxy is merely dropped. + if matches!(event, DeviceEvent::Removed) { + if let Some(dead) = state.devices.remove(&device.id()) { + for (mid, _) in &dead.modes { + state.mode_dims.remove(mid); + } + } + if device.version() >= 21 { + device.release(); + } + return; + } + let entry = state.device_entry(device.id()); entry.global = *global; if entry.proxy.is_none() { entry.proxy = Some(device.clone()); @@ -363,6 +439,12 @@ impl Dispatch for State { _: &Connection, _: &QueueHandle, ) { + // `removed` first, and NOT through the entry below: re-inserting a destroyed mode is exactly + // the stale row a later `config.mode(...)` would send back to KWin (see [`State::forget_mode`]). + if matches!(event, ModeEvent::Removed) { + state.forget_mode(&mode.id()); + return; + } let entry = state.mode_dims.entry(mode.id()).or_insert((0, 0, 0)); match event { ModeEvent::Size { width, height } => { @@ -370,6 +452,7 @@ impl Dispatch for State { entry.1 = height.max(0) as u32; } ModeEvent::Refresh { refresh } => entry.2 = refresh.max(0) as u32, + // `preferred` / `flags` / `cvt` carry nothing we drive an apply from. _ => {} } } @@ -419,13 +502,76 @@ struct Session { next_sync: u32, } +/// Why [`Session::open`] declined, i.e. why this operation degraded to the `kscreen-doctor` +/// shell-out. +/// +/// The distinction is the whole value of the type: a bare `None` made every one of these read as +/// "not a KDE box", which is how a genuine regression — KWin ≥ 6.7 no longer advertising per-output +/// `kde_output_device_v2` globals, so the device list came back EMPTY — shipped as a fallback that +/// fired on every current KDE machine with nothing in the log to say so. +enum OpenFailure { + /// No Wayland connection at all (`WAYLAND_DISPLAY` unset/stale) — not a session we can drive. + Connect(String), + /// The compositor accepted the connection but did not answer the registry barrier in budget: + /// the wedge case this whole module exists for. + RegistryBarrier, + /// Connected and answering, but `kde_output_management_v2` is not advertised to this client + /// (too old a KWin, or not KWin at all). + NoManagementGlobal, + /// Management is there, but the outputs' own property bursts never completed in budget. + DeviceBarrier, +} + +impl std::fmt::Display for OpenFailure { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + OpenFailure::Connect(e) => write!(f, "no Wayland connection ({e})"), + OpenFailure::RegistryBarrier => { + write!( + f, + "the compositor did not answer the registry roundtrip in budget" + ) + } + OpenFailure::NoManagementGlobal => { + write!( + f, + "kde_output_management_v2 is not advertised to this client" + ) + } + OpenFailure::DeviceBarrier => { + write!( + f, + "the outputs never finished announcing their state in budget" + ) + } + } + } +} + impl Session { + /// [`Session::connect`] for the operation named by `op`, logging the reason on the way out. + /// + /// One log site for all six callers: every one of them silently degraded to `kscreen-doctor` + /// before, so on a box where the in-process path never worked the only symptom was that + /// topology took ~26 s and nothing said why. + fn open(op: &'static str) -> Result { + let opened = Session::connect(); + if let Err(reason) = &opened { + tracing::warn!( + op, + %reason, + "KWin in-process output management unavailable — falling back to kscreen-doctor" + ); + } + opened + } + /// Connect to the KWin Wayland socket, bind `kde_output_management_v2` + every - /// `kde_output_device_v2`, and read each output's state — all bounded by `OP_BUDGET`. `None` if - /// we can't connect, the management global isn't advertised, or the compositor doesn't answer in - /// budget (the wedge case — the caller then falls back to `kscreen-doctor`). - fn open() -> Option { - let conn = Connection::connect_to_env().ok()?; + /// `kde_output_device_v2`, and read each output's state — all bounded by `OP_BUDGET`. The + /// [`OpenFailure`] says which rung declined; every one of them sends the caller to + /// `kscreen-doctor`. + fn connect() -> Result { + let conn = Connection::connect_to_env().map_err(|e| OpenFailure::Connect(e.to_string()))?; let queue = conn.new_event_queue(); let qh = queue.handle(); let _registry = conn.display().get_registry(&qh, ()); @@ -438,19 +584,15 @@ impl Session { let deadline = Instant::now() + OP_BUDGET; // Phase 1: process the registry globals (binds management + every device in the handler). if !s.sync_barrier(deadline) { - return None; + return Err(OpenFailure::RegistryBarrier); } if s.state.management.is_none() { - tracing::debug!( - "KWin does not advertise kde_output_management_v2 to this client — kscreen-doctor \ - fallback" - ); - return None; + return Err(OpenFailure::NoManagementGlobal); } // Phase 2: flush the device binds issued in phase 1 and drain each output's state burst // (name / enabled / priority / current_mode / mode sizes / done). if !s.sync_barrier(deadline) { - return None; + return Err(OpenFailure::DeviceBarrier); } // Phase 3 (KWin ≥ 6.7, the registry model): the devices themselves only arrive as the // registry's `output` events during phase 2, so their property bursts are one round further @@ -460,9 +602,9 @@ impl Session { && s.state.devices.values().any(|d| !d.seen_done) && !s.sync_barrier(deadline) { - return None; + return Err(OpenFailure::DeviceBarrier); } - Some(s) + Ok(s) } /// Send a `wl_display.sync` and pump the queue until its `done` arrives or `deadline` passes. @@ -554,6 +696,26 @@ impl Session { let id = dev.current_mode.as_ref()?; self.state.mode_dims.get(id).copied() } + + /// Resolve OUR just-created virtual output: a managed-prefix name AND a current size equal to + /// the size we created it at — only the just-created output sits there during a supersede, + /// because the replacement deliberately reuses the per-slot name while the predecessor is still + /// alive. Newest wins the remaining tie: the global `name` number where there is one, else + /// announce order (see [`DeviceState::seq`] — on KWin ≥ 6.7 that is every device). + /// + /// One resolve for all three operations (topology / de-mirror / custom mode). They had drifted + /// into three copies of the same filter, which is how a tie-break fix lands in two of them. + fn resolve_ours(&self, our_prefix: &str, our_w: u32, our_h: u32) -> Option { + self.state + .devices + .values() + .filter(|d| { + d.name.as_deref().is_some_and(|n| n.starts_with(our_prefix)) + && self.current_dims(d).map(|(w, h, _)| (w, h)) == Some((our_w, our_h)) + }) + .max_by_key(|d| (d.global, d.seq)) + .cloned() + } } /// `(width, height, "WxH@Hz")` capture of a device's current mode, Hz rounded — the same shape the @@ -563,10 +725,10 @@ fn mode_spec(dims: (u32, u32, u32)) -> String { format!("{}x{}@{}", dims.0, dims.1, hz) } -/// Prefix EVERY managed KWin output shares (mirrors `kwin::MANAGED_PREFIX`) — the streamed outputs -/// are `Virtual-punktfunk` / `Virtual-punktfunk-`, so a same-family sibling session is never -/// treated as a physical to disable, and its primary is never stolen (first-slot-wins). -const MANAGED_PREFIX: &str = "Virtual-punktfunk"; +// `MANAGED_PREFIX` — the prefix EVERY managed KWin output shares (`Virtual-punktfunk` / +// `Virtual-punktfunk-`), so a same-family sibling session is never treated as a physical to +// disable and its primary is never stolen (first-slot-wins) — is imported at the top of this file +// from `kwin.rs`, which owns the naming. /// Every head KWin reports, for [`crate::monitors::list`]. /// @@ -575,12 +737,8 @@ const MANAGED_PREFIX: &str = "Virtual-punktfunk"; /// burst is skipped rather than reported half-read (its geometry would be a guess, and geometry is /// exactly what callers key on). pub(crate) fn list_monitors() -> anyhow::Result> { - let session = Session::open().ok_or_else(|| { - anyhow::anyhow!( - "KWin did not answer kde_output_management_v2 (not a KWin session, the protocol is \ - not advertised to this client, or the compositor is wedged)" - ) - })?; + let session = Session::open("list_monitors") + .map_err(|e| anyhow::anyhow!("KWin did not answer kde_output_management_v2: {e}"))?; let mut out: Vec<_> = session .state .devices @@ -630,24 +788,12 @@ pub(crate) fn apply_topology( disabled: Vec::new(), handled: false, }; - let Some(mut sess) = Session::open() else { + let Ok(mut sess) = Session::open("topology") else { return miss(); }; let deadline = Instant::now() + OP_BUDGET; - // Resolve OUR output: managed-prefix name AND current size == the birth size (only the - // just-created output sits there during a supersede); newest global wins the tie. - let ours = sess - .state - .devices - .values() - .filter(|d| { - d.name.as_deref().is_some_and(|n| n.starts_with(our_prefix)) - && sess.current_dims(d).map(|(w, h, _)| (w, h)) == Some((our_w, our_h)) - }) - .max_by_key(|d| d.global) - .cloned(); - let Some(ours) = ours else { + let Some(ours) = sess.resolve_ours(our_prefix, our_w, our_h) else { tracing::warn!( our_prefix, our_w, @@ -846,7 +992,7 @@ pub(crate) fn apply_topology( /// which is broken under every topology equally. So this reads the state and applies **only** when /// our output really is mirroring; the ordinary session pays one bounded enumerate and no apply. pub(crate) fn clear_replication_source(our_prefix: &str, our_w: u32, our_h: u32) { - let Some(mut sess) = Session::open() else { + let Ok(mut sess) = Session::open("clear_replication_source") else { return; }; let deadline = Instant::now() + OP_BUDGET; @@ -858,18 +1004,7 @@ pub(crate) fn clear_replication_source(our_prefix: &str, our_w: u32, our_h: u32) if mgmt_version < REPLICATION_SOURCE_SINCE { return; } - // Same resolve as `apply_topology`: managed-prefix name AND the birth size, newest global wins. - let Some(ours) = sess - .state - .devices - .values() - .filter(|d| { - d.name.as_deref().is_some_and(|n| n.starts_with(our_prefix)) - && sess.current_dims(d).map(|(w, h, _)| (w, h)) == Some((our_w, our_h)) - }) - .max_by_key(|d| d.global) - .cloned() - else { + let Some(ours) = sess.resolve_ours(our_prefix, our_w, our_h) else { return; }; if !is_mirroring(ours.replication_source.as_deref()) { @@ -918,7 +1053,7 @@ pub(crate) fn set_custom_mode( want_h: u32, want_hz: u32, ) -> Option<(u32, u32, u32)> { - let mut sess = Session::open()?; + let mut sess = Session::open("custom_mode").ok()?; let deadline = Instant::now() + OP_BUDGET; // `set_custom_modes` is `since 18`; calling it on an older bound management object is a protocol @@ -928,16 +1063,9 @@ pub(crate) fn set_custom_mode( return None; } - // Resolve our output at its birth size (newest global wins a supersede). + // Resolve our output at its birth size (newest wins a supersede — see `resolve_ours`). let our_proxy = sess - .state - .devices - .values() - .filter(|d| { - d.name.as_deref().is_some_and(|n| n.starts_with(our_prefix)) - && sess.current_dims(d).map(|(w, h, _)| (w, h)) == Some((birth_w, birth_h)) - }) - .max_by_key(|d| d.global) + .resolve_ours(our_prefix, birth_w, birth_h) .and_then(|d| d.proxy.clone())?; let our_key = our_proxy.id(); @@ -985,10 +1113,15 @@ pub(crate) fn set_custom_mode( } // Grab the generated mode's proxy, then select it (this is what changes the size). + // Newest match wins: `modes` is in announce order, and the entry we just had KWin generate is + // the last one. An earlier session's identical custom mode may still be listed here — KWin only + // destroys it (`kde_output_device_mode_v2.removed`) when it processes our `set_custom_modes`, + // and that removal may not have been dispatched yet. let mode_proxy = { let dev = sess.state.devices.get(&our_key)?; dev.modes .iter() + .rev() .find(|(mid, _)| mode_matches(&sess.state, mid)) .map(|(_, p)| p.clone())? }; @@ -1031,19 +1164,31 @@ pub(crate) fn set_custom_mode( } /// Re-enable outputs by name at their captured `WxH@Hz` modes (teardown), in-process. Returns -/// `true` if the config applied; `false` (compositor unresponsive / management absent) tells the -/// caller to fall back to `kscreen-doctor`. +/// `true` only if EVERY requested output was staged and the config applied; `false` (compositor +/// unresponsive, management absent, or an output we could not address) tells the caller to fall +/// back to `kscreen-doctor`. +/// +/// The "every requested output" half is load-bearing, not pedantry. The names in `outputs` were +/// captured on a DIFFERENT connection during [`apply_topology`] and this restore opens a fresh +/// session minutes later, when the display group's last member drops — so a name that no longer +/// resolves is a live possibility. An empty `kde_output_configuration_v2` still gets an `applied` +/// event, so returning the apply verdict alone reported SUCCESS for a total no-op, suppressed the +/// `reenable_outputs_kscreen` backstop, and left a physical monitor dark. pub(crate) fn reenable_outputs(outputs: &[(String, String)]) -> bool { if outputs.is_empty() { return true; } - let Some(mut sess) = Session::open() else { + let Ok(mut sess) = Session::open("restore_outputs") else { return false; }; let deadline = Instant::now() + OP_BUDGET; let config = sess.new_config(); + let mut matched = 0usize; for (name, spec) in outputs { - // Find the device by name (physical names are stable across a session). + // Find the device by name (physical names are stable across a session). BOTH misses below + // leave `matched` un-incremented, the proxy one included: a `DeviceState` can be created by + // the event handler ([`State::device_entry`]) and carry a name before the announce that + // records its proxy has been dispatched, and a name with no proxy is not addressable. let Some(dev) = sess .state .devices @@ -1056,6 +1201,7 @@ pub(crate) fn reenable_outputs(outputs: &[(String, String)]) -> bool { let Some(proxy) = dev.proxy.as_ref() else { continue; }; + matched += 1; // Enable first — a bare enable always succeeds, so a physical is never left dark. config.enable(proxy, 1); // Then re-assert the captured mode so a 120 Hz panel doesn't return at KWin's ~60 Hz default. @@ -1063,18 +1209,39 @@ pub(crate) fn reenable_outputs(outputs: &[(String, String)]) -> bool { config.mode(proxy, &mode); } } + if matched == 0 { + // Nothing staged: applying would ack an empty config and read as success. Hand the whole + // restore to kscreen-doctor, which addresses outputs by name and needs no live proxy. + config.destroy(); + tracing::warn!( + requested = ?outputs, + "KWin output management: none of the outputs to restore are addressable on this \ + connection — kscreen-doctor fallback" + ); + return false; + } let ok = sess.apply(&config, deadline); config.destroy(); - if ok { + let complete = ok && matched == outputs.len(); + if complete { tracing::info!(reenabled = ?outputs, "KWin output management: restored outputs (in-process)"); + } else { + tracing::warn!( + requested = ?outputs, + matched, + applied = ok, + reason = ?sess.state.failure_reason, + "KWin output management: restore incomplete — kscreen-doctor backstop takes the rest \ + (an output left disabled is a physical left dark)" + ); } - ok + complete } /// Position the output identified by `uuid` at `(x, y)` in the desktop layout, in-process. Returns /// `true` if applied; `false` tells the caller to fall back to `kscreen-doctor`. pub(crate) fn set_position(uuid: &str, x: i32, y: i32) -> bool { - let Some(mut sess) = Session::open() else { + let Ok(mut sess) = Session::open("position") else { return false; }; let deadline = Instant::now() + OP_BUDGET; diff --git a/crates/pf-vdisplay/src/vdisplay/monitors.rs b/crates/pf-vdisplay/src/vdisplay/monitors.rs index d46ede70..2a2c639f 100644 --- a/crates/pf-vdisplay/src/vdisplay/monitors.rs +++ b/crates/pf-vdisplay/src/vdisplay/monitors.rs @@ -18,15 +18,22 @@ use crate::Compositor; use anyhow::{bail, Result}; -/// One head as the compositor currently reports it. Logical (post-scale) geometry throughout — -/// the same coordinate space libei regions and compositor layout use, *not* pixels. +/// One head as the compositor currently reports it. +/// +/// **The two halves live in different spaces, and that is not an accident.** `x`/`y` are LOGICAL — +/// the compositor's global layout coordinates, the same space libei regions use — while +/// `width`/`height` are the current mode in PIXELS, because that is what every backend actually +/// reports (KWin's `current_mode` size, `hyprctl`'s mode, the CCD path's source mode) and what a +/// capturer has to open against. `scale` is the factor between them: see [`Self::logical_size`], +/// which is the only correct way to compare a size against `x`/`y`. An earlier version of this doc +/// claimed logical geometry "throughout", which is a trap for exactly the consumer that mixes them. #[derive(Clone, Debug, PartialEq)] pub struct PhysicalMonitor { /// Connector name — `DP-1`, `HDMI-A-2`, `eDP-1`. The id `PUNKTFUNK_CAPTURE_MONITOR` names. pub connector: String, /// Human label for a picker (`make model`, else the connector). Never used for matching. pub description: String, - /// Current mode, in pixels. + /// Current mode, in PIXELS (not the logical size — see the type doc and [`Self::logical_size`]). pub width: u32, pub height: u32, /// Refresh in mHz (60000 = 60 Hz). 0 when the backend doesn't report it. @@ -71,6 +78,24 @@ pub(crate) fn describe(make: &str, model: &str, connector: &str) -> String { } impl PhysicalMonitor { + /// The head's extent in the SAME space as `x`/`y` — mode pixels divided by `scale`. + /// + /// The bridge between the two spaces this type carries, and the only correct way to ask "does + /// this head's box contain that layout coordinate?". A consumer that compares `width`/`height` + /// against `x`/`y` directly is right only at scale 1.0 and silently wrong on every fractional + /// KDE/GNOME desk (a 3840-px panel at 150 % occupies 2560 logical units, so a naive + /// `x + width` overlaps the head to its right by 1280). + /// + /// A non-positive scale can only come from a backend that reported nonsense; it is treated as + /// 1.0 rather than dividing by zero. + pub fn logical_size(&self) -> (f64, f64) { + let scale = if self.scale > 0.0 { self.scale } else { 1.0 }; + ( + f64::from(self.width) / scale, + f64::from(self.height) / scale, + ) + } + /// `1920x1080@60` — for logs and pickers. pub fn mode_label(&self) -> String { if self.refresh_mhz == 0 { @@ -94,8 +119,11 @@ impl PhysicalMonitor { /// callers resolving a pinned monitor must not (see [`resolve`]). pub fn list(compositor: Compositor) -> Result> { match compositor { + // Via the `kwin` backend rather than `kwin_output_mgmt` directly: it owns the + // in-process-then-`kscreen-doctor` ladder, so this read degrades the same way every other + // KWin operation does instead of being the one that hard-fails on a wedged/old compositor. #[cfg(target_os = "linux")] - Compositor::Kwin => crate::kwin_output_mgmt::list_monitors(), + Compositor::Kwin => crate::kwin::list_monitors(), #[cfg(target_os = "linux")] Compositor::Mutter => crate::mutter::list_monitors(), #[cfg(target_os = "linux")] @@ -133,15 +161,21 @@ pub fn list(compositor: Compositor) -> Result> { /// * `refresh_mhz` comes from the path's own rational rate, which keeps 59.94 distinct from 60. #[cfg(windows)] pub fn list_windows() -> Result> { - let inv = pf_win_display::win_display::target_inventory(); - if inv.is_empty() { - // Distinguish "reached it, nothing there" from a failure, exactly as [`list`] promises: - // an empty CCD database is a real state (every panel off — measured on .173 with the TV - // powered down), not an error. - return Ok(Vec::new()); - } - Ok(inv - .into_iter() + // `Ok` even when the inventory is empty, exactly as [`list`] promises: an empty CCD database is + // a real state (every panel off — measured on .173 with the TV powered down), not a failure. + // Everything past the OS call is the pure mapping, so it lives where a test can reach it. + Ok(from_inventory( + pf_win_display::win_display::target_inventory(), + )) +} + +/// The CCD inventory → [`PhysicalMonitor`] mapping, split from the OS call so the Windows test leg +/// can exercise it (`list_windows` touches the display database on its first line, which left the +/// only mapping that decides what an operator can PIN with no coverage on the one platform that +/// runs it). +#[cfg(windows)] +fn from_inventory(inv: Vec) -> Vec { + inv.into_iter() .map(|t| { // The GDI name is what an operator recognises and what capture pins on; an inactive // path has none, so fall back to the stable target id rather than an empty string — @@ -167,7 +201,7 @@ pub fn list_windows() -> Result> { managed: t.ours, } }) - .collect()) + .collect() } /// Resolve a configured monitor name against `monitors`, exactly then case-insensitively. @@ -257,6 +291,24 @@ mod tests { assert_eq!(describe(" ", "unknown", "DP-2"), "DP-2"); } + /// The two spaces this type carries: the mode is pixels, `x`/`y` are logical, and `scale` is + /// the only thing that relates them. A 4K panel at KDE's 150 % really does occupy 2560x1440 + /// logical units, which is what a consumer comparing against `x`/`y` must use. + #[test] + fn logical_size_divides_the_mode_by_the_scale() { + let mut m = mon("DP-1"); + m.width = 3840; + m.height = 2160; + m.scale = 1.5; + assert_eq!(m.logical_size(), (2560.0, 1440.0)); + // Unscaled: the two spaces coincide, which is why the trap goes unnoticed on most desks. + m.scale = 1.0; + assert_eq!(m.logical_size(), (3840.0, 2160.0)); + // A backend that reported nonsense must not produce an infinity or a NaN. + m.scale = 0.0; + assert_eq!(m.logical_size(), (3840.0, 2160.0)); + } + #[test] fn mode_label_drops_an_unknown_refresh() { let mut m = mon("DP-1"); @@ -265,3 +317,84 @@ mod tests { assert_eq!(m.mode_label(), "1920x1080"); } } + +/// The Windows inventory mapping. Windows-only because it maps a Windows-only type — the CI leg +/// that runs it (`windows-host.yml`, `cargo test --release -p pf-vdisplay`) already exists; until +/// [`from_inventory`] was split out of the OS call there was simply nothing there to run. +#[cfg(all(test, windows))] +mod windows_tests { + use super::*; + use pf_win_display::win_display::TargetInventory; + + /// One inventory row. Built through a single helper so a field rename shows up in one place — + /// the struct is another crate's and carries no `Default`. + fn target(target_id: u32, gdi_name: &str, active: bool) -> TargetInventory { + TargetInventory { + target_id, + active, + external_physical: true, + internal_panel: false, + tech: "HDMI", + friendly: "ACME TV".into(), + monitor_device_path: r"\\?\DISPLAY#ACM1234#".into(), + ours: false, + gdi_name: gdi_name.into(), + x: 0, + y: 0, + width: 1920, + height: 1080, + refresh_mhz: 59940, + primary: active, + } + } + + /// An INACTIVE path has no source and therefore no GDI name. It must still be listed (the + /// "why can't I pick it?" contract) under an id that can actually be pinned — a blank connector + /// could never be resolved, and an operator would have no way to name the head at all. + #[test] + fn an_inactive_path_gets_a_target_id_connector_and_enabled_false() { + let mons = from_inventory(vec![target(4352, "", false)]); + assert_eq!(mons.len(), 1); + assert_eq!(mons[0].connector, "target-4352"); + assert!(!mons[0].enabled); + // Windows applies DPI per application rather than a compositor-global logical scale, so + // the geometry above is pixels and the factor is honestly 1.0 — see the fn doc. + assert_eq!(mons[0].scale, 1.0); + } + + /// The two halves must agree: whatever connector this mapping synthesizes has to be a name + /// [`resolve`] can find, because that pair is the whole pin round-trip the console offers. + #[test] + fn resolve_can_find_a_synthesized_target_name() { + let mons = from_inventory(vec![ + target(4352, "", false), + target(1, r"\\.\DISPLAY1", true), + ]); + assert_eq!( + resolve(&mons, "target-4352") + .expect("synthesized name") + .width, + 1920 + ); + // An active path keeps its GDI name — the id an operator recognises. + assert_eq!( + resolve(&mons, r"\\.\DISPLAY1").expect("gdi name").connector, + r"\\.\DISPLAY1" + ); + assert!( + resolve(&mons, r"\\.\display1").is_ok(), + "and case-insensitively, as `resolve` promises" + ); + } + + /// Our own IddCx display is flagged, so a picker can grey it out — the one thing Windows can + /// answer reliably and the Linux backends cannot. + #[test] + fn our_own_idd_is_marked_managed() { + let mut ours = target(257, r"\\.\DISPLAY2", true); + ours.ours = true; + let mons = from_inventory(vec![ours]); + assert!(mons[0].managed); + assert!(!from_inventory(vec![target(1, r"\\.\DISPLAY1", true)])[0].managed); + } +}