From b5c20932cb55ce13afca2f6eebf1c3f88efb8fe2 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:59:46 +0800 Subject: [PATCH] fix(diff-overlay): keep the row key pointing at the snapshot that landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sync_diff_rows` wrote `rows_key` only when it rebuilt the rows. A probe that found nothing new lands a fresh `Arc` over an equal snapshot, so `describes` answered yes the long way — by walking every line of the patch — and then left the key holding the *old* `Arc`. Every frame after that paid the same walk: once per wheel event, over the ~1 MB of `String` at the 20k-line budget, which is the cost the key exists to avoid. Retarget it on a match, so the pointer comparison settles the next frame. The preview was doubly exposed: `install_diff_snapshot` clears it on every landing, so the re-read produces a fresh `Arc` too. Its comparison also ran eagerly, ahead of the scalars that could already have answered no; move it into the `&&` chain. `stack_corners` lost its last non-test caller when the file cards went, and `cargo build` had started reporting it. Drop it and the test that held it up. --- src/ui/diff_overlay.rs | 91 +++++++++++++++++++++++++++++++++++++----- src/ui/rounding.rs | 34 ---------------- 2 files changed, 82 insertions(+), 43 deletions(-) diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 074b1bac..1639cce4 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -844,11 +844,11 @@ impl Tty7App { oversized: focused.is_none() && snap.stats().oversized, expanded: &overlay.expanded, }; - if overlay + let stale = overlay .rows_key .as_ref() - .is_none_or(|held| !held.describes(&from)) - { + .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( @@ -863,6 +863,8 @@ impl Tty7App { 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)) } @@ -978,19 +980,40 @@ impl RowsKey { /// 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 { - let same_preview = match (&self.preview, from.preview) { - (None, None) => true, - (Some(a), Some(b)) => Arc::ptr_eq(a, b) || a == b, - _ => false, - }; self.mode == from.mode && self.focused == from.focused && self.oversized == from.oversized && self.expanded == *from.expanded - && same_preview + && 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. @@ -2115,6 +2138,56 @@ mod tests { 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 { 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)); - } }