From b61e9a940ea560b0d99da3737e53ec086cf7de6d Mon Sep 17 00:00:00 2001 From: ForLoveOfCats Date: Thu, 18 Aug 2022 11:46:11 -0400 Subject: [PATCH 1/3] Avoid triggering goto-definition links while with a pending selection Co-Authored-By: Antonio Scandurra --- crates/editor/src/editor.rs | 12 ++--- crates/editor/src/element.rs | 50 +++++++++++++---- crates/editor/src/link_go_to_definition.rs | 63 +++++++++++++--------- 3 files changed, 84 insertions(+), 41 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 2fd3c06538..9e5a6329bf 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -1789,15 +1789,15 @@ impl Editor { cx.notify(); } - pub fn are_selections_empty(&self) -> bool { - let pending_empty = match self.selections.pending_anchor() { - Some(Selection { start, end, .. }) => start == end, - None => true, + pub fn has_pending_nonempty_selection(&self) -> bool { + let pending_nonempty_selection = match self.selections.pending_anchor() { + Some(Selection { start, end, .. }) => start != end, + None => false, }; - pending_empty && self.columnar_selection_tail.is_none() + pending_nonempty_selection || self.columnar_selection_tail.is_some() } - pub fn is_selecting(&self) -> bool { + pub fn has_pending_selection(&self) -> bool { self.selections.pending_anchor().is_some() || self.columnar_selection_tail.is_some() } diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 6140731579..0c899cd421 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -179,14 +179,14 @@ impl EditorElement { cx: &mut EventContext, ) -> bool { let view = self.view(cx.app.as_ref()); - let end_selection = view.is_selecting(); - let selections_empty = view.are_selections_empty(); + let end_selection = view.has_pending_selection(); + let pending_nonempty_selections = view.has_pending_nonempty_selection(); if end_selection { cx.dispatch_action(Select(SelectPhase::End)); } - if selections_empty && cmd && paint.text_bounds.contains_point(position) { + if !pending_nonempty_selections && cmd && paint.text_bounds.contains_point(position) { let (point, target_point) = paint.point_for_position(&self.snapshot(cx), layout, position); @@ -206,14 +206,38 @@ impl EditorElement { fn mouse_dragged( &self, - position: Vector2F, + MouseMovedEvent { + cmd, + shift, + position, + .. + }: MouseMovedEvent, layout: &mut LayoutState, paint: &mut PaintState, cx: &mut EventContext, ) -> bool { - let view = self.view(cx.app.as_ref()); + // This will be handled more correctly once https://github.com/zed-industries/zed/issues/1218 is completed + // Don't trigger hover popover if mouse is hovering over context menu + let point = if paint.text_bounds.contains_point(position) { + let (point, target_point) = + paint.point_for_position(&self.snapshot(cx), layout, position); + if point == target_point { + Some(point) + } else { + None + } + } else { + None + }; - if view.is_selecting() { + cx.dispatch_action(UpdateGoToDefinitionLink { + point, + cmd_held: cmd, + shift_held: shift, + }); + + let view = self.view(cx.app); + if view.has_pending_selection() { let rect = paint.text_bounds; let mut scroll_delta = Vector2F::zero(); @@ -250,8 +274,11 @@ impl EditorElement { scroll_position: (snapshot.scroll_position() + scroll_delta) .clamp(Vector2F::zero(), layout.scroll_max), })); + + cx.dispatch_action(HoverAt { point }); true } else { + cx.dispatch_action(HoverAt { point }); false } } @@ -1572,11 +1599,12 @@ impl Element for EditorElement { .. }) => self.mouse_up(position, cmd, shift, layout, paint, cx), - Event::MouseMoved(MouseMovedEvent { - pressed_button: Some(MouseButton::Left), - position, - .. - }) => self.mouse_dragged(*position, layout, paint, cx), + Event::MouseMoved( + event @ MouseMovedEvent { + pressed_button: Some(MouseButton::Left), + .. + }, + ) => self.mouse_dragged(*event, layout, paint, cx), Event::ScrollWheel(ScrollWheelEvent { position, diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index 6eb38df758..79b2e950cb 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -70,6 +70,8 @@ pub fn update_go_to_definition_link( }: &UpdateGoToDefinitionLink, cx: &mut ViewContext, ) { + let pending_nonempty_selection = editor.has_pending_nonempty_selection(); + // Store new mouse point as an anchor let snapshot = editor.snapshot(cx); let point = point.map(|point| { @@ -89,6 +91,12 @@ pub fn update_go_to_definition_link( } editor.link_go_to_definition_state.last_mouse_location = point.clone(); + + if pending_nonempty_selection { + hide_link_definition(editor, cx); + return; + } + if cmd_held { if let Some(point) = point { let kind = if shift_held { @@ -113,12 +121,14 @@ pub fn cmd_shift_changed( }: &CmdShiftChanged, cx: &mut ViewContext, ) { + let pending_nonempty_selection = editor.has_pending_nonempty_selection(); + if let Some(point) = editor .link_go_to_definition_state .last_mouse_location .clone() { - if cmd_down { + if cmd_down && !pending_nonempty_selection { let snapshot = editor.snapshot(cx); let kind = if shift_down { LinkDefinitionKind::Type @@ -127,10 +137,11 @@ pub fn cmd_shift_changed( }; show_link_definition(kind, editor, point, snapshot, cx); - } else { - hide_link_definition(editor, cx) + return; } } + + hide_link_definition(editor, cx) } #[derive(Debug, Clone, Copy, PartialEq)] @@ -243,28 +254,32 @@ pub fn show_link_definition( this.link_go_to_definition_state.definitions = definitions.clone(); let buffer_snapshot = buffer.read(cx).snapshot(); + // Only show highlight if there exists a definition to jump to that doesn't contain // the current location. - if definitions.iter().any(|definition| { - let target = &definition.target; - if target.buffer == buffer { - let range = &target.range; - // Expand range by one character as lsp definition ranges include positions adjacent - // but not contained by the symbol range - let start = buffer_snapshot.clip_offset( - range.start.to_offset(&buffer_snapshot).saturating_sub(1), - Bias::Left, - ); - let end = buffer_snapshot.clip_offset( - range.end.to_offset(&buffer_snapshot) + 1, - Bias::Right, - ); - let offset = buffer_position.to_offset(&buffer_snapshot); - !(start <= offset && end >= offset) - } else { - true - } - }) { + let any_definition_does_not_contain_current_location = + definitions.iter().any(|definition| { + let target = &definition.target; + if target.buffer == buffer { + let range = &target.range; + // Expand range by one character as lsp definition ranges include positions adjacent + // but not contained by the symbol range + let start = buffer_snapshot.clip_offset( + range.start.to_offset(&buffer_snapshot).saturating_sub(1), + Bias::Left, + ); + let end = buffer_snapshot.clip_offset( + range.end.to_offset(&buffer_snapshot) + 1, + Bias::Right, + ); + let offset = buffer_position.to_offset(&buffer_snapshot); + !(start <= offset && end >= offset) + } else { + true + } + }); + + if any_definition_does_not_contain_current_location { // If no symbol range returned from language server, use the surrounding word. let highlight_range = symbol_range.unwrap_or_else(|| { let snapshot = &snapshot.buffer_snapshot; @@ -280,7 +295,7 @@ pub fn show_link_definition( vec![highlight_range], style, cx, - ) + ); } else { hide_link_definition(this, cx); } From b8b951deabaa3c02c96b0d60833abb944cca7001 Mon Sep 17 00:00:00 2001 From: ForLoveOfCats Date: Wed, 17 Aug 2022 18:11:54 -0400 Subject: [PATCH 2/3] Clear last-mouse-moved pressed button when that button gets a mouse-up This fixes an annoying issue where if the last mouse moved event was during a drag it would never trigger mouse cursor changes until next mouse move reset it. It makes sense to continue to not change the cursor while the button is pressed so instead this tracks when the mouse button is released in order to update the mouse move event --- crates/gpui/src/platform/event.rs | 2 +- crates/gpui/src/presenter.rs | 96 +++++++++++++++++-------------- 2 files changed, 54 insertions(+), 44 deletions(-) diff --git a/crates/gpui/src/platform/event.rs b/crates/gpui/src/platform/event.rs index 6ac75926be..9a84b8ef2f 100644 --- a/crates/gpui/src/platform/event.rs +++ b/crates/gpui/src/platform/event.rs @@ -64,7 +64,7 @@ impl Default for MouseButton { } } -#[derive(Clone, Debug, Default)] +#[derive(Clone, Copy, Debug, Default)] pub struct MouseButtonEvent { pub button: MouseButton, pub position: Vector2F, diff --git a/crates/gpui/src/presenter.rs b/crates/gpui/src/presenter.rs index 023e013d13..3e6c97a208 100644 --- a/crates/gpui/src/presenter.rs +++ b/crates/gpui/src/presenter.rs @@ -31,7 +31,7 @@ pub struct Presenter { font_cache: Arc, text_layout_cache: TextLayoutCache, asset_cache: Arc, - last_mouse_moved_event: Option, + last_mouse_moved_event: Option, hovered_region_ids: HashSet, clicked_region: Option, right_clicked_region: Option, @@ -268,15 +268,27 @@ impl Presenter { } } } - Event::MouseUp(e @ MouseButtonEvent { position, .. }) => { + + &Event::MouseUp( + e @ MouseButtonEvent { + position, button, .. + }, + ) => { self.prev_drag_position.take(); if let Some(region) = self.clicked_region.take() { invalidated_views.push(region.view_id); - if region.bounds.contains_point(*position) { + if region.bounds.contains_point(position) { clicked_region = Some((region, MouseRegionEvent::Click(e.clone()))); } } + + if let Some(moved) = &mut self.last_mouse_moved_event { + if moved.pressed_button == Some(button) { + moved.pressed_button = None; + } + } } + Event::MouseMoved(e @ MouseMovedEvent { position, .. }) => { if let Some((clicked_region, prev_drag_position)) = self .clicked_region @@ -290,13 +302,17 @@ impl Presenter { *prev_drag_position = *position; } - self.last_mouse_moved_event = Some(event.clone()); + self.last_mouse_moved_event = Some(e.clone()); } + _ => {} } - let (mut handled, mut event_cx) = - self.handle_hover_events(&event, &mut invalidated_views, cx); + let (mut handled, mut event_cx) = if let Event::MouseMoved(e) = &event { + self.handle_hover_events(e, &mut invalidated_views, cx) + } else { + (false, self.build_event_context(cx)) + }; for (handler, view_id, region_event) in mouse_down_out_handlers { event_cx.with_current_view(view_id, |event_cx| handler(region_event, event_cx)) @@ -353,51 +369,45 @@ impl Presenter { fn handle_hover_events<'a>( &'a mut self, - event: &Event, + e @ MouseMovedEvent { + position, + pressed_button, + .. + }: &MouseMovedEvent, invalidated_views: &mut Vec, cx: &'a mut MutableAppContext, ) -> (bool, EventContext<'a>) { let mut hover_regions = Vec::new(); - if let Event::MouseMoved( - e @ MouseMovedEvent { - position, - pressed_button, - .. - }, - ) = event - { - if pressed_button.is_none() { - let mut style_to_assign = CursorStyle::Arrow; - for region in self.cursor_regions.iter().rev() { - if region.bounds.contains_point(*position) { - style_to_assign = region.style; - break; - } - } - cx.platform().set_cursor_style(style_to_assign); - let mut hover_depth = None; - for (region, depth) in self.mouse_regions.iter().rev() { - if region.bounds.contains_point(*position) - && hover_depth.map_or(true, |hover_depth| hover_depth == *depth) - { - hover_depth = Some(*depth); - if let Some(region_id) = region.id() { - if !self.hovered_region_ids.contains(®ion_id) { - invalidated_views.push(region.view_id); - hover_regions - .push((region.clone(), MouseRegionEvent::Hover(true, *e))); - self.hovered_region_ids.insert(region_id); - } - } - } else if let Some(region_id) = region.id() { - if self.hovered_region_ids.contains(®ion_id) { + if pressed_button.is_none() { + let mut style_to_assign = CursorStyle::Arrow; + for region in self.cursor_regions.iter().rev() { + if region.bounds.contains_point(*position) { + style_to_assign = region.style; + break; + } + } + cx.platform().set_cursor_style(style_to_assign); + + let mut hover_depth = None; + for (region, depth) in self.mouse_regions.iter().rev() { + if region.bounds.contains_point(*position) + && hover_depth.map_or(true, |hover_depth| hover_depth == *depth) + { + hover_depth = Some(*depth); + if let Some(region_id) = region.id() { + if !self.hovered_region_ids.contains(®ion_id) { invalidated_views.push(region.view_id); - hover_regions - .push((region.clone(), MouseRegionEvent::Hover(false, *e))); - self.hovered_region_ids.remove(®ion_id); + hover_regions.push((region.clone(), MouseRegionEvent::Hover(true, *e))); + self.hovered_region_ids.insert(region_id); } } + } else if let Some(region_id) = region.id() { + if self.hovered_region_ids.contains(®ion_id) { + invalidated_views.push(region.view_id); + hover_regions.push((region.clone(), MouseRegionEvent::Hover(false, *e))); + self.hovered_region_ids.remove(®ion_id); + } } } } From 8583320e9bc33fbf0f093c704e9adf82acdd9aff Mon Sep 17 00:00:00 2001 From: ForLoveOfCats Date: Thu, 18 Aug 2022 18:33:37 -0400 Subject: [PATCH 3/3] Add test for pending selection influence on go-to links Co-authored-by: Max Brunsfeld --- crates/editor/src/link_go_to_definition.rs | 54 ++++++++++++++++++++++ crates/editor/src/test.rs | 2 +- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index 79b2e950cb..6af985171a 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -821,5 +821,59 @@ mod tests { fn test() { do_work(); } fn «do_workˇ»() { test(); } "}); + + // 1. We have a pending selection, mouse point is over a symbol that we have a response for, hitting cmd and nothing happens + // 2. Selection is completed, hovering + let hover_point = cx.display_point(indoc! {" + fn test() { do_wˇork(); } + fn do_work() { test(); } + "}); + let target_range = cx.lsp_range(indoc! {" + fn test() { do_work(); } + fn «do_work»() { test(); } + "}); + let mut requests = cx.handle_request::(move |url, _, _| async move { + Ok(Some(lsp::GotoDefinitionResponse::Link(vec![ + lsp::LocationLink { + origin_selection_range: None, + target_uri: url, + target_range, + target_selection_range: target_range, + }, + ]))) + }); + + // create a pending selection + let selection_range = cx.ranges(indoc! {" + fn «test() { do_w»ork(); } + fn do_work() { test(); } + "})[0] + .clone(); + cx.update_editor(|editor, cx| { + let snapshot = editor.buffer().read(cx).snapshot(cx); + let anchor_range = snapshot.anchor_before(selection_range.start) + ..snapshot.anchor_after(selection_range.end); + editor.change_selections(Some(crate::Autoscroll::Fit), cx, |s| { + s.set_pending_anchor_range(anchor_range, crate::SelectMode::Character) + }); + }); + cx.update_editor(|editor, cx| { + update_go_to_definition_link( + editor, + &UpdateGoToDefinitionLink { + point: Some(hover_point), + cmd_held: true, + shift_held: false, + }, + cx, + ); + }); + cx.foreground().run_until_parked(); + assert!(requests.try_next().is_err()); + cx.assert_editor_text_highlights::(indoc! {" + fn test() { do_work(); } + fn do_work() { test(); } + "}); + cx.foreground().run_until_parked(); } } diff --git a/crates/editor/src/test.rs b/crates/editor/src/test.rs index 5f6528db1c..43e50829f5 100644 --- a/crates/editor/src/test.rs +++ b/crates/editor/src/test.rs @@ -183,7 +183,7 @@ impl<'a> EditorTestContext<'a> { } } - fn ranges(&self, marked_text: &str) -> Vec> { + pub fn ranges(&self, marked_text: &str) -> Vec> { let (unmarked_text, ranges) = marked_text_ranges(marked_text, false); assert_eq!(self.buffer_text(), unmarked_text); ranges