From d18060e16a330ade6e38d221c19af97a470eca8a Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:46:25 -0400 Subject: [PATCH] git_graph: Fix height realignment issues (#54429) Special thanks to @lingyaochu for finding out the root cause of the issue. Took the test from: https://github.com/zed-industries/zed/pull/54233 The git graph was using `buffer_ui_size` when calculating the canvas row height and the table row height. This is wrong because elements such as `Labels` in the graph table were rendered using `ui_font_size`. This PR normalizes the row height calculation by always using `ui_font_size` for the canvas and table row height calculation, and taking into account the scaling factor. This PR also fixes a bug where the bottom of the canvas could flicker on its first redraw because the uniform list hadn't cached the viewport size yet, and we underestimated the visible size of the canvas and underdrew it. We now fallback to the window height as the viewport size. This means we'll overdraw for a single frame whenever the uniform list hasn't cached the last item size, but it avoids the flicker! Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Closes #53492 Closes #53469 Release Notes: - git_graph: Fix misalignment issues between the graph canvas and the graph table --------- Co-authored-by: lingyaochu --- crates/git_graph/src/git_graph.rs | 170 ++++++++++++++++++++++++------ 1 file changed, 139 insertions(+), 31 deletions(-) diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 34599ddefdac5baf62259bccf2c7ab46feb6c43f..e0175db09f1ef95fbd62d06ef71d20cfa63e4444 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -27,7 +27,6 @@ use search::{ SearchOption, SearchOptions, SearchSource, SelectNextMatch, SelectPreviousMatch, ToggleCaseSensitive, buffer_search, }; -use settings::Settings; use smallvec::{SmallVec, smallvec}; use std::{ cell::Cell, @@ -37,7 +36,6 @@ use std::{ time::{Duration, Instant}, }; use theme::AccentColors; -use theme_settings::ThemeSettings; use time::{OffsetDateTime, UtcOffset, format_description::BorrowedFormatItem}; use ui::{ ButtonLike, Chip, ColumnWidthConfig, CommonAnimationExt as _, ContextMenu, DiffStat, Divider, @@ -58,6 +56,9 @@ const LEFT_PADDING: Pixels = px(12.0); const LINE_WIDTH: Pixels = px(1.5); const RESIZE_HANDLE_WIDTH: f32 = 8.0; const COPIED_STATE_DURATION: Duration = Duration::from_secs(2); +// Extra vertical breathing room added to the UI line height when computing +// the git graph's row height, so commit dots and lines have space around them. +const ROW_VERTICAL_PADDING: Pixels = px(4.0); struct CopiedState { copied_at: Option, @@ -901,7 +902,6 @@ pub struct GitGraph { git_store: Entity, workspace: WeakEntity, context_menu: Option<(Entity, Point, Subscription)>, - row_height: Pixels, table_interaction_state: Entity, column_widths: Entity, selected_entry_idx: Option, @@ -927,10 +927,19 @@ impl GitGraph { cx.notify(); } - fn row_height(cx: &App) -> Pixels { - let settings = ThemeSettings::get_global(cx); - let font_size = settings.buffer_font_size(cx); - font_size + px(12.0) + /// Computes the height of a single commit row in the git graph. + /// + /// The returned value is snapped to the nearest physical pixel. This is + /// required so that the canvas's float math and the `uniform_list` layout + /// (which snaps to device pixels) agree on row positions; otherwise rows + /// drift apart as the user scrolls when `ui_font_size` is fractional. + fn row_height(window: &Window, _cx: &App) -> Pixels { + let rem_size = window.rem_size(); + let line_height = window.text_style().line_height_in_pixels(rem_size); + let raw = line_height + ROW_VERTICAL_PADDING; + let scale = window.scale_factor(); + + (raw * scale).round() / scale } fn graph_canvas_content_width(&self) -> Pixels { @@ -1035,12 +1044,14 @@ impl GitGraph { ], ) }); - let mut row_height = Self::row_height(cx); + let mut row_height = Self::row_height(window, cx); - cx.observe_global_in::(window, move |this, _window, cx| { - let new_row_height = Self::row_height(cx); + cx.observe_global_in::(window, move |this, window, cx| { + let new_row_height = Self::row_height(window, cx); if new_row_height != row_height { - this.row_height = new_row_height; + // The `uniform_list` powering the table caches the item size + // from its last layout; invalidate it so it re-measures with + // the new row height on the next frame. this.table_interaction_state.update(cx, |state, _cx| { state.scroll_handle.0.borrow_mut().last_item_size = None; }); @@ -1064,7 +1075,6 @@ impl GitGraph { graph_data: graph, _commit_diff_task: None, context_menu: None, - row_height, table_interaction_state, column_widths, selected_entry_idx: None, @@ -1216,7 +1226,7 @@ impl GitGraph { fn render_table_rows( &mut self, range: Range, - _window: &mut Window, + window: &mut Window, cx: &mut Context, ) -> Vec> { let repository = self.get_repository(cx); @@ -1229,7 +1239,7 @@ impl GitGraph { .map(|branch| SharedString::from(branch.name().to_string())) }); - let row_height = self.row_height; + let row_height = Self::row_height(window, cx); // We fetch data outside the visible viewport to avoid loading entries when // users scroll through the git graph @@ -2168,7 +2178,7 @@ impl GitGraph { } pub fn render_graph(&self, window: &Window, cx: &mut Context) -> impl IntoElement { - let row_height = self.row_height; + let row_height = Self::row_height(window, cx); let table_state = self.table_interaction_state.read(cx); let viewport_height = table_state .scroll_handle @@ -2176,7 +2186,7 @@ impl GitGraph { .borrow() .last_item_size .map(|size| size.item.height) - .unwrap_or(px(600.0)); + .unwrap_or(window.viewport_size().height); let loaded_commit_count = self.graph_data.commits.len(); let content_height = row_height * loaded_commit_count; @@ -2434,7 +2444,12 @@ impl GitGraph { .h_full() } - fn row_at_position(&self, position_y: Pixels, cx: &Context) -> Option { + fn row_at_position( + &self, + position_y: Pixels, + window: &Window, + cx: &Context, + ) -> Option { let canvas_bounds = self.graph_canvas_bounds.get()?; let table_state = self.table_interaction_state.read(cx); let scroll_offset_y = -table_state.scroll_offset().y; @@ -2443,7 +2458,8 @@ impl GitGraph { if local_y >= px(0.) && local_y < canvas_bounds.size.height { let absolute_y = local_y + scroll_offset_y; - let absolute_row = (absolute_y / self.row_height).floor() as usize; + let row_height = Self::row_height(window, cx); + let absolute_row = (absolute_y / row_height).floor() as usize; if absolute_row < self.graph_data.commits.len() { return Some(absolute_row); @@ -2456,10 +2472,10 @@ impl GitGraph { fn handle_graph_mouse_move( &mut self, event: &gpui::MouseMoveEvent, - _window: &mut Window, + window: &mut Window, cx: &mut Context, ) { - if let Some(row) = self.row_at_position(event.position.y, cx) { + if let Some(row) = self.row_at_position(event.position.y, window, cx) { if self.hovered_entry_idx != Some(row) { self.hovered_entry_idx = Some(row); cx.notify(); @@ -2476,7 +2492,7 @@ impl GitGraph { window: &mut Window, cx: &mut Context, ) { - if let Some(row) = self.row_at_position(event.position().y, cx) { + if let Some(row) = self.row_at_position(event.position().y, window, cx) { self.select_entry(row, ScrollStrategy::Nearest, cx); if event.click_count() >= 2 { self.open_commit_view(row, window, cx); @@ -2502,7 +2518,7 @@ impl GitGraph { AllCommitCount::Loaded(count) => count, AllCommitCount::NotLoaded => self.graph_data.commits.len(), }; - let content_height = self.row_height * commit_count; + let content_height = Self::row_height(window, cx) * commit_count; let max_vertical_scroll = (viewport_height - content_height).min(px(0.)); let new_y = (current_offset.y + delta.y).clamp(max_vertical_scroll, px(0.)); @@ -2660,7 +2676,7 @@ impl Render for GitGraph { cx, )) .child({ - let row_height = self.row_height; + let row_height = Self::row_height(window, cx); let selected_entry_idx = self.selected_entry_idx; let hovered_entry_idx = self.hovered_entry_idx; let weak_self = cx.weak_entity(); @@ -3089,12 +3105,12 @@ mod tests { use fs::FakeFs; use git::Oid; use git::repository::InitialGraphCommitData; - use gpui::TestAppContext; + use gpui::{TestAppContext, UpdateGlobal}; use project::Project; use project::git_store::{GitStoreEvent, RepositoryEvent}; use rand::prelude::*; use serde_json::json; - use settings::SettingsStore; + use settings::{SettingsStore, ThemeSettingsContent}; use smallvec::{SmallVec, smallvec}; use std::path::Path; use std::sync::{Arc, Mutex}; @@ -4163,24 +4179,27 @@ mod tests { }); cx.run_until_parked(); - git_graph.update(cx, |graph, cx| { + git_graph.update_in(cx, |graph, window, cx| { assert!( graph.graph_data.commits.len() >= 10, "graph should load dummy commits" ); - graph.row_height = px(20.0); + let row_height = GitGraph::row_height(window, cx); let origin_y = px(100.0); graph.graph_canvas_bounds.set(Some(Bounds { origin: point(px(0.0), origin_y), - size: gpui::size(px(100.0), px(1000.0)), + size: gpui::size(px(100.0), row_height * 50.0), })); + // Scroll down by half a row so the row under a position near the + // top of the canvas is row 1 rather than row 0. + let scroll_offset = row_height * 0.75; graph.table_interaction_state.update(cx, |state, _| { - state.set_scroll_offset(point(px(0.0), px(-15.0))) + state.set_scroll_offset(point(px(0.0), -scroll_offset)) }); - let pos_y = origin_y + px(10.0); - let absolute_calc_row = graph.row_at_position(pos_y, cx); + let pos_y = origin_y + row_height * 0.5; + let absolute_calc_row = graph.row_at_position(pos_y, window, cx); assert_eq!( absolute_calc_row, @@ -4189,4 +4208,93 @@ mod tests { ); }); } + + #[gpui::test] + async fn test_row_height_matches_uniform_list_item_height(cx: &mut TestAppContext) { + init_test(cx); + + cx.update(|cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + *settings.theme = ThemeSettingsContent { + ui_font_size: Some(12.7.into()), + ..Default::default() + } + }); + }) + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project"), + serde_json::json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + + let mut rng = StdRng::seed_from_u64(99); + let commits = generate_random_commit_dag(&mut rng, 20, false); + fs.set_graph_commits(Path::new("/project/.git"), commits); + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + cx.run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project + .active_repository(cx) + .expect("should have a repository") + }); + + let (multi_workspace, cx) = cx.add_window_view(|window, cx| { + workspace::MultiWorkspace::test_new(project.clone(), window, cx) + }); + + let workspace_weak = + multi_workspace.read_with(&*cx, |multi, _| multi.workspace().downgrade()); + + let git_graph = cx.new_window_entity(|window, cx| { + GitGraph::new( + repository.read(cx).id, + project.read(cx).git_store().clone(), + workspace_weak, + window, + cx, + ) + }); + cx.run_until_parked(); + + cx.draw( + point(px(0.), px(0.)), + gpui::size(px(1200.), px(800.)), + |_, _| git_graph.clone().into_any_element(), + ); + cx.run_until_parked(); + + git_graph.update_in(cx, |graph, window, cx| { + let commit_count = graph.graph_data.commits.len(); + assert!( + commit_count > 0, + "need at least one commit to measure item height" + ); + + let table_state = graph.table_interaction_state.read(cx); + let item_size = table_state.scroll_handle.0.borrow().last_item_size.expect( + "uniform_list should have populated last_item_size after draw(); \ + the table has not been laid out", + ); + + let measured_item_height = item_size.contents.height / commit_count as f32; + let computed_row_height = GitGraph::row_height(window, cx); + + assert_eq!( + computed_row_height, measured_item_height, + "GitGraph::row_height ({}) must exactly match the height that \ + uniform_list measured for each table row ({}). \ + A mismatch means the canvas and table rows will drift when scrolling.", + computed_row_height, measured_item_height, + ); + }); + } }