From e6686d0d49e85bb882f78ac5814d8580ede74bd8 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:09:44 +0800 Subject: [PATCH 1/7] fix(switcher): name a machine whose profile is gone (#786) Refs #485. Also stops PaneWorkspace::route_header spelling a profile UUID on the restore path. --- src/terminal/remote.rs | 79 +++++++++++++++++++++++++++++++++++++--- src/ui/remote_connect.rs | 47 +++++++++++++++++++++--- src/ui/switcher.rs | 60 +++++++++++++++++++++++++++++- 3 files changed, 174 insertions(+), 12 deletions(-) diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 86d517dc..d7cf82ce 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -136,11 +136,24 @@ impl PaneWorkspace { RouteHeader::local_stdio(program.clone(), &argv) } (_, Some(spec)) => RouteHeader::ssh((**spec).clone()), - (target, None) => { - return Err(anyhow::anyhow!( - "this workspace has no SSH connection details ({target:?}), so its panes \ - cannot be routed" - )); + (_, None) => { + // Deliberately not the target: a `Profile` spells itself as + // its config UUID in `Display` and in `Debug` alike, and a + // deleted profile is exactly what empties `spec` here. This + // sentence is not only logged — `land_pane` hands it to the + // pending pane, which prints the reason verbatim under + // "could not reach {machine}", so the UUID reached the screen + // (#485). The workspace's own name is what every other + // surface calls this thing. + return Err(match self.label.as_deref() { + Some(label) => anyhow::anyhow!( + "{label} has no SSH connection details, so its panes cannot be routed" + ), + None => anyhow::anyhow!( + "this workspace has no SSH connection details, so its panes \ + cannot be routed" + ), + }); } }; Ok(header.for_pane()) @@ -3073,6 +3086,62 @@ mod windows_tests { } } +/// Ungated on purpose: what a workspace can build a route out of is the same +/// on every platform, and so is the name the refusal carries. +#[cfg(test)] +mod route_header_tests { + use super::*; + use crate::core::session::{RemoteTarget, WorkspaceId}; + + fn unroutable(target: RemoteTarget, label: Option<&str>) -> PaneWorkspace { + PaneWorkspace { + workspace: WorkspaceId::new(), + target, + spec: None, + label: label.map(str::to_string), + resize_echo: false, + } + } + + /// A deleted profile is what empties `spec`, and the refusal built here is + /// what the pending pane prints verbatim under "could not reach + /// {machine}" — so this is one of the screens #485 is about. It used to + /// carry `{target:?}`, which for a `Profile` is its config UUID and + /// nothing else. + #[test] + fn an_unroutable_workspace_is_not_named_by_its_profile_uuid() { + let id = uuid::Uuid::new_v4(); + let gone = RemoteTarget::Profile { id }; + + let named = unroutable(gone.clone(), Some("lager")); + let e = named + .route_header() + .expect_err("no spec, no route") + .to_string(); + assert!( + !e.contains(&id.to_string()), + "a bare profile UUID reached the UI: {e}" + ); + assert!( + e.contains("lager"), + "the entry's own name is what it is called: {e}" + ); + assert!(e.contains("cannot be routed"), "{e}"); + + // Nothing to call it by is still no reason to print the UUID. + let bare = unroutable(gone, None); + let e = bare + .route_header() + .expect_err("no spec, no route") + .to_string(); + assert!( + !e.contains(&id.to_string()), + "a bare profile UUID reached the UI: {e}" + ); + assert!(e.contains("cannot be routed"), "{e}"); + } +} + #[cfg(all(test, unix))] mod tests { use super::*; diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index 42d101b4..01e3d189 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -69,11 +69,18 @@ pub fn available_hosts(cx: &App) -> Vec { /// exists, the target's own spelling when that is human-readable, and the /// deleted-profile placeholder for the bare-UUID case (#485). pub fn target_label(cx: &App, target: &RemoteTarget) -> String { - if let Some(choice) = available_hosts(cx) - .into_iter() - .find(|h| h.target == *target) - { - return choice.label; + label_from_hosts(&available_hosts(cx), target) +} + +/// `target_label`'s rule applied to a listing the caller already has. The +/// switcher builds `available_hosts` once per frame and names several targets +/// from it; re-listing per target would re-read `~/.ssh/config` off the disk +/// on the render path, which is the cost `route_label` goes out of its way to +/// avoid. One rule, two entry points — so a name shown next to a pane and the +/// same name shown in the switcher cannot drift apart (#485). +pub fn label_from_hosts(hosts: &[HostChoice], target: &RemoteTarget) -> String { + if let Some(choice) = hosts.iter().find(|h| h.target == *target) { + return choice.label.clone(); } match target { RemoteTarget::Profile { .. } => t(L10nKey::RemoteProfileGone).to_string(), @@ -835,6 +842,36 @@ pub fn restart_server_blocking(header: RouteHeader, label: &str) -> Result<(), S mod tests { use super::*; + /// The one rule for "what do we call a target we cannot resolve" (#485), + /// pinned where both `target_label` and the switcher's group listing read + /// it from. + #[test] + fn an_unresolvable_profile_is_named_never_spelled_as_its_uuid() { + let id = uuid::Uuid::new_v4(); + let gone = RemoteTarget::Profile { id }; + + let label = label_from_hosts(&[], &gone); + assert!( + !label.contains(&id.to_string()), + "a bare profile UUID reached the UI: {label}" + ); + assert_eq!(label, t(L10nKey::RemoteProfileGone)); + + // While the profile is configured, its own name wins. + let listed = vec![HostChoice { + target: gone.clone(), + label: "lager".into(), + detail: "qhw@222.29.101.16".into(), + }]; + assert_eq!(label_from_hosts(&listed, &gone), "lager"); + + // Targets that spell themselves readably never need the placeholder. + let alias = RemoteTarget::Alias { + alias: "build-box".into(), + }; + assert_eq!(label_from_hosts(&[], &alias), "build-box"); + } + fn request() -> InstallRequest { InstallRequest { host: "me@build-box:22".into(), diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index aca45583..a379e595 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -716,6 +716,11 @@ impl Tty7App { } } + // Listed once for the whole frame: the pending groups below name + // themselves from it, and so does the pass that settles every group's + // link state further down. + let configured = remote_connect::available_hosts(cx); + for target in self.pending_machines() { let key = target.to_string(); if index.contains_key(&key) { @@ -723,7 +728,12 @@ impl Tty7App { } index.insert(key.clone(), groups.len()); groups.push(Group { - label: key.clone(), + // Not `key`: a `Profile` target spells itself as its config + // UUID, so a machine whose profile has been deleted would + // announce itself to the banners below by a raw UUID (#485). + // The pass below overwrites this while the profile is still + // configured; this is what is left when it is not. + label: remote_connect::label_from_hosts(&configured, &target), key, endpoint: String::new(), target: Some(target), @@ -838,7 +848,6 @@ impl Tty7App { // trouble banners under the list come out in a stable order. groups.sort_by(|a, b| a.key.is_empty().cmp(&b.key.is_empty()).reverse()); - let configured = remote_connect::available_hosts(cx); for group in &mut groups { let Some(target) = group.target.clone() else { group.link = Link::Local; @@ -3290,6 +3299,53 @@ fn glyph_col(w: f32, child: impl IntoElement) -> impl IntoElement { mod tests { use super::*; + /// #485 on the path #645 did not cover. A machine the switcher knows only + /// from a listing snapshot has no store entry to name it, so its group + /// used to be labelled by the target's own spelling — and a `Profile` + /// target spells itself as its config UUID. Delete the profile and every + /// banner under the list announced a raw UUID. + #[gpui::test] + fn a_pending_machine_whose_profile_is_gone_is_not_named_by_its_uuid( + cx: &mut gpui::TestAppContext, + ) { + use crate::core::session::RemoteTarget; + + let (app, _vcx) = crate::ui::app::test_window::harness(cx); + + // A profile id that is in no config: the state left behind when the + // profile a machine was reached through is deleted. + let id = uuid::Uuid::new_v4(); + let target = RemoteTarget::Profile { id }; + + app.update(cx, |app, _| { + app.host_snapshots.insert( + target.host_id(), + super::HostSnapshot { + target: target.clone(), + rows: Vec::new(), + }, + ); + }); + + app.update(cx, |app, cx| { + let groups = app.switcher_groups(cx); + let group = groups + .iter() + .find(|g| g.target.as_ref() == Some(&target)) + .expect("the snapshot puts its machine in the list"); + assert!( + !group.label.contains(&id.to_string()), + "the switcher named a machine by its raw profile UUID: {}", + group.label + ); + assert_eq!( + group.label, + t(L10nKey::RemoteProfileGone), + "a gone profile is named here the way a pane's route names it" + ); + }); + } + /// A wrong hostname or a stale password used to be fixable only by /// finding the same machine again in Settings (#438). The machine is on /// screen here, so its host row is too — worded for what the row can From 2de94472ec41d12296503f61aaa4bf469664c528 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:28:22 +0800 Subject: [PATCH 2/7] feat(tabs): name a titleless tab after its working directory (#792) Refs #740. --- src/terminal/remote.rs | 2 +- src/terminal/view.rs | 53 ++++++ src/ui/app.rs | 61 +++++++ src/ui/switcher.rs | 43 +---- src/ui/tab_sidebar.rs | 23 ++- src/ui/tab_strip.rs | 387 ++++++++++++++++++++++++++++++++++++++--- 6 files changed, 502 insertions(+), 67 deletions(-) diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index d7cf82ce..3cc42e90 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -718,7 +718,7 @@ impl RemoteTerminal { /// here wins: it describes where the shell actually landed, which is not /// always where we asked (a missing directory sends the daemon home, an /// rc file may `cd` on its own). - fn seed_cwd(&self, cwd: Option) { + pub(crate) fn seed_cwd(&self, cwd: Option) { let Some(cwd) = cwd else { return }; if let Ok(mut guard) = self.cwd.lock() { guard.get_or_insert(cwd); diff --git a/src/terminal/view.rs b/src/terminal/view.rs index f1755005..800f1713 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -163,6 +163,24 @@ pub struct NativeSshParts { /// What a pane is called when nothing running in it has said otherwise. pub(crate) const DEFAULT_TITLE: &str = "tty7"; +/// What a pane is *saying* about itself, if anything — the reading behind +/// [`TerminalView::stated_title`], split out so it can be pinned without a +/// live pane. +/// +/// Anything but the placeholder counts. That is wider than "arrived over OSC +/// 0/2" on purpose: an SSH pane answers to the host it dialled and a workspace +/// pane to its workspace's name, and those are names tty7 gave the pane +/// deliberately (#438) rather than the absence of one. The literal string +/// `tty7` is the only title that says nothing, because it is the app's own +/// name standing in for a pane that has never introduced itself. +pub(crate) fn stated_title(title: &str) -> Option<&str> { + match title.trim() { + "" => None, + t if t == DEFAULT_TITLE => None, + t => Some(t), + } +} + pub struct ShellParts { terminal: RemoteTerminal, pub(crate) pane_id: u64, @@ -1563,6 +1581,16 @@ impl TerminalView { self.terminal.foreground_cwd() } + /// The title this pane is showing, or `None` while it is still answering + /// to the app's own name — see [`stated_title`]. The label ladder reads + /// this where the machine tree reads + /// [`PaneRecord::osc_title`](tty7_core::core::machine::PaneRecord::osc_title), + /// which is what lets the tab strip and the switcher name a tab the same + /// way. + pub(crate) fn stated_title(&self) -> Option<&str> { + stated_title(&self.title) + } + /// Sets how opaque the pane wants this terminal painted; the pane leaf /// calls this every frame while rendering, and the terminal element /// blends its colours toward the window background during paint (see @@ -7343,6 +7371,31 @@ fn drag_scroll_step(overshoot: f32) -> i32 { #[cfg(test)] mod tests { + /// What the label ladder asks a pane: are you showing a name of your own, + /// or still standing under the app's? (#740) + #[test] + fn a_pane_states_a_title_whenever_it_is_not_the_placeholder() { + use super::stated_title; + + // Nothing has spoken — this is the pane a directory stands in for. + assert_eq!(stated_title("tty7"), None); + assert_eq!(stated_title(" tty7 "), None); + assert_eq!(stated_title(" "), None); + + // A title from the program running in it. + assert_eq!(stated_title("vim — main.rs"), Some("vim — main.rs")); + assert_eq!(stated_title(" user@host:~/repo "), Some("user@host:~/repo")); + // A default tty7 chose for the pane itself is a name, not the absence + // of one: an SSH pane answers to its host (#438) and a workspace pane + // to its workspace, and neither gives way to a directory. + assert_eq!(stated_title("prod-web"), Some("prod-web")); + // So does the state a finished pane is left showing. + assert_eq!( + stated_title("tty7 — process exited"), + Some("tty7 — process exited") + ); + } + #[test] fn an_unfocused_input_caret_is_always_a_steady_outline() { use super::{InputCaretPaint, input_caret_paint}; diff --git a/src/ui/app.rs b/src/ui/app.rs index e3b74ca3..2a1c7ca8 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -536,6 +536,67 @@ impl Tab { (leaf.title.clone(), leaf.display_home(cx)) } + /// This tab as the shared label ladder reads it, together with what a `~` + /// in whatever it ends up named would mean. + /// + /// [`TabView`](tty7_core::core::tab_view::TabView) is how a tab looks to + /// someone who is *not* the window showing it — the switcher listing + /// another window's workspace, `tty7 tab ls` on the far side of a socket. + /// Building one here from the live pane is what stops this window having a + /// second opinion: both sides then rank a given name, a title, an agent and + /// a directory through + /// [`TabView::label`](tty7_core::core::tab_view::TabView::label), so the + /// strip's answer to "which repo is this?" is the switcher's answer too. + /// + /// Everything comes off the one leaf the tab names itself after, so the + /// title and the directory standing in for it can never describe different + /// panes (#580). + pub(crate) fn label_view( + &self, + window: Option<&Window>, + cx: &App, + ) -> ( + tty7_core::core::tab_view::TabView, + Option, + ) { + let name = self.name.clone(); + let Some(leaf) = self.title_leaf(window, cx) else { + return ( + tty7_core::core::tab_view::TabView { + id: self.tree_id.get(), + name, + title: String::new(), + osc_title: None, + cwd: None, + agent: None, + status: None, + live: false, + panes: 0, + }, + None, + ); + }; + let leaf = leaf.read(cx); + let view = tty7_core::core::tab_view::TabView { + id: self.tree_id.get(), + name, + // The tree's `title` is the foreground process name — what it falls + // back on once a pane has said nothing about itself. A live pane's + // equivalent is the placeholder it answers to unprompted: any + // *other* default it was given (an SSH host, a workspace name) is a + // name tty7 chose for it deliberately, and `stated_title` hands + // those up as the title the pane is showing. + title: crate::terminal::view::DEFAULT_TITLE.to_string(), + osc_title: leaf.stated_title().map(str::to_string), + cwd: leaf.cwd().map(|p| p.display().to_string()), + agent: leaf.agent(), + status: leaf.agent_session().map(|s| s.status), + live: !leaf.terminal.exited, + panes: self.pane.terminals().len(), + }; + (view, leaf.display_home(cx)) + } + pub(crate) fn git_status( &self, window: Option<&Window>, diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index a379e595..e94f451d 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -2965,47 +2965,20 @@ impl TabRow { } } -/// Names a tab of a workspace this window does not own, matching what -/// `Tty7App::tab_label` shows for local ones. +/// Names a tab of a workspace this window does not own. /// -/// The two read different sources and have to be talked into agreeing. A local -/// tab is named by its live terminal's OSC title, which shells set to the -/// working directory and agents overwrite with what they are doing. The tree -/// carries a copy of that title (`PaneRecord::osc_title`), which is what makes -/// the two columns agree; `PaneRecord::title` is the *foreground process name* -/// ("zsh") and only stands in when there is no title at all. +/// The two surfaces used to read different sources and had to be talked into +/// agreeing: a local tab was named by its live terminal's title, this one by +/// the tree's copy of it (`PaneRecord::osc_title`). They now go through the one +/// renderer, [`crate::ui::tab_strip::label_of`] — a local tab is turned into +/// the same [`TabView`](crate::ui::machine_mirror::TabView) this one already +/// is, so neither column can rank the evidence its own way. fn tab_view_label( view: &crate::ui::machine_mirror::TabView, index: usize, home: Option<&std::path::Path>, ) -> String { - let unnamed = || { - t_fmt( - L10nKey::TabUnnamedShell, - &[("n", &((index + 1).to_string()))], - ) - }; - // A path can shorten away to nothing (a bare "user@host:"), and the process - // name is still worth more than a number. - let shortened = |raw: &str| match crate::ui::tab_strip::short_title(raw, home) { - shortened if !shortened.trim().is_empty() => shortened, - _ => match view.title.trim() { - "" => unnamed(), - title => title.to_string(), - }, - }; - match view.label() { - crate::ui::machine_mirror::TabLabel::Named(name) => name.to_string(), - // Through `short_title` because the local strip puts its own titles - // through it too: the shell integration writes `user@host:~/dir`, and a - // tab that spelled that out in full where the strip says "…/dir" would - // be the same disagreement in a new place. - crate::ui::machine_mirror::TabLabel::Osc(title) => shortened(title), - crate::ui::machine_mirror::TabLabel::Agent(agent) => agent.display_name().to_string(), - crate::ui::machine_mirror::TabLabel::Cwd(cwd) => shortened(cwd), - crate::ui::machine_mirror::TabLabel::Process(title) => title.to_string(), - crate::ui::machine_mirror::TabLabel::Unknown => unnamed(), - } + crate::ui::tab_strip::label_of(view, index, home) } impl Group { diff --git a/src/ui/tab_sidebar.rs b/src/ui/tab_sidebar.rs index 6324133e..cb093257 100644 --- a/src/ui/tab_sidebar.rs +++ b/src/ui/tab_sidebar.rs @@ -326,9 +326,24 @@ impl Tty7App { ); (shown, Some(full)) } else { - let (raw_title, home) = tab.leaf_title_and_home(Some(window), cx); - let title = strip_host_prefix(raw_title.trim()); - let raw = abbreviate_home(title, home.as_deref()); + // The ladder the strip and the switcher climb, read + // here for the name and not for the shortening: this + // column measures in pixels and lets a card expand the + // row back to the whole string, so it wants what + // `label_of` would have cut down rather than the cut. + use crate::ui::machine_mirror::TabLabel; + let (view, home) = tab.label_view(Some(window), cx); + let raw = match view.label() { + TabLabel::Osc(title) | TabLabel::Cwd(title) => { + abbreviate_home(strip_host_prefix(title.trim()), home.as_deref()) + .into_owned() + } + TabLabel::Agent(agent) => agent.display_name().to_string(), + // A tab holding a name got one above. + TabLabel::Named(name) => name.to_string(), + TabLabel::Process(title) => title.to_string(), + TabLabel::Unknown => String::new(), + }; if raw.trim().is_empty() { // Nothing to expand: the row is naming an unnamed // shell, not hiding a title behind an ellipsis. @@ -338,7 +353,7 @@ impl Tty7App { )); (placeholder, None) } else { - let full = SharedString::from(raw.as_ref()); + let full = SharedString::from(raw); let shown = elide_label( &window.text_system(), title_font, diff --git a/src/ui/tab_strip.rs b/src/ui/tab_strip.rs index 33982dd7..3ba13fcb 100644 --- a/src/ui/tab_strip.rs +++ b/src/ui/tab_strip.rs @@ -140,6 +140,94 @@ pub(crate) fn short_title(raw: &str, home: Option<&std::path::Path>) -> String { label } +/// The one place a tab gets its displayed name, whichever surface is asking. +/// +/// `label()` ranks the evidence — a given name, then the title the pane is +/// showing, then an agent, then the working directory, then the process it is +/// running — and this renders whatever came back. Both callers arrive with a +/// [`TabView`](crate::ui::machine_mirror::TabView): the switcher reads one out +/// of the machine tree for a window it does not own, and the strip builds one +/// from its own live panes in +/// [`Tab::label_view`](crate::ui::app::Tab::label_view). +/// +/// They used to rank their own evidence, and disagreed where it mattered most: +/// a pane with a working directory and no title — every non-PowerShell shell +/// tty7 ships integration for reports OSC 7 and no OSC 0 — was listed by the +/// switcher as `~/repo/tty7` and by the strip that owned it as "tty7", the +/// app's own name (#740). +pub(crate) fn label_of( + view: &crate::ui::machine_mirror::TabView, + index: usize, + home: Option<&std::path::Path>, +) -> String { + use crate::ui::machine_mirror::TabLabel; + + let unnamed = || { + t_fmt( + L10nKey::TabUnnamedShell, + &[("n", &((index + 1).to_string()))], + ) + }; + // A path can shorten away to nothing (a bare "user@host:"), and the process + // name the tree carries ("zsh") is still worth more than a number. + // + // Through `stated_title` because a tab of *this* window has no process name + // to offer: `Tab::label_view` fills that slot with the placeholder a pane + // answers to before anything has spoken, and printing the app's own name + // here is the one thing #740 exists to stop. Nothing to say falls to the + // number, which is what the strip showed before it shared this renderer. + let shortened = |raw: &str| match short_title(raw, home) { + shortened if !shortened.trim().is_empty() => shortened, + _ => match crate::terminal::view::stated_title(&view.title) { + Some(title) => title.to_string(), + None => unnamed(), + }, + }; + match view.label() { + TabLabel::Named(name) => name.to_string(), + // Through `short_title` because a title is so often a path: the shell + // integration writes `user@host:~/dir`, and a tab spelling that out in + // full where the one beside it says "…/dir" would be the same + // disagreement in a new place. + TabLabel::Osc(title) => shortened(title), + TabLabel::Agent(agent) => agent.display_name().to_string(), + TabLabel::Cwd(cwd) => shortened(cwd), + TabLabel::Process(title) => title.to_string(), + TabLabel::Unknown => unnamed(), + } +} + +/// What a row can add on hover: the name behind the one [`label_of`] cut down, +/// or `None` when it cut nothing and the tooltip would only repeat the row. +/// +/// The comparison has to happen on the *same* spelling, which is the whole +/// trick here. `label_of` abbreviates a path under the home before it elides +/// it, and this returns the abbreviated form too, so a raw `/Users/x/repo` +/// measured against a label of `~/repo` looks like a difference that isn't +/// one — and every tab named after a directory inside the home would hang a +/// tooltip saying exactly what it already says. Abbreviate first, compare +/// after. +fn tooltip_of( + view: &crate::ui::machine_mirror::TabView, + index: usize, + home: Option<&std::path::Path>, +) -> Option { + use crate::ui::machine_mirror::TabLabel; + + // The other rungs are never shortened: a given name and a process name are + // printed whole, and an agent's is a word. + let raw = match view.label() { + TabLabel::Osc(title) => title, + TabLabel::Cwd(cwd) => cwd, + _ => return None, + }; + let full = abbreviate_home(raw.trim(), home); + if full.trim().is_empty() || full.as_ref() == label_of(view, index, home).as_str() { + return None; + } + Some(SharedString::from(full.into_owned())) +} + /// Width of `text` shaped in `font` at `size`, in pixels. /// /// The window's text system caches shaped runs, so measuring the same labels @@ -1229,6 +1317,11 @@ impl Tty7App { /// read `…/a/b/c` with no way to find out which `a` that was. `None` when /// nothing was dropped, so tabs that already show their whole name stay /// quiet under the pointer. + /// + /// It has to unshorten whatever the label was *made of*, which is why it + /// reads the same [`TabView`](crate::ui::machine_mirror::TabView) the label + /// did: a tab named after its directory wants that directory spelled out, + /// not the title it never had. See [`tooltip_of`]. pub(crate) fn tab_title_tooltip( &self, tab: &Tab, @@ -1236,19 +1329,13 @@ impl Tty7App { window: Option<&Window>, cx: &App, ) -> Option { - if tab.name.as_ref().is_some_and(|n| !n.trim().is_empty()) { - return None; - } - let (raw, home) = tab.leaf_title_and_home(window, cx); - let raw = raw.trim(); - if raw.is_empty() || raw == self.tab_label(tab, index, window, cx) { - return None; - } - Some(SharedString::from( - abbreviate_home(raw, home.as_deref()).into_owned(), - )) + let (view, home) = tab.label_view(window, cx); + tooltip_of(&view, index, home.as_deref()) } + /// What this window puts on a tab of its own — the same ladder, through the + /// same renderer, as the switcher uses for a tab of somebody else's window. + /// See [`label_of`]. pub(crate) fn tab_label( &self, tab: &Tab, @@ -1256,22 +1343,8 @@ impl Tty7App { window: Option<&Window>, cx: &App, ) -> String { - if let Some(name) = tab.name.as_ref() { - let trimmed = name.trim(); - if !trimmed.is_empty() { - return trimmed.to_string(); - } - } - let (raw, home) = tab.leaf_title_and_home(window, cx); - let label = short_title(&raw, home.as_deref()); - if label.trim().is_empty() { - t_fmt( - L10nKey::TabUnnamedShell, - &[("n", &((index + 1).to_string()))], - ) - } else { - label - } + let (view, home) = tab.label_view(window, cx); + label_of(&view, index, home.as_deref()) } /// The New Tab control: one `+` that drops the list of everything it could @@ -2688,4 +2761,264 @@ mod tests { assert_eq!(spec.args, ["--login"]); assert!(!spec.args_are_tty7_defaults); } + + /// A tab of this window as the strip reads it: `tab_label` is nothing but + /// [`label_of`] over the [`TabView`](crate::ui::machine_mirror::TabView) + /// that [`Tab::label_view`](crate::ui::app::Tab::label_view) builds from + /// the live leaf, so naming one here climbs the same ladder a real tab + /// climbs. `title` is the placeholder `label_view` fills that slot with — + /// the machine tree puts a process name there, a live pane has only the + /// name it answers to before anything has spoken. + fn strip_tab() -> crate::ui::machine_mirror::TabView { + crate::ui::machine_mirror::TabView { + id: tty7_core::core::machine::TabId::new(), + name: None, + title: crate::terminal::view::DEFAULT_TITLE.to_string(), + osc_title: None, + cwd: None, + agent: None, + status: None, + live: true, + panes: 1, + } + } + + /// The home the paths below are measured against — named rather than read + /// off this machine, so the assertions do not depend on who is running + /// them (#580). + fn home() -> &'static Path { + Path::new("/Users/x") + } + + #[test] + fn a_renamed_tab_keeps_its_name_over_every_other_answer() { + let mut tab = strip_tab(); + tab.name = Some(" build ".into()); + tab.osc_title = Some("vim — main.rs".into()); + tab.cwd = Some("/Users/x/repo/tty7".into()); + + assert_eq!(label_of(&tab, 0, Some(home())), "build"); + } + + #[test] + fn a_pane_showing_a_title_is_named_by_it_and_not_by_its_directory() { + let mut tab = strip_tab(); + tab.osc_title = Some("vim — main.rs".into()); + tab.cwd = Some("/Users/x/repo/tty7".into()); + + assert_eq!(label_of(&tab, 0, Some(home())), "vim — main.rs"); + + // Including the title an SSH pane answers to before the far shell has + // said anything (#438): `label_view` hands that up here, so a window + // full of them still reads as hosts rather than as directories. + tab.osc_title = Some("prod-web".into()); + assert_eq!(label_of(&tab, 0, Some(home())), "prod-web"); + } + + /// #740: every shell tty7 ships integration for except PowerShell reports + /// its directory over OSC 7 and never sets a title, which left the tab + /// reading "tty7" — the app's own name — while the switcher listing the + /// very same tab showed the directory. + #[test] + fn a_pane_that_has_only_said_where_it_is_is_named_after_that() { + let mut tab = strip_tab(); + tab.cwd = Some("/Users/x/repo/tty7".into()); + + assert_eq!(label_of(&tab, 0, Some(home())), "~/repo/tty7"); + // Through the same shortener as a title, so a deep directory is cut + // where a deep path in a title would be. + tab.cwd = Some("/Users/x/repo/tty7/crates/tty7-core/src".into()); + assert_eq!( + label_of(&tab, 0, Some(home())), + super::short_title("/Users/x/repo/tty7/crates/tty7-core/src", Some(home())), + ); + } + + /// A tooltip exists to say what the row had to leave out. One that repeats + /// the row is worse than none, and the label and the raw string it came + /// from are not comparable until both have been abbreviated: `~/repo` and + /// `/Users/x/repo` are the same name spelled two ways, and reading them as + /// a difference hung a tooltip on every tab named after a directory under + /// the home — which, after this change, is most of them. + #[test] + fn a_tab_named_after_a_directory_says_nothing_more_on_hover_unless_it_was_cut() { + let mut tab = strip_tab(); + tab.cwd = Some("/Users/x/repo".into()); + + assert_eq!(label_of(&tab, 0, Some(home())), "~/repo"); + assert_eq!( + tooltip_of(&tab, 0, Some(home())), + None, + "the row is already showing the whole directory" + ); + + // Cut down to its last three segments, so the head is worth having. + tab.cwd = Some("/Users/x/repo/crates/tty7-core/src".into()); + assert_eq!(label_of(&tab, 0, Some(home())), "…/crates/tty7-core/src"); + assert_eq!( + tooltip_of(&tab, 0, Some(home())).as_deref(), + Some("~/repo/crates/tty7-core/src") + ); + + // The same holds for a title that happens to be a path — the rung this + // guard was already getting wrong before a directory could reach it. + let mut titled = strip_tab(); + titled.osc_title = Some("/Users/x/repo".into()); + assert_eq!(tooltip_of(&titled, 0, Some(home())), None); + + // A shell integration's `user@host:` head is not in the label, so it + // is still worth spelling out. + titled.osc_title = Some("me@box:/Users/x/repo".into()); + assert_eq!( + tooltip_of(&titled, 0, Some(home())).as_deref(), + Some("me@box:/Users/x/repo") + ); + } + + /// The one test that fails if any of the wiring is put back: a real tab, + /// built the way the window builds one, named through `tab_label` — and + /// checked against what the switcher renders from the machine tree's view + /// of that very same pane. Before this change the strip said "tty7" and + /// the switcher said the directory (#740). + #[gpui::test] + fn the_strip_names_a_titleless_pane_exactly_as_the_switcher_does(cx: &mut TestAppContext) { + use crate::ui::pane::{Pane, PaneSlot}; + + let (app, mut vcx) = crate::ui::app::test_window::harness(cx); + let _stream = app.update_in(&mut vcx, |app, window, cx| { + let (view, stream) = crate::terminal::view::quiet_test_pane(1, window, cx); + // A pane that has reported where it is over OSC 7 and has never + // titled itself — every shell tty7 ships integration for except + // PowerShell. + view.read(cx) + .terminal + .seed_cwd(Some(std::path::PathBuf::from("/work/repo"))); + app.tabs + .push(crate::ui::app::Tab::new(Pane::leaf(PaneSlot::Ready(view)))); + app.active = app.tabs.len() - 1; + stream + }); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| { + let index = app.active; + let tab = &app.tabs[index]; + let (view, home) = tab.label_view(Some(window), cx); + assert_eq!(view.osc_title, None, "the pane never titled itself"); + assert_eq!(view.cwd.as_deref(), Some("/work/repo")); + + let strip = app.tab_label(tab, index, Some(window), cx); + assert_eq!(strip, "/work/repo"); + assert_ne!( + strip, + crate::terminal::view::DEFAULT_TITLE, + "and is not named after the app any more" + ); + + // The machine tree's reading of the same pane, which is all the + // switcher ever has: no title was seen, the cwd is the one above, + // and `title` is the foreground process name. + let from_tree = crate::ui::machine_mirror::TabView { + id: tab.tree_id.get(), + name: None, + title: "zsh".into(), + osc_title: None, + cwd: Some("/work/repo".into()), + agent: None, + status: None, + live: true, + panes: 1, + }; + assert_eq!( + strip, + label_of(&from_tree, index, home.as_deref()), + "the two columns name the same tab the same way" + ); + + assert_eq!( + app.tab_title_tooltip(tab, index, Some(window), cx), + None, + "and the row is showing the whole path, so it stays quiet" + ); + }); + } + + #[test] + fn a_pane_with_nothing_to_say_falls_back_the_way_it_always_did() { + // No title and no directory: the placeholder, exactly as before. + let tab = strip_tab(); + assert_eq!(label_of(&tab, 0, Some(home())), "tty7"); + + // And a tab holding no live pane at all is still numbered. + let mut empty = strip_tab(); + empty.title = String::new(); + assert!(label_of(&empty, 2, Some(home())).contains('3')); + } + + /// The rung under the shortener, which the two surfaces reach holding + /// different things. A shell that has said who and where it is but not + /// *where* — `user@host:` with nothing after the colon — leaves nothing to + /// show, and whatever stands in has to be something the tab does not + /// already say: the switcher has the foreground process name, and a tab of + /// this window has only the placeholder, which is the answer #740 removed. + #[test] + fn a_title_that_shortens_away_never_puts_the_app_name_back_on_the_tab() { + let mut strip = strip_tab(); + strip.osc_title = Some("user@host:".into()); + assert_ne!( + label_of(&strip, 0, Some(home())), + crate::terminal::view::DEFAULT_TITLE + ); + assert!( + label_of(&strip, 0, Some(home())).contains('1'), + "the numbered placeholder, which is what the strip showed here \ + before it shared this renderer" + ); + + // The switcher arrives with a real process name in that slot, and it + // is still worth more than a number. + let from_tree = crate::ui::machine_mirror::TabView { + title: "zsh".into(), + osc_title: Some("user@host:".into()), + ..strip_tab() + }; + assert_eq!(label_of(&from_tree, 0, Some(home())), "zsh"); + } + + /// A path is spelled the way the machine it is on spells it, and which + /// machine that is has nothing to do with which one tty7 is running on: a + /// remote pane reports POSIX to a Windows client, and a Windows pane + /// reports backslashes to a client that has never seen one (#580). + #[test] + fn a_cwd_is_cut_in_its_own_spelling_whichever_client_is_reading_it() { + let windows_home = Path::new(r"C:\Users\x"); + + // A Windows pane: shortened under its own home, and a path too deep to + // fit is rejoined with its own separator rather than with `/`. + let mut win = strip_tab(); + win.cwd = Some(r"C:\Users\x\repo".into()); + assert_eq!(label_of(&win, 0, Some(windows_home)), "~/repo"); + win.cwd = Some(r"D:\work\a\b\proj".into()); + assert_eq!(label_of(&win, 0, Some(windows_home)), r"…\a\b\proj"); + + // A remote pane's cwd is POSIX even when the client reading it is the + // Windows one: no drive to hang it off, no `~` borrowed from this + // machine's home, and no backslash anywhere in the answer. + let mut remote = strip_tab(); + remote.cwd = Some("/srv/app".into()); + assert_eq!(label_of(&remote, 0, Some(windows_home)), "/srv/app"); + remote.cwd = Some("/home/deploy/app".into()); + assert_eq!( + label_of(&remote, 0, Some(Path::new("/home/deploy"))), + "~/app", + "measured against the home of the host it is on, not of this one" + ); + + // The root of a filesystem is a directory like any other: a tab + // sitting in it says so, and says nothing more on hover. + let mut root = strip_tab(); + root.cwd = Some("/".into()); + assert_eq!(label_of(&root, 0, Some(home())), "/"); + assert_eq!(tooltip_of(&root, 0, Some(home())), None); + } } From 79527ca871c3ac20ae41de76ab6d3143b82e1a8a Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:33:02 +0800 Subject: [PATCH 3/7] feat(diff): let the pointer take a range of diff lines and copy it (#794) Refs #721. Reconciled onto the virtualised row list from #799: the selection is keyed on RowAt { path, RowId } rather than the flat list index, since collapsing a file above the selection re-points flat indices. --- src/ui/diff_list.rs | 113 ++++++- src/ui/diff_overlay.rs | 691 ++++++++++++++++++++++++++++++++++++++++- src/ui/diff_rows.rs | 345 +++++++++++++++++++- src/ui/i18n/en.rs | 1 + src/ui/i18n/ja.rs | 1 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + 7 files changed, 1115 insertions(+), 38 deletions(-) diff --git a/src/ui/diff_list.rs b/src/ui/diff_list.rs index 08a77818..ef474f29 100644 --- a/src/ui/diff_list.rs +++ b/src/ui/diff_list.rs @@ -14,11 +14,13 @@ use std::collections::HashMap; +use gpui::SharedString; + use crate::core::config::DiffViewMode; use crate::terminal::git_diff::{ AUTO_COLLAPSE_LINES, DiffSnapshot, FileDiff, FileStatus, MAX_RENDERED_FILES, Truncation, }; -use crate::ui::diff_rows::{SplitRow, UnifiedRow, split_hunk, unified_rows}; +use crate::ui::diff_rows::{RowId, SplitRow, UnifiedRow, split_hunk, unified_rows}; /// Everything the file header row draws, lifted out of its [`FileDiff`]. /// @@ -39,6 +41,21 @@ pub(crate) struct FileHead { pub(crate) expanded: bool, } +/// Where a drawn line sits in the patch: which file's rows it belongs to, and +/// its place among them. +/// +/// Carried on the row rather than read off its position in the list. The list +/// is spliced — collapsing a file above this one moves every index below it — +/// so a range keyed on list positions would go on naming the same slots while +/// the code under them changed. +#[derive(Clone, PartialEq, Eq)] +pub(crate) struct RowAt { + /// One of these rides on every line of a patch, so the path is shared + /// rather than copied per row. + pub(crate) path: SharedString, + pub(crate) id: RowId, +} + #[derive(PartialEq, Eq)] pub(crate) enum DiffRow { /// The space that used to be the `gap_3` of a flex column. @@ -54,8 +71,14 @@ pub(crate) enum DiffRow { /// the hunk before. leads: bool, }, - Split(SplitRow), - Unified(UnifiedRow), + Split { + row: SplitRow, + at: RowAt, + }, + Unified { + row: UnifiedRow, + at: RowAt, + }, Truncated(Truncation), MoreFiles { rest: usize, @@ -152,18 +175,29 @@ fn file_rows(index: usize, file: &FileDiff, expanded: bool, mode: DiffViewMode) })]; if expanded && (!file.hunks.is_empty() || file.truncated.is_some()) { + let path = SharedString::from(file.path.clone()); for (h, hunk) in file.hunks.iter().enumerate() { rows.push(DiffRow::HunkHeader { text: hunk.header.clone(), leads: h == 0, }); + let at = |row| RowAt { + path: path.clone(), + id: RowId { hunk: h, row }, + }; match mode { - DiffViewMode::Split => { - rows.extend(split_hunk(&hunk.lines).into_iter().map(DiffRow::Split)) - } - DiffViewMode::Unified => { - rows.extend(unified_rows(&hunk.lines).into_iter().map(DiffRow::Unified)) - } + DiffViewMode::Split => rows.extend( + split_hunk(&hunk.lines) + .into_iter() + .enumerate() + .map(|(r, row)| DiffRow::Split { row, at: at(r) }), + ), + DiffViewMode::Unified => rows.extend( + unified_rows(&hunk.lines) + .into_iter() + .enumerate() + .map(|(r, row)| DiffRow::Unified { row, at: at(r) }), + ), } } if let Some(reason) = file.truncated { @@ -252,6 +286,63 @@ mod tests { } } + /// `(path, hunk, row)` for each line row, in list order. + fn line_coords(rows: &[DiffRow]) -> Vec<(String, usize, usize)> { + rows.iter() + .filter_map(|row| match row { + DiffRow::Split { at, .. } | DiffRow::Unified { at, .. } => { + Some((at.path.to_string(), at.id.hunk, at.id.row)) + } + _ => None, + }) + .collect() + } + + /// A row says where it is in the *patch*, not where it is in the list. + /// Collapsing a file splices the rows below it up the list; a range keyed + /// on list positions would stay where it was and come away naming other + /// code. + #[test] + fn a_line_row_names_its_file_and_its_place_in_the_hunk() { + let snap = snapshot(vec![file("a.rs", 1), file("b.rs", 1)]); + let open = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + let coords = line_coords(&open); + assert_eq!( + coords, + vec![ + ("a.rs".to_string(), 0, 0), + ("a.rs".to_string(), 0, 1), + ("a.rs".to_string(), 0, 2), + ("a.rs".to_string(), 0, 3), + ("b.rs".to_string(), 0, 0), + ("b.rs".to_string(), 0, 1), + ("b.rs".to_string(), 0, 2), + ("b.rs".to_string(), 0, 3), + ] + ); + + let collapsed = HashMap::from([("a.rs".to_string(), false)]); + let after = build_rows(&snap, &collapsed, None, DiffViewMode::Unified, false); + assert_eq!( + line_coords(&after), + coords[4..].to_vec(), + "b.rs's lines moved up the list and kept the coordinates they had" + ); + } + + /// Every hunk restarts the row count, so a range that spans two of them is + /// read hunk by hunk rather than as one run. + #[test] + fn each_hunk_numbers_its_own_rows() { + let snap = snapshot(vec![file("a.rs", 2)]); + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Split, false); + let hunks: Vec<_> = line_coords(&rows) + .into_iter() + .map(|(_, hunk, row)| (hunk, row)) + .collect(); + assert_eq!(hunks, vec![(0, 0), (0, 1), (0, 2), (1, 0), (1, 1), (1, 2)]); + } + fn shape(rows: &[DiffRow]) -> Vec<&'static str> { rows.iter() .map(|row| match row { @@ -259,8 +350,8 @@ mod tests { DiffRow::Oversized => "oversized", DiffRow::FileHeader(_) => "file", DiffRow::HunkHeader { .. } => "hunk", - DiffRow::Split(_) => "split", - DiffRow::Unified(_) => "unified", + DiffRow::Split { .. } => "split", + DiffRow::Unified { .. } => "unified", DiffRow::Truncated(_) => "truncated", DiffRow::MoreFiles { .. } => "more-files", DiffRow::UntrackedHeader { .. } => "untracked-header", diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index d3d60b72..8d5eb30c 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -4,11 +4,11 @@ use std::rc::Rc; use std::sync::Arc; use gpui::{ - AnyElement, Background, FocusHandle, FontWeight, Hsla, KeyDownEvent, Pixels, SharedString, - Window, div, prelude::*, px, + AnyElement, Background, FocusHandle, FontWeight, Hsla, KeyDownEvent, MouseButton, + MouseDownEvent, MouseMoveEvent, Pixels, SharedString, Window, div, prelude::*, px, }; use gpui_component::button::Button; -use gpui_component::menu::ContextMenuExt as _; +use gpui_component::menu::{ContextMenuExt as _, PopupMenuItem}; use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex}; use crate::core::config::{Config, DiffViewMode}; @@ -23,8 +23,8 @@ use crate::terminal::git_diff::{ /// line budget below cuts rendering long before this does anyway. const MAX_PREVIEW_BYTES: u64 = 4 * 1024 * 1024; use crate::ui::app::Tty7App; -use crate::ui::diff_list::{DiffRow, FileHead}; -use crate::ui::diff_rows::{Side, SplitCell, SplitRow, UnifiedRow}; +use crate::ui::diff_list::{DiffRow, FileHead, RowAt}; +use crate::ui::diff_rows::{DiffSelection, Side, SplitCell, SplitRow, UnifiedRow}; use crate::ui::document_column::DocumentChrome; use crate::ui::i18n::{L10nKey, t, t_fmt, t_plural}; use crate::ui::right_panel::info_chip; @@ -57,6 +57,15 @@ pub(crate) struct DiffOverlayState { /// cadence a tracked file's does. pub(crate) preview: Option<(String, Option>)>, pub(crate) preview_loading: Option, + /// The rows the pointer has dragged over, and whether it is still down. + /// + /// A diff is read in order to be copied out of, and until this existed the + /// text on screen was unreachable — no selection, no clipboard, nothing + /// but retyping it (#721). Line-granular on purpose: the rows are a grid + /// of independent elements, not one text run, so a range of them is the + /// selection this layout can honestly offer. + pub(crate) selection: Option, + pub(crate) selecting: bool, /// The virtualised list the rows scroll in. Held across frames: it owns /// the scroll position, and the row heights gpui has measured. pub(crate) list: gpui::ListState, @@ -142,6 +151,10 @@ impl Tty7App { } Some(o) => { o.focus = focus; + // Another file is on screen now; the range belonged to the + // one that left. + o.selection = None; + o.selecting = false; let handle = o.focus_handle.clone(); window.focus(&handle, cx); cx.notify(); @@ -168,6 +181,8 @@ impl Tty7App { focus, preview: None, preview_loading: None, + selection: None, + selecting: false, list: gpui::ListState::new(0, gpui::ListAlignment::Top, px(256.)) .with_size_hint(DIFF_LINE_H), rows: Rc::new(Vec::new()), @@ -206,6 +221,127 @@ impl Tty7App { } } + /// Begin a drag at `at`, in the column it was pressed in. + fn start_diff_selection( + &mut self, + at: &RowAt, + mode: DiffViewMode, + side: Option, + window: &mut Window, + cx: &mut Context, + ) { + let active = self.active; + let Some(overlay) = self + .tabs + .get_mut(active) + .and_then(|t| t.diff_overlay.as_mut()) + else { + return; + }; + overlay.selection = Some(DiffSelection { + path: at.path.to_string(), + mode, + side, + anchor: at.id, + head: at.id, + }); + overlay.selecting = true; + let handle = overlay.focus_handle.clone(); + // Copying needs the overlay to hold the keyboard. Docked beside a + // shell it often does not, and Ctrl+C would otherwise reach the pane + // and interrupt whatever is running in it. + window.focus(&handle, cx); + cx.notify(); + } + + /// Extend the drag in flight to `at`. + /// + /// `held` is what the pointer is still pressing. A move with nothing held + /// means the button came up somewhere the overlay never saw — over a pane, + /// or outside the window — so the drag ends here rather than resuming the + /// next time the pointer wanders back over a row. + fn extend_diff_selection( + &mut self, + at: &RowAt, + side: Option, + held: Option, + cx: &mut Context, + ) { + let active = self.active; + let Some(overlay) = self + .tabs + .get_mut(active) + .and_then(|t| t.diff_overlay.as_mut()) + else { + return; + }; + if !overlay.selecting { + return; + } + if held != Some(MouseButton::Left) { + overlay.selecting = false; + cx.notify(); + return; + } + let Some(sel) = overlay.selection.as_mut() else { + return; + }; + if sel.path != at.path.as_ref() || sel.side != side || sel.head == at.id { + return; + } + sel.head = at.id; + cx.notify(); + } + + fn end_diff_selection(&mut self, cx: &mut Context) { + let active = self.active; + if let Some(overlay) = self + .tabs + .get_mut(active) + .and_then(|t| t.diff_overlay.as_mut()) + && overlay.selecting + { + overlay.selecting = false; + cx.notify(); + } + } + + /// Put the selected rows on the clipboard, as the file spells them. + fn copy_diff_selection(&self, cx: &mut Context) { + let Some(overlay) = self + .tabs + .get(self.active) + .and_then(|t| t.diff_overlay.as_ref()) + else { + return; + }; + let Some(sel) = overlay.selection.as_ref() else { + return; + }; + let hunks = match &overlay.load { + DiffLoad::Ready(snap) => snap + .files + .iter() + .find(|f| f.path == sel.path) + .map(|f| f.hunks.as_slice()), + _ => None, + } + // An untracked file has no patch in the snapshot — its rows are + // synthesized from the file's own bytes, and so is its text. + .or_else(|| match &overlay.preview { + Some((held, Some(file))) if *held == sel.path => Some(file.hunks.as_slice()), + _ => None, + }); + let Some(hunks) = hunks else { + return; + }; + let text = sel.text(hunks); + if text.is_empty() { + return; + } + cx.write_to_clipboard(gpui::ClipboardItem::new_string(text)); + } + fn spawn_diff_probe(&mut self, cx: &mut Context) { let active = self.active; let Some(overlay) = self.tabs.get(active).and_then(|t| t.diff_overlay.as_ref()) else { @@ -326,6 +462,10 @@ impl Tty7App { // A new snapshot restarts any untracked preview: the file may // have changed with the tree, and the re-read costs one file. overlay.preview = None; + // The rows it was drawn against are gone. A range that survived + // would keep its coordinates and quietly cover other code. + overlay.selection = None; + overlay.selecting = false; landed = true; } if landed { @@ -429,7 +569,22 @@ impl Tty7App { if ev.keystroke.key.as_str() == "escape" { this.close_diff_overlay(window, cx); } + // The overlay takes focus when a row is dragged, so this is + // the copy key for the selection that drag made — and only + // then: with nothing selected it falls through to whatever + // else the window binds it to. + let mods = ev.keystroke.modifiers; + if ev.keystroke.key.as_str() == "c" && mods.secondary() && !mods.alt { + this.copy_diff_selection(cx); + } })) + // A drag that ends anywhere in the overlay ends here; one that + // ends outside it is caught by the next move over a row, which + // sees no button held. + .on_mouse_up( + MouseButton::Left, + cx.listener(|this, _, _window, cx| this.end_diff_selection(cx)), + ) .children(header) .child(content) .into_any_element(), @@ -811,6 +966,16 @@ impl Tty7App { let active = self.active; let overlay = self.tabs.get_mut(active)?.diff_overlay.as_mut()?; + // Forget a range the rows under it no longer answer to. The two views + // pair the same lines differently, so a row range drawn in one of them + // points at other code in the other. Here rather than beside the + // switch that flips the mode: this is the one place that knows which + // rows are about to be drawn. + if overlay.selection.as_ref().is_some_and(|s| s.mode != mode) { + overlay.selection = None; + overlay.selecting = false; + } + let snap = match &overlay.load { DiffLoad::Loading => return Some(DiffBody::Message(t(L10nKey::DiffReading))), DiffLoad::NotARepo => return Some(DiffBody::Message(t(L10nKey::DiffNotARepo))), @@ -881,11 +1046,22 @@ impl Tty7App { let font = SharedString::from(self.font_family.clone()); let app = cx.entity().downgrade(); let list = overlay.list.clone(); + // The selection is read here, once a frame, rather than keyed into + // `RowsKey`: it changes what a row *looks like*, not which rows there + // are, and re-flattening the patch for every step of a drag is the + // cost this list exists to avoid. `extend_diff_selection` notifies, + // the view renders, and the list rebuilds the rows on screen from the + // `Drag` this frame carries. + let drag = Drag { + sel: overlay.selection.clone().map(Rc::new), + selecting: overlay.selecting, + mode: view_mode(cx), + }; let body = gpui::list(list.clone(), move |ix, _window, cx| { #[cfg(test)] row_probe::record(); match rows.get(ix) { - Some(row) => diff_row_element(row, &font, &snap, &app, cx), + Some(row) => diff_row_element(row, ix, &drag, &font, &snap, &app, cx), // The list is spliced in step with `rows`, so this is // unreachable — and an empty row is a better answer to a bug // than an index panic in a paint. @@ -931,6 +1107,40 @@ pub(crate) mod row_probe { } } +/// What a row needs to take part in a drag, for the frame it is drawn in. +/// +/// Read off the overlay once and moved into the list's item builder, so a step +/// of a drag costs a refcount bump rather than a walk of the patch. +struct Drag { + sel: Option>, + /// Whether a drag is in flight. Rows only listen for pointer movement + /// while one is: a diff runs to thousands of rows, and a listener each is + /// worth paying for during a drag and not otherwise. + selecting: bool, + /// The view the rows on screen are drawn in, which is the view a press + /// starts its selection in. + mode: DiffViewMode, +} + +impl Drag { + /// Whether the selection covers this cell. `side` names the column a split + /// cell sits in, and is `None` for a unified row — a selection made in one + /// column never lights up the other. + fn covers(&self, at: &RowAt, side: Option) -> bool { + self.sel + .as_ref() + .is_some_and(|sel| sel.covers(at.path.as_ref(), at.id, side)) + } + + /// Whether this row is inside the selection at all, whichever column the + /// drag ran down. What decides whether the row offers to copy it. + fn holds(&self, at: &RowAt) -> bool { + self.sel + .as_ref() + .is_some_and(|sel| sel.covers(at.path.as_ref(), at.id, sel.side)) + } +} + /// What the overlay's scrolling area holds this frame. enum DiffBody { Message(&'static str), @@ -1077,6 +1287,8 @@ fn hunk_rule(cx: &gpui::App) -> Hsla { /// One row, inset the way every row in the list is. fn diff_row_element( row: &DiffRow, + ix: usize, + drag: &Drag, font: &SharedString, snap: &Arc, app: &gpui::WeakEntity, @@ -1106,8 +1318,20 @@ fn diff_row_element( // The lines run the full width of the list. A diff is read as a // column of code, and code that is inset from both sides reads as a // quotation of itself. - DiffRow::Split(row) => diff_split_row(row, font, cx).into_any_element(), - DiffRow::Unified(row) => diff_unified_row(row, font, cx).into_any_element(), + DiffRow::Split { row, at } => copy_menu( + diff_split_row(row, at, drag, font, app, cx), + ix, + at, + drag, + app, + ), + DiffRow::Unified { row, at } => copy_menu( + diff_unified_row(row, at, drag, font, app, cx), + ix, + at, + drag, + app, + ), DiffRow::Truncated(reason) => { let note = match reason { Truncation::PerFile => t_fmt( @@ -1144,6 +1368,72 @@ fn diff_row_element( } } +/// The one place a copy is offered by name, on the rows that would be copied. +/// +/// A drag says what will be copied; the menu says that copying is a thing you +/// can do. It hangs on the selected rows themselves because with the cards +/// gone there is no longer an element that owns a file's lines, and the +/// overlay root already carries the header's own menu — two of them over one +/// right-click would open two popups. +fn copy_menu( + row: gpui::Div, + ix: usize, + at: &RowAt, + drag: &Drag, + app: &gpui::WeakEntity, +) -> AnyElement { + if !drag.holds(at) { + return row.into_any_element(); + } + let app = app.clone(); + row.id(("diff-row-menu", ix)) + .context_menu(move |menu, _window, _cx| { + menu.item(PopupMenuItem::new(t(L10nKey::DiffCopySelection)).on_click({ + let app = app.clone(); + move |_, _window, cx| { + app.update(cx, |this, cx| this.copy_diff_selection(cx)).ok(); + } + })) + }) + .into_any_element() +} + +/// Wire one drawn row into the drag: a press starts a selection there, and +/// while one is in flight a move across the row extends it. +fn diff_row_drag( + el: E, + at: &RowAt, + side: Option, + drag: &Drag, + app: &gpui::WeakEntity, +) -> E { + let mode = drag.mode; + // The I-beam is the only standing sign that this text can be taken; + // nothing else about a row says so until one is dragged. + let el = el.cursor_text().on_mouse_down(MouseButton::Left, { + let (app, at) = (app.clone(), at.clone()); + move |_: &MouseDownEvent, window, cx| { + app.update(cx, |this, cx| { + this.start_diff_selection(&at, mode, side, window, cx); + }) + .ok(); + } + }); + if !drag.selecting { + return el; + } + el.on_mouse_move({ + let (app, at) = (app.clone(), at.clone()); + move |ev: &MouseMoveEvent, _window, cx| { + let held = ev.pressed_button; + app.update(cx, |this, cx| { + this.extend_diff_selection(&at, side, held, cx); + }) + .ok(); + } + }) +} + /// The margin the file rows keep from the edge of the list. fn padded(row: AnyElement) -> AnyElement { div().w_full().px_2().child(row).into_any_element() @@ -1329,29 +1619,76 @@ fn diff_untracked_row( .into_any_element() } -fn diff_split_row(row: &SplitRow, font: &SharedString, cx: &gpui::App) -> impl IntoElement { +fn diff_split_row( + row: &SplitRow, + at: &RowAt, + drag: &Drag, + font: &SharedString, + app: &gpui::WeakEntity, + cx: &gpui::App, +) -> gpui::Div { h_flex() .w_full() .h(DIFF_LINE_H) .items_stretch() .text_xs() .font_family(font.clone()) - .child(diff_split_cell(row.left.as_ref(), Side::Old, cx)) + .child(diff_split_cell( + row.left.as_ref(), + Side::Old, + at, + drag, + app, + cx, + )) .child(div().flex_shrink_0().w(px(1.)).bg(hunk_rule(cx))) - .child(diff_split_cell(row.right.as_ref(), Side::New, cx)) + .child(diff_split_cell( + row.right.as_ref(), + Side::New, + at, + drag, + app, + cx, + )) } -fn diff_split_cell(cell: Option<&SplitCell>, side: Side, cx: &gpui::App) -> AnyElement { +fn diff_split_cell( + cell: Option<&SplitCell>, + side: Side, + at: &RowAt, + drag: &Drag, + app: &gpui::WeakEntity, + cx: &gpui::App, +) -> AnyElement { let base = h_flex().flex_1().min_w_0().h_full().items_center(); let Some(cell) = cell else { - return base.bg(cx.theme().muted.opacity(0.3)).into_any_element(); + // Blank, but still this row's half of this column. Left inert it is a + // dead band under the pointer — no I-beam, a press that starts + // nothing, and a range that visibly stops at the padding and resumes + // below it. A one-sided change is the ordinary shape of a diff, so + // that band runs down most of one column. + let fill = match drag.covers(at, Some(side)) { + true => cx.theme().selection, + false => cx.theme().muted.opacity(0.3), + }; + return diff_row_drag(base, at, Some(side), drag, app) + .bg(fill) + .into_any_element(); }; let (marker, tint) = match (cell.changed, side) { (true, Side::Old) => ("−", Some(cx.theme().danger.opacity(0.12))), (true, Side::New) => ("+", Some(cx.theme().success.opacity(0.12))), (false, _) => (" ", None), }; - base.when_some(tint, |row, bg| row.bg(bg)) + // A selected cell wears the theme's selection colour in place of its own + // wash, the way selected text does anywhere else. The `+`/`−` in front of + // the code still says which side of the change it is. + let fill = match drag.covers(at, Some(side)) { + true => Some(cx.theme().selection), + false => tint, + }; + diff_row_drag(base, at, Some(side), drag, app) + .when_some(fill, |row, bg| row.bg(bg)) .child( h_flex() .flex_shrink_0() @@ -1384,7 +1721,14 @@ fn diff_split_cell(cell: Option<&SplitCell>, side: Side, cx: &gpui::App) -> AnyE /// than riding in the text: with three kinds of line stacked in one column, an /// inlined marker would leave the context lines' code starting two characters /// left of everything else. -fn diff_unified_row(row: &UnifiedRow, font: &SharedString, cx: &gpui::App) -> impl IntoElement { +fn diff_unified_row( + row: &UnifiedRow, + at: &RowAt, + drag: &Drag, + font: &SharedString, + app: &gpui::WeakEntity, + cx: &gpui::App, +) -> gpui::Div { let (marker_color, tint) = match row.kind { LineKind::Added => (cx.theme().success, Some(cx.theme().success.opacity(0.12))), LineKind::Removed => (cx.theme().danger, Some(cx.theme().danger.opacity(0.12))), @@ -1399,13 +1743,17 @@ fn diff_unified_row(row: &UnifiedRow, font: &SharedString, cx: &gpui::App) -> im .text_color(cx.theme().muted_foreground.opacity(0.7)) .child(no.map(|n| n.to_string()).unwrap_or_default()) }; - h_flex() + let fill = match drag.covers(at, None) { + true => Some(cx.theme().selection), + false => tint, + }; + diff_row_drag(h_flex(), at, None, drag, app) .w_full() .h(DIFF_LINE_H) .items_center() .text_xs() .font_family(font.clone()) - .when_some(tint, |line, bg| line.bg(bg)) + .when_some(fill, |line, bg| line.bg(bg)) .child(gutter(row.old)) .child(gutter(row.new)) // The split view's centre rule, in the one place it still means the @@ -2899,3 +3247,312 @@ mod render_idle_gpui_tests { let _ = std::fs::remove_dir_all(&root); } } + +/// The drag itself, in a window: a press, a move, and what lands on the +/// clipboard. The row geometry is `diff_rows`' business and tested there — +/// what these check is the wiring the list hangs on it. +#[cfg(test)] +mod selection_gpui_tests { + use super::*; + use crate::terminal::git_diff::{DiffLine, LineKind}; + use crate::ui::app::test_window; + use crate::ui::diff_rows::RowId; + use crate::ui::pane::{Pane, PaneSlot}; + use crate::ui::pending_pane::{PendingPane, PendingSpawn}; + use gpui::{Entity, MouseButton, TestAppContext, VisualTestContext}; + + const PATH: &str = "src/a.rs"; + + fn line(kind: LineKind, old: Option, new: Option, text: &str) -> DiffLine { + DiffLine { + kind, + old_no: old, + new_no: new, + text: text.to_string(), + } + } + + /// `a` kept, `b`/`c` replaced by `B`, `d` kept — four split rows, five + /// unified ones. + fn patched_file() -> FileDiff { + FileDiff { + path: PATH.to_string(), + old_path: None, + status: FileStatus::Modified, + added: 1, + removed: 2, + binary: false, + truncated: None, + hunks: vec![git_diff::Hunk { + header: "@@ -1,4 +1,3 @@".to_string(), + lines: vec![ + line(LineKind::Context, Some(1), Some(1), "a"), + line(LineKind::Removed, Some(2), None, "b"), + line(LineKind::Removed, Some(3), None, "c"), + line(LineKind::Added, None, Some(2), "B"), + line(LineKind::Context, Some(4), Some(3), "d"), + ], + }], + } + } + + /// The coordinate the list carries on the row `row` of the only hunk. + fn at(row: usize) -> RowAt { + RowAt { + path: PATH.into(), + id: RowId { hunk: 0, row }, + } + } + + /// A window with one tab, showing that patch. Built by hand rather than + /// through `open_diff_overlay`: that dispatches a probe, and a probe + /// landing mid-test would drop the selection under it. + fn window(cx: &mut TestAppContext) -> (Entity, VisualTestContext) { + let (app, mut vcx) = test_window::harness(cx); + app.update_in(&mut vcx, |app, _, cx| { + let pending = cx.new(|cx| { + PendingPane::new( + "test-box", + PendingSpawn { + workspace: None, + working_directory: None, + restore_pane: None, + shell: None, + agent: None, + agent_session_id: None, + agent_launch_argv: None, + owner: None, + font_size: 14.0, + }, + cx, + ) + }); + app.tabs + .push(crate::ui::app::Tab::new(Pane::leaf(PaneSlot::Connecting( + pending, + )))); + app.active = 0; + app.tabs[0].diff_overlay = Some(DiffOverlayState { + host_id: crate::ui::host_ops::HostId::LOCAL, + cwd: PathBuf::from("/repo"), + source: DiffSource::Head, + focus_handle: cx.focus_handle(), + load: DiffLoad::Ready(Arc::new(DiffSnapshot { + files: vec![patched_file()], + ..Default::default() + })), + loading: false, + expanded: HashMap::new(), + focus: None, + preview: None, + preview_loading: None, + selection: None, + selecting: false, + list: gpui::ListState::new(0, gpui::ListAlignment::Top, px(256.)) + .with_size_hint(DIFF_LINE_H), + rows: Rc::new(Vec::new()), + rows_key: None, + epoch: None, + }); + }); + (app, vcx) + } + + fn drag( + app: &Entity, + vcx: &mut VisualTestContext, + mode: DiffViewMode, + side: Option, + from: usize, + to: usize, + ) { + // The view mode is a window-wide setting, and the overlay drops a + // selection whose rows the current view never drew — so a drag in the + // unified view has to happen with the unified view on. + vcx.update(|_, cx| { + let mut cfg = cx.global::().clone(); + cfg.diff_view = mode; + cx.set_global(cfg); + }); + app.update_in(vcx, |this, window, cx| { + this.start_diff_selection(&at(from), mode, side, window, cx); + this.extend_diff_selection(&at(to), side, Some(MouseButton::Left), cx); + }); + } + + fn copied(app: &Entity, vcx: &mut VisualTestContext) -> Option { + app.update_in(vcx, |this, _, cx| this.copy_diff_selection(cx)); + vcx.update(|_, cx| cx.read_from_clipboard().and_then(|item| item.text())) + } + + fn selection(app: &Entity, vcx: &mut VisualTestContext) -> Option { + app.update_in(vcx, |this, _, _| { + this.tabs[0] + .diff_overlay + .as_ref() + .and_then(|o| o.selection.clone()) + }) + } + + #[gpui::test] + fn a_drag_down_a_column_copies_that_column(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 0, 3); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("a\nB\nd")); + + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::Old), 0, 3); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("a\nb\nc\nd")); + + drag(&app, &mut vcx, DiffViewMode::Unified, None, 1, 3); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("b\nc\nB")); + } + + /// A drag that runs up the list leaves the head above the anchor. The + /// range is read in drawn order either way, so it copies what the same two + /// rows copy dragged the other way round. + #[gpui::test] + fn a_drag_up_a_column_copies_the_same_rows(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 3, 0); + let sel = selection(&app, &mut vcx).expect("the drag this test just made"); + assert!( + sel.head < sel.anchor, + "the drag ended above where it started" + ); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("a\nB\nd")); + + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::Old), 3, 0); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("a\nb\nc\nd")); + + drag(&app, &mut vcx, DiffViewMode::Unified, None, 3, 1); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("b\nc\nB")); + } + + /// The overlay is often drawn beside a live shell that holds the keyboard. + /// Ctrl+C is the copy key only once the overlay has taken focus — until + /// then the same keystroke would reach the pane and interrupt whatever is + /// running in it. + #[gpui::test] + fn starting_a_drag_takes_the_keyboard(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 0, 1); + let focused = app.update_in(&mut vcx, |this, window, _| { + this.tabs[0] + .diff_overlay + .as_ref() + .expect("the overlay this window was built with") + .focus_handle + .is_focused(window) + }); + assert!(focused); + } + + /// A move with no button held is a release the overlay never saw — over a + /// pane, or outside the window. The drag ends there rather than resuming + /// the next time the pointer crosses a row. + #[gpui::test] + fn a_release_the_overlay_missed_ends_the_drag(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 0, 1); + + app.update_in(&mut vcx, |this, _, cx| { + this.extend_diff_selection(&at(3), Some(Side::New), None, cx); + }); + assert_eq!( + copied(&app, &mut vcx).as_deref(), + Some("a\nB"), + "the range stops where the pointer was last seen holding the button" + ); + } + + /// The two views pair the same lines into different rows, so a range drawn + /// in one of them points at other code in the other. `sync_diff_rows` is + /// where the rows for a frame are settled, so it is where the range that + /// no longer names any of them is dropped. + #[gpui::test] + fn switching_the_view_drops_the_selection(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 0, 3); + + vcx.update(|_, cx| { + let mut cfg = cx.global::().clone(); + cfg.diff_view = DiffViewMode::Unified; + cx.set_global(cfg); + }); + app.update_in(&mut vcx, |this, _, cx| { + let _ = this.sync_diff_rows(cx); + }); + assert!(selection(&app, &mut vcx).is_none()); + } + + /// A fresh read re-cuts the hunks. A range that survived one would keep + /// its coordinates and quietly cover other code. + #[gpui::test] + fn a_fresh_snapshot_drops_the_selection(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + drag(&app, &mut vcx, DiffViewMode::Split, Some(Side::New), 0, 3); + + app.update_in(&mut vcx, |this, _, cx| { + this.install_diff_snapshot( + crate::ui::host_ops::HostId::LOCAL, + &PathBuf::from("/repo"), + &DiffSource::Head, + Some(Arc::new(DiffSnapshot { + files: vec![patched_file()], + ..Default::default() + })), + cx, + ); + let overlay = this.tabs[0].diff_overlay.as_ref().unwrap(); + assert!(overlay.selection.is_none()); + assert!(!overlay.selecting); + }); + } + + #[gpui::test] + fn a_copy_with_nothing_selected_leaves_the_clipboard_alone(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + vcx.update(|_, cx| { + cx.write_to_clipboard(gpui::ClipboardItem::new_string("untouched".into())) + }); + assert_eq!(copied(&app, &mut vcx).as_deref(), Some("untouched")); + } + + /// Collapsing a file above the selection re-cuts the list, but not the + /// rows the range names. A selection keyed on list positions would come + /// away pointing at whatever slid into those slots. + #[gpui::test] + fn collapsing_a_file_above_the_range_leaves_it_on_the_same_lines(cx: &mut TestAppContext) { + let (app, mut vcx) = window(cx); + app.update_in(&mut vcx, |this, _, _| { + let overlay = this.tabs[0].diff_overlay.as_mut().unwrap(); + overlay.load = DiffLoad::Ready(Arc::new(DiffSnapshot { + files: vec![ + FileDiff { + path: "src/above.rs".to_string(), + ..patched_file() + }, + patched_file(), + ], + ..Default::default() + })); + }); + drag(&app, &mut vcx, DiffViewMode::Unified, None, 1, 3); + + app.update_in(&mut vcx, |this, _, cx| { + let active = this.active; + { + let overlay = this.tabs[active].diff_overlay.as_mut().unwrap(); + overlay.expanded.insert("src/above.rs".to_string(), false); + } + let _ = this.sync_diff_rows(cx); + }); + assert_eq!( + copied(&app, &mut vcx).as_deref(), + Some("b\nc\nB"), + "the range still names the same three lines of the same file" + ); + } +} diff --git a/src/ui/diff_rows.rs b/src/ui/diff_rows.rs index b5316d50..e6f405ad 100644 --- a/src/ui/diff_rows.rs +++ b/src/ui/diff_rows.rs @@ -4,7 +4,8 @@ //! the pairing logic lives here — outside either renderer — and is unit tested //! without a window. -use crate::terminal::git_diff::{DiffLine, LineKind}; +use crate::core::config::DiffViewMode; +use crate::terminal::git_diff::{DiffLine, Hunk, LineKind}; /// A tab is worth this many columns. Not configurable: a diff is read next to /// the file's other lines, not on its own, and the grid has to line up. @@ -28,6 +29,10 @@ pub(crate) struct SplitCell { pub(crate) no: Option, pub(crate) text: String, pub(crate) changed: bool, + /// Which of the hunk's own lines this cell draws. `text` is the tab- + /// expanded copy the grid needs; a copy to the clipboard has to reach past + /// it to the line as the file wrote it. + pub(crate) line: usize, } #[derive(PartialEq, Eq)] @@ -40,18 +45,24 @@ pub(crate) struct SplitRow { /// rewritten line sits opposite the line it replaced. Whichever run is shorter /// leaves empty cells at the bottom of the pair. pub(crate) fn split_hunk(lines: &[DiffLine]) -> Vec { - fn flush(rows: &mut Vec, rem: &mut Vec<&DiffLine>, add: &mut Vec<&DiffLine>) { + fn flush( + rows: &mut Vec, + rem: &mut Vec<(usize, &DiffLine)>, + add: &mut Vec<(usize, &DiffLine)>, + ) { for i in 0..rem.len().max(add.len()) { rows.push(SplitRow { - left: rem.get(i).map(|l| SplitCell { + left: rem.get(i).map(|(idx, l)| SplitCell { no: l.old_no, text: expand_tabs(&l.text), changed: true, + line: *idx, }), - right: add.get(i).map(|l| SplitCell { + right: add.get(i).map(|(idx, l)| SplitCell { no: l.new_no, text: expand_tabs(&l.text), changed: true, + line: *idx, }), }); } @@ -60,12 +71,12 @@ pub(crate) fn split_hunk(lines: &[DiffLine]) -> Vec { } let mut rows = Vec::new(); - let mut rem: Vec<&DiffLine> = Vec::new(); - let mut add: Vec<&DiffLine> = Vec::new(); - for line in lines { + let mut rem: Vec<(usize, &DiffLine)> = Vec::new(); + let mut add: Vec<(usize, &DiffLine)> = Vec::new(); + for (idx, line) in lines.iter().enumerate() { match line.kind { - LineKind::Removed => rem.push(line), - LineKind::Added => add.push(line), + LineKind::Removed => rem.push((idx, line)), + LineKind::Added => add.push((idx, line)), LineKind::Context => { flush(&mut rows, &mut rem, &mut add); rows.push(SplitRow { @@ -73,11 +84,13 @@ pub(crate) fn split_hunk(lines: &[DiffLine]) -> Vec { no: line.old_no, text: expand_tabs(&line.text), changed: false, + line: idx, }), right: Some(SplitCell { no: line.new_no, text: expand_tabs(&line.text), changed: false, + line: idx, }), }); } @@ -93,6 +106,9 @@ pub(crate) struct UnifiedRow { pub(crate) new: Option, pub(crate) kind: LineKind, pub(crate) text: String, + /// Which of the hunk's own lines this row draws — the same reach past + /// `text` a [`SplitCell`] needs, and one row is one line here. + pub(crate) line: usize, } /// One row per line, in git's own order — every removal in a run first, then @@ -102,15 +118,134 @@ pub(crate) struct UnifiedRow { pub(crate) fn unified_rows(lines: &[DiffLine]) -> Vec { lines .iter() - .map(|line| UnifiedRow { + .enumerate() + .map(|(idx, line)| UnifiedRow { old: line.old_no, new: line.new_no, kind: line.kind, text: expand_tabs(&line.text), + line: idx, }) .collect() } +/// One hunk, already turned into whichever kind of row the current view draws. +pub(crate) enum HunkRows { + Split(Vec), + Unified(Vec), +} + +impl HunkRows { + pub(crate) fn build(mode: DiffViewMode, lines: &[DiffLine]) -> Self { + match mode { + DiffViewMode::Split => Self::Split(split_hunk(lines)), + DiffViewMode::Unified => Self::Unified(unified_rows(lines)), + } + } +} + +/// Where a row sits in a file's card: which hunk, and which row inside it. +/// +/// Ordered the way the rows are drawn, so a drag is nothing more than the +/// range between the two of these the pointer touched. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug)] +pub(crate) struct RowId { + pub(crate) hunk: usize, + pub(crate) row: usize, +} + +/// The rows a drag has run over, in one file. +#[derive(Clone, Debug)] +pub(crate) struct DiffSelection { + /// The file the drag started in. A selection never spans two cards: two + /// files are two documents, and a range across them would copy code from + /// one into the middle of another. + pub(crate) path: String, + /// The view the rows were drawn in when the drag happened. The two views + /// pair the same lines into different rows, so a selection means nothing + /// in the other one — the overlay drops it when the mode changes rather + /// than pretending to translate it. + pub(crate) mode: DiffViewMode, + /// Which column of the split view the drag started in; that column alone + /// is copied, so dragging down the left gets the code as it was and down + /// the right gets it as it is. `None` in the unified view, where a row + /// *is* the line. + pub(crate) side: Option, + pub(crate) anchor: RowId, + pub(crate) head: RowId, +} + +impl DiffSelection { + fn range(&self) -> (RowId, RowId) { + match self.anchor <= self.head { + true => (self.anchor, self.head), + false => (self.head, self.anchor), + } + } + + /// Whether the selection covers this row. `side` names the column a split + /// cell sits in, and is `None` for a unified row — a selection made in one + /// column never lights up the other. + pub(crate) fn covers(&self, path: &str, id: RowId, side: Option) -> bool { + if self.path != path || self.side != side { + return false; + } + let (start, end) = self.range(); + start <= id && id <= end + } + + /// The code the drag ran over, spelled the way the file spells it. + /// + /// No `+`/`−` marker and no line numbers: what lands on the clipboard has + /// to compile when it is pasted back, and the gutter is the diff talking + /// about the code rather than the code itself. Tabs are left alone for the + /// same reason — [`expand_tabs`] is how the grid draws a tab, not how the + /// file stores one, and four spaces pasted into a tab-indented file is a + /// whitespace bug the copier did not ask for. + /// + /// A split row with nothing on the selected side contributes nothing: the + /// blank half of a pair is padding the layout invented, not an empty line + /// in the file. + pub(crate) fn text(&self, hunks: &[Hunk]) -> String { + let (start, end) = self.range(); + let mut out: Vec<&str> = Vec::new(); + for (h, hunk) in hunks.iter().enumerate() { + if h < start.hunk || h > end.hunk { + continue; + } + let first = if h == start.hunk { start.row } else { 0 }; + let last = if h == end.hunk { end.row } else { usize::MAX }; + let mut push = |line: usize| { + if let Some(l) = hunk.lines.get(line) { + out.push(&l.text); + } + }; + match HunkRows::build(self.mode, &hunk.lines) { + HunkRows::Split(rows) => { + for row in rows.iter().take(last.saturating_add(1)).skip(first) { + let cell = match self.side.unwrap_or(Side::New) { + Side::Old => row.left.as_ref(), + Side::New => row.right.as_ref(), + }; + if let Some(cell) = cell { + push(cell.line); + } + } + } + HunkRows::Unified(rows) => { + for row in rows.iter().take(last.saturating_add(1)).skip(first) { + push(row.line); + } + } + } + } + out.join( + " +", + ) + } +} + #[cfg(test)] mod tests { use super::*; @@ -242,4 +377,194 @@ mod tests { "a context line fills two cells, a change fills one" ); } + fn patch(lines: Vec) -> Hunk { + Hunk { + header: "@@ -1,4 +1,3 @@".to_string(), + lines, + } + } + + fn drag( + mode: DiffViewMode, + side: Option, + anchor: (usize, usize), + head: (usize, usize), + ) -> DiffSelection { + DiffSelection { + path: "src/a.rs".to_string(), + mode, + side, + anchor: RowId { + hunk: anchor.0, + row: anchor.1, + }, + head: RowId { + hunk: head.0, + row: head.1, + }, + } + } + + #[test] + fn a_drag_down_the_new_column_copies_the_file_as_it_now_reads() { + let hunks = [patch(hunk())]; + let text = drag(DiffViewMode::Split, Some(Side::New), (0, 0), (0, 3)).text(&hunks); + assert_eq!( + text, "a\nB\nd", + "the removed-only row is padding on this side, not an empty line" + ); + } + + #[test] + fn a_drag_down_the_old_column_copies_the_file_as_it_was() { + let hunks = [patch(hunk())]; + let text = drag(DiffViewMode::Split, Some(Side::Old), (0, 0), (0, 3)).text(&hunks); + assert_eq!(text, "a\nb\nc\nd"); + } + + #[test] + fn the_unified_view_copies_the_rows_in_the_order_it_drew_them() { + let hunks = [patch(hunk())]; + let text = drag(DiffViewMode::Unified, None, (0, 1), (0, 3)).text(&hunks); + assert_eq!( + text, "b\nc\nB", + "both removals then the addition — the order on screen" + ); + } + + #[test] + fn a_drag_the_other_way_round_copies_the_same_rows() { + let hunks = [patch(hunk())]; + let forwards = drag(DiffViewMode::Unified, None, (0, 1), (0, 3)).text(&hunks); + let backwards = drag(DiffViewMode::Unified, None, (0, 3), (0, 1)).text(&hunks); + assert_eq!(forwards, backwards); + } + + #[test] + fn one_row_copies_one_line() { + let hunks = [patch(hunk())]; + assert_eq!( + drag(DiffViewMode::Unified, None, (0, 2), (0, 2)).text(&hunks), + "c" + ); + } + + /// What lands on the clipboard has to compile when it is pasted back, so + /// none of the diff's own furniture may ride along with it. + #[test] + fn nothing_the_gutter_draws_is_copied() { + let lines = vec![ + line(LineKind::Removed, Some(9), None, "let old = 1;"), + line(LineKind::Added, None, Some(9), "let new = 2;"), + ]; + let hunks = [patch(lines)]; + for (mode, side) in [ + (DiffViewMode::Split, Some(Side::Old)), + (DiffViewMode::Split, Some(Side::New)), + (DiffViewMode::Unified, None), + ] { + let text = drag(mode, side, (0, 0), (0, 9)).text(&hunks); + assert!(!text.contains('+'), "marker in {text:?}"); + assert!(!text.contains('−'), "marker in {text:?}"); + assert!(!text.contains('9'), "line number in {text:?}"); + } + } + + /// [`expand_tabs`] is how the grid draws a tab, not how the file stores + /// one. Copying the drawn text would paste four spaces into a tab-indented + /// file — a whitespace change nobody asked for. + #[test] + fn copying_keeps_the_tabs_the_file_was_written_with() { + let hunks = [patch(vec![line( + LineKind::Added, + None, + Some(1), + "\tindented", + )])]; + assert_eq!( + unified_rows(&hunks[0].lines)[0].text, + " indented", + "drawn with the tab expanded" + ); + assert_eq!( + drag(DiffViewMode::Unified, None, (0, 0), (0, 0)).text(&hunks), + "\tindented", + "copied with the tab intact" + ); + } + + #[test] + fn a_drag_across_hunks_copies_every_row_between_its_ends() { + let hunks = [ + patch(vec![ + line(LineKind::Context, Some(1), Some(1), "one"), + line(LineKind::Context, Some(2), Some(2), "two"), + ]), + patch(vec![ + line(LineKind::Context, Some(9), Some(9), "nine"), + line(LineKind::Context, Some(10), Some(10), "ten"), + ]), + ]; + let text = drag(DiffViewMode::Unified, None, (0, 1), (1, 0)).text(&hunks); + assert_eq!( + text, "two\nnine", + "the tail of the first hunk and the head of the second, nothing else" + ); + let all = drag(DiffViewMode::Split, Some(Side::New), (0, 0), (1, 1)).text(&hunks); + assert_eq!(all, "one\ntwo\nnine\nten"); + } + + #[test] + fn a_selection_lights_only_the_column_the_drag_started_in() { + let sel = drag(DiffViewMode::Split, Some(Side::New), (0, 0), (0, 2)); + let inside = RowId { hunk: 0, row: 1 }; + assert!(sel.covers("src/a.rs", inside, Some(Side::New))); + assert!(!sel.covers("src/a.rs", inside, Some(Side::Old))); + assert!( + !sel.covers("src/b.rs", inside, Some(Side::New)), + "a selection belongs to one file's card" + ); + assert!(!sel.covers("src/a.rs", RowId { hunk: 0, row: 3 }, Some(Side::New))); + assert!(!sel.covers("src/a.rs", RowId { hunk: 1, row: 0 }, Some(Side::New))); + } + + #[test] + fn a_unified_selection_never_lights_a_split_cell() { + let sel = drag(DiffViewMode::Unified, None, (0, 0), (0, 2)); + let inside = RowId { hunk: 0, row: 1 }; + assert!(sel.covers("src/a.rs", inside, None)); + assert!(!sel.covers("src/a.rs", inside, Some(Side::New))); + } + + /// Rows are ordered the way they are drawn, so the range between two of + /// them is exactly what the pointer crossed. + #[test] + fn rows_order_by_hunk_before_row() { + assert!(RowId { hunk: 0, row: 9 } < RowId { hunk: 1, row: 0 }); + assert!(RowId { hunk: 1, row: 0 } < RowId { hunk: 1, row: 1 }); + } + + #[test] + fn a_selection_pointing_past_the_patch_copies_nothing() { + let hunks = [patch(hunk())]; + assert_eq!( + drag(DiffViewMode::Unified, None, (4, 0), (4, 2)).text(&hunks), + "" + ); + assert_eq!( + drag(DiffViewMode::Unified, None, (0, 0), (0, 2)).text(&[]), + "" + ); + } + + /// The one place a view mode turns into rows, so a copy is read off the + /// rows the list drew rather than a second guess at them. + #[test] + fn each_view_builds_the_rows_its_renderer_draws() { + let lines = hunk(); + let split = HunkRows::build(DiffViewMode::Split, &lines); + assert!(matches!(split, HunkRows::Split(rows) if rows.len() == 4)); + let unified = HunkRows::build(DiffViewMode::Unified, &lines); + assert!(matches!(unified, HunkRows::Unified(rows) if rows.len() == 5)); + } } diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index ea205f5d..4bc309b2 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1205,6 +1205,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::DiffUntrackedSummary => "{count} untracked", L10nKey::DiffViewSplit => "Side by Side", L10nKey::DiffViewUnified => "Unified", + L10nKey::DiffCopySelection => "Copy Selected Lines", L10nKey::PendingConnecting => "Connecting to {machine}…", L10nKey::PendingUnreachable => "Could not reach {machine}", L10nKey::WorktreePromptNeedsName => "The worktree needs a name", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index ce986fc0..3a61cd94 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1271,6 +1271,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::DiffUntrackedSummary => "未追跡 {count}", L10nKey::DiffViewSplit => "左右分割", L10nKey::DiffViewUnified => "統合", + L10nKey::DiffCopySelection => "選択した行をコピー", L10nKey::PendingConnecting => "{machine} に接続中…", L10nKey::PendingUnreachable => "{machine} に到達できませんでした", L10nKey::WorktreePromptNeedsName => "ワークツリーには名前が必要です", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 1219d497..9464f67f 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -901,6 +901,7 @@ l10n_keys! { DiffUntrackedSummary, DiffViewSplit, DiffViewUnified, + DiffCopySelection, PendingConnecting, PendingUnreachable, WorktreePromptNeedsName, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 7063697a..3c6e6b27 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1143,6 +1143,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::DiffUntrackedSummary => "{count} 个未跟踪", L10nKey::DiffViewSplit => "并排", L10nKey::DiffViewUnified => "统一", + L10nKey::DiffCopySelection => "复制选中的行", L10nKey::PendingConnecting => "正在连接 {machine}…", L10nKey::PendingUnreachable => "无法连接到 {machine}", L10nKey::WorktreePromptNeedsName => "worktree 需要一个名称", From 1f9663020f26a0a8645ee92a076d83aee3f9e9fd Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:33:07 +0800 Subject: [PATCH 4/7] test(terminal): cover a pane's replay across a workspace switch (#789) Refs #711. Does not fix it; #711 stays open. --- src/terminal/remote.rs | 554 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 547 insertions(+), 7 deletions(-) diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 3cc42e90..29ab35be 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -2872,17 +2872,557 @@ fn win_size(size: TermSize, cell_w: u16, cell_h: u16) -> WinSize { } } +/// What a re-attach's replay actually leaves in the grid. +/// +/// Switching workspaces tears every pane down and attaches to the same daemon +/// pane again (#711), so the whole of a pane's screen has to survive one trip +/// through the replay — several ring segments, each preceded by the geometry it +/// was written at, and a snapshot frame far larger than one socket read. These +/// drive that over a real socket pair rather than a mock, because the loss +/// being hunted is between the wire and the grid; and they are not `cfg(unix)` +/// like their neighbours because nothing about the path is. +#[cfg(test)] +mod replay_tests { + use super::*; + use crate::daemon::protocol::DaemonMsg; + use crate::daemon::transport::Stream; + + pub(super) fn socket_pair() -> (Stream, Stream) { + #[cfg(unix)] + { + std::os::unix::net::UnixStream::pair().unwrap() + } + #[cfg(windows)] + { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let client_side = std::net::TcpStream::connect(addr).unwrap(); + let (daemon_side, _) = listener.accept().unwrap(); + (client_side, daemon_side) + } + } + + fn ws(cols: u16, rows: u16) -> WinSize { + WinSize { + cols, + rows, + cell_w: 8, + cell_h: 17, + } + } + + /// Everything the grid holds, scrollback included, one row per line. + fn all_text(term: &RemoteTerminal) -> String { + use alacritty_terminal::grid::Dimensions as _; + use alacritty_terminal::index::{Column, Line}; + let t = term.term.lock(); + let grid = t.grid(); + let mut out = String::new(); + let top = -(grid.history_size() as i32); + for line in top..grid.screen_lines() as i32 { + let row = &grid[Line(line)]; + let mut text = String::new(); + for col in 0..grid.columns() { + text.push(row[Column(col)].c); + } + let text = text.trim_end(); + if !text.is_empty() { + out.push_str(text); + out.push('\n'); + } + } + out + } + + /// The visible screen alone. + fn screen_text(term: &RemoteTerminal) -> String { + use alacritty_terminal::grid::Dimensions as _; + use alacritty_terminal::index::{Column, Line}; + let t = term.term.lock(); + let grid = t.grid(); + let mut out = String::new(); + for line in 0..grid.screen_lines() as i32 { + let row = &grid[Line(line)]; + let mut text = String::new(); + for col in 0..grid.columns() { + text.push(row[Column(col)].c); + } + let text = text.trim_end(); + if !text.is_empty() { + out.push_str(text); + out.push('\n'); + } + } + out + } + + /// The grid's width. A grid is born at the size its `RemoteTerminal` was + /// built with and only ever leaves it on a `DaemonMsg::Size`, so this is + /// what says which geometry frames were applied. + fn columns(term: &RemoteTerminal) -> usize { + use alacritty_terminal::grid::Dimensions as _; + term.term.lock().grid().columns() + } + + /// Waits for the reader thread to have applied everything named, then + /// returns the whole grid either way so a failure can print what landed. + fn settled(term: &RemoteTerminal, needles: &[&str]) -> String { + for _ in 0..600 { + let text = all_text(term); + if needles.iter().all(|n| text.contains(n)) { + return text; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + all_text(term) + } + + /// The shape a re-attach takes: the client's grid is born at the hardcoded + /// attach size, and the daemon replays every ring segment at the geometry + /// it was recorded at. + #[test] + fn every_replayed_segment_reaches_the_grid() { + crate::core::config::pin_test_config_dir(); + let (client_side, mut daemon) = socket_pair(); + let term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)).unwrap(); + + DaemonMsg::Size(ws(80, 24)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"BIRTH-BANNER\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"SECOND-SEGMENT\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"THIRD-SEGMENT\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + + let text = settled(&term, &["BIRTH-BANNER", "SECOND-SEGMENT", "THIRD-SEGMENT"]); + assert!(text.contains("BIRTH-BANNER"), "grid held:\n{text}"); + assert!(text.contains("SECOND-SEGMENT"), "grid held:\n{text}"); + assert!(text.contains("THIRD-SEGMENT"), "grid held:\n{text}"); + // Each segment's geometry too, or this would pass on a replay that + // dropped every `Size`. The unix-gated + // `segmented_ring_replay_reproduces_live_rendering` asserts that half + // already; nothing did on the platforms this module exists for. + assert_eq!( + columns(&term), + 120, + "the geometry each segment was recorded at never reached the grid, \ + so it is still at the attach size" + ); + drop(daemon); + } + + /// A real pane's ring is megabytes; one `Snapshot` frame is far larger than + /// the reader's 256 KiB read buffer, so the frame is assembled across many + /// reads — several of which time out at `QUIT_POLL` while the sender is + /// still pushing. + #[test] + fn a_snapshot_larger_than_one_read_still_lands_whole() { + crate::core::config::pin_test_config_dir(); + let (client_side, daemon) = socket_pair(); + let term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)).unwrap(); + + let mut bulk = Vec::new(); + bulk.extend_from_slice(b"HEAD-OF-THE-RING\r\n"); + for i in 0..40_000 { + bulk.extend_from_slice(format!("line {i} of the pane's history\r\n").as_bytes()); + } + bulk.extend_from_slice(b"TAIL-OF-THE-RING\r\n"); + + let feeder = std::thread::spawn(move || { + let mut daemon = daemon; + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(bulk).encode(&mut daemon).unwrap(); + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"AFTER-THE-BULK\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + daemon + }); + + let text = settled(&term, &["AFTER-THE-BULK"]); + assert!( + text.contains("AFTER-THE-BULK"), + "the frame after a multi-megabyte snapshot never arrived; grid tail:\n{}", + screen_text(&term) + ); + assert!( + text.contains("TAIL-OF-THE-RING"), + "the snapshot itself was truncated" + ); + drop(feeder.join().unwrap()); + } + + /// The view resizes to its real geometry the first time it paints, which + /// happens while the replay is still arriving. Against a resize-echoing + /// daemon the grid must not reflow when the request goes out — only when + /// the daemon echoes the `Size` back, which is the stream position where + /// the bytes stop being old-width — and neither step may cost the + /// replayed screen. + /// + /// The two geometries are deliberately different: at identical dimensions + /// a reflow and a deferred reflow look the same, so the invariant would be + /// unobservable. + #[test] + fn a_resize_racing_the_replay_keeps_the_replayed_screen() { + crate::core::config::pin_test_config_dir(); + let (client_side, mut daemon) = socket_pair(); + let mut term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)).unwrap(); + term.route = echoing_route(); + + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"REPLAYED-SCREEN\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + let text = settled(&term, &["REPLAYED-SCREEN"]); + assert!(text.contains("REPLAYED-SCREEN"), "grid held:\n{text}"); + + assert_eq!(columns(&term), 120, "the replay set the grid's geometry"); + + // First paint: the pane is 100x30 on screen, not the 120x40 the ring + // was recorded at. The request goes down the link and the grid stays + // where the replay left it. + term.resize(TermSize::new(100, 30), 8, 17); + assert_eq!( + columns(&term), + 120, + "the grid reflowed at request time instead of waiting for the echo" + ); + let _ = daemon.set_read_timeout(Some(std::time::Duration::from_secs(5))); + match ClientMsg::read(&mut daemon) { + Ok(ClientMsg::Resize(size)) => assert_eq!((size.cols, size.rows), (100, 30)), + other => panic!("the resize never reached the daemon: {other:?}"), + } + + // The echo is the stream position the reflow belongs at. + DaemonMsg::Size(ws(100, 30)).encode(&mut daemon).unwrap(); + for _ in 0..600 { + if columns(&term) == 100 { + break; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + let text = all_text(&term); + assert_eq!(columns(&term), 100, "the echo never reflowed the grid"); + assert!( + text.contains("REPLAYED-SCREEN"), + "the first paint's resize cost the replay; grid held:\n{text}" + ); + drop(daemon); + } + + /// The attach handshake reads off the same socket the reader will, far + /// enough to tell an `Error` frame from a replay, and hands what it read + /// on as the reader's starting buffer. A whole replay can already be + /// sitting in the socket when it looks, so the prefix it takes is several + /// frames wide and the split lands mid-frame — every byte of it has to + /// reach the grid. + #[test] + fn the_attach_handshakes_prefix_carries_the_replay_it_swallowed() { + crate::core::config::pin_test_config_dir(); + let (mut client_side, mut daemon) = socket_pair(); + + // The whole replay before the client looks: this is the daemon that + // answered instantly, which is the daemon a local attach meets. + DaemonMsg::Size(ws(80, 24)).encode(&mut daemon).unwrap(); + DaemonMsg::Snapshot(b"BIRTH-BANNER\r\n".to_vec()) + .encode(&mut daemon) + .unwrap(); + DaemonMsg::Size(ws(120, 40)).encode(&mut daemon).unwrap(); + let mut bulk = Vec::new(); + for i in 0..400 { + bulk.extend_from_slice(format!("scrollback row {i}\r\n").as_bytes()); + } + bulk.extend_from_slice(b"LAST-ROW-OF-THE-RING\r\n"); + DaemonMsg::Snapshot(bulk).encode(&mut daemon).unwrap(); + + let buffered = + attach_reply_prefix(&mut client_side, 1, std::time::Duration::from_secs(2)).unwrap(); + assert!( + !buffered.is_empty(), + "the handshake read nothing, so it proves nothing" + ); + let term = + RemoteTerminal::from_stream_with(client_side, TermSize::new(80, 24), buffered).unwrap(); + + let text = settled(&term, &["BIRTH-BANNER", "LAST-ROW-OF-THE-RING"]); + assert!(text.contains("BIRTH-BANNER"), "grid held:\n{text}"); + assert!( + text.contains("LAST-ROW-OF-THE-RING"), + "the replay the handshake swallowed never reached the grid" + ); + drop(daemon); + } + + // ---- The switch, end to end, with a real daemon pane ---------------- + // + // Everything above drives the client half against a scripted daemon. This + // drives the real one: a live `DaemonPane` over a real pty, in this + // process, with its `DaemonMsg` stream forwarded onto a socket pair the + // way `spawn_writer` forwards it, so an attach here is the same attach a + // re-attach makes. It is the switch (#711) minus gpui: attach, resize to + // the geometry the window actually has, produce output, drop the client, + // produce more, attach again — and read the grid the second client ends up + // with. + + /// A client hung off `pane`, the way `stream_pane_with_attach` hangs one + /// off it: subscribe, forward every queued message onto the wire, and read + /// the client's own frames back the way `run_stream` does — epoch guard + /// included, so a displaced client's resize is dropped here exactly as the + /// daemon drops it. + /// + /// The route is a resize-echoing one, which is what the local daemon is: + /// the grid must not reflow until the `Size` the daemon echoes back. + fn attach_client( + pane: &std::sync::Arc, + ) -> (u64, RemoteTerminal, std::thread::JoinHandle<()>) { + let (client_side, daemon_side) = socket_pair(); + let mut daemon_write = daemon_side.try_clone().expect("clone the daemon half"); + let (tx, rx) = std::sync::mpsc::channel::(); + let epoch = pane.attach(tx); + let gate = pane.gate(); + let forward = std::thread::spawn(move || { + while let Ok(msg) = rx.recv() { + let drained = match &msg { + DaemonMsg::Output(b) | DaemonMsg::Image(b) => b.len(), + _ => 0, + }; + let ok = msg.encode(&mut daemon_write).is_ok(); + if drained > 0 { + gate.sub(drained); + } + if !ok { + break; + } + } + }); + { + let pane = pane.clone(); + let mut daemon_read = daemon_side; + std::thread::spawn(move || { + use std::io::Read as _; + let mut pending: Vec = Vec::new(); + let mut chunk = [0u8; 65536]; + loop { + while let Ok(Some((kind, payload))) = + crate::daemon::protocol::take_frame(&mut pending) + { + match ClientMsg::from_frame(kind, payload) { + Ok(ClientMsg::Input(bytes)) if pane.controls(epoch) => { + pane.write_input(&bytes) + } + Ok(ClientMsg::Resize(size)) if pane.controls(epoch) => { + pane.resize(size) + } + Ok(_) => {} + Err(_) => return, + } + } + match daemon_read.read(&mut chunk) { + Ok(0) | Err(_) => return, + Ok(n) => pending.extend_from_slice(&chunk[..n]), + } + } + }); + } + let mut term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)) + .expect("a client over the pair"); + term.route = echoing_route(); + (epoch, term, forward) + } + + /// A route whose daemon echoes `Size` when it applies a resize — which is + /// what `PaneRoute::Local` is against a current daemon, and what the + /// harness above implements. + /// + /// Built the way the neighbouring resize tests build one, rather than + /// through a `PaneWorkspace`: routing a workspace is not what any of these + /// cover, and going that way would have a change to `NativeSshSpec`'s + /// serde shape fail them on an `unwrap` that has nothing to do with + /// replay. + fn echoing_route() -> PaneRoute { + PaneRoute::Remote { + header: Box::new(crate::daemon::router::RouteHeader::wsl("Ubuntu-22.04")), + resize_echo: true, + } + } + + fn wait_for(term: &RemoteTerminal, needle: &str) -> bool { + for _ in 0..600 { + if all_text(term).contains(needle) { + return true; + } + std::thread::sleep(std::time::Duration::from_millis(25)); + } + false + } + + /// Switching workspaces drops every pane and attaches to the same daemon + /// panes again. Whatever the pane put on screen while the window was + /// elsewhere — and whatever it had already — has to come back with it. + #[test] + fn a_pane_re_attached_after_a_switch_gets_its_screen_back() { + crate::core::config::pin_test_config_dir(); + let pane = tty7_core::daemon::pane::DaemonPane::spawn( + 7711, + std::env::current_dir().ok(), + ws(80, 24), + None, + None, + None, + None, + false, + || {}, + ) + .expect("a pty-backed pane"); + + // The window this pane is shown in, attaching for the first time. + let (epoch, mut first, forward) = attach_client(&pane); + // What the first paint does, from the same side it does it on: the + // pane is not 80x24 on screen, so the real geometry goes down the link + // and the grid waits for the daemon to echo it back. + first.resize(TermSize::new(120, 40), 8, 17); + pane.write_input(b"echo BEFORE-THE-SWITCH\r"); + assert!( + wait_for(&first, "BEFORE-THE-SWITCH"), + "the pane never echoed the first command; grid held:\n{}", + all_text(&first) + ); + + // Switching away: the view is dropped, which closes the link, and the + // daemon gives up the seat. + drop(first); + pane.detach(epoch); + drop(forward); + + // The pane keeps working while the window is showing another workspace. + pane.write_input(b"echo DURING-THE-SWITCH\r"); + std::thread::sleep(std::time::Duration::from_millis(600)); + + // Switching back: a brand new view, attaching at the hardcoded size. + // + // It deliberately never resizes. Resizing is the workaround #711's + // reporter found — it makes the daemon `TIOCSWINSZ` the pty and the + // *child* repaint, which would put the screen back whether or not the + // replay ever arrived, and would also feed the grid a `Size` echo that + // `resize_state` sends unconditionally. Everything asserted below has + // to come from the replay itself. + let (_epoch, second, forward) = attach_client(&pane); + let landed = wait_for(&second, "DURING-THE-SWITCH"); + let text = all_text(&second); + let width = columns(&second); + pane.kill(); + drop(forward); + + assert!( + landed, + "the re-attached pane never got the output produced while it was hidden; \ + grid held:\n{text}" + ); + assert!( + text.contains("BEFORE-THE-SWITCH"), + "the re-attached pane lost the screen it had before the switch; grid held:\n{text}" + ); + // Nothing resized this client, so 120 can only have come from the + // `Size` frame the replay sends ahead of the segment recorded at it. + assert_eq!( + width, 120, + "the replay's geometry never reached the grid, so it is still at the attach size" + ); + } + + /// A switch can rebuild a window twice — a second hydration lands while the + /// first one's panes are still draining their replay — so the same pane is + /// attached to twice in quick succession and the window keeps the later + /// view. That view must hold the screen, and it must still be the one the + /// daemon obeys: the displaced attach's teardown must not take the seat + /// with it. + /// + /// The needle is weaker here than in the switch test above, and knowingly + /// so: the pane is attached to throughout, and a ConPTY that owns the whole + /// viewport reprints what is on screen as ordinary live output, so + /// `RACED-OUTPUT` can reach the second view without the replay. What only + /// the replay can supply is the geometry — nothing resizes this client — + /// and what only the seat can supply is `AFTER-THE-RACE`. Those two are the + /// assertions that discriminate. + #[test] + fn the_later_of_two_racing_attaches_keeps_the_screen_and_the_seat() { + crate::core::config::pin_test_config_dir(); + let pane = tty7_core::daemon::pane::DaemonPane::spawn( + 7712, + std::env::current_dir().ok(), + ws(80, 24), + None, + None, + None, + None, + false, + || {}, + ) + .expect("a pty-backed pane"); + + let (first_epoch, mut first, first_forward) = attach_client(&pane); + first.resize(TermSize::new(120, 40), 8, 17); + pane.write_input(b"echo RACED-OUTPUT\r"); + assert!(wait_for(&first, "RACED-OUTPUT"), "the pane never echoed"); + + // The second rebuild attaches before the first one's view is dropped. + let (second_epoch, second, second_forward) = attach_client(&pane); + drop(first); + drop(first_forward); + // The displaced connection detaches on its way out, epoch-guarded. + pane.detach(first_epoch); + + // Never resized, for the reason the switch test is not: a resize would + // repaint the pane from the child and echo a `Size` back, and both are + // exactly what must not be allowed to stand in for the replay. + let landed = wait_for(&second, "RACED-OUTPUT"); + let width = columns(&second); + // Two halves of "the seat survived", and only one of them can catch a + // detach that took it. `controls` answers from `subscriber_epoch` + // alone, and `DaemonPane::detach` clears `subscriber` without ever + // touching that counter — so it says the pane would obey this view's + // input and resizes, but it would keep saying so with the epoch guard + // stripped out of `detach`. `live` is what notices that: a detach that + // dropped the wrong subscriber silences the surviving view. + let controls = pane.controls(second_epoch); + pane.write_input(b"echo AFTER-THE-RACE\r"); + let live = wait_for(&second, "AFTER-THE-RACE"); + let text = all_text(&second); + pane.kill(); + drop(second_forward); + + assert!( + landed, + "the second attach's replay was lost; grid held:\n{text}" + ); + assert_eq!( + width, 120, + "the second attach's replay carried no geometry, so the grid is still at the \ + attach size" + ); + assert!( + controls, + "the pane no longer answers to the surviving view, so nothing it \ + types or resizes would reach the pty" + ); + assert!( + live, + "the displaced attach's detach took the live seat: the surviving \ + view stopped receiving output" + ); + } +} + #[cfg(all(test, windows))] mod windows_tests { use super::*; - fn tcp_pair() -> (std::net::TcpStream, std::net::TcpStream) { - let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let client_side = std::net::TcpStream::connect(addr).unwrap(); - let (daemon_side, _) = listener.accept().unwrap(); - (client_side, daemon_side) - } + use super::replay_tests::socket_pair as tcp_pair; /// On Windows, `shutdown()` does not wake a thread parked in a blocking /// `read` on the same socket (it does on unix). `detach_link`, From 661a8c2ed215ceedb8c5314835ded7a5ca5c7d4a Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:40:03 +0800 Subject: [PATCH 5/7] feat(ui): mark the tab whose pane is zoomed (#782) Refs #752. --- src/ui/app.rs | 101 ++++++++++++++++++++++++++++++++++++++++++ src/ui/i18n/en.rs | 1 + src/ui/i18n/ja.rs | 1 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + src/ui/pane.rs | 29 ++++++++++++ src/ui/tab_sidebar.rs | 16 ++++++- src/ui/tab_strip.rs | 32 +++++++++++++ 8 files changed, 181 insertions(+), 1 deletion(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index 2a1c7ca8..30db9895 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -4195,6 +4195,27 @@ impl Tty7App { } } + /// Whether tab `index` has a pane zoomed over hidden siblings — what the + /// chrome marks so the state is readable without toggling it (#752). + /// + /// Zoom rides with its tab (#599): the active tab's lives in + /// `self.maximized`, every other tab's is parked in `Tab::zoomed`. Either + /// can name a pane that exited while nobody was looking, which is why the + /// answer is asked of the layout rather than of the handle alone. + pub(crate) fn tab_is_zoomed(&self, index: usize) -> bool { + let Some(tab) = self.tabs.get(index) else { + return false; + }; + let zoom = match index == self.active { + true => self.maximized.as_ref(), + false => tab.zoomed.as_ref(), + }; + zoom.is_some_and(|zoom| { + tab.pane + .zoom_hides_siblings(|slot| slot.entity_id() == zoom.entity_id()) + }) + } + fn toggle_maximize(&mut self, window: &mut Window, cx: &mut Context) { if self.maximized.is_some() { self.maximized = None; @@ -10223,6 +10244,86 @@ mod zoom_gpui_tests { ); }); } + + /// The mark the chrome wears (#752) has to go out again by every road the + /// zoom itself leaves by, and it has to name the right tab while several + /// tabs are each holding one. + #[gpui::test] + fn the_zoom_mark_is_on_whichever_tabs_are_hiding_panes(cx: &mut TestAppContext) { + use crate::terminal::view::quiet_test_pane; + use crate::ui::pane::{Pane, PaneSlot}; + + let (app, mut vcx, _streams) = harness_with_tabs(cx, 3); + + app.update_in(&mut vcx, |app, window, cx| { + // A zoom only hides something where there is a sibling to hide, so + // tabs 0 and 1 get a second pane and tab 2 stays single. + let mut held = Vec::new(); + for tab in 0..2 { + let (view, stream) = quiet_test_pane(90 + tab as u64, window, cx); + held.push(stream); + let first = app.tabs[tab].pane.first_leaf().expect("tab has a pane"); + app.tabs[tab].pane = Pane::split_node( + gpui::Axis::Horizontal, + 0.5, + Pane::leaf(first), + Pane::leaf(PaneSlot::Ready(view)), + ); + } + + for i in 0..app.tabs.len() { + assert!(!app.tab_is_zoomed(i), "nothing is zoomed yet"); + } + + // Zooming marks the tab it happened in, and only that one. + app.toggle_maximize(window, cx); + assert!(app.tab_is_zoomed(0), "the zoomed tab wears the mark"); + assert!(!app.tab_is_zoomed(1)); + assert!(!app.tab_is_zoomed(2)); + + // And un-zooming takes it away again. + app.toggle_maximize(window, cx); + assert!(!app.tab_is_zoomed(0), "un-zooming clears the mark"); + + // The mark rides with its tab across a switch (#599) — an inactive + // tab holding a zoom still wears it — and two tabs can wear one at + // the same time, each reading its own handle. + app.toggle_maximize(window, cx); + app.activate(1, window, cx); + assert!(app.tab_is_zoomed(0), "the parked zoom is still a zoom"); + assert!(!app.tab_is_zoomed(1)); + app.toggle_maximize(window, cx); + assert!(app.tab_is_zoomed(0) && app.tab_is_zoomed(1)); + assert!(!app.tab_is_zoomed(2), "a single-pane tab hides nothing"); + + // A parked zoom naming a pane that has since left the tab is no + // zoom: it would not come back on a switch, so it is not marked. + let parked = app.tabs[0].zoomed.clone().expect("tab 0 parked a zoom"); + let elsewhere = app.tabs[2] + .pane + .first_leaf() + .and_then(|slot| slot.terminal().cloned()); + app.tabs[0].zoomed = elsewhere; + assert!( + !app.tab_is_zoomed(0), + "a zoom over a pane this tab does not hold is not marked" + ); + app.tabs[0].zoomed = Some(parked.clone()); + assert!(app.tab_is_zoomed(0)); + + // Nor is a zoom over the last pane standing: its siblings closed + // while the tab was away, and the tab now looks like — and draws + // as — an ordinary single pane. + app.tabs[0].pane = Pane::leaf(PaneSlot::Ready(parked)); + assert!( + !app.tab_is_zoomed(0), + "a zoom that covers nothing stops being marked" + ); + + assert!(!app.tab_is_zoomed(9), "there is no tab 9 to mark"); + drop(held); + }); + } } // A test window has no daemon behind it — its socket path is under the pinned diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 4bc309b2..29725aec 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1776,6 +1776,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::TabTooltipHideSidebar => "Hide Sidebar", L10nKey::TabTooltipHideDetailPanel => "Hide Detail Panel", L10nKey::TabTooltipShowDetailPanel => "Show Detail Panel", + L10nKey::TabTooltipZoomed => "Pane zoomed — other panes hidden", L10nKey::TabMenuLocalShells => "Local", L10nKey::TabMenuAddHost => "Add SSH Host…", L10nKey::TabMenuAllHosts => "All SSH Hosts…", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 3a61cd94..3dee3ef7 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1847,6 +1847,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::TabTooltipHideSidebar => "サイドバーを非表示", L10nKey::TabTooltipHideDetailPanel => "詳細パネルを非表示", L10nKey::TabTooltipShowDetailPanel => "詳細パネルを表示", + L10nKey::TabTooltipZoomed => "ペインを拡大中 — 他のペインは非表示", L10nKey::TabMenuLocalShells => "ローカル", L10nKey::TabMenuAddHost => "SSH ホストを追加…", L10nKey::TabMenuAllHosts => "すべての SSH ホスト…", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 9464f67f..4d80197f 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -998,6 +998,7 @@ l10n_keys! { TabTooltipHideSidebar, TabTooltipHideDetailPanel, TabTooltipShowDetailPanel, + TabTooltipZoomed, TabMenuLocalShells, TabMenuAddHost, TabMenuAllHosts, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 3c6e6b27..66512ae6 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1686,6 +1686,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::TabTooltipHideSidebar => "隐藏侧栏", L10nKey::TabTooltipHideDetailPanel => "隐藏详情面板", L10nKey::TabTooltipShowDetailPanel => "显示详情面板", + L10nKey::TabTooltipZoomed => "窗格已缩放 — 其他窗格已隐藏", L10nKey::TabMenuLocalShells => "本地", L10nKey::TabMenuAddHost => "添加 SSH 主机…", L10nKey::TabMenuAllHosts => "所有 SSH 主机…", diff --git a/src/ui/pane.rs b/src/ui/pane.rs index 497570b1..34778762 100644 --- a/src/ui/pane.rs +++ b/src/ui/pane.rs @@ -183,6 +183,17 @@ impl Pane { } } + /// Whether zooming the leaf `pred` names would actually hide anything. + /// + /// The zoom that gets marked in the chrome is the one a reader cannot see + /// for themselves: a zoom naming a pane that has since exited names no + /// leaf here, and a zoom over the last pane standing covers nothing. Both + /// look exactly like an unzoomed single pane, so neither earns a badge. + pub fn zoom_hides_siblings(&self, pred: impl Fn(&L) -> bool) -> bool { + let leaves = self.leaves(); + leaves.len() > 1 && leaves.iter().any(pred) + } + pub fn leaf_matching_or_first(&self, pred: impl Fn(&L) -> bool) -> Option { self.leaves() .into_iter() @@ -1279,6 +1290,24 @@ mod tests { assert_eq!(TestPane::Empty.leaf_matching_or_first(is(0)), None); } + #[test] + fn a_zoom_is_only_worth_marking_while_it_covers_something() { + let mut pane = TestPane::leaf(0); + assert!( + !pane.zoom_hides_siblings(is(0)), + "zooming the only pane covers nothing" + ); + + split(&mut pane, 0, Axis::Horizontal, 1); + assert!(pane.zoom_hides_siblings(is(0))); + assert!(pane.zoom_hides_siblings(is(1))); + assert!( + !pane.zoom_hides_siblings(is(99)), + "a zoom whose pane has exited is not a zoom" + ); + assert!(!TestPane::Empty.zoom_hides_siblings(is(0))); + } + #[test] fn closing_the_root_leaf_defers_removal_to_the_caller() { let mut pane = TestPane::leaf(7); diff --git a/src/ui/tab_sidebar.rs b/src/ui/tab_sidebar.rs index cb093257..bec9a8ee 100644 --- a/src/ui/tab_sidebar.rs +++ b/src/ui/tab_sidebar.rs @@ -47,6 +47,8 @@ mod row_metrics { pub(super) const GAP: f32 = 8.; /// The ⌘N badge, when one is shown. pub(super) const BADGE: f32 = 20.; + /// The zoom mark, when the tab has a pane zoomed over the others. + pub(super) const ZOOM: f32 = 16.; /// `gap_1p5`, between the branch icon and its text and before the counts. pub(super) const META_GAP: f32 = 6.; /// The branch icon. @@ -296,9 +298,16 @@ impl Tty7App { } else { 0. }; + let zoomed = self.tab_is_zoomed(i); + let zoom_extra = if zoomed { + row_metrics::ZOOM + row_metrics::GAP + } else { + 0. + }; // Elision is measured against this budget so the label and // branch never wrap or overflow into CSS truncation. - let label_avail = (row_metrics::text_budget(width) - badge_extra).max(48.); + let label_avail = + (row_metrics::text_budget(width) - badge_extra - zoom_extra).max(48.); let title_size = 0.875 * rem; let meta_size = 0.75 * rem; let title_font = if is_active { &title_font_active } else { &font }; @@ -738,6 +747,11 @@ impl Tty7App { 22., cx, )) + // Leading, like the chip's: the trailing end of a row is + // the badge's, and the close button fades in over it. + .when(zoomed, |row| { + row.child(self.zoom_mark(("sidebar-zoom", i), cx)) + }) .child(label_region) .when(show_badges && badge_pos < 9, |row| { row.child( diff --git a/src/ui/tab_strip.rs b/src/ui/tab_strip.rs index 3ba13fcb..9ba70898 100644 --- a/src/ui/tab_strip.rs +++ b/src/ui/tab_strip.rs @@ -1310,6 +1310,30 @@ impl Tty7App { } } + /// The mark a tab wears while one of its panes is zoomed over the others + /// (#752). Without it a zoomed tab is pixel-for-pixel a tab that only ever + /// had one pane, and the only way to tell was to toggle the zoom off. + /// + /// Drawn in the tab entry rather than on the pane so it reads from either + /// tab surface, and so it says something about the tabs you are *not* + /// looking at — the zoom outlives a switch away from them. + pub(crate) fn zoom_mark(&self, id: impl Into, cx: &App) -> gpui::AnyElement { + let tip = chord_hint(t(L10nKey::TabTooltipZoomed), "ToggleMaximizePane", cx); + div() + .id(id) + .flex_shrink_0() + .flex() + .items_center() + .justify_center() + .size(px(16.)) + .text_color(cx.theme().muted_foreground) + .child(Icon::new(IconName::Maximize).size(px(11.))) + .tooltip(move |window, cx| { + gpui_component::tooltip::Tooltip::new(tip.clone()).build(window, cx) + }) + .into_any_element() + } + /// The full title behind a shortened one, for the row to name on hover. /// /// `tab_label` hands back a path elided to its last three segments and then @@ -1705,6 +1729,7 @@ impl Tty7App { let agent = tab.agent(cx); let agent_status = tab.agent_status(cx); let agent_unread = tab.agent_unread_count(cx); + let zoomed = self.tab_is_zoomed(i); let rename_input = self .renaming @@ -1832,6 +1857,13 @@ impl Tty7App { cx, )) }) + // Leading, beside the other state marks: the trailing end of a + // chip belongs to the badge and to the close button that fades + // in over it, and a mark parked there would vanish under the + // pointer that came to read it. + .when(zoomed, |chip| { + chip.child(self.zoom_mark(("tab-zoom", i), cx)) + }) .child(label_region) .when(show_badges && i < 9, |chip| { chip.child( From 92c1ae9f26ac947b4d23c51e6ba6afdc020e706a Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:45:17 +0800 Subject: [PATCH 6/7] feat(ui): make opening a new window a bindable action (#793) Refs #710; does not close it (the request was a jump list opening a chosen existing workspace). Registers NewWindow globally as well as on the render root, since with the tray icon on (the default) closing the last window retires to the tray and leaves no window to dispatch it. --- docs/reference/keyboard-shortcuts.mdx | 1 + src/core/actions.rs | 1 + src/ui/app.rs | 137 ++++++++++++++++++++++++++ src/ui/i18n/en.rs | 1 + src/ui/i18n/ja.rs | 1 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + src/ui/keymap.rs | 77 +++++++++++++++ src/ui/palette.rs | 4 + 9 files changed, 224 insertions(+) diff --git a/docs/reference/keyboard-shortcuts.mdx b/docs/reference/keyboard-shortcuts.mdx index 81314b1a..a526c78c 100644 --- a/docs/reference/keyboard-shortcuts.mdx +++ b/docs/reference/keyboard-shortcuts.mdx @@ -17,6 +17,7 @@ it shows what *your* copy is bound to. This is the shipped default. | Go to Tab 1–9 | ⌘ 1⌘ 9 | Alt 1Alt 9 | | New Workspace | ⌘ ⇧ N | Ctrl ⇧ N | | Switch Workspace | ⌘ ⇧ O | Ctrl ⇧ O | +| New Window | ⌘ N | — | ## Panes diff --git a/src/core/actions.rs b/src/core/actions.rs index 30788b42..7f9a551d 100644 --- a/src/core/actions.rs +++ b/src/core/actions.rs @@ -18,6 +18,7 @@ actions!( SelectWorkspace7, SelectWorkspace8, SelectWorkspace9, + NewWindow, CloseWindow, CloseActiveTab, RenameTab, diff --git a/src/ui/app.rs b/src/ui/app.rs index 30db9895..27ffc2a2 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1508,6 +1508,22 @@ impl Tty7App { crate::ui::windows::refresh_menu(cx); } + /// Opens a second window, on a workspace of its own. + /// + /// A window on *this* workspace is not the other reading of "new window"; + /// it is a thing the app cannot hold. `WindowRegistry` is keyed by + /// workspace — `window_for`, `app_for`, `unregister` and `rebind` all + /// address a window by the workspace it shows — and `windows::open` + /// answers a workspace that already has a window by activating it. Asking + /// for the current one here would raise the window you are already in. + /// + /// So this is the same call the switcher makes for "Open in New Window", + /// with no workspace named: a fresh one, which is also what a new window + /// holds everywhere else it is offered. + pub(crate) fn new_window(&self, cx: &mut App) { + crate::ui::windows::open(cx, None); + } + fn prepare_window_close(&self, cx: &mut App) { let last_window = crate::ui::windows::WindowRegistry::count(cx) <= 1; self.detach_workspace(cx); @@ -4983,6 +4999,7 @@ impl Tty7App { match kind { NewTab => self.new_tab(window, cx), NewWorkspace => self.open_workspace_form(window, cx), + NewWindow => self.new_window(cx), OpenWorkspacePicker => self.open_switcher(window, cx), StopWorkspace => self.stop_workspace(self.workspace, window, cx), DeleteWorkspace => self.delete_workspace(self.workspace, window, cx), @@ -7483,6 +7500,9 @@ impl Render for Tty7App { .on_action(cx.listener(|this, _: &NewWorkspace, window, cx| { this.open_workspace_form(window, cx); })) + .on_action(cx.listener(|this, _: &NewWindow, _window, cx| { + this.new_window(cx); + })) .on_action( cx.listener(|this, _: &CloseWindow, window, cx| this.close_window(window, cx)), ) @@ -10420,6 +10440,123 @@ mod managed_forward_gpui_tests { } } +#[cfg(test)] +mod new_window_action_tests { + use crate::core::actions::NewWindow; + use crate::core::config::Config; + use crate::core::session::Session; + use crate::ui::app::Tty7App; + use crate::ui::windows::WindowRegistry; + use gpui::{AppContext as _, TestAppContext, VisualTestContext}; + + /// `NewWindow` has to open a window, not merely exist. + /// + /// Everything else about the action is a table entry — the `actions!` + /// row, the keymap slot, the palette command — and every one of those can + /// be there while the action reaches nothing. This drives the real + /// dispatch path and then asks the registry, so the assertion is "a second + /// window is open, on a workspace of its own, and the first one is still + /// here": the same `windows::open` the switcher calls for "Open in New + /// Window", with no workspace named. + #[gpui::test] + fn dispatching_new_window_opens_a_second_window_beside_the_first(cx: &mut TestAppContext) { + crate::core::config::pin_test_config_dir(); + cx.executor().allow_parking(); + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(Config::default()); + crate::ui::keymap::init(cx); + WindowRegistry::init(cx); + }); + let window = cx.add_window(|window, cx| { + let app = + cx.new(|cx| Tty7App::with_session(None, Some(Session::default()), window, cx)); + gpui_component::Root::new(app, window, cx) + }); + let app = window + .update(cx, |root, _, _| { + root.view() + .clone() + .downcast::() + .ok() + .expect("window root wraps a Tty7App") + }) + .unwrap(); + // Registered the way an opened window registers itself; without it the + // registry cannot tell the two windows apart afterwards. + let handle = window.into(); + let weak = app.downgrade(); + app.update(cx, |app, cx| { + WindowRegistry::register(cx, app.workspace, handle, weak); + }); + + let mut vcx = VisualTestContext::from_window(handle, cx); + vcx.background_executor.run_until_parked(); + let first = app.update(&mut vcx, |app, _| app.workspace); + assert_eq!( + vcx.update(|_, cx| WindowRegistry::count(cx)), + 1, + "the harness starts with exactly the one window" + ); + + vcx.dispatch_action(NewWindow); + vcx.background_executor.run_until_parked(); + + let open = vcx.update(|_, cx| WindowRegistry::open_windows(cx)); + assert_eq!( + open.len(), + 2, + "NewWindow has to reach windows::open; it opened {} window(s)", + open.len() + ); + assert!( + open.iter().any(|(id, _)| *id == first), + "the window the action was fired from must survive it" + ); + // The registry is keyed by workspace, so a second window on the + // current one is not a thing it could tell apart from the first. + assert!( + open.iter().any(|(id, _)| *id != first), + "the new window belongs on a workspace of its own" + ); + } + + /// The windowless state is the one `NewWindow` exists for. + /// + /// `show_tray_icon` is on by default, so closing the last window retires + /// tty7 to the tray rather than quitting it: the process is alive, the + /// menu bar is still tty7's, and there is nothing on screen. A listener + /// that lives only on `Tty7App`'s render root reaches nothing there, and + /// the chord that means "give me a window" is the one chord that has to + /// answer. `App::dispatch_action` falls through to the global listeners + /// when no window is active, which is where `keymap::init` puts this one. + #[gpui::test] + fn new_window_answers_with_no_window_to_dispatch_it(cx: &mut TestAppContext) { + crate::core::config::pin_test_config_dir(); + cx.executor().allow_parking(); + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(Config::default()); + crate::ui::keymap::init(cx); + WindowRegistry::init(cx); + assert_eq!( + WindowRegistry::count(cx), + 0, + "the retired-to-tray state this covers has no window in it" + ); + cx.dispatch_action(&NewWindow); + }); + cx.run_until_parked(); + cx.update(|cx| { + assert_eq!( + WindowRegistry::count(cx), + 1, + "NewWindow has to reach windows::open with no window to bubble through" + ); + }); + } +} + #[cfg(test)] mod close_window_action_tests { use crate::core::actions::CloseWindow; diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 29725aec..fea7d64f 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1405,6 +1405,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::CmdGroupAgents => "Agents", L10nKey::CmdGroupApplication => "Application", L10nKey::CmdNewTab => "New Tab", + L10nKey::CmdNewWindow => "New Window", L10nKey::CmdNewWorktreeTab => "New Worktree Tab…", L10nKey::CmdNewWorktreeTabSubtitle => "isolated checkout on a fresh branch", L10nKey::CmdRenameTab => "Rename Tab…", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 3dee3ef7..50a29245 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1462,6 +1462,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::CmdGroupAgents => "エージェント", L10nKey::CmdGroupApplication => "アプリケーション", L10nKey::CmdNewTab => "新しいタブ", + L10nKey::CmdNewWindow => "新しいウィンドウ", L10nKey::CmdNewWorktreeTab => "新しいワークツリータブ…", L10nKey::CmdNewWorktreeTabSubtitle => "新しいブランチでの独立したチェックアウト", L10nKey::CmdRenameTab => "タブの名前を変更…", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 4d80197f..c7bd1d96 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -1125,6 +1125,7 @@ l10n_keys! { CmdGroupAgents, CmdGroupApplication, CmdNewTab, + CmdNewWindow, CmdNewWorktreeTab, CmdNewWorktreeTabSubtitle, CmdRenameTab, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 66512ae6..9ea57025 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1323,6 +1323,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::CmdGroupAgents => "Agents", L10nKey::CmdGroupApplication => "应用", L10nKey::CmdNewTab => "新标签页", + L10nKey::CmdNewWindow => "新建窗口", L10nKey::CmdNewWorktreeTab => "新建 worktree 标签页…", L10nKey::CmdNewWorktreeTabSubtitle => "在全新分支上独立检出", L10nKey::CmdRenameTab => "重命名标签页…", diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 7c175e57..e7f0015c 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -28,6 +28,16 @@ pub fn init(cx: &mut App) { } rebuild_keymap(cx); cx.on_action(|_: &Quit, cx: &mut App| cx.quit()); + // On the app rather than on a window, for the same reason `Quit` is: this + // is the one action in the table whose whole point is the state where no + // window is there to dispatch it. `show_tray_icon` is on by default, so + // closing the last window retires tty7 to the tray instead of quitting — + // process alive, nothing on screen — and a `NewWindow` that only exists on + // a window's render root is dead in exactly the state a New Window chord + // is for. `Tty7App`'s own listener still wins wherever there is a window: + // gpui runs the window's bubble phase first and returns before the global + // one, so the two never both fire. + cx.on_action(|_: &NewWindow, cx: &mut App| crate::ui::windows::open(cx, None)); set_menus(cx); } @@ -265,6 +275,16 @@ pub(crate) fn default_bindings() -> Vec<(&'static str, &'static str)> { vec![ ("NewTab", per_platform("secondary-t", "secondary-shift-t")), ("NewWorkspace", "secondary-shift-n"), + // Cmd+N is what "New Window" means on macOS, and nothing else here + // claims it. Off macOS both chords the convention offers are gone: + // Ctrl+N is a C0 byte the shell is owed, which + // `no_default_binding_sits_on_a_terminal_control_code` fails a build + // over, and Ctrl+Shift+N — where that same test says window actions + // belong — has been `NewWorkspace` for far longer than this action has + // existed. Minting an unguessable third chord would be worse than + // shipping unbound: the palette and the Keybindings page both carry + // this, so a key is one line of config away. + ("NewWindow", per_platform("secondary-n", "")), ("CloseWindow", ""), ( "CloseActiveTab", @@ -734,6 +754,10 @@ fn authored_entry(action: &str) -> Option<(CommandGroup, String)> { CommandGroup::Application, t(L10nKey::AppMenuCommandPalette).to_string(), ), + "NewWindow" => ( + CommandGroup::Application, + t(L10nKey::CmdNewWindow).to_string(), + ), "CloseWindow" => ( CommandGroup::Application, t(L10nKey::CmdCloseWindow).to_string(), @@ -1041,6 +1065,7 @@ fn make_binding(action: &str, keystroke: &str) -> Option { "DeleteWorkspace" => KeyBinding::new(keystroke, DeleteWorkspace, None), "RenameWorkspace" => KeyBinding::new(keystroke, RenameWorkspace, None), "ToggleSwitcher" => KeyBinding::new(keystroke, ToggleSwitcher, None), + "NewWindow" => KeyBinding::new(keystroke, NewWindow, None), "CloseWindow" => KeyBinding::new(keystroke, CloseWindow, None), "CloseActiveTab" => KeyBinding::new(keystroke, CloseActiveTab, None), "RenameTab" => KeyBinding::new(keystroke, RenameTab, None), @@ -1223,6 +1248,7 @@ mod tests { // palette and the docs all say Zoom Pane. assert_eq!(action_entry("ToggleMaximizePane").1, "Zoom Pane"); assert_eq!(action_entry("CloseActiveTab").1, "Close Pane / Tab"); + assert_eq!(action_entry("NewWindow").1, "New Window"); assert_eq!(action_entry("CloseWindow").1, "Close Window"); assert_eq!(action_entry("ClearScrollback").1, "Clear Scrollback"); assert_eq!(action_entry("TogglePalette").1, "Command Palette…"); @@ -1237,6 +1263,57 @@ mod tests { assert_eq!(action_entry("ForkAgentSessionUp").0, CommandGroup::Agents); } + #[test] + fn new_window_ships_a_chord_only_where_one_is_free() { + let mut effective: Vec<(String, String)> = default_bindings() + .into_iter() + .map(|(a, k)| (a.to_string(), k.to_string())) + .collect(); + let default = effective + .iter() + .find(|(action, _)| action == "NewWindow") + .map(|(_, key)| key.clone()) + .expect("NewWindow has to be listed here or it cannot be bound at all"); + assert_eq!( + default, + if cfg!(target_os = "macos") { + "secondary-n" + } else { + "" + }, + "macOS gets Cmd+N; off macOS this ships unbound on purpose" + ); + + // Ask the keymap rather than the table. A default can look bound and + // dispatch nothing — gpui folds shift into the punctuation glyph, so + // `secondary-shift-]` reached no action at all off macOS (#750). An + // exact match is also the conflict check: any other action holding + // this chord would show up in the list. + if !default.is_empty() { + assert_eq!( + dispatched(&effective, &default, "Terminal"), + vec![NewWindow::name_for_type()], + "{default} must reach NewWindow, and nothing else may answer it" + ); + } + + // Unbound still has to mean bindable. `set_binding` only writes into + // slots this table already has, and `make_binding` is what turns the + // name back into a dispatchable binding; miss either and the action + // sits on the Keybindings page, takes a key, and does nothing — which + // is the whole complaint in #710, not just the missing default. + effective + .iter_mut() + .find(|(action, _)| action == "NewWindow") + .expect("found once already") + .1 = "ctrl-alt-shift-n".to_string(); + assert_eq!( + dispatched(&effective, "ctrl-alt-shift-n", "Terminal"), + vec![NewWindow::name_for_type()], + "a chord the user assigns to NewWindow has to reach it" + ); + } + #[cfg(target_os = "macos")] const SECONDARY: &str = "⌘"; #[cfg(not(target_os = "macos"))] diff --git a/src/ui/palette.rs b/src/ui/palette.rs index fd749760..3d05371f 100644 --- a/src/ui/palette.rs +++ b/src/ui/palette.rs @@ -22,6 +22,7 @@ pub enum CommandKind { RenameWorkspace, StopWorkspace, DeleteWorkspace, + NewWindow, CloseWindow, SplitRight, SplitDown, @@ -125,6 +126,7 @@ impl CommandKind { RenameWorkspace => "rename-workspace", StopWorkspace => "stop-workspace", DeleteWorkspace => "delete-workspace", + NewWindow => "new-window", CloseWindow => "close-window", SplitRight => "split-right", SplitDown => "split-down", @@ -232,6 +234,7 @@ impl CommandKind { RenameWorkspace => "RenameWorkspace", StopWorkspace => "StopWorkspace", DeleteWorkspace => "DeleteWorkspace", + NewWindow => "NewWindow", CloseWindow => "CloseWindow", SplitRight => "SplitRight", SplitDown => "SplitDown", @@ -568,6 +571,7 @@ impl Command { ]; let application = [ + Command::localized(L10nKey::CmdNewWindow, NewWindow), Command::localized(L10nKey::CmdSettings, OpenSettings), Command::localized(L10nKey::CmdKeyboardShortcuts, ShowKeyboardShortcuts), Command::localized(L10nKey::CmdAboutTty7, About), From e3630eb8a1b13ccf22c6f90a303ca8bce9ed8856 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:47:33 +0800 Subject: [PATCH 7/7] fix(keymap): fold a shift-punctuation chord into the key the platform reports (#784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs #750; only half fixed — the US SHIFTED_GLYPHS table means secondary-shift-]/[ stay unpressable on German/French/Nordic layouts. A layout-correct fold needs KeyBinding::load with the real keyboard mapper. Also runs the control-code guard over the folded spelling, so ctrl-shift-2 no longer installs ctrl-@ (NUL) beside it. --- src/ui/app.rs | 7 +- src/ui/keymap.rs | 419 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 391 insertions(+), 35 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index 27ffc2a2..08365c3d 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6768,10 +6768,15 @@ impl Tty7App { } fn assign_keybinding(&mut self, action: String, spec: String, cx: &mut Context) { + // Compared as chords, not as spellings: a recorded `secondary-}` and a + // config's `secondary-shift-]` are one keystroke written two ways, and + // only `same_chord` sees it. Compared as text, the displacement never + // fires and both bindings survive onto that keystroke, where which one + // wins is arbitrary (#750). let displaced = crate::ui::keymap::effective_bindings(cx) .into_iter() .chain(crate::ui::keymap::extra_bindings(cx)) - .find(|(a, k)| *k == spec && *a != action) + .find(|(a, k)| *a != action && crate::ui::keymap::same_chord(k, &spec)) .map(|(a, _)| a); // A trailing "…" on an action name marks a command that opens // something; it is not punctuation, and inside a sentence it reads as diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index e7f0015c..615b63e0 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -173,12 +173,31 @@ pub(crate) fn extra_bindings(cx: &App) -> Vec<(String, String)> { .collect() } +/// Installs `key` for `action`, and alongside it the chord the platform will +/// actually deliver when the two are spelled differently — `secondary-shift-]` +/// is pressed as `secondary-}`, and only the second ever reaches the +/// dispatcher. Both go in rather than one replacing the other, so a spec that +/// was already live stays live whatever the backend does with Shift (#750). +/// +/// Answers whether the action was known at all, which is the caller's cue to +/// warn about a name it cannot bind. +fn push_binding(bindings: &mut Vec, action: &str, key: &str) -> bool { + let Some(binding) = make_binding(action, key) else { + return false; + }; + bindings.push(binding); + if let Some(folded) = folded_spec(key) + && let Some(alias) = make_binding(action, &folded) + { + bindings.push(alias); + } + true +} + fn action_bindings(effective: &[(String, String)]) -> Vec { let mut bindings = Vec::new(); for (action, key) in extra_keystrokes(effective) { - if let Some(b) = make_binding(action, key) { - bindings.push(b); - } + push_binding(&mut bindings, action, key); } for (action, key) in effective { if key.is_empty() { @@ -194,26 +213,36 @@ fn action_bindings(effective: &[(String, String)]) -> Vec { // without saying so — `no_default_binding_sits_on_a_terminal_control_code` // is the half of it that fails a build. A single chord only, since a // prefix like `ctrl-b n` is that choice made deliberately. - if !key.contains(' ') - && steals_a_control_code(key) - && !control_code_binding_allowed(action, key) + // Over both spellings that reach the keymap, because the fold is what + // decides which of them the shell actually loses. `steals_a_control_code` + // only ever recognises a bare Ctrl, so it waves `ctrl-shift-2` through — + // and then the fold installs `ctrl-@` beside it, which is NUL. The chord + // was dead before it was folded and stole nothing; now that it is live + // the warning has to follow it (#750). At most one of the two can trip: + // a written chord that folds carries Shift, and one that carries Shift + // is never a control code. + let folded = folded_spec(key); + for chord in [Some(key.as_str()), folded.as_deref()] + .into_iter() + .flatten() { - log::warn!( - "keybinding '{key}' for '{action}' takes a control code away from the shell" - ); - } - match make_binding(action, key) { - Some(b) => { - bindings.push(b); - if is_default_insert_newline_binding(action, key) { - bindings.push(KeyBinding::new( - INSERT_NEWLINE_DEFAULT, - InsertNewlineFallback, - Some("Terminal"), - )); - } + if !chord.contains(' ') + && steals_a_control_code(chord) + && !control_code_binding_allowed(action, chord) + { + log::warn!( + "keybinding '{chord}' for '{action}' takes a control code away from the shell" + ); } - None => log::warn!("ignoring keybinding: unknown action '{action}'"), + } + if !push_binding(&mut bindings, action, key) { + log::warn!("ignoring keybinding: unknown action '{action}'"); + } else if is_default_insert_newline_binding(action, key) { + bindings.push(KeyBinding::new( + INSERT_NEWLINE_DEFAULT, + InsertNewlineFallback, + Some("Terminal"), + )); } } bindings @@ -964,6 +993,126 @@ pub(crate) fn spec_from_keystroke(ks: &Keystroke) -> Option { Some(spec) } +/// The glyph a US keyboard prints on the shifted half of every key that is not +/// a letter — `shift-]` types `}`, `shift-2` types `@`. +const SHIFTED_GLYPHS: [(char, char); 21] = [ + ('`', '~'), + ('1', '!'), + ('2', '@'), + ('3', '#'), + ('4', '$'), + ('5', '%'), + ('6', '^'), + ('7', '&'), + ('8', '*'), + ('9', '('), + ('0', ')'), + ('-', '_'), + ('=', '+'), + ('[', '{'), + (']', '}'), + ('\\', '|'), + (';', ':'), + ('\'', '"'), + (',', '<'), + ('.', '>'), + ('/', '?'), +]; + +/// Rewrites one chord into the shape the platform actually delivers, or +/// returns `None` when it is already that shape. +/// +/// Every gpui backend carries Shift as a modifier for letters only. Over the +/// digits and the punctuation it spends the modifier on the character instead +/// and clears the flag: that is the `chars_with_shift` branch on macOS, +/// `need_to_convert_to_shifted_key` in the Windows mapper, and Linux's "we +/// only include the shift for upper-case letters by convention". So the chord +/// written `secondary-shift-]` arrives as `secondary-}` and the two never +/// meet — the binding installs and is unreachable, which is why Ctrl+Shift+] +/// stopped cycling panes off macOS and why hand-editing a config to that +/// spelling changed nothing (#750). +/// +/// Folding the spec the same way on the lookup side closes the round trip. +/// The recorder already writes `secondary-}`, because that is what +/// `spec_from_keystroke` was handed; a default or a config written the other +/// way now installs the very same binding, so both spellings keep working and +/// nobody's config comes undone. `secondary-shift-}`, which people also reach +/// for, folds onto it too — the flag is simply redundant there. +/// +/// The table is US-layout, and a chord it does not know is left alone rather +/// than guessed at. It cannot be layout-aware from here: `?` is Shift+ß on a +/// German keyboard and Shift+, on a French one, and the only mapper that knows +/// which is gpui's, reached through `KeyBinding::load` rather than the +/// `KeyBinding::new` that `make_binding` uses — and it exists on Windows only, +/// where `MacKeyboardMapper` does no shift folding at all. So off a US layout +/// the fold is a widening that may not land: `ctrl-shift-4` picks up `ctrl-$`, +/// which on AZERTY is a key of its own. Harmless, because the fold only ever +/// adds — the spec as written stays bound whatever the layout — but it is why +/// this is not the last word on #750. +/// +/// The fold is *added* to a spec's bindings, never substituted for them, +/// because the convention is not universal. Windows leaves Shift intact on +/// the numpad's `/ * + -`: `need_to_convert_to_shifted_key` lists the OEM +/// keys and `VK_0..VK_9` but none of `VK_DIVIDE`/`VK_MULTIPLY`/`VK_ADD`/ +/// `VK_SUBTRACT`, so `get_keystroke_key` falls through to `get_key_from_vkey` +/// and Ctrl+Shift+numpad-/ really does arrive as `ctrl-shift-/`. Substituting +/// `ctrl-?` for it would unbind a chord that works today — the same class of +/// bug this is fixing. macOS clears the flag there anyway (the numpad glyph +/// is the same shifted or not, so `chars_with_shift` takes the branch that +/// drops it) and Linux names those keys `divide`/`multiply`/`add`/`subtract`, +/// which are not one character and never reach this. +fn fold_shift_into_glyph(chord: &str) -> Option { + let mut ks = Keystroke::parse(chord).ok()?; + if !ks.modifiers.shift { + return None; + } + let mut chars = ks.key.chars(); + let (Some(key), None) = (chars.next(), chars.next()) else { + return None; + }; + if let Some((_, shifted)) = SHIFTED_GLYPHS.iter().find(|(plain, _)| *plain == key) { + ks.key = shifted.to_string(); + } else if !SHIFTED_GLYPHS.iter().any(|(_, shifted)| *shifted == key) { + return None; + } + ks.modifiers.shift = false; + spec_from_keystroke(&ks) +} + +/// `spec` with every chord folded, or `None` when it is already the shape the +/// platform delivers and there is nothing to add. +pub(crate) fn folded_spec(spec: &str) -> Option { + let mut folded = String::with_capacity(spec.len()); + let mut changed = false; + for chord in spec.split_whitespace() { + if !folded.is_empty() { + folded.push(' '); + } + match fold_shift_into_glyph(chord) { + Some(chord) => { + folded.push_str(&chord); + changed = true; + } + None => folded.push_str(chord), + } + } + changed.then_some(folded) +} + +/// Whether two specs claim the same keystroke — spelled the same way, or one +/// the fold of the other. `secondary-shift-]` and `secondary-}` are one chord +/// written twice, and a rebind that cannot see that leaves both installed on +/// it, where which one fires is arbitrary (#750). +pub(crate) fn same_chord(a: &str, b: &str) -> bool { + if a == b { + return true; + } + let (folded_a, folded_b) = (folded_spec(a), folded_spec(b)); + folded_a.as_deref() == Some(b) + || folded_b.as_deref() == Some(a) + || (folded_a.is_some() && folded_a == folded_b) +} + pub(crate) fn key_chords(spec: &str) -> Vec> { spec.split_whitespace().map(key_tokens).collect() } @@ -1189,21 +1338,49 @@ mod tests { /// table, so a chord the app would not really install, or would install in /// another context, cannot pass. fn dispatched(effective: &[(String, String)], keys: &str, context: &str) -> Vec<&'static str> { - let mut keymap = gpui::Keymap::default(); - keymap.add_bindings(action_bindings(effective)); let input: Vec = keys .split(' ') .map(|k| Keystroke::parse(k).expect("the typed keystroke parses")) .collect(); + dispatched_keystrokes(effective, &input, context) + } + + /// The same lookup for input a spec cannot spell — the shifted-glyph shape + /// a backend hands over for `secondary-shift-]` has no written form that + /// `Keystroke::parse` turns back into it. + fn dispatched_keystrokes( + effective: &[(String, String)], + input: &[Keystroke], + context: &str, + ) -> Vec<&'static str> { + let mut keymap = gpui::Keymap::default(); + keymap.add_bindings(action_bindings(effective)); let context = [gpui::KeyContext::parse(context).expect("the context parses")]; keymap - .bindings_for_input(&input, &context) + .bindings_for_input(input, &context) .0 .iter() .map(|b| b.action().name()) .collect() } + /// What every gpui backend delivers when the secondary modifier is held + /// over a key whose shifted half is `glyph`: the glyph, and no shift flag — + /// the modifier is already spent on the character. + fn delivered_with_secondary(glyph: &str) -> Keystroke { + Keystroke { + modifiers: gpui::Modifiers { + control: cfg!(not(target_os = "macos")), + platform: cfg!(target_os = "macos"), + shift: false, + alt: false, + function: false, + }, + key: glyph.to_string(), + key_char: None, + } + } + #[test] fn every_dispatchable_action_has_a_slot_to_bind_it_in() { // `make_binding` is what turns an action name into a real binding, and @@ -1673,15 +1850,25 @@ mod tests { // `control_code_binding_allowed`; anything new needs a fall-through of // its own to join them. for (action, spec) in default_bindings() { - for chord in spec.split_whitespace() { - Keystroke::parse(chord).expect("default chords parse"); - assert!( - !steals_a_control_code(chord) || control_code_binding_allowed(action, chord), - "{action} is bound to {chord}, which the shell needs as a control code \ - (Ctrl+[ is ESC, Ctrl+D is EOF, Ctrl+W deletes a word, \ - Ctrl+2..8 are NUL/ESC/FS/GS/RS/US/DEL). \ - Window actions belong on ctrl-shift-* off macOS." - ); + // Both spellings, because `push_binding` installs both. "Window + // actions belong on ctrl-shift-*" stops being an escape over the + // digits and the punctuation the moment the fold clears the Shift + // again: `ctrl-shift-2` goes into the keymap as `ctrl-@` (#750). + let folded = folded_spec(spec); + for spelling in [Some(spec), folded.as_deref()].into_iter().flatten() { + for chord in spelling.split_whitespace() { + Keystroke::parse(chord).expect("default chords parse"); + assert!( + !steals_a_control_code(chord) + || control_code_binding_allowed(action, chord), + "{action} is bound to {spec}, installed as {chord}, which the shell \ + needs as a control code \ + (Ctrl+[ is ESC, Ctrl+D is EOF, Ctrl+W deletes a word, \ + Ctrl+2..8 are NUL/ESC/FS/GS/RS/US/DEL). \ + Window actions belong on ctrl-shift-* off macOS — over a letter, \ + where the platform keeps the Shift." + ); + } } } } @@ -1738,13 +1925,18 @@ mod tests { // `ScmCommit` and `ToggleFullscreen` both take secondary-enter on that // basis. Two bindings sharing a chord *and* a context is still a bug, // because then which one fires is arbitrary. + // `same_chord`, not string equality: `secondary-shift-]` and + // `secondary-}` are two spellings of one keystroke and both install it. let mut seen: Vec<(&str, &str, Option<&'static str>)> = Vec::new(); for (action, spec) in default_bindings() { if spec.is_empty() { continue; } let context = action_context(action); - if let Some((other, _, _)) = seen.iter().find(|(_, s, c)| *s == spec && *c == context) { + if let Some((other, _, _)) = seen + .iter() + .find(|(_, s, c)| same_chord(s, spec) && *c == context) + { panic!("{action} and {other} both claim {spec} in context {context:?}"); } seen.push((action, spec, context)); @@ -1771,6 +1963,165 @@ mod tests { } } + #[test] + fn a_shifted_punctuation_chord_dispatches_however_it_is_spelled() { + // Recording ⌘⇧] wrote `secondary-}` — correctly, since that is the + // keystroke it was handed — while the default table and every config + // in the wild spell the same chord `secondary-shift-]`. Only one of + // the two used to reach the dispatcher (#750); all three do now. + let typed = delivered_with_secondary("}"); + let recorded = spec_from_keystroke(&typed).expect("the recorder spells the chord"); + assert_eq!(recorded, "secondary-}"); + for spec in [recorded.as_str(), "secondary-shift-]", "secondary-shift-}"] { + let effective = [("NextTab".to_string(), spec.to_string())]; + assert_eq!( + dispatched_keystrokes(&effective, std::slice::from_ref(&typed), ""), + vec![NextTab::name_for_type()], + "{spec} never reaches the dispatcher" + ); + } + } + + #[test] + fn the_default_pane_cycle_chord_reaches_its_action() { + // `secondary-]` on macOS, `secondary-shift-]` everywhere else — and + // either way the backend reports the chord as the bare glyph. The + // second spelling installed a binding nothing could press. + let effective: Vec<(String, String)> = default_bindings() + .into_iter() + .map(|(a, k)| (a.to_string(), k.to_string())) + .collect(); + let glyph = if cfg!(target_os = "macos") { "]" } else { "}" }; + assert!( + dispatched_keystrokes(&effective, &[delivered_with_secondary(glyph)], "") + .contains(&FocusNextPane::name_for_type()), + "the default pane-cycle chord dispatches nothing" + ); + } + + #[test] + fn folding_leaves_every_other_spelling_alone() { + // Shift over a letter is a real modifier, and the named keys have no + // shifted half at all — folding either would break far more than it + // fixed. + for spec in [ + "secondary-shift-t", + "shift-enter", + "shift-insert", + "shift-tab", + "ctrl-shift-up", + "secondary-t", + "secondary--", + "ctrl-b n", + ] { + assert_eq!(folded_spec(spec), None, "{spec} was rewritten"); + } + // A prefix chord folds per chord, not as a whole. + assert_eq!(folded_spec("ctrl-b shift-5").as_deref(), Some("ctrl-b %")); + } + + #[test] + fn a_chord_the_platform_delivers_with_shift_intact_stays_bound() { + // Windows leaves Shift on the numpad's `/ * + -` — they are absent + // from gpui's `need_to_convert_to_shifted_key`, so Ctrl+Shift+numpad-/ + // arrives as `ctrl-shift-/` and the recorder writes exactly that. + // Folding is an addition, never a substitution, so that spec has to + // survive the treatment `secondary-shift-]` gets (#750). + let numpad = Keystroke { + modifiers: gpui::Modifiers { + control: cfg!(not(target_os = "macos")), + platform: cfg!(target_os = "macos"), + shift: true, + alt: false, + function: false, + }, + key: "/".to_string(), + key_char: None, + }; + assert_eq!( + spec_from_keystroke(&numpad).as_deref(), + Some("secondary-shift-/") + ); + let effective = [("NextTab".to_string(), "secondary-shift-/".to_string())]; + assert_eq!( + dispatched_keystrokes(&effective, std::slice::from_ref(&numpad), ""), + vec![NextTab::name_for_type()], + "the numpad chord lost the binding written for it" + ); + // And the main row's `?`, which the same spec also stands for, still + // reaches the action — the fold added it rather than taking over. + assert_eq!( + dispatched_keystrokes(&effective, &[delivered_with_secondary("?")], ""), + vec![NextTab::name_for_type()], + ); + } + + #[test] + fn two_spellings_of_one_chord_are_recognised_as_one() { + // What `assign_keybinding` leans on to displace the action already + // holding a chord, whichever way either of them is written down. + assert!(same_chord("secondary-shift-]", "secondary-}")); + assert!(same_chord("secondary-}", "secondary-shift-]")); + assert!(same_chord("secondary-shift-}", "secondary-shift-]")); + assert!(same_chord("secondary-t", "secondary-t")); + assert!(!same_chord("secondary-shift-]", "secondary-shift-[")); + assert!(!same_chord("secondary-}", "secondary-{")); + } + + #[test] + fn the_control_code_guard_follows_the_folded_chord() { + // The half of the control-code rule that only warns. A chord carrying + // Shift reads as safe on its own — `steals_a_control_code` recognises a + // bare Ctrl and nothing else — and it *was* safe while nothing could + // press it. The fold makes it live, and Ctrl+Shift+2 is delivered as + // Ctrl+@, which is the NUL the program on the far end is waiting for. + // So the guard is asked about the spelling that goes into the keymap + // (#750); `action_bindings` runs exactly this pair. + for (written, folded) in [ + ("ctrl-shift-2", per_platform("ctrl-@", "secondary-@")), + ("ctrl-shift-6", per_platform("ctrl-^", "secondary-^")), + ("ctrl-shift--", per_platform("ctrl-_", "secondary-_")), + ("ctrl-shift-/", per_platform("ctrl-?", "secondary-?")), + ] { + assert!( + !steals_a_control_code(written), + "{written} reads as safe as written, which is why the fold has to be checked" + ); + assert_eq!(folded_spec(written).as_deref(), Some(folded)); + assert!( + steals_a_control_code(folded), + "{written} is installed as {folded} and takes a control code" + ); + } + } + + #[test] + fn a_key_the_us_table_does_not_know_is_left_alone() { + // `SHIFTED_GLYPHS` is a US keyboard, and every other layout prints + // something else on the shifted half: `?` is Shift+ß on a German + // keyboard and Shift+, on a French one. The fold cannot know that, so + // it declines rather than guesses — a key it has never seen keeps its + // Shift, and the binding written for it stays exactly as written. That + // is also what keeps the fold additive: it can widen a spec's reach but + // never move it onto a chord the user did not ask for. + for spec in [ + "secondary-shift-ß", + "secondary-shift-é", + "secondary-shift-ä", + "secondary-shift-ç", + "secondary-shift-ñ", + ] { + assert_eq!(folded_spec(spec), None, "{spec} was guessed at"); + } + // And the recorded spelling, which is what a non-US layout actually + // produces, needs no fold to reach the dispatcher in the first place. + let effective = [("NextTab".to_string(), "secondary-?".to_string())]; + assert_eq!( + dispatched_keystrokes(&effective, &[delivered_with_secondary("?")], ""), + vec![NextTab::name_for_type()], + ); + } + #[test] fn spec_from_keystroke_ignores_a_lone_modifier() { let ks = Keystroke::parse("secondary").unwrap();