Repository navigation
Discord's call stage: stream tiles, a focused stream, and a screen picker - #153
Conversation
…cker A share or camera used to show as a "X is sharing · Watch" pill, then a 16:9 box above the tiles, and whoever shared saw nothing of it. Now everyone sharing their screen or camera has a stream tile beside their own, "Watch stream" until it is watched. Clicking one watches it and fills the stage with it, the people in a strip under it, over it or hidden, with a full-screen button; clicking it again, or Escape, is the grid again. My own share is a tile too, drawn from what my call sends (or my seat, when my call's list lags), with Stop and a dimmed picture of what I picked. Share opens a picker of every screen and, on Windows, every window, each with a small picture (`mediad --screens`, off the main thread). The choice rides on `ShareSend` as an optional `screen` (left off the wire when empty, so older frames still read), and mediad captures that screen (GDI, per-monitor DPI) or window (PrintWindow). macOS and X11 still share the primary screen; a phone keeps the system's own picker. A click that redraws the stage removed the tile it focused, and Escape then found no binding, so the stage puts the keys back on the window's root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
unRekable
left a comment
unRekable
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅ (0 critical, 4 robustness warnings worth a follow-up, several suggestions) — read the full diff (26 files) plus surrounding code.
⚠️ Warnings (robustness, not correctness — nothing blocking, see the follow-up note)
- share.rs:206 — no timeout on
mediad --screens; a wedged mediad leaves the picker spinning with Share disabled indefinitely (cancel and reopens stack extra blocking jobs). - share.rs:30 — the off-thread listing is fire-and-forget without a generation token; close-then-reopen can deliver a stale listing (old window handles, old thumbnails) into the new picker.
- screens.rs:110 — Windows recycles HWNDs, so
IsWindowcan pass on a different window after the listed one closed — sharing the wrong window; a malformedwindow:id silently falls through to the primary display instead of erroring. - screens.rs:41 — the listing sweep deletes every
*.jpgin the shared folder while an open (or stale) picker may still hold those paths, rendering a broken thumbnail.
💡 Suggestions
- screens.rs:218 — the
Program Managerfilter is hard-coded English; on localized Windows the shell desktop would be listed as shareable. Filtering by window class (Progman/WorkerW) is locale-proof. - mod.rs:1058 — Escape clears focus but leaves the strip (and per-list
screen_windows) as-is, so a hidden strip silently persists for the next focused stream. Confirm that's intentional Discord parity. - A test for the
read_screens -> Some(empty) -> share primaryfallback and captures/enumeration is the main coverage gap (understandable for window-bound Windows code; stage.rs, serde round-trip and BGRA are well covered). - screens.rs:47 — thumbnails are captured full-res then downscaled (up to ~30 MB churn per grab at 4K); capping capture size would soften that.
✅ Looks Good
- The stage state machine is pure and unit-tested, including the merge order call-knows-first / seat-lag fallback / seat-missing synthesis, and unfocus on end/leave.
screenis wire-compatible in both directions —skip_serializing_ifout,#[serde(default)]in — with round-trip tests for the old shape; old builds safely ignore the field.- GDI capture is disciplined: every
SelectObject/DeleteObject/DeleteDC/ReleaseDChas its match including error paths, and the DPI guard restores correctly. - Platform parity is honest: macOS/X11 and phones fall back to primary screen or the system picker rather than pretending to support picks they cannot do.
- The focus-restore fix matches the codebase's idiom and every stage entry point funnels through it.
Suggested follow-up (not in this PR): a bounded timeout plus a job-generation guard in the picker path, an error instead of primary-screen fall-through for malformed ids, and the locale-proof shell-window filter.
| /// and the share is then the primary screen. | ||
| fn read_screens() -> Vec<ScreenSource> { | ||
| let folder = protocol::support_dir().join("screens"); | ||
| let stdout = protocol::media_host::output(protocol::launch::no_console(&mut std::process::Command::new(crate::gui::settings::mediad_path())).arg("--screens").arg(&folder).stderr(std::process::Stdio::null())).unwrap_or_default(); |
There was a problem hiding this comment.
media_host::output() has no deadline, so if the spawned mediad wedges, this stays at "Looking for screens and windows…" forever with the share button disabled — cancel is the only exit, and each reopen stacks another blocking off-thread job. A timeout with a "share primary screen / retry" fallback would keep the modal usable. Non-blocking, but high value.
| self.tiny_state.screen_pick = None; | ||
| self.tiny_state.screen_windows = false; | ||
| self.tiny_open_modal(Modal::Share, window, cx); | ||
| self.off_thread(cx, read_screens, |this, screens, cx| { |
There was a problem hiding this comment.
off_thread is fire-and-forget with no generation token, so closing the picker quickly and reopening leaves two read_screens jobs racing; the earlier one can finish last and populate the new modal with stale windows/thumbnails. Guard with a job-generation counter (or only accept a result while the current modal is still Modal::Share).
| /// One screen or window, by id; the primary screen when the id is `None` or names nothing that | ||
| /// is still there as a screen. A window that closed is said in words, so the share ends. | ||
| #[cfg(windows)] | ||
| pub(super) fn capture(id: Option<&str>) -> Result<Rgba, String> { |
There was a problem hiding this comment.
IsWindow may still pass on a different window with the recycled handle — the share then captures the wrong window. And a window: id that fails parse::<isize>() silently falls through to the primary display instead of erroring, which shares the whole screen where the user picked a window. Consider erroring on a malformed id; the reuse hazard is inherently racy but worth a comment.
| if std::fs::create_dir_all(folder).is_err() { | ||
| return found; | ||
| } | ||
| for entry in std::fs::read_dir(folder).into_iter().flatten().flatten() { |
There was a problem hiding this comment.
💡 Suggestion: this sweep deletes every *.jpg in the shared folder at listing time, while an already-populated picker (or a stale off-thread result) may still hold those exact paths in ScreenSource.thumbnail and then render a broken image. Deleting only files that are not part of the "current" stamp set, or stamping per picker-id, would keep the wipe and the read coordinated.
| let got = GetWindowTextW(hwnd, title.as_mut_ptr(), title.len() as i32); | ||
| let title = String::from_utf16_lossy(&title[..got.max(0) as usize]); | ||
| // The desktop itself is a titled window too. | ||
| if !title.trim().is_empty() && title != "Program Manager" { |
There was a problem hiding this comment.
💡 Suggestion: "Program Manager" is locale-specific — on a localized Windows the shell desktop shows up as a shareable window. Filtering by window class (Progman/WorkerW) instead is locale-proof.
| self.tiny_close_settings(cx); | ||
| } else if self.tiny_state.card.is_some() { | ||
| self.tiny_state.card = None; | ||
| } else if self.tiny_state.focus.is_some() && self.tiny_state.channel.is_some() && matches!(self.sidebar_state.mode, Mode::Network(Network::Tiny)) { |
There was a problem hiding this comment.
💡 Suggestion: Escape clears focus but leaves strip (and the per-list screen_windows) untouched, so a hidden strip silently persists into the next focused stream. If that is deliberate Discord parity, fine — a one-line comment would make it explicit.
The call stage now works like Discord's for screen shares and cameras.
Stage
ShareWatchand fills the stage with it.gui/tiny/stage.rs, with unit tests.Screen picker
mediad --screensoff the main thread. Double-click shares straight away.MediaRequest::ShareSendgets an optionalscreenfield (#[serde(default)], left off the wire when it's empty, with a round-trip test). chatd passes it through as before. mediad captures the chosen screen with GDI at per-monitor DPI, or the chosen window withPrintWindow.Fix along the way: clicking a stage tile focused that tile, then the redraw removed it. After that, Escape had no binding to match. The stage now moves keyboard focus back to the window root.
Checks
cargo build -p tinyclient --features gui -p chatd -p mediadis clean.cargo test -p tinyclient --features gui: 538 passed. 1 failed:invite_ipc::second_launch_delivers_to_the_primary, because the installed tinyclient on the test machine already holds that named pipe. Unrelated to this change.🤖 Generated with Claude Code