From fd75e660411c0750a9bc16b4ef42b877335f37a4 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 31 Jul 2026 22:05:05 +0200 Subject: [PATCH] feat(windows): the shell spawns from the shared brain, and links, tests and pairing stop losing things MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows-shell parity gaps from the 2026-07-31 sweep, closed: * The spawner goes through the shared brain: `ConnectPlan::for_target` → `session_args()` + a written `--resolved-spec`, replacing the hand-assembled argv that meant Windows sessions ALWAYS took the compat path (re-resolving every setting from the stores — the drift orchestrate.rs documents as a trap) and that any field added to the spec was silently Windows-dead. Fullscreen now comes from the plan's effective settings (profile-aware) instead of a caller argument, the spec temp file is cleaned up at exit, and the reader finally parses the `{"window":…}` line so the SPAWNER persists the match-window size (§5) instead of the renderer's load-modify-save fallback. * A deep link to an unpaired host runs the trust ceremony instead of refusing: the Pair screen opens seeded with what the link CLAIMED — name shown as claimed, fingerprint carried, and the launch/profile surviving the detour (`Target` grew a `launch` field for exactly this). GTK parity; a shared link was a dead end here before. * The speed test learns the Ask tier: a bound host whose profile INHERITS bitrate now offers both "Set as default" and "Set in profile" instead of silently creating a profile override. The global write (and the forwarded-controller handler) also rebase on the file before their whole-struct saves — two more stale-snapshot writers. * The Controllers card shows the detected-pad inventory (read-only, with the Steam-Input-virtual note) — the fastest answer to "is my controller even detected?", present on GTK all along. Co-Authored-By: Claude Fable 5 --- clients/windows/src/app/connect.rs | 16 +--- clients/windows/src/app/hosts.rs | 3 + clients/windows/src/app/mod.rs | 47 +++++++++--- clients/windows/src/app/settings.rs | 45 +++++++++++- clients/windows/src/app/speed.rs | 110 ++++++++++++++++++---------- clients/windows/src/spawn.rs | 102 +++++++++++++++++++------- 6 files changed, 239 insertions(+), 84 deletions(-) diff --git a/clients/windows/src/app/connect.rs b/clients/windows/src/app/connect.rs index bd2b0556..7b9ed191 100644 --- a/clients/windows/src/app/connect.rs +++ b/clients/windows/src/app/connect.rs @@ -251,16 +251,6 @@ fn connect_spawn( *ctx.shared.session.lock().unwrap() = child.clone(); ctx.shared.stats_line.lock().unwrap().clear(); ctx.shared.browse.store(false, Ordering::SeqCst); - // Through the same resolver the session uses, not the raw globals: "Start streams - // fullscreen" is a profileable (tier-P) field, so a host bound to a windowed profile has to - // win here too — the child takes this decision from the argv, not from its own settings. - let fullscreen = pf_client_core::trust::effective_settings( - &target.addr, - target.port, - target.profile.as_deref(), - ) - .0 - .fullscreen_on_stream; set_status.call(String::new()); set_screen.call(if opts.awaiting_approval { Screen::RequestAccess @@ -278,13 +268,15 @@ fn connect_spawn( // The closure owns `target`/`fp_hex`; the call itself borrows copies. let (addr, port, fp_arg) = (target.addr.clone(), target.port, fp_hex.clone()); let profile_arg = target.profile.clone(); + // The launch id: an explicit opts pick (the library's tap-to-play), else one riding + // the target — a deep link's `launch=` that detoured through the PIN ceremony. + let launch_arg = opts.launch.clone().or_else(|| target.launch.clone()); let spawned = crate::spawn::spawn_session( &addr, port, &fp_arg, opts.connect_timeout.as_secs(), - fullscreen, - opts.launch.as_deref(), + launch_arg.as_deref(), profile_arg.as_deref(), child, move |event| { diff --git a/clients/windows/src/app/hosts.rs b/clients/windows/src/app/hosts.rs index da6f9cbc..5312d5e6 100644 --- a/clients/windows/src/app/hosts.rs +++ b/clients/windows/src/app/hosts.rs @@ -670,6 +670,7 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element { pair_optional: false, mac: k.mac.clone(), profile: None, + launch: None, }; // Online = advertising on mDNS OR proven reachable by the last probe sweep (the latter // covers a routed/Tailscale host that never advertises — the display companion to @@ -959,6 +960,7 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element { pair_optional: h.pair == "optional", mac: h.mac.clone(), profile: None, + launch: None, }; let (ctx2, ss, st) = (ctx.clone(), set_screen.clone(), set_status.clone()); let (badge, kind) = if h.pair == "required" { @@ -1052,6 +1054,7 @@ pub(crate) fn hosts_page(props: &HostsProps, cx: &mut RenderCx) -> Element { pair_optional: false, mac: Vec::new(), profile: None, + launch: None, }, &ss, &st, diff --git a/clients/windows/src/app/mod.rs b/clients/windows/src/app/mod.rs index 6c4f6d05..a218882f 100644 --- a/clients/windows/src/app/mod.rs +++ b/clients/windows/src/app/mod.rs @@ -107,6 +107,10 @@ pub(crate) struct Target { /// `None` honors the binding. It never rebinds anything — the default changes only through /// the picker in the host editor (design/client-settings-profiles.md §5.2). pub(crate) profile: Option, + /// A library title id (`steam:570`, …) to launch on connect — carried on the target so it + /// survives a detour through the PIN ceremony (a deep link's `launch=` toward an unpaired + /// host must still launch the game once pairing succeeds). + pub(crate) launch: Option, } /// Stable app services handed to the page components as props. Each routed screen that uses @@ -392,6 +396,7 @@ fn root(cx: &mut RenderCx, ctx: &Arc) -> Element { pair_optional: false, mac: p.host.mac.clone(), profile: p.profile_override.clone(), + launch: None, // routed explicitly below (initiate_launch*) }; // With a MAC it takes the dial first wake path, so a sleeping host wakes // instead of erroring — exactly what clicking its tile would do. The @@ -399,22 +404,46 @@ fn root(cx: &mut RenderCx, ctx: &Arc) -> Element { // to drop it, so a game link opened a plain desktop session. match (p.launch.clone(), p.wake && !target.mac.is_empty()) { (Some(id), true) => { - connect::initiate_launch_waking(&ctx, target, id, &set_screen, &set_status); + connect::initiate_launch_waking( + &ctx, + target, + id, + &set_screen, + &set_status, + ); } (Some(id), false) => { connect::initiate_launch(&ctx, target, id, &set_screen, &set_status); } - (None, true) => connect::initiate_waking(&ctx, target, &set_screen, &set_status), + (None, true) => { + connect::initiate_waking(&ctx, target, &set_screen, &set_status) + } (None, false) => connect::initiate(&ctx, target, &set_screen, &set_status), } } - // Known but never pinned, or not known at all: a link may not pair or trust on - // its own, so it lands on the host list with the reason shown. The user pairs - // there, under their own eyes. - Ok(PlanOutcome::ConfirmUnknown(u)) => refuse(format!( - "{} isn't paired with this device yet \u{2014} pair it, then use the link again.", - u.name.clone().unwrap_or_else(|| u.addr.clone()) - )), + // Known but never pinned, or not known at all: a link may not pair and may not + // trust on its own, so it opens the ordinary PIN ceremony seeded with what the + // link CLAIMED — name shown as claimed, the fingerprint pre-filling the pin so + // the first connect is verified against it rather than blind TOFU, and the + // launch/profile surviving the detour (§3.1; GTK-shell parity — this used to + // refuse outright and make shared links a dead end on Windows). + Ok(PlanOutcome::ConfirmUnknown(u)) => { + let name = u.name.clone().unwrap_or_else(|| u.addr.clone()); + *ctx.shared.target.lock().unwrap() = Target { + name: name.clone(), + addr: u.addr.clone(), + port: u.port, + fp_hex: u.fp.clone(), + pair_optional: false, + mac: Vec::new(), + profile: u.profile.clone(), + launch: u.launch.clone(), + }; + set_status.call(format!( + "{name} isn't paired with this device yet \u{2014} pair it to continue." + )); + set_screen.call(Screen::Pair); + } Ok(PlanOutcome::Unsupported(route)) => refuse(format!( "Punktfunk can't open \u{201c}{}\u{201d} links yet.", route.as_str() diff --git a/clients/windows/src/app/settings.rs b/clients/windows/src/app/settings.rs index 0f3277a0..55deeb45 100644 --- a/clients/windows/src/app/settings.rs +++ b/clients/windows/src/app/settings.rs @@ -886,9 +886,12 @@ pub(crate) fn settings_page( keys.get(sel - 1).cloned() }; // Apply live to the gamepad service and persist — the spawned session - // reads `forward_pad` at connect. + // reads `forward_pad` at connect. Rebase on the file first (the same + // discipline as `commit()`): this handler bypasses commit and a stale + // whole-struct save would revert other writers. svc.set_pinned(key.clone()); let mut s = ctx2.settings.lock().unwrap(); + *s = Settings::load(); s.forward_pad = key.unwrap_or_default(); s.save(); }) @@ -1172,6 +1175,46 @@ pub(crate) fn settings_page( group( None, [ + // The read-only pad inventory (GTK parity): what THIS device sees right + // now — the fastest answer to "is my controller even detected?". A + // device fact, so defaults scope only, like the forward picker below. + (!profile_mode).then(|| { + let inventory: Element = if pads.is_empty() { + text_block("No controllers detected") + .font_size(12.0) + .foreground(ThemeRef::SecondaryText) + .into() + } else { + vstack( + pads.iter() + .map(|p| { + let sub = if p.steam_virtual { + "Steam Input's virtual pad \u{2014} Automatic skips \ + it while a real pad is connected" + .to_string() + } else { + p.kind_label().to_string() + }; + vstack(( + text_block(p.name.clone()).semibold(), + text_block(sub) + .font_size(11.0) + .foreground(ThemeRef::SecondaryText), + )) + .spacing(1.0) + .into() + }) + .collect::>(), + ) + .spacing(8.0) + .into() + }; + described_labeled( + "Detected controllers", + inventory, + "Plug in or pair a controller and it appears here.", + ) + }), // NOT Apple's wording: Apple forwards ONE pad as player 1, this client // forwards every controller as its own player. Same picker, different rule. // Which physical pad this device forwards is a device fact (tier G), so it diff --git a/clients/windows/src/app/speed.rs b/clients/windows/src/app/speed.rs index 47ed665d..9f70b6aa 100644 --- a/clients/windows/src/app/speed.rs +++ b/clients/windows/src/app/speed.rs @@ -140,39 +140,80 @@ pub(crate) fn speed_page(props: &SpeedProps, cx: &mut RenderCx) -> Element { None => bound, } .and_then(|reference| ProfilesFile::load().resolve(&reference).0.cloned()); - let apply_btn = { - let (ctx, ss, kbps) = (ctx.clone(), set_screen.clone(), *recommended_kbps); - let profile = profile.clone(); - button(match &profile { - Some(p) => format!( - "Set {recommended_mbps:.0} Mb/s in \u{201c}{}\u{201d}", - p.name - ), - None => format!("Use {recommended_mbps:.0} Mb/s"), - }) - .accent() - .icon(Symbol::Accept) - .on_click(move || { - match &profile { - Some(p) => { - let mut catalog = ProfilesFile::load(); - if let Some(slot) = catalog.profiles.iter_mut().find(|x| x.id == p.id) { - slot.overrides.bitrate_kbps = Some(kbps); - if let Err(e) = catalog.save() { - tracing::warn!(error = %format!("{e:#}"), - "saving the measured bitrate"); - } - } - } - None => { - let mut s = ctx.settings.lock().unwrap(); - s.bitrate_kbps = kbps; - s.save(); + let kbps = *recommended_kbps; + let write_global = { + let (ctx, ss) = (ctx.clone(), set_screen.clone()); + move || { + // Rebase on the file before the whole-struct save — same discipline as + // `commit()`; another writer may have moved it under this snapshot. + let mut s = ctx.settings.lock().unwrap(); + *s = crate::trust::Settings::load(); + s.bitrate_kbps = kbps; + s.save(); + ss.call(Screen::Hosts); + } + }; + let write_profile = |id: String| { + let ss = set_screen.clone(); + move || { + let mut catalog = ProfilesFile::load(); + if let Some(slot) = catalog.profiles.iter_mut().find(|x| x.id == id) { + slot.overrides.bitrate_kbps = Some(kbps); + if let Err(e) = catalog.save() { + tracing::warn!(error = %format!("{e:#}"), + "saving the measured bitrate"); } } ss.call(Screen::Hosts); - }) + } }; + // Which button(s): no binding → the global; a binding that already overrides + // bitrate → that override (it's what this host reads). Bound but INHERITING + // bitrate could legitimately mean either layer — offer both rather than + // guessing (the GTK client's Ask tier; this shell used to silently CREATE an + // override on the profile). + let mut buttons: Vec = Vec::new(); + match &profile { + None => buttons.push( + button(format!("Use {recommended_mbps:.0} Mb/s")) + .accent() + .icon(Symbol::Accept) + .on_click(write_global.clone()) + .into(), + ), + Some(p) if p.overrides.bitrate_kbps.is_some() => buttons.push( + button(format!( + "Set {recommended_mbps:.0} Mb/s in \u{201c}{}\u{201d}", + p.name + )) + .accent() + .icon(Symbol::Accept) + .on_click(write_profile(p.id.clone())) + .into(), + ), + Some(p) => { + buttons.push( + button("Set as default") + .icon(Symbol::Accept) + .on_click(write_global.clone()) + .into(), + ); + buttons.push( + button(format!("Set in \u{201c}{}\u{201d}", p.name)) + .accent() + .icon(Symbol::Accept) + .on_click(write_profile(p.id.clone())) + .into(), + ); + } + } + buttons.push({ + let ss = set_screen.clone(); + button("Close") + .icon(Symbol::Cancel) + .on_click(move || ss.call(Screen::Hosts)) + .into() + }); let results = card( vstack(( text_block(format!("{mbps:.0} Mbit/s")) @@ -190,14 +231,9 @@ pub(crate) fn speed_page(props: &SpeedProps, cx: &mut RenderCx) -> Element { .font_size(12.0) .foreground(ThemeRef::SecondaryText) .horizontal_alignment(HorizontalAlignment::Center), - hstack((apply_btn, { - let ss = set_screen.clone(); - button("Close") - .icon(Symbol::Cancel) - .on_click(move || ss.call(Screen::Hosts)) - })) - .spacing(8.0) - .horizontal_alignment(HorizontalAlignment::Center), + hstack(buttons) + .spacing(8.0) + .horizontal_alignment(HorizontalAlignment::Center), )) .spacing(12.0), ); diff --git a/clients/windows/src/spawn.rs b/clients/windows/src/spawn.rs index cf92eb68..536fd618 100644 --- a/clients/windows/src/spawn.rs +++ b/clients/windows/src/spawn.rs @@ -55,9 +55,19 @@ impl SessionChild { /// One parsed stdout line of the session contract; `None` for anything unrecognized. enum ChildLine { Ready, - Error { msg: String, trust_rejected: bool }, + Error { + msg: String, + trust_rejected: bool, + }, Ended(String), Stats(String), + /// The session window's logical size settled here under match-window — the SPAWNER + /// persists it (design/client-architecture-split.md §5). This shell ignored the line + /// until 2026-07-31, so its sessions fell back to persisting from the renderer. + Window { + w: u32, + h: u32, + }, } fn parse_line(line: &str) -> Option { @@ -77,6 +87,12 @@ fn parse_line(line: &str) -> Option { if let Some(msg) = v.get("ended").and_then(|m| m.as_str()) { return Some(ChildLine::Ended(msg.to_string())); } + if let Some(win) = v.get("window") { + let dim = |k: &str| win.get(k).and_then(|n| n.as_u64()).map(|n| n as u32); + if let (Some(w), Some(h)) = (dim("w"), dim("h")) { + return Some(ChildLine::Window { w, h }); + } + } None } @@ -107,42 +123,60 @@ pub(crate) fn session_binary() -> std::path::PathBuf { /// Spawn the session binary for a connect with `fp_hex` pinned and feed its lifecycle to /// `on_event` from a reader thread. The child is parked in `slot` so Disconnect/Cancel -/// can kill it. `fullscreen` starts the stream window fullscreen (the Settings "Start -/// streams fullscreen" toggle); `launch` carries a library title id for the host to -/// launch during the handshake. `Err` = the spawn itself failed (binary missing?) — -/// surfaced as a connect error by the caller. +/// can kill it. `launch` carries a library title id for the host to launch during the +/// handshake; `profile` is a ONE-OFF settings-profile pick. `Err` = the spawn itself +/// failed (binary missing?) — surfaced as a connect error by the caller. +/// +/// The argv and the `--resolved-spec` both come from the shared brain +/// ([`ConnectPlan::for_target`] → `session_args()` + `spec()`): this shell hand-assembled +/// its argv until 2026-07-31, which meant no spec — its sessions took the compat path and +/// re-resolved every setting from the stores (the drift `orchestrate.rs` documents as a +/// trap), and any field added to the spec was silently Windows-dead. Fullscreen now also +/// comes from the plan's EFFECTIVE settings (profile-aware) instead of a caller argument. #[allow(clippy::too_many_arguments)] // one cohesive spawn spec (session_params precedent) pub(crate) fn spawn_session( addr: &str, port: u16, fp_hex: &str, connect_timeout_secs: u64, - fullscreen: bool, launch: Option<&str>, profile: Option<&str>, slot: SessionChild, on_event: impl FnMut(SpawnEvent) + Send + 'static, ) -> Result<(), String> { + use pf_client_core::orchestrate::{ConnectPlan, HostTarget}; + let mut plan = ConnectPlan::for_target( + HostTarget { + name: String::new(), // display-only; this shell's screens carry their own copy + addr: addr.to_string(), + port, + fp_hex: Some(fp_hex.to_string()), + mac: Vec::new(), // wake ran before this spawn (initiate_waking) — not the plan's job + id: None, + }, + launch.map(str::to_string), + profile.map(str::to_string), + ); + plan.connect_timeout_secs = Some(connect_timeout_secs); let mut cmd = Command::new(session_binary()); - cmd.arg("--connect") - .arg(format!("{addr}:{port}")) - .arg("--fp") - .arg(fp_hex) - .arg("--connect-timeout") - .arg(connect_timeout_secs.to_string()); - if fullscreen { - cmd.arg("--fullscreen"); - } - if let Some(id) = launch { - cmd.arg("--launch").arg(id); - } - // Only a ONE-OFF pick rides the flag: without it the session resolves the host's own - // binding through the same helper this shell would have used, so the two can't disagree. - if let Some(reference) = profile { - cmd.arg("--profile").arg(reference); - } + let mut args = plan.session_args(); + // Spec mode (design/client-architecture-split.md §5): the child reads no stores and + // cannot disagree with us about a file either of us might write. A spec we fail to + // write is not fatal — the compat path resolves the same values via the same helper. + let spec_path = match plan.spec(plan.clipboard).write_temp() { + Ok(path) => { + args.push("--resolved-spec".into()); + args.push(path.to_string_lossy().into_owned()); + Some(path) + } + Err(e) => { + tracing::warn!(error = %e, "couldn't write the resolved spec; the session will resolve for itself"); + None + } + }; + cmd.args(args); add_window_pos(&mut cmd); - spawn_with(cmd, &format!("{addr}:{port}"), slot, on_event) + spawn_with(cmd, &format!("{addr}:{port}"), spec_path, slot, on_event) } /// Spawn the session binary in `--browse` mode: the console (gamepad) library for a @@ -169,7 +203,7 @@ pub(crate) fn spawn_browse( } add_window_pos(&mut cmd); let label = target.map_or_else(|| "console".to_string(), |(a, p)| format!("{a}:{p}")); - spawn_with(cmd, &label, slot, on_event) + spawn_with(cmd, &label, None, slot, on_event) } /// Hand the shell window's position to the child (`--window-pos`) so the session window @@ -182,9 +216,11 @@ fn add_window_pos(cmd: &mut Command) { } /// The shared spawn + stdout-contract reader behind [`spawn_session`]/[`spawn_browse`]. +/// `spec_path` is the child's `--resolved-spec` temp file, deleted once the child exits. fn spawn_with( mut cmd: Command, host_label: &str, + spec_path: Option, slot: SessionChild, mut on_event: impl FnMut(SpawnEvent) + Send + 'static, ) -> Result<(), String> { @@ -225,9 +261,19 @@ fn spawn_with( trust_rejected, }) => error = Some((msg, trust_rejected)), Some(ChildLine::Ended(msg)) => ended = Some(msg), + // The window size is the spawner's to persist — the renderer only + // reports it (same handling as orchestrate's own reader). + Some(ChildLine::Window { w, h }) => { + pf_client_core::orchestrate::persist_window_size(w, h); + } None => {} } } + // The spec has done its job the moment the child has read it; a leftover temp + // file in %TEMP% is litter, and one per launch adds up. + if let Some(path) = &spec_path { + let _ = std::fs::remove_file(path); + } // EOF — reap the child (killed-by-Disconnect lands here too; -1 = no code). let code = slot .0 @@ -277,6 +323,12 @@ mod tests { Some(ChildLine::Stats(s)) => assert!(s.starts_with("1280")), _ => panic!("stats line"), } + // The match-window report: the SPAWNER persists it (§5) — dropping this line was + // why Windows sessions fell back to renderer-local persistence. + match parse_line("{\"window\":{\"w\":1600,\"h\":900}}") { + Some(ChildLine::Window { w, h }) => assert_eq!((w, h), (1600, 900)), + _ => panic!("window line"), + } assert!(parse_line("").is_none()); assert!(parse_line("{\"other\":1}").is_none()); }