From 20b73adb2ac10b3aefbae2c9c7beda01a66a3e9f Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:00:55 +0800 Subject: [PATCH 01/15] chore(release): v26.9.1 --- CHANGELOG.md | 39 +++++++++++++++++++++++++++++++++++++++ Cargo.lock | 8 ++++---- Cargo.toml | 2 +- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f108c4b2..2c83283d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,43 @@ All notable changes to tty7 are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [26.9.1] - 2026-09-07 + +### Fixed + +- **A remote server that will not start now says why** (#774). A remote + workspace could sit in a loop nobody could get out of: every reconnect failed + with "started but nothing was answering on the control socket after 15s", the + strip showed a copy bar frozen at 100%, and no button was offered. A daemon + whose control listener would not open logged one line and kept running — and a + running daemon holds the single-server lock, so every later start stood down + at once and every probe failed, forever. Whether something else is serving is + now settled by connecting rather than by reading the errno, and a daemon that + cannot listen exits. The reason is kept too: the remote daemon's output and + exit status land beside the binary, stamped with the launch's own nonce so a + restart never reads the outgoing daemon's status as the incoming one's, and a + start that has already failed no longer waits out the full timeout. On the + window side, an automatic reconnect retires its install progress instead of + drawing an install still in flight, and a long error no longer stretches the + status card past the window and takes the retry button off screen with it. + +- **A stale pane socket no longer stops every later daemon** (#779). A daemon + that died without unlinking its Unix socket left a file `bind` refuses, so the + client launched a daemon, it exited on the bind, and the client launched + another — forever. The removal is now decided by the single-server seat rather + than the pidfile: holding the seat means nobody else can be serving that config + dir, so anything still at the endpoint belongs to a process that is gone. + Where there is no seat, a socket that answers is refused rather than removed. + +### Changed + +- **The control dialect is v9.** v8 added the project verbs; this build takes + them back out and speaks v7's messages again, message for message — but the + number does not go back with them. v8 is deployed, and a number that moves + backwards stops being an identity: a 7 on the wire would mean either "before + projects" or "after them" depending on which build put it there, and the + handshake has nothing but the number to tell the two apart. + ## [26.9.0] - 2026-09-04 ### Added @@ -4401,6 +4438,8 @@ Initial release. - zsh shell integration (OSC 7 cwd + OSC 133 prompt marks) via a throwaway `ZDOTDIR`. - Native macOS light/dark themes that follow the system appearance. +[26.9.1]: https://github.com/l0ng-ai/tty7/compare/v26.9.0...v26.9.1 +[26.9.0]: https://github.com/l0ng-ai/tty7/compare/v26.8.3...v26.9.0 [26.8.3]: https://github.com/l0ng-ai/tty7/compare/v26.8.2...v26.8.3 [26.8.2]: https://github.com/l0ng-ai/tty7/compare/v26.8.1...v26.8.2 [26.8.1]: https://github.com/l0ng-ai/tty7/compare/v26.8.0...v26.8.1 diff --git a/Cargo.lock b/Cargo.lock index 7974f7e1..4e345fcb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9775,7 +9775,7 @@ dependencies = [ [[package]] name = "tty7" -version = "26.9.0" +version = "26.9.1" dependencies = [ "alacritty_terminal", "anyhow", @@ -9820,7 +9820,7 @@ dependencies = [ [[package]] name = "tty7-cli" -version = "26.9.0" +version = "26.9.1" dependencies = [ "alacritty_terminal", "anyhow", @@ -9835,7 +9835,7 @@ dependencies = [ [[package]] name = "tty7-core" -version = "26.9.0" +version = "26.9.1" dependencies = [ "anyhow", "base64 0.22.1", @@ -9869,7 +9869,7 @@ dependencies = [ [[package]] name = "tty7-server" -version = "26.9.0" +version = "26.9.1" dependencies = [ "libc", "tempfile", diff --git a/Cargo.toml b/Cargo.toml index 31d358e5..55848281 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -271,7 +271,7 @@ edition = "2024" # they must never drift apart. This is the line a release bump edits (and the # one nightly's `awk '/^version = /'` stamps: it is the first such line in the # file, since the package entries above are all `version.workspace = true`). -version = "26.9.0" +version = "26.9.1" [workspace.dependencies] # Our fork's `tty7` branch carries the local customizations tty7 relies on: From f6358494b608bc6e2790760a364d1a4fb8c21af7 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:09:07 +0800 Subject: [PATCH 02/15] test(remote): give each machine in the route-origin tests its own key (#798) --- src/ui/remote_connect.rs | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/src/ui/remote_connect.rs b/src/ui/remote_connect.rs index b04ea116..42d101b4 100644 --- a/src/ui/remote_connect.rs +++ b/src/ui/remote_connect.rs @@ -960,6 +960,7 @@ mod tests { fn native_spec(user: &str, host: &str, port: u16) -> NativeSshSpec { let mut profile = crate::core::ssh_profile::SshProfile::new(host.to_string()); + profile.host = host.to_string(); profile.user = user.to_string(); profile.port = port; crate::ui::ssh_connect::build_native_ssh_spec( @@ -970,6 +971,36 @@ mod tests { ) } + /// `SshProfile::new` takes a *name*, and the address lives in a separate + /// `host` field; every production caller assigns both. `native_spec` used + /// to assign only the name, so the spec it handed back addressed nobody + /// and `ConnectionKey::from_spec` spelled it `me@:22` — one key for every + /// machine in this module that talks to port 22. + /// + /// `ORIGINS` is keyed by exactly that string, so the two tests below that + /// note an origin and read it back were writing to and reading from the + /// same slot. Whichever noted last won, and the other was handed the wrong + /// machine: 17 failures in 20 runs of this module alone, 6 in 20 of the + /// whole binary, and none when either test ran by itself. + #[test] + fn two_machines_do_not_share_one_route_origin_key() { + use crate::daemon::router::RouteTarget; + + let build = RouteTarget::Ssh(Box::new(native_spec("me", "build-box", 22))); + let twin = RouteTarget::Ssh(Box::new(native_spec("me", "twin-box", 22))); + + assert_eq!( + build.origin_key(), + "me@build-box:22", + "a route origin key names the machine it dials" + ); + assert_ne!( + build.origin_key(), + twin.origin_key(), + "two machines sharing one key make `note_origin` overwrite the other's" + ); + } + #[test] fn a_routed_auth_prompt_carries_the_machine_that_raised_it() { let _turn = claim_mailbox(); @@ -978,6 +1009,7 @@ mod tests { let route = crate::daemon::router::RouteTarget::Ssh(Box::new(native_spec("me", "build-box", 22))); note_origin(&route, &target); + let origin_key = route.origin_key(); let handle = std::thread::spawn(move || { use crate::daemon::router::RouteAuthResponder as _; @@ -1005,7 +1037,12 @@ mod tests { ); std::thread::sleep(Duration::from_millis(5)); }; - assert_eq!(pending.host, target.host_id()); + assert_eq!( + pending.host, + target.host_id(), + "the prompt names the machine noted under {:?}", + origin_key + ); pending.answer(AuthResponse::Secret("hunter2".into())); assert_eq!( handle.join().unwrap(), From 6474e25a24a725aafa4956959ad0dc04931c9ab3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:09:19 +0800 Subject: [PATCH 03/15] deps: bump the cargo-minor-patch group across 1 directory with 3 updates (#776) --- Cargo.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4e345fcb..c48f27fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4581,9 +4581,9 @@ dependencies = [ [[package]] name = "keyring" -version = "4.1.6" +version = "4.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72585bb6cc9bc370d1d545b7e23fcce71dfd4461c5e15275e3cf51bdfd9a980a" +checksum = "2270074a3d26bcac93c1dc5d2845eb4c089e8d761ccf6e0ea266a16004640627" dependencies = [ "apple-native-keyring-store", "keyring-core", @@ -4901,9 +4901,9 @@ dependencies = [ [[package]] name = "log" -version = "0.4.33" +version = "0.4.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0ceec5bc11778974d1bcb055b18002eba7f4b3518b6a0081b3af5f21666da9ad" +checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6" dependencies = [ "serde_core", "value-bag", @@ -10156,9 +10156,9 @@ dependencies = [ [[package]] name = "uuid" -version = "1.24.1" +version = "1.26.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2cefc03fd367c0c6d4305de1b312cf00248c4114f4a0418ce6a6af769e3b0bd9" +checksum = "b5772d71c9be8a8a6ac2117d949c5b224c1b72241bb611d9a3012edcf8af7812" dependencies = [ "getrandom 0.4.3", "js-sys", From dac70ec4c838cffec3bd8e2a38b1bbc39e8201e5 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:11:06 +0800 Subject: [PATCH 04/15] perf(protocol): put a frame's header and payload on the wire in one write (#797) Refs #713. --- crates/tty7-core/src/daemon/protocol.rs | 85 +++++++++++++++++++++++-- 1 file changed, 81 insertions(+), 4 deletions(-) diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index ecdc62bd..0eaa807e 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -5,6 +5,9 @@ use serde::{Deserialize, Serialize}; pub const MAX_FRAME: usize = 64 * 1024 * 1024; +/// A frame is a little-endian `u32` length, a one-byte kind, then the payload. +const HEADER: usize = 5; + pub const PROTOCOL_VERSION: u32 = 6; pub const FEATURE_PANE_OWNER: &str = "pane-owner"; @@ -952,9 +955,33 @@ pub fn write_frame(w: &mut W, kind: u8, payload: &[u8]) -> io::Result< "frame payload exceeds MAX_FRAME", )); } - w.write_all(&(len as u32).to_le_bytes())?; - w.write_all(&[kind])?; - w.write_all(payload)?; + // One write, not three. The pane socket is a loopback `TcpStream` with + // `TCP_NODELAY` set, so three `write_all`s put the length, the kind and the + // payload on the wire as three separate segments, and the reader on the far + // side wakes from `read()` three times for one frame. Under a PTY flood the + // daemon frames every ConPTY read, so that is two extra syscalls on each + // side per frame, tens of thousands a second (issue #713). + let mut header = [0u8; HEADER]; + header[..4].copy_from_slice(&(len as u32).to_le_bytes()); + header[4] = kind; + let mut bufs = [io::IoSlice::new(&header), io::IoSlice::new(payload)]; + let mut rest: &mut [io::IoSlice<'_>] = &mut bufs; + while !rest.is_empty() { + match w.write_vectored(rest) { + Ok(0) => { + return Err(io::Error::new( + io::ErrorKind::WriteZero, + "the frame could not be written in full", + )); + } + // A writer that does not implement `write_vectored` natively falls + // back to writing the first non-empty slice, so this loop still + // terminates — it just costs the two writes it used to cost. + Ok(n) => io::IoSlice::advance_slices(&mut rest, n), + Err(e) if e.kind() == io::ErrorKind::Interrupted => {} + Err(e) => return Err(e), + } + } Ok(()) } @@ -984,7 +1011,6 @@ pub fn is_error_kind(kind: u8) -> bool { } pub fn take_frame(buf: &mut Vec) -> io::Result)>> { - const HEADER: usize = 5; if buf.len() < HEADER { return Ok(None); } @@ -2046,6 +2072,57 @@ mod tests { assert!(buf.is_empty()); } + /// The pane socket has `TCP_NODELAY` set, so a write is a segment and a + /// segment is a wakeup on the far side. A frame must therefore cost one + /// write, not one for the length, one for the kind and one for the payload + /// (issue #713) — at flood rates that difference is tens of thousands of + /// syscalls a second on each end. + #[test] + fn a_frame_is_one_write_on_a_vectored_writer() { + #[derive(Default)] + struct Counting { + writes: usize, + bytes: Vec, + } + impl Write for Counting { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.writes += 1; + self.bytes.extend_from_slice(buf); + Ok(buf.len()) + } + fn write_vectored(&mut self, bufs: &[io::IoSlice<'_>]) -> io::Result { + self.writes += 1; + let mut n = 0; + for b in bufs { + self.bytes.extend_from_slice(b); + n += b.len(); + } + Ok(n) + } + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + let mut w = Counting::default(); + write_frame(&mut w, kind::OUTPUT, b"a chunk of pty output").expect("write the frame"); + assert_eq!(w.writes, 1, "one frame must cost one write"); + + // An empty payload is a frame too — the header still has to land, and + // the empty second slice must not spin the loop. + let mut empty = Counting::default(); + write_frame(&mut empty, kind::DETACH, &[]).expect("write the empty frame"); + assert_eq!(empty.writes, 1); + + // Whatever the write count, the bytes on the wire are unchanged: a + // `read_frame` over them gives back exactly what went in. + let mut cursor = io::Cursor::new(w.bytes); + assert_eq!( + read_frame(&mut cursor).expect("read it back"), + (kind::OUTPUT, b"a chunk of pty output".to_vec()) + ); + } + #[test] fn from_frame_rejects_unknown_kind() { assert!(ClientMsg::from_frame(99, vec![]).is_err()); From 081e191bb0918695dc56d5461b2d74f4a8c15ae2 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:13:57 +0800 Subject: [PATCH 05/15] perf(diff-overlay): draw the patch as a virtualised row list (#799) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The overlay built its whole patch as a nested element tree on every frame: a card per file, a header per hunk, six elements per line. gpui notifies the view on each scroll wheel event, so a few hundred lines of diff rebuilt tens of thousands of elements tens of times a second, and the window stalled. Flatten the tree into one row per line in a new `diff_list` module and draw it with `gpui::list`, which builds only the rows on screen. The rows are rebuilt only when what they are built from changes, so scrolling no longer re-splits hunks or re-clones every line, and a change to one file splices just the rows it touched rather than resetting the list and losing the scroll position. The key that decides a rebuild takes the snapshot each frame was asked about even when it matched only by contents. A probe that finds nothing new still lands a fresh `Arc` over an equal snapshot; a key left pointing at the old one would go on walking the whole patch to prove the two equal, once per wheel event, which is the cost the key exists to avoid. A list counts a row it has not laid out yet as zero tall, which left the scrollbar reading an 800-line patch as one viewport: its thumb filled the track, and a drag from top to bottom travelled 248px and stopped. The rows below the fold are counted at the 19px both views already give a line of a patch, through `ListState::with_size_hint` — added to the gpui fork for this, `Cargo.lock` following its `tty7` branch to `ece710e3`. A card cannot survive that flattening — its rows are separate items now — so the frame it drew is gone, and with it the grey header bars and hunk bands that made the overlay the one view in the app still speaking gpui-component's default container language. The rows take the source control panel's own measurements instead: 26px, 10px inset, 5px radius, colour only under the pointer. The title bar's view switch loses its border for the same reason. --- Cargo.lock | 90 +-- Cargo.toml | 10 +- src/ui/diff_list.rs | 484 +++++++++++++ src/ui/diff_overlay.rs | 1480 ++++++++++++++++++++++++---------------- src/ui/diff_rows.rs | 3 + src/ui/mod.rs | 1 + src/ui/rounding.rs | 34 - src/ui/scrollbar.rs | 10 +- 8 files changed, 1447 insertions(+), 665 deletions(-) create mode 100644 src/ui/diff_list.rs diff --git a/Cargo.lock b/Cargo.lock index c48f27fc..a0f31ea1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -298,7 +298,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -309,7 +309,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -891,7 +891,7 @@ dependencies = [ "bitflags 2.13.1", "cexpr", "clang-sys", - "itertools 0.13.0", + "itertools 0.11.0", "log", "prettyplease", "proc-macro2", @@ -1483,7 +1483,7 @@ dependencies = [ [[package]] name = "collections" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "gpui_util", "indexmap", @@ -2047,7 +2047,7 @@ dependencies = [ [[package]] name = "derive_refineable" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "proc-macro2", "quote", @@ -2104,7 +2104,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -2142,7 +2142,7 @@ version = "0.5.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ab8ecd87370524b461f8557c119c405552c396ed91fc0a8eec68679eab26f94a" dependencies = [ - "libloading 0.8.9", + "libloading 0.7.4", ] [[package]] @@ -2394,7 +2394,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -3199,7 +3199,7 @@ dependencies = [ "log", "presser", "thiserror 2.0.20", - "windows 0.62.2", + "windows 0.58.0", ] [[package]] @@ -3225,7 +3225,7 @@ dependencies = [ [[package]] name = "gpui" version = "0.2.2" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "accesskit", "anyhow", @@ -3416,7 +3416,7 @@ dependencies = [ [[package]] name = "gpui_linux" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "accesskit", "accesskit_unix", @@ -3467,7 +3467,7 @@ dependencies = [ [[package]] name = "gpui_macos" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "accesskit", "accesskit_macos", @@ -3514,7 +3514,7 @@ dependencies = [ [[package]] name = "gpui_macros" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "heck 0.5.0", "proc-macro2", @@ -3525,7 +3525,7 @@ dependencies = [ [[package]] name = "gpui_platform" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "console_error_panic_hook", "gpui", @@ -3538,7 +3538,7 @@ dependencies = [ [[package]] name = "gpui_shared_string" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "schemars", "serde", @@ -3548,7 +3548,7 @@ dependencies = [ [[package]] name = "gpui_util" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "log", @@ -3557,7 +3557,7 @@ dependencies = [ [[package]] name = "gpui_web" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "console_error_panic_hook", @@ -3581,7 +3581,7 @@ dependencies = [ [[package]] name = "gpui_wgpu" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "bytemuck", @@ -3610,7 +3610,7 @@ dependencies = [ [[package]] name = "gpui_windows" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "accesskit", "accesskit_windows", @@ -3939,7 +3939,7 @@ dependencies = [ [[package]] name = "http_client" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "async-compression", @@ -3964,7 +3964,7 @@ dependencies = [ [[package]] name = "http_client_tls" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "rustls", "rustls-platform-verifier", @@ -4057,7 +4057,7 @@ dependencies = [ "js-sys", "log", "wasm-bindgen", - "windows-core 0.62.2", + "windows-core 0.58.0", ] [[package]] @@ -5092,7 +5092,7 @@ checksum = "7ebb8d8732c6a6df3d8f032a82911cfc747e00efb95cc46e8d0acd5b5b88570c" [[package]] name = "media" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "bindgen", @@ -5202,7 +5202,7 @@ version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "536bfad37a309d62069485248eeaba1e8d9853aaf951caaeaed0585a95346f08" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -5258,7 +5258,7 @@ dependencies = [ "once_cell", "png", "thiserror 2.0.20", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -5481,7 +5481,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -6204,7 +6204,7 @@ checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" [[package]] name = "perf" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "collections", "serde", @@ -6900,7 +6900,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -7207,7 +7207,7 @@ dependencies = [ [[package]] name = "refineable" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "derive_refineable", ] @@ -7250,7 +7250,7 @@ checksum = "19b30a45b0cd0bcca8037f3d0dc3421eaf95327a17cad11964fb8179b4fc4832" [[package]] name = "reqwest_client" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "bytes", @@ -7611,7 +7611,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -7784,7 +7784,7 @@ dependencies = [ [[package]] name = "scheduler" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "async-task", "backtrace", @@ -8389,7 +8389,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c3d1e2c7f27f8d4cb10542a02c49005dbd6e93095799d6f3be745fae9f8fedd4" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -8515,7 +8515,7 @@ dependencies = [ "cfg-if", "libc", "psm", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -8627,7 +8627,7 @@ checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "sum_tree" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "heapless", "log", @@ -8917,7 +8917,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix 1.1.4", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -9384,7 +9384,7 @@ dependencies = [ "once_cell", "png", "thiserror 2.0.20", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -9896,7 +9896,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" dependencies = [ "memoffset 0.9.1", "tempfile", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -10108,7 +10108,7 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "util" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "async-fs", @@ -10147,7 +10147,7 @@ dependencies = [ [[package]] name = "util_macros" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "perf", "quote", @@ -10740,7 +10740,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.48.0", ] [[package]] @@ -11491,7 +11491,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7d6f32a0ff4a9f6f01231eb2059cc85479330739333e0e58cadf03b6af2cca10" dependencies = [ "cfg-if", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -12048,7 +12048,7 @@ dependencies = [ [[package]] name = "zlog" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "anyhow", "chrono", @@ -12093,7 +12093,7 @@ dependencies = [ [[package]] name = "ztracing" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" dependencies = [ "tracing", "tracing-subscriber", @@ -12104,7 +12104,7 @@ dependencies = [ [[package]] name = "ztracing_macro" version = "0.1.0" -source = "git+https://github.com/l0ng-ai/zed?branch=tty7#d99a40a5f79a173465feec00363d480f75881a81" +source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a8d20488769b2de9a6" [[package]] name = "zune-core" diff --git a/Cargo.toml b/Cargo.toml index 55848281..48f90ceb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -367,7 +367,7 @@ lto = "thin" codegen-units = 1 # ---- gpui fork ------------------------------------------------------------ -# Our `tty7` branch (cut from the pinned upstream rev, three commits on top) carries: +# Our `tty7` branch (cut from the pinned upstream rev) carries: # # 1. `prefers_ime_for_printable_keys` takes the keystroke, so an input handler can # answer per key instead of per view. tty7 needs it for Option-as-Meta — macOS @@ -386,6 +386,14 @@ codegen-units = 1 # 3. resvg/usvg bumped 0.45 → 0.47 so gpui's SVG stack unifies with the resvg # tty7 pins directly above (follow-up to the #227 dependabot bump). # +# 4. `ListState::with_size_hint`. A list counts an item it has not laid out yet +# as zero tall, so a long document reports itself as about one viewport and +# a scrollbar reading that height drags one viewport and stops — the diff +# overlay scrolls a whole working tree through one of these. The hint counts +# the unmeasured items at an estimate instead, which `measure_all` would +# settle exactly at the price of laying out the whole document on the first +# frame (see `ui::diff_overlay::DIFF_LINE_H`). +# # Patching by source rather than editing the `gpui`/`gpui_platform` pins above is # deliberate: `gpui-component` declares its own `gpui` from the upstream URL, and # a plain pin swap would put two incompatible copies of gpui in the tree. `[patch]` diff --git a/src/ui/diff_list.rs b/src/ui/diff_list.rs new file mode 100644 index 00000000..08a77818 --- /dev/null +++ b/src/ui/diff_list.rs @@ -0,0 +1,484 @@ +//! Flattening a diff snapshot into the one-dimensional list of rows the +//! overlay scrolls. +//! +//! The overlay used to build its whole patch as a nested element tree — +//! a card per file, a hunk header and one element per line inside it, every +//! one of them constructed on every frame. A `20_000` line budget is around +//! `120_000` elements to lay out, and gpui notifies the view on each scroll +//! wheel event, so a diff of any size rebuilt the entire tree tens of times a +//! second. +//! +//! [`gpui::list`] only builds the rows it can see, but it is one-dimensional: +//! it takes an index, not a tree. So the tree is flattened here, once per +//! change to what is on screen. + +use std::collections::HashMap; + +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}; + +/// Everything the file header row draws, lifted out of its [`FileDiff`]. +/// +/// A copy rather than a borrow: the row list outlives the frame that built it, +/// and these are a handful of small fields against a file's whole patch. +#[derive(PartialEq, Eq)] +pub(crate) struct FileHead { + /// Position in `snap.files`, for the element id and nothing else. + pub(crate) index: usize, + pub(crate) path: String, + /// `old → new` for a rename, the path itself otherwise. + pub(crate) shown_path: String, + pub(crate) status: FileStatus, + pub(crate) added: u32, + pub(crate) removed: u32, + pub(crate) binary: bool, + pub(crate) expandable: bool, + pub(crate) expanded: bool, +} + +#[derive(PartialEq, Eq)] +pub(crate) enum DiffRow { + /// The space that used to be the `gap_3` of a flex column. + Gap, + /// The banner above an auto-collapsed tree. Its text is derived from the + /// snapshot at render time, so nothing is carried here. + Oversized, + FileHeader(FileHead), + HunkHeader { + text: String, + /// The first hunk of a file follows its header and needs no rule + /// above it; every later one is separating itself from the lines of + /// the hunk before. + leads: bool, + }, + Split(SplitRow), + Unified(UnifiedRow), + Truncated(Truncation), + MoreFiles { + rest: usize, + }, + UntrackedHeader { + total: usize, + }, + Untracked { + index: usize, + path: String, + }, + MoreUntracked { + rest: usize, + }, +} + +/// The rows of the whole overlay, in the order they scroll past. +pub(crate) fn build_rows( + snap: &DiffSnapshot, + expanded: &HashMap, + focused: Option, + mode: DiffViewMode, + oversized: bool, +) -> Vec { + let mut out: Vec = Vec::new(); + // Stands in for the gap between the groups this list used to be a flex + // column of: gpui's list stacks its items with nothing between them. + let gap = |out: &mut Vec| { + if !out.is_empty() { + out.push(DiffRow::Gap); + } + }; + + if oversized { + out.push(DiffRow::Oversized); + } + + let shown = snap.files.len().min(MAX_RENDERED_FILES); + for (idx, file) in snap.files.iter().enumerate() { + if focused.is_some_and(|f| f != idx) { + continue; + } + if focused.is_none() && idx >= shown { + break; + } + let is_expanded = if focused == Some(idx) { + expanded.get(&file.path).copied().unwrap_or(true) + } else { + file_expanded(file, expanded, oversized) + }; + gap(&mut out); + out.extend(file_rows(idx, file, is_expanded, mode)); + } + + if focused.is_none() && snap.files.len() > shown { + gap(&mut out); + out.push(DiffRow::MoreFiles { + rest: snap.files.len() - shown, + }); + } + + if focused.is_none() && !snap.untracked.is_empty() { + gap(&mut out); + out.extend(untracked_rows(snap)); + } + + out +} + +/// The single synthesized file an untracked file's preview shows. +/// +/// `usize::MAX` keeps its element ids clear of the real list's. +pub(crate) fn preview_rows(file: &FileDiff, mode: DiffViewMode) -> Vec { + file_rows(usize::MAX, file, true, mode) +} + +fn file_rows(index: usize, file: &FileDiff, expanded: bool, mode: DiffViewMode) -> Vec { + let expandable = + !file.binary && (!file.hunks.is_empty() || file.truncated == Some(Truncation::Budget)); + let shown_path = match &file.old_path { + Some(old) => format!("{old} → {}", file.path), + None => file.path.clone(), + }; + let mut rows = vec![DiffRow::FileHeader(FileHead { + index, + path: file.path.clone(), + shown_path, + status: file.status, + added: file.added, + removed: file.removed, + binary: file.binary, + expandable, + expanded, + })]; + + if expanded && (!file.hunks.is_empty() || file.truncated.is_some()) { + for (h, hunk) in file.hunks.iter().enumerate() { + rows.push(DiffRow::HunkHeader { + text: hunk.header.clone(), + leads: h == 0, + }); + 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)) + } + } + } + if let Some(reason) = file.truncated { + rows.push(DiffRow::Truncated(reason)); + } + } + + rows +} + +fn untracked_rows(snap: &DiffSnapshot) -> Vec { + let total = snap.untracked_count(); + let shown = &snap.untracked[..snap.untracked.len().min(MAX_RENDERED_FILES)]; + let mut rows = vec![DiffRow::UntrackedHeader { total }]; + for (index, path) in shown.iter().enumerate() { + rows.push(DiffRow::Untracked { + index, + path: path.clone(), + }); + } + if total > shown.len() { + rows.push(DiffRow::MoreUntracked { + rest: total - shown.len(), + }); + } + rows +} + +/// Whether a file's lines show on their own, absent an explicit choice. +pub(crate) fn file_expanded( + file: &FileDiff, + expanded: &HashMap, + collapse_all: bool, +) -> bool { + if let Some(&want) = expanded.get(&file.path) { + return want; + } + !collapse_all && file.added + file.removed <= AUTO_COLLAPSE_LINES +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::terminal::git_diff::{DiffLine, Hunk, LineKind}; + + fn line(kind: LineKind, old: Option, new: Option) -> DiffLine { + DiffLine { + kind, + old_no: old, + new_no: new, + text: "x".to_string(), + } + } + + /// One removal answered by one addition, wrapped in a context line either + /// side — four lines unified, three rows split. + fn hunk() -> Hunk { + Hunk { + header: "@@ -1,3 +1,3 @@".to_string(), + lines: vec![ + line(LineKind::Context, Some(1), Some(1)), + line(LineKind::Removed, Some(2), None), + line(LineKind::Added, None, Some(2)), + line(LineKind::Context, Some(3), Some(3)), + ], + } + } + + fn file(path: &str, hunks: usize) -> FileDiff { + FileDiff { + path: path.to_string(), + old_path: None, + status: FileStatus::Modified, + added: 1, + removed: 1, + binary: false, + truncated: None, + hunks: std::iter::repeat_n(hunk(), hunks).collect(), + } + } + + fn snapshot(files: Vec) -> DiffSnapshot { + DiffSnapshot { + files, + ..Default::default() + } + } + + fn shape(rows: &[DiffRow]) -> Vec<&'static str> { + rows.iter() + .map(|row| match row { + DiffRow::Gap => "gap", + DiffRow::Oversized => "oversized", + DiffRow::FileHeader(_) => "file", + DiffRow::HunkHeader { .. } => "hunk", + DiffRow::Split(_) => "split", + DiffRow::Unified(_) => "unified", + DiffRow::Truncated(_) => "truncated", + DiffRow::MoreFiles { .. } => "more-files", + DiffRow::UntrackedHeader { .. } => "untracked-header", + DiffRow::Untracked { .. } => "untracked", + DiffRow::MoreUntracked { .. } => "more-untracked", + }) + .collect() + } + + fn open(paths: [&str; 1]) -> HashMap { + paths.into_iter().map(|p| (p.to_string(), true)).collect() + } + + #[test] + fn an_open_file_flattens_to_a_header_a_hunk_header_and_a_row_per_line() { + let snap = snapshot(vec![file("a.rs", 1)]); + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + assert_eq!( + shape(&rows), + ["file", "hunk", "unified", "unified", "unified", "unified"] + ); + + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Split, false); + assert_eq!( + shape(&rows), + ["file", "hunk", "split", "split", "split"], + "the split view pairs the removal with the addition that replaced it" + ); + } + + #[test] + fn only_the_first_hunk_of_a_file_follows_its_header() { + let rows = build_rows( + &snapshot(vec![file("a.rs", 2)]), + &HashMap::new(), + None, + DiffViewMode::Unified, + false, + ); + let leads: Vec = rows + .iter() + .filter_map(|r| match r { + DiffRow::HunkHeader { leads, .. } => Some(*leads), + _ => None, + }) + .collect(); + assert_eq!( + leads, + [true, false], + "the second hunk draws the rule that separates it from the first" + ); + } + + #[test] + fn a_collapsed_file_is_a_single_row() { + let snap = snapshot(vec![file("a.rs", 1)]); + let shut: HashMap = [("a.rs".to_string(), false)].into_iter().collect(); + let rows = build_rows(&snap, &shut, None, DiffViewMode::Unified, false); + assert_eq!(shape(&rows), ["file"]); + } + + #[test] + fn a_truncation_note_lands_under_the_lines_it_is_about() { + let mut f = file("a.rs", 1); + f.truncated = Some(Truncation::Budget); + let rows = build_rows( + &snapshot(vec![f]), + &HashMap::new(), + None, + DiffViewMode::Unified, + false, + ); + assert_eq!(*shape(&rows).last().unwrap(), "truncated"); + } + + #[test] + fn files_are_separated_by_a_gap_and_the_list_never_opens_with_one() { + let snap = snapshot(vec![file("a.rs", 1), file("b.rs", 1)]); + let shut: HashMap = + snap.files.iter().map(|f| (f.path.clone(), false)).collect(); + let rows = build_rows(&snap, &shut, None, DiffViewMode::Unified, false); + assert_eq!(shape(&rows), ["file", "gap", "file"]); + } + + #[test] + fn the_oversized_banner_leads_and_collapses_what_was_not_asked_for() { + let snap = snapshot(vec![file("a.rs", 1), file("b.rs", 1)]); + let rows = build_rows(&snap, &open(["b.rs"]), None, DiffViewMode::Unified, true); + assert_eq!( + shape(&rows), + [ + "oversized", + "gap", + "file", + "gap", + "file", + "hunk", + "unified", + "unified", + "unified", + "unified" + ], + "the file the reader opened stays open; the other one does not" + ); + } + + #[test] + fn a_focused_file_is_the_only_one_flattened() { + let snap = snapshot(vec![file("a.rs", 1), file("b.rs", 1)]); + let rows = build_rows( + &snap, + &HashMap::new(), + Some(1), + DiffViewMode::Unified, + false, + ); + assert_eq!( + shape(&rows), + ["file", "hunk", "unified", "unified", "unified", "unified"] + ); + let DiffRow::FileHeader(head) = &rows[0] else { + panic!("the focused file leads"); + }; + assert_eq!(head.path, "b.rs"); + assert_eq!(head.index, 1, "the element id still points into `files`"); + } + + #[test] + fn the_file_list_is_capped_and_the_tail_gets_one_line() { + let snap = snapshot( + (0..MAX_RENDERED_FILES + 25) + .map(|i| { + let mut f = file(&format!("f{i}.rs"), 1); + f.hunks.clear(); + f + }) + .collect(), + ); + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + let files = rows + .iter() + .filter(|r| matches!(r, DiffRow::FileHeader(_))) + .count(); + assert_eq!(files, MAX_RENDERED_FILES); + assert!(matches!(rows.last(), Some(DiffRow::MoreFiles { rest: 25 }))); + } + + #[test] + fn the_untracked_list_is_capped_but_its_count_stays_true() { + let mut snap = snapshot(Vec::new()); + snap.untracked = (0..MAX_RENDERED_FILES + 5) + .map(|i| format!("p{i}")) + .collect(); + snap.untracked_total = 9_000; + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + + assert!(matches!( + rows.first(), + Some(DiffRow::UntrackedHeader { total: 9_000 }) + )); + let shown = rows + .iter() + .filter(|r| matches!(r, DiffRow::Untracked { .. })) + .count(); + assert_eq!(shown, MAX_RENDERED_FILES); + assert!(matches!( + rows.last(), + Some(DiffRow::MoreUntracked { rest }) if *rest == 9_000 - MAX_RENDERED_FILES + )); + } + + #[test] + fn a_preview_is_one_open_file() { + let rows = preview_rows(&file("new.md", 1), DiffViewMode::Unified); + assert_eq!( + shape(&rows), + ["file", "hunk", "unified", "unified", "unified", "unified"] + ); + let DiffRow::FileHeader(head) = &rows[0] else { + panic!("a file opens with its header"); + }; + assert_eq!( + head.index, + usize::MAX, + "its element ids stay clear of the real list's" + ); + assert!(head.expanded); + } + + #[test] + fn a_rename_shows_both_names_and_a_binary_file_cannot_be_opened() { + let mut renamed = file("new.rs", 1); + renamed.old_path = Some("old.rs".to_string()); + let rows = preview_rows(&renamed, DiffViewMode::Unified); + let DiffRow::FileHeader(head) = &rows[0] else { + unreachable!() + }; + assert_eq!(head.shown_path, "old.rs → new.rs"); + assert_eq!(head.path, "new.rs", "the key is still the file itself"); + + let mut binary = file("logo.png", 0); + binary.binary = true; + let rows = preview_rows(&binary, DiffViewMode::Unified); + let DiffRow::FileHeader(head) = &rows[0] else { + unreachable!() + }; + assert!(!head.expandable); + } + + /// The threshold that decides whether a file opens on its own, kept where + /// the flattening that reads it lives. + #[test] + fn a_file_over_the_line_count_opens_only_when_asked() { + let mut big = file("big.rs", 1); + big.added = AUTO_COLLAPSE_LINES + 1; + let none = HashMap::new(); + assert!(!file_expanded(&big, &none, false)); + assert!(file_expanded(&big, &open(["big.rs"]), true)); + assert!(file_expanded(&file("small.rs", 1), &none, false)); + } +} diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 2e869e31..d3d60b72 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use std::rc::Rc; use std::sync::Arc; use gpui::{ @@ -13,8 +14,8 @@ use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_f use crate::core::config::{Config, DiffViewMode}; use crate::core::git::status::DecoStatus; use crate::terminal::git_diff::{ - self, AUTO_COLLAPSE_LINES, CommitLabel, DiffSnapshot, DiffSource, DiffStats, FileDiff, - FileStatus, LineKind, MAX_RENDERED_FILES, Truncation, + self, CommitLabel, DiffSnapshot, DiffSource, DiffStats, FileDiff, FileStatus, LineKind, + Truncation, }; /// How much of an untracked file the preview will read. Past this the card @@ -22,12 +23,12 @@ 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_rows::{Side, SplitCell, SplitRow, UnifiedRow, split_hunk, unified_rows}; +use crate::ui::diff_list::{DiffRow, FileHead}; +use crate::ui::diff_rows::{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; use crate::ui::rounding; -use crate::ui::rounding::RoundedCorners as _; use crate::ui::scm::path::relative_time; use crate::ui::scm::status::{status_color, status_glyph}; @@ -56,7 +57,13 @@ pub(crate) struct DiffOverlayState { /// cadence a tracked file's does. pub(crate) preview: Option<(String, Option>)>, pub(crate) preview_loading: Option, - pub(crate) scroll: gpui::ScrollHandle, + /// 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, + /// The patch, flattened into one row per line — see + /// [`crate::ui::diff_list`]. Rebuilt only when [`RowsKey`] changes. + pub(crate) rows: Rc>, + rows_key: Option, /// The [`ScmData`](crate::terminal::git_data::ScmData) epoch this patch was /// read at, for the two sources that can go stale. /// @@ -67,25 +74,6 @@ pub(crate) struct DiffOverlayState { pub(crate) epoch: Option, } -/// One hunk, already turned into whichever kind of row the current view draws. -enum HunkRows { - Split(Vec), - Unified(Vec), -} - -impl HunkRows { - fn len(&self) -> usize { - match self { - HunkRows::Split(rows) => rows.len(), - HunkRows::Unified(rows) => rows.len(), - } - } - - fn is_empty(&self) -> bool { - self.len() == 0 - } -} - /// Paints the full-window diff surface without inheriting workspace opacity. /// The preset's solid or gradient design remains intact, but neither it nor /// the plain theme fallback may reveal the OS backdrop through diff text. @@ -180,7 +168,10 @@ impl Tty7App { focus, preview: None, preview_loading: None, - scroll: gpui::ScrollHandle::new(), + 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, }); window.focus(&focus_handle, cx); @@ -395,38 +386,12 @@ impl Tty7App { cx: &mut Context, ) -> Option { self.spawn_untracked_preview_if_needed(cx); + let body = self.sync_diff_rows(cx)?; let overlay = self.tabs.get(self.active)?.diff_overlay.as_ref()?; - let content = match &overlay.load { - DiffLoad::Loading => self.diff_message(t(L10nKey::DiffReading), cx), - DiffLoad::NotARepo => self.diff_message(t(L10nKey::DiffNotARepo), cx), - DiffLoad::Ready(snap) if empty_snapshot(snap) && snap.read_failed => { - self.diff_message(t(L10nKey::DiffReadFailed), cx) - } - DiffLoad::Ready(snap) if empty_snapshot(snap) => { - self.diff_message(t(L10nKey::DiffWorkingTreeClean), cx) - } - // A focused *untracked* file has no patch in the snapshot; its - // card is synthesized from the file's own bytes — see `preview`. - DiffLoad::Ready(snap) if untracked_focus(snap, overlay.focus.as_deref()).is_some() => { - let path = untracked_focus(snap, overlay.focus.as_deref()).unwrap(); - match &overlay.preview { - Some((held, Some(file))) if held == path => { - self.diff_preview_card(file.as_ref(), &overlay.scroll, cx) - } - Some((held, None)) if held == path => { - self.diff_message(t(L10nKey::DiffReadFailed), cx) - } - _ => self.diff_message(t(L10nKey::DiffReading), cx), - } - } - DiffLoad::Ready(snap) => self.diff_file_list( - snap, - &overlay.expanded, - focused_file(snap, overlay), - &overlay.scroll, - cx, - ), + let content = match body { + DiffBody::Message(text) => self.diff_message(text, cx), + DiffBody::Rows(snap) => self.diff_rows_list(overlay, snap, cx), }; let header = chrome @@ -675,25 +640,12 @@ impl Tty7App { .when(subject.label.is_none() && !subject_takes_the_slack, |bar| { bar.child(div().flex_1()) }) - .child(div().occlude().flex_shrink_0().child({ - let sf = cx.global::().window; - let selected = usize::from(view_mode(cx) == DiffViewMode::Unified); - self.segmented_on( - sf, - "diff-overlay-view", - &[t(L10nKey::DiffViewSplit), t(L10nKey::DiffViewUnified)], - selected, - cx, - |this, index, _window, cx| { - let mode = if index == 0 { - DiffViewMode::Split - } else { - DiffViewMode::Unified - }; - this.update_config(cx, |cfg| cfg.diff_view = mode); - }, - ) - })) + .child( + div() + .occlude() + .flex_shrink_0() + .child(self.diff_view_switch(cx)), + ) .child( div().occlude().flex_shrink_0().child( crate::ui::tab_strip::chrome_tile_sized( @@ -715,6 +667,54 @@ impl Tty7App { }) } + /// The two views, as a switch rather than a control. + /// + /// Not [`Tty7App::segmented_on`]: that one is a bordered track, which is + /// right in a settings row, where it ends a line of prose and has to + /// announce itself as something you operate. On a title bar it was the + /// only bordered thing on the strip — the close tile beside it is a bare + /// glyph, and so is every tile at the other end of the window — so it read + /// as pasted on. Same two choices, no frame: the live one carries a soft + /// fill, the other is quiet text that lights up under the pointer. + fn diff_view_switch(&self, cx: &mut Context) -> AnyElement { + let sf = cx.global::().window; + let current = view_mode(cx); + let cells = [ + (DiffViewMode::Split, t(L10nKey::DiffViewSplit)), + (DiffViewMode::Unified, t(L10nKey::DiffViewUnified)), + ]; + h_flex() + .id("diff-overlay-view") + .flex_shrink_0() + .gap(px(2.)) + .children(cells.into_iter().enumerate().map(|(i, (mode, label))| { + let live = mode == current; + h_flex() + .id(("diff-overlay-view-cell", i)) + .items_center() + .h(px(22.)) + .px(px(8.)) + .rounded(ROW_RADIUS) + .text_sm() + .cursor_pointer() + .when(live, |cell| { + cell.bg(gpui::rgb(sf.selected)) + .text_color(gpui::rgb(sf.text_selected)) + .font_weight(FontWeight::MEDIUM) + }) + .when(!live, |cell| { + cell.text_color(cx.theme().muted_foreground) + .hover(|h| h.bg(gpui::rgb(sf.hover))) + }) + .active(|cell| cell.bg(gpui::rgb(sf.pressed))) + .on_click(cx.listener(move |this, _, _window, cx| { + this.update_config(cx, |cfg| cfg.diff_view = mode); + })) + .child(label) + })) + .into_any_element() + } + /// Dispatch the byte read behind an untracked file's preview, at most /// once per (path, snapshot). Runs from `render`, so the guards are the /// point: `preview` says the answer is in hand, `preview_loading` says it @@ -788,33 +788,6 @@ impl Tty7App { ); } - /// The one synthesized card, in the same scroll shell the file list uses. - fn diff_preview_card( - &self, - file: &FileDiff, - scroll: &gpui::ScrollHandle, - cx: &mut Context, - ) -> AnyElement { - let mode = view_mode(cx); - let list = v_flex() - .gap_3() - .p_4() - .w_full() - // `usize::MAX` keeps the element ids clear of the real list's. - .child(self.diff_file_card(usize::MAX, file, true, mode, cx)); - crate::ui::scrollbar::with_vertical_scrollbar( - "diff-overlay-scrollbar", - div() - .id("diff-overlay-scroll") - .flex_1() - .min_h_0() - .overflow_y_scroll() - .track_scroll(scroll) - .child(list), - scroll, - ) - } - fn diff_message(&self, text: &'static str, cx: &Context) -> AnyElement { div() .flex_1() @@ -827,500 +800,627 @@ impl Tty7App { .into_any_element() } - fn diff_file_list( - &self, - snap: &DiffSnapshot, - expanded: &HashMap, - focused: Option, - scroll: &gpui::ScrollHandle, - cx: &mut Context, - ) -> AnyElement { - let stats = snap.stats(); + /// Brings the active overlay's flattened rows up to date with what it is + /// meant to be showing, and says what to draw. + /// + /// Called from `render`, so the [`RowsKey`] comparison is what keeps it + /// cheap: flattening a twenty-thousand-line patch allocates a row per + /// line, and nothing about that changes between two frames of scrolling. + fn sync_diff_rows(&mut self, cx: &mut Context) -> Option { let mode = view_mode(cx); - let oversized = focused.is_none() && stats.oversized; - let mut list = v_flex().gap_3().p_4().w_full(); - if oversized { - list = list.child(self.diff_oversized_notice(snap, &stats, cx)); - } - let shown = snap.files.len().min(MAX_RENDERED_FILES); - for (idx, file) in snap.files.iter().enumerate() { - if focused.is_some_and(|f| f != idx) { - continue; - } - if focused.is_none() && idx >= shown { - break; - } - let is_expanded = if focused == Some(idx) { - expanded.get(&file.path).copied().unwrap_or(true) - } else { - file_expanded(file, expanded, oversized) - }; - list = list.child(self.diff_file_card(idx, file, is_expanded, mode, cx)); - } - if focused.is_none() && snap.files.len() > shown { - let rest = snap.files.len() - shown; - list = list.child( - div() - .w_full() - .px_2p5() - .py_1p5() - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(t_plural(L10nKey::DiffMoreFiles, rest, &[])), - ); - } - if focused.is_none() && !snap.untracked.is_empty() { - list = list.child(self.diff_untracked_section(snap, cx)); - } - // A whole working tree can scroll past here with nothing to say how - // far it runs or where in it you are — the one long document in the - // app without the bar every other scroll area has. - crate::ui::scrollbar::with_vertical_scrollbar( - "diff-overlay-scrollbar", - div() - .id("diff-overlay-scroll") - .flex_1() - .min_h_0() - .overflow_y_scroll() - .track_scroll(scroll) - .child(list), - scroll, - ) - } + let active = self.active; + let overlay = self.tabs.get_mut(active)?.diff_overlay.as_mut()?; - fn diff_oversized_notice( - &self, - snap: &DiffSnapshot, - stats: &DiffStats, - cx: &Context, - ) -> AnyElement { - let text = t_fmt( - L10nKey::DiffOversizedNotice, - &[("summary", &oversized_summary(snap, stats))], - ); - div() - .w_full() - .px_2p5() - .py_2() - .rounded_md() - .border_1() - .border_color(cx.theme().border) - .bg(cx.theme().secondary) - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(text) - .into_any_element() - } - - fn diff_file_card( - &self, - idx: usize, - file: &FileDiff, - expanded: bool, - mode: DiffViewMode, - cx: &mut Context, - ) -> AnyElement { - let expandable = - !file.binary && (!file.hunks.is_empty() || file.truncated == Some(Truncation::Budget)); - let deco = deco_status(file.status); - let (glyph, glyph_color) = (status_glyph(deco), status_color(deco, cx)); - let shown_path = match &file.old_path { - Some(old) => format!("{old} → {}", file.path), - None => file.path.clone(), + let snap = match &overlay.load { + DiffLoad::Loading => return Some(DiffBody::Message(t(L10nKey::DiffReading))), + DiffLoad::NotARepo => return Some(DiffBody::Message(t(L10nKey::DiffNotARepo))), + DiffLoad::Ready(snap) if empty_snapshot(snap) && snap.read_failed => { + return Some(DiffBody::Message(t(L10nKey::DiffReadFailed))); + } + DiffLoad::Ready(snap) if empty_snapshot(snap) => { + return Some(DiffBody::Message(t(L10nKey::DiffWorkingTreeClean))); + } + DiffLoad::Ready(snap) => Arc::clone(snap), }; - let has_body = expanded && (!file.hunks.is_empty() || file.truncated.is_some()); + // A focused *untracked* file has no patch in the snapshot; its card is + // synthesized from the file's own bytes — see `preview`. + let preview = match untracked_focus(&snap, overlay.focus.as_deref()) { + Some(path) => match &overlay.preview { + Some((held, file)) if held == path => match file { + Some(file) => Some(Arc::clone(file)), + None => return Some(DiffBody::Message(t(L10nKey::DiffReadFailed))), + }, + _ => return Some(DiffBody::Message(t(L10nKey::DiffReading))), + }, + None => None, + }; - let header_corners = rounding::stack_corners( - 0, - if has_body { 2 } else { 1 }, - rounding::CARD_RADIUS, - rounding::HAIRLINE, - ); - let mut header = h_flex() - .id(("diff-file-header", idx)) - .w_full() - .items_center() - .gap_2() - .px_2p5() - .py_1p5() - .rounded_corners(header_corners) - .bg(cx.theme().secondary) - .when(expandable, |h| { - let path = file.path.clone(); - h.cursor_pointer() - .hover(|s| s.bg(cx.theme().list_hover)) - .on_click(cx.listener(move |this, _, _window, cx| { + let focused = focused_file(&snap, overlay); + let from = RowsFrom { + snap: &snap, + preview: preview.as_ref(), + mode, + focused, + oversized: focused.is_none() && snap.stats().oversized, + expanded: &overlay.expanded, + }; + let stale = overlay + .rows_key + .as_ref() + .is_none_or(|held| !held.describes(&from)); + if stale { + let rows = match from.preview { + Some(file) => crate::ui::diff_list::preview_rows(file, from.mode), + None => crate::ui::diff_list::build_rows( + from.snap, + from.expanded, + from.focused, + from.mode, + from.oversized, + ), + }; + let key = from.to_key(); + resync_list(&overlay.list, &overlay.rows, &rows); + overlay.rows = Rc::new(rows); + overlay.rows_key = Some(key); + } else if let Some(held) = overlay.rows_key.as_mut() { + held.retarget(&from); + } + Some(DiffBody::Rows(snap)) + } + + /// The rows, in the virtualised list that draws only the visible ones. + fn diff_rows_list( + &self, + overlay: &DiffOverlayState, + snap: Arc, + cx: &mut Context, + ) -> AnyElement { + let rows = Rc::clone(&overlay.rows); + let font = SharedString::from(self.font_family.clone()); + let app = cx.entity().downgrade(); + let list = overlay.list.clone(); + 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), + // 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. + None => div().into_any_element(), + } + }) + .size_full() + // Only the vertical padding: `List` lays every item out at its own + // full width and puts it at its own left edge, so a horizontal + // padding here would be silently ignored. The rows carry their own — + // see `diff_row_element`. + .py_4(); + // The bar reads the list's own height, and a list only counts the + // rows it has measured. Left at that, a patch of any length would + // report itself as one screen long and the thumb would fill the + // track: a drag from top to bottom would travel one screen and stop, + // on the one document in the app long enough to need the bar. The + // rows below the fold are counted at `DIFF_LINE_H` until they are + // laid out — `ListState::measure_all` would settle it exactly, by + // laying out every row on the first frame, which is the cost this + // whole list exists to avoid. + crate::ui::scrollbar::with_vertical_scrollbar("diff-overlay-scrollbar", body, &list) + } +} + +/// Counts the rows the list actually built, so a test can tell that a patch of +/// any size costs the handful of rows on screen rather than all of them. +#[cfg(test)] +pub(crate) mod row_probe { + use std::cell::Cell; + + thread_local! { + static BUILT: Cell = const { Cell::new(0) }; + } + + pub(crate) fn record() { + BUILT.set(BUILT.get() + 1); + } + + /// The count since the last call, and zero from here. + pub(crate) fn take() -> u64 { + BUILT.replace(0) + } +} + +/// What the overlay's scrolling area holds this frame. +enum DiffBody { + Message(&'static str), + /// The rows are in [`DiffOverlayState::rows`]; the snapshot rides along + /// for the few rows whose text is derived from it. + Rows(Arc), +} + +/// What this frame would flatten its rows from, borrowed from the overlay. +struct RowsFrom<'a> { + snap: &'a Arc, + preview: Option<&'a Arc>, + mode: DiffViewMode, + focused: Option, + oversized: bool, + expanded: &'a HashMap, +} + +impl RowsFrom<'_> { + fn to_key(&self) -> RowsKey { + RowsKey { + snap: Arc::clone(self.snap), + preview: self.preview.cloned(), + mode: self.mode, + focused: self.focused, + oversized: self.oversized, + expanded: self.expanded.clone(), + } + } +} + +/// What [`DiffOverlayState::rows`] was flattened from, kept so the next frame +/// can tell whether it would flatten the same rows again. +struct RowsKey { + snap: Arc, + preview: Option>, + mode: DiffViewMode, + focused: Option, + oversized: bool, + expanded: HashMap, +} + +impl RowsKey { + /// Whether the rows built from `from` would be the rows already held. + /// + /// The snapshot is compared by pointer first and by contents second: a + /// probe that found nothing new still lands a fresh `Arc` over an equal + /// snapshot, and rebuilding every row of the patch for that would undo the + /// point of keeping them. + /// + /// The scalars go first so that the walk of the patch behind that second + /// comparison is only ever paid to answer a question the cheap fields + /// have not already answered. + fn describes(&self, from: &RowsFrom<'_>) -> bool { + self.mode == from.mode + && self.focused == from.focused + && self.oversized == from.oversized + && self.expanded == *from.expanded + && self.same_preview(from) + && (Arc::ptr_eq(&self.snap, from.snap) || self.snap == *from.snap) + } + + /// The preview, by pointer and then by contents — a re-read of an + /// untracked file lands a fresh `Arc` over bytes that did not change. + fn same_preview(&self, from: &RowsFrom<'_>) -> bool { + match (&self.preview, from.preview) { + (None, None) => true, + (Some(a), Some(b)) => Arc::ptr_eq(a, b) || a == b, + _ => false, + } + } + + /// Points the key at the `Arc`s this frame was asked about, having just + /// found them equal to the ones held. + /// + /// Without this the key goes on holding the snapshot from the last + /// *rebuild*, so every frame after a probe that found nothing new proves + /// the two equal the long way — a walk of every line of the patch, once + /// per wheel event, which is the cost this key exists to avoid. + fn retarget(&mut self, from: &RowsFrom<'_>) { + self.snap = Arc::clone(from.snap); + self.preview = from.preview.cloned(); + } +} + +/// Tells the list which rows changed, rather than that all of them did. +/// +/// `ListState::reset` would drop the scroll position, so collapsing one file +/// would throw the reader back to the top of the tree. The rows either side of +/// an edit are untouched, so the shared prefix and suffix are kept and only +/// what is between them is spliced. +fn resync_list(list: &gpui::ListState, old: &[DiffRow], new: &[DiffRow]) { + let (replaced, with) = spliced_range(old, new); + list.splice(replaced, with); +} + +/// Which of the old rows were replaced, and by how many new ones. +fn spliced_range(old: &[DiffRow], new: &[DiffRow]) -> (std::ops::Range, usize) { + let prefix = old.iter().zip(new).take_while(|(a, b)| a == b).count(); + // Whatever is left of the shorter list once the shared head is off it — + // the most the shared tail can be, and what keeps the two slices below in + // step with each other. + let rest = old.len().min(new.len()) - prefix; + let suffix = old[old.len() - rest..] + .iter() + .rev() + .zip(new[new.len() - rest..].iter().rev()) + .take_while(|(a, b)| a == b) + .count(); + (prefix..old.len() - suffix, new.len() - prefix - suffix) +} + +/// The row inset every row of the list shares, matching the source control +/// panel's — the overlay is a second view of that panel's list, and the two +/// stopped looking like one app when this one drew cards. +const ROW_INSET: Pixels = px(10.); + +/// The height of a row that is a *file* rather than a line of one: the same +/// 26px the panel gives its file rows. +const FILE_ROW_H: Pixels = px(26.); + +/// The radius on a row that lights up under the pointer. Matches the panel's. +const ROW_RADIUS: Pixels = px(5.); + +/// The height of one line of a patch, in either view. +/// +/// Also what the list counts a row it has not laid out yet at. A list knows +/// only the rows it has measured, so without an estimate for the rest a patch +/// of any length reports itself as one screen long — and the scrollbar, which +/// reads that height, drags the reader one screen and stops. The file and hunk +/// rows are a few pixels taller, so the estimate runs a little short until +/// they have been measured; a diff is overwhelmingly its lines. +const DIFF_LINE_H: Pixels = px(19.); + +/// The rule between one hunk and the last line of the one before it. +/// +/// Barely there on purpose: with the cards gone it is the only line left in +/// the list, and it is separating two parts of one file rather than two +/// files. +fn hunk_rule(cx: &gpui::App) -> Hsla { + cx.theme().border.opacity(0.6) +} + +/// One row, inset the way every row in the list is. +fn diff_row_element( + row: &DiffRow, + font: &SharedString, + snap: &Arc, + app: &gpui::WeakEntity, + cx: &mut gpui::App, +) -> AnyElement { + match row { + // Stands in for the gap between the groups this list used to be a + // flex column of. + DiffRow::Gap => div().w_full().h(gpui::rems(0.75)).into_any_element(), + DiffRow::Oversized => padded(diff_oversized_notice(snap, cx)), + DiffRow::FileHeader(head) => padded(diff_file_header(head, font, app, cx)), + DiffRow::HunkHeader { text, leads } => padded( + div() + .w_full() + .px(ROW_INSET) + .py_1() + .when(!leads, |h| { + h.mt_1().border_t_1().border_color(hunk_rule(cx)) + }) + .text_xs() + .font_family(font.clone()) + .text_color(cx.theme().muted_foreground) + .truncate() + .child(text.clone()) + .into_any_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::Truncated(reason) => { + let note = match reason { + Truncation::PerFile => t_fmt( + L10nKey::DiffTruncatedPerFile, + &[("limit", &git_diff::MAX_LINES_PER_FILE.to_string())], + ), + Truncation::Budget => t(L10nKey::DiffTruncatedBudget).to_string(), + }; + padded(note_row(note, cx)) + } + DiffRow::MoreFiles { rest } => { + padded(note_row(t_plural(L10nKey::DiffMoreFiles, *rest, &[]), cx)) + } + // A section label, in the shape the sidebar gives its group headings: + // small, quiet, and carried by the space around it rather than a bar + // of its own. + DiffRow::UntrackedHeader { total } => padded( + div() + .w_full() + .px(ROW_INSET) + .py_1() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(t_plural(L10nKey::DiffUntrackedHeader, *total, &[])) + .into_any_element(), + ), + DiffRow::Untracked { index, path } => { + padded(diff_untracked_row(*index, path, font, app, cx)) + } + DiffRow::MoreUntracked { rest } => padded(note_row( + t_plural(L10nKey::DiffMoreUntracked, *rest, &[]), + cx, + )), + } +} + +/// 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() +} + +/// An aside in the list's own voice — a cap that was hit, a tail that was not +/// drawn. Never a row you can act on, so never one that lights up. +fn note_row(text: String, cx: &gpui::App) -> AnyElement { + div() + .w_full() + .px(ROW_INSET) + .py_1() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(text) + .into_any_element() +} + +fn diff_oversized_notice(snap: &DiffSnapshot, cx: &gpui::App) -> AnyElement { + let stats = snap.stats(); + let text = t_fmt( + L10nKey::DiffOversizedNotice, + &[("summary", &oversized_summary(snap, &stats))], + ); + div() + .w_full() + .px(ROW_INSET) + .py_2() + .rounded(rounding::CARD_RADIUS) + .bg(cx.theme().secondary) + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(text) + .into_any_element() +} + +fn diff_file_header( + head: &FileHead, + font: &SharedString, + app: &gpui::WeakEntity, + cx: &gpui::App, +) -> AnyElement { + let hover = gpui::rgb(cx.global::().window.hover); + let deco = deco_status(head.status); + let (glyph, glyph_color) = (status_glyph(deco), status_color(deco, cx)); + let mut header = h_flex() + .id(("diff-file-header", head.index)) + .w_full() + .items_center() + .gap_2() + .h(FILE_ROW_H) + .px(ROW_INSET) + .rounded(ROW_RADIUS) + .when(head.expandable, |h| { + let path = head.path.clone(); + let want = !head.expanded; + let app = app.clone(); + h.cursor_pointer() + .hover(|s| s.bg(hover)) + .on_click(move |_, _window, cx| { + let path = path.clone(); + app.update(cx, |this, cx| { let active = this.active; if let Some(overlay) = this .tabs .get_mut(active) .and_then(|t| t.diff_overlay.as_mut()) { - overlay.expanded.insert(path.clone(), !expanded); + overlay.expanded.insert(path, want); cx.notify(); } - })) - .child( - Icon::new(if expanded { - IconName::ChevronDown - } else { - IconName::ChevronRight - }) - .small() - .text_color(cx.theme().muted_foreground), - ) - }) - .child( - div() - .flex_shrink_0() - .font_family(self.font_family.clone()) - .text_xs() - .font_weight(FontWeight::BOLD) - .text_color(glyph_color) - .child(glyph), - ) - .child( - div() - .flex_1() - .min_w_0() - .truncate() - .text_xs() - .font_family(self.font_family.clone()) - .child(shown_path), - ); - if file.binary { - header = header.child( - div() - .flex_shrink_0() - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(t(L10nKey::Binary)), - ); - } - if file.added > 0 { - header = header.child( - div() - .flex_shrink_0() - .text_xs() - .text_color(cx.theme().success) - .child(format!("+{}", file.added)), - ); - } - if file.removed > 0 { - header = header.child( - div() - .flex_shrink_0() - .text_xs() - .text_color(cx.theme().danger) - .child(format!("−{}", file.removed)), - ); - } - - let mut card = v_flex() - .w_full() - .border_1() - .border_color(cx.theme().border) - .rounded(rounding::CARD_RADIUS) - .overflow_hidden() - .child(header); - - if has_body { - let mut body = v_flex().w_full(); - let hunks: Vec<_> = file - .hunks - .iter() - .map(|hunk| { - let rows = match mode { - DiffViewMode::Split => HunkRows::Split(split_hunk(&hunk.lines)), - DiffViewMode::Unified => HunkRows::Unified(unified_rows(&hunk.lines)), - }; - (hunk, rows) + }) + .ok(); }) - .collect(); - let closing_row = if file.truncated.is_some() { - None - } else { - hunks - .iter() - .rposition(|(_, rows)| !rows.is_empty()) - .map(|h| (h, hunks[h].1.len() - 1)) - }; - for (h, (hunk, rows)) in hunks.iter().enumerate() { - body = body.child( - div() - .w_full() - .px_2() - .py_0p5() - .bg(cx.theme().muted) - .text_xs() - .font_family(self.font_family.clone()) - .text_color(cx.theme().muted_foreground) - .truncate() - .child(hunk.header.clone()), - ); - match rows { - HunkRows::Split(rows) => { - for (r, row) in rows.iter().enumerate() { - body = body.child(self.diff_split_row( - row, - closing_row == Some((h, r)), - cx, - )); - } - } - HunkRows::Unified(rows) => { - for (r, row) in rows.iter().enumerate() { - body = body.child(self.diff_unified_row( - row, - closing_row == Some((h, r)), - cx, - )); - } - } - } - } - if let Some(reason) = file.truncated { - let note = match reason { - Truncation::PerFile => t_fmt( - L10nKey::DiffTruncatedPerFile, - &[("limit", &git_diff::MAX_LINES_PER_FILE.to_string())], - ), - Truncation::Budget => t(L10nKey::DiffTruncatedBudget).to_string(), + .child( + Icon::new(if head.expanded { + IconName::ChevronDown + } else { + IconName::ChevronRight + }) + .small() + .text_color(cx.theme().muted_foreground), + ) + }) + .child( + div() + .flex_shrink_0() + .font_family(font.clone()) + .text_xs() + .font_weight(FontWeight::BOLD) + .text_color(glyph_color) + .child(glyph), + ) + .child( + div() + .flex_1() + .min_w_0() + .truncate() + .text_xs() + .font_family(font.clone()) + .child(head.shown_path.clone()), + ); + if head.binary { + header = header.child( + div() + .flex_shrink_0() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(t(L10nKey::Binary)), + ); + } + if head.added > 0 { + header = header.child( + div() + .flex_shrink_0() + .text_xs() + .text_color(cx.theme().success) + .child(format!("+{}", head.added)), + ); + } + if head.removed > 0 { + header = header.child( + div() + .flex_shrink_0() + .text_xs() + .text_color(cx.theme().danger) + .child(format!("−{}", head.removed)), + ); + } + header.into_any_element() +} + +// An untracked file has no patch in the snapshot, so it cannot be expanded in +// place the way the files above it are — its contents are read one file at a +// time and shown on their own. The row asks for that read, which until now +// only the Source Control panel could: in the overlay these rows were the only +// files in a list of files that did nothing when clicked. +fn diff_untracked_row( + index: usize, + path: &str, + font: &SharedString, + app: &gpui::WeakEntity, + cx: &gpui::App, +) -> AnyElement { + let hover = gpui::rgb(cx.global::().window.hover); + let for_focus = path.to_string(); + let app = app.clone(); + h_flex() + .id(("diff-untracked", index)) + .w_full() + .items_center() + .gap_2() + .h(FILE_ROW_H) + .px(ROW_INSET) + .rounded(ROW_RADIUS) + .text_xs() + .font_family(font.clone()) + .cursor_pointer() + .hover(|s| s.bg(hover)) + .on_click(move |_, window, cx| { + let for_focus = for_focus.clone(); + app.update(cx, |this, cx| { + let Some((host, cwd, source)) = this + .tabs + .get(this.active) + .and_then(|t| t.diff_overlay.as_ref()) + .map(|o| (o.host_id, o.cwd.clone(), o.source.clone())) + else { + return; }; - body = body.child( - div() - .w_full() - .px_2() - .py_1() - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(note), - ); - } - card = card.child(body); - } - card.into_any_element() - } + this.open_diff_overlay(host, cwd, source, Some(for_focus), window, cx); + }) + .ok(); + }) + .child( + div() + .flex_shrink_0() + .font_weight(FontWeight::BOLD) + .text_color(status_color(DecoStatus::Untracked, cx)) + .child(status_glyph(DecoStatus::Untracked)), + ) + .child(div().flex_1().min_w_0().truncate().child(path.to_string())) + .into_any_element() +} - fn diff_split_row(&self, row: &SplitRow, closes_card: bool, cx: &Context) -> AnyElement { - let radius = if closes_card { - rounding::inner_radius(rounding::CARD_RADIUS, rounding::HAIRLINE) - } else { - px(0.) - }; - h_flex() - .w_full() - .h(px(19.)) - .items_stretch() - .text_xs() - .font_family(self.font_family.clone()) - .child(self.diff_split_cell(row.left.as_ref(), Side::Old, radius, cx)) - .child(div().flex_shrink_0().w(px(1.)).bg(cx.theme().border)) - .child(self.diff_split_cell(row.right.as_ref(), Side::New, radius, cx)) - .into_any_element() - } +fn diff_split_row(row: &SplitRow, font: &SharedString, cx: &gpui::App) -> impl IntoElement { + 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(div().flex_shrink_0().w(px(1.)).bg(hunk_rule(cx))) + .child(diff_split_cell(row.right.as_ref(), Side::New, cx)) +} - fn diff_split_cell( - &self, - cell: Option<&SplitCell>, - side: Side, - outer_radius: Pixels, - cx: &Context, - ) -> AnyElement { - let base = h_flex().flex_1().min_w_0().h_full().items_center(); - let base = match side { - Side::Old => base.rounded_bl(outer_radius), - Side::New => base.rounded_br(outer_radius), - }; - let Some(cell) = cell else { - return base.bg(cx.theme().muted.opacity(0.3)).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)) - .child( - h_flex() - .flex_shrink_0() - .w(px(42.)) - .justify_end() - .pr_1p5() - .text_color(cx.theme().muted_foreground.opacity(0.7)) - .child(cell.no.map(|n| n.to_string()).unwrap_or_default()), - ) - .child( - div() - .flex_1() - .min_w_0() - .truncate() - .child(format!("{marker} {}", cell.text)), - ) - .into_any_element() - } - - /// One line of the unified view. - /// - /// Every measurement it shares with [`Self::diff_split_cell`] is shared on - /// purpose — the same 19px row, the same `text_xs` in the same family, and - /// above all the same `0.12` wash behind an addition and a removal. The two - /// views are one diff seen twice; a different green would read as a - /// different thing. - /// - /// What differs is forced by the shape. The line numbers get 34px a side - /// rather than 42 (there are two gutters here in front of one column of - /// text, not one in front of each), and the `+`/`−` gets a column of its - /// own rather 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( - &self, - row: &UnifiedRow, - closes_card: bool, - cx: &Context, - ) -> AnyElement { - let radius = if closes_card { - rounding::inner_radius(rounding::CARD_RADIUS, rounding::HAIRLINE) - } else { - px(0.) - }; - 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))), - LineKind::Context => (cx.theme().muted_foreground, None), - }; - let gutter = |no: Option| { +fn diff_split_cell(cell: Option<&SplitCell>, side: Side, 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(); + }; + 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)) + .child( h_flex() .flex_shrink_0() - .w(px(34.)) + .w(px(42.)) .justify_end() .pr_1p5() .text_color(cx.theme().muted_foreground.opacity(0.7)) - .child(no.map(|n| n.to_string()).unwrap_or_default()) - }; - h_flex() - .w_full() - .h(px(19.)) - .items_center() - .text_xs() - .font_family(self.font_family.clone()) - .rounded_bl(radius) - .rounded_br(radius) - .when_some(tint, |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 - // same thing: everything left of it is a number, everything right - // of it is the file. - .child( - div() - .flex_shrink_0() - .w(px(1.)) - .h_full() - .bg(cx.theme().border), - ) - .child( - div() - .flex_shrink_0() - .w(px(12.)) - .text_center() - .text_color(marker_color) - .child(unified_marker(row.kind)), - ) - .child(div().flex_1().min_w_0().truncate().child(row.text.clone())) - .into_any_element() - } + .child(cell.no.map(|n| n.to_string()).unwrap_or_default()), + ) + .child( + div() + .flex_1() + .min_w_0() + .truncate() + .child(format!("{marker} {}", cell.text)), + ) + .into_any_element() +} - fn diff_untracked_section(&self, snap: &DiffSnapshot, cx: &Context) -> AnyElement { - let total = snap.untracked_count(); - let untracked = &snap.untracked[..snap.untracked.len().min(MAX_RENDERED_FILES)]; - let header_corners = rounding::stack_corners( - 0, - if total == 0 { 1 } else { 2 }, - rounding::CARD_RADIUS, - rounding::HAIRLINE, - ); - let mut section = v_flex() - .w_full() - .border_1() - .border_color(cx.theme().border) - .rounded(rounding::CARD_RADIUS) - .overflow_hidden() - .child( - div() - .w_full() - .px_2p5() - .py_1p5() - .rounded_corners(header_corners) - .bg(cx.theme().secondary) - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(t_plural(L10nKey::DiffUntrackedHeader, total, &[])), - ); - for (i, path) in untracked.iter().enumerate() { - // An untracked file has no patch in the snapshot, so it cannot be - // expanded in place the way the cards above it are — its contents - // are read one file at a time and shown on their own. The row - // asks for that read, which until now only the Source Control - // panel could: in the overlay these rows were the only files in a - // list of files that did nothing when clicked. - let for_focus = path.clone(); - section = section.child( - h_flex() - .id(("diff-untracked", i)) - .w_full() - .items_center() - .gap_2() - .px_2p5() - .py_1() - .text_xs() - .font_family(self.font_family.clone()) - .cursor_pointer() - .hover(|s| s.bg(cx.theme().secondary)) - .on_click(cx.listener(move |this, _, window, cx| { - let Some((host, cwd, source)) = this - .tabs - .get(this.active) - .and_then(|t| t.diff_overlay.as_ref()) - .map(|o| (o.host_id, o.cwd.clone(), o.source.clone())) - else { - return; - }; - this.open_diff_overlay( - host, - cwd, - source, - Some(for_focus.clone()), - window, - cx, - ); - })) - .child( - div() - .flex_shrink_0() - .font_weight(FontWeight::BOLD) - .text_color(status_color(DecoStatus::Untracked, cx)) - .child(status_glyph(DecoStatus::Untracked)), - ) - .child(div().flex_1().min_w_0().truncate().child(path.clone())), - ); - } - if total > untracked.len() { - let rest = total - untracked.len(); - section = section.child( - div() - .w_full() - .px_2p5() - .py_1() - .text_xs() - .text_color(cx.theme().muted_foreground) - .child(t_plural(L10nKey::DiffMoreUntracked, rest, &[])), - ); - } - section.into_any_element() - } +/// One line of the unified view. +/// +/// Every measurement it shares with [`diff_split_cell`] is shared on purpose — +/// the same 19px row, the same `text_xs` in the same family, and above all the +/// same `0.12` wash behind an addition and a removal. The two views are one +/// diff seen twice; a different green would read as a different thing. +/// +/// What differs is forced by the shape. The line numbers get 34px a side +/// rather than 42 (there are two gutters here in front of one column of text, +/// not one in front of each), and the `+`/`−` gets a column of its own rather +/// 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 { + 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))), + LineKind::Context => (cx.theme().muted_foreground, None), + }; + let gutter = |no: Option| { + h_flex() + .flex_shrink_0() + .w(px(34.)) + .justify_end() + .pr_1p5() + .text_color(cx.theme().muted_foreground.opacity(0.7)) + .child(no.map(|n| n.to_string()).unwrap_or_default()) + }; + h_flex() + .w_full() + .h(DIFF_LINE_H) + .items_center() + .text_xs() + .font_family(font.clone()) + .when_some(tint, |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 + // same thing: everything left of it is a number, everything right of + // it is the file. + .child(div().flex_shrink_0().w(px(1.)).h_full().bg(hunk_rule(cx))) + .child( + div() + .flex_shrink_0() + .w(px(12.)) + .text_center() + .text_color(marker_color) + .child(unified_marker(row.kind)), + ) + .child(div().flex_1().min_w_0().truncate().child(row.text.clone())) } /// Which layout the overlay draws. One setting for the window, not one per @@ -1480,13 +1580,6 @@ fn empty_snapshot(snap: &DiffSnapshot) -> bool { snap.files.is_empty() && snap.untracked.is_empty() } -fn file_expanded(file: &FileDiff, expanded: &HashMap, collapse_all: bool) -> bool { - if let Some(&want) = expanded.get(&file.path) { - return want; - } - !collapse_all && file.added + file.removed <= AUTO_COLLAPSE_LINES -} - fn oversized_summary(snap: &DiffSnapshot, stats: &DiffStats) -> String { let mut parts = vec![t_plural(L10nKey::DiffChangedFiles, snap.files.len(), &[])]; let (added, removed) = stats.totals; @@ -1544,7 +1637,9 @@ fn probe_key( #[cfg(test)] mod tests { use super::*; - use crate::terminal::git_diff::{DiffLine, LineKind}; + use crate::terminal::git_diff::{AUTO_COLLAPSE_LINES, DiffLine, LineKind, MAX_RENDERED_FILES}; + use crate::ui::diff_list::{build_rows, file_expanded}; + use crate::ui::diff_rows::split_hunk; use crate::ui::i18n::set_locale; #[test] @@ -2000,6 +2095,109 @@ mod tests { ); } + /// `ListState::reset` would drop the scroll position, so opening or + /// closing one file in a long tree would throw the reader back to the top. + /// Only the rows that actually changed are spliced. + #[test] + fn only_the_rows_that_changed_are_spliced() { + let snap = DiffSnapshot { + files: vec![ + small_file("a.rs", 2), + small_file("b.rs", 2), + small_file("c.rs", 2), + ], + ..Default::default() + }; + let shut = |path: &str| -> HashMap { + [(path.to_string(), false)].into_iter().collect() + }; + let open = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + let middle_shut = build_rows(&snap, &shut("b.rs"), None, DiffViewMode::Unified, false); + + let (replaced, with) = spliced_range(&open, &middle_shut); + assert_eq!( + (replaced.start, with), + (5, 1), + "the first card and the gap after it are untouched" + ); + assert_eq!( + open.len() - replaced.end, + middle_shut.len() - (replaced.start + with), + "and so is everything below the file that closed" + ); + + let (replaced, with) = spliced_range(&open, &open); + assert_eq!( + (replaced.start, replaced.end, with), + (open.len(), open.len(), 0) + ); + } + + #[test] + fn a_list_that_was_empty_or_becomes_empty_splices_in_one_piece() { + let snap = DiffSnapshot { + files: vec![small_file("a.rs", 2)], + ..Default::default() + }; + let rows = build_rows(&snap, &HashMap::new(), None, DiffViewMode::Unified, false); + + let (replaced, with) = spliced_range(&[], &rows); + assert_eq!((replaced.start, replaced.end, with), (0, 0, rows.len())); + + let (replaced, with) = spliced_range(&rows, &[]); + assert_eq!((replaced.start, replaced.end, with), (0, rows.len(), 0)); + } + + /// A probe that found nothing new still lands a fresh `Arc` over an equal + /// snapshot. The rows are rightly kept — and the key has to come away + /// holding the `Arc` that landed, or every frame from then on proves the + /// two equal the long way: a walk of every line of the patch, per wheel + /// event. + #[test] + fn an_equal_snapshot_leaves_the_key_pointing_at_the_one_that_landed() { + let held = Arc::new(DiffSnapshot { + files: vec![small_file("a.rs", 2)], + ..Default::default() + }); + let landed = Arc::new(DiffSnapshot { + files: vec![small_file("a.rs", 2)], + ..Default::default() + }); + assert!( + !Arc::ptr_eq(&held, &landed), + "two separate Arcs over equal contents" + ); + + let expanded = HashMap::new(); + let mut key = RowsFrom { + snap: &held, + preview: None, + mode: DiffViewMode::Unified, + focused: None, + oversized: false, + expanded: &expanded, + } + .to_key(); + let landed_from = RowsFrom { + snap: &landed, + preview: None, + mode: DiffViewMode::Unified, + focused: None, + oversized: false, + expanded: &expanded, + }; + + assert!( + key.describes(&landed_from), + "nothing about the rows changed" + ); + key.retarget(&landed_from); + assert!( + Arc::ptr_eq(&key.snap, &landed), + "so the next frame settles it by pointer rather than by contents" + ); + } + #[test] fn a_focused_untracked_file_asks_for_a_preview_not_the_list() { let snap = DiffSnapshot { @@ -2490,6 +2688,128 @@ mod render_idle_gpui_tests { assert!(out.status.success(), "git {args:?} failed"); } + /// The whole point of the flattened row list: a patch of any size costs + /// the rows on screen, not the rows in the patch. + /// + /// Before this, the overlay built a card per file and an element per line + /// on every frame, and gpui notifies the view on every scroll wheel event + /// — so a few hundred lines of diff rebuilt tens of thousands of elements + /// tens of times a second, and the window visibly stalled. + #[gpui::test] + fn a_long_patch_builds_only_the_rows_on_screen(cx: &mut TestAppContext) { + const LINES: usize = 800; + + let root = std::env::temp_dir().join(format!("tty7-diff-rows-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let root = std::fs::canonicalize(&root).unwrap(); + git(&root, &["init", "--quiet"]); + let before: String = (0..LINES).map(|i| format!("line {i}\n")).collect(); + std::fs::write(root.join("long.txt"), &before).unwrap(); + git(&root, &["add", "long.txt"]); + git( + &root, + &[ + "-c", + "user.email=t@x", + "-c", + "user.name=t", + "commit", + "-qm", + "one", + ], + ); + // Every line rewritten, so the patch is a removal and an addition per + // line rather than a handful of hunks in a sea of context. + let after: String = (0..LINES).map(|i| format!("edited {i}\n")).collect(); + std::fs::write(root.join("long.txt"), &after).unwrap(); + + let (app, mut vcx, _pane) = test_window::harness_with_tabs(cx, 1); + let open = root.clone(); + app.update_in(&mut vcx, |app, window, cx| { + app.open_diff_overlay(HostId::LOCAL, open, DiffSource::Head, None, window, cx); + }); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + loop { + vcx.background_executor.run_until_parked(); + let ready = app.update_in(&mut vcx, |app, _, _| { + app.tabs[app.active] + .diff_overlay + .as_ref() + .is_some_and(|o| matches!(o.load, DiffLoad::Ready(_)) && !o.loading) + }); + if ready { + break; + } + assert!( + std::time::Instant::now() < deadline, + "the overlay never landed a snapshot" + ); + std::thread::sleep(std::time::Duration::from_millis(20)); + } + // 1600 changed lines is well past the auto-collapse threshold, so the + // card starts shut. Open it: a reader opening a large file is exactly + // the frame this is about. + app.update_in(&mut vcx, |app, _, cx| { + let active = app.active; + app.tabs[active] + .diff_overlay + .as_mut() + .expect("the overlay is open") + .expanded + .insert("long.txt".to_string(), true); + cx.notify(); + }); + vcx.background_executor.run_until_parked(); + + let rows = app.update_in(&mut vcx, |app, _, _| { + app.tabs[app.active] + .diff_overlay + .as_ref() + .map(|o| o.rows.len()) + .unwrap_or(0) + }); + assert!( + rows > LINES, + "the patch flattens to a row per changed line ({rows} rows)" + ); + + row_probe::take(); + app.update_in(&mut vcx, |_, _, cx| cx.notify()); + vcx.background_executor.run_until_parked(); + let built = row_probe::take(); + assert!( + built > 0, + "the list drew something — a probe that counts nothing proves nothing" + ); + assert!( + built < rows as u64 / 4, + "a frame built {built} of {rows} rows: the list is not virtualised" + ); + + // The other half of virtualising: a list counts the rows it has not + // laid out at zero unless it is given an estimate, and the scrollbar + // reads that count as the length of the document. Without the size + // hint the bar reaches 248px into this patch — its thumb fills the + // track, and dragging it to the bottom lands a screen down. + let reach = app.update_in(&mut vcx, |app, _, _| { + app.tabs[app.active] + .diff_overlay + .as_ref() + .expect("the overlay is open") + .list + .max_offset_for_scrollbar() + .y + }); + assert!( + reach > DIFF_LINE_H * (rows as f32 * 0.75), + "the scrollbar reaches {reach} into a patch of {rows} rows" + ); + + let _ = std::fs::remove_dir_all(&root); + } + #[gpui::test] fn an_overlay_over_a_stale_branch_reaches_render_idle(cx: &mut TestAppContext) { let root = std::env::temp_dir().join(format!("tty7-diff-idle-{}", std::process::id())); diff --git a/src/ui/diff_rows.rs b/src/ui/diff_rows.rs index 4aa16876..b5316d50 100644 --- a/src/ui/diff_rows.rs +++ b/src/ui/diff_rows.rs @@ -23,12 +23,14 @@ pub(crate) enum Side { New, } +#[derive(PartialEq, Eq)] pub(crate) struct SplitCell { pub(crate) no: Option, pub(crate) text: String, pub(crate) changed: bool, } +#[derive(PartialEq, Eq)] pub(crate) struct SplitRow { pub(crate) left: Option, pub(crate) right: Option, @@ -85,6 +87,7 @@ pub(crate) fn split_hunk(lines: &[DiffLine]) -> Vec { rows } +#[derive(PartialEq, Eq)] pub(crate) struct UnifiedRow { pub(crate) old: Option, pub(crate) new: Option, diff --git a/src/ui/mod.rs b/src/ui/mod.rs index 31f360c8..149da06b 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -1,6 +1,7 @@ pub mod app; pub mod assets; pub mod code_editor; +pub mod diff_list; pub mod diff_overlay; pub mod diff_rows; pub mod document_column; diff --git a/src/ui/rounding.rs b/src/ui/rounding.rs index 86d0ad44..87b3dd7f 100644 --- a/src/ui/rounding.rs +++ b/src/ui/rounding.rs @@ -40,24 +40,6 @@ pub(crate) fn segment_corners( } } -pub(crate) fn stack_corners( - i: usize, - count: usize, - outer: Pixels, - border: Pixels, -) -> Corners { - let r = inner_radius(outer, border); - let zero = px(0.); - let first = i < count && i == 0; - let last = i < count && i + 1 == count; - Corners { - top_left: if first { r } else { zero }, - top_right: if first { r } else { zero }, - bottom_left: if last { r } else { zero }, - bottom_right: if last { r } else { zero }, - } -} - #[cfg(test)] mod tests { use super::*; @@ -109,20 +91,4 @@ mod tests { Corners::all(px(0.)) ); } - - #[test] - fn a_stack_caps_its_first_and_last_band() { - let r = inner_radius(CARD_RADIUS, HAIRLINE); - let zero = px(0.); - - let top = stack_corners(0, 2, CARD_RADIUS, HAIRLINE); - assert_eq!((top.top_left, top.top_right), (r, r)); - assert_eq!((top.bottom_left, top.bottom_right), (zero, zero)); - - let bottom = stack_corners(1, 2, CARD_RADIUS, HAIRLINE); - assert_eq!((bottom.bottom_left, bottom.bottom_right), (r, r)); - assert_eq!((bottom.top_left, bottom.top_right), (zero, zero)); - - assert_eq!(stack_corners(0, 1, CARD_RADIUS, HAIRLINE), Corners::all(r)); - } } diff --git a/src/ui/scrollbar.rs b/src/ui/scrollbar.rs index 3e6efea4..5a8102f0 100644 --- a/src/ui/scrollbar.rs +++ b/src/ui/scrollbar.rs @@ -1,5 +1,5 @@ use gpui::{AnyElement, ElementId, Pixels, ScrollHandle, div, prelude::*, px}; -use gpui_component::scroll::Scrollbar; +use gpui_component::scroll::{Scrollbar, ScrollbarHandle}; use gpui_component::v_flex; /// Overlays the shared vertical scrollbar on a scroll area. @@ -9,10 +9,10 @@ use gpui_component::v_flex; /// dropping it straight into whatever is around it. Without that the wrapper /// sizes to its content, the `size_full` scroll area inside grows with it, and /// the pane stops scrolling because nothing overflows any more. -pub(crate) fn with_vertical_scrollbar( +pub(crate) fn with_vertical_scrollbar( id: impl Into, scroll_area: impl IntoElement, - handle: &ScrollHandle, + handle: &H, ) -> AnyElement { with_inset_vertical_scrollbar(id, scroll_area, handle, px(0.)) } @@ -23,10 +23,10 @@ pub(crate) fn with_vertical_scrollbar( /// the border is already the edge. A scroll area that *is* the window has no /// such edge, and a bar that runs to the last pixel lands on the rounded /// corner and reads as if it had been clipped. -pub(crate) fn with_inset_vertical_scrollbar( +pub(crate) fn with_inset_vertical_scrollbar( id: impl Into, scroll_area: impl IntoElement, - handle: &ScrollHandle, + handle: &H, inset_y: Pixels, ) -> AnyElement { v_flex() From cb710c4d7984aa9d93b9a4a57c565361d972707e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:23:28 +0800 Subject: [PATCH 06/15] deps: bump async_zip from 0.0.18 to 0.0.19 (#764) Also bump the version requirement in Cargo.toml, which dependabot left at 0.0.18 and which made every --locked job fail. --- Cargo.lock | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- Cargo.toml | 2 +- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a0f31ea1..5f3168d5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -376,6 +376,12 @@ dependencies = [ "password-hash", ] +[[package]] +name = "array-init" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d62b7694a562cdf5a74227903507c56ab2cc8bdd1f781ed5cb4cf9c9f810bfc" + [[package]] name = "arraydeque" version = "0.5.1" @@ -681,6 +687,20 @@ dependencies = [ "thiserror 2.0.20", ] +[[package]] +name = "async_zip" +version = "0.0.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fb7f5f40e1eb30949a266fc900d37fd3267c7baf50c3705ac09c5d8ced5def63" +dependencies = [ + "async-compression", + "binrw", + "crc32fast", + "futures-lite 2.6.1", + "pin-project", + "thiserror 2.0.20", +] + [[package]] name = "atk" version = "0.18.2" @@ -902,6 +922,30 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "binrw" +version = "0.15.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ad120d555272286c1017d25165ab8bd74806f13fc85b258484ec7e4ce75458f" +dependencies = [ + "array-init", + "binrw_derive", + "bytemuck", +] + +[[package]] +name = "binrw_derive" +version = "0.15.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6df92e0e9baae4dc82c7bad7715ca40c0a5c71539057bf2ea04a5c29c980410b" +dependencies = [ + "either", + "owo-colors", + "proc-macro2", + "quote", + "syn 2.0.119", +] + [[package]] name = "bit-set" version = "0.8.0" @@ -6006,6 +6050,12 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "owo-colors" +version = "4.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13c45bb4a6ae1280ec0803b1ef9d3455eb50f01efbbe1447ab020f1d54fba9d8" + [[package]] name = "p256" version = "0.14.0-rc.15" @@ -9779,7 +9829,7 @@ version = "26.9.1" dependencies = [ "alacritty_terminal", "anyhow", - "async_zip", + "async_zip 0.0.19", "core-foundation 0.10.0", "gpui", "gpui-component", @@ -10112,7 +10162,7 @@ source = "git+https://github.com/l0ng-ai/zed?branch=tty7#ece710e365575bc08656c4a dependencies = [ "anyhow", "async-fs", - "async_zip", + "async_zip 0.0.18", "collections", "command-fds", "dirs", diff --git a/Cargo.toml b/Cargo.toml index 48f90ceb..84a83f5b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -172,7 +172,7 @@ windows-sys = { version = "0.61", features = [ # The standalone updater extracts only Windows release ZIPs. Reuse the async # reader already present in Cargo.lock and enable only the Deflate codec emitted # by PowerShell's Compress-Archive. -async_zip = { version = "0.0.18", default-features = false, features = ["deflate"], optional = true } +async_zip = { version = "0.0.19", default-features = false, features = ["deflate"], optional = true } # COM for toast branding (`core::aumid`): IShellLinkW + IPropertyStore stamp # System.AppUserModel.ID onto the Start Menu shortcut, which Windows requires From d8a0245e7560ba7a484e568c76575af1668c9f7a Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:26:39 +0800 Subject: [PATCH 07/15] fix(terminal): name every ligature feature off, not just calt (#788) --- docs/customization/fonts.mdx | 13 ++++- src/terminal/element.rs | 94 +++++++++++++++++++++++++++++++----- 2 files changed, 95 insertions(+), 12 deletions(-) diff --git a/docs/customization/fonts.mdx b/docs/customization/fonts.mdx index 6be3ea61..3e1b59f5 100644 --- a/docs/customization/fonts.mdx +++ b/docs/customization/fonts.mdx @@ -14,7 +14,7 @@ description: "The bundled default, fallback chains, ligatures, and why CJK needs | **Interface font family** | system UI font | The face for everything outside the grid | | **Line height** | 1.4 | A multiple of the font size | | **Bold font** / **Italic font** | — | Distinct faces, when you want them | -| **Font ligatures** | off | Contextual alternates stay off unless you ask | +| **Font ligatures** | off | `calt`, `liga` and `clig` all stay off unless you ask | ## Hack is bundled @@ -60,6 +60,17 @@ A tag must be four alphanumeric characters; `true`/`false` map to `1`/`0`. Anything malformed is skipped with a log line rather than failing the whole config. +Writing *any* tag hands the whole feature set to you: the terminal only names +`calt: 0`, `liga: 0` and `clig: 0` itself while `font_features` is unset. So +`{"font_features": {"zero": 1}}` turns slashed zeroes on **and** ligatures back +on. Name them explicitly if you want both: + +```json +{ + "font_features": { "zero": 1, "calt": 0, "liga": 0, "clig": 0 } +} +``` + ## CJK and the two-column grid diff --git a/src/terminal/element.rs b/src/terminal/element.rs index 6eee0416..5dc1da49 100644 --- a/src/terminal/element.rs +++ b/src/terminal/element.rs @@ -149,11 +149,39 @@ fn build_font(base: &Font, bold: bool, italic: bool) -> Font { FontStyle::Normal }; if f.features.tag_value_list().is_empty() { - f.features = gpui::FontFeatures::disable_ligatures(); + f.features = ligatures_off(); } f } +/// The feature set that actually turns ligatures off in a terminal grid. +/// +/// gpui's own `FontFeatures::disable_ligatures()` emits `calt: 0` and nothing +/// else. `calt` is only the contextual-alternate feature a programming face +/// drives `=>` and `!=` from; the `fi`/`ffl`/`ffi` ligatures live in `liga` and +/// `clig`, which that call never mentions — so they were left at the shaper's +/// default, and CoreText, DirectWrite and rustybuzz all default them on. A +/// terminal cannot have them: a ligature is one glyph where the grid budgeted a +/// cell per character, so the row drifts. +/// +/// Naming all three is therefore the whole fix, and it is not platform +/// specific. Windows is the sharpest case only because gpui's DirectWrite +/// backend always attaches an `IDWriteTypography` to the run: an empty feature +/// list leaves that object empty, which suppresses DirectWrite's own defaults, +/// while any non-empty list has `liga: 1`/`clig: 1` appended to it +/// (`gpui_windows/src/direct_write.rs`, `apply_font_features`). Asking for +/// `calt: 0` there bought ligatures that asking for nothing would not have. +/// +/// `rlig` is deliberately left alone: it carries the required ligatures a +/// script cannot be written without. +fn ligatures_off() -> gpui::FontFeatures { + gpui::FontFeatures(std::sync::Arc::new(vec![ + ("calt".to_string(), 0), + ("liga".to_string(), 0), + ("clig".to_string(), 0), + ])) +} + fn snapshot_cell( cell: &Cell, point: AlacPoint, @@ -2717,20 +2745,64 @@ mod tests { assert_eq!(seg_budget(false, 1, false, cell), px(12.5)); } + /// The emitted feature set is the whole contract with the shaper, so pin it + /// tag for tag rather than asking after one tag at a time. + /// + /// `calt: 0` alone is not "ligatures off" on any platform: it never asks + /// for `liga`/`clig`, which every shaper defaults on. Shaping "office + /// waffle fluffier" in Calibri through the real DirectWrite shaper gives 15 + /// glyphs for 22 characters with `calt: 0`, and 22 once all three are named + /// zero. #[test] - fn build_font_disables_ligatures_unless_features_are_configured() { - let font = build_font(&gpui::font("Test"), false, false); - assert_eq!(font.features.is_calt_enabled(), Some(false)); - - let mut configured = gpui::font("Test"); - configured.features = serde_json::from_str(r#"{"calt":true,"liga":1}"#).unwrap(); - let font = build_font(&configured, false, false); - assert_eq!(font.features.is_calt_enabled(), Some(true)); - assert!( + fn build_font_emits_the_pinned_feature_set_for_each_ligature_setting() { + fn tags(font: &Font) -> Vec<(&str, u32)> { font.features .tag_value_list() .iter() - .any(|(tag, value)| tag == "liga" && *value == 1) + .map(|(tag, value)| (tag.as_str(), *value)) + .collect() + } + + // Default config, and the settings toggle switched off: both leave + // `font_features` unset, so the terminal names its own. + let font = build_font(&gpui::font("Test"), false, false); + assert_eq!( + tags(&font), + vec![("calt", 0), ("liga", 0), ("clig", 0)], + "an unconfigured face must name every ligature feature off", + ); + assert_eq!(font.features.is_calt_enabled(), Some(false)); + + // Every face the grid paints with gets the same set, not just the plain one. + for (bold, italic) in [(true, false), (false, true), (true, true)] { + assert_eq!( + tags(&build_font(&gpui::font("Test"), bold, italic)), + vec![("calt", 0), ("liga", 0), ("clig", 0)], + "bold={bold} italic={italic}", + ); + } + + // Explicitly on: what Settings → Appearance → Font ligatures writes. + let mut configured = gpui::font("Test"); + configured.features = crate::core::config::gpui_font_features( + &serde_json::from_str(r#"{"calt":true,"liga":1}"#).unwrap(), + ); + let font = build_font(&configured, false, false); + assert_eq!( + tags(&font), + vec![("calt", 1), ("liga", 1)], + "an explicit request for ligatures must survive untouched", + ); + assert_eq!(font.features.is_calt_enabled(), Some(true)); + + // Explicitly off in `config.json`: also passed through unchanged. + let mut configured = gpui::font("Test"); + configured.features = crate::core::config::gpui_font_features( + &serde_json::from_str(r#"{"calt":0,"liga":0,"clig":0}"#).unwrap(), + ); + assert_eq!( + tags(&build_font(&configured, false, false)), + vec![("calt", 0), ("liga", 0), ("clig", 0)], ); } From 42424c7737608d624fc3d9aa6b82c3f5431b916a Mon Sep 17 00:00:00 2001 From: bytehello Date: Mon, 7 Sep 2026 22:29:08 +0800 Subject: [PATCH 08/15] feat(ui): add a bindable Close Window action (#773) (#778) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 26.9.0's tray-retire model made closing the last window the "keep the daemon, drop the UI process weight" gesture, but that path was reachable only from the OS red close button: no action, no palette entry, nothing to bind. `⌘W` closes a pane or tab, `⌘H` hides the window but keeps it in memory, and `⌘Q` stops the server. There was nothing that closed the window and left the shells running. `CloseWindow` is that action, with no default key — the slot is left free. The logic that decided what a window close means (detach the workspace, and on the last window retire to the tray if an icon is actually up, otherwise quit) moves out of `on_window_should_close` into `prepare_window_close`, so the button and the action share one decision instead of two that can drift. Notably not routed through `close_window_for`: that one recycles the last window onto a fresh workspace, which is what deleting a workspace wants and not what closing a window wants. It reaches the command palette, the Keybindings UI, and the reference table of actions with no default key. In the palette it sits beside Quit, because that pair is the whole point of the action: both end the window in front of you and only one takes your shells with it. Their subtitles now say which — including Quit's, which had been promising "shells keep running" while calling `daemon::spawn::stop()`. Co-authored-by: bytehello https://claude.ai/code/session_01LKMZVh6mUBxXAn6v7P6JC6 --- docs/reference/keyboard-shortcuts.mdx | 2 +- src/core/actions.rs | 1 + src/ui/app.rs | 137 +++++++++++++++++++++----- src/ui/i18n/en.rs | 4 +- src/ui/i18n/ja.rs | 4 +- src/ui/i18n/mod.rs | 2 + src/ui/i18n/zh.rs | 4 +- src/ui/keymap.rs | 7 ++ src/ui/palette.rs | 8 ++ 9 files changed, 143 insertions(+), 26 deletions(-) diff --git a/docs/reference/keyboard-shortcuts.mdx b/docs/reference/keyboard-shortcuts.mdx index e82a1077..81314b1a 100644 --- a/docs/reference/keyboard-shortcuts.mdx +++ b/docs/reference/keyboard-shortcuts.mdx @@ -112,7 +112,7 @@ Keybindings**: | Git | `ScmStageAll` · `ScmUnstageAll` · `ScmDiscardAll` · `ScmCommitAmend` · `ScmRefresh` · `ScmSync` · `ScmPush` · `ScmPull` · `ScmFetch` · `ScmCheckoutBranch` · `ScmCreateBranch` · `ScmToggleGraph` · `ToggleDiffViewMode` | | Panels | `ShowRightPanelInfo` · `ShowRightPanelChanges` · `ShowRightPanelFiles` | | SSH | `ToggleSftp` · `ShowSshForwards` · `OpenSshProfiles` | -| Application | `About` · `CheckForUpdates` · `OpenDocumentation` · `OpenDiscord` · `ReportIssue` · `ShowAll` · `ZoomWindow` | +| Application | `CloseWindow` · `About` · `CheckForUpdates` · `OpenDocumentation` · `OpenDiscord` · `ReportIssue` · `ShowAll` · `ZoomWindow` | ## Rebinding syntax diff --git a/src/core/actions.rs b/src/core/actions.rs index 09604411..30788b42 100644 --- a/src/core/actions.rs +++ b/src/core/actions.rs @@ -18,6 +18,7 @@ actions!( SelectWorkspace7, SelectWorkspace8, SelectWorkspace9, + CloseWindow, CloseActiveTab, RenameTab, NewWorktreeTab, diff --git a/src/ui/app.rs b/src/ui/app.rs index dea55890..e3b74ca3 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1395,29 +1395,8 @@ impl Tty7App { let weak_app = cx.weak_entity(); window.on_window_should_close(cx, move |_window, cx| { - let last_window = crate::ui::windows::WindowRegistry::count(cx) <= 1; if let Some(app) = weak_app.upgrade() { - app.update(cx, |app, cx| app.detach_workspace(cx)); - } - if last_window { - // With a tray icon, closing the last window retires to the - // tray: the daemon stays reachable (show / quit-and-stop) - // instead of being orphaned behind a dead icon. Without one - // the app quits — the only way it stays visible at all. - // - // The icon has to actually be up, not merely asked for: the - // backend can fail for the whole run (a Linux session with no - // StatusNotifier host), and retiring into an icon that never - // appeared leaves a process with no window and no tray — no - // way back in, and the daemon still held. - let retire_to_tray = - cx.global::().show_tray_icon && crate::ui::tray::icon_is_up(); - if !retire_to_tray { - cx.spawn(async move |cx| { - let _ = cx.update(|cx| cx.quit()); - }) - .detach(); - } + app.update(cx, |app, cx| app.prepare_window_close(cx)); } true }); @@ -1468,6 +1447,36 @@ impl Tty7App { crate::ui::windows::refresh_menu(cx); } + fn prepare_window_close(&self, cx: &mut App) { + let last_window = crate::ui::windows::WindowRegistry::count(cx) <= 1; + self.detach_workspace(cx); + if last_window { + // With a tray icon, closing the last window retires to the + // tray: the daemon stays reachable (show / quit-and-stop) + // instead of being orphaned behind a dead icon. Without one + // the app quits — the only way it stays visible at all. + // + // The icon has to actually be up, not merely asked for: the + // backend can fail for the whole run (a Linux session with no + // StatusNotifier host), and retiring into an icon that never + // appeared leaves a process with no window and no tray — no + // way back in, and the daemon still held. + let retire_to_tray = + cx.global::().show_tray_icon && crate::ui::tray::icon_is_up(); + if !retire_to_tray { + cx.spawn(async move |cx| { + let _ = cx.update(|cx| cx.quit()); + }) + .detach(); + } + } + } + + fn close_window(&self, window: &mut Window, cx: &mut App) { + self.prepare_window_close(cx); + window.remove_window(); + } + pub(crate) fn teardown_workspace_forwards(&self, cx: &gpui::App) { let Some(route) = self .tabs @@ -4895,6 +4904,7 @@ impl Tty7App { OpenWorkspacePicker => self.open_switcher(window, cx), StopWorkspace => self.stop_workspace(self.workspace, window, cx), DeleteWorkspace => self.delete_workspace(self.workspace, window, cx), + CloseWindow => self.close_window(window, cx), SplitRight => self.split(Axis::Horizontal, window, cx), SplitDown => self.split(Axis::Vertical, window, cx), ClosePane => self.close_pane(window, cx), @@ -7391,6 +7401,9 @@ impl Render for Tty7App { .on_action(cx.listener(|this, _: &NewWorkspace, window, cx| { this.open_workspace_form(window, cx); })) + .on_action( + cx.listener(|this, _: &CloseWindow, window, cx| this.close_window(window, cx)), + ) .on_action(cx.listener(|this, _: &CloseActiveTab, window, cx| { if !this.editor_close_active_if_focused(window, cx) { this.close_pane(window, cx) @@ -10244,3 +10257,83 @@ mod managed_forward_gpui_tests { }); } } + +#[cfg(test)] +mod close_window_action_tests { + use crate::core::actions::CloseWindow; + 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}; + + /// `CloseWindow` has to close the window, and close it the way the red + /// button does. + /// + /// The action is otherwise all table entries — the `actions!` row, the + /// keymap slot, the palette command, the Keybindings label — and every one + /// of those can be in place while the action reaches nothing at all. So + /// this drives the real dispatch path and then asks two separate + /// questions: the window is gone from gpui, *and* it left the + /// `WindowRegistry` on the way out. The second is what makes it the same + /// close as the native one — `detach_workspace` is where the session is + /// saved and the workspace is retired, and a `remove_window` that skipped + /// it would still pass the first assertion while quietly dropping a + /// window's tabs on the floor. + #[gpui::test] + fn dispatching_close_window_takes_the_window_down_with_its_registration( + 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; the registry + // is where the close has to show up, so an unregistered window would + // make the assertion below pass for the wrong reason. + 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(); + assert_eq!( + vcx.update(|_, cx| WindowRegistry::count(cx)), + 1, + "the harness starts with exactly the one window" + ); + + vcx.dispatch_action(CloseWindow); + drop(vcx); + + assert!( + cx.update(|cx| cx.windows().is_empty()), + "CloseWindow has to reach `remove_window`; the window is still open" + ); + assert!( + cx.update(|cx| WindowRegistry::open_windows(cx).is_empty()), + "the close has to run the same cleanup the red button runs, \ + which is what takes the window out of the registry" + ); + } +} diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index eb62d36f..81949a5f 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1430,6 +1430,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::CmdForkSessionSubtitle => "branch this agent session into a new tab", L10nKey::CmdMarkTabAsUnread => "Mark Tab as Unread", L10nKey::CmdClosePaneTab => "Close Pane / Tab", + L10nKey::CmdCloseWindow => "Close Window", + L10nKey::CmdCloseWindowSubtitle => "shells keep running", L10nKey::CmdCloseOtherTabs => "Close Other Tabs", L10nKey::CmdCloseTabsToTheRight => "Close Tabs to the Right", L10nKey::CmdReopenClosedTab => "Reopen Closed Tab", @@ -1506,7 +1508,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::CmdRestartServer => "Restart Server…", L10nKey::CmdRestartServerSubtitle => "ends every running shell; layout is kept", L10nKey::CmdQuitTty7 => "Quit tty7", - L10nKey::CmdQuitTty7Subtitle => "shells keep running", + L10nKey::CmdQuitTty7Subtitle => "stops the server; every running shell ends", L10nKey::CmdQuickConnect => "Connect to \"{target}\"", L10nKey::CmdQuickConnectSaveProfile => "Save \"{target}\" as profile…", L10nKey::CmdRecent => "Recent", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 367c4d95..16305f28 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1487,6 +1487,8 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::CmdForkSessionSubtitle => "このエージェントのセッションを新しいタブにフォーク", L10nKey::CmdMarkTabAsUnread => "タブを未読としてマーク", L10nKey::CmdClosePaneTab => "ペイン / タブを閉じる", + L10nKey::CmdCloseWindow => "ウィンドウを閉じる", + L10nKey::CmdCloseWindowSubtitle => "シェルは実行を継続", L10nKey::CmdCloseOtherTabs => "他のタブを閉じる", L10nKey::CmdCloseTabsToTheRight => "右側のタブを閉じる", L10nKey::CmdReopenClosedTab => "閉じたタブをもう一度開く", @@ -1561,7 +1563,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::CmdRestartServer => "サーバーを再起動…", L10nKey::CmdRestartServerSubtitle => "実行中のすべてのシェルを終了し、レイアウトは保持", L10nKey::CmdQuitTty7 => "tty7 を終了", - L10nKey::CmdQuitTty7Subtitle => "シェルは実行を継続", + L10nKey::CmdQuitTty7Subtitle => "サーバーを停止し、実行中のすべてのシェルを終了", L10nKey::CmdQuickConnect => "「{target}」に接続", L10nKey::CmdQuickConnectSaveProfile => "「{target}」をプロファイルとして保存…", L10nKey::CmdRecent => "最近", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 1b54fbca..1219d497 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -1150,6 +1150,8 @@ l10n_keys! { CmdForkSessionSubtitle, CmdMarkTabAsUnread, CmdClosePaneTab, + CmdCloseWindow, + CmdCloseWindowSubtitle, CmdCloseOtherTabs, CmdCloseTabsToTheRight, CmdReopenClosedTab, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index f802207a..6cf65437 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1348,6 +1348,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::CmdForkSessionSubtitle => "将此 agent 会话 fork 到新标签页", L10nKey::CmdMarkTabAsUnread => "将标签页标记为未读", L10nKey::CmdClosePaneTab => "关闭窗格/标签页", + L10nKey::CmdCloseWindow => "关闭窗口", + L10nKey::CmdCloseWindowSubtitle => "shell 保持运行", L10nKey::CmdCloseOtherTabs => "关闭其他标签页", L10nKey::CmdCloseTabsToTheRight => "关闭右侧标签页", L10nKey::CmdReopenClosedTab => "重新打开已关闭标签页", @@ -1422,7 +1424,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::CmdRestartServer => "重启 server…", L10nKey::CmdRestartServerSubtitle => "结束所有运行中的 shell；保留布局", L10nKey::CmdQuitTty7 => "退出 tty7", - L10nKey::CmdQuitTty7Subtitle => "shell 保持运行", + L10nKey::CmdQuitTty7Subtitle => "停止服务；结束所有运行中的 shell", L10nKey::CmdQuickConnect => "连接到“{target}”", L10nKey::CmdQuickConnectSaveProfile => "将“{target}”保存为主机配置…", L10nKey::CmdRecent => "最近使用", diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 67fd2c97..7c175e57 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -265,6 +265,7 @@ pub(crate) fn default_bindings() -> Vec<(&'static str, &'static str)> { vec![ ("NewTab", per_platform("secondary-t", "secondary-shift-t")), ("NewWorkspace", "secondary-shift-n"), + ("CloseWindow", ""), ( "CloseActiveTab", per_platform("secondary-w", "secondary-shift-w"), @@ -733,6 +734,10 @@ fn authored_entry(action: &str) -> Option<(CommandGroup, String)> { CommandGroup::Application, t(L10nKey::AppMenuCommandPalette).to_string(), ), + "CloseWindow" => ( + CommandGroup::Application, + t(L10nKey::CmdCloseWindow).to_string(), + ), "OpenSettings" => ( CommandGroup::Application, t(L10nKey::CmdSettings).to_string(), @@ -1036,6 +1041,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), + "CloseWindow" => KeyBinding::new(keystroke, CloseWindow, None), "CloseActiveTab" => KeyBinding::new(keystroke, CloseActiveTab, None), "RenameTab" => KeyBinding::new(keystroke, RenameTab, None), "NewWorktreeTab" => KeyBinding::new(keystroke, NewWorktreeTab, None), @@ -1217,6 +1223,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("CloseWindow").1, "Close Window"); assert_eq!(action_entry("ClearScrollback").1, "Clear Scrollback"); assert_eq!(action_entry("TogglePalette").1, "Command Palette…"); assert_eq!(action_entry("ToggleSwitcher").1, "Switch Workspace…"); diff --git a/src/ui/palette.rs b/src/ui/palette.rs index 28272380..fd749760 100644 --- a/src/ui/palette.rs +++ b/src/ui/palette.rs @@ -22,6 +22,7 @@ pub enum CommandKind { RenameWorkspace, StopWorkspace, DeleteWorkspace, + CloseWindow, SplitRight, SplitDown, ClosePane, @@ -124,6 +125,7 @@ impl CommandKind { RenameWorkspace => "rename-workspace", StopWorkspace => "stop-workspace", DeleteWorkspace => "delete-workspace", + CloseWindow => "close-window", SplitRight => "split-right", SplitDown => "split-down", ClosePane => "close-pane", @@ -230,6 +232,7 @@ impl CommandKind { RenameWorkspace => "RenameWorkspace", StopWorkspace => "StopWorkspace", DeleteWorkspace => "DeleteWorkspace", + CloseWindow => "CloseWindow", SplitRight => "SplitRight", SplitDown => "SplitDown", ClosePane => "CloseActiveTab", @@ -574,6 +577,11 @@ impl Command { Command::localized(L10nKey::CmdReportIssue, ReportIssue), Command::localized(L10nKey::CmdRestartServer, RestartDaemon) .with_subtitle(t(L10nKey::CmdRestartServerSubtitle)), + // Beside Quit, because the pair is the whole point of the action: + // both end the window you are looking at, and only one of them + // takes your shells with it. Read together the subtitles say which. + Command::localized(L10nKey::CmdCloseWindow, CloseWindow) + .with_subtitle(t(L10nKey::CmdCloseWindowSubtitle)), Command::localized(L10nKey::CmdQuitTty7, Quit) .with_subtitle(t(L10nKey::CmdQuitTty7Subtitle)), ]; From c125803077aafbdb3b4c23c22939e146a175f380 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:31:23 +0800 Subject: [PATCH 09/15] feat(daemon): list a Windows pane's listening ports (#787) --- crates/tty7-core/Cargo.toml | 3 + crates/tty7-core/src/daemon/procinfo.rs | 404 +++++++++++++++++++++++- 2 files changed, 406 insertions(+), 1 deletion(-) diff --git a/crates/tty7-core/Cargo.toml b/crates/tty7-core/Cargo.toml index 9c12b7d5..a016ab0c 100644 --- a/crates/tty7-core/Cargo.toml +++ b/crates/tty7-core/Cargo.toml @@ -163,8 +163,11 @@ getrandom = "0.3" # tree on hangup (ConPTY's `kill` only reaches the shell itself). `Registry` # additionally lets `daemon::windows_env` re-read the two environment hives at # pane-spawn time so a long-lived daemon stops handing out its startup `PATH`. +# `IpHelper` is `GetExtendedTcpTable`, which is how `daemon::procinfo` answers +# the Info tab's Ports section here — the platform has no `lsof` to shell out to. windows-sys = { version = "0.59", features = [ "Win32_Foundation", + "Win32_NetworkManagement_IpHelper", "Win32_Security", "Win32_System_Console", "Win32_System_Diagnostics_ToolHelp", diff --git a/crates/tty7-core/src/daemon/procinfo.rs b/crates/tty7-core/src/daemon/procinfo.rs index 03732ccb..30a8fe85 100644 --- a/crates/tty7-core/src/daemon/procinfo.rs +++ b/crates/tty7-core/src/daemon/procinfo.rs @@ -296,7 +296,225 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { ports } -#[cfg(not(unix))] +/// Windows has no `lsof`, and the Ports section was simply never drawn there — +/// the daemon answered `QueryProcs` with an empty list no matter what the pane +/// was running, so a `npm run dev` in a Windows pane showed processes and no +/// port to click. +/// +/// `GetExtendedTcpTable` is the same answer without a subprocess: the kernel's +/// own table of listening sockets, each already tagged with the pid that owns +/// it. The table is machine-wide, so the filter against the pane's tree below +/// is the whole difference between this panel and `netstat -ano`. +/// +/// **Cost.** Two calls per poll — one per address family — into a buffer sized +/// for far more listeners than a real machine has; a family only pays for a +/// second call when its table outgrew that. The Info tab re-polls every two +/// seconds while it is open, so this is a fixed handful of microseconds, with +/// no process spawn and nothing allocated per pid. +#[cfg(windows)] +fn listening_ports(procs: &[ProcEntry]) -> Vec { + use std::net::{Ipv4Addr, Ipv6Addr}; + + use windows_sys::Win32::NetworkManagement::IpHelper::{ + MIB_TCP6ROW_OWNER_PID, MIB_TCP6TABLE_OWNER_PID, MIB_TCPROW_OWNER_PID, + MIB_TCPTABLE_OWNER_PID, + }; + + if procs.is_empty() { + return Vec::new(); + } + let by_pid: HashMap = procs.iter().map(|p| (p.pid, p.name.as_str())).collect(); + let mut ports: Vec = Vec::new(); + + let v4 = tcp_table(AF_INET); + // SAFETY: `tcp_table` hands back either an empty buffer or one the kernel + // filled with a `MIB_TCPTABLE_OWNER_PID`; the `Vec` gives it the 4-byte + // alignment every field of that struct wants, and `rows` is clamped to what + // the buffer can actually hold before anything is read out of it. + unsafe { + if let Some((rows, count)) = table_rows::(&v4) + { + for i in 0..count { + let row = &*rows.add(i); + // Filter before spelling the address: the table is the whole + // machine's, and formatting a string for every stranger's + // socket is the one avoidable allocation on this path. + let Some(name) = by_pid.get(&row.dwOwningPid) else { + continue; + }; + let addr = Ipv4Addr::from(row.dwLocalAddr.to_ne_bytes()); + record_listener( + &mut ports, + name, + local_port(row.dwLocalPort), + row.dwOwningPid, + spell_v4(addr), + ); + } + } + } + + let v6 = tcp_table(AF_INET6); + // SAFETY: as above, for the IPv6 shape of the same table. + unsafe { + if let Some((rows, count)) = + table_rows::(&v6) + { + for i in 0..count { + let row = &*rows.add(i); + let Some(name) = by_pid.get(&row.dwOwningPid) else { + continue; + }; + let addr = Ipv6Addr::from(row.ucLocalAddr); + record_listener( + &mut ports, + name, + local_port(row.dwLocalPort), + row.dwOwningPid, + spell_v6(addr), + ); + } + } + } + + ports.sort_by_key(|e| (e.port, e.pid)); + ports +} + +/// The two Winsock address families, named here rather than by switching on +/// `windows-sys`'s `Win32_Networking_WinSock`: the whole socket module is a +/// long compile for two integers the ABI froze decades ago. +#[cfg(windows)] +const AF_INET: u32 = 2; +#[cfg(windows)] +const AF_INET6: u32 = 23; + +/// One `GetExtendedTcpTable` snapshot of the listening sockets in `family`, as +/// the raw buffer the kernel filled, or an empty buffer if it would not answer. +/// +/// Failure is soft, the way an absent `lsof` is soft on unix: the panel shows no +/// ports rather than an error. A machine with IPv6 disabled takes that path for +/// `AF_INET6` alone and still gets its IPv4 ports. +#[cfg(windows)] +fn tcp_table(family: u32) -> Vec { + use windows_sys::Win32::Foundation::{ERROR_INSUFFICIENT_BUFFER, NO_ERROR}; + use windows_sys::Win32::NetworkManagement::IpHelper::{ + GetExtendedTcpTable, TCP_TABLE_OWNER_PID_LISTENER, + }; + + // 8 KiB: room for ~340 IPv4 or ~146 IPv6 listeners, where a busy desktop has + // a few dozen. Sizing it up front is what keeps the common poll to one call + // per family instead of the usual size-then-fetch pair. + let mut buf = vec![0u32; 2048]; + // Two rounds, not a loop until it fits: the table can keep growing between + // calls, and this runs on a 2 s timer where giving up costs one poll. + for _ in 0..2 { + let mut size = (buf.len() * std::mem::size_of::()) as u32; + // SAFETY: `buf` is at least `size` bytes, 4-aligned, and writable; the + // kernel writes no more than `size` and reports what it needed instead. + let rc = unsafe { + GetExtendedTcpTable( + buf.as_mut_ptr().cast(), + &mut size, + // No kernel-side sort: the rows are ordered by (port, pid) below + // anyway, and this one is over the address, not the port. + 0, + family, + TCP_TABLE_OWNER_PID_LISTENER, + 0, + ) + }; + match rc { + NO_ERROR => return buf, + ERROR_INSUFFICIENT_BUFFER => { + buf = vec![0u32; (size as usize).div_ceil(std::mem::size_of::()) + 64] + } + _ => break, + } + } + Vec::new() +} + +/// Where the rows of a `MIB_*TABLE_OWNER_PID` start in `buf`, and how many of +/// them the buffer can be trusted for. +/// +/// The count is `min`'d against the buffer's own capacity rather than taken from +/// `dwNumEntries` alone: that field is the kernel's, but the read that follows +/// it is ours, and a table shorter than its header claims must not walk off the +/// end of the allocation. +/// +/// # Safety +/// +/// `buf` must be empty or hold a `Table` the kernel filled. +#[cfg(windows)] +unsafe fn table_rows(buf: &[u32]) -> Option<(*const Row, usize)> { + let bytes = std::mem::size_of_val(buf); + if bytes < std::mem::size_of::() { + return None; + } + let table = buf.as_ptr().cast::
(); + // Every `MIB_*TABLE_OWNER_PID` is `{ dwNumEntries: u32, table: [Row; 1] }`, + // so the count is the first word and the rows begin where the padding ends. + let count = buf[0] as usize; + let offset = std::mem::size_of::
() - std::mem::size_of::(); + let capacity = (bytes - offset) / std::mem::size_of::(); + let rows = unsafe { table.cast::().add(offset) }.cast::(); + Some((rows, count.min(capacity))) +} + +/// `dwLocalPort` carries the port in *network* byte order in its low 16 bits. +/// Reading it as a plain number is the classic way to end up showing 41247 for +/// a server on 8099. +#[cfg(windows)] +fn local_port(raw: u32) -> u16 { + u16::from_be(raw as u16) +} + +/// The wildcard binds are spelled `*`, exactly as `lsof -n` spells them on the +/// other platforms, so the same server reads the same in the panel wherever it +/// runs — and so `PortEntry::authority` resolves it to `localhost`. +#[cfg(windows)] +fn spell_v4(addr: std::net::Ipv4Addr) -> String { + match addr.is_unspecified() { + true => "*".to_string(), + false => addr.to_string(), + } +} + +/// See `spell_v4`. A specific IPv6 address keeps `lsof`'s brackets, which is +/// what makes `[::1]:5173` a pastable authority. +#[cfg(windows)] +fn spell_v6(addr: std::net::Ipv6Addr) -> String { + match addr.is_unspecified() { + true => "*".to_string(), + false => format!("[{addr}]"), + } +} + +/// Adds one listening socket to the list, merging it with a row already there +/// for the same port and pid. +/// +/// The merge rule is the unix path's, for the same reason: a process bound to +/// both `192.168.1.5` and `*` is on localhost, and the row the panel turns into +/// a clickable URL should say so rather than whichever address the kernel +/// happened to list first. +#[cfg(windows)] +fn record_listener(ports: &mut Vec, name: &str, port: u16, pid: u32, addr: String) { + if let Some(seen) = ports.iter_mut().find(|e| e.port == port && e.pid == pid) { + if !PortEntry::reaches_loopback(&seen.addr) && PortEntry::reaches_loopback(&addr) { + seen.addr = addr; + } + return; + } + ports.push(PortEntry { + port, + pid, + addr, + name: name.to_string(), + }); +} + +#[cfg(not(any(unix, windows)))] fn listening_ports(_procs: &[ProcEntry]) -> Vec { Vec::new() } @@ -408,3 +626,187 @@ mod tests { assert_eq!(entry("192.168.1.20").authority(), "192.168.1.20:8080"); } } + +#[cfg(all(test, windows))] +mod windows_tests { + use std::io::Write as _; + use std::net::TcpListener; + use std::time::{Duration, Instant}; + + use super::*; + + /// The port lives in the low half of a DWORD in *network* order. Getting + /// this wrong does not fail loudly — it yields a plausible port number for + /// a socket nobody is listening on. + #[test] + fn a_local_port_is_read_out_of_network_order() { + // 8099 == 0x1FA3, so the wire spells it 0xA31F. + assert_eq!(local_port(0x0000_A31F), 8099); + assert_eq!(local_port(0xBB01), 443); + assert_eq!(local_port(0x5000), 80); + } + + /// The panel renders one server the same way on every platform, so a + /// Windows wildcard bind has to arrive spelled the way `lsof -n` spells it. + #[test] + fn addresses_are_spelled_the_way_lsof_spells_them() { + assert_eq!(spell_v4("0.0.0.0".parse().unwrap()), "*"); + assert_eq!(spell_v4("127.0.0.1".parse().unwrap()), "127.0.0.1"); + assert_eq!(spell_v4("192.168.1.20".parse().unwrap()), "192.168.1.20"); + assert_eq!(spell_v6("::".parse().unwrap()), "*"); + assert_eq!(spell_v6("::1".parse().unwrap()), "[::1]"); + } + + /// A dual-stack server shows up twice in the kernel's tables, once per + /// family, and is one row in the panel — the reachable one. + #[test] + fn a_dual_stack_listener_collapses_to_its_reachable_address() { + let mut ports = Vec::new(); + record_listener(&mut ports, "node.exe", 3000, 42, "192.168.1.5".into()); + record_listener(&mut ports, "node.exe", 3000, 42, "*".into()); + assert_eq!(ports.len(), 1, "one port, not one per address family"); + assert_eq!(ports[0].addr, "*", "the loopback-reachable bind wins"); + + // ...and never the other way round: a wildcard already recorded is not + // downgraded to an interface nobody can reach on localhost. + let mut ports = Vec::new(); + record_listener(&mut ports, "node.exe", 3000, 42, "[::]".into()); + record_listener(&mut ports, "node.exe", 3000, 42, "192.168.1.5".into()); + assert_eq!(ports[0].addr, "[::]"); + + // Two processes on the same port number are two rows. + record_listener(&mut ports, "python.exe", 3000, 43, "127.0.0.1".into()); + assert_eq!(ports.len(), 2); + } + + /// The whole feature, against the live kernel table: a real + /// `cmd.exe -> powershell.exe` chain holding a real socket. + /// + /// Both halves matter. The port has to show up with the right number and + /// the right owner — that is the half that was missing entirely, since + /// `listening_ports` was `#[cfg(unix)]` and Windows got an empty list. And + /// a socket held *outside* the tree must not show up, because + /// `GetExtendedTcpTable` answers for the whole machine: without the pid + /// filter this panel would list every port on the box and still pass the + /// first assertion. + #[test] + fn listening_ports_finds_the_pane_tree_s_socket_and_only_its_tree_s() { + // The out-of-tree listener. It belongs to the test process, which is + // the parent of the chain and so is never inside the tree rooted at it. + let outsider = TcpListener::bind("127.0.0.1:0").expect("bind an out-of-tree listener"); + let outside_port = outsider.local_addr().expect("read its port").port(); + + let stamp = format!("tty7-ports-{}", std::process::id()); + let dir = std::env::temp_dir(); + let script = dir.join(format!("{stamp}.ps1")); + let port_file = dir.join(format!("{stamp}.port")); + let _ = std::fs::remove_file(&port_file); + // The port comes back through a file rather than a pipe: PowerShell + // buffers redirected stdout, and a test that waits on a flush that + // never comes is a test that hangs. + let mut f = std::fs::File::create(&script).expect("write the listener script"); + write!( + f, + "$l = [System.Net.Sockets.TcpListener]::new([System.Net.IPAddress]::Loopback, 0)\r\n\ + $l.Start()\r\n\ + [System.IO.File]::WriteAllText('{}', [string]$l.LocalEndpoint.Port)\r\n\ + while ($true) {{ Start-Sleep -Seconds 1 }}\r\n", + port_file.display().to_string().replace('\'', "''") + ) + .expect("write the listener script"); + drop(f); + + // `cmd.exe` in front of PowerShell is what makes this a *tree* and not + // one child: the listener sits at depth 1, reached only by walking. + let mut child = std::process::Command::new("cmd.exe") + .args([ + "/c", + "powershell", + "-NoProfile", + "-ExecutionPolicy", + "Bypass", + "-File", + ]) + .arg(&script) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .expect("spawn the cmd -> powershell chain"); + let root = child.id(); + + let cleanup = |child: &mut std::process::Child| { + let doomed = crate::daemon::winproc::descendants( + &crate::daemon::winproc::snapshot(), + child.id(), + ); + let _ = child.kill(); + let _ = child.wait(); + crate::daemon::winproc::terminate_and_wait_all( + &doomed, + Instant::now() + Duration::from_secs(5), + ); + }; + + // PowerShell's startup is measured in seconds on a cold machine, and + // `WriteAllText` can be observed mid-write, so parse until it parses. + let deadline = Instant::now() + Duration::from_secs(60); + let inside_port = loop { + if let Some(port) = std::fs::read_to_string(&port_file).ok().and_then(|text| { + text.trim() + .trim_start_matches('\u{feff}') + .parse::() + .ok() + }) { + break port; + } + if Instant::now() >= deadline { + cleanup(&mut child); + let _ = std::fs::remove_file(&script); + panic!("the in-tree listener never reported its port"); + } + std::thread::sleep(Duration::from_millis(100)); + }; + + let got = snapshot(root, None); + cleanup(&mut child); + let _ = std::fs::remove_file(&script); + let _ = std::fs::remove_file(&port_file); + drop(outsider); + + assert!( + got.procs.iter().any(|p| p.depth > 0), + "the chain must be walked past its root: {:?}", + got.procs + ); + let found = got + .ports + .iter() + .find(|e| e.port == inside_port) + .unwrap_or_else(|| { + panic!("port {inside_port} is missing from {:?}", got.ports); + }); + assert_eq!( + found.addr, "127.0.0.1", + "a loopback bind keeps its address, as it does under lsof" + ); + assert!( + got.procs.iter().any(|p| p.pid == found.pid), + "the port's owner must be one of the pane's own processes" + ); + assert!( + found.name.eq_ignore_ascii_case("powershell.exe"), + "the row names the process holding the socket, got {:?}", + found.name + ); + assert!( + !got.ports.iter().any(|e| e.port == outside_port), + "port {outside_port} is held outside the tree and must not be listed: {:?}", + got.ports + ); + assert!( + got.ports.windows(2).all(|w| w[0].port <= w[1].port), + "rows arrive ordered by port, as they do on unix" + ); + } +} From 5ff0567c7df76a38a40294b316248ae32634fddb Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:36:58 +0800 Subject: [PATCH 10/15] fix(terminal): keep a solo glyph inside its cell when the next one is taken (#783) --- src/terminal/element.rs | 135 +++++++++++++++++++++++++++++++++++----- 1 file changed, 119 insertions(+), 16 deletions(-) diff --git a/src/terminal/element.rs b/src/terminal/element.rs index 5dc1da49..55618483 100644 --- a/src/terminal/element.rs +++ b/src/terminal/element.rs @@ -1003,12 +1003,26 @@ fn native_cell_residue(style: &GlyphStyle) -> Option { /// and letting it spill. This follows WezTerm, whose answer shows the glyph /// whole most often: a quarter cell of slack always, and a whole extra cell /// when the neighbouring cell is blank and has nothing to lose. -fn seg_budget(solo: bool, cells: usize, room: bool, cell_width: Pixels) -> Pixels { +fn seg_budget(solo: bool, measured: bool, cells: usize, room: bool, cell_width: Pixels) -> Pixels { if solo { - // Single-cell glyphs have always been allowed to lean into the next - // cell. Narrowing that here would shrink a pile of symbols that look - // fine today, so it stays a separate decision. - cell_width * 2. + // Single-cell glyphs are allowed to lean into the next cell — that is + // what keeps the pile of symbols that look fine today from shrinking — + // but only while that cell is empty. A Nerd Font icon pulled from a + // fallback face inks well past a narrow primary's cell (1.6 cells is + // typical), and where the next cell has a glyph of its own the lean is + // not a lean, it is an overlap: the neighbour is painted afterwards + // and lands on top of the overshoot. Hand those their own cell and let + // `fit_scale` bring them down into it, the way kitty and ghostty do. + // + // Only where the ink was measured, though. This number is the clip as + // well as the threshold to shrink at, so narrowing it for a segment + // nothing could measure would cut the glyph instead of scaling it — + // see `ink_covers_segment`. + if room || !measured { + cell_width * 2. + } else { + cell_width + } } else if room { cell_width * (cells as f32 + 1.) } else { @@ -1065,6 +1079,21 @@ fn ink_extent( }) } +/// Whether [`ink_extent`]'s answer speaks for the whole segment. +/// +/// It measures the segment's first character in the run's first face. That is +/// all of a one-character segment and only part of anything longer: a +/// cluster's combining marks can ink well to the right of the base they hang +/// off — Devanagari `\u{915}` + `\u{93E}` — and are not in the number. `None` +/// is a face that answered no bounds at all, which is no measurement either. +/// +/// Neither can be scaled to fit, so neither may be held to one cell: the +/// budget doubles as the clip, and a segment that cannot shrink into a +/// narrowed one is simply cut off at it. +fn ink_covers_segment(ink: Option, text: &str) -> bool { + ink.is_some() && text.chars().nth(1).is_none() +} + /// Whether the cell after a segment is free for its glyph to lean into. /// /// A blank still owns its cell if it paints anything there: a background, a @@ -1201,9 +1230,20 @@ fn paint_glyphs( }; let x = geom.origin.x + geom.cell_width * (start as f32); + + let mut shaped = + window + .text_system() + .shape_line(text.clone(), font_size, run_buf, force_width); + // Measured before the budget is set, because how far a solo glyph + // may reach turns on whether its ink is known at all. + let ink = fit + .then(|| ink_extent(cx, &shaped, &text, font_size)) + .flatten(); let budget = if fit { seg_budget( solo, + ink_covers_segment(ink, &text), cells, has_room_after(row_cells, start, cells), geom.cell_width, @@ -1211,12 +1251,7 @@ fn paint_glyphs( } else { geom.cell_width * cells as f32 }; - - let mut shaped = - window - .text_system() - .shape_line(text.clone(), font_size, run_buf, force_width); - if fit && let Some(ink) = ink_extent(cx, &shaped, &text, font_size) { + if let Some(ink) = ink { let scale = fit_scale(ink, budget); if scale < 1. { shaped = window.text_system().shape_line( @@ -2386,7 +2421,7 @@ mod tests { let ink = px(18.75); let scale = |advance_em: f32, room: bool| { let cell = px(15. * advance_em); - fit_scale(ink, seg_budget(false, 2, room, cell)) + fit_scale(ink, seg_budget(false, true, 2, room, cell)) }; // Menlo and friends: a quarter cell of slack is enough on its own. @@ -2735,14 +2770,82 @@ mod tests { #[test] fn seg_budget_frees_solo_symbols_and_lends_a_cell_only_when_one_is_free() { let cell = px(10.); - assert_eq!(seg_budget(true, 1, false, cell), px(20.), "solo keeps two"); assert_eq!( - seg_budget(false, 2, false, cell), + seg_budget(true, true, 1, true, cell), + px(20.), + "a solo glyph leans into a free cell" + ); + assert_eq!( + seg_budget(true, true, 1, false, cell), + px(10.), + "but keeps to its own once the next cell is taken" + ); + assert_eq!( + seg_budget(false, true, 2, false, cell), px(22.5), "a quarter cell" ); - assert_eq!(seg_budget(false, 2, true, cell), px(30.), "a whole cell"); - assert_eq!(seg_budget(false, 1, false, cell), px(12.5)); + assert_eq!( + seg_budget(false, true, 2, true, cell), + px(30.), + "a whole cell" + ); + assert_eq!(seg_budget(false, true, 1, false, cell), px(12.5)); + } + + #[test] + fn a_fallback_icon_is_fitted_to_its_cell_only_when_the_next_one_is_taken() { + // Measured on Windows: Maple Mono NF CN supplying U+F059 to a + // 15px Cascadia Mono grid inks 13.85px across an 8.79px cell. + let cell = px(8.789); + let ink = px(13.845); + + let leaning = fit_scale(ink, seg_budget(true, true, 1, true, cell)); + assert_eq!(leaning, 1., "a blank neighbour still lends its cell"); + + let crowded = fit_scale(ink, seg_budget(true, true, 1, false, cell)); + assert!(crowded < 1., "an occupied neighbour does not"); + assert!( + ink * crowded <= cell, + "and the icon has to end inside its own cell" + ); + } + + #[test] + fn a_solo_segment_is_held_to_its_cell_only_where_its_ink_was_measured() { + let cell = px(10.); + let budget = |ink, text| seg_budget(true, ink_covers_segment(ink, text), 1, false, cell); + + // One character the face answered bounds for: the whole of it was + // measured, so `fit_scale` can bring it into one cell and the clip + // may be drawn there. + assert!(ink_covers_segment(Some(px(15.)), "\u{f059}")); + assert_eq!(budget(Some(px(15.)), "\u{f059}"), px(10.)); + + // A base with a combining mark hanging off it reaches paint_glyphs as + // a one-cell `Cluster`, which counts as solo. `ink_extent` read the + // base alone: Devanagari ka plus the aa matra inks to the right of the + // base, and none of that overhang is in the 9px. Nothing would shrink + // it, so a one-cell clip would cut the matra clean off. + assert!(!ink_covers_segment(Some(px(9.)), "\u{915}\u{93E}")); + assert_eq!(budget(Some(px(9.)), "\u{915}\u{93E}"), px(20.)); + + // A face that answers no bounds at all is the same story from the + // other side: `fit_scale` is never reached, so halving the clip only + // clips. + assert!(!ink_covers_segment(None, "\u{f059}")); + assert_eq!(budget(None, "\u{f059}"), px(20.)); + + // A free neighbour lends its cell either way \u2014 the measurement only + // decides whether the lean can be withdrawn. + for ink in [Some(px(15.)), None] { + for text in ["\u{f059}", "\u{915}\u{93E}"] { + assert_eq!( + seg_budget(true, ink_covers_segment(ink, text), 1, true, cell), + px(20.) + ); + } + } } /// The emitted feature set is the whole contract with the shaper, so pin it From 7b4c6c050d95bd0fad8c1aae8709c46c6491c3dc Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:42:11 +0800 Subject: [PATCH 11/15] fix(terminal): read a pane's paths in its own host's spelling (#795) --- src/terminal/search.rs | 470 ++++++++++++++++++++++++++++++++++++----- src/terminal/view.rs | 82 ++++++- 2 files changed, 489 insertions(+), 63 deletions(-) diff --git a/src/terminal/search.rs b/src/terminal/search.rs index 09b7d2e9..dff459e1 100644 --- a/src/terminal/search.rs +++ b/src/terminal/search.rs @@ -727,6 +727,103 @@ pub(super) fn local_probe(path: &Path, require_file: bool) -> Probe { } } +/// Which language a pane's paths are written in. +/// +/// The machine tty7 runs on and the machine a pane's paths live on need not +/// agree, and `std::path` only ever speaks the first one's dialect. On a +/// Windows client that is the whole difference between a link and nothing: a +/// leading `/` is not absolute to `Path` there, so `/etc/hosts` printed by a +/// Linux pane used to be measured from that pane's directory instead of +/// standing alone, and a relative `src/lib.rs` was joined onto it with a +/// backslash the far side has never heard of. +/// +/// The two arms are the two dialects, not the two operating systems: a WSL +/// distro and an SSH host both speak [`PathStyle::Posix`] whatever the client +/// is, and a pane on this machine speaks [`PathStyle::NATIVE`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum PathStyle { + /// `/`-rooted and `/`-joined. Everything a Unix host, a WSL distro or a + /// Git-Bash-style shell prints. + Posix, + /// Rooted by a drive letter or a UNC share, joined with `\`. + Windows, +} + +impl Default for PathStyle { + fn default() -> Self { + Self::NATIVE + } +} + +impl PathStyle { + /// The dialect the machine tty7 is running on speaks. + pub const NATIVE: PathStyle = match cfg!(windows) { + true => PathStyle::Windows, + false => PathStyle::Posix, + }; + + /// The dialect a host that called `sample` one of its own directories + /// speaks: a drive letter or a UNC share is a Windows host's, and anything + /// else is read as POSIX. + /// + /// Only a directory that names a *Windows* root counts as one, rather than + /// every directory that does not name a POSIX one. A cwd that is neither — + /// a relative one, or an empty one from a host that has not settled yet — + /// is the same "nothing to go on" as no cwd at all, and has to fall the + /// same way; reading it as Windows would spell a Linux host's paths with + /// backslashes on the strength of a directory it never really reported. + /// + /// This is inference from what a host says about itself in passing. It is + /// sound in the direction that matters: nothing but a POSIX host reports a + /// `/`-rooted cwd. + pub fn of_dir(sample: &Path) -> Self { + match PathStyle::Windows.is_absolute(&sample.to_string_lossy()) { + true => PathStyle::Windows, + false => PathStyle::Posix, + } + } + + /// Whether a token says for itself which filesystem root it hangs off. + /// + /// Deliberately textual rather than [`Path::is_absolute`], which answers + /// for *this* machine: the same string has to be read the pane's way on + /// every client, or a link works on a Mac and not on the Windows box next + /// to it. + pub fn is_absolute(self, path: &str) -> bool { + match self { + PathStyle::Posix => path.starts_with('/'), + // A UNC share, or a drive letter with a separator behind it. + // `C:foo` is drive-*relative* and deliberately not included, which + // is what `Path::is_absolute` says on Windows too. + PathStyle::Windows => { + if path.starts_with("\\\\") { + return true; + } + let mut chars = path.chars(); + chars.next().is_some_and(|c| c.is_ascii_alphabetic()) + && chars.next() == Some(':') + && matches!(chars.next(), Some('\\' | '/')) + } + } + } + + /// `rel` measured from `root`, spelled the way the pane's host spells it. + /// + /// The POSIX arm joins textually because `Path::join` would reach for this + /// machine's separator: on Windows it turns `/home/u` and `src/lib.rs` + /// into `/home/u\src/lib.rs`, which the Linux box on the other end of the + /// probe cannot stat. + pub fn join(self, root: &Path, rel: &str) -> PathBuf { + match self { + PathStyle::Posix => PathBuf::from(format!( + "{}/{rel}", + root.to_string_lossy().trim_end_matches('/') + )), + PathStyle::Windows => root.join(rel), + } + } +} + /// Where a relative path printed by a pane is measured from. /// /// `local_home` is the part that is easy to miss. `~` has to become a real @@ -743,15 +840,18 @@ pub(super) struct LinkRoots { /// Whether this machine's `$HOME` may stand in for a `~` the roots cannot /// explain. pub local_home: bool, + /// How the pane's host spells the paths it prints. + pub style: PathStyle, } impl LinkRoots { /// Roots on the machine tty7 is running on, where `$HOME` means what it - /// says. + /// says and paths are spelled this OS's way. pub fn local(dirs: Vec) -> Self { Self { dirs, local_home: true, + style: PathStyle::NATIVE, } } @@ -841,18 +941,18 @@ impl FileCandidate { /// Every path this token could mean, best guess first: absolute paths /// stand alone, relative ones are joined onto each root in turn. pub fn paths(&self, roots: &LinkRoots) -> Vec { - let Some(expanded) = expand_home(&self.path, roots.cwd(), roots.local_home) else { + let Some(expanded) = expand_home(&self.path, roots) else { return Vec::new(); }; - if expanded.as_os_str().is_empty() { + if expanded.is_empty() { return Vec::new(); } - if expanded.is_absolute() { - return vec![expanded]; + if roots.style.is_absolute(&expanded) { + return vec![PathBuf::from(expanded)]; } let mut out: Vec = Vec::new(); for root in &roots.dirs { - let joined = root.join(&expanded); + let joined = roots.style.join(root, &expanded); if !out.contains(&joined) { out.push(joined); } @@ -864,17 +964,26 @@ impl FileCandidate { /// measured from anywhere. [`Self::paths`] ignores the roots entirely for /// these, so a report about one must not name a directory as the place it /// was looked for. - pub fn is_rooted(&self) -> bool { - self.path.starts_with('~') || Path::new(&self.path).is_absolute() + /// + /// Takes the pane's own dialect for the same reason `paths` does: on a + /// Windows client `/etc/hosts` printed by a Linux pane is rooted and + /// `/etc/hosts` printed by a `cmd.exe` pane is not, and `Path` alone + /// cannot tell those apart. + pub fn is_rooted(&self, style: PathStyle) -> bool { + self.path.starts_with('~') || style.is_absolute(&self.path) } /// Whether the token is written enough like a path to be worth telling the /// user about when nothing answers for it. A bare word is not — every /// modifier-click on ordinary output would raise a notification saying so. - pub fn looks_like_a_path(&self) -> bool { + /// + /// A backslash counts only where it separates directories. In a POSIX + /// pane it is an escape or an ordinary filename character, so `foo\ bar` + /// there is a word, not a path. + pub fn looks_like_a_path(&self, style: PathStyle) -> bool { self.path.starts_with('~') || self.path.contains('/') - || (cfg!(windows) && self.path.contains('\\')) + || (style == PathStyle::Windows && self.path.contains('\\')) } } @@ -1054,14 +1163,29 @@ fn strip_numeric_suffix(token: &str) -> Option<(&str, u32)> { Some((prefix, value)) } -fn expand_home(path: &str, cwd: Option<&Path>, local_home: bool) -> Option { +/// The token with a leading `~` turned into a real directory, still spelled +/// the pane's way — a string rather than a `PathBuf`, because deciding what is +/// absolute and how to join is the [`PathStyle`]'s job from here on and +/// `Path` would answer for the wrong machine. +fn expand_home(path: &str, roots: &LinkRoots) -> Option { + let home = || { + home_dir(roots.cwd(), roots.local_home, roots.style) + .map(|home| home.to_string_lossy().into_owned()) + }; if path == "~" { - return home_dir(cwd, local_home); + return home(); } - if let Some(rest) = path.strip_prefix("~/").or_else(|| path.strip_prefix("~\\")) { - return home_dir(cwd, local_home).map(|home| home.join(rest)); + let rest = match roots.style { + PathStyle::Posix => path.strip_prefix("~/"), + PathStyle::Windows => path.strip_prefix("~/").or_else(|| path.strip_prefix("~\\")), + }; + if let Some(rest) = rest { + return home().map(|home| match roots.style { + PathStyle::Posix => format!("{}/{rest}", home.trim_end_matches('/')), + PathStyle::Windows => Path::new(&home).join(rest).to_string_lossy().into_owned(), + }); } - Some(PathBuf::from(path)) + Some(path.to_string()) } /// The home `~` stands for, read out of the cwd where it can be and out of the @@ -1072,8 +1196,8 @@ fn expand_home(path: &str, cwd: Option<&Path>, local_home: bool) -> Option, local_home: bool) -> Option { - if let Some(home) = cwd.and_then(home_from_cwd) { +fn home_dir(cwd: Option<&Path>, local_home: bool, style: PathStyle) -> Option { + if let Some(home) = cwd.and_then(|cwd| home_from_cwd(cwd, style)) { return Some(home); } if !local_home { @@ -1085,25 +1209,26 @@ fn home_dir(cwd: Option<&Path>, local_home: bool) -> Option { .map(PathBuf::from) } -#[cfg(unix)] -fn home_from_cwd(cwd: &Path) -> Option { - let mut components = cwd.components(); - let root = components.next()?; - let base = components.next()?; - let user = components.next()?; - let base = base.as_os_str().to_str()?; - matches!(base, "Users" | "home").then(|| { - let mut home = PathBuf::new(); - home.push(root.as_os_str()); - home.push(base); - home.push(user.as_os_str()); - home - }) -} - -#[cfg(not(unix))] -fn home_from_cwd(_cwd: &Path) -> Option { - None +/// The `/home/` or `/Users/` a POSIX cwd sits under. +/// +/// Read off the string rather than `Path::components`, and keyed off the +/// pane's dialect rather than `cfg!(unix)`. The old spelling was gated to Unix +/// clients, which meant a Windows tty7 could not say what `~` meant in *any* +/// pane it was looking at — `~/.zshrc` printed by a Linux host resolved on a +/// Mac and silently did not on a Windows box beside it. +/// +/// A Windows-dialect cwd still gets no answer: nothing about `D:\Users\team` +/// says whose home it is, and the environment fallback in [`home_dir`] is both +/// available and right for the panes that spell paths that way. +fn home_from_cwd(cwd: &Path, style: PathStyle) -> Option { + if style != PathStyle::Posix { + return None; + } + let cwd = cwd.to_str()?; + let mut parts = cwd.strip_prefix('/')?.split('/').filter(|p| !p.is_empty()); + let base = parts.next()?; + let user = parts.next()?; + matches!(base, "Users" | "home").then(|| PathBuf::from(format!("/{base}/{user}"))) } fn trim_trailing_punct(token: &mut String) { @@ -1481,13 +1606,19 @@ mod tests { assert_eq!((link.start, link.end), (6, 21)); } + /// Ungated with the rest: what a `~` in a POSIX pane stands for is that + /// pane's business and not its client's, and the `#[cfg(unix)]` this used + /// to carry described the wrong machine. #[test] - #[cfg(unix)] fn tilde_expansion_prefers_home_inferred_from_the_pane_cwd() { - let cwd = Path::new("/Users/alice/clone/tty7"); + let roots = LinkRoots { + dirs: vec![PathBuf::from("/Users/alice/clone/tty7")], + local_home: true, + style: PathStyle::Posix, + }; assert_eq!( - expand_home("~/clone/tty7/src/main.rs", Some(cwd), true), - Some(PathBuf::from("/Users/alice/clone/tty7/src/main.rs")) + expand_home("~/clone/tty7/src/main.rs", &roots), + Some("/Users/alice/clone/tty7/src/main.rs".to_string()) ); } @@ -1495,17 +1626,24 @@ mod tests { /// machine: this machine's `$HOME` describes nobody there, and a path built /// out of it would be asked about — and possibly answered — on the far side. #[test] - #[cfg(unix)] fn tilde_expansion_does_not_borrow_this_machines_home_for_another_one() { - let cwd = Path::new("/srv/app"); - assert_eq!(expand_home("~/.zshrc", Some(cwd), false), None); + let elsewhere = |cwd: &str| LinkRoots { + dirs: vec![PathBuf::from(cwd)], + local_home: false, + style: PathStyle::Posix, + }; + assert_eq!(expand_home("~/.zshrc", &elsewhere("/srv/app")), None); assert_eq!( - expand_home("~/.zshrc", Some(Path::new("/home/deploy/app")), false), - Some(PathBuf::from("/home/deploy/.zshrc")), + expand_home("~/.zshrc", &elsewhere("/home/deploy/app")), + Some("/home/deploy/.zshrc".to_string()), "a cwd that does reveal the home needs nothing from us" ); assert!( - expand_home("~/.zshrc", Some(cwd), true).is_some(), + expand_home( + "~/.zshrc", + &LinkRoots::local(vec![PathBuf::from("/srv/app")]) + ) + .is_some(), "a local pane still falls back to the environment" ); } @@ -1682,14 +1820,25 @@ mod tests { fn a_path_shaped_token_is_kept_apart_from_a_bare_word() { let path_shaped = file_candidate_at("wrote scratchpad/notes.md now", 8).expect("candidate"); assert_eq!(path_shaped.path, "scratchpad/notes.md"); - assert!(path_shaped.looks_like_a_path()); + assert!(path_shaped.looks_like_a_path(PathStyle::NATIVE)); let word = file_candidate_at("wrote notes now", 8).expect("candidate"); assert_eq!(word.path, "notes"); assert!( - !word.looks_like_a_path(), + !word.looks_like_a_path(PathStyle::NATIVE), "a bare word must not raise a notification on every modifier-click" ); + + let escaped = file_candidate_at(r"wrote a\b now", 6).expect("candidate"); + assert_eq!(escaped.path, r"a\b"); + assert!( + escaped.looks_like_a_path(PathStyle::Windows), + "a backslash separates directories in a Windows pane" + ); + assert!( + !escaped.looks_like_a_path(PathStyle::Posix), + "and escapes a space in a POSIX one, whatever this client runs" + ); } #[test] @@ -1721,23 +1870,230 @@ mod tests { /// `is_rooted` decides whether a report about an unresolved token may name /// a directory it was "looked for under", so it has to agree with /// [`FileCandidate::paths`] about when the roots are consulted at all. - /// Both ask `is_absolute`, and on Windows a leading `/` does not make a - /// path that — which is why this only claims to hold where it does. + /// + /// Both used to ask `Path::is_absolute`, which answers for the machine + /// tty7 runs on rather than the one the pane's paths are on — so this only + /// held on Unix and was gated to it. Both now ask the pane's own dialect, + /// and the agreement holds on every client. #[test] - #[cfg(unix)] fn a_rooted_candidate_is_told_apart_from_one_measured_from_a_root() { + let posix_roots = LinkRoots { + dirs: vec![PathBuf::from("/home/u/proj")], + local_home: false, + style: PathStyle::Posix, + }; for line in ["open /etc/hosts now", "open ~/.zshrc now"] { + let candidate = file_candidate_at(line, 6).expect("candidate"); assert!( - file_candidate_at(line, 6).expect("candidate").is_rooted(), + candidate.is_rooted(PathStyle::Posix), + "{line} says for itself where it starts" + ); + let paths = candidate.paths(&posix_roots); + assert!( + !paths.iter().any(|p| p.starts_with("/home/u/proj")), + "{line} was not measured from the pane's directory: {paths:?}" + ); + } + for line in [r"open C:\Windows\win.ini now", "open ~/.gitconfig now"] { + assert!( + file_candidate_at(line, 6) + .expect("candidate") + .is_rooted(PathStyle::Windows), "{line} says for itself where it starts" ); } - assert!( - !file_candidate_at("see src/lib.rs here", 5) - .expect("candidate") - .is_rooted(), - "a relative path is only ever found by measuring from somewhere" + for style in [PathStyle::Posix, PathStyle::Windows] { + assert!( + !file_candidate_at("see src/lib.rs here", 5) + .expect("candidate") + .is_rooted(style), + "a relative path is only ever found by measuring from somewhere" + ); + } + } + + /// The bug this whole [`PathStyle`] exists for: a Windows tty7 looking at + /// a Linux pane — a remote workspace, a native-SSH pane, or a WSL distro + /// reached as a host — used to read `/etc/hosts` as a *relative* path, + /// because `Path::is_absolute` speaks for the client and a leading `/` is + /// not absolute on Windows. The pane's paths were then measured from its + /// own directory and the far side was asked about something it never + /// printed, so nothing ever underlined. + #[test] + fn a_posix_pane_roots_its_own_paths_on_every_client() { + let roots = LinkRoots { + dirs: vec![PathBuf::from("/home/u/proj")], + local_home: false, + style: PathStyle::Posix, + }; + let candidate = file_candidate_at("open /etc/hosts now", 6).expect("candidate"); + assert_eq!( + candidate.paths(&roots), + vec![PathBuf::from("/etc/hosts")], + "the pane said where the path starts; the roots have nothing to add" ); + + // And with no cwd reported at all there is still exactly one thing it + // can mean. This is the case that failed outright: no roots meant no + // paths, so the host was never even asked. + let roots = LinkRoots { + dirs: Vec::new(), + local_home: false, + style: PathStyle::Posix, + }; + assert_eq!(candidate.paths(&roots), vec![PathBuf::from("/etc/hosts")]); + } + + /// A relative path is the other half, and it fails more quietly: the join + /// used to reach for the *client's* separator, so a Windows tty7 asked a + /// Linux host about `/home/u/proj\src/lib.rs`. + #[test] + fn a_posix_pane_joins_a_relative_path_with_its_own_separator() { + let roots = LinkRoots { + dirs: vec![PathBuf::from("/home/u/proj"), PathBuf::from("/home/u")], + local_home: false, + style: PathStyle::Posix, + }; + let paths = file_candidate_at("see src/lib.rs here", 5) + .expect("candidate") + .paths(&roots); + assert_eq!( + paths + .iter() + .map(|p| p.to_string_lossy()) + .collect::>(), + vec!["/home/u/proj/src/lib.rs", "/home/u/src/lib.rs"], + "the string the far side is asked about has to be one it can stat" + ); + } + + /// `~` in a POSIX pane is read out of the pane's own cwd — on every + /// client. The rule was gated to Unix ones, so a Windows tty7 could not + /// resolve `~/.zshrc` in any pane it was looking at. + #[test] + fn a_posix_pane_reads_a_tilde_out_of_its_own_cwd() { + let roots = LinkRoots { + dirs: vec![PathBuf::from("/home/u/proj")], + local_home: false, + style: PathStyle::Posix, + }; + let paths = file_candidate_at("open ~/.zshrc now", 6) + .expect("candidate") + .paths(&roots); + assert_eq!( + paths + .iter() + .map(|p| p.to_string_lossy()) + .collect::>(), + vec!["/home/u/.zshrc"] + ); + + let no_home = LinkRoots { + dirs: vec![PathBuf::from("/srv/app")], + local_home: false, + style: PathStyle::Posix, + }; + assert!( + file_candidate_at("open ~/.zshrc now", 6) + .expect("candidate") + .paths(&no_home) + .is_empty(), + "a cwd that reveals no home may not borrow this machine's (#568)" + ); + } + + /// The deliberate other half: a pane *on this machine* keeps this OS's + /// reading of its own output. `/etc` in a `cmd.exe` pane sitting on `C:` + /// means `C:\etc`, the way `cd /etc` does there — so it is measured from + /// the pane's root and not turned into a link to a file Windows has not + /// got. Pinned because the temptation is to make every `/`-rooted token + /// stand alone, and that would underline `/etc/hosts` in a PowerShell pane + /// with nothing behind it. + #[test] + fn a_local_pane_reads_its_own_output_the_way_its_own_os_does() { + let candidate = file_candidate_at("open /etc/hosts now", 6).expect("candidate"); + assert_eq!( + candidate.is_rooted(PathStyle::NATIVE), + cfg!(unix), + "a leading slash roots a path on Unix and names a drive-relative \ + directory on Windows" + ); + + let roots = LinkRoots::local(vec![PathBuf::from("/w")]); + assert_eq!( + roots.style, + PathStyle::NATIVE, + "a pane on this machine spells paths this machine's way" + ); + assert_eq!( + candidate.paths(&roots), + vec![PathBuf::from("/etc/hosts")], + "which comes to the same thing under a root with no drive letter" + ); + } + + /// The Windows half of the rule above, spelled out where it can be: the + /// drive the pane is on is what a leading `/` there is measured from. + /// Cannot be asserted on a Unix client, where `PathBuf` has no notion of a + /// drive at all — the *rule* is pinned ungated above, this is the reading. + #[test] + #[cfg(windows)] + fn a_local_windows_pane_measures_a_leading_slash_from_its_own_drive() { + let roots = LinkRoots::local(vec![PathBuf::from(r"C:\proj")]); + assert_eq!( + file_candidate_at("open /etc/hosts now", 6) + .expect("candidate") + .paths(&roots), + vec![PathBuf::from(r"C:\etc\hosts")], + "`/etc` in a cmd.exe pane on C: is C:\\etc, the way `cd /etc` is" + ); + } + + #[test] + fn a_windows_pane_roots_a_drive_letter_and_a_share() { + let style = PathStyle::Windows; + for rooted in [r"C:\Windows", "C:/Windows", r"\\server\share\x"] { + assert!(style.is_absolute(rooted), "{rooted} names its own root"); + } + for measured in ["/etc/hosts", "C:notes.txt", r"src\lib.rs", "", "C:"] { + assert!( + !style.is_absolute(measured), + "{measured} has to be measured from somewhere" + ); + } + for measured in [r"C:\Windows", r"src\lib.rs", ""] { + assert!( + !PathStyle::Posix.is_absolute(measured), + "{measured} is not a POSIX root" + ); + } + } + + #[test] + fn a_hosts_dialect_is_read_off_the_directory_it_reports() { + assert_eq!( + PathStyle::of_dir(Path::new("/home/u/proj")), + PathStyle::Posix + ); + assert_eq!( + PathStyle::of_dir(Path::new(r"C:\Users\u\proj")), + PathStyle::Windows + ); + assert_eq!( + PathStyle::of_dir(Path::new(r"\\wsl$\Ubuntu\home\u")), + PathStyle::Windows + ); + // A directory that names no root at all says nothing about the host, + // so it has to fall the way no directory does — POSIX. Reading it as + // Windows would hand a Linux host `\`-joined paths on the strength of + // a cwd it never really reported. + for nothing_to_go_on in ["", "proj", "~/proj", "C:notes"] { + assert_eq!( + PathStyle::of_dir(Path::new(nothing_to_go_on)), + PathStyle::Posix, + "{nothing_to_go_on:?} names no Windows root" + ); + } } #[test] diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 316d10cd..5978fd8f 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -219,6 +219,30 @@ fn cwd_is_on_host(pane_runs_remotely: bool, host_is_local: bool) -> bool { } } +/// Which path dialect a pane's output is written in. +/// +/// A pane running on this machine spells paths the way this OS does, and that +/// is the end of it: `/etc` printed by a `cmd.exe` pane sitting on `C:` means +/// `C:\etc`, exactly as `cd /etc` would there. Reading it as a rooted POSIX +/// path would underline a file this machine has not got, and a link that +/// cannot be opened is worse than no link. +/// +/// A pane whose paths live somewhere else is asked instead — by the only thing +/// that host ever says about its own spelling, the directory it reports. A +/// `/`-rooted cwd is a POSIX host's. A pane that has not said where it is +/// falls to POSIX: there is no local drive to measure it from either way, and +/// every host tty7 installs a server on over SSH or WSL spells paths that way. +fn link_path_style( + paths_are_local: bool, + host_cwd: Option<&std::path::Path>, +) -> super::search::PathStyle { + use super::search::PathStyle; + match paths_are_local { + true => PathStyle::NATIVE, + false => host_cwd.map_or(PathStyle::Posix, PathStyle::of_dir), + } +} + pub struct TerminalView { pub terminal: RemoteTerminal, host_id: crate::ui::host_ops::HostId, @@ -5327,9 +5351,10 @@ impl TerminalView { window: &mut Window, cx: &mut Context, ) -> bool { + let roots = self.link_roots(cx); // A word that is not written like a path was never a link, and saying // so on every modifier-click over ordinary output would be noise. - if !candidate.looks_like_a_path() { + if !candidate.looks_like_a_path(roots.style) { return false; } // The host has not answered yet. The underline is the promise that it @@ -5340,10 +5365,10 @@ impl TerminalView { // An absolute or `~`-rooted path was never measured from anywhere, so // naming a directory it was "looked for under" would send the user to // somewhere nothing was ever asked about. - let rooted = candidate.is_rooted(); + let rooted = candidate.is_rooted(roots.style); let root = match rooted { true => None, - false => self.link_roots(cx).dirs.into_iter().next(), + false => roots.dirs.into_iter().next(), }; let message = match root { Some(root) => t_fmt( @@ -5596,10 +5621,12 @@ impl TerminalView { /// that exists in both is the near one. fn link_roots(&mut self, cx: &mut Context) -> super::search::LinkRoots { let local_home = self.host_id.is_local(); + let style = self.link_path_style(); let Some(cwd) = self.effective_host_cwd() else { return super::search::LinkRoots { dirs: Vec::new(), local_home, + style, }; }; self.request_link_repo_root(&cwd, cx); @@ -5610,7 +5637,17 @@ impl TerminalView { { dirs.push(root.clone()); } - super::search::LinkRoots { dirs, local_home } + super::search::LinkRoots { + dirs, + local_home, + style, + } + } + + /// Which path dialect this pane's output is written in — see + /// [`link_path_style`]. + fn link_path_style(&self) -> super::search::PathStyle { + link_path_style(self.paths_are_local(), self.effective_host_cwd().as_deref()) } fn request_link_repo_root(&mut self, cwd: &std::path::Path, cx: &mut Context) { @@ -7285,7 +7322,7 @@ mod tests { use super::{ COMPLETION_MENU_MAX_W, LoopbackPlan, RawInput, SelectEndCopy, Typeahead, WheelRoute, clipboard_paste_text, compose_notification_title, cwd_is_on_host, display_width, - is_typeahead_interrupt, loopback_plan, observe_typeahead_for_owner, + is_typeahead_interrupt, link_path_style, loopback_plan, observe_typeahead_for_owner, }; use super::{SCROLL_ANIM_FRAME, scroll_anim_step}; use super::{ @@ -8812,6 +8849,39 @@ mod tests { assert!(!cwd_is_on_host(false, false)); } + /// Which machine's spelling a pane's paths are read in. Ungated on + /// purpose: the bug this settles was a Windows-only one that hid behind a + /// `#[cfg(unix)]` on the test that covered it. + #[test] + fn a_panes_paths_are_read_in_its_own_hosts_spelling() { + use super::super::search::PathStyle; + use std::path::Path; + + assert_eq!( + link_path_style(true, Some(Path::new("/home/u/proj"))), + PathStyle::NATIVE, + "a pane on this machine reads its own output this OS's way, \ + whatever its shell spells the cwd like" + ); + assert_eq!( + link_path_style(false, Some(Path::new("/home/u/proj"))), + PathStyle::Posix, + "an SSH host, a remote workspace or a WSL distro reporting a \ + /-rooted cwd is a POSIX one on every client" + ); + assert_eq!( + link_path_style(false, Some(Path::new(r"C:\Users\u\proj"))), + PathStyle::Windows, + "and a remote Windows host is not" + ); + assert_eq!( + link_path_style(false, None), + PathStyle::Posix, + "a remote pane that has not said where it is still has no local \ + drive its paths could hang off" + ); + } + #[test] fn a_panes_host_is_its_workspaces_machine() { use crate::core::session::{RemoteTarget, WorkspaceId}; @@ -9500,7 +9570,7 @@ mod gpui_tests { LinkAt::Unresolved { candidate, pending } => { assert_eq!(candidate.path, "scratchpad/gone.md"); assert!( - candidate.looks_like_a_path(), + candidate.looks_like_a_path(view.link_path_style()), "so the click reports it instead of staying silent" ); assert!(!pending, "a local pane answers on the spot"); From 23919628c057724c6129047e967650869237e3e1 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:43:00 +0800 Subject: [PATCH 12/15] fix(settings): say what turning off mouse reporting costs (#780) --- src/ui/i18n/en.rs | 3 ++- src/ui/i18n/ja.rs | 3 ++- src/ui/i18n/zh.rs | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 81949a5f..ea205f5d 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -437,7 +437,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SettingsMouseZoomOff => "Off", L10nKey::SettingsReportMouseToApps => "Report mouse to apps", L10nKey::SettingsReportMouseToAppsDesc => { - "Let full-screen apps (vim, tmux) handle clicks and scrolling; hold Shift to keep a gesture local." + "Let full-screen apps (vim, tmux) handle clicks and scrolling; hold Shift to keep a gesture local. \ + Off keeps clicks from reaching them and turns the wheel into arrow keys." } L10nKey::SettingsBell => "Bell", L10nKey::SettingsTerminalBell => "Terminal bell", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 16305f28..ce986fc0 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -448,7 +448,8 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsMouseZoomOff => "オフ", L10nKey::SettingsReportMouseToApps => "マウスイベントをアプリに報告", L10nKey::SettingsReportMouseToAppsDesc => { - "フルスクリーンアプリ（vim、tmux）にクリックとスクロールを処理させる。Shift を押している間はローカルで処理されます" + "フルスクリーンアプリ（vim、tmux）にクリックとスクロールを処理させる。Shift を押している間はローカルで処理されます。\ + オフにするとクリックは届かず、ホイールは矢印キーとして送られます" } L10nKey::SettingsBell => "ベル通知", L10nKey::SettingsTerminalBell => "ターミナルベル", diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 6cf65437..7063697a 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -385,7 +385,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsMouseZoomOff => "关闭", L10nKey::SettingsReportMouseToApps => "向应用报告鼠标", L10nKey::SettingsReportMouseToAppsDesc => { - "让全屏应用（如 vim、tmux）处理点击和滚动；按住 Shift 可让操作保持本地。" + "让全屏应用（如 vim、tmux）处理点击和滚动；按住 Shift 可让操作保持本地。\ + 关闭后点击不再传给它们，滚轮也会变成方向键。" } L10nKey::SettingsBell => "铃声", L10nKey::SettingsTerminalBell => "终端铃声", From 8c1315b7ab05c5e762daf9b6339398abe49a6cf0 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:48:39 +0800 Subject: [PATCH 13/15] feat(ssh): edit the host from the tab that is connected to it (#801) Refs #438. --- docs/remote/ssh.mdx | 8 ++ src/terminal/view.rs | 44 ++++++++-- src/ui/ssh_connect.rs | 102 ++++++++++++++++++++++- src/ui/switcher.rs | 8 +- src/ui/tab_strip.rs | 188 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 339 insertions(+), 11 deletions(-) diff --git a/docs/remote/ssh.mdx b/docs/remote/ssh.mdx index ea42d507..8c84afa0 100644 --- a/docs/remote/ssh.mdx +++ b/docs/remote/ssh.mdx @@ -67,6 +67,14 @@ instead of a password echoing into your shell. **Defaults** at the top of the list is inherited by every host, so a setting you want everywhere is set once. +Right-clicking an SSH tab opens that connection's host form — **Edit Host…** +for a saved one, **Save as SSH Host…** for an address typed by hand. It is the +same row the workspace switcher's machine menu carries, so a hostname or +password typed wrong is corrected from the tab you noticed it on. The menu acts +on the tab it was opened on, not on whichever pane is focused. Saving one opens +on the whole live connection — its proxy, keys and forwards as well as its +address — so the host that lands is the one you were already on. + Passwords and key passphrases go in the **OS keychain**, never in `config.json` and never on disk in plain text. **Forget Password** in a profile's menu removes the stored one. diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 5978fd8f..1fc29a53 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -8940,20 +8940,48 @@ pub(crate) fn quiet_test_pane( (view, daemon_side) } -#[cfg(all(test, unix))] +/// A quiet pane that was dialled by hand, with no saved host behind it. +/// +/// Ungated on purpose: the transport this hands back is already +/// platform-neutral, and gating it left every test that wanted an SSH pane +/// silently skipped on Windows. +#[cfg(test)] pub(crate) fn quiet_test_ssh_pane( pane_id: u64, window: &mut Window, cx: &mut gpui::App, -) -> (gpui::Entity, std::os::unix::net::UnixStream) { +) -> (gpui::Entity, crate::daemon::transport::Stream) { + quiet_test_ssh_pane_of(pane_id, None, window, cx) +} + +/// The same, for a pane opened from a saved host — `profile_id` is what tells +/// the two apart everywhere the connection is offered back to the user. +#[cfg(test)] +pub(crate) fn quiet_test_ssh_pane_of( + pane_id: u64, + profile_id: Option, + window: &mut Window, + cx: &mut gpui::App, +) -> (gpui::Entity, crate::daemon::transport::Stream) { + let mut spec: crate::daemon::protocol::NativeSshSpec = + serde_json::from_str(r#"{"host":"build-box","port":22,"user":"me","auth_mode":"auto"}"#) + .expect("a minimal NativeSshSpec decodes"); + spec.profile_id = profile_id.map(|id| id.to_string()); + quiet_test_ssh_pane_with(pane_id, spec, window, cx) +} + +/// The same again, over a spec the caller shaped — for everything a live +/// connection carries beyond its address. +#[cfg(test)] +pub(crate) fn quiet_test_ssh_pane_with( + pane_id: u64, + spec: crate::daemon::protocol::NativeSshSpec, + window: &mut Window, + cx: &mut gpui::App, +) -> (gpui::Entity, crate::daemon::transport::Stream) { let (view, stream) = quiet_test_pane(pane_id, window, cx); view.update(cx, |view, _| { - view.ssh_spec = Some(Box::new( - serde_json::from_str( - r#"{"host":"build-box","port":22,"user":"me","auth_mode":"auto"}"#, - ) - .expect("a minimal NativeSshSpec decodes"), - )); + view.ssh_spec = Some(Box::new(spec)); }); (view, stream) } diff --git a/src/ui/ssh_connect.rs b/src/ui/ssh_connect.rs index adc03a3d..167543fe 100644 --- a/src/ui/ssh_connect.rs +++ b/src/ui/ssh_connect.rs @@ -186,7 +186,18 @@ impl Tty7App { let Some(spec) = self.unsaved_ssh_session(window, cx) else { return; }; - let profile = profile_from_live_spec(&spec); + self.save_ssh_spec_as_host(&spec, window, cx); + } + + /// The same form, for a connection named by the caller rather than by the + /// focus — the tab menu's row offers it for the tab it was opened on. + pub(crate) fn save_ssh_spec_as_host( + &mut self, + spec: &NativeSshSpec, + window: &mut gpui::Window, + cx: &mut gpui::Context, + ) { + let profile = profile_from_live_spec(spec); let jumped = spec.jump.is_some(); self.open_settings_section(crate::ui::settings::SettingsSection::Ssh, window, cx); self.ssh_form_load(&profile, window, cx); @@ -244,6 +255,52 @@ impl Tty7App { } } + /// The host form a tab's own context menu offers, and what the row calls + /// it — read off the tab the menu was opened on rather than off whichever + /// pane happens to be focused, so right-clicking a background tab reaches + /// that tab's connection. + /// + /// `None` for a tab there is no host form to open: a local shell and a + /// remote workspace pane were never dialled with an SSH spec of their own, + /// and a WSL distro is configured nowhere this form could edit. The label + /// comes from the same [`host_form_label`] the switcher's machine menu + /// uses, so the two rows cannot drift apart. + /// + /// [`host_form_label`]: crate::ui::switcher::host_form_label + pub(crate) fn tab_ssh_host_form( + &self, + index: usize, + window: &gpui::Window, + cx: &gpui::App, + ) -> Option<(TabHostForm, &'static str)> { + let leaf = self.tabs.get(index)?.pane.focused_or_first(window, cx)?; + let spec = leaf.read(cx).ssh_spec()?; + let target = ssh_host_target_of_spec(&spec, &cx.global::().ssh_profiles); + let label = crate::ui::switcher::host_form_label(&target)?; + let form = match target { + crate::core::session::RemoteTarget::Profile { .. } => TabHostForm::Saved(target), + _ => TabHostForm::Unsaved(spec), + }; + Some((form, label)) + } + + /// Open what the row offered. A saved host goes to its own record; an + /// unsaved one goes through the same "save this connection" path the + /// command already uses, so the proxy, the identity files and the forwards + /// the session was dialled with land in the draft rather than being + /// thrown away with everything that does not fit in `user@host:port`. + pub(crate) fn open_tab_ssh_host_form( + &mut self, + form: &TabHostForm, + window: &mut gpui::Window, + cx: &mut gpui::Context, + ) { + match form { + TabHostForm::Saved(target) => self.edit_ssh_host_of_target(target, window, cx), + TabHostForm::Unsaved(spec) => self.save_ssh_spec_as_host(spec, window, cx), + } + } + fn bump_ssh_frecency(&mut self, profile_id: uuid::Uuid, cx: &mut gpui::Context) { self.update_config(cx, |cfg| { let entry = cfg.ssh_profile_frecency.entry(profile_id).or_default(); @@ -409,6 +466,49 @@ fn build_spec_inner( } } +/// What a tab's host row opens when it is taken. +/// +/// The two halves are not the same form. A saved host is already a record, so +/// it is addressed by the target that names it and nothing about the live +/// session is needed. An unsaved one is only ever the session, and it goes to +/// the form whole: an address dialled by hand carries a proxy, a jump host, +/// identity files and forwards, and a draft built from `user@host:port` alone +/// would save fine and then not connect. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum TabHostForm { + Saved(crate::core::session::RemoteTarget), + Unsaved(Box), +} + +/// Which host form a live connection belongs to: the saved host it was opened +/// from, or the address it was dialled by. +/// +/// A transient profile is handed a fresh uuid on its way to the daemon, so an +/// id alone does not mean a host was saved — only one that still resolves +/// against the saved list does. Anything else is an address worth keeping. +/// +/// The `Direct` this hands back is the gate and the label, not the draft: +/// [`host_form_label`] reads it to decide the row exists and what it says, +/// while the form itself opens on the whole live spec, which carries far more +/// than an address does. +/// +/// [`host_form_label`]: crate::ui::switcher::host_form_label +pub(crate) fn ssh_host_target_of_spec( + spec: &NativeSshSpec, + profiles: &[SshProfile], +) -> crate::core::session::RemoteTarget { + use crate::core::session::RemoteTarget; + let saved = spec + .profile_id + .as_deref() + .and_then(|s| Uuid::parse_str(s).ok()) + .filter(|id| profiles.iter().any(|p| p.id == *id)); + match saved { + Some(id) => RemoteTarget::Profile { id }, + None => RemoteTarget::direct(spec.user.clone(), spec.host.clone(), spec.port), + } +} + /// A live connection read back as a profile someone could keep — the return /// leg of [`build_native_ssh_spec`], for a session that was dialled by hand /// and turned out to be worth saving. diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 2468a40c..aca45583 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -3066,10 +3066,14 @@ impl RowRef { } } -/// What the machine menu's host row says, or `None` for a machine that has no +/// What a host row offering the form says, or `None` for a machine that has no /// SSH host behind it at all — WSL and the local stdio server are configured /// nowhere this form could edit. -fn host_form_label(target: &RemoteTarget) -> Option<&'static str> { +/// +/// Shared with the tab menu, which offers the same row for the connection a tab +/// is on (#438), so the two surfaces cannot drift on which machines are +/// editable or on what the row is called. +pub(crate) fn host_form_label(target: &RemoteTarget) -> Option<&'static str> { match target { RemoteTarget::Profile { .. } => Some(t(L10nKey::SwitcherEditHost)), RemoteTarget::Alias { .. } | RemoteTarget::Direct { .. } => { diff --git a/src/ui/tab_strip.rs b/src/ui/tab_strip.rs index eb4f5198..33982dd7 100644 --- a/src/ui/tab_strip.rs +++ b/src/ui/tab_strip.rs @@ -1426,6 +1426,25 @@ impl Tty7App { ); } + // The connection this tab is on, editable from the tab itself. A + // hostname or password typed wrong used to be fixable only by finding + // the same host again in Settings, and right-clicking the connection — + // the gesture that asks "change this" — offered nothing (#438). The row + // is the switcher machine menu's, word for word: the saved host when + // there is one, an offer to keep the address when it was dialled by + // hand, and nothing at all for a tab with no host form behind it. + if let Some((form, label)) = this.tab_ssh_host_form(index, window, cx) { + menu = menu.separator().item(PopupMenuItem::new(label).on_click({ + let app = app.clone(); + move |_, window, cx| { + let form = form.clone(); + let _ = app.update(cx, |this, cx| { + this.open_tab_ssh_host_form(&form, window, cx) + }); + } + })); + } + menu = menu .separator() .item( @@ -1907,6 +1926,175 @@ impl Tty7App { } } +/// The tab menu's SSH row, against real tabs in a real window. +/// +/// `PopupMenu` keeps its items to itself — nothing outside `gpui_component` can +/// read back what a built menu says — so these drive the predicate the menu +/// branches on instead, which is where every decision about the row is made. +/// +/// Ungated: `test_window::harness` and `quiet_test_pane` both run on Windows, +/// and a `unix` gate here would skip the one platform this was written on. +#[cfg(test)] +mod ssh_host_row_tests { + use crate::core::config::Config; + use crate::core::session::RemoteTarget; + use crate::core::ssh_profile::SshProfile; + use crate::daemon::protocol::SshProxy; + use crate::terminal::view::{ + quiet_test_pane, quiet_test_ssh_pane, quiet_test_ssh_pane_of, quiet_test_ssh_pane_with, + }; + use crate::ui::app::{Tab, test_window::harness}; + use crate::ui::i18n::{L10nKey, set_locale, t}; + use crate::ui::pane::{Pane, PaneSlot}; + use crate::ui::ssh_connect::TabHostForm; + use gpui::TestAppContext; + + #[gpui::test] + fn only_a_tab_on_an_ssh_host_is_offered_the_host_form(cx: &mut TestAppContext) { + set_locale("en"); + let (app, mut vcx) = harness(cx); + let saved = uuid::Uuid::new_v4(); + + // Held for the life of the test: dropping the daemon end of a pane's + // transport tears the pane down under the assertions. + let _ends = app.update_in(&mut vcx, |app, window, cx| { + let mut cfg = cx.global::().clone(); + let mut profile = SshProfile::new("build-box"); + profile.id = saved; + profile.user = "me".to_string(); + profile.host = "build-box".to_string(); + cfg.ssh_profiles = vec![profile]; + cx.set_global(cfg); + + let (local, a) = quiet_test_pane(1, window, cx); + let (dialled, b) = quiet_test_ssh_pane(2, window, cx); + let (from_host, c) = quiet_test_ssh_pane_of(3, Some(saved), window, cx); + for view in [local, dialled, from_host] { + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(view)))); + } + app.active = 0; + cx.notify(); + (a, b, c) + }); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| { + // A local shell has no connection to edit, so the menu it opens is + // the one it always was. + assert_eq!( + app.tab_ssh_host_form(0, window, cx), + None, + "a local tab was offered an SSH host form" + ); + + // An address typed by hand is worth keeping, not editing: there is + // no saved host behind it yet, so the live session itself is what + // the form opens on. + let (form, label) = app + .tab_ssh_host_form(1, window, cx) + .expect("a tab dialled by hand offers to save the host"); + let TabHostForm::Unsaved(spec) = form else { + panic!("a hand-dialled tab must offer its own session, not a bare address"); + }; + assert_eq!( + (spec.user.as_str(), spec.host.as_str(), spec.port), + ("me", "build-box", 22) + ); + assert_eq!(label, t(L10nKey::SwitcherSaveAsHost)); + + // One opened from a saved host edits that host — by its id, so the + // form lands on the record the connection actually came from. + let (form, label) = app + .tab_ssh_host_form(2, window, cx) + .expect("a tab on a saved host offers to edit it"); + assert_eq!( + form, + TabHostForm::Saved(RemoteTarget::Profile { id: saved }) + ); + assert_eq!(label, t(L10nKey::SwitcherEditHost)); + }); + } + + #[gpui::test] + fn a_host_deleted_under_a_live_tab_is_offered_back_as_a_new_one(cx: &mut TestAppContext) { + // The id a pane carries is the one it was spawned with, and a quick + // connection is handed a fresh uuid on its way to the daemon. Trusting + // the id alone would open the form on a host that is not there. + set_locale("en"); + let (app, mut vcx) = harness(cx); + let _end = app.update_in(&mut vcx, |app, window, cx| { + let (view, end) = quiet_test_ssh_pane_of(1, Some(uuid::Uuid::new_v4()), window, cx); + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(view)))); + app.active = 0; + cx.notify(); + end + }); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| { + let (form, label) = app + .tab_ssh_host_form(0, window, cx) + .expect("an unresolvable profile id still names a host worth keeping"); + let TabHostForm::Unsaved(spec) = form else { + panic!("a dangling profile id must not open a form on a host that is gone"); + }; + assert_eq!( + (spec.user.as_str(), spec.host.as_str(), spec.port), + ("me", "build-box", 22) + ); + assert_eq!(label, t(L10nKey::SwitcherSaveAsHost)); + }); + } + + /// The row says "Save as SSH Host", and a host saved without the proxy it + /// was reached through is a host that will not connect. What the session + /// was dialled with has to reach the form whole — an address is only the + /// part of it that fits in `user@host:port`. + #[gpui::test] + fn saving_a_hand_dialled_tab_keeps_what_it_was_dialled_with(cx: &mut TestAppContext) { + set_locale("en"); + let (app, mut vcx) = harness(cx); + let _end = app.update_in(&mut vcx, |app, window, cx| { + let mut spec: crate::daemon::protocol::NativeSshSpec = serde_json::from_str( + r#"{"host":"build-box","port":2222,"user":"me","auth_mode":"auto"}"#, + ) + .expect("a minimal NativeSshSpec decodes"); + spec.proxy = SshProxy::Socks { + host: "127.0.0.1".to_string(), + port: 1080, + }; + spec.identity_files = vec!["/keys/id_ed25519".to_string()]; + spec.login_script = vec!["tmux attach".to_string()]; + let (view, end) = quiet_test_ssh_pane_with(1, spec, window, cx); + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(view)))); + app.active = 0; + cx.notify(); + end + }); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| { + let (form, _) = app + .tab_ssh_host_form(0, window, cx) + .expect("a hand-dialled tab offers to save the host"); + let TabHostForm::Unsaved(spec) = form else { + panic!("nothing here is saved, so nothing here is an edit"); + }; + assert_eq!( + spec.proxy, + SshProxy::Socks { + host: "127.0.0.1".to_string(), + port: 1080, + }, + "the proxy the session was reached through was dropped on the way to the form" + ); + assert_eq!(spec.identity_files, vec!["/keys/id_ed25519".to_string()]); + assert_eq!(spec.login_script, vec!["tmux attach".to_string()]); + assert_eq!(spec.port, 2222, "a non-default port is part of the address"); + }); + } +} + #[cfg(test)] mod tests { use super::*; From 28530a476a26a16b20140632c38cfd9532be418f Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:55:01 +0800 Subject: [PATCH 14/15] fix(git): key a repository by one spelling of its root (#796) --- crates/tty7-core/src/core/git/diff.rs | 2 +- crates/tty7-core/src/core/git/mod.rs | 130 ++++++- crates/tty7-core/src/core/git/status.rs | 6 +- crates/tty7-core/src/core/mod.rs | 1 + crates/tty7-core/src/core/path_spelling.rs | 409 +++++++++++++++++++++ crates/tty7-core/src/core/worktree.rs | 12 +- crates/tty7-core/src/host/local.rs | 15 +- src/terminal/git_status.rs | 240 +++++++++++- src/ui/path_display.rs | 57 +-- src/ui/scm/panel.rs | 8 +- 10 files changed, 801 insertions(+), 79 deletions(-) create mode 100644 crates/tty7-core/src/core/path_spelling.rs diff --git a/crates/tty7-core/src/core/git/diff.rs b/crates/tty7-core/src/core/git/diff.rs index cb27a3a3..552cfa90 100644 --- a/crates/tty7-core/src/core/git/diff.rs +++ b/crates/tty7-core/src/core/git/diff.rs @@ -428,7 +428,7 @@ pub fn probe_diff(host: &dyn Host, root: &Path, req: &DiffRequest<'_>) -> Option return None; } let toplevel = git::git(host, root, &["rev-parse", "--show-toplevel"])?; - let toplevel = PathBuf::from(toplevel.trim_end_matches(['\n', '\r'])); + let toplevel = git::git_path(host, toplevel.trim_end_matches(['\n', '\r'])); let branch = git::branch_name(host, root)?; let argv = req.args(); diff --git a/crates/tty7-core/src/core/git/mod.rs b/crates/tty7-core/src/core/git/mod.rs index 73536ea1..07314e7e 100644 --- a/crates/tty7-core/src/core/git/mod.rs +++ b/crates/tty7-core/src/core/git/mod.rs @@ -48,8 +48,8 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { ], )?; let mut lines = paths.lines().map(|l| l.trim_end_matches(['\n', '\r'])); - let root = PathBuf::from(lines.next()?); - let home = repo_home(&root, lines.next(), lines.next()); + let root = git_path(host, lines.next()?); + let home = repo_home(host, &root, lines.next(), lines.next()); let branch = branch_name(host, cwd)?; Some(RepoSnapshot { home, @@ -59,18 +59,43 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { }) } -pub(crate) fn repo_home(root: &Path, git_dir: Option<&str>, common_dir: Option<&str>) -> PathBuf { +/// A path `git` just printed, in the spelling the rest of tty7 keys by. +/// +/// Git for Windows is MSYS2 and answers `rev-parse` with `C:/Users/…` whatever +/// shell asked it. `Path` forgives that much on its own, but the same root also +/// has to compare equal to one that came past `fs::canonicalize` — which spells +/// it `\\?\C:\Users\…`, a different prefix component and so a different key. +/// One spelling at the boundary, rather than a normalisation remembered at each +/// of the places these roots are later compared. See +/// [`crate::core::path_spelling`]. +/// +/// Asked of `host`, not of `cfg!(windows)`: the same probes run against a +/// remote box, whose `/home/u/src` is native over there and goes straight +/// back over the wire as the cwd of the next `git`. A Windows client +/// re-spelling it would ask a Linux server about `\home\u\src`. +pub(crate) fn git_path(host: &dyn Host, printed: &str) -> PathBuf { + crate::core::path_spelling::spelling_on_buf(host.id(), printed) +} + +pub(crate) fn repo_home( + host: &dyn Host, + root: &Path, + git_dir: Option<&str>, + common_dir: Option<&str>, +) -> PathBuf { let (Some(git_dir), Some(common)) = (git_dir, common_dir) else { return root.to_path_buf(); }; if git_dir == common { return root.to_path_buf(); } - let common = Path::new(common); - match (common.file_name(), common.parent()) { - (Some(name), Some(parent)) if name == ".git" => parent.to_path_buf(), - _ => common.to_path_buf(), + let common = git_path(host, common); + if common.file_name().is_some_and(|name| name == ".git") + && let Some(parent) = common.parent() + { + return parent.to_path_buf(); } + common } pub fn branch_name(host: &dyn Host, cwd: &Path) -> Option { @@ -659,24 +684,105 @@ mod tests { } #[test] fn repo_home_resolves_worktree_layouts() { + let host = h(); + let host = &*host; let root = Path::new("/repo/.wt/feat"); assert_eq!( - repo_home(Path::new("/repo"), Some("/repo/.git"), Some("/repo/.git")), + repo_home( + host, + Path::new("/repo"), + Some("/repo/.git"), + Some("/repo/.git") + ), PathBuf::from("/repo") ); assert_eq!( - repo_home(root, Some("/repo/.git/worktrees/feat"), Some("/repo/.git")), + repo_home( + host, + root, + Some("/repo/.git/worktrees/feat"), + Some("/repo/.git") + ), PathBuf::from("/repo") ); assert_eq!( - repo_home(root, Some("/bare.git/worktrees/feat"), Some("/bare.git")), + repo_home( + host, + root, + Some("/bare.git/worktrees/feat"), + Some("/bare.git") + ), PathBuf::from("/bare.git") ); assert_eq!( - repo_home(root, Some("/repo/.git"), None), + repo_home(host, root, Some("/repo/.git"), None), root.to_path_buf() ); - assert_eq!(repo_home(root, None, None), root.to_path_buf()); + assert_eq!(repo_home(host, root, None, None), root.to_path_buf()); + } + + /// The root every git probe answers with is the directory the *OS* names, + /// compared as a plain `PathBuf` — which is how every consumer compares + /// it. + /// + /// Not gated to any platform, deliberately. Git for Windows is MSYS2 and + /// prints `C:/Users/…`; `Host::canonicalize` used to answer `\\?\C:\Users\ + /// …`; those are different `Prefix` components, so the two never matched + /// and every SCM cache keyed by one missed the other. Nothing compared + /// them on Windows, which is exactly why nobody noticed — a `#[cfg(unix)]` + /// on this would put it straight back. + #[test] + fn a_probed_root_is_the_same_path_the_host_canonicalizes_to() { + let host = h(); + let dir = std::env::temp_dir().join(format!("tty7-root-spelling-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + let made = git(&*host, &dir, &["init", "--quiet"]).is_some(); + if !made { + let _ = std::fs::remove_dir_all(&dir); + return; // no git on this machine + } + assert!(super::test_support::pin_repo_config(&dir)); + std::fs::write(dir.join("a.txt"), "one\n").unwrap(); + let mut commit = super::test_support::PINS.to_vec(); + commit.extend_from_slice(&["commit", "--quiet", "-m", "base"]); + assert!(git(&*host, &dir, &["add", "-A"]).is_some()); + assert!(git(&*host, &dir, &commit).is_some()); + + // The one directory, under the two names this process can learn it by: + // what the OS handed back, and what resolving it answers. + let canonical = host.canonicalize(&dir).expect("the scratch dir resolves"); + + let snap = probe(&*host, &dir).expect("a repository was just created here"); + assert_eq!( + snap.root, canonical, + "the probed root and the resolved directory are one key" + ); + assert_eq!(snap.home, canonical, "and so is a non-worktree's home"); + + // Asking from the resolved spelling has to reach the same answer, or + // a pane whose cwd arrived that way lands in a second repository. + let from_canonical = + probe(&*host, &canonical).expect("the same repository, asked from its other name"); + assert_eq!(from_canonical.root, snap.root); + + // The other two probes answer the same question and must not disagree + // with it: `probe_status` keys `ScmData`, `probe_diff` keys the diff + // overlay, and a disagreement between any two of them is a re-probe + // that never settles. + let status = match super::status::probe_status(&*host, &dir) { + super::status::StatusProbe::Status(status) => *status, + other => panic!("expected a repository, got {other:?}"), + }; + assert_eq!(status.root, snap.root, "probe_status agrees with probe"); + assert_eq!(status.home, snap.home); + + let diff = super::diff::probe_diff(&*host, &dir, &Default::default()) + .expect("an empty repository still has a diff"); + assert_eq!(diff.root, snap.root, "probe_diff agrees with probe"); + + let _ = std::fs::remove_dir_all(&dir); } } diff --git a/crates/tty7-core/src/core/git/status.rs b/crates/tty7-core/src/core/git/status.rs index 20a1b4c4..7ab8a5b3 100644 --- a/crates/tty7-core/src/core/git/status.rs +++ b/crates/tty7-core/src/core/git/status.rs @@ -845,12 +845,12 @@ pub fn probe_status(host: &dyn Host, cwd: &Path) -> StatusProbe { } let paths = String::from_utf8_lossy(&out.stdout).into_owned(); let mut lines = paths.lines().map(|l| l.trim_end_matches(['\n', '\r'])); - let Some(root) = lines.next().map(PathBuf::from) else { + let Some(root) = lines.next().map(|l| super::git_path(host, l)) else { return StatusProbe::Unreachable; }; let git_dir = lines.next(); - let home = super::repo_home(&root, git_dir, lines.next()); - let Some(git_dir) = git_dir.map(PathBuf::from) else { + let home = super::repo_home(host, &root, git_dir, lines.next()); + let Some(git_dir) = git_dir.map(|l| super::git_path(host, l)) else { return StatusProbe::Unreachable; }; diff --git a/crates/tty7-core/src/core/mod.rs b/crates/tty7-core/src/core/mod.rs index eb5ba282..51853442 100644 --- a/crates/tty7-core/src/core/mod.rs +++ b/crates/tty7-core/src/core/mod.rs @@ -12,6 +12,7 @@ pub mod kitty_graphics; pub mod logfile; pub mod machine; pub mod osc; +pub mod path_spelling; pub mod proc; pub mod session; pub mod shells; diff --git a/crates/tty7-core/src/core/path_spelling.rs b/crates/tty7-core/src/core/path_spelling.rs new file mode 100644 index 00000000..5df26d44 --- /dev/null +++ b/crates/tty7-core/src/core/path_spelling.rs @@ -0,0 +1,409 @@ +//! The one spelling tty7 stores a path on **this** machine in. +//! +//! The same directory reaches this process under three names on Windows and +//! only two of them compare equal: +//! +//! - `C:\Users\x\repo` — what the OS, a shell and a pane's cwd all say; +//! - `C:/Users/x/repo` — what Git for Windows says, whatever shell asked it: +//! `rev-parse --show-toplevel` and `--git-common-dir` are MSYS2 paths and +//! always come back with forward slashes; +//! - `\\?\C:\Users\x\repo` — what [`std::fs::canonicalize`] says, because Rust +//! asks the OS for the extended-length form. +//! +//! `Path` on Windows already forgives the first two of each other: it compares, +//! hashes and prefix-matches by *component*, and both `/` and `\` end a +//! component, so a drive letter's case is folded on the way past too. What it +//! does not forgive is the third. `\\?\C:` parses as [`Prefix::VerbatimDisk`] +//! and `C:` as [`Prefix::Disk`], those are different components, and so +//! `\\?\C:\Users\x\repo != C:/Users/x/repo` — by equality, by hash, and by +//! `starts_with`. Every cache in the SCM layer is keyed by exactly that +//! comparison, so a root that came in past `canonicalize` and a root that came +//! out of `git` name the same repository and share nothing. +//! +//! [`local_spelling`] is where that is settled, once, at the boundary a path +//! is *created* at rather than at each of the places it is later compared. +//! The spelling it lands on is the plain one — native separators, no +//! extended-length prefix — because that is the one every other consumer +//! wants: the Win32 shell's `ParseDisplayName` rejects both a mixed-separator +//! path and a `\\?\` one, `git` takes either on its command line, and it is +//! the only one of the three a person would recognise in a tooltip. +//! +//! Off Windows all three collapse: `/` is the separator, there is no +//! extended-length form, and both functions here are the identity. +//! +//! [`Prefix::VerbatimDisk`]: std::path::Prefix::VerbatimDisk +//! [`Prefix::Disk`]: std::path::Prefix::Disk + +use std::borrow::Cow; +use std::path::Path; + +/// Re-spells a path on **this** machine with the separators this OS expects. +/// +/// On Windows the shell's `IShellFolder::ParseDisplayName` bails out with +/// `E_INVALIDARG` on a mixed-separator path — a forward-slash prefix joined +/// with backslash entries. The forward slashes get in from two routes: the +/// shell's PWD (OSC 7 from Git Bash / MSYS bash reports `/`, and that string +/// survives `Path::ancestors()` when the file tree walks up to find `.git`), +/// and `git rev-parse --show-toplevel` from Git for Windows (MSYS2), which +/// always prints `/` regardless of the calling shell. +/// +/// **Only for paths on the machine this window runs on.** A remote host's +/// `/home/u/src` is already native over there; re-spelling it would put a +/// path on the clipboard that names nothing on either machine. +/// +/// The rewrite runs on the path's own UTF-16 code units, not on a +/// `to_string_lossy` copy of them. A Windows filename may hold unpaired +/// surrogates, which `to_string_lossy` turns into `U+FFFD` — the returned +/// path would then silently name a *different* file. `/` and `\` are ASCII, +/// so a code unit equal to one of them is that character and never half of a +/// surrogate pair, which is what makes the swap safe to do one unit at a time. +#[cfg(windows)] +pub fn native_separators(path: &Path) -> Cow<'_, Path> { + use std::ffi::OsString; + use std::os::windows::ffi::{OsStrExt, OsStringExt}; + use std::path::PathBuf; + + let os = path.as_os_str(); + // Nothing to fix — including every UNC (`\\wsl$\…`, `\\?\…`) and + // already-native path — hands the caller's own path straight back. + if !os.encode_wide().any(|unit| unit == SLASH) { + return Cow::Borrowed(path); + } + let wide: Vec = os + .encode_wide() + .map(|unit| if unit == SLASH { BACKSLASH } else { unit }) + .collect(); + Cow::Owned(PathBuf::from(OsString::from_wide(&wide))) +} + +/// Off Windows the OS separator is already `/`, and a backslash in a path is +/// an ordinary filename character — there is nothing to re-spell. +#[cfg(not(windows))] +pub fn native_separators(path: &Path) -> Cow<'_, Path> { + Cow::Borrowed(path) +} + +#[cfg(windows)] +const SLASH: u16 = b'/' as u16; +#[cfg(windows)] +const BACKSLASH: u16 = b'\\' as u16; + +/// The spelling tty7 stores a local path in, so that two of them naming one +/// directory are one key. +/// +/// Native separators (see [`native_separators`]) *and* no extended-length +/// prefix: `\\?\C:\x` becomes `C:\x` and `\\?\UNC\srv\share` becomes +/// `\\srv\share`, which is the same path as far as every Win32 API is +/// concerned and the only spelling `Path` will compare equal to the one a +/// shell, a pane cwd or `git` reports. +/// +/// What it deliberately does **not** do: +/// +/// - **fold case.** `Path` already folds the drive letter, which is where +/// Windows case instability actually lives; folding the rest would make two +/// genuinely different names on a case-sensitive volume — or on a remote +/// host, whose paths also pass through here unchanged off Windows — collide. +/// - **trim a trailing separator.** `Path` already ignores one: `C:\x\` and +/// `C:\x` are equal, hash alike and prefix-match each other. +/// - **touch the disk.** This is a re-spelling, not a resolution: a junction, +/// a `subst` drive or an 8.3 short name is left exactly as it arrived. +/// `git` has already resolved its own answer, and a caller that wants the +/// real path calls `Host::canonicalize`, which now lands here on its way +/// out. +/// - **rewrite anything but a drive or UNC verbatim path.** `\\?\pipe\…` and +/// the other device namespaces have no plain form to fall back to. +#[cfg(windows)] +pub fn local_spelling(path: &Path) -> Cow<'_, Path> { + use std::ffi::OsString; + use std::os::windows::ffi::{OsStrExt, OsStringExt}; + use std::path::PathBuf; + + let native = native_separators(path); + let wide: Vec = native.as_os_str().encode_wide().collect(); + let Some(bare) = strip_extended_length(&wide) else { + return native; + }; + Cow::Owned(PathBuf::from(OsString::from_wide(&bare))) +} + +/// Off Windows there is no second spelling to fold into the first. +#[cfg(not(windows))] +pub fn local_spelling(path: &Path) -> Cow<'_, Path> { + Cow::Borrowed(path) +} + +/// [`local_spelling`], for a caller that is building the path anyway and has +/// nothing to hand back borrowed. +pub fn local_spelling_buf(path: impl AsRef) -> std::path::PathBuf { + local_spelling(path.as_ref()).into_owned() +} + +/// The spelling a path that lives on `host` is stored in. +/// +/// [`local_spelling`] answers for the machine this process runs on, and every +/// caller that keys a repository by its root has to ask this one instead: the +/// same caches, the same `git` probes and the same SCM panel serve a pane on +/// another machine, and `/home/u/src` from a Linux box is already native over +/// there. Re-spelling it here would send `\home\u\src` back over the +/// wire — `Host::git` puts the path on the far side's command line verbatim — +/// and name nothing on either machine. +/// +/// A remote host is left exactly as it arrived, which is what this tree did +/// everywhere before the local rule existed. Path syntax is a property of the +/// machine the path is *on*, not of the one asking. +pub fn spelling_on(host: crate::host::HostId, path: &Path) -> Cow<'_, Path> { + match host.is_local() { + true => local_spelling(path), + false => Cow::Borrowed(path), + } +} + +/// [`spelling_on`], for a caller with nothing to hand back borrowed. +pub fn spelling_on_buf(host: crate::host::HostId, path: impl AsRef) -> std::path::PathBuf { + spelling_on(host, path.as_ref()).into_owned() +} + +/// The plain form of an extended-length path, or `None` when there is not one. +/// +/// Split out so the rule is testable on literal UTF-16, which is the only way +/// to write the surrogate case down and the only way a non-Windows developer +/// ever sees either shape. +#[cfg(windows)] +fn strip_extended_length(wide: &[u16]) -> Option> { + const VERBATIM: [u16; 4] = [BACKSLASH, BACKSLASH, b'?' as u16, BACKSLASH]; + const UNC: [u16; 4] = [b'U' as u16, b'N' as u16, b'C' as u16, BACKSLASH]; + + let rest = wide.strip_prefix(&VERBATIM)?; + // `\\?\UNC\srv\share` → `\\srv\share`. The `\` that follows `UNC` is kept + // and one more put in front of it, which is the pair a plain UNC path + // opens with. Windows spells the segment `UNC` but accepts any case, so + // match it the way the OS would. + let head: Vec = rest + .iter() + .take(4) + .map(|u| u16::from(u8::try_from(*u).unwrap_or(0).to_ascii_uppercase())) + .collect(); + if head == UNC { + let mut plain = vec![BACKSLASH]; + plain.extend_from_slice(&rest[3..]); + return Some(plain); + } + // `\\?\C:\…` → `C:\…`, and `\\?\C:` on its own too. Anything else behind + // the prefix is a device namespace with no plain form — leave it whole. + let (drive, colon) = (*rest.first()?, *rest.get(1)?); + let drive_letter = u8::try_from(drive).is_ok_and(|b| b.is_ascii_alphabetic()); + if drive_letter && colon == b':' as u16 && rest.get(2).is_none_or(|u| *u == BACKSLASH) { + return Some(rest.to_vec()); + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_path_with_nothing_to_fix_is_handed_back_borrowed() { + // No allocation for the overwhelmingly common case: a path already + // spelled the way this machine spells one. That is every path in the + // app once the boundaries below have done their work, so the cost of + // asking again at a lookup is a scan and nothing else. + let native: &[&str] = match cfg!(windows) { + true => &["README.md", r"C:\code\repo", r"\\server\share\proj"], + false => &["README.md", "/code/repo"], + }; + for p in native { + let got = local_spelling(Path::new(p)); + assert_eq!(got.as_ref(), Path::new(p), "{p:?}"); + assert!(matches!(got, Cow::Borrowed(_)), "{p:?} should not allocate"); + assert!(matches!(native_separators(Path::new(p)), Cow::Borrowed(_))); + } + } + + /// The whole point: the three spellings of one directory become one key. + #[test] + fn every_spelling_of_one_directory_lands_on_the_same_key() { + let want = local_spelling_buf(Path::new(if cfg!(windows) { + r"C:\Users\x\repo" + } else { + "/home/x/repo" + })); + let spellings: &[&str] = if cfg!(windows) { + &[ + r"C:\Users\x\repo", + "C:/Users/x/repo", + r"\\?\C:\Users\x\repo", + // Mixed, which is what `root.join(rel)` produces once a + // forward-slash root has had a native component added to it. + r"C:/Users/x\repo", + ] + } else { + &["/home/x/repo"] + }; + for spelling in spellings { + assert_eq!( + local_spelling(Path::new(spelling)).as_ref(), + want.as_path(), + "{spelling:?}" + ); + } + } + + /// The local rule is asked of the machine the path is *on*. + /// + /// A pane, a git probe and the SCM panel all serve a remote workspace + /// with the same code, and the root they settle on goes back over the + /// wire as the cwd of the next `git` — `RemoteHost` sends + /// `to_string_lossy` of it, verbatim. A Windows client folding a Linux + /// box's `/home/u/src` would ask that box about `\home\u\src`. + /// + /// Ungated: on unix both arms are the identity anyway, and Windows is the + /// only client where getting this wrong is visible. + #[test] + fn a_path_on_another_machine_is_left_in_that_machines_spelling() { + use crate::host::HostId; + + let remote = HostId::from_connection_key("ssh-direct:me@box:22"); + for posix in ["/home/u/src", "/home/u/a b/c", "/"] { + let got = spelling_on(remote, Path::new(posix)); + assert_eq!(got.as_ref(), Path::new(posix), "{posix:?}"); + assert!(matches!(got, Cow::Borrowed(_)), "{posix:?}"); + assert_eq!(spelling_on_buf(remote, posix).to_string_lossy(), posix); + } + // A remote *Windows* box is left alone too: its spelling is its own + // business, and this client may not even have a notion of a drive. + let win = r"C:/Users/x/repo"; + assert_eq!(spelling_on_buf(remote, win).to_string_lossy(), win); + // This machine's own paths still go through the rule, which on + // Windows is what makes the two arms different answers at all. + if cfg!(windows) { + assert_eq!( + spelling_on_buf(HostId::LOCAL, win), + std::path::PathBuf::from(r"C:\Users\x\repo") + ); + } + } + + /// What `Path` already does for us, pinned so a later "improvement" here + /// cannot quietly start folding things it must not. These are the cases + /// the #791 gate blamed for the SCM divergence; they were never the cause. + #[test] + fn path_equality_already_forgives_case_slashes_and_a_trailing_separator() { + if !cfg!(windows) { + return; + } + let root = Path::new(r"C:\Users\x\repo"); + for same in [ + "C:/Users/x/repo", + r"c:\Users\x\repo", + r"C:\Users\x\repo\", + "C:/Users/x/repo/", + ] { + assert_eq!(Path::new(same), root, "{same:?}"); + assert_eq!(local_spelling(Path::new(same)).as_ref(), root, "{same:?}"); + } + // …and the one it does not, which is why this module exists. + assert_ne!(Path::new(r"\\?\C:\Users\x\repo"), root); + } + + #[cfg(windows)] + #[test] + fn a_unc_verbatim_path_falls_back_to_its_plain_form() { + assert_eq!( + local_spelling(Path::new(r"\\?\UNC\server\share\proj")).as_ref(), + Path::new(r"\\server\share\proj") + ); + // Lowercase `unc` is the same namespace to Windows. + assert_eq!( + local_spelling(Path::new(r"\\?\unc\server\share")).as_ref(), + Path::new(r"\\server\share") + ); + // A plain UNC path is already plain, and must keep its leading `\\`. + for p in [r"\\server\share\proj", r"\\wsl$\Ubuntu\home"] { + let got = local_spelling(Path::new(p)); + assert_eq!(got.as_ref(), Path::new(p), "{p:?}"); + assert!(matches!(got, Cow::Borrowed(_)), "{p:?} should not allocate"); + } + } + + #[cfg(windows)] + #[test] + fn a_device_namespace_has_no_plain_form_and_is_left_whole() { + // `\\?\pipe\…` and `\\.\…` are not filesystem paths with a drive to + // fall back to; rewriting either would name nothing. + for p in [r"\\?\pipe\tty7", r"\\.\PhysicalDrive0", r"\\?\Volume{0}\x"] { + assert_eq!(local_spelling(Path::new(p)).as_ref(), Path::new(p), "{p:?}"); + } + } + + #[cfg(windows)] + #[test] + fn a_bare_verbatim_drive_keeps_its_root() { + assert_eq!( + local_spelling(Path::new(r"\\?\C:\")).as_ref(), + Path::new(r"C:\") + ); + assert_eq!( + local_spelling(Path::new(r"\\?\C:")).as_ref(), + Path::new("C:") + ); + } + + #[cfg(windows)] + #[test] + fn a_non_ascii_component_survives_both_rewrites() { + assert_eq!( + local_spelling(Path::new(r"\\?\C:\Users\x\中文名\проект")).as_ref(), + Path::new(r"C:\Users\x\中文名\проект") + ); + assert_eq!( + local_spelling(Path::new("C:/Users/x/中文名/проект")).as_ref(), + Path::new(r"C:\Users\x\中文名\проект") + ); + } + + #[cfg(windows)] + #[test] + fn a_name_a_string_cannot_hold_is_kept() { + use std::ffi::OsString; + use std::os::windows::ffi::{OsStrExt, OsStringExt}; + use std::path::PathBuf; + + // `0xD800` is a lone high surrogate — legal in an NTFS name, and not + // representable in a Rust `str`. Going through `to_string_lossy` + // would swap it for `U+FFFD` and hand back a path naming a + // *different* file. Working on the UTF-16 units keeps the name. + let raw: Vec = r"\\?\C:\a" + .encode_utf16() + .chain([0xD800]) + .chain("/b".encode_utf16()) + .collect(); + let path = PathBuf::from(OsString::from_wide(&raw)); + let want: Vec = r"C:\a" + .encode_utf16() + .chain([0xD800]) + .chain(r"\b".encode_utf16()) + .collect(); + assert_eq!( + local_spelling(&path) + .as_os_str() + .encode_wide() + .collect::>(), + want + ); + // The round-trip this avoids really does destroy it. + assert!(path.to_string_lossy().contains('\u{FFFD}')); + } + + #[cfg(not(windows))] + #[test] + fn off_windows_both_are_the_identity() { + // A backslash in a Unix path is an ordinary filename character, and a + // remote host's paths pass through this same code on a Windows client. + for p in ["/home/u/tty7", r"C:\Users\dev", r"mixed/path\here"] { + let got = local_spelling(Path::new(p)); + assert_eq!(got.as_ref(), Path::new(p), "{p:?}"); + assert!(matches!(got, Cow::Borrowed(_))); + } + } +} diff --git a/crates/tty7-core/src/core/worktree.rs b/crates/tty7-core/src/core/worktree.rs index bc0fceed..55ec586c 100644 --- a/crates/tty7-core/src/core/worktree.rs +++ b/crates/tty7-core/src/core/worktree.rs @@ -1,6 +1,7 @@ use std::path::{Path, PathBuf}; use crate::core::codename::Names; +use crate::core::git::git_path; use crate::host::Host; #[derive(Debug)] @@ -63,14 +64,17 @@ pub fn managed(host: &dyn Host, cwd: &Path) -> Option { if !cwd.ancestors().any(|a| a.ends_with(&suffix)) { return None; } - let path = PathBuf::from(git(host, &cwd, &["rev-parse", "--show-toplevel"]).ok()?); + let path = git_path( + host, + &git(host, &cwd, &["rev-parse", "--show-toplevel"]).ok()?, + ); let main_root = git( host, &path, &["rev-parse", "--path-format=absolute", "--git-common-dir"], ) .ok() - .map(PathBuf::from)? + .map(|d| git_path(host, &d))? .parent()? .to_path_buf(); if !path.starts_with(managed_root(host, &main_root)) { @@ -111,14 +115,14 @@ pub fn remove(host: &dyn Host, wt: &ManagedWorktree, force: bool) -> Result<(), fn repo_dir(host: &dyn Host, cwd: &Path) -> Result<(PathBuf, PathBuf), String> { let repo_root = git(host, cwd, &["rev-parse", "--show-toplevel"]) .map_err(|_| "not inside a git repository".to_string())?; - let repo_root = PathBuf::from(repo_root); + let repo_root = git_path(host, &repo_root); let main_root = git( host, cwd, &["rev-parse", "--path-format=absolute", "--git-common-dir"], ) .ok() - .map(PathBuf::from) + .map(|d| git_path(host, &d)) .and_then(|d| d.parent().map(Path::to_path_buf)) .unwrap_or_else(|| repo_root.clone()); let dir = managed_root(host, &main_root); diff --git a/crates/tty7-core/src/host/local.rs b/crates/tty7-core/src/host/local.rs index 6288037e..627c8f9f 100644 --- a/crates/tty7-core/src/host/local.rs +++ b/crates/tty7-core/src/host/local.rs @@ -177,9 +177,22 @@ impl Host for LocalHost { fs::read(p) } + /// The real path behind `p`, in the spelling the rest of tty7 keys by. + /// + /// `fs::canonicalize` answers with the extended-length form on Windows — + /// `\\?\C:\Users\x\repo` — which is a different `Prefix` component from + /// the `C:\Users\x\repo` a shell, a pane cwd and `git` all report, and so + /// compares unequal, hashes differently and fails `starts_with` against + /// every one of them. `\\?\` is a Win32 API escape hatch rather than part + /// of the path's identity, so it comes off here, at the one boundary that + /// produces it. What the call is actually *for* — resolving a junction, a + /// `subst` drive, an 8.3 short name or a symlink — is untouched. See + /// [`crate::core::path_spelling`]. fn canonicalize(&self, p: &Path) -> io::Result { guard_off_ui(); - fs::canonicalize(p) + Ok(crate::core::path_spelling::local_spelling_buf( + fs::canonicalize(p)?, + )) } fn search( diff --git a/src/terminal/git_status.rs b/src/terminal/git_status.rs index 715249b3..a8fa0250 100644 --- a/src/terminal/git_status.rs +++ b/src/terminal/git_status.rs @@ -1,9 +1,30 @@ +use std::borrow::Cow; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; pub use crate::core::git::{GitStatus, RepoSnapshot, probe}; use crate::ui::host_ops::{ByHost, HostId, InFlight}; +/// The spelling a directory is keyed by in here. +/// +/// This cache is where a repository gets its identity: `roots` maps a cwd to a +/// root, and `homes`, `status` and `last_probe` are then all keyed by that +/// root, which is in turn the key `ScmData` and the diff overlay use. The two +/// halves of a lookup arrive from different places — a cwd from the pane, a +/// root from `git`, and either one possibly past `fs::canonicalize` — so this +/// is the one place that has to insist they agree. Off Windows, and for every +/// path that already spells itself the OS's way, it is a borrow and nothing +/// else. See [`tty7_core::core::path_spelling`]. +/// +/// Keyed by the *pane's* host, not by this process. Every method here serves a +/// remote workspace too, whose `/home/u/src` is native over there and is handed +/// straight back to `Host::git` and to `ScmData`'s watcher — folding it +/// with Windows rules on a Windows client would ask a Linux box about +/// `\home\u\src`. A path from another machine is left exactly as it arrived. +fn key(host: HostId, path: &Path) -> Cow<'_, Path> { + tty7_core::core::path_spelling::spelling_on(host, path) +} + #[derive(Default)] pub struct GitStatusCache { roots: ByHost>, @@ -17,12 +38,12 @@ impl gpui::Global for GitStatusCache {} impl GitStatusCache { pub fn status_for(&self, host: HostId, cwd: &Path) -> Option { - let root = self.roots.get(host, cwd)?.as_ref()?; - self.status.get(host, root).cloned() + let root = self.roots.get(host, &*key(host, cwd))?.as_ref()?; + self.status.get(host, root.as_path()).cloned() } pub fn known_repo_for(&self, host: HostId, cwd: &Path) -> Option> { - let root = self.roots.get(host, cwd)?; + let root = self.roots.get(host, &*key(host, cwd))?; Some(root.as_ref().map(|root| { self.homes .get(host, root) @@ -38,7 +59,7 @@ impl GitStatusCache { /// a "which project is this" question wants. This answers with the root, /// which is the key everything git-shaped is stored under. pub fn repo_root_for(&self, host: HostId, cwd: &Path) -> Option<&Path> { - self.roots.get(host, cwd)?.as_deref() + self.roots.get(host, &*key(host, cwd))?.as_deref() } /// Forget a machine we have stopped talking to, so a reconnect starts from @@ -51,7 +72,7 @@ impl GitStatusCache { } pub fn begin_probe(&mut self, host: HostId, cwd: &Path) -> bool { - let key = (host, cwd.to_path_buf()); + let key = (host, key(host, cwd).into_owned()); if self.probes.begin(key.clone()) { true } else { @@ -66,22 +87,25 @@ impl GitStatusCache { cwd: &Path, min_interval: Duration, ) -> bool { + let cwd = key(host, cwd); if self.probes.is_pending(&(host, cwd.to_path_buf())) { return false; } - let key = self.throttle_key(host, cwd).to_path_buf(); + let throttle = self.throttle_key(host, &cwd).to_path_buf(); if self .last_probe - .get(host, key.as_path()) + .get(host, throttle.as_path()) .is_some_and(|at| at.elapsed() < min_interval) { return false; } - self.last_probe.insert(host, key, Instant::now()); - self.probes.begin((host, cwd.to_path_buf())); + self.last_probe.insert(host, throttle, Instant::now()); + self.probes.begin((host, cwd.into_owned())); true } + /// `cwd` is already in the cache's own spelling — every caller of this one + /// has been past [`key`]. fn throttle_key<'a>(&'a self, host: HostId, cwd: &'a Path) -> &'a Path { match self.roots.get(host, cwd) { Some(Some(root)) => root, @@ -122,7 +146,8 @@ impl GitStatusCache { branch: &str, counts: Option<(u32, u32)>, ) -> bool { - let Some(status) = self.status.get(host, root) else { + let root = key(host, root); + let Some(status) = self.status.get(host, &*root) else { return false; }; let (added, removed) = counts.unwrap_or((status.added, status.removed)); @@ -131,7 +156,7 @@ impl GitStatusCache { } self.status.insert( host, - root.to_path_buf(), + root.into_owned(), GitStatus { branch: branch.to_string(), added, @@ -147,12 +172,21 @@ impl GitStatusCache { cwd: &Path, snapshot: Option, ) -> bool { + // A snapshot arrives spelled by `git`, the cwd by whoever asked for + // the probe. Both land in the cache's own spelling or the root a + // status is filed under is not the root the next lookup asks for. + let cwd = key(host, cwd); + let snapshot = snapshot.map(|snap| RepoSnapshot { + root: key(host, &snap.root).into_owned(), + home: key(host, &snap.home).into_owned(), + ..snap + }); let rerun = !self.probes.finish(&(host, cwd.to_path_buf())); - let key = match &snapshot { + let throttle = match &snapshot { Some(snap) => snap.root.clone(), - None => self.throttle_key(host, cwd).to_path_buf(), + None => self.throttle_key(host, &cwd).to_path_buf(), }; - self.last_probe.insert(host, key, Instant::now()); + self.last_probe.insert(host, throttle, Instant::now()); match snapshot { Some(snap) => { let (added, removed) = snap.counts.unwrap_or_else(|| { @@ -465,6 +499,184 @@ mod tests { assert!(cache.begin_probe(L, cwd)); } + /// Every way one directory can be spelled on the way into this cache is + /// one key. + /// + /// Ungated on purpose. The spellings below are the ones Windows actually + /// produces — a pane says `C:\repo`, `git rev-parse` says `C:/repo`, + /// `fs::canonicalize` says `\\?\C:\repo` — and on Unix they collapse to + /// one, so this costs nothing there and is the whole test here. Gating it + /// to unix is what let the divergence live: the *only* platform that has + /// three spellings was the only one not running the comparison. + #[test] + fn one_directory_spelled_three_ways_is_one_repository() { + let mut cache = GitStatusCache::default(); + let (a, b, c) = match cfg!(windows) { + true => (r"C:\code\repo", "C:/code/repo", r"\\?\C:\code\repo"), + false => ("/code/repo", "/code/repo", "/code/repo"), + }; + let (a, b, c) = (Path::new(a), Path::new(b), Path::new(c)); + + // Probed under the resolved spelling, which is what a caller that went + // through `Host::canonicalize` has. + cache.finish_probe(L, c, Some(snap(c.to_str().unwrap(), "main", Some((9, 9))))); + + for spelling in [a, b, c] { + assert_eq!( + cache.repo_root_for(L, spelling), + Some(a), + "{spelling:?} names the repository the others do" + ); + assert_eq!( + cache.known_repo_for(L, spelling), + Some(Some(a.to_path_buf())), + "{spelling:?}" + ); + assert_eq!( + cache.status_for(L, spelling).unwrap().branch, + "main", + "{spelling:?}" + ); + } + } + + /// A repository on another machine keeps that machine's spelling. + /// + /// The rule above is a *local* one, and this cache serves a remote + /// workspace with the same four methods. The root it hands back is what + /// `Host::git` puts on the far side's command line — `wire_path` is + /// `to_string_lossy`, verbatim — and what `ScmData` opens the `.git` + /// watch on. Folding `/home/u/src` with this client's rules would ask a + /// Linux box about `\home\u\src`, which names nothing there. + /// + /// Ungated, like the one above and for the same reason: the assertion is + /// only ever interesting on Windows, so gating it away from Windows is + /// how it would stop holding. + #[test] + fn a_repository_on_another_machine_keeps_that_machines_spelling() { + let mut cache = GitStatusCache::default(); + let remote = HostId::from_connection_key("ssh-direct:me@box:22"); + let (cwd, root) = (Path::new("/home/u/src/crates/app"), "/home/u/src"); + + cache.finish_probe(remote, cwd, Some(snap(root, "main", Some((2, 1))))); + + assert_eq!( + cache.repo_root_for(remote, cwd).map(Path::to_string_lossy), + Some(root.into()), + "the far side is handed this string back unchanged" + ); + assert_eq!( + cache.known_repo_for(remote, cwd), + Some(Some(PathBuf::from(root))) + ); + assert_eq!(cache.status_for(remote, cwd).unwrap().branch, "main"); + // And a diff read filed under git's own answer still reaches it. + assert!(cache.note_diff_read(remote, Path::new(root), "moved-on", Some((0, 0)))); + assert_eq!(cache.status_for(remote, cwd).unwrap().branch, "moved-on"); + } + + /// The diff overlay's spin, in the cache underneath it. + /// + /// `install_diff_snapshot` hands the branch it just read back with the + /// root `git rev-parse` printed, while the status it is correcting was + /// filed under the root whoever probed had. When those two spellings miss + /// each other the correction is dropped, the overlay's next + /// `maybe_refresh` finds the same disagreement it just tried to settle, + /// and it re-reads the diff — `load=ready loading=true`, two `git` + /// processes a lap, for as long as the overlay is open. + #[test] + fn a_diff_read_settles_a_branch_it_learned_the_root_of_from_git() { + let mut cache = GitStatusCache::default(); + let (probed, from_git) = match cfg!(windows) { + true => (r"\\?\C:\code\repo", "C:/code/repo"), + false => ("/code/repo", "/code/repo"), + }; + cache.finish_probe( + L, + Path::new(probed), + Some(snap(probed, "a-branch-this-repo-has-left", Some((99, 99)))), + ); + + assert!( + cache.note_diff_read(L, Path::new(from_git), "main", Some((1, 0))), + "the correction has to land, or the overlay reprobes forever" + ); + let got = cache.status_for(L, Path::new(probed)).unwrap(); + assert_eq!(got.branch, "main"); + assert_eq!((got.added, got.removed), (1, 0)); + assert!( + !cache.note_diff_read(L, Path::new(from_git), "main", Some((1, 0))), + "and the second lap has nothing left to say — this is what ends it" + ); + } + + /// The same loop, end to end against a repository `git` actually created, + /// because the literals above only prove the rule and not that this is the + /// rule the real answers need. + /// + /// This is the shape the diff overlay runs every frame: a status filed + /// under the cwd a probe was asked about, then a diff read filed under the + /// root `git rev-parse` printed. No window and no gpui, so it runs + /// everywhere the test binary does. + #[test] + fn a_real_repository_files_its_probe_and_its_diff_under_one_root() { + use tty7_core::core::git::diff::{DiffRequest, probe_diff}; + + let host = tty7_core::host::local::LocalHost::new(); + let dir = std::env::temp_dir().join(format!("tty7-one-root-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let ok = host + .git(&dir, &["init", "--quiet"]) + .is_ok_and(|o| o.success()); + if !ok { + let _ = std::fs::remove_dir_all(&dir); + return; // no git on this machine + } + for cfg in [ + ["config", "user.email", "t@x"].as_slice(), + ["config", "user.name", "t"].as_slice(), + ] { + assert!(host.git(&dir, cfg).is_ok_and(|o| o.success())); + } + std::fs::write(dir.join("a.rs"), "fn main() {}\n").unwrap(); + assert!(host.git(&dir, &["add", "-A"]).is_ok_and(|o| o.success())); + assert!( + host.git(&dir, &["commit", "--quiet", "-m", "one"]) + .is_ok_and(|o| o.success()) + ); + std::fs::write(dir.join("a.rs"), "fn main() { /* edited */ }\n").unwrap(); + + // The cwd a pane reports can have been past `Host::canonicalize`; the + // root a diff carries never has been. + let cwd = host.canonicalize(&dir).expect("the scratch dir resolves"); + let mut cache = GitStatusCache::default(); + let snapshot = crate::core::git::probe(&*host, &cwd).expect("a repository is here"); + cache.finish_probe(L, &cwd, Some(snapshot)); + assert_eq!( + cache.repo_root_for(L, &cwd), + Some(cwd.as_path()), + "the probed root is the directory the cache was asked about" + ); + + let diff = probe_diff(&*host, &cwd, &DiffRequest::default()).expect("a diff is readable"); + assert_eq!(diff.root, cwd, "and the diff names that same directory"); + // A branch switched outside tty7 is what makes this correction the + // thing that ends the overlay's loop rather than a no-op: the read has + // to land the first time and have nothing to say the second. + assert!( + cache.note_diff_read(L, &diff.root, "moved-on", Some((0, 0))), + "a diff read filed under git's root must reach the probe's status" + ); + assert_eq!(cache.status_for(L, &cwd).unwrap().branch, "moved-on"); + assert!( + !cache.note_diff_read(L, &diff.root, "moved-on", Some((0, 0))), + "and the second lap says nothing — this is what ends the loop" + ); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn throttle_collapses_subdirectories_of_one_repo() { let mut cache = GitStatusCache::default(); diff --git a/src/ui/path_display.rs b/src/ui/path_display.rs index 51a319c8..5a76cf6b 100644 --- a/src/ui/path_display.rs +++ b/src/ui/path_display.rs @@ -69,56 +69,27 @@ fn normalized(s: &str) -> String { .to_ascii_lowercase() } -/// Re-spells a path on **this** machine with the separators this OS expects. +/// Re-spells a path on **this** machine with the separators this OS expects, +/// so the Win32 shell will take it. /// -/// On Windows the shell's `IShellFolder::ParseDisplayName` bails out with -/// `E_INVALIDARG` on a mixed-separator path — a forward-slash prefix joined -/// with backslash entries. The forward slashes get in from two routes: the -/// shell's PWD (OSC 7 from Git Bash / MSYS bash reports `/`, and that string -/// survives `Path::ancestors()` when the file tree walks up to find `.git`), -/// and `git rev-parse --show-toplevel` from Git for Windows (MSYS2), which -/// always prints `/` regardless of the calling shell. `reveal_path` swallows -/// that failure (it only logs), so handing it native separators is what makes -/// "open folder" actually open. +/// `IShellFolder::ParseDisplayName` bails out with `E_INVALIDARG` on a +/// mixed-separator path — a forward-slash prefix joined with backslash +/// entries — and `reveal_path` swallows that failure (it only logs), so +/// handing it native separators is what makes "open folder" actually open. +/// +/// The rule itself lives in [`tty7_core::core::path_spelling`], next to the +/// prefix rule the SCM caches need, because a path spelled two ways is one +/// problem and it must not have two answers in two crates. This is the +/// separators half on its own: a `\\?\` path is already something +/// `ParseDisplayName` will not take, and re-spelling one here would be a +/// silent change of subject rather than a fix. /// /// **Only for paths on the machine this window runs on.** A remote host's /// `/home/u/src` is already native over there; re-spelling it would put a /// path on the clipboard that names nothing on either machine. Every caller /// sits behind a locality check for that reason. -/// -/// The rewrite runs on the path's own UTF-16 code units, not on a -/// `to_string_lossy` copy of them. A Windows filename may hold unpaired -/// surrogates, which `to_string_lossy` turns into `U+FFFD` — the returned -/// path would then silently name a *different* file, and reveal would open -/// nothing without reporting why. `/` and `\` are ASCII, so a code unit -/// equal to one of them is that character and never half of a surrogate -/// pair, which is what makes the swap safe to do one unit at a time. -#[cfg(windows)] pub(crate) fn native_separators(path: &Path) -> Cow<'_, Path> { - use std::ffi::OsString; - use std::os::windows::ffi::{OsStrExt, OsStringExt}; - - const SLASH: u16 = b'/' as u16; - const BACKSLASH: u16 = b'\\' as u16; - - let os = path.as_os_str(); - // Nothing to fix — including every UNC (`\\wsl$\…`, `\\?\…`) and - // already-native path — hands the caller's own path straight back. - if !os.encode_wide().any(|unit| unit == SLASH) { - return Cow::Borrowed(path); - } - let wide: Vec = os - .encode_wide() - .map(|unit| if unit == SLASH { BACKSLASH } else { unit }) - .collect(); - Cow::Owned(PathBuf::from(OsString::from_wide(&wide))) -} - -/// Off Windows the OS separator is already `/`, and a backslash in a path is -/// an ordinary filename character — there is nothing to re-spell. -#[cfg(not(windows))] -pub(crate) fn native_separators(path: &Path) -> Cow<'_, Path> { - Cow::Borrowed(path) + tty7_core::core::path_spelling::native_separators(path) } /// Shortens `path` to start from `~` when it is (inside) `home` — the home diff --git a/src/ui/scm/panel.rs b/src/ui/scm/panel.rs index bf3dc850..fc591cdf 100644 --- a/src/ui/scm/panel.rs +++ b/src/ui/scm/panel.rs @@ -1255,12 +1255,18 @@ impl Tty7App { }, move |this, out, cx| { this.scm.root_lookups.remove(&key); + // In the spelling everything else keys by: Git for Windows + // answers `C:/Users/…` and this root is what the panel, the + // commit detail and `ScmData` all compare against a path the + // OS spelled. Only for a repository on this machine — a remote + // root is native over there and every write below runs `git` + // from it on that box. See `tty7_core::core::path_spelling`. let root = out .as_deref() .and_then(|s| s.lines().next()) .map(str::trim) .filter(|l| !l.is_empty()) - .map(PathBuf::from); + .map(|l| tty7_core::core::path_spelling::spelling_on_buf(id, l)); this.scm.roots.insert(key, (Instant::now(), root)); cx.notify(); }, From eca98a13181e329d3f25c772962684655aea21ac Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:58:36 +0800 Subject: [PATCH 15/15] fix(terminal): submit a plain single line as typed, not as a paste (#790) Refs #660. --- src/terminal/cmd_editor.rs | 79 +++++++++++++ src/terminal/hold.rs | 63 +++++++++++ src/terminal/view.rs | 219 ++++++++++++++++++++++++++++++++----- 3 files changed, 332 insertions(+), 29 deletions(-) diff --git a/src/terminal/cmd_editor.rs b/src/terminal/cmd_editor.rs index e6b806b4..352472ad 100644 --- a/src/terminal/cmd_editor.rs +++ b/src/terminal/cmd_editor.rs @@ -6,6 +6,7 @@ pub struct CmdEditor { undo: Vec<(Vec, usize)>, redo: Vec<(Vec, usize)>, kill: String, + pasted: bool, } const UNDO_LIMIT: usize = 200; @@ -80,6 +81,41 @@ impl CmdEditor { } } + /// Insert clipboard (or dropped-file) text, and remember that this line has + /// carried some. + /// + /// The mark is what lets the submit path keep bracketed paste's contract — + /// what was pasted is what runs, with no shell-side rewriting — for a line + /// that came from outside, while a line the user typed goes to the shell as + /// typed. See `submit_bytes` in the terminal view. + pub fn insert_pasted(&mut self, s: &str) { + self.pasted = true; + self.insert_str(s); + } + + /// Whether clipboard text has been inserted into the line being edited. + /// + /// Deliberately sticky for the life of the buffer rather than tracked per + /// character: `clear` runs on every submit and every handoff, so the mark + /// is already scoped to exactly one line, and every edit that survives + /// inside that line — a completion accepted over the pasted text, a ghost + /// suggestion, a kill and yank — keeps it. Erring toward "pasted" only ever + /// costs the shell-side expansion this line might have had; erring the + /// other way would hand the shell's binding table something the user pasted. + pub fn pasted(&self) -> bool { + self.pasted + } + + /// Prepend text the gap hold collected, and remember that some of it came + /// off the clipboard. The counterpart to [`insert_pasted`](Self::insert_pasted) + /// for the one route into this buffer that does not go through the editor: + /// a paste made before the prompt arrived is held outside it and prepended + /// when the editor takes over. + pub fn prepend_pasted(&mut self, s: &str) { + self.pasted = true; + self.prepend_str(s); + } + pub fn prepend_str(&mut self, s: &str) { if s.is_empty() { return; @@ -391,6 +427,7 @@ impl CmdEditor { self.anchor = None; self.undo.clear(); self.redo.clear(); + self.pasted = false; } pub fn set(&mut self, text: &str) { @@ -833,4 +870,46 @@ mod tests { e.set_with_cursor("hi", 99); assert_eq!((e.text().as_str(), e.cursor()), ("hi", 2)); } + + #[test] + fn the_paste_mark_lasts_exactly_one_line() { + let mut e = ed("git ", 4); + assert!(!e.pasted(), "a typed line carries no mark"); + e.insert_str("status"); + assert!(!e.pasted()); + + e.insert_pasted(" --short"); + assert!(e.pasted()); + + // Every edit that leaves the pasted text inside this line keeps the + // mark, including the ones that rewrite the buffer wholesale on top of + // it -- a completion accepted over the paste, a ghost suggestion. + e.backspace(); + e.delete_word_left(); + e.set("git status --shor"); + e.set_with_cursor("git status --short", 18); + assert!( + e.pasted(), + "losing the mark mid-line would hand the paste to the shell's bindings" + ); + + // `clear` runs on every submit and every handoff, so the next line + // starts clean and gets the typed delivery again. + e.clear(); + assert!(!e.pasted()); + e.insert_str("j build"); + assert!(!e.pasted()); + + // Text pasted before the prompt arrived is held outside this buffer + // and prepended when the editor takes over; it has to bring the mark + // with it, or the gap would be a way around `insert_pasted`. + e.clear(); + e.insert_str(" /tmp"); + e.prepend_pasted("l"); + assert_eq!(e.text(), "l /tmp"); + assert!(e.pasted()); + e.clear(); + e.prepend_str("ls"); + assert!(!e.pasted(), "typed gap text stays typed"); + } } diff --git a/src/terminal/hold.rs b/src/terminal/hold.rs index 39203e1a..29fe7a2e 100644 --- a/src/terminal/hold.rs +++ b/src/terminal/hold.rs @@ -17,6 +17,7 @@ pub struct GapHold { net: String, bytes: Vec, epoch: u64, + pasted: bool, } impl GapHold { @@ -28,6 +29,28 @@ impl GapHold { self.hold(bytes, |net| net.push_str(s)) } + /// [`hold_text`](Self::hold_text) for text that came off the clipboard + /// rather than the keyboard. + /// + /// The provenance has to ride along with the held text: a paste made while + /// the prompt was still on its way lands here, not in the editor, and the + /// editor takes the whole net over when the gap ends. Without the mark + /// that text would arrive looking typed and be submitted as typed (#660), + /// which is exactly what `CmdEditor::insert_pasted` exists to prevent. + pub fn hold_pasted_text(&mut self, s: &str, bytes: &[u8]) -> Verdict { + let verdict = self.hold_text(s, bytes); + if matches!(verdict, Verdict::Held(_)) { + self.pasted = true; + } + verdict + } + + /// Whether any of the text held for the editor came off the clipboard. + /// Read it before [`engage`](Self::engage), which clears it with the net. + pub fn pasted(&self) -> bool { + self.pasted + } + pub fn hold_backspace(&mut self, bytes: &[u8]) -> Verdict { self.hold(bytes, |net| { net.pop(); @@ -53,6 +76,7 @@ impl GapHold { pub fn release(&mut self) -> Option<(String, Vec)> { let held = matches!(self.state, State::Holding); self.state = State::Passthrough; + self.pasted = false; held.then(|| { ( std::mem::take(&mut self.net), @@ -72,6 +96,7 @@ impl GapHold { pub fn engage(&mut self) -> Option { self.state = State::Idle; self.bytes.clear(); + self.pasted = false; let net = std::mem::take(&mut self.net); (!net.is_empty()).then_some(net) } @@ -90,6 +115,44 @@ mod tests { assert_eq!(h.engage(), None); } + #[test] + fn a_paste_held_in_the_gap_reaches_the_editor_marked() { + // Pasting while the previous command is still finishing puts the + // clipboard text here rather than in the editor. It must not arrive + // looking typed: the submit path would then hand it to the shell's + // binding table (#660). + let mut h = GapHold::new(); + assert!(!h.pasted(), "a fresh hold carries nothing pasted"); + assert!(matches!( + h.hold_text("cat ", b"cat "), + Verdict::Held(Some(_)) + )); + assert!(!h.pasted()); + assert!(matches!( + h.hold_pasted_text("/tmp/x", b"/tmp/x"), + Verdict::Held(None) + )); + assert!(h.pasted(), "the whole net is pasted once any of it is"); + assert_eq!(h.engage(), Some("cat /tmp/x".to_string())); + assert!(!h.pasted(), "engage hands the mark over with the net"); + + // A dump to the PTY takes the mark with it too: what the editor never + // receives cannot be submitted from it. + let mut h = GapHold::new(); + h.hold_pasted_text("ls", b"ls"); + assert!(h.pasted()); + assert_eq!(h.release(), Some(("ls".to_string(), b"ls".to_vec()))); + assert!(!h.pasted()); + + // Past the window the hold is a passthrough, so there is nothing to + // mark -- the bytes went straight to the shell. + assert!(matches!( + h.hold_pasted_text("x", b"x"), + Verdict::Passthrough + )); + assert!(!h.pasted()); + } + #[test] fn timeout_dumps_typed_bytes_once_and_goes_passthrough() { let mut h = GapHold::new(); diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 1fc29a53..f1755005 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -721,14 +721,76 @@ fn paste_bytes(text: &str, bracketed: bool) -> Vec { } } -fn submit_bytes(line: &str, bracketed: bool) -> Vec { +/// A line the shell can be handed byte for byte, as if it had been typed at its +/// own prompt. +/// +/// Control characters are what rule a line out. Under bracketed paste the shell +/// inserts every byte literally; delivered raw, each one runs through the line +/// editor's binding table instead, and a Tab completes, a `^U` kills, a `^C` +/// abandons the line. ESC is already stripped upstream; `is_control` covers the +/// rest, embedded newlines included — a multi-line command still goes as one +/// paste, which is what [`submit_bytes`] was built for. +/// +/// Printable keys are deliberately *not* excluded, and cannot be: a line editor +/// binding one is exactly the mechanism this exists to reach. fish binds space +/// to `expand-abbr`; zsh users bind `.` to `rationalise-dot` and quotes to +/// zsh-autopair. Reaching the first means reaching the others, which is why the +/// caller keeps pasted text away from this path entirely. +/// +/// The length bound is a cost ceiling, not a correctness one, and it is a +/// policy dial rather than a discontinuity in the data. A paste lands in one +/// go; raw bytes make the shell's line editor redraw as it consumes them, so +/// the added latency is linear in length from the very first byte — measured on +/// a pty it rises perfectly smoothly, with no knee to hang a bound on. +/// +/// What 512 buys is a ceiling on that latency. The worst configuration measured +/// is zsh with zsh-syntax-highlighting and zsh-autosuggestions, which both +/// re-run per keystroke: ~0.26 ms per byte, so +15 ms on a typical 60-byte +/// command and +126 ms at the bound. Bare zsh is ~0.012 ms per byte (+6 ms at +/// the bound), bash is free at every length, and fish — the shell #660 is about +/// — shows no penalty this harness can resolve. Halving the bound would halve +/// the worst case; the number is a judgement about how much latency a long +/// typed line may pay, not something the curve picks out. +fn types_cleanly(line: &str) -> bool { + line.len() <= 512 && !line.chars().any(char::is_control) +} + +/// Build the byte sequence that submits the local editor's buffer to the shell. +/// +/// A plain single-line command the user *typed* goes raw, no paste markers: the +/// shell's own reader then sees the same bytes typing at its prompt would +/// produce, so its input-time expansions run — fish abbreviations (#660), zsh +/// `magic-space`, a readline macro on a printable key. Inside a bracketed paste +/// none of that fires; fish's expand-on-execute only reaches the token under +/// the cursor, so `j` expanded but `j build` ran literally. +/// +/// `pasted` is what keeps that from rewriting text the user did not type. A +/// paste's contract is that what went in is what runs, and a fish user with +/// `abbr -a l 'ls -la'` pasting `l /tmp` from their notes must not get +/// `ls -la /tmp`. So a line that has carried clipboard content keeps the paste +/// framing whatever else is true of it — see [`CmdEditor::pasted`]. +/// +/// Multi-line and control characters keep it too. A multi-line command goes in +/// as one paste and one CR, so it costs one prompt cycle instead of one per +/// line — preexec, the user's precmd chain, a syntax-highlight pass over the +/// whole buffer — and zle keeps the embedded newlines in its buffer, so +/// backslash / open-quote continuation and heredocs still parse as one unit. +/// +/// ESC is stripped either way (unlike the paste path): clipboard text carrying +/// its own `ESC[201~` could otherwise close the paste early and have the rest +/// run as typed input, and a raw ESC reaching zle is an editor command. +/// +/// An empty buffer skips the markers: zsh's `bracketed-paste-magic` (which +/// oh-my-zsh turns on) errors on a paste with nothing between them. +fn submit_bytes(line: &str, bracketed: bool, pasted: bool) -> Vec { let clean: String = line .replace("\r\n", "\n") .chars() .filter(|&c| c != '\x1b') .map(|c| if c == '\r' { '\n' } else { c }) .collect(); - let mut bytes = paste_bytes(&clean, bracketed && !clean.is_empty()); + let framed = bracketed && !clean.is_empty() && (pasted || !types_cleanly(&clean)); + let mut bytes = paste_bytes(&clean, framed); bytes.push(b'\r'); bytes } @@ -2783,7 +2845,7 @@ impl TerminalView { self.jump_to_prompt(); if self.input_active() { let trimmed = text.strip_suffix('\n').unwrap_or(&text); - self.cmd.insert_str(trimmed); + self.cmd.insert_pasted(trimmed); self.history_nav = None; self.editor_goal_col = None; self.close_completion(); @@ -2797,7 +2859,7 @@ impl TerminalView { .lock() .mode() .contains(TermMode::BRACKETED_PASTE); - self.write_gap_text(&text, paste_bytes(&text, bracketed), cx); + self.write_gap_text(&text, paste_bytes(&text, bracketed), true, cx); cx.notify(); } @@ -3979,14 +4041,21 @@ impl TerminalView { self.terminal.shell_active() && !self.on_alt_screen() && !self.shell_owns_prompt() } - fn write_gap_text(&mut self, text: &str, bytes: Vec, cx: &mut Context) { + /// `pasted` says the text came off the clipboard rather than the keyboard, + /// so that a paste the hold keeps for the editor still reaches it marked. + fn write_gap_text(&mut self, text: &str, bytes: Vec, pasted: bool, cx: &mut Context) { if self.shell_owns_prompt() { self.release_hold(); self.terminal.write(bytes); return; } if self.gap_holdable() && !text.chars().any(char::is_control) { - match self.hold.hold_text(text, &bytes) { + let held = if pasted { + self.hold.hold_pasted_text(text, &bytes) + } else { + self.hold.hold_text(text, &bytes) + }; + match held { Verdict::Held(arm) => { if let Some(epoch) = arm { self.arm_hold_timer(epoch, cx); @@ -4002,6 +4071,26 @@ impl TerminalView { self.observe_typeahead(RawInput::Text(text)); } + /// Move whatever the gap hold collected into the editor's buffer, keeping + /// the paste mark with it. + /// + /// The hold is the one route into that buffer that does not run through + /// the editor: text arriving before the prompt does is kept out here and + /// prepended when the editor takes over. A paste that lost its provenance + /// on the way would be submitted as typed (#660) — see + /// [`CmdEditor::prepend_pasted`]. + fn engage_hold_into_editor(&mut self) { + let pasted = self.hold.pasted(); + let Some(net) = self.hold.engage() else { + return; + }; + if pasted { + self.cmd.prepend_pasted(&net); + } else { + self.cmd.prepend_str(&net); + } + } + fn release_hold(&mut self) { if let Some((net, bytes)) = self.hold.release() { self.terminal.write(bytes); @@ -4074,9 +4163,7 @@ impl TerminalView { if self.terminal.exited || !self.accepts_input(cx) { return; } - if let Some(net) = self.hold.engage() { - self.cmd.prepend_str(&net); - } + self.engage_hold_into_editor(); let line = self.cmd.text(); if !line.trim().is_empty() { let cwd = self.cwd(); @@ -4119,7 +4206,8 @@ impl TerminalView { .lock() .mode() .contains(TermMode::BRACKETED_PASTE); - self.terminal.write(submit_bytes(&line, bracketed)); + let pasted = self.cmd.pasted(); + self.terminal.write(submit_bytes(&line, bracketed, pasted)); self.cmd.clear(); self.cursor_visible = true; self.jump_to_prompt(); @@ -4346,9 +4434,7 @@ impl TerminalView { if !self.accepts_input(cx) { return; } - if let Some(net) = self.hold.engage() { - self.cmd.prepend_str(&net); - } + self.engage_hold_into_editor(); let line = self.cmd.text(); if line.contains('\n') { cx.notify(); @@ -4839,7 +4925,7 @@ impl TerminalView { cx.notify(); return; } - self.write_gap_text(text, text.as_bytes().to_vec(), cx); + self.write_gap_text(text, text.as_bytes().to_vec(), false, cx); self.cursor_visible = true; cx.notify(); } @@ -6362,9 +6448,7 @@ impl Render for TerminalView { } self.typeahead.drain(); } else if self.input_active() { - if let Some(net) = self.hold.engage() { - self.cmd.prepend_str(&net); - } + self.engage_hold_into_editor(); if self.terminal.zle_reading() { self.flush_typeahead(); } @@ -8385,45 +8469,122 @@ mod tests { #[test] fn submit_bytes_sends_a_multi_line_command_as_one_bracketed_paste() { assert_eq!( - submit_bytes("echo a\necho b\necho c", true), + submit_bytes("echo a\necho b\necho c", true, false), b"\x1b[200~echo a\necho b\necho c\x1b[201~\r".to_vec() ); - let out = submit_bytes("a\nb\nc\nd", true); + let out = submit_bytes("a\nb\nc\nd", true, false); assert_eq!(out.iter().filter(|&&b| b == b'\r').count(), 1); + } + + #[test] + fn submit_bytes_types_a_plain_single_line_instead_of_pasting_it() { + // #660: inside a bracketed paste fish never runs `expand-abbr`, so an + // abbreviation with arguments reached the shell verbatim and `j build` + // died as "command not found". Raw bytes are what typing produces, and + // that is the delivery every input-time expansion -- fish + // abbreviations, zsh magic-space -- is bound to. + assert_eq!(submit_bytes("j build", true, false), b"j build\r".to_vec()); assert_eq!( - submit_bytes("ls -la", true), - b"\x1b[200~ls -la\x1b[201~\r".to_vec() + submit_bytes("echo 'a b' | cat", true, false), + b"echo 'a b' | cat\r".to_vec() + ); + // Non-ASCII is text, not a control character. + assert_eq!( + submit_bytes("echo 中文", true, false), + "echo 中文\r".as_bytes() + ); + + // A control character would be acted on by the shell's binding table + // rather than inserted -- a Tab completes, a ^U kills the line -- so + // those keep the paste framing. + assert_eq!( + submit_bytes("echo a\tb", true, false), + b"\x1b[200~echo a\tb\x1b[201~\r".to_vec() + ); + assert_eq!( + submit_bytes("echo a\x15b", true, false), + b"\x1b[200~echo a\x15b\x1b[201~\r".to_vec() + ); + + // Past the length bound the flat cost of a paste wins over the shell's + // per-byte redraw. The bound is a latency ceiling, not a knee in the + // curve -- see `types_cleanly`. + let at_bound = format!("echo {}", "y".repeat(507)); + assert_eq!(at_bound.len(), 512); + assert_eq!( + submit_bytes(&at_bound, true, false), + [at_bound.as_bytes(), b"\r"].concat() + ); + let over_bound = format!("echo {}", "y".repeat(508)); + assert_eq!(over_bound.len(), 513); + assert_eq!( + submit_bytes(&over_bound, true, false), + [b"\x1b[200~", over_bound.as_bytes(), b"\x1b[201~\r"].concat() ); } + #[test] + fn submit_bytes_keeps_pasted_text_inside_a_bracketed_paste() { + // A paste's contract is that what went in is what runs. The typed path + // hands the line to the shell's binding table, and a printable key can + // be bound there: a fish user with `abbr -a l 'ls -la'` who pastes + // `l /tmp` out of their notes must not run `ls -la /tmp`, and a zsh + // user with `bindkey . rationalise-dot` must not have a pasted + // `echo a...b` become `echo a../..b`. Both were reproduced on a pty. + assert_eq!( + submit_bytes("l /tmp", true, true), + b"\x1b[200~l /tmp\x1b[201~\r".to_vec() + ); + assert_eq!( + submit_bytes("echo a...b", true, true), + b"\x1b[200~echo a...b\x1b[201~\r".to_vec() + ); + // Same text typed rather than pasted takes the typed path -- that is + // the whole point, and it is the same delivery the user's own shell + // prompt would have given it. + assert_eq!(submit_bytes("l /tmp", true, false), b"l /tmp\r".to_vec()); + // A shell with no bracketed paste has no framing to fall back on, so + // the mark changes nothing there. + assert_eq!(submit_bytes("l /tmp", false, true), b"l /tmp\r".to_vec()); + // An empty buffer still skips the markers, pasted or not. + assert_eq!(submit_bytes("", true, true), b"\r".to_vec()); + } + #[test] fn submit_bytes_falls_back_to_per_line_cr_without_bracketed_paste() { - assert_eq!(submit_bytes("a\nb", false), b"a\rb\r".to_vec()); - assert_eq!(submit_bytes("a\r\nb", false), b"a\rb\r".to_vec()); + assert_eq!(submit_bytes("a\nb", false, false), b"a\rb\r".to_vec()); + assert_eq!(submit_bytes("a\r\nb", false, false), b"a\rb\r".to_vec()); } #[test] fn submit_bytes_normalizes_line_breaks_inside_the_paste() { assert_eq!( - submit_bytes("a\r\nb", true), + submit_bytes("a\r\nb", true, false), b"\x1b[200~a\nb\x1b[201~\r".to_vec() ); assert_eq!( - submit_bytes("a\rb", true), + submit_bytes("a\rb", true, false), b"\x1b[200~a\nb\x1b[201~\r".to_vec() ); - assert_eq!(submit_bytes("a\rb", false), b"a\rb\r".to_vec()); + assert_eq!(submit_bytes("a\rb", false, false), b"a\rb\r".to_vec()); } #[test] fn submit_bytes_strips_esc_and_skips_markers_on_an_empty_line() { - let out = submit_bytes("foo\x1b[201~\nrm -rf ~", true); + let out = submit_bytes("foo\x1b[201~\nrm -rf ~", true, false); let end = b"\x1b[201~"; assert_eq!(out.windows(end.len()).filter(|w| *w == end).count(), 1); assert_eq!(out, b"\x1b[200~foo[201~\nrm -rf ~\x1b[201~\r".to_vec()); - assert_eq!(submit_bytes("a\x1bb", false), b"ab\r".to_vec()); + assert_eq!(submit_bytes("a\x1bb", false, false), b"ab\r".to_vec()); + // The same smuggling attempt on the typed path is just literal text at + // the prompt: there is no paste to break out of, and no ESC survives to + // reach the line editor as a command. + assert_eq!( + submit_bytes("foo\x1b[201~; rm -rf ~", true, false), + b"foo[201~; rm -rf ~\r".to_vec() + ); - assert_eq!(submit_bytes("", true), b"\r".to_vec()); + assert_eq!(submit_bytes("", true, false), b"\r".to_vec()); } #[test]