diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index e2781436d491b8ec7c6237ed4909427c1cdfa084..5e5a39f07ea014e54152d823778ccb475f2531dc 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -1021,6 +1021,16 @@ struct PhantomBreakpointIndicator { collides_with_existing_breakpoint: bool, } +/// Represents a diff review button indicator that shows up when hovering over lines in the gutter +/// in diff view mode. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct PhantomDiffReviewIndicator { + pub display_row: DisplayRow, + /// There's a small debounce between hovering over the line and showing the indicator. + /// We don't want to show the indicator when moving the mouse from editor to e.g. project panel. + pub is_active: bool, +} + /// Zed's primary implementation of text input, allowing users to edit a [`MultiBuffer`]. /// /// See the [module level documentation](self) for more information. @@ -1081,6 +1091,7 @@ pub struct Editor { show_code_actions: Option, show_runnables: Option, show_breakpoints: Option, + show_diff_review_button: bool, show_wrap_guides: Option, show_indent_guides: Option, buffers_with_disabled_indent_guides: HashSet, @@ -1183,6 +1194,7 @@ pub struct Editor { tasks_update_task: Option>, breakpoint_store: Option>, gutter_breakpoint_indicator: (Option, Option>), + pub(crate) gutter_diff_review_indicator: (Option, Option>), hovered_diff_hunk_row: Option, pull_diagnostics_task: Task<()>, pull_diagnostics_background_task: Task<()>, @@ -2246,6 +2258,7 @@ impl Editor { show_code_actions: None, show_runnables: None, show_breakpoints: None, + show_diff_review_button: false, show_wrap_guides: None, show_indent_guides, buffers_with_disabled_indent_guides: HashSet::default(), @@ -2346,6 +2359,7 @@ impl Editor { breakpoint_store, gutter_breakpoint_indicator: (None, None), + gutter_diff_review_indicator: (None, None), hovered_diff_hunk_row: None, _subscriptions: (!is_minimap) .then(|| { @@ -8632,11 +8646,15 @@ impl Editor { (true, true) => ui::IconName::DebugDisabledLogBreakpoint, }; - let color = cx.theme().colors(); + let theme_colors = cx.theme().colors(); let color = if is_phantom { if collides_with_existing { - Color::Custom(color.debugger_accent.blend(color.text.opacity(0.6))) + Color::Custom( + theme_colors + .debugger_accent + .blend(theme_colors.text.opacity(0.6)), + ) } else { Color::Hint } @@ -20732,6 +20750,15 @@ impl Editor { cx.notify(); } + pub fn set_show_diff_review_button(&mut self, show: bool, cx: &mut Context) { + self.show_diff_review_button = show; + cx.notify(); + } + + pub fn show_diff_review_button(&self) -> bool { + self.show_diff_review_button + } + pub fn set_masked(&mut self, masked: bool, cx: &mut Context) { if self.display_map.read(cx).masked != masked { self.display_map.update(cx, |map, _| map.masked = masked); diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index f63644adb9808c28f68fb6b775b56cbc570d701e..d996d09f73be767e2405d85d2504ca6e65f663d7 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -30204,3 +30204,161 @@ fn test_editor_rendering_when_positioned_above_viewport(cx: &mut TestAppContext) // If we get here without hanging, the test passes } + +#[gpui::test] +async fn test_diff_review_indicator_created_on_gutter_hover(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({ "file.txt": "hello\nworld\n" })) + .await; + + let project = Project::test(fs, [path!("/root").as_ref()], cx).await; + let workspace = cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + + let editor = workspace + .update(cx, |workspace, window, cx| { + workspace.open_abs_path( + PathBuf::from(path!("/root/file.txt")), + OpenOptions::default(), + window, + cx, + ) + }) + .unwrap() + .await + .unwrap() + .downcast::() + .unwrap(); + + // Enable diff review button mode + editor.update(cx, |editor, cx| { + editor.set_show_diff_review_button(true, cx); + }); + + // Initially, no indicator should be present + editor.update(cx, |editor, _cx| { + assert!( + editor.gutter_diff_review_indicator.0.is_none(), + "Indicator should be None initially" + ); + }); +} + +#[gpui::test] +async fn test_diff_review_button_hidden_when_ai_disabled(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + // Register DisableAiSettings and set disable_ai to true + cx.update(|cx| { + project::DisableAiSettings::register(cx); + project::DisableAiSettings::override_global( + project::DisableAiSettings { disable_ai: true }, + cx, + ); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({ "file.txt": "hello\nworld\n" })) + .await; + + let project = Project::test(fs, [path!("/root").as_ref()], cx).await; + let workspace = cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + + let editor = workspace + .update(cx, |workspace, window, cx| { + workspace.open_abs_path( + PathBuf::from(path!("/root/file.txt")), + OpenOptions::default(), + window, + cx, + ) + }) + .unwrap() + .await + .unwrap() + .downcast::() + .unwrap(); + + // Enable diff review button mode + editor.update(cx, |editor, cx| { + editor.set_show_diff_review_button(true, cx); + }); + + // Verify AI is disabled + cx.read(|cx| { + assert!( + project::DisableAiSettings::get_global(cx).disable_ai, + "AI should be disabled" + ); + }); + + // The indicator should not be created when AI is disabled + // (The mouse_moved handler checks DisableAiSettings before creating the indicator) + editor.update(cx, |editor, _cx| { + assert!( + editor.gutter_diff_review_indicator.0.is_none(), + "Indicator should be None when AI is disabled" + ); + }); +} + +#[gpui::test] +async fn test_diff_review_button_shown_when_ai_enabled(cx: &mut TestAppContext) { + init_test(cx, |_| {}); + + // Register DisableAiSettings and set disable_ai to false + cx.update(|cx| { + project::DisableAiSettings::register(cx); + project::DisableAiSettings::override_global( + project::DisableAiSettings { disable_ai: false }, + cx, + ); + }); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/root"), json!({ "file.txt": "hello\nworld\n" })) + .await; + + let project = Project::test(fs, [path!("/root").as_ref()], cx).await; + let workspace = cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + + let editor = workspace + .update(cx, |workspace, window, cx| { + workspace.open_abs_path( + PathBuf::from(path!("/root/file.txt")), + OpenOptions::default(), + window, + cx, + ) + }) + .unwrap() + .await + .unwrap() + .downcast::() + .unwrap(); + + // Enable diff review button mode + editor.update(cx, |editor, cx| { + editor.set_show_diff_review_button(true, cx); + }); + + // Verify AI is enabled + cx.read(|cx| { + assert!( + !project::DisableAiSettings::get_global(cx).disable_ai, + "AI should be enabled" + ); + }); + + // The show_diff_review_button flag should be true + editor.update(cx, |editor, _cx| { + assert!( + editor.show_diff_review_button(), + "show_diff_review_button should be true" + ); + }); +} diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 52fbc129702a4a1c23926c4600d1b1138ba1fa10..2d4e4f82a5feedb5f4c2dcb676363a2246db208a 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -7,9 +7,9 @@ use crate::{ EditorStyle, FILE_HEADER_HEIGHT, FocusedBlock, GutterDimensions, HalfPageDown, HalfPageUp, HandleInput, HoveredCursor, InlayHintRefreshReason, JumpData, LineDown, LineHighlight, LineUp, MAX_LINE_LEN, MINIMAP_FONT_SIZE, MULTI_BUFFER_EXCERPT_HEADER_HEIGHT, OpenExcerpts, PageDown, - PageUp, PhantomBreakpointIndicator, Point, RowExt, RowRangeExt, SelectPhase, - SelectedTextHighlight, Selection, SelectionDragState, SelectionEffects, SizingBehavior, - SoftWrap, StickyHeaderExcerpt, ToPoint, ToggleFold, ToggleFoldAll, + PageUp, PhantomBreakpointIndicator, PhantomDiffReviewIndicator, Point, RowExt, RowRangeExt, + SelectPhase, SelectedTextHighlight, Selection, SelectionDragState, SelectionEffects, + SizingBehavior, SoftWrap, StickyHeaderExcerpt, ToPoint, ToggleFold, ToggleFoldAll, code_context_menus::{CodeActionsMenu, MENU_ASIDE_MAX_WIDTH, MENU_ASIDE_MIN_WIDTH, MENU_GAP}, column_pixels, display_map::{ @@ -36,6 +36,7 @@ use crate::{ }; use buffer_diff::{DiffHunkStatus, DiffHunkStatusKind}; use collections::{BTreeMap, HashMap}; +use feature_flags::{DiffReviewFeatureFlag, FeatureFlagAppExt as _}; use file_icons::FileIcons; use git::{Oid, blame::BlameEntry, commit::ParsedCommitMessage, status::FileStatus}; use gpui::{ @@ -60,7 +61,7 @@ use multi_buffer::{ use edit_prediction_types::EditPredictionGranularity; use project::{ - Entry, ProjectPath, + DisableAiSettings, Entry, ProjectPath, debugger::breakpoint_store::{Breakpoint, BreakpointSessionState}, project_settings::ProjectSettings, }; @@ -1266,7 +1267,60 @@ impl EditorElement { } } - let breakpoint_indicator = if gutter_hovered { + // Handle diff review indicator when gutter is hovered in diff mode with AI enabled + let show_diff_review = editor.show_diff_review_button() + && cx.has_flag::() + && !DisableAiSettings::get_global(cx).disable_ai; + + let diff_review_indicator = if gutter_hovered && show_diff_review { + let is_visible = editor + .gutter_diff_review_indicator + .0 + .is_some_and(|indicator| indicator.is_active); + + if !is_visible { + editor + .gutter_diff_review_indicator + .1 + .get_or_insert_with(|| { + cx.spawn(async move |this, cx| { + cx.background_executor() + .timer(Duration::from_millis(200)) + .await; + + this.update(cx, |this, cx| { + if let Some(indicator) = + this.gutter_diff_review_indicator.0.as_mut() + { + indicator.is_active = true; + cx.notify(); + } + }) + .ok(); + }) + }); + } + + Some(PhantomDiffReviewIndicator { + display_row: valid_point.row(), + is_active: is_visible, + }) + } else { + editor.gutter_diff_review_indicator.1 = None; + None + }; + + if diff_review_indicator != editor.gutter_diff_review_indicator.0 { + editor.gutter_diff_review_indicator.0 = diff_review_indicator; + cx.notify(); + } + + // Don't show breakpoint indicator when diff review indicator is active on this row + let is_on_diff_review_button_row = diff_review_indicator.is_some_and(|indicator| { + indicator.is_active && indicator.display_row == valid_point.row() + }); + + let breakpoint_indicator = if gutter_hovered && !is_on_diff_review_button_row { let buffer_anchor = position_map .snapshot .display_point_to_anchor(valid_point, Bias::Left); @@ -3013,7 +3067,7 @@ impl EditorElement { let button = editor.render_breakpoint(text_anchor, display_row, &bp, state, cx); let button = prepaint_gutter_button( - button, + button.into_any_element(), display_row, line_height, gutter_dimensions, @@ -3029,6 +3083,50 @@ impl EditorElement { }) } + fn should_render_diff_review_button( + &self, + range: Range, + row_infos: &[RowInfo], + cx: &App, + ) -> Option { + if !cx.has_flag::() { + return None; + } + + let show_diff_review_button = self.editor.read(cx).show_diff_review_button(); + if !show_diff_review_button { + return None; + } + + let indicator = self.editor.read(cx).gutter_diff_review_indicator.0?; + if !indicator.is_active { + return None; + } + + let display_row = indicator.display_row; + + // Don't show on rows with expand excerpt buttons + if row_infos + .get((display_row.0.saturating_sub(range.start.0)) as usize) + .is_some_and(|row_info| row_info.expand_info.is_some()) + { + return None; + } + + Some(display_row) + } + + fn diff_review_button() -> AnyElement { + IconButton::new("diff_review_button", ui::IconName::Plus) + .icon_size(ui::IconSize::XSmall) + .size(ui::ButtonSize::None) + .icon_color(ui::Color::Default) + .style(ui::ButtonStyle::Filled) + .layer(ui::ElevationIndex::Surface) + .tooltip(Tooltip::text("Add Review")) + .into_any_element() + } + #[allow(clippy::too_many_arguments)] fn layout_run_indicators( &self, @@ -3115,16 +3213,17 @@ impl EditorElement { return None; } + let removed_breakpoint = breakpoints.remove(&display_row); let button = editor.render_run_indicator( &self.style, Some(display_row) == active_task_indicator_row, display_row, - breakpoints.remove(&display_row), + removed_breakpoint, cx, ); let button = prepaint_gutter_button( - button, + button.into_any_element(), display_row, line_height, gutter_dimensions, @@ -6470,6 +6569,10 @@ impl EditorElement { for test_indicator in layout.test_indicators.iter_mut() { test_indicator.paint(window, cx); } + + if let Some(diff_review_button) = layout.diff_review_button.as_mut() { + diff_review_button.paint(window, cx); + } }); } @@ -8238,7 +8341,7 @@ impl AcceptEditPredictionBinding { } fn prepaint_gutter_button( - button: IconButton, + mut button: AnyElement, row: DisplayRow, line_height: Pixels, gutter_dimensions: &GutterDimensions, @@ -8248,8 +8351,6 @@ fn prepaint_gutter_button( window: &mut Window, cx: &mut App, ) -> AnyElement { - let mut button = button.into_any_element(); - let available_space = size( AvailableSpace::MinContent, AvailableSpace::Definite(line_height), @@ -8276,11 +8377,7 @@ fn prepaint_gutter_button( .and_then(|ix| Some(display_hunks[ix].1.as_ref()?.size.width)); let left_offset = blame_width.max(gutter_width).unwrap_or_default(); - let mut x = left_offset; - let available_width = gutter_dimensions.margin + gutter_dimensions.left_padding - - indicator_size.width - - left_offset; - x += available_width / 2.; + let x = left_offset; let mut y = Pixels::from((row.as_f64() - scroll_position.y) * ScrollPixelOffset::from(line_height)); @@ -10331,6 +10428,22 @@ impl Element for EditorElement { Vec::new() }; + let diff_review_button = self + .should_render_diff_review_button(start_row..end_row, &row_infos, cx) + .map(|display_row| { + prepaint_gutter_button( + Self::diff_review_button(), + display_row, + line_height, + &gutter_dimensions, + scroll_position, + &gutter_hitbox, + &display_hunks, + window, + cx, + ) + }); + self.layout_signature_help( &hitbox, content_origin, @@ -10528,6 +10641,7 @@ impl Element for EditorElement { mouse_context_menu, test_indicators, breakpoints, + diff_review_button, crease_toggles, crease_trailers, tab_invisible, @@ -10706,6 +10820,7 @@ pub struct EditorLayout { selections: Vec<(PlayerColor, Vec)>, test_indicators: Vec, breakpoints: Vec, + diff_review_button: Option, crease_toggles: Vec>, expand_toggles: Vec)>>, diff_hunk_controls: Vec, diff --git a/crates/feature_flags/src/flags.rs b/crates/feature_flags/src/flags.rs index 83e422b5bf0824999a994e1facf57d5472634ed1..093470b64ba16700eea444fac47f4ec1a0b295ad 100644 --- a/crates/feature_flags/src/flags.rs +++ b/crates/feature_flags/src/flags.rs @@ -28,6 +28,10 @@ pub struct ToolPermissionsFeatureFlag; impl FeatureFlag for ToolPermissionsFeatureFlag { const NAME: &'static str = "tool-permissions"; + + fn enabled_for_staff() -> bool { + false + } } pub struct AgentSharingFeatureFlag; @@ -46,6 +50,16 @@ impl FeatureFlag for SubagentsFeatureFlag { } } +pub struct DiffReviewFeatureFlag; + +impl FeatureFlag for DiffReviewFeatureFlag { + const NAME: &'static str = "diff-review"; + + fn enabled_for_staff() -> bool { + false + } +} + /// Whether to use the OpenAI Responses API format when sending requests to Cloud. pub struct OpenAiResponsesApiFeatureFlag; diff --git a/crates/git_ui/src/commit_view.rs b/crates/git_ui/src/commit_view.rs index e406b5f2e4ef8980967f4653819b742118e3f094..f33f877851455c292dd83dcf140ee55407b3b481 100644 --- a/crates/git_ui/src/commit_view.rs +++ b/crates/git_ui/src/commit_view.rs @@ -204,6 +204,7 @@ impl CommitView { editor.disable_inline_diagnostics(); editor.set_show_breakpoints(false, cx); + editor.set_show_diff_review_button(true, cx); editor.set_expand_all_diff_hunks(cx); editor.disable_header_for_buffer(message_buffer.read(cx).remote_id(), cx); editor.disable_indent_guides_for_buffer(message_buffer.read(cx).remote_id(), cx); diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index c1ee5dff9217404e73464e71152546de7640209b..ce85835e3dd0d4afc859db7f3ee2d47564adc04a 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -289,6 +289,7 @@ impl ProjectDiff { .primary_editor() .update(cx, |editor, cx| { editor.disable_diagnostics(cx); + editor.set_show_diff_review_button(true, cx); match branch_diff.read(cx).diff_base() { DiffBase::Head => { diff --git a/plans/visual-test-foreground-executor.md b/plans/visual-test-foreground-executor.md deleted file mode 100644 index 5ab2f4699a1082e23d8813d0b6cc385bc29e017b..0000000000000000000000000000000000000000 --- a/plans/visual-test-foreground-executor.md +++ /dev/null @@ -1,506 +0,0 @@ -# Controllable Foreground Executor for Visual Tests - -## Status: COMPLETED ✓ - -The implementation is complete. See the summary at the end of this document. - -## Problem Statement - -The visual test framework (`VisualTestAppContext`) cannot properly test UI interactions that rely on foreground async tasks, such as tooltips, hover states with delays, and other deferred UI updates. This is because: - -1. **The visual test framework uses the real macOS platform** for actual rendering (needed for screenshot capture) -2. **But it doesn't drive the macOS run loop**, so foreground tasks spawned via `window.spawn()` never execute -3. **`run_until_parked()` only drives the background executor**, not the foreground executor - -### Specific Example: Tooltip Testing - -When trying to capture a screenshot of a tooltip: - -1. We call `window.simulate_mouse_move(position, cx)` to hover over a button -2. The button's tooltip handler in `register_tooltip_mouse_handlers` (in `div.rs`) spawns a delayed task: - ```rust - let delayed_show_task = window.spawn(cx, { - async move |cx| { - cx.background_executor().timer(TOOLTIP_SHOW_DELAY).await; // 500ms - cx.update(|window, cx| { - // Set active_tooltip state - window.refresh(); - }).ok(); - } - }); - ``` -3. We wait with `cx.background_executor().timer(700ms).await` -4. We take a screenshot - **but the tooltip never appears** - -The reason: `window.spawn()` schedules work on the **foreground executor**, but our test only drives the **background executor**. The tooltip task is sitting in the foreground queue, never being processed. - -## Architecture Overview - -### Current Executor Setup - -``` -┌─────────────────────────────────────────────────────────────┐ -│ VisualTestAppContext │ -├─────────────────────────────────────────────────────────────┤ -│ platform: Rc ←── MacPlatform (real) │ -│ background_executor: BackgroundExecutor │ -│ foreground_executor: ForegroundExecutor │ -└─────────────────────────────────────────────────────────────┘ - │ - ▼ -┌─────────────────────────────────────────────────────────────┐ -│ MacPlatform │ -├─────────────────────────────────────────────────────────────┤ -│ dispatcher: Arc │ -│ - Uses real macOS run loop (CFRunLoop) │ -│ - as_test() returns None │ -└─────────────────────────────────────────────────────────────┘ -``` - -### Test Platform (for comparison) - -``` -┌─────────────────────────────────────────────────────────────┐ -│ TestAppContext │ -├─────────────────────────────────────────────────────────────┤ -│ platform: TestPlatform │ -│ dispatcher: TestDispatcher │ -│ - Controllable task queue │ -│ - as_test() returns Some(self) │ -│ - run_until_parked() drives both bg and fg tasks │ -└─────────────────────────────────────────────────────────────┘ -``` - -## Key Files to Investigate - -### 1. Platform Dispatcher Trait -**File:** `crates/gpui/src/platform.rs` - -```rust -pub trait PlatformDispatcher: Send + Sync { - fn is_main_thread(&self) -> bool; - fn dispatch(&self, runnable: Runnable, label: Option); - fn dispatch_on_main_thread(&self, runnable: Runnable); - fn dispatch_after(&self, duration: Duration, runnable: Runnable); - fn park(&self, timeout: Option) -> bool; - fn unparker(&self) -> Unparker; - - #[cfg(any(test, feature = "test-support"))] - fn as_test(&self) -> Option<&TestDispatcher> { - None // Default implementation - real platforms return None - } -} -``` - -### 2. Test Dispatcher Implementation -**File:** `crates/gpui/src/platform/test/dispatcher.rs` - -This is the reference implementation for a controllable dispatcher: - -```rust -pub struct TestDispatcher { - state: Mutex, - // ... -} - -struct TestDispatcherState { - random: StdRng, - foreground: Vec, // ← Foreground task queue - background: Vec, // ← Background task queue - on_parking: Vec, - waiting_hint: Option, - block_on_ticks: RangeInclusive, - forbid_parking: bool, -} - -impl TestDispatcher { - pub fn run_until_parked(&self) { - // Drives BOTH foreground and background tasks - while self.poll(true) {} - } - - fn poll(&self, background_only: bool) -> bool { - // Randomly selects and runs tasks from both queues - } -} -``` - -### 3. Mac Dispatcher (Current Visual Test Dispatcher) -**File:** `crates/gpui/src/platform/mac/dispatcher.rs` - -```rust -pub(crate) struct MacDispatcher; - -impl PlatformDispatcher for MacDispatcher { - fn dispatch_on_main_thread(&self, runnable: Runnable) { - // Dispatches to actual macOS main thread via CFRunLoop - unsafe { - dispatch_async_f( - dispatch_get_main_queue(), - // ... - ); - } - } - - // as_test() returns None (default) -} -``` - -### 4. Visual Test Context Creation -**File:** `crates/gpui/src/app/visual_test_context.rs` - -```rust -impl VisualTestAppContext { - pub fn new() -> Self { - let platform = current_platform(false); // ← Gets MacPlatform - let background_executor = platform.background_executor(); - let foreground_executor = platform.foreground_executor(); - // ... - } - - pub fn run_until_parked(&self) { - self.background_executor.run_until_parked(); // ← Only bg! - } -} -``` - -### 5. BackgroundExecutor::run_until_parked -**File:** `crates/gpui/src/executor.rs` - -```rust -impl BackgroundExecutor { - pub fn run_until_parked(&self) { - self.dispatcher.as_test().unwrap().run_until_parked() - // ^^^^^^^^^^^^^^^^ - // This panics for MacDispatcher because as_test() returns None! - } -} -``` - -Wait - this means `run_until_parked()` should be panicking in visual tests. Let me check if it's actually being called... - -Actually, looking at the test output, it seems like it doesn't panic. This needs investigation - perhaps there's a different code path or the method isn't being called. - -## Proposed Solution - -### Option A: Hybrid Visual Test Platform (Recommended) - -Create a new platform type that: -- Uses **real Mac rendering** (Metal, text system, etc.) for actual pixel output -- Uses **TestDispatcher** for controllable task execution - -```rust -pub struct VisualTestPlatform { - // Real Mac components for rendering - mac_text_system: Arc, - mac_renderer: MacRenderer, // Or access via TestWindow with real backing - - // Test dispatcher for controllable execution - dispatcher: Arc, -} -``` - -**Pros:** -- Full control over task execution timing -- Deterministic test behavior -- Can still capture real rendered output - -**Cons:** -- Significant implementation effort -- Need to carefully separate "rendering" from "event loop" concerns -- May need to refactor how windows get their renderer - -### Option B: Foreground Task Pumping in Visual Tests - -Add a mechanism to manually pump foreground tasks in the visual test context: - -```rust -impl VisualTestAppContext { - pub fn pump_foreground_tasks(&mut self, duration: Duration) { - let deadline = Instant::now() + duration; - while Instant::now() < deadline { - // Manually poll the foreground task queue - if !self.platform.pump_one_foreground_task() { - std::thread::sleep(Duration::from_millis(1)); - } - } - } -} -``` - -This would require adding a new method to `PlatformDispatcher`: - -```rust -pub trait PlatformDispatcher: Send + Sync { - // Existing methods... - - /// Attempt to run one foreground task if available. - /// Returns true if a task was run, false if queue was empty. - fn pump_one_foreground_task(&self) -> bool { false } -} -``` - -**Pros:** -- Smaller change -- Doesn't require new platform type - -**Cons:** -- Less deterministic (still time-based) -- May not work well with macOS's dispatch queue semantics - -### Option C: Window-Spawned Task Interception - -Intercept tasks spawned via `window.spawn()` and redirect them to a controllable queue: - -```rust -impl Window { - pub fn spawn(&mut self, cx: &mut App, f: impl Future) -> Task { - #[cfg(any(test, feature = "test-support"))] - if cx.is_visual_test_mode() { - return self.spawn_to_test_queue(f); - } - - // Normal spawn path - self.foreground_executor.spawn(f) - } -} -``` - -**Pros:** -- Targeted fix for the specific problem -- Minimal changes to existing code - -**Cons:** -- Doesn't solve the general problem -- Test-specific code paths can diverge from production behavior - -## Implementation Plan for Option A - -### Step 1: Create TestDispatcher with Real Timing Option - -Modify `TestDispatcher` to support real-time delays instead of simulated time: - -```rust -pub struct TestDispatcher { - state: Mutex, - use_real_time: bool, // New flag -} - -impl TestDispatcher { - pub fn new_with_real_time() -> Self { - Self { - state: Mutex::new(TestDispatcherState::new()), - use_real_time: true, - } - } -} -``` - -### Step 2: Create VisualTestPlatform - -**New file:** `crates/gpui/src/platform/visual_test/mod.rs` - -```rust -pub struct VisualTestPlatform { - dispatcher: Arc, - text_system: Arc, - // Other Mac components needed for rendering -} - -impl Platform for VisualTestPlatform { - fn dispatcher(&self) -> Arc { - self.dispatcher.clone() - } - - fn text_system(&self) -> Arc { - self.text_system.clone() - } - - fn open_window(...) -> ... { - // Create window with real Metal rendering - // but using our test dispatcher - } -} -``` - -### Step 3: Create VisualTestWindow - -The window needs to use real rendering but the test dispatcher: - -```rust -pub struct VisualTestWindow { - // Real Metal/rendering components from MacWindow - renderer: Renderer, - native_view: ..., - - // But dispatches through TestDispatcher - dispatcher: Arc, -} -``` - -### Step 4: Update VisualTestAppContext - -```rust -impl VisualTestAppContext { - pub fn new() -> Self { - let dispatcher = Arc::new(TestDispatcher::new_with_real_time()); - let platform = Arc::new(VisualTestPlatform::new(dispatcher.clone())); - // ... - } - - pub fn run_until_parked(&self) { - // Now this works because we have a TestDispatcher - self.dispatcher.run_until_parked(); - } -} -``` - -### Step 5: Test the Tooltip Capture - -```rust -// In visual_test_runner.rs -cx.simulate_mouse_move(window, button_position, None, Modifiers::default()); - -// Wait real time for the tooltip delay -cx.background_executor() - .timer(Duration::from_millis(600)) - .await; - -// Drive all pending tasks including the tooltip show task -cx.run_until_parked(); - -// Now the tooltip should be visible -cx.update_window(window, |_, window, _| window.refresh())?; - -// Capture screenshot with tooltip -let screenshot = capture_screenshot(window, cx)?; -``` - -## Testing the Fix - -After implementation, these scenarios should work: - -1. **Tooltip on hover**: Mouse over button → wait → tooltip appears in screenshot -2. **Hover styles**: Mouse over element → hover style visible in screenshot -3. **Delayed animations**: Any animation triggered by foreground tasks -4. **Debounced updates**: UI updates that use debouncing/throttling - -## Files to Modify - -1. `crates/gpui/src/platform.rs` - May need new trait methods -2. `crates/gpui/src/platform/test/dispatcher.rs` - Add real-time mode -3. `crates/gpui/src/platform/visual_test/mod.rs` - New file -4. `crates/gpui/src/platform/visual_test/platform.rs` - New file -5. `crates/gpui/src/platform/visual_test/window.rs` - New file -6. `crates/gpui/src/app/visual_test_context.rs` - Use new platform -7. `crates/gpui/src/platform/mod.rs` - Export new module -8. `crates/zed/src/visual_test_runner.rs` - Update test code - -## Questions to Resolve - -1. **Can Metal rendering work without the macOS run loop?** - - Need to investigate if `CAMetalLayer` and friends require the run loop - -2. **How does `render_to_image()` work?** - - This is used for screenshot capture - need to ensure it works with test platform - -3. **What about system events (keyboard, mouse)?** - - Visual tests simulate these - should work with test dispatcher - -4. **Thread safety concerns?** - - TestDispatcher is designed for single-threaded use - - Metal rendering may have threading requirements - -## Related Code References - -- `Window::spawn` - `crates/gpui/src/window.rs` -- `register_tooltip_mouse_handlers` - `crates/gpui/src/elements/div.rs:2845` -- `handle_tooltip_mouse_move` - `crates/gpui/src/elements/div.rs:2873` -- `TOOLTIP_SHOW_DELAY` - `crates/gpui/src/elements/div.rs:48` (500ms) -- `TestWindow` - `crates/gpui/src/platform/test/window.rs` -- `MacWindow` - `crates/gpui/src/platform/mac/window.rs` - -## Success Criteria - -1. `diff_review_button_tooltip.png` shows the "Add Review" tooltip -2. Button shows hover border when mouse is over it -3. Tests remain deterministic (same output every run) -4. No reliance on wall-clock timing for correctness - ---- - -## Implementation Summary - -### Changes Made - -1. **Created `VisualTestPlatform`** (`crates/gpui/src/platform/visual_test.rs`) - - A hybrid platform that combines real Mac rendering with controllable `TestDispatcher` - - Uses `MacPlatform` for window creation, text system, rendering, and display management - - Uses `TestDispatcher` for deterministic task scheduling - - Implements the `Platform` trait, delegating rendering operations to `MacPlatform` - - Passes its own `ForegroundExecutor` (from `TestDispatcher`) to `MacWindow::open()` - -2. **Added `renderer_context()` method to `MacPlatform`** (`crates/gpui/src/platform/mac/platform.rs`) - - Allows `VisualTestPlatform` to access the renderer context for window creation - - Conditionally compiled for test-support - -3. **Updated `VisualTestAppContext`** (`crates/gpui/src/app/visual_test_context.rs`) - - Now creates and uses `VisualTestPlatform` instead of `current_platform()` - - Gets dispatcher and executors from the platform - - This ensures `App::spawn()` and `Window::spawn()` use the `TestDispatcher` - -4. **Added tests** (`crates/gpui/src/app/visual_test_context.rs`) - - `test_foreground_tasks_run_with_run_until_parked` - verifies foreground tasks execute - - `test_advance_clock_triggers_delayed_tasks` - verifies timer-based tasks work - - `test_window_spawn_uses_test_dispatcher` - verifies window.spawn uses TestDispatcher - - All tests are marked `#[ignore]` because they require macOS main thread - -### How It Works - -The key insight was that `App::new_app()` gets its executors from `platform.foreground_executor()`. Previously: - -``` -VisualTestAppContext - └── creates TestDispatcher (unused!) - └── creates App with MacPlatform - └── MacPlatform has MacDispatcher - └── App uses MacDispatcher's executors ❌ -``` - -After the fix: - -``` -VisualTestAppContext - └── creates VisualTestPlatform - └── Has TestDispatcher - └── Has MacPlatform (for rendering) - └── foreground_executor() returns TestDispatcher's executor ✓ - └── creates App with VisualTestPlatform - └── App uses TestDispatcher's executors ✓ - └── Window::spawn() uses TestDispatcher ✓ -``` - -### Running Visual Tests - -Visual tests require the macOS main thread. Run them with: - -```bash -cargo test -p gpui visual_test_context -- --ignored --test-threads=1 -cargo test -p zed visual_tests -- --ignored --test-threads=1 -``` - -### Tooltip Testing - -With this fix, tooltip testing now works: - -```rust -// Simulate hovering over a button -cx.simulate_mouse_move(window, button_position, None, Modifiers::default()); - -// Advance clock past TOOLTIP_SHOW_DELAY (500ms) -cx.advance_clock(Duration::from_millis(600)); - -// The tooltip task spawned via window.spawn() is now executed! -// Take screenshot - tooltip will be visible -let screenshot = cx.capture_screenshot(window)?; -``` \ No newline at end of file diff --git a/typos.toml b/typos.toml index 37a7a37a43e891661ec885d1be21ea2f3b364d67..b8dc55b8e53a066933a7c8b70bf521b663c16cba 100644 --- a/typos.toml +++ b/typos.toml @@ -63,6 +63,8 @@ extend-exclude = [ [default] extend-ignore-re = [ + # PNG is a file format, not a typo + "PNG", 'cl\[ist]', '\[lan\]guage', '"ba"',