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..5f3168d5 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]] @@ -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" @@ -891,7 +911,7 @@ dependencies = [ "bitflags 2.13.1", "cexpr", "clang-sys", - "itertools 0.13.0", + "itertools 0.11.0", "log", "prettyplease", "proc-macro2", @@ -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" @@ -1483,7 +1527,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 +2091,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 +2148,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -2142,7 +2186,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 +2438,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 +3243,7 @@ dependencies = [ "log", "presser", "thiserror 2.0.20", - "windows 0.62.2", + "windows 0.58.0", ] [[package]] @@ -3225,7 +3269,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 +3460,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 +3511,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 +3558,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 +3569,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 +3582,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 +3592,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 +3601,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 +3625,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 +3654,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 +3983,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 +4008,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 +4101,7 @@ dependencies = [ "js-sys", "log", "wasm-bindgen", - "windows-core 0.62.2", + "windows-core 0.58.0", ] [[package]] @@ -4581,9 +4625,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 +4945,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", @@ -5092,7 +5136,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 +5246,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 +5302,7 @@ dependencies = [ "once_cell", "png", "thiserror 2.0.20", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -5481,7 +5525,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]] @@ -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" @@ -6204,7 +6254,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 +6950,7 @@ dependencies = [ "once_cell", "socket2", "tracing", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -7207,7 +7257,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 +7300,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 +7661,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -7784,7 +7834,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 +8439,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 +8565,7 @@ dependencies = [ "cfg-if", "libc", "psm", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -8627,7 +8677,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 +8967,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 +9434,7 @@ dependencies = [ "once_cell", "png", "thiserror 2.0.20", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -9775,11 +9825,11 @@ dependencies = [ [[package]] name = "tty7" -version = "26.9.0" +version = "26.9.1" dependencies = [ "alacritty_terminal", "anyhow", - "async_zip", + "async_zip 0.0.19", "core-foundation 0.10.0", "gpui", "gpui-component", @@ -9820,7 +9870,7 @@ dependencies = [ [[package]] name = "tty7-cli" -version = "26.9.0" +version = "26.9.1" dependencies = [ "alacritty_terminal", "anyhow", @@ -9835,7 +9885,7 @@ dependencies = [ [[package]] name = "tty7-core" -version = "26.9.0" +version = "26.9.1" dependencies = [ "anyhow", "base64 0.22.1", @@ -9869,7 +9919,7 @@ dependencies = [ [[package]] name = "tty7-server" -version = "26.9.0" +version = "26.9.1" dependencies = [ "libc", "tempfile", @@ -9896,7 +9946,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" dependencies = [ "memoffset 0.9.1", "tempfile", - "windows-sys 0.61.2", + "windows-sys 0.60.2", ] [[package]] @@ -10108,11 +10158,11 @@ 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", - "async_zip", + "async_zip 0.0.18", "collections", "command-fds", "dirs", @@ -10147,7 +10197,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", @@ -10156,9 +10206,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", @@ -10740,7 +10790,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 +11541,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 +12098,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 +12143,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 +12154,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 31d358e5..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 @@ -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: @@ -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/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/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/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" + ); + } +} 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()); 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/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/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/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/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/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/element.rs b/src/terminal/element.rs index 6eee0416..55618483 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, @@ -975,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 { @@ -1037,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 @@ -1173,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, @@ -1183,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( @@ -2358,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. @@ -2707,30 +2770,142 @@ 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 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)); + 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 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)); + 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 + /// 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_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)], ); } 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/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/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 41249aad..f60f5ddd 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, @@ -697,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 } @@ -2759,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(); @@ -2773,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(); } @@ -3955,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); @@ -3978,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); @@ -4050,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(); @@ -4095,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(); @@ -4322,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(); @@ -4815,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(); } @@ -5327,9 +5437,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 +5451,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 +5707,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 +5723,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) { @@ -6325,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(); } @@ -7285,7 +7406,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::{ @@ -8348,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] @@ -8812,6 +9010,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}; @@ -8890,20 +9121,48 @@ pub(crate) fn quiet_test_pane( (view, daemon_side) } +/// 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, 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) } @@ -9520,7 +9779,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"); @@ -9586,13 +9845,19 @@ mod gpui_tests { /// yet" for the life of the pane — no underline, and a click that says /// nothing, which is the silence this whole path exists to remove. /// - /// Unix-only because the path it prints is: `Path::new("/etc/hosts")` - /// is not absolute on Windows, so `FileCandidate::paths` measures it - /// from the roots rather than letting it stand alone — and a workspace - /// that never connected has no roots, so nothing is ever wanted. Which - /// is itself the divergence: a Windows tty7 looking at a *remote* Linux - /// pane never probes the POSIX paths that pane prints, and so never - /// underlines them. + /// Was unix-only because the path it prints is: `Path::new("/etc/hosts")` + /// is not absolute on Windows, so `FileCandidate::paths` measured it from + /// the roots rather than letting it stand alone — and a workspace that + /// never connected has no roots, so nothing was ever wanted. Which was + /// itself the divergence: a Windows tty7 looking at a *remote* Linux pane + /// never probed the POSIX paths that pane printed. + /// + /// #795 settled that. `paths` now asks the pane's own + /// [`super::search::PathStyle`] rather than this machine's, and a remote + /// pane that has not reported a cwd is read as `Posix`, so `/etc/hosts` + /// stands alone on every client. The gate is only still here because + /// nothing has run this test on Windows yet; lifting it belongs in a + /// change that can show it green, not in a merge. #[cfg(unix)] #[gpui::test] fn a_probe_with_no_host_to_ask_stays_wanted(cx: &mut TestAppContext) { diff --git a/src/ui/app.rs b/src/ui/app.rs index ab6dad2b..bb8b6af1 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) @@ -10239,3 +10252,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/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 41401cfb..2840f6c7 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 { @@ -2469,18 +2667,22 @@ mod overlay_gpui_tests { /// terminal, an editor, a worktree command — and the cached branch is a branch /// the repository has left. That is what the stale entry below stands for. /// -/// Unix-only, and not for the harness: on Windows the repository root git -/// reports (`C:/Users/—`, straight out of MSYS2 git) is not the root the -/// seeded cache below holds. Not for the slash direction — `Path` compares -/// by component, so `C:/x` and `C:\x` are already equal. The splitter is -/// the prefix: the seeded root came past `fs::canonicalize`, which spells -/// it `\\?\C:\Users\—` and makes its prefix component `VerbatimDisk` where -/// git's answer parses as `Disk`. So `scm_epoch` never agrees with the -/// landing snapshot and the overlay re-probes on every frame — `load` -/// reaches `Ready` and `loading` goes straight back to `true`, which is the -/// exact spin this test exists to catch. That is a real divergence in the -/// SCM layer's path comparisons rather than a test artefact, so the gate -/// stays until those roots are keyed by one spelling (#796). +/// Unix-only, and not for the harness: on Windows the root this test seeds +/// the cache with is not the root the probe lands with, so `scm_epoch` never +/// agrees with the landing snapshot and the overlay re-probes on every frame +/// — `load` reaches `Ready` and `loading` goes straight back to `true`, which +/// is the exact spin this test exists to catch. +/// +/// Not the slash direction — `Path` compares by component, so `C:/x` and +/// `C:\x` are already equal. It is the prefix, and since #796 it is this +/// test's own: the product keys a repository by one spelling now +/// (`Host::canonicalize` drops the `\\?\` extended-length prefix and +/// `core::git::git_path` re-spells what git prints), while the seed below +/// still comes straight from `std::fs::canonicalize` and so carries +/// `\\?\C:\Users\—` — a `VerbatimDisk` prefix where everything it is +/// compared against is now `Disk`. Seeding through +/// `tty7_core::core::path_spelling` should lift this, as a change that can +/// show it green rather than a drive-by. #[cfg(all(test, unix))] mod render_idle_gpui_tests { use super::*; @@ -2500,6 +2702,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/i18n/en.rs b/src/ui/i18n/en.rs index eb62d36f..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", @@ -1430,6 +1431,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 +1509,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..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 => "ターミナルベル", @@ -1487,6 +1488,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 +1564,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..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 => "终端铃声", @@ -1348,6 +1349,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 +1425,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/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/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)), ]; 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/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(), 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/scm/detail.rs b/src/ui/scm/detail.rs index 5bc2eff2..043e2747 100644 --- a/src/ui/scm/detail.rs +++ b/src/ui/scm/detail.rs @@ -975,17 +975,19 @@ mod tests { /// character — goes wrong during layout and paint, so these arm the render /// probe and insist something was actually drawn. /// -/// Still unix-only, and for a reason worth naming rather than a harness -/// one: on Windows `git rev-parse --show-toplevel` (Git for Windows is -/// MSYS2) prints `C:/Users/—`, and that string is what -/// `tty7_core::core::git::probe` stores as `RepoSnapshot::root`. The -/// forward slashes are not what breaks it — `Path` compares by component, -/// so `C:/x` and `C:\x` are equal. The prefix is: `scratch` below hands the -/// pane `fs::canonicalize`'s `\\?\C:\Users\—`, whose prefix component is -/// `VerbatimDisk` where git's answer parses as `Disk`, so the plain -/// `PathBuf` equalities here — and in the SCM cache — never match and the -/// panel never settles on the directory it is already showing. Taking the -/// gate off needs those roots keyed by one spelling (#796). +/// Still unix-only, and for a reason worth naming rather than a harness one: +/// on Windows the root the panel settles on is not the root this module hands +/// it, so the panel never settles on the directory it is already showing. +/// +/// The forward slashes `git rev-parse --show-toplevel` prints are not what +/// breaks it — `Path` compares by component, so `C:/x` and `C:\x` are equal. +/// The prefix is, and since #796 it is this module's own: the product keys a +/// repository by one spelling now, while `scratch` below still hands the pane +/// `std::fs::canonicalize`'s `\\?\C:\Users\—`, a `VerbatimDisk` prefix where +/// every root it is compared against is `Disk`. Taking the gate off needs +/// that helper to spell its answer the way +/// `tty7_core::core::path_spelling` does, in a change that can show these +/// green rather than a drive-by. #[cfg(all(test, unix))] mod detail_gpui_tests { use super::*; diff --git a/src/ui/scm/panel.rs b/src/ui/scm/panel.rs index ed15602a..f828597d 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(); }, @@ -2974,14 +2980,16 @@ mod render_idle_gpui_tests { render_probe::draws() } - /// The only test in this module that waits on `repo.root`, and so the - /// only one Windows cannot run: git spells that root `C:/Users/—`, which - /// parses to a `Disk` prefix, while the pane's cwd came out of `scratch` - /// above — `fs::canonicalize`, so `\\?\C:\Users\—` and a `VerbatimDisk` - /// prefix — and the equality below never holds between the two. The - /// slashes are the red herring here; `Path` compares by component, so - /// `C:/x` and `C:\x` are equal. Its sibling keys off the pane's cwd - /// instead, and runs everywhere. + /// The only test in this module that waits on `repo.root`, and so the only + /// one Windows cannot run: since #796 that root is keyed by one spelling + /// and carries a `Disk` prefix, while the pane's cwd came out of `scratch` + /// above — `std::fs::canonicalize`, so `\\?\C:\Users\—` and a + /// `VerbatimDisk` prefix — and the equality below never holds between the + /// two. The slashes are the red herring; `Path` compares by component, so + /// `C:/x` and `C:\x` are equal. Spelling `scratch`'s answer the way + /// `tty7_core::core::path_spelling` does should lift this, in a change + /// that can show it green. Its sibling keys off the pane's cwd instead, + /// and runs everywhere. #[cfg(unix)] #[gpui::test] fn a_settled_source_control_panel_reaches_render_idle(cx: &mut TestAppContext) { 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() 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 b69a2349..495b561f 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::*;