Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: cycle through function signatures/overloads #9974

Merged
merged 1 commit into from
Apr 16, 2024
Merged

feat: cycle through function signatures/overloads #9974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 67 additions & 45 deletions helix-term/src/handlers/signature_help.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use helix_core::syntax::LanguageServerFeature;
use helix_event::{
cancelable_future, cancelation, register_hook, send_blocking, CancelRx, CancelTx,
};
use helix_lsp::lsp;
use helix_lsp::lsp::{self, SignatureInformation};
use helix_stdx::rope::RopeSliceExt;
use helix_view::document::Mode;
use helix_view::events::{DocumentDidChange, SelectionDidChange};
Expand All @@ -18,7 +18,7 @@ use crate::commands::Open;
use crate::compositor::Compositor;
use crate::events::{OnModeSwitch, PostInsertChar};
use crate::handlers::Handlers;
use crate::ui::lsp::SignatureHelp;
use crate::ui::lsp::{Signature, SignatureHelp};
use crate::ui::Popup;
use crate::{job, ui};

Expand Down Expand Up @@ -82,6 +82,7 @@ impl helix_event::AsyncHook for SignatureHelpHandler {
}
}
self.state = if open { State::Open } else { State::Closed };

return timeout;
}
}
Expand Down Expand Up @@ -138,6 +139,31 @@ pub fn request_signature_help(
});
}

fn active_param_range(
signature: &SignatureInformation,
response_active_parameter: Option<u32>,
) -> 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]) => {
// LS sends offsets based on utf-16 based string representation
// but highlighting in helix is done using byte offset.
use helix_core::str_utils::char_to_byte_idx;
let from = char_to_byte_idx(&signature.label, *start as usize);
let to = char_to_byte_idx(&signature.label, *end as usize);
Some((from, to))
}
}
}

pub fn show_signature_help(
editor: &mut Editor,
compositor: &mut Compositor,
Expand Down Expand Up @@ -184,54 +210,50 @@ pub fn show_signature_help(
let doc = doc!(editor);
let language = doc.language_name().unwrap_or("");

let signature = match response
if response.signatures.is_empty() {
return;
}

let signatures: Vec<Signature> = response
.signatures
.get(response.active_signature.unwrap_or(0) as usize)
{
Some(s) => s,
None => return,
};
let mut contents = SignatureHelp::new(
signature.label.clone(),
.into_iter()
.map(|s| {
let active_param_range = active_param_range(&s, response.active_parameter);

let signature_doc = if config.lsp.display_signature_help_docs {
s.documentation.map(|doc| match doc {
lsp::Documentation::String(s) => s,
lsp::Documentation::MarkupContent(markup) => markup.value,
})
} else {
None
};

Signature {
signature: s.label,
signature_doc,
active_param_range,
}
})
.collect();

let old_popup = compositor.find_id::<Popup<SignatureHelp>>(SignatureHelp::ID);
let mut active_signature = old_popup
.as_ref()
.map(|popup| popup.contents().active_signature())
.unwrap_or_else(|| response.active_signature.unwrap_or_default() as usize);

if active_signature >= signatures.len() {
active_signature = signatures.len() - 1;
}
Comment on lines +241 to +248
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should prefer the active_signature we get from the language server here instead:

let active_signature = response
    .active_signature
    .map(|n| n as usize)
    .or_else(|| {
        old_popup
            .as_ref()
            .map(|popup| popup.contents().active_signature())
    })
    .unwrap_or_default()
    .min(signatures.len() - 1);

Some language servers update the active_signature they send when you enter enough parameters. For example with Erlang you might be typing maps:get( which is a multiarity function. maps:get(a, b should show the signature of maps:get/2 (takes 2 arguments) but for maps:get(a, b, the language server will show maps:get/3.

We could make this more complicated though and separately track which signature the language server sends and which you navigate to with A-n/A-p, preferring the signature you navigate to until the server changes which signature it considers active.

Copy link
Contributor Author

@karthago1 karthago1 May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have played with your code. My c++ LSP somehow doesn't update the active signature at all. it is always 0.
so IMO it is very annoying to switch to the right signature manually, then it switches always back to the first one.

but in the same time using the C# LSP your code brings some improvements, since the suggested LSP signature is in most cases useful.

I created #10655, which implements the bit more complicated solution you have suggested


let contents = SignatureHelp::new(
language.to_string(),
Arc::clone(&editor.syn_loader),
active_signature,
signatures,
);

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]) => {
// LS sends offsets based on utf-16 based string representation
// but highlighting in helix is done using byte offset.
use helix_core::str_utils::char_to_byte_idx;
let from = char_to_byte_idx(&signature.label, *start as usize);
let to = char_to_byte_idx(&signature.label, *end as usize);
Some((from, to))
}
}
};
contents.set_active_param_range(active_param_range());

let old_popup = compositor.find_id::<Popup<SignatureHelp>>(SignatureHelp::ID);
let mut popup = Popup::new(SignatureHelp::ID, contents)
.position(old_popup.and_then(|p| p.get_position()))
.position_bias(Open::Above)
Expand Down
95 changes: 73 additions & 22 deletions helix-term/src/ui/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,60 +3,95 @@ use std::sync::Arc;
use arc_swap::ArcSwap;
use helix_core::syntax;
use helix_view::graphics::{Margin, Rect, Style};
use helix_view::input::Event;
use tui::buffer::Buffer;
use tui::layout::Alignment;
use tui::text::Text;
use tui::widgets::{BorderType, Paragraph, Widget, Wrap};

use crate::compositor::{Component, Compositor, Context};
use crate::compositor::{Component, Compositor, Context, EventResult};

use crate::alt;
use crate::ui::Markdown;

use super::Popup;

pub struct SignatureHelp {
signature: String,
signature_doc: Option<String>,
pub struct Signature {
pub signature: String,
pub signature_doc: Option<String>,
/// Part of signature text
active_param_range: Option<(usize, usize)>,
pub active_param_range: Option<(usize, usize)>,
}

pub struct SignatureHelp {
language: String,
config_loader: Arc<ArcSwap<syntax::Loader>>,
active_signature: usize,
signatures: Vec<Signature>,
}

impl SignatureHelp {
pub const ID: &'static str = "signature-help";

pub fn new(
signature: String,
language: String,
config_loader: Arc<ArcSwap<syntax::Loader>>,
active_signature: usize,
signatures: Vec<Signature>,
) -> Self {
Self {
signature,
signature_doc: None,
active_param_range: None,
language,
config_loader,
active_signature,
signatures,
}
}

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 active_signature(&self) -> usize {
self.active_signature
}

pub fn visible_popup(compositor: &mut Compositor) -> Option<&mut Popup<Self>> {
compositor.find_id::<Popup<Self>>(Self::ID)
}

fn signature_index(&self) -> String {
format!("({}/{})", self.active_signature + 1, self.signatures.len())
}
}

impl Component for SignatureHelp {
fn handle_event(&mut self, event: &Event, _cx: &mut Context) -> EventResult {
let Event::Key(event) = event else {
return EventResult::Ignored(None);
};

if self.signatures.len() <= 1 {
return EventResult::Ignored(None);
}

match event {
alt!('p') => {
self.active_signature = self
.active_signature
.checked_sub(1)
.unwrap_or(self.signatures.len() - 1);
EventResult::Consumed(None)
}
alt!('n') => {
self.active_signature = (self.active_signature + 1) % self.signatures.len();
EventResult::Consumed(None)
}
_ => EventResult::Ignored(None),
}
}

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)| {
let signature = &self.signatures[self.active_signature];

let active_param_span = signature.active_param_range.map(|(start, end)| {
vec![(
cx.editor
.theme
Expand All @@ -66,21 +101,29 @@ impl Component for SignatureHelp {
)]
});

let sig = &self.signatures[self.active_signature];
let sig_text = crate::ui::markdown::highlighted_code_block(
&self.signature,
sig.signature.as_str(),
&self.language,
Some(&cx.editor.theme),
Arc::clone(&self.config_loader),
active_param_span,
);

if self.signatures.len() > 1 {
let signature_index = self.signature_index();
let text = Text::from(signature_index);
let paragraph = Paragraph::new(&text).alignment(Alignment::Right);
paragraph.render(area.clip_top(1).with_height(1).clip_right(1), surface);
}

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_area = sig_text_area.inner(&margin).intersection(surface.area);
let sig_text_para = Paragraph::new(&sig_text).wrap(Wrap { trim: false });
sig_text_para.render(sig_text_area, surface);

if self.signature_doc.is_none() {
if sig.signature_doc.is_none() {
return;
}

Expand All @@ -92,7 +135,7 @@ impl Component for SignatureHelp {
}
}

let sig_doc = match &self.signature_doc {
let sig_doc = match &sig.signature_doc {
None => return,
Some(doc) => Markdown::new(doc.clone(), Arc::clone(&self.config_loader)),
};
Expand All @@ -110,13 +153,15 @@ impl Component for SignatureHelp {
const PADDING: u16 = 2;
const SEPARATOR_HEIGHT: u16 = 1;

let sig = &self.signatures[self.active_signature];

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,
sig.signature.as_str(),
&self.language,
None,
Arc::clone(&self.config_loader),
Expand All @@ -125,7 +170,7 @@ impl Component for SignatureHelp {
let (sig_width, sig_height) =
crate::ui::text::required_size(&signature_text, max_text_width);

let (width, height) = match self.signature_doc {
let (width, height) = match sig.signature_doc {
Some(ref doc) => {
let doc_md = Markdown::new(doc.clone(), Arc::clone(&self.config_loader));
let doc_text = doc_md.parse(None);
Expand All @@ -139,6 +184,12 @@ impl Component for SignatureHelp {
None => (sig_width, sig_height),
};

Some((width + PADDING, height + PADDING))
let sig_index_width = if self.signatures.len() > 1 {
self.signature_index().len() + 1
} else {
0
};

Some((width + PADDING + sig_index_width as u16, height + PADDING))
}
}
Loading