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 1…Alt 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/terminal/remote.rs b/src/terminal/remote.rs
index 86d517dc..29ab35be 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())
@@ -705,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);
@@ -2859,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`,
@@ -3073,6 +3626,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/terminal/view.rs b/src/terminal/view.rs
index 0dbc8628..7c7ab1c5 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
@@ -7403,6 +7431,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..08365c3d 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>,
@@ -1447,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);
@@ -4134,6 +4211,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;
@@ -4901,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),
@@ -6669,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
@@ -7401,6 +7505,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)),
)
@@ -10162,6 +10269,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
@@ -10258,6 +10445,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/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..fea7d64f 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",
@@ -1404,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…",
@@ -1775,6 +1777,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 ce986fc0..50a29245 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 => "ワークツリーには名前が必要です",
@@ -1461,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 => "タブの名前を変更…",
@@ -1846,6 +1848,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 1219d497..c7bd1d96 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,
@@ -997,6 +998,7 @@ l10n_keys! {
TabTooltipHideSidebar,
TabTooltipHideDetailPanel,
TabTooltipShowDetailPanel,
+ TabTooltipZoomed,
TabMenuLocalShells,
TabMenuAddHost,
TabMenuAllHosts,
@@ -1123,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 7063697a..9ea57025 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 需要一个名称",
@@ -1322,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 => "重命名标签页…",
@@ -1685,6 +1687,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/keymap.rs b/src/ui/keymap.rs
index 7c175e57..615b63e0 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);
}
@@ -163,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() {
@@ -184,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
@@ -265,6 +304,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 +783,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(),
@@ -940,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()
}
@@ -1041,6 +1214,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),
@@ -1164,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
@@ -1223,6 +1425,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 +1440,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"))]
@@ -1596,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."
+ );
+ }
}
}
}
@@ -1661,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));
@@ -1694,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();
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),
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/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..e94f451d 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;
@@ -2956,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 {
@@ -3290,6 +3272,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
diff --git a/src/ui/tab_sidebar.rs b/src/ui/tab_sidebar.rs
index 6324133e..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 };
@@ -326,9 +335,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 +362,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,
@@ -723,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 33982dd7..9ba70898 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
@@ -1222,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
@@ -1229,6 +1341,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 +1353,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 +1367,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
@@ -1632,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
@@ -1759,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(
@@ -2688,4 +2793,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);
+ }
}