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

Add more context to the symbol picker #2452

Closed
wants to merge 3 commits into from
Closed

Add more context to the symbol picker #2452

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd5152d
Add more context to the symbol picker
mdelaney May 7, 2022
a3db019
add TODO to show ancestor symbols in the symbol picker
mdelaney May 14, 2022
1f29d08
add logging for unknown symbol kind in symbol picker
mdelaney May 15, 2022
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
79 changes: 68 additions & 11 deletions helix-term/src/commands/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,28 +64,83 @@ fn jump_to_location(
align_view(doc, view, Align::Center);
}

/// Returns a string showing the symbol nested in its containing symbol if it exists.
///
/// # Arguments
/// * `symbol` - the symbol that we're creating the string for
///
/// # Example
/// If you have something like:
/// ```
/// mod foo {
/// fn bar() {}
/// }
/// ```
/// the result will be: `function : foo → bar`
fn get_symbol_string(symbol: &lsp::SymbolInformation) -> String {
let kind = match symbol.kind {
lsp::SymbolKind::FILE => "file ",
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some idea but not sure if it's good or not. Why not do [file] ?

Copy link
Author

Choose a reason for hiding this comment

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

The downside of [symbol kind] is that it would take more horizontal space. It also feels a bit more cluttery (at least in my view). I think the ideal would be to eventually differentiate with color (which would also remove the need for the → separator altogether).

lsp::SymbolKind::MODULE => "module ",
lsp::SymbolKind::NAMESPACE => "namespace ",
lsp::SymbolKind::PACKAGE => "package ",
lsp::SymbolKind::CLASS => "class ",
lsp::SymbolKind::METHOD => "method ",
lsp::SymbolKind::PROPERTY => "property ",
lsp::SymbolKind::FIELD => "field ",
lsp::SymbolKind::CONSTRUCTOR => "construct ",
lsp::SymbolKind::ENUM => "enum ",
lsp::SymbolKind::INTERFACE => "interface ",
lsp::SymbolKind::FUNCTION => "function ",
lsp::SymbolKind::VARIABLE => "variable ",
lsp::SymbolKind::CONSTANT => "constant ",
lsp::SymbolKind::STRING => "string ",
lsp::SymbolKind::NUMBER => "number ",
lsp::SymbolKind::BOOLEAN => "boolean ",
lsp::SymbolKind::ARRAY => "array ",
lsp::SymbolKind::OBJECT => "object ",
lsp::SymbolKind::KEY => "key ",
lsp::SymbolKind::NULL => "null ",
lsp::SymbolKind::ENUM_MEMBER => "enum mem ",
lsp::SymbolKind::STRUCT => "struct ",
lsp::SymbolKind::EVENT => "event ",
lsp::SymbolKind::OPERATOR => "operator ",
lsp::SymbolKind::TYPE_PARAMETER => "type param",
Comment on lines +82 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

I think even if you want to align, it is better to remove the spaces, then use left align format specifier to do the spacing rather than having to do that manually. Doing it manually will cause changing the largest variant require changing all the lines, bad for git diff.

_ => {
log::warn!("Unknown symbol kind: {:?}", symbol.kind);
""
}
};

// TODO: we currently only show the symbol directly containing the current symbol; however,
// it'd be really useful to have the ability to show all the ancestor symbols as well.
let prefix = symbol
.clone()
.container_name
.map_or(String::new(), |name| format!("{} → ", name));
format!("{}: {}{}", kind, prefix, symbol.name.clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this look bad for the case in which we don't match? Given that now it will show : ......

}

fn sym_picker(
symbols: Vec<lsp::SymbolInformation>,
current_path: Option<lsp::Url>,
offset_encoding: OffsetEncoding,
workspace: bool,
) -> FilePicker<lsp::SymbolInformation> {
// TODO: drop current_path comparison and instead use workspace: bool flag?
let current_path2 = current_path.clone();
FilePicker::new(
symbols,
move |symbol| {
if current_path.as_ref() == Some(&symbol.location.uri) {
symbol.name.as_str().into()
let symbol_string = get_symbol_string(symbol);
if !workspace {
symbol_string.into()
} else {
let path = symbol.location.uri.to_file_path().unwrap();
let relative_path = helix_core::path::get_relative_path(path.as_path())
.to_string_lossy()
.into_owned();
format!("{} ({})", &symbol.name, relative_path).into()
format!("{} ({})", &symbol_string, relative_path).into()
}
},
move |cx, symbol, action| {
if current_path2.as_ref() == Some(&symbol.location.uri) {
if !workspace {
push_jump(cx.editor);
} else {
let path = symbol.location.uri.to_file_path().unwrap();
Expand Down Expand Up @@ -130,7 +185,6 @@ pub fn symbol_picker(cx: &mut Context) {
let doc = doc!(cx.editor);

let language_server = language_server!(cx.editor, doc);
let current_url = doc.url();
let offset_encoding = language_server.offset_encoding();

let future = language_server.document_symbols(doc.identifier());
Expand All @@ -153,7 +207,7 @@ pub fn symbol_picker(cx: &mut Context) {
}
};

let picker = sym_picker(symbols, current_url, offset_encoding);
let picker = sym_picker(symbols, offset_encoding, false);
compositor.push(Box::new(overlayed(picker)))
}
},
Expand All @@ -162,7 +216,6 @@ pub fn symbol_picker(cx: &mut Context) {

pub fn workspace_symbol_picker(cx: &mut Context) {
let doc = doc!(cx.editor);
let current_url = doc.url();
let language_server = language_server!(cx.editor, doc);
let offset_encoding = language_server.offset_encoding();
let future = language_server.workspace_symbols("".to_string());
Expand All @@ -171,7 +224,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) {
future,
move |_editor, compositor, response: Option<Vec<lsp::SymbolInformation>>| {
if let Some(symbols) = response {
let picker = sym_picker(symbols, current_url, offset_encoding);
let picker = sym_picker(symbols, offset_encoding, true);
compositor.push(Box::new(overlayed(picker)))
}
},
Expand Down Expand Up @@ -265,6 +318,7 @@ pub fn code_action(cx: &mut Context) {
},
)
}

pub fn execute_lsp_command(editor: &mut Editor, cmd: lsp::Command) {
let doc = doc!(editor);
let language_server = language_server!(editor, doc);
Expand Down Expand Up @@ -442,6 +496,7 @@ pub fn apply_workspace_edit(
}
}
}

fn goto_impl(
editor: &mut Editor,
compositor: &mut Compositor,
Expand Down Expand Up @@ -607,6 +662,7 @@ pub fn signature_help(cx: &mut Context) {
},
);
}

pub fn hover(cx: &mut Context) {
let (view, doc) = current!(cx.editor);
let language_server = language_server!(cx.editor, doc);
Expand Down Expand Up @@ -656,6 +712,7 @@ pub fn hover(cx: &mut Context) {
},
);
}

pub fn rename_symbol(cx: &mut Context) {
ui::prompt(
cx,
Expand Down