From 60ff61f6bac97ff735b2fd943f23f382123b3aa8 Mon Sep 17 00:00:00 2001 From: berce <75228263+bercedev@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:03:25 +0300 Subject: [PATCH 1/2] fix(cli): collect the changed cells once before painting a coloured diff colored() asked the diff about every painted cell, which scanned the whole changed list each time: a full-screen change on a large terminal took seconds (#595). The changed cells now go into a HashSet first. A CLI test diffs two 1000x1000 screens that differ everywhere. Signed-off-by: berce <75228263+bercedev@users.noreply.github.com> --- CHANGELOG.md | 7 ++++ crates/termlens-cli/src/main.rs | 7 ++-- crates/termlens-cli/tests/cli.rs | 59 ++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 445bb1b..ea42e24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,13 @@ reads that marker. ## [Unreleased] +### Fixed + +- A coloured `termlens diff` no longer takes time quadratic in the number of + changed cells (#595). Painting checked each cell against the whole list of + changed cells, so a full-screen change on a large terminal took seconds + (8 s at 80,000 cells). The changed cells are now collected once. + ## [0.11.6] - 2026-10-14 ### Fixed diff --git a/crates/termlens-cli/src/main.rs b/crates/termlens-cli/src/main.rs index 3d87717..6e96917 100644 --- a/crates/termlens-cli/src/main.rs +++ b/crates/termlens-cli/src/main.rs @@ -17,6 +17,7 @@ //! not propagated — and the screen alone goes to stdout, so `inspect … > //! file` is a saved screen `diff` and `render` read back (#340). +use std::collections::HashSet; use std::io::{self, IsTerminal, Read, Write}; use std::path::Path; use std::process::ExitCode; @@ -428,6 +429,9 @@ fn colored(before: &Screen, after: &Screen, diff: &ScreenDiff) -> String { } let mut changed_rows: Vec = diff.cells().map(|(row, _, _, _)| row).collect(); changed_rows.dedup(); + // Collected once: asking the diff about every painted cell made a + // full-screen change quadratic (#595). + let changed: HashSet<(u16, u16)> = diff.cells().map(|(row, col, _, _)| (row, col)).collect(); for row in changed_rows { let paint = |screen: &Screen, sgr: &str| -> String { let mut cells: Vec = Vec::new(); @@ -443,8 +447,7 @@ fn colored(before: &Screen, after: &Screen, diff: &ScreenDiff) -> String { } else { cell.contents() }; - let changed = diff.cells().any(|(r, c, _, _)| r == row && c == col); - cells.push(if changed { + cells.push(if changed.contains(&(row, col)) { format!("\x1b[{sgr}m{text}\x1b[0m") } else { text.to_owned() diff --git a/crates/termlens-cli/tests/cli.rs b/crates/termlens-cli/tests/cli.rs index 5e810ff..de6c337 100644 --- a/crates/termlens-cli/tests/cli.rs +++ b/crates/termlens-cli/tests/cli.rs @@ -133,6 +133,65 @@ fn diff_color_always_keeps_the_overlap_note_and_paints_the_rows() -> termlens::R /// flag ended the line, `Some("before.snap")` when it quietly ate the /// operand after it — internals that name neither the flag's requirement /// nor the mistake (#452). +/// Two 1000x1000 screens that differ in every cell. Painting used to ask +/// the diff about each cell in turn, which took 8 s at 80,000 changed cells +/// and would not finish here (#595). +#[test] +fn diff_color_always_paints_a_full_screen_change_quickly() { + use std::process::{Command, Stdio}; + use std::time::{Duration, Instant}; + + let dir = std::env::temp_dir().join(format!("termlens-big-diff-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("temp dir"); + let screen = |ch: char| { + let row = ch.to_string().repeat(1000); + format!( + "size: 1000x1000 cursor: 0,0\n{}\n", + vec![row; 1000].join("\n") + ) + }; + let (before, after, out) = ( + dir.join("before.snap"), + dir.join("after.snap"), + dir.join("out"), + ); + std::fs::write(&before, screen('a')).expect("write before"); + std::fs::write(&after, screen('b')).expect("write after"); + + // The output is tens of megabytes, so it goes to a file rather than a + // pipe nobody drains while the test waits on the deadline. + let mut child = Command::new(env!("CARGO_BIN_EXE_termlens")) + .args(["diff", "--color", "always"]) + .arg(&before) + .arg(&after) + .stdout(std::fs::File::create(&out).expect("out file")) + .stderr(Stdio::null()) + .spawn() + .expect("spawn termlens"); + let deadline = Instant::now() + Duration::from_secs(60); + let status = loop { + if let Some(status) = child.try_wait().expect("wait") { + break status; + } + if Instant::now() > deadline { + let _ = child.kill(); + panic!("a coloured diff of two 1000x1000 screens took over 60 s"); + } + std::thread::sleep(Duration::from_millis(50)); + }; + assert_eq!(status.code(), Some(1)); + let printed = std::fs::read_to_string(&out).expect("read output"); + assert!( + printed.contains("\x1b[31ma\x1b[0m"), + "the before side is painted red" + ); + assert!( + printed.contains("\x1b[32mb\x1b[0m"), + "the after side is painted green" + ); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn diff_names_a_missing_or_unknown_color_value() -> termlens::Result<()> { // The operand after the flag is taken as the value and refused as one From 901790a584558989ac30614bd9dd67d93587449f Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:56:21 +0700 Subject: [PATCH 2/2] test(cli): give the #452 doc back to its test, and wait without a sleep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new test was inserted between the #452 doc comment and the test it documents, so `diff_names_a_missing_or_unknown_color_value` lost its doc and the new test inherited it. It now sits above that comment. Its deadline was a try_wait loop with a 50 ms sleep, and CONTRIBUTING §3 allows only two sleeps in tests. A thread now waits on the exit and the test bounds that with recv_timeout. Checked against the old main.rs: the test still fails at the 60 s deadline. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com> --- crates/termlens-cli/tests/cli.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/crates/termlens-cli/tests/cli.rs b/crates/termlens-cli/tests/cli.rs index de6c337..a331fd3 100644 --- a/crates/termlens-cli/tests/cli.rs +++ b/crates/termlens-cli/tests/cli.rs @@ -129,17 +129,14 @@ fn diff_color_always_keeps_the_overlap_note_and_paints_the_rows() -> termlens::R Ok(()) } -/// `--color` with no WHEN used to print the raw Option — `None` when the -/// flag ended the line, `Some("before.snap")` when it quietly ate the -/// operand after it — internals that name neither the flag's requirement -/// nor the mistake (#452). /// Two 1000x1000 screens that differ in every cell. Painting used to ask /// the diff about each cell in turn, which took 8 s at 80,000 changed cells /// and would not finish here (#595). #[test] fn diff_color_always_paints_a_full_screen_change_quickly() { use std::process::{Command, Stdio}; - use std::time::{Duration, Instant}; + use std::sync::mpsc; + use std::time::Duration; let dir = std::env::temp_dir().join(format!("termlens-big-diff-{}", std::process::id())); std::fs::create_dir_all(&dir).expect("temp dir"); @@ -168,16 +165,17 @@ fn diff_color_always_paints_a_full_screen_change_quickly() { .stderr(Stdio::null()) .spawn() .expect("spawn termlens"); - let deadline = Instant::now() + Duration::from_secs(60); - let status = loop { - if let Some(status) = child.try_wait().expect("wait") { - break status; - } - if Instant::now() > deadline { - let _ = child.kill(); - panic!("a coloured diff of two 1000x1000 screens took over 60 s"); - } - std::thread::sleep(Duration::from_millis(50)); + // The exit is waited for on a thread of its own, so the deadline is a + // bounded wait on the exit itself rather than a polling sleep + // (CONTRIBUTING §3). On a regression the child is left to finish on its + // own; the test has already failed by then. + let (exited, exit) = mpsc::channel(); + std::thread::spawn(move || { + let _ = exited.send(child.wait()); + }); + let status = match exit.recv_timeout(Duration::from_secs(60)) { + Ok(status) => status.expect("wait for termlens"), + Err(_) => panic!("a coloured diff of two 1000x1000 screens took over 60 s"), }; assert_eq!(status.code(), Some(1)); let printed = std::fs::read_to_string(&out).expect("read output"); @@ -192,6 +190,10 @@ fn diff_color_always_paints_a_full_screen_change_quickly() { let _ = std::fs::remove_dir_all(&dir); } +/// `--color` with no WHEN used to print the raw Option — `None` when the +/// flag ended the line, `Some("before.snap")` when it quietly ate the +/// operand after it — internals that name neither the flag's requirement +/// nor the mistake (#452). #[test] fn diff_names_a_missing_or_unknown_color_value() -> termlens::Result<()> { // The operand after the flag is taken as the value and refused as one