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 2fd3c0653801074f6112569deb2f6cfb79ed6cbb..9e5a6329bf3099f64c074acfa1483a0aa9836018 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 6140731579455a9a98887bf524a5372edb5bad24..0c899cd4214b8120b6d7f0f1a9ae66417676bef7 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 + }; + + cx.dispatch_action(UpdateGoToDefinitionLink { + point, + cmd_held: cmd, + shift_held: shift, + }); - if view.is_selecting() { + 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 6eb38df75818050d342420e42b343ea37cb8b6ad..79b2e950cb477798addd8ae26709c39dc2fa74b7 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 | 94 +++++++++++++++++-------------- 2 files changed, 53 insertions(+), 43 deletions(-) diff --git a/crates/gpui/src/platform/event.rs b/crates/gpui/src/platform/event.rs index 6ac75926be426e2a7512d35ea9c06cd5c7f64047..9a84b8ef2f26a1e67488f3548263037b18be4d28 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 023e013d1393428752cd43ea45fc2f0c61fc101a..3e6c97a2087a4b45d5bd0f9890832cb3a4935084 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; - } + + 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); + } + 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) { + 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 79b2e950cb477798addd8ae26709c39dc2fa74b7..6af985171a87cd9d678d46a8fc7504b93199ba54 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 5f6528db1c32b816694859c0e5e13b028207f23a..43e50829f55ae47299abbe41ad33066dcc3be653 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