From 358c324e2639bc380703aadfef282330f8809d44 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Wed, 7 May 2025 20:23:54 +0200 Subject: [PATCH] editor: Use default gutter margin instead of `horizontal_padding` for horizontal content padding (#30138) This PR changes the way a horizontal margin is added in editors. It removes the possibility to set a custom `horizontal_padding` for an editor and utilizes the default `gutter_dimension` instead. This change is made to ensure that no issues with soft-wrapping occurs for any editor that has a `horizontal_margin` set (see #26893 for more context on the implications here`. Furthermore, it ensures that the text actually renders properly when scrolling horizontally and is not cut-off. ### Horizontal padding: | `main` | This PR | | --- | --- | | ![main padding](https://github.com/user-attachments/assets/4e7ea020-f92d-4f28-8cc1-89d0b0350683) | ![PR padding](https://github.com/user-attachments/assets/a05bae17-c384-431b-bb79-a1fffe7a29d7) | ### Editor horizontally scrolled: | `main` | This PR | | --- | --- | | ![main scrolled](https://github.com/user-attachments/assets/1a30156f-6c08-4cf9-94aa-9d087c0408cc) | ![pr scrolled](https://github.com/user-attachments/assets/d0daa72e-3b02-479b-aea0-41e1a376c567) | Notice the difference at the horizontal borders. The margin added for the `edit_file_tool` was 4 pixels. The `descent`, whilst not exactly, is roughly the same here and also scales with the font size nicely. Furthermore, it seems that the `gutter_dimensions.margin` should be present anyway, given the following comment https://github.com/zed-industries/zed/blob/0b00256f5884fd17b4f834730b31f365613f3683/crates/editor/src/element.rs#L6887-L6889 so ensuring this property is actually set and not 0 seems to be reasonable given the circumstances. Please note though that this will apply to all editors in the app. Again, this seems like it should be the case anyway, just wanted to mention this again. Should the fix like this not be wanted, I can change this here so that the `horizontal_margin` is better accounted for when soft-wrapping in an editor. Feel free to let me know in this case. Release Notes: - N/A --- crates/assistant_tools/src/edit_file_tool.rs | 41 ++++++------------ crates/editor/src/editor.rs | 16 ++++--- crates/editor/src/element.rs | 44 +++++++++----------- 3 files changed, 43 insertions(+), 58 deletions(-) diff --git a/crates/assistant_tools/src/edit_file_tool.rs b/crates/assistant_tools/src/edit_file_tool.rs index 29586f7d17..73a51b3d30 100644 --- a/crates/assistant_tools/src/edit_file_tool.rs +++ b/crates/assistant_tools/src/edit_file_tool.rs @@ -8,11 +8,11 @@ use assistant_tool::{ ActionLog, AnyToolCard, Tool, ToolCard, ToolResult, ToolResultOutput, ToolUseStatus, }; use buffer_diff::{BufferDiff, BufferDiffSnapshot}; -use editor::{Editor, EditorElement, EditorMode, EditorStyle, MultiBuffer, PathKey}; +use editor::{Editor, EditorMode, MultiBuffer, PathKey}; use futures::StreamExt; use gpui::{ Animation, AnimationExt, AnyWindowHandle, App, AppContext, AsyncApp, Entity, EntityId, Task, - TextStyle, WeakEntity, pulsating_between, + TextStyleRefinement, WeakEntity, pulsating_between, }; use indoc::formatdoc; use language::{ @@ -574,33 +574,16 @@ impl ToolCard for EditFileToolCard { .map(|style| style.text.line_height_in_pixels(window.rem_size())) .unwrap_or_default(); - let settings = ThemeSettings::get_global(cx); - let element = EditorElement::new( - &cx.entity(), - EditorStyle { - background: cx.theme().colors().editor_background, - horizontal_padding: rems(0.25).to_pixels(window.rem_size()), - local_player: cx.theme().players().local(), - text: TextStyle { - color: cx.theme().colors().editor_foreground, - font_family: settings.buffer_font.family.clone(), - font_features: settings.buffer_font.features.clone(), - font_fallbacks: settings.buffer_font.fallbacks.clone(), - font_size: TextSize::Small - .rems(cx) - .to_pixels(settings.agent_font_size(cx)) - .into(), - font_weight: settings.buffer_font.weight, - line_height: relative(settings.buffer_line_height.value()), - ..Default::default() - }, - scrollbar_width: EditorElement::SCROLLBAR_WIDTH, - syntax: cx.theme().syntax().clone(), - status: cx.theme().status().clone(), - ..Default::default() - }, - ); - + editor.set_text_style_refinement(TextStyleRefinement { + font_size: Some( + TextSize::Small + .rems(cx) + .to_pixels(ThemeSettings::get_global(cx).agent_font_size(cx)) + .into(), + ), + ..TextStyleRefinement::default() + }); + let element = editor.render(window, cx); (element.into_any_element(), line_height) }); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 06d86bb988..236bfa0441 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -517,7 +517,6 @@ pub enum SoftWrap { #[derive(Clone)] pub struct EditorStyle { pub background: Hsla, - pub horizontal_padding: Pixels, pub local_player: PlayerColor, pub text: TextStyle, pub scrollbar_width: Pixels, @@ -532,7 +531,6 @@ impl Default for EditorStyle { fn default() -> Self { Self { background: Hsla::default(), - horizontal_padding: Pixels::default(), local_player: PlayerColor::default(), text: TextStyle::default(), scrollbar_width: Pixels::default(), @@ -1042,6 +1040,16 @@ pub struct GutterDimensions { } impl GutterDimensions { + fn default_with_margin(font_id: FontId, font_size: Pixels, cx: &App) -> Self { + Self { + margin: Self::default_gutter_margin(font_id, font_size, cx), + ..Default::default() + } + } + + fn default_gutter_margin(font_id: FontId, font_size: Pixels, cx: &App) -> Pixels { + -cx.text_system().descent(font_id, font_size) + } /// The full width of the space taken up by the gutter. pub fn full_width(&self) -> Pixels { self.margin + self.width @@ -20006,7 +20014,6 @@ impl EditorSnapshot { return None; } - let descent = cx.text_system().descent(font_id, font_size); let em_width = cx.text_system().em_width(font_id, font_size).log_err()?; let em_advance = cx.text_system().em_advance(font_id, font_size).log_err()?; @@ -20079,7 +20086,7 @@ impl EditorSnapshot { left_padding, right_padding, width: line_gutter_width + left_padding + right_padding, - margin: -descent, + margin: GutterDimensions::default_gutter_margin(font_id, font_size, cx), git_blame_entries_width, }) } @@ -20284,7 +20291,6 @@ impl Render for Editor { &cx.entity(), EditorStyle { background, - horizontal_padding: Pixels::default(), local_player: cx.theme().players().local(), text: text_style, scrollbar_width: EditorElement::SCROLLBAR_WIDTH, diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 9188a8a5a3..7d6e1ea9fd 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -170,7 +170,7 @@ pub struct EditorElement { type DisplayRowDelta = u32; impl EditorElement { - pub const SCROLLBAR_WIDTH: Pixels = px(15.); + pub(crate) const SCROLLBAR_WIDTH: Pixels = px(15.); pub fn new(editor: &Entity, style: EditorStyle) -> Self { Self { @@ -6781,28 +6781,13 @@ impl Element for EditorElement { self.max_line_number_width(&snapshot, window, cx), cx, ) - .unwrap_or_default(); - let hitbox = window.insert_hitbox(bounds, false); - let gutter_hitbox = - window.insert_hitbox(gutter_bounds(bounds, gutter_dimensions), false); - let text_hitbox = window.insert_hitbox( - Bounds { - origin: gutter_hitbox.top_right() - + point(style.horizontal_padding, Pixels::default()), - size: size( - bounds.size.width - - gutter_dimensions.width - - 2. * style.horizontal_padding, - bounds.size.height, - ), - }, - false, - ); + .unwrap_or_else(|| { + GutterDimensions::default_with_margin(font_id, font_size, cx) + }); + let text_width = bounds.size.width - gutter_dimensions.width; - let editor_width = text_hitbox.size.width - - gutter_dimensions.margin - - em_width - - style.scrollbar_width; + let editor_width = + text_width - gutter_dimensions.margin - em_width - style.scrollbar_width; snapshot = self.editor.update(cx, |editor, cx| { editor.last_bounds = Some(bounds); @@ -6838,13 +6823,24 @@ impl Element for EditorElement { .map(|(guide, active)| (self.column_pixels(*guide, window, cx), *active)) .collect::>(); + let hitbox = window.insert_hitbox(bounds, false); + let gutter_hitbox = + window.insert_hitbox(gutter_bounds(bounds, gutter_dimensions), false); + let text_hitbox = window.insert_hitbox( + Bounds { + origin: gutter_hitbox.top_right(), + size: size(text_width, bounds.size.height), + }, + false, + ); + // Offset the content_bounds from the text_bounds by the gutter margin (which // is roughly half a character wide) to make hit testing work more like how we want. let content_offset = point(gutter_dimensions.margin, Pixels::ZERO); let content_origin = text_hitbox.origin + content_offset; let editor_text_bounds = - Bounds::from_corners(content_origin, text_hitbox.bounds.bottom_right()); + Bounds::from_corners(content_origin, bounds.bottom_right()); let height_in_lines = editor_text_bounds.size.height / line_height; @@ -8646,7 +8642,7 @@ fn compute_auto_height_layout( let mut snapshot = editor.snapshot(window, cx); let gutter_dimensions = snapshot .gutter_dimensions(font_id, font_size, max_line_number_width, cx) - .unwrap_or_default(); + .unwrap_or_else(|| GutterDimensions::default_with_margin(font_id, font_size, cx)); editor.gutter_dimensions = gutter_dimensions; let text_width = width - gutter_dimensions.width;