mirror of
https://github.com/zed-industries/zed.git
synced 2026-07-09 16:00:35 +00:00
Add diff review button in git diff views (#46326)
Adds a very simple diff review button behind a feature flag, which appears in the gutter of people who have the flag (currently just me; it's not even enabled for staff, since it doesn't do anything yet). Mostly this PR just adds the feature flag, the button, and tests. Release Notes: - N/A
This commit is contained in:
Richard Feldman
GitHub
parent
756637fef5
commit
3b67868675
8 changed files with 336 additions and 524 deletions
|
|
@ -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<bool>,
|
||||
show_runnables: Option<bool>,
|
||||
show_breakpoints: Option<bool>,
|
||||
show_diff_review_button: bool,
|
||||
show_wrap_guides: Option<bool>,
|
||||
show_indent_guides: Option<bool>,
|
||||
buffers_with_disabled_indent_guides: HashSet<BufferId>,
|
||||
|
|
@ -1183,6 +1194,7 @@ pub struct Editor {
|
|||
tasks_update_task: Option<Task<()>>,
|
||||
breakpoint_store: Option<Entity<BreakpointStore>>,
|
||||
gutter_breakpoint_indicator: (Option<PhantomBreakpointIndicator>, Option<Task<()>>),
|
||||
pub(crate) gutter_diff_review_indicator: (Option<PhantomDiffReviewIndicator>, Option<Task<()>>),
|
||||
hovered_diff_hunk_row: Option<DisplayRow>,
|
||||
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>) {
|
||||
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<Self>) {
|
||||
if self.display_map.read(cx).masked != masked {
|
||||
self.display_map.update(cx, |map, _| map.masked = masked);
|
||||
|
|
|
|||
|
|
@ -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::<Editor>()
|
||||
.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::<Editor>()
|
||||
.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::<Editor>()
|
||||
.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"
|
||||
);
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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::<DiffReviewFeatureFlag>()
|
||||
&& !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<DisplayRow>,
|
||||
row_infos: &[RowInfo],
|
||||
cx: &App,
|
||||
) -> Option<DisplayRow> {
|
||||
if !cx.has_flag::<DiffReviewFeatureFlag>() {
|
||||
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<SelectionLayout>)>,
|
||||
test_indicators: Vec<AnyElement>,
|
||||
breakpoints: Vec<AnyElement>,
|
||||
diff_review_button: Option<AnyElement>,
|
||||
crease_toggles: Vec<Option<AnyElement>>,
|
||||
expand_toggles: Vec<Option<(AnyElement, gpui::Point<Pixels>)>>,
|
||||
diff_hunk_controls: Vec<AnyElement>,
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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 => {
|
||||
|
|
|
|||
|
|
@ -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<dyn Platform> ←── MacPlatform (real) │
|
||||
│ background_executor: BackgroundExecutor │
|
||||
│ foreground_executor: ForegroundExecutor │
|
||||
└─────────────────────────────────────────────────────────────┘
|
||||
│
|
||||
▼
|
||||
┌─────────────────────────────────────────────────────────────┐
|
||||
│ MacPlatform │
|
||||
├─────────────────────────────────────────────────────────────┤
|
||||
│ dispatcher: Arc<MacDispatcher> │
|
||||
│ - 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<TaskLabel>);
|
||||
fn dispatch_on_main_thread(&self, runnable: Runnable);
|
||||
fn dispatch_after(&self, duration: Duration, runnable: Runnable);
|
||||
fn park(&self, timeout: Option<Duration>) -> 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<TestDispatcherState>,
|
||||
// ...
|
||||
}
|
||||
|
||||
struct TestDispatcherState {
|
||||
random: StdRng,
|
||||
foreground: Vec<TestTask>, // ← Foreground task queue
|
||||
background: Vec<TestTask>, // ← Background task queue
|
||||
on_parking: Vec<UnparkCallback>,
|
||||
waiting_hint: Option<String>,
|
||||
block_on_ticks: RangeInclusive<usize>,
|
||||
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<MacTextSystem>,
|
||||
mac_renderer: MacRenderer, // Or access via TestWindow with real backing
|
||||
|
||||
// Test dispatcher for controllable execution
|
||||
dispatcher: Arc<TestDispatcher>,
|
||||
}
|
||||
```
|
||||
|
||||
**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<R>(&mut self, cx: &mut App, f: impl Future<Output = R>) -> Task<R> {
|
||||
#[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<TestDispatcherState>,
|
||||
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<TestDispatcher>,
|
||||
text_system: Arc<MacTextSystem>,
|
||||
// Other Mac components needed for rendering
|
||||
}
|
||||
|
||||
impl Platform for VisualTestPlatform {
|
||||
fn dispatcher(&self) -> Arc<dyn PlatformDispatcher> {
|
||||
self.dispatcher.clone()
|
||||
}
|
||||
|
||||
fn text_system(&self) -> Arc<dyn PlatformTextSystem> {
|
||||
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<TestDispatcher>,
|
||||
}
|
||||
```
|
||||
|
||||
### 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)?;
|
||||
```
|
||||
|
|
@ -63,6 +63,8 @@ extend-exclude = [
|
|||
|
||||
[default]
|
||||
extend-ignore-re = [
|
||||
# PNG is a file format, not a typo
|
||||
"PNG",
|
||||
'cl\[ist]',
|
||||
'\[lan\]guage',
|
||||
'"ba"',
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue