Add hover test and tweak dismiss logic

Keith Simmons created

Change summary

crates/editor/src/editor.rs        | 191 +++++++++++++++++++++++++++++++
crates/editor/src/hover_popover.rs |  62 ++++++---
crates/editor/src/test.rs          | 168 +++++++++++++++++++++++++++
crates/gpui/src/executor.rs        |  13 ++
crates/language/src/language.rs    |   4 
5 files changed, 410 insertions(+), 28 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -6147,8 +6147,15 @@ pub fn styled_runs_for_code_label<'a>(
 
 #[cfg(test)]
 mod tests {
-    use crate::test::{
-        assert_text_with_selections, build_editor, select_ranges, EditorTestContext,
+    use crate::{
+        hover_popover::{
+            hover, hover_at, HoverAt, HOVER_DELAY_MILLIS, HOVER_GRACE_MILLIS,
+            HOVER_REQUEST_DELAY_MILLIS,
+        },
+        test::{
+            assert_text_with_selections, build_editor, select_ranges, EditorLspTestContext,
+            EditorTestContext,
+        },
     };
 
     use super::*;
@@ -6159,7 +6166,7 @@ mod tests {
     use indoc::indoc;
     use language::{FakeLspAdapter, LanguageConfig};
     use lsp::FakeLanguageServer;
-    use project::FakeFs;
+    use project::{FakeFs, HoverBlock};
     use settings::LanguageOverride;
     use smol::stream::StreamExt;
     use std::{cell::RefCell, rc::Rc, time::Instant};
@@ -9391,6 +9398,184 @@ mod tests {
         }
     }
 
+    #[gpui::test]
+    async fn test_hover_popover(cx: &mut gpui::TestAppContext) {
+        let mut cx = EditorLspTestContext::new_rust(
+            lsp::ServerCapabilities {
+                hover_provider: Some(lsp::HoverProviderCapability::Simple(true)),
+                ..Default::default()
+            },
+            cx,
+        )
+        .await;
+
+        // Basic hover delays and then pops without moving the mouse
+        cx.set_state(indoc! {"
+            fn |test()
+                println!();"});
+        let hover_point = cx.display_point(indoc! {"
+            fn test()
+                print|ln!();"});
+        cx.update_editor(|editor, cx| {
+            hover_at(
+                editor,
+                &HoverAt {
+                    point: Some(hover_point),
+                },
+                cx,
+            )
+        });
+        assert!(!cx.editor(|editor, _| editor.hover_state.visible()));
+
+        // After delay, hover should be visible.
+        let symbol_range = cx.lsp_range(indoc! {"
+            fn test()
+                [println!]();"});
+        cx.handle_request::<lsp::request::HoverRequest, _>(move |_| {
+            Some(lsp::Hover {
+                contents: lsp::HoverContents::Markup(lsp::MarkupContent {
+                    kind: lsp::MarkupKind::Markdown,
+                    value: indoc! {"
+                        # Some basic docs
+                        Some test documentation"}
+                    .to_string(),
+                }),
+                range: Some(symbol_range),
+            })
+        })
+        .await;
+        cx.foreground()
+            .advance_clock(Duration::from_millis(HOVER_DELAY_MILLIS + 100));
+
+        cx.editor(|editor, _| {
+            assert!(editor.hover_state.visible());
+            assert_eq!(
+                editor.hover_state.popover.clone().unwrap().contents,
+                vec![
+                    HoverBlock {
+                        text: "Some basic docs".to_string(),
+                        language: None
+                    },
+                    HoverBlock {
+                        text: "Some test documentation".to_string(),
+                        language: None
+                    }
+                ]
+            )
+        });
+
+        // Mouse moved with no hover response dismisses
+        let hover_point = cx.display_point(indoc! {"
+            fn te|st()
+                println!();"});
+        cx.update_editor(|editor, cx| {
+            hover_at(
+                editor,
+                &HoverAt {
+                    point: Some(hover_point),
+                },
+                cx,
+            )
+        });
+        cx.handle_request::<lsp::request::HoverRequest, _>(move |_| None)
+            .await;
+        cx.foreground().run_until_parked();
+        cx.editor(|editor, _| {
+            assert!(!editor.hover_state.visible());
+        });
+        cx.foreground()
+            .advance_clock(Duration::from_millis(HOVER_GRACE_MILLIS + 100));
+
+        // Hover with keyboard has no delay
+        cx.set_state(indoc! {"
+            f|n test()
+                println!();"});
+        cx.update_editor(|editor, cx| hover(editor, &hover_popover::Hover, cx));
+        let symbol_range = cx.lsp_range(indoc! {"
+            [fn] test()
+                println!();"});
+        cx.handle_request::<lsp::request::HoverRequest, _>(move |_| {
+            Some(lsp::Hover {
+                contents: lsp::HoverContents::Markup(lsp::MarkupContent {
+                    kind: lsp::MarkupKind::Markdown,
+                    value: indoc! {"
+                        # Some other basic docs
+                        Some other test documentation"}
+                    .to_string(),
+                }),
+                range: Some(symbol_range),
+            })
+        })
+        .await;
+        cx.foreground().run_until_parked();
+        cx.editor(|editor, _| {
+            assert!(editor.hover_state.visible());
+            assert_eq!(
+                editor.hover_state.popover.clone().unwrap().contents,
+                vec![
+                    HoverBlock {
+                        text: "Some other basic docs".to_string(),
+                        language: None
+                    },
+                    HoverBlock {
+                        text: "Some other test documentation".to_string(),
+                        language: None
+                    }
+                ]
+            )
+        });
+
+        // Open hover popover disables delay
+        let hover_point = cx.display_point(indoc! {"
+            fn test()
+                print|ln!();"});
+        cx.update_editor(|editor, cx| {
+            hover_at(
+                editor,
+                &HoverAt {
+                    point: Some(hover_point),
+                },
+                cx,
+            )
+        });
+
+        let symbol_range = cx.lsp_range(indoc! {"
+            fn test()
+                [println!]();"});
+        cx.handle_request::<lsp::request::HoverRequest, _>(move |_| {
+            Some(lsp::Hover {
+                contents: lsp::HoverContents::Markup(lsp::MarkupContent {
+                    kind: lsp::MarkupKind::Markdown,
+                    value: indoc! {"
+                        # Some third basic docs
+                        Some third test documentation"}
+                    .to_string(),
+                }),
+                range: Some(symbol_range),
+            })
+        })
+        .await;
+        cx.foreground().run_until_parked();
+        // No delay as the popover is already visible
+
+        cx.editor(|editor, _| {
+            assert!(editor.hover_state.visible());
+            assert_eq!(
+                editor.hover_state.popover.clone().unwrap().contents,
+                vec![
+                    HoverBlock {
+                        text: "Some third basic docs".to_string(),
+                        language: None
+                    },
+                    HoverBlock {
+                        text: "Some third test documentation".to_string(),
+                        language: None
+                    }
+                ]
+            )
+        });
+    }
+
     #[gpui::test]
     async fn test_toggle_comment(cx: &mut gpui::TestAppContext) {
         cx.update(|cx| cx.set_global(Settings::test(cx)));

crates/editor/src/hover_popover.rs 🔗

@@ -19,6 +19,10 @@ use crate::{
     EditorStyle,
 };
 
+pub const HOVER_DELAY_MILLIS: u64 = 500;
+pub const HOVER_REQUEST_DELAY_MILLIS: u64 = 250;
+pub const HOVER_GRACE_MILLIS: u64 = 250;
+
 #[derive(Clone, PartialEq)]
 pub struct HoverAt {
     pub point: Option<DisplayPoint>,
@@ -33,14 +37,14 @@ pub fn init(cx: &mut MutableAppContext) {
 }
 
 /// Bindable action which uses the most recent selection head to trigger a hover
-fn hover(editor: &mut Editor, _: &Hover, cx: &mut ViewContext<Editor>) {
+pub fn hover(editor: &mut Editor, _: &Hover, cx: &mut ViewContext<Editor>) {
     let head = editor.selections.newest_display(cx).head();
     show_hover(editor, head, true, cx);
 }
 
 /// The internal hover action dispatches between `show_hover` or `hide_hover`
 /// depending on whether a point to hover over is provided.
-fn hover_at(editor: &mut Editor, action: &HoverAt, cx: &mut ViewContext<Editor>) {
+pub fn hover_at(editor: &mut Editor, action: &HoverAt, cx: &mut ViewContext<Editor>) {
     if let Some(point) = action.point {
         show_hover(editor, point, false, cx);
     } else {
@@ -57,12 +61,12 @@ pub fn hide_hover(editor: &mut Editor, cx: &mut ViewContext<Editor>) -> bool {
     // only notify the context once
     if editor.hover_state.popover.is_some() {
         editor.hover_state.popover = None;
-        editor.hover_state.hidden_at = Some(Instant::now());
-        editor.hover_state.symbol_range = None;
+        editor.hover_state.hidden_at = Some(cx.background().now());
         did_hide = true;
         cx.notify();
     }
     editor.hover_state.task = None;
+    editor.hover_state.symbol_range = None;
 
     editor.clear_background_highlights::<HoverState>(cx);
 
@@ -118,13 +122,13 @@ fn show_hover(
 
     // We should only delay if the hover popover isn't visible, it wasn't recently hidden, and
     // the hover wasn't triggered from the keyboard
-    let should_delay = editor.hover_state.popover.is_none() // Hover visible currently
+    let should_delay = editor.hover_state.popover.is_none() // Hover not visible currently
         && editor
             .hover_state
             .hidden_at
-            .map(|hidden| hidden.elapsed().as_millis() > 200)
-            .unwrap_or(true) // Hover was visible recently enough
-        && !ignore_timeout; // Hover triggered from keyboard
+            .map(|hidden| hidden.elapsed().as_millis() > HOVER_GRACE_MILLIS as u128)
+            .unwrap_or(true) // Hover wasn't recently visible
+        && !ignore_timeout; // Hover was not triggered from keyboard
 
     if should_delay {
         if let Some(range) = &editor.hover_state.symbol_range {
@@ -145,8 +149,18 @@ fn show_hover(
 
     let task = cx.spawn_weak(|this, mut cx| {
         async move {
+            // If we need to delay, delay a set amount initially before making the lsp request
             let delay = if should_delay {
-                Some(cx.background().timer(Duration::from_millis(500)))
+                // Construct delay task to wait for later
+                let total_delay = Some(
+                    cx.background()
+                        .timer(Duration::from_millis(HOVER_DELAY_MILLIS)),
+                );
+
+                cx.background()
+                    .timer(Duration::from_millis(HOVER_REQUEST_DELAY_MILLIS))
+                    .await;
+                total_delay
             } else {
                 None
             };
@@ -157,6 +171,8 @@ fn show_hover(
                     return None;
                 }
 
+                // Create symbol range of anchors for highlighting and filtering
+                // of future requests.
                 let range = if let Some(range) = hover_result.range {
                     let start = snapshot
                         .buffer_snapshot
@@ -189,7 +205,7 @@ fn show_hover(
 
             if let Some(this) = this.upgrade(&cx) {
                 this.update(&mut cx, |this, cx| {
-                    if this.hover_state.popover.is_some() || hover_popover.is_some() {
+                    if hover_popover.is_some() {
                         // Highlight the selected symbol using a background highlight
                         if let Some(range) = this.hover_state.symbol_range.clone() {
                             this.highlight_background::<HoverState>(
@@ -198,18 +214,16 @@ fn show_hover(
                                 cx,
                             );
                         }
-
                         this.hover_state.popover = hover_popover;
-
-                        if this.hover_state.popover.is_none() {
-                            this.hover_state.hidden_at = Some(Instant::now());
-                        }
-
                         cx.notify();
-                    }
-
-                    if this.hover_state.popover.is_none() {
-                        this.hover_state.symbol_range = None;
+                    } else {
+                        if this.hover_state.popover.is_some() {
+                            // Popover was visible, but now is hidden. Dismiss it
+                            hide_hover(this, cx);
+                        } else {
+                            // Clear selected symbol range for future requests
+                            this.hover_state.symbol_range = None;
+                        }
                     }
                 });
             }
@@ -229,7 +243,13 @@ pub struct HoverState {
     pub task: Option<Task<Option<()>>>,
 }
 
-#[derive(Clone)]
+impl HoverState {
+    pub fn visible(&self) -> bool {
+        self.popover.is_some()
+    }
+}
+
+#[derive(Debug, Clone)]
 pub struct HoverPopover {
     pub project: ModelHandle<Project>,
     pub anchor: Anchor,

crates/editor/src/test.rs 🔗

@@ -1,10 +1,16 @@
-use std::ops::{Deref, DerefMut, Range};
+use std::{
+    ops::{Deref, DerefMut, Range},
+    sync::Arc,
+};
 
+use futures::StreamExt;
 use indoc::indoc;
 
 use collections::BTreeMap;
-use gpui::{keymap::Keystroke, ModelHandle, ViewContext, ViewHandle};
-use language::Selection;
+use gpui::{keymap::Keystroke, AppContext, ModelHandle, ViewContext, ViewHandle};
+use language::{point_to_lsp, FakeLspAdapter, Language, LanguageConfig, Selection};
+use lsp::request;
+use project::{FakeFs, Project};
 use settings::Settings;
 use util::{
     set_eq,
@@ -13,7 +19,8 @@ use util::{
 
 use crate::{
     display_map::{DisplayMap, DisplaySnapshot, ToDisplayPoint},
-    Autoscroll, DisplayPoint, Editor, EditorMode, MultiBuffer,
+    multi_buffer::ToPointUtf16,
+    Autoscroll, DisplayPoint, Editor, EditorMode, MultiBuffer, ToPoint,
 };
 
 #[cfg(test)]
@@ -102,6 +109,13 @@ impl<'a> EditorTestContext<'a> {
         }
     }
 
+    pub fn editor<F, T>(&mut self, read: F) -> T
+    where
+        F: FnOnce(&Editor, &AppContext) -> T,
+    {
+        self.editor.read_with(self.cx, read)
+    }
+
     pub fn update_editor<F, T>(&mut self, update: F) -> T
     where
         F: FnOnce(&mut Editor, &mut ViewContext<Editor>) -> T,
@@ -132,6 +146,14 @@ impl<'a> EditorTestContext<'a> {
         }
     }
 
+    pub fn display_point(&mut self, cursor_location: &str) -> DisplayPoint {
+        let (_, locations) = marked_text(cursor_location);
+        let snapshot = self
+            .editor
+            .update(self.cx, |editor, cx| editor.snapshot(cx));
+        locations[0].to_display_point(&snapshot.display_snapshot)
+    }
+
     // Sets the editor state via a marked string.
     // `|` characters represent empty selections
     // `[` to `}` represents a non empty selection with the head at `}`
@@ -365,3 +387,141 @@ impl<'a> DerefMut for EditorTestContext<'a> {
         &mut self.cx
     }
 }
+
+pub struct EditorLspTestContext<'a> {
+    pub cx: EditorTestContext<'a>,
+    lsp: lsp::FakeLanguageServer,
+}
+
+impl<'a> EditorLspTestContext<'a> {
+    pub async fn new(
+        mut language: Language,
+        capabilities: lsp::ServerCapabilities,
+        cx: &'a mut gpui::TestAppContext,
+    ) -> EditorLspTestContext<'a> {
+        let file_name = format!(
+            "/file.{}",
+            language
+                .path_suffixes()
+                .first()
+                .unwrap_or(&"txt".to_string())
+        );
+
+        let mut fake_servers = language.set_fake_lsp_adapter(FakeLspAdapter {
+            capabilities,
+            ..Default::default()
+        });
+
+        let fs = FakeFs::new(cx.background().clone());
+        fs.insert_file(file_name.clone(), "".to_string()).await;
+
+        let project = Project::test(fs, [file_name.as_ref()], cx).await;
+        project.update(cx, |project, _| project.languages().add(Arc::new(language)));
+        let buffer = project
+            .update(cx, |project, cx| project.open_local_buffer(file_name, cx))
+            .await
+            .unwrap();
+
+        let (window_id, editor) = cx.update(|cx| {
+            cx.set_global(Settings::test(cx));
+            crate::init(cx);
+
+            let (window_id, editor) = cx.add_window(Default::default(), |cx| {
+                let buffer = cx.add_model(|cx| MultiBuffer::singleton(buffer, cx));
+
+                Editor::new(EditorMode::Full, buffer, Some(project), None, None, cx)
+            });
+
+            editor.update(cx, |_, cx| cx.focus_self());
+
+            (window_id, editor)
+        });
+
+        let lsp = fake_servers.next().await.unwrap();
+
+        Self {
+            cx: EditorTestContext {
+                cx,
+                window_id,
+                editor,
+            },
+            lsp,
+        }
+    }
+
+    pub async fn new_rust(
+        capabilities: lsp::ServerCapabilities,
+        cx: &'a mut gpui::TestAppContext,
+    ) -> EditorLspTestContext<'a> {
+        let language = Language::new(
+            LanguageConfig {
+                name: "Rust".into(),
+                path_suffixes: vec!["rs".to_string()],
+                ..Default::default()
+            },
+            Some(tree_sitter_rust::language()),
+        );
+
+        Self::new(language, capabilities, cx).await
+    }
+
+    pub async fn handle_request<T, F>(&mut self, mut construct_result: F)
+    where
+        T: 'static + request::Request,
+        T::Params: 'static + Send,
+        T::Result: 'static + Send + Clone,
+        F: 'static + Send + FnMut(T::Params) -> T::Result,
+    {
+        self.lsp
+            .handle_request::<T, _, _>(move |params, _| {
+                let result = construct_result(params);
+                async move { Ok(result.clone()) }
+            })
+            .next()
+            .await;
+    }
+
+    // Constructs lsp range using a marked string with '[', ']' range delimiters
+    pub fn lsp_range(&mut self, marked_text: &str) -> lsp::Range {
+        let (unmarked, mut ranges) = marked_text_ranges_by(marked_text, vec![('[', ']').into()]);
+        assert_eq!(unmarked, self.editor_text());
+        let snapshot = self.update_editor(|editor, cx| editor.snapshot(cx));
+
+        let offset_range = ranges.remove(&('[', ']').into()).unwrap()[0].clone();
+        let start_point = offset_range.start.to_point(&snapshot.buffer_snapshot);
+        let end_point = offset_range.end.to_point(&snapshot.buffer_snapshot);
+        self.editor(|editor, cx| {
+            let buffer = editor.buffer().read(cx);
+            let start = point_to_lsp(
+                buffer
+                    .point_to_buffer_offset(start_point, cx)
+                    .unwrap()
+                    .1
+                    .to_point_utf16(&buffer.read(cx)),
+            );
+            let end = point_to_lsp(
+                buffer
+                    .point_to_buffer_offset(end_point, cx)
+                    .unwrap()
+                    .1
+                    .to_point_utf16(&buffer.read(cx)),
+            );
+
+            lsp::Range { start, end }
+        })
+    }
+}
+
+impl<'a> Deref for EditorLspTestContext<'a> {
+    type Target = EditorTestContext<'a>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.cx
+    }
+}
+
+impl<'a> DerefMut for EditorLspTestContext<'a> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.cx
+    }
+}

crates/gpui/src/executor.rs 🔗

@@ -330,6 +330,11 @@ impl Deterministic {
         Timer::Deterministic(DeterministicTimer { rx, id, state })
     }
 
+    pub fn now(&self) -> std::time::Instant {
+        let state = self.state.lock();
+        state.now.clone()
+    }
+
     pub fn advance_clock(&self, duration: Duration) {
         let new_now = self.state.lock().now + duration;
         loop {
@@ -647,6 +652,14 @@ impl Background {
         }
     }
 
+    pub fn now(&self) -> std::time::Instant {
+        match self {
+            Background::Production { .. } => std::time::Instant::now(),
+            #[cfg(any(test, feature = "test-support"))]
+            Background::Deterministic { executor } => executor.now(),
+        }
+    }
+
     #[cfg(any(test, feature = "test-support"))]
     pub async fn simulate_random_delay(&self) {
         use rand::prelude::*;

crates/language/src/language.rs 🔗

@@ -571,6 +571,10 @@ impl Language {
         &self.config.brackets
     }
 
+    pub fn path_suffixes(&self) -> &[String] {
+        &self.config.path_suffixes
+    }
+
     pub fn should_autoclose_before(&self, c: char) -> bool {
         c.is_whitespace() || self.config.autoclose_before.contains(c)
     }