Add lsp signature help (#1755)

* Add lsp signature help

* Do not move signature help popup on multiple triggers

* Highlight current parameter in signature help

* Auto close signature help

* Position signature help above to not block completion

* Update signature help on backspace/insert mode delete

* Add lsp.auto-signature-help config option

* Add serde default annotation for LspConfig

* Show LSP inactive message only if signature help is invoked manually

* Do not assume valid signature help response from LSP

Malformed LSP responses are common, and these should not crash the
editor.

* Check signature help capability before sending request

* Reuse Open enum for PositionBias in popup

* Close signature popup and exit insert mode on escape

* Add config to control signature help docs display

* Use new Margin api in signature help

* Invoke signature help on changing to insert mode

13 files changed, 380 insertions, 63 deletions

diff --git a/book/src/configuration.md b/book/src/configuration.md
index b8b939e7..c209dc3d 100644
--- a/book/src/configuration.md
+++ b/book/src/configuration.md
@@ -80,9 +80,11 @@ The following elements can be configured:
 
 ### `[editor.lsp]` Section
 
-| Key                | Description                                 | Default |
-| ---                | -----------                                 | ------- |
-| `display-messages` | Display LSP progress messages below statusline[^1] | `false` |
+| Key                   | Description                                                 | Default |
+| ---                   | -----------                                                 | ------- |
+| `display-messages`    | Display LSP progress messages below statusline[^1]          | `false` |
+| `auto-signature-help` | Enable automatic popup of signature help (parameter hints)  | `true`  |
+| `display-signature-help-docs` | Display docs under signature help popup             | `true`  |
 
 [^1]: By default, a progress spinner is shown in the statusline beside the file path.
 
diff --git a/helix-lsp/src/client.rs b/helix-lsp/src/client.rs
index 9187a61e..f6cec6aa 100644
--- a/helix-lsp/src/client.rs
+++ b/helix-lsp/src/client.rs
@@ -322,6 +322,16 @@ impl Client {
                         content_format: Some(vec![lsp::MarkupKind::Markdown]),
                         ..Default::default()
                     }),
+                    signature_help: Some(lsp::SignatureHelpClientCapabilities {
+                        signature_information: Some(lsp::SignatureInformationSettings {
+                            documentation_format: Some(vec![lsp::MarkupKind::Markdown]),
+                            parameter_information: Some(lsp::ParameterInformationSettings {
+                                label_offset_support: Some(true),
+                            }),
+                            active_parameter_support: Some(true),
+                        }),
+                        ..Default::default()
+                    }),
                     rename: Some(lsp::RenameClientCapabilities {
                         dynamic_registration: Some(false),
                         prepare_support: Some(false),
@@ -646,7 +656,12 @@ impl Client {
         text_document: lsp::TextDocumentIdentifier,
         position: lsp::Position,
         work_done_token: Option<lsp::ProgressToken>,
-    ) -> impl Future<Output = Result<Value>> {
+    ) -> Option<impl Future<Output = Result<Value>>> {
+        let capabilities = self.capabilities.get().unwrap();
+
+        // Return early if signature help is not supported
+        capabilities.signature_help_provider.as_ref()?;
+
         let params = lsp::SignatureHelpParams {
             text_document_position_params: lsp::TextDocumentPositionParams {
                 text_document,
@@ -657,7 +672,7 @@ impl Client {
             // lsp::SignatureHelpContext
         };
 
-        self.call::<lsp::request::SignatureHelpRequest>(params)
+        Some(self.call::<lsp::request::SignatureHelpRequest>(params))
     }
 
     pub fn text_document_hover(
diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs
index dad3db86..3ee75f6a 100644
--- a/helix-term/src/commands.rs
+++ b/helix-term/src/commands.rs
@@ -715,6 +715,8 @@ fn kill_to_line_start(cx: &mut Context) {
         Range::new(head, anchor)
     });
     delete_selection_insert_mode(doc, view, &selection);
+
+    lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
 }
 
 fn kill_to_line_end(cx: &mut Context) {
@@ -734,6 +736,8 @@ fn kill_to_line_end(cx: &mut Context) {
         new_range
     });
     delete_selection_insert_mode(doc, view, &selection);
+
+    lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
 }
 
 fn goto_first_nonwhitespace(cx: &mut Context) {
@@ -2399,7 +2403,8 @@ async fn make_format_callback(
     Ok(call)
 }
 
-enum Open {
+#[derive(PartialEq)]
+pub enum Open {
     Below,
     Above,
 }
@@ -2797,6 +2802,9 @@ pub mod insert {
         use helix_lsp::lsp;
         // if ch matches signature_help char, trigger
         let doc = doc_mut!(cx.editor);
+        // The language_server!() macro is not used here since it will
+        // print an "LSP not active for current buffer" message on
+        // every keypress.
         let language_server = match doc.language_server() {
             Some(language_server) => language_server,
             None => return,
@@ -2816,26 +2824,15 @@ pub mod insert {
         {
             // TODO: what if trigger is multiple chars long
             let is_trigger = triggers.iter().any(|trigger| trigger.contains(ch));
+            // lsp doesn't tell us when to close the signature help, so we request
+            // the help information again after common close triggers which should
+            // return None, which in turn closes the popup.
+            let close_triggers = &[')', ';', '.'];
 
-            if is_trigger {
-                super::signature_help(cx);
+            if is_trigger || close_triggers.contains(&ch) {
+                super::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
             }
         }
-
-        // SignatureHelp {
-        // signatures: [
-        //  SignatureInformation {
-        //      label: "fn open(&mut self, path: PathBuf, action: Action) -> Result<DocumentId, Error>",
-        //      documentation: None,
-        //      parameters: Some(
-        //          [ParameterInformation { label: Simple("path: PathBuf"), documentation: None },
-        //          ParameterInformation { label: Simple("action: Action"), documentation: None }]
-        //      ),
-        //      active_parameter: Some(0)
-        //  }
-        // ],
-        // active_signature: None, active_parameter: Some(0)
-        // }
     }
 
     // The default insert hook: simply insert the character
@@ -2870,7 +2867,6 @@ pub mod insert {
         // this could also generically look at Transaction, but it's a bit annoying to look at
         // Operation instead of Change.
         for hook in &[language_server_completion, signature_help] {
-            // for hook in &[signature_help] {
             hook(cx, c);
         }
     }
@@ -3042,6 +3038,8 @@ pub mod insert {
                 }
             });
         doc.apply(&transaction, view.id);
+
+        lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
     }
 
     pub fn delete_char_forward(cx: &mut Context) {
@@ -3058,6 +3056,8 @@ pub mod insert {
                 )
             });
         doc.apply(&transaction, view.id);
+
+        lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
     }
 
     pub fn delete_word_backward(cx: &mut Context) {
@@ -3071,6 +3071,8 @@ pub mod insert {
             exclude_cursor(text, next, range)
         });
         delete_selection_insert_mode(doc, view, &selection);
+
+        lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
     }
 
     pub fn delete_word_forward(cx: &mut Context) {
@@ -3083,6 +3085,8 @@ pub mod insert {
             .clone()
             .transform(|range| movement::move_next_word_start(text, range, count));
         delete_selection_insert_mode(doc, view, &selection);
+
+        lsp::signature_help_impl(cx, SignatureHelpInvoked::Automatic);
     }
 }
 
diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs
index a91e3792..1785a50c 100644
--- a/helix-term/src/commands/lsp.rs
+++ b/helix-term/src/commands/lsp.rs
@@ -6,18 +6,19 @@ use helix_lsp::{
 };
 use tui::text::{Span, Spans};
 
-use super::{align_view, push_jump, Align, Context, Editor};
+use super::{align_view, push_jump, Align, Context, Editor, Open};
 
 use helix_core::{path, Selection};
 use helix_view::{editor::Action, theme::Style};
 
 use crate::{
     compositor::{self, Compositor},
-    ui::{self, overlay::overlayed, FileLocation, FilePicker, Popup, PromptEvent},
+    ui::{
+        self, lsp::SignatureHelp, overlay::overlayed, FileLocation, FilePicker, Popup, PromptEvent,
+    },
 };
 
-use std::collections::BTreeMap;
-use std::{borrow::Cow, path::PathBuf};
+use std::{borrow::Cow, collections::BTreeMap, path::PathBuf, sync::Arc};
 
 /// Gets the language server that is attached to a document, and
 /// if it's not active displays a status message. Using this macro
@@ -805,31 +806,116 @@ pub fn goto_reference(cx: &mut Context) {
     );
 }
 
+#[derive(PartialEq)]
+pub enum SignatureHelpInvoked {
+    Manual,
+    Automatic,
+}
+
 pub fn signature_help(cx: &mut Context) {
+    signature_help_impl(cx, SignatureHelpInvoked::Manual)
+}
+
+pub fn signature_help_impl(cx: &mut Context, invoked: SignatureHelpInvoked) {
     let (view, doc) = current!(cx.editor);
-    let language_server = language_server!(cx.editor, doc);
+    let was_manually_invoked = invoked == SignatureHelpInvoked::Manual;
+
+    let language_server = match doc.language_server() {
+        Some(language_server) => language_server,
+        None => {
+            // Do not show the message if signature help was invoked
+            // automatically on backspace, trigger characters, etc.
+            if was_manually_invoked {
+                cx.editor
+                    .set_status("Language server not active for current buffer");
+            }
+            return;
+        }
+    };
     let offset_encoding = language_server.offset_encoding();
 
     let pos = doc.position(view.id, offset_encoding);
 
-    let future = language_server.text_document_signature_help(doc.identifier(), pos, None);
+    let future = match language_server.text_document_signature_help(doc.identifier(), pos, None) {
+        Some(f) => f,
+        None => return,
+    };
 
     cx.callback(
         future,
-        move |_editor, _compositor, response: Option<lsp::SignatureHelp>| {
-            if let Some(signature_help) = response {
-                log::info!("{:?}", signature_help);
-                // signatures
-                // active_signature
-                // active_parameter
-                // render as:
-
-                // signature
-                // ----------
-                // doc
-
-                // with active param highlighted
+        move |editor, compositor, response: Option<lsp::SignatureHelp>| {
+            let config = &editor.config();
+
+            if !(config.lsp.auto_signature_help
+                || SignatureHelp::visible_popup(compositor).is_some()
+                || was_manually_invoked)
+            {
+                return;
             }
+
+            let response = match response {
+                // According to the spec the response should be None if there
+                // are no signatures, but some servers don't follow this.
+                Some(s) if !s.signatures.is_empty() => s,
+                _ => {
+                    compositor.remove(SignatureHelp::ID);
+                    return;
+                }
+            };
+            let doc = doc!(editor);
+            let language = doc
+                .language()
+                .and_then(|scope| scope.strip_prefix("source."))
+                .unwrap_or("");
+
+            let signature = match response
+                .signatures
+                .get(response.active_signature.unwrap_or(0) as usize)
+            {
+                Some(s) => s,
+                None => return,
+            };
+            let mut contents = SignatureHelp::new(
+                signature.label.clone(),
+                language.to_string(),
+                Arc::clone(&editor.syn_loader),
+            );
+
+            let signature_doc = if config.lsp.display_signature_help_docs {
+                signature.documentation.as_ref().map(|doc| match doc {
+                    lsp::Documentation::String(s) => s.clone(),
+                    lsp::Documentation::MarkupContent(markup) => markup.value.clone(),
+                })
+            } else {
+                None
+            };
+
+            contents.set_signature_doc(signature_doc);
+
+            let active_param_range = || -> Option<(usize, usize)> {
+                let param_idx = signature
+                    .active_parameter
+                    .or(response.active_parameter)
+                    .unwrap_or(0) as usize;
+                let param = signature.parameters.as_ref()?.get(param_idx)?;
+                match &param.label {
+                    lsp::ParameterLabel::Simple(string) => {
+                        let start = signature.label.find(string.as_str())?;
+                        Some((start, start + string.len()))
+                    }
+                    lsp::ParameterLabel::LabelOffsets([start, end]) => {
+                        Some((*start as usize, *end as usize))
+                    }
+                }
+            };
+            contents.set_active_param_range(active_param_range());
+
+            let old_popup = compositor.find_id::<Popup<SignatureHelp>>(SignatureHelp::ID);
+            let popup = Popup::new(SignatureHelp::ID, contents)
+                .position(old_popup.and_then(|p| p.get_position()))
+                .position_bias(Open::Above)
+                .ignore_escape_key(true);
+            compositor.replace_or_push(SignatureHelp::ID, popup);
         },
     );
 }
diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs
index 4e1ac0da..fb03af44 100644
--- a/helix-term/src/commands/typed.rs
+++ b/helix-term/src/commands/typed.rs
@@ -1501,11 +1501,9 @@ fn run_shell_command(
                         format!("```sh\n{}\n```", output),
                         editor.syn_loader.clone(),
                     );
-                    let mut popup = Popup::new("shell", contents);
-                    popup.set_position(Some(helix_core::Position::new(
-                        editor.cursor().0.unwrap_or_default().row,
-                        2,
-                    )));
+                    let popup = Popup::new("shell", contents).position(Some(
+                        helix_core::Position::new(editor.cursor().0.unwrap_or_default().row, 2),
+                    ));
                     compositor.replace_or_push("shell", popup);
                 });
             Ok(call)
diff --git a/helix-term/src/compositor.rs b/helix-term/src/compositor.rs
index 61a3bfaf..5548e832 100644
--- a/helix-term/src/compositor.rs
+++ b/helix-term/src/compositor.rs
@@ -150,6 +150,14 @@ impl Compositor {
         self.layers.pop()
     }
 
+    pub fn remove(&mut self, id: &'static str) -> Option<Box<dyn Component>> {
+        let idx = self
+            .layers
+            .iter()
+            .position(|layer| layer.id() == Some(id))?;
+        Some(self.layers.remove(idx))
+    }
+
     pub fn handle_event(&mut self, event: Event, cx: &mut Context) -> bool {
         // If it is a key event and a macro is being recorded, push the key event to the recording.
         if let (Event::Key(key), Some((_, keys))) = (event, &mut cx.editor.macro_recording) {
diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs
index a3637415..c1816db1 100644
--- a/helix-term/src/ui/completion.rs
+++ b/helix-term/src/ui/completion.rs
@@ -85,6 +85,8 @@ pub struct Completion {
 }
 
 impl Completion {
+    pub const ID: &'static str = "completion";
+
     pub fn new(
         editor: &Editor,
         items: Vec<CompletionItem>,
@@ -214,7 +216,7 @@ impl Completion {
                 }
             };
         });
-        let popup = Popup::new("completion", menu);
+        let popup = Popup::new(Self::ID, menu);
         let mut completion = Self {
             popup,
             start_offset,
diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs
index 911ee0f0..849f0b0b 100644
--- a/helix-term/src/ui/editor.rs
+++ b/helix-term/src/ui/editor.rs
@@ -1,7 +1,7 @@
 use crate::{
     commands,
     compositor::{Component, Context, EventResult},
-    key,
+    job, key,
     keymap::{KeymapResult, Keymaps},
     ui::{Completion, ProgressSpinners},
 };
@@ -28,6 +28,7 @@ use std::borrow::Cow;
 use crossterm::event::{Event, MouseButton, MouseEvent, MouseEventKind};
 use tui::buffer::Buffer as Surface;
 
+use super::lsp::SignatureHelp;
 use super::statusline;
 
 pub struct EditorView {
@@ -1205,10 +1206,21 @@ impl Component for EditorView {
                             _ => unimplemented!(),
                         };
                         self.last_insert.1.clear();
+                        commands::signature_help_impl(
+                            &mut cx,
+                            commands::SignatureHelpInvoked::Automatic,
+                        );
                     }
                     (Mode::Insert, Mode::Normal) => {
                         // if exiting insert mode, remove completion
                         self.completion = None;
+                        // TODO: Use an on_mode_change hook to remove signature help
+                        context.jobs.callback(async {
+                            let call: job::Callback = Box::new(|_editor, compositor| {
+                                compositor.remove(SignatureHelp::ID);
+                            });
+                            Ok(call)
+                        });
                     }
                     _ => (),
                 }
diff --git a/helix-term/src/ui/lsp.rs b/helix-term/src/ui/lsp.rs
new file mode 100644
index 00000000..f2854551
--- /dev/null
+++ b/helix-term/src/ui/lsp.rs
@@ -0,0 +1,133 @@
+use std::sync::Arc;
+
+use helix_core::syntax;
+use helix_view::graphics::{Margin, Rect, Style};
+use tui::buffer::Buffer;
+use tui::widgets::{BorderType, Paragraph, Widget, Wrap};
+
+use crate::compositor::{Component, Compositor, Context};
+
+use crate::ui::Markdown;
+
+use super::Popup;
+
+pub struct SignatureHelp {
+    signature: String,
+    signature_doc: Option<String>,
+    /// Part of signature text
+    active_param_range: Option<(usize, usize)>,
+
+    language: String,
+    config_loader: Arc<syntax::Loader>,
+}
+
+impl SignatureHelp {
+    pub const ID: &'static str = "signature-help";
+
+    pub fn new(signature: String, language: String, config_loader: Arc<syntax::Loader>) -> Self {
+        Self {
+            signature,
+            signature_doc: None,
+            active_param_range: None,
+            language,
+            config_loader,
+        }
+    }
+
+    pub fn set_signature_doc(&mut self, signature_doc: Option<String>) {
+        self.signature_doc = signature_doc;
+    }
+
+    pub fn set_active_param_range(&mut self, offset: Option<(usize, usize)>) {
+        self.active_param_range = offset;
+    }
+
+    pub fn visible_popup(compositor: &mut Compositor) -> Option<&mut Popup<Self>> {
+        compositor.find_id::<Popup<Self>>(Self::ID)
+    }
+}
+
+impl Component for SignatureHelp {
+    fn render(&mut self, area: Rect, surface: &mut Buffer, cx: &mut Context) {
+        let margin = Margin::horizontal(1);
+
+        let active_param_span = self.active_param_range.map(|(start, end)| {
+            vec![(
+                cx.editor.theme.find_scope_index("ui.selection").unwrap(),
+                start..end,
+            )]
+        });
+
+        let sig_text = crate::ui::markdown::highlighted_code_block(
+            self.signature.clone(),
+            &self.language,
+            Some(&cx.editor.theme),
+            Arc::clone(&self.config_loader),
+            active_param_span,
+        );
+
+        let (_, sig_text_height) = crate::ui::text::required_size(&sig_text, area.width);
+        let sig_text_area = area.clip_top(1).with_height(sig_text_height);
+        let sig_text_para = Paragraph::new(sig_text).wrap(Wrap { trim: false });
+        sig_text_para.render(sig_text_area.inner(&margin), surface);
+
+        if self.signature_doc.is_none() {
+            return;
+        }
+
+        let sep_style = Style::default();
+        let borders = BorderType::line_symbols(BorderType::Plain);
+        for x in sig_text_area.left()..sig_text_area.right() {
+            if let Some(cell) = surface.get_mut(x, sig_text_area.bottom()) {
+                cell.set_symbol(borders.horizontal).set_style(sep_style);
+            }
+        }
+
+        let sig_doc = match &self.signature_doc {
+            None => return,
+            Some(doc) => Markdown::new(doc.clone(), Arc::clone(&self.config_loader)),
+        };
+        let sig_doc = sig_doc.parse(Some(&cx.editor.theme));
+        let sig_doc_area = area.clip_top(sig_text_area.height + 2);
+        let sig_doc_para = Paragraph::new(sig_doc)
+            .wrap(Wrap { trim: false })
+            .scroll((cx.scroll.unwrap_or_default() as u16, 0));
+        sig_doc_para.render(sig_doc_area.inner(&margin), surface);
+    }
+
+    fn required_size(&mut self, viewport: (u16, u16)) -> Option<(u16, u16)> {
+        const PADDING: u16 = 2;
+        const SEPARATOR_HEIGHT: u16 = 1;
+
+        if PADDING >= viewport.1 || PADDING >= viewport.0 {
+            return None;
+        }
+        let max_text_width = (viewport.0 - PADDING).min(120);
+
+        let signature_text = crate::ui::markdown::highlighted_code_block(
+            self.signature.clone(),
+            &self.language,
+            None,
+            Arc::clone(&self.config_loader),
+            None,
+        );
+        let (sig_width, sig_height) =
+            crate::ui::text::required_size(&signature_text, max_text_width);
+
+        let (width, height) = match self.signature_doc {
+            Some(ref doc) => {
+                let doc_md = Markdown::new(doc.clone(), Arc::clone(&self.config_loader));
+                let doc_text = doc_md.parse(None);
+                let (doc_width, doc_height) =
+                    crate::ui::text::required_size(&doc_text, max_text_width);
+                (
+                    sig_width.max(doc_width),
+                    sig_height + SEPARATOR_HEIGHT + doc_height,
+                )
+            }
+            None => (sig_width, sig_height),
+        };
+
+        Some((width + PADDING, height + PADDING))
+    }
+}
diff --git a/helix-term/src/ui/markdown.rs b/helix-term/src/ui/markdown.rs
index a5c78c41..c53b3b66 100644
--- a/helix-term/src/ui/markdown.rs
+++ b/helix-term/src/ui/markdown.rs
@@ -144,7 +144,7 @@ impl Markdown {
         }
     }
 
-    fn parse(&self, theme: Option<&Theme>) -> tui::text::Text<'_> {
+    pub fn parse(&self, theme: Option<&Theme>) -> tui::text::Text<'_> {
         fn push_line<'a>(spans: &mut Vec<Span<'a>>, lines: &mut Vec<Spans<'a>>) {
             let spans = std::mem::take(spans);
             if !spans.is_empty() {
diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs
index 88a226e9..257608f0 100644
--- a/helix-term/src/ui/mod.rs
+++ b/helix-term/src/ui/mod.rs
@@ -1,11 +1,12 @@
 mod completion;
 pub(crate) mod editor;
 mod info;
+pub mod lsp;
 mod markdown;
 pub mod menu;
 pub mod overlay;
 mod picker;
-mod popup;
+pub mod popup;
 mod prompt;
 mod spinner;
 mod statusline;
diff --git a/helix-term/src/ui/popup.rs b/helix-term/src/ui/popup.rs
index f5b79526..77ab2462 100644
--- a/helix-term/src/ui/popup.rs
+++ b/helix-term/src/ui/popup.rs
@@ -1,4 +1,5 @@
 use crate::{
+    commands::Open,
     compositor::{Callback, Component, Context, EventResult},
     ctrl, key,
 };
@@ -17,8 +18,10 @@ pub struct Popup<T: Component> {
     margin: Margin,
     size: (u16, u16),
     child_size: (u16, u16),
+    position_bias: Open,
     scroll: usize,
     auto_close: bool,
+    ignore_escape_key: bool,
     id: &'static str,
 }
 
@@ -29,15 +32,27 @@ impl<T: Component> Popup<T> {
             position: None,
             margin: Margin::none(),
             size: (0, 0),
+            position_bias: Open::Below,
             child_size: (0, 0),
             scroll: 0,
             auto_close: false,
+            ignore_escape_key: false,
             id,
         }
     }
 
-    pub fn set_position(&mut self, pos: Option<Position>) {
+    pub fn position(mut self, pos: Option<Position>) -> Self {
         self.position = pos;
+        self
+    }
+
+    pub fn get_position(&self) -> Option<Position> {
+        self.position
+    }
+
+    pub fn position_bias(mut self, bias: Open) -> Self {
+        self.position_bias = bias;
+        self
     }
 
     pub fn margin(mut self, margin: Margin) -> Self {
@@ -50,6 +65,18 @@ impl<T: Component> Popup<T> {
         self
     }
 
+    /// Ignores an escape keypress event, letting the outer layer
+    /// (usually the editor) handle it. This is useful for popups
+    /// in insert mode like completion and signature help where
+    /// the popup is closed on the mode change from insert to normal
+    /// which is done with the escape key. Otherwise the popup consumes
+    /// the escape key event and closes it, and an additional escape
+    /// would be required to exit insert mode.
+    pub fn ignore_escape_key(mut self, ignore: bool) -> Self {
+        self.ignore_escape_key = ignore;
+        self
+    }
+
     pub fn get_rel_position(&mut self, viewport: Rect, cx: &Context) -> (u16, u16) {
         let position = self
             .position
@@ -68,13 +95,23 @@ impl<T: Component> Popup<T> {
             rel_x = rel_x.saturating_sub((rel_x + width).saturating_sub(viewport.width));
         }
 
-        // TODO: be able to specify orientation preference. We want above for most popups, below
-        // for menus/autocomplete.
-        if viewport.height > rel_y + height {
-            rel_y += 1 // position below point
-        } else {
-            rel_y = rel_y.saturating_sub(height) // position above point
-        }
+        let can_put_below = viewport.height > rel_y + height;
+        let can_put_above = rel_y.checked_sub(height).is_some();
+        let final_pos = match self.position_bias {
+            Open::Below => match can_put_below {
+                true => Open::Below,
+                false => Open::Above,
+            },
+            Open::Above => match can_put_above {
+                true => Open::Above,
+                false => Open::Below,
+            },
+        };
+
+        rel_y = match final_pos {
+            Open::Above => rel_y.saturating_sub(height),
+            Open::Below => rel_y + 1,
+        };
 
         (rel_x, rel_y)
     }
@@ -112,9 +149,13 @@ impl<T: Component> Component for Popup<T> {
             _ => return EventResult::Ignored(None),
         };
 
+        if key!(Esc) == key.into() && self.ignore_escape_key {
+            return EventResult::Ignored(None);
+        }
+
         let close_fn: Callback = Box::new(|compositor, _| {
             // remove the layer
-            compositor.pop();
+            compositor.remove(self.id.as_ref());
         });
 
         match key.into() {
diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs
index babb5c43..0178d399 100644
--- a/helix-view/src/editor.rs
+++ b/helix-view/src/editor.rs
@@ -167,10 +167,25 @@ pub struct Config {
     pub color_modes: bool,
 }
 
-#[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)]
-#[serde(rename_all = "kebab-case", deny_unknown_fields)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(default, rename_all = "kebab-case", deny_unknown_fields)]
 pub struct LspConfig {
+    /// Display LSP progress messages below statusline
     pub display_messages: bool,
+    /// Enable automatic pop up of signature help (parameter hints)
+    pub auto_signature_help: bool,
+    /// Display docs under signature help popup
+    pub display_signature_help_docs: bool,
+}
+
+impl Default for LspConfig {
+    fn default() -> Self {
+        Self {
+            display_messages: false,
+            auto_signature_help: true,
+            display_signature_help_docs: true,
+        }
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]