From 2c5d8f9889a0ce3b54d2910bb28164ffb1523271 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 a8b8e4305a48178965a386c65a58be3cca34821b..876943f98bc6265e312cb0a664268599e6322000 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::{ @@ -587,33 +587,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 1c3318b08964c0364a948698703058c4243042f2..2fbb66444f59c296dcc135feec1837f000e364b8 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(), @@ -1043,6 +1041,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 @@ -20061,7 +20069,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()?; @@ -20138,7 +20145,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, }) } @@ -20343,7 +20350,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 f7540f3ff9c7f885d89281b36ae5081e4914c8e1..37178601d7292b74047eec02f19d4a5204c428a2 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 { @@ -6831,28 +6831,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); @@ -6888,13 +6873,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; @@ -8745,7 +8741,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;