Skip to content

honour hover.content_format client capability #6140

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.

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

Merged
merged 4 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 45 additions & 6 deletions crates/ide/src/hover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use test_utils::mark;
use crate::{
display::{macro_label, ShortLabel, ToNav, TryToNav},
link_rewrite::{remove_links, rewrite_links},
markdown_remove::remove_markdown,
markup::Markup,
runnables::runnable,
FileId, FilePosition, NavigationTarget, RangeInfo, Runnable,
Expand All @@ -27,6 +28,7 @@ pub struct HoverConfig {
pub debug: bool,
pub goto_type_def: bool,
pub links_in_hover: bool,
pub markdown: bool,
}

impl Default for HoverConfig {
Expand All @@ -37,6 +39,7 @@ impl Default for HoverConfig {
debug: true,
goto_type_def: true,
links_in_hover: true,
markdown: true,
}
}
}
Expand All @@ -48,6 +51,7 @@ impl HoverConfig {
debug: false,
goto_type_def: false,
links_in_hover: true,
markdown: true,
};

pub fn any(&self) -> bool {
Expand Down Expand Up @@ -91,6 +95,7 @@ pub(crate) fn hover(
db: &RootDatabase,
position: FilePosition,
links_in_hover: bool,
markdown: bool,
) -> Option<RangeInfo<HoverResult>> {
let sema = Semantics::new(db);
let file = sema.parse(position.file_id).syntax().clone();
Expand All @@ -109,7 +114,9 @@ pub(crate) fn hover(
};
if let Some(definition) = definition {
if let Some(markup) = hover_for_definition(db, definition) {
let markup = if links_in_hover {
let markup = if !markdown {
remove_markdown(&markup.as_str())
} else if links_in_hover {
rewrite_links(db, &markup.as_str(), &definition)
} else {
remove_links(&markup.as_str())
Expand Down Expand Up @@ -147,7 +154,11 @@ pub(crate) fn hover(
}
};

res.markup = Markup::fenced_block(&ty.display(db));
res.markup = if markdown {
Markup::fenced_block(&ty.display(db))
} else {
ty.display(db).to_string().into()
};
let range = sema.original_range(&node).range;
Some(RangeInfo::new(range, res))
}
Expand Down Expand Up @@ -383,12 +394,12 @@ mod tests {

fn check_hover_no_result(ra_fixture: &str) {
let (analysis, position) = fixture::position(ra_fixture);
assert!(analysis.hover(position, true).unwrap().is_none());
assert!(analysis.hover(position, true, true).unwrap().is_none());
}

fn check(ra_fixture: &str, expect: Expect) {
let (analysis, position) = fixture::position(ra_fixture);
let hover = analysis.hover(position, true).unwrap().unwrap();
let hover = analysis.hover(position, true, true).unwrap().unwrap();

let content = analysis.db.file_text(position.file_id);
let hovered_element = &content[hover.range];
Expand All @@ -399,7 +410,18 @@ mod tests {

fn check_hover_no_links(ra_fixture: &str, expect: Expect) {
let (analysis, position) = fixture::position(ra_fixture);
let hover = analysis.hover(position, false).unwrap().unwrap();
let hover = analysis.hover(position, false, true).unwrap().unwrap();

let content = analysis.db.file_text(position.file_id);
let hovered_element = &content[hover.range];

let actual = format!("*{}*\n{}\n", hovered_element, hover.info.markup);
expect.assert_eq(&actual)
}

fn check_hover_no_markdown(ra_fixture: &str, expect: Expect) {
let (analysis, position) = fixture::position(ra_fixture);
let hover = analysis.hover(position, true, false).unwrap().unwrap();

let content = analysis.db.file_text(position.file_id);
let hovered_element = &content[hover.range];
Expand All @@ -410,7 +432,7 @@ mod tests {

fn check_actions(ra_fixture: &str, expect: Expect) {
let (analysis, position) = fixture::position(ra_fixture);
let hover = analysis.hover(position, true).unwrap().unwrap();
let hover = analysis.hover(position, true, true).unwrap().unwrap();
expect.assert_debug_eq(&hover.info.actions)
}

Expand All @@ -433,6 +455,23 @@ fn main() {
);
}

#[test]
fn hover_remove_markdown_if_configured() {
check_hover_no_markdown(
r#"
pub fn foo() -> u32 { 1 }

fn main() {
let foo_test = foo()<|>;
}
"#,
expect![[r#"
*foo()*
u32
"#]],
);
}

#[test]
fn hover_shows_long_type_of_an_expression() {
check(
Expand Down
4 changes: 3 additions & 1 deletion crates/ide/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod syntax_highlighting;
mod syntax_tree;
mod typing;
mod link_rewrite;
mod markdown_remove;

use std::sync::Arc;

Expand Down Expand Up @@ -376,8 +377,9 @@ impl Analysis {
&self,
position: FilePosition,
links_in_hover: bool,
markdown: bool,
) -> Cancelable<Option<RangeInfo<HoverResult>>> {
self.with_db(|db| hover::hover(db, position, links_in_hover))
self.with_db(|db| hover::hover(db, position, links_in_hover, markdown))
}

/// Computes parameter information for the given call expression.
Expand Down
23 changes: 23 additions & 0 deletions crates/ide/src/markdown_remove.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! Removes markdown from strings.

use pulldown_cmark::{Event, Parser, Tag};

/// Removes all markdown, keeping the text and code blocks
///
/// Currently limited in styling, i.e. no ascii tables or lists
pub fn remove_markdown(markdown: &str) -> String {
let mut out = String::new();
let parser = Parser::new(markdown);

for event in parser {
match event {
Event::Text(text) | Event::Code(text) => out.push_str(&text),
Event::SoftBreak | Event::HardBreak | Event::Rule | Event::End(Tag::CodeBlock(_)) => {
out.push('\n')
}
_ => {}
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 you should also push * for list items. Otherwise

  • foo
  • bar

would be rendered as

foo
bar

which doesn't look like a list at all. You should also render tables using ascii art I think.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where it goes, but is it possible to preserve the empty line in the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lnicola in the screenshot? that might be possible, and indeed look better.

@bjorn3 that would be a good idea, but i think this is enough when only the hover implementation uses it, as it only uses simple markdown. I can add a comment stating the lack off styling

Copy link
Member

Choose a reason for hiding this comment

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

Event::End(Tag::CodeBlock(_))

Was the empty line missing otherwise?

Looks good to me, (with the exception of the two boolean arguments function, which I'm not a fan of but is probably fine for now), but I didn't test it.

}
}

out
}
6 changes: 5 additions & 1 deletion crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use ide::{
AssistConfig, CompletionConfig, DiagnosticsConfig, HoverConfig, InlayHintsConfig,
MergeBehaviour,
};
use lsp_types::ClientCapabilities;
use lsp_types::{ClientCapabilities, MarkupKind};
use project_model::{CargoConfig, ProjectJson, ProjectJsonData, ProjectManifest};
use rustc_hash::FxHashSet;
use serde::Deserialize;
Expand Down Expand Up @@ -327,13 +327,17 @@ impl Config {
debug: data.hoverActions_enable && data.hoverActions_debug,
goto_type_def: data.hoverActions_enable && data.hoverActions_gotoTypeDef,
links_in_hover: data.hoverActions_linksInHover,
markdown: true,
};

log::info!("Config::update() = {:#?}", self);
}

pub fn update_caps(&mut self, caps: &ClientCapabilities) {
if let Some(doc_caps) = caps.text_document.as_ref() {
if let Some(value) = doc_caps.hover.as_ref().and_then(|it| it.content_format.as_ref()) {
self.hover.markdown = value.contains(&MarkupKind::Markdown)
}
if let Some(value) = doc_caps.definition.as_ref().and_then(|it| it.link_support) {
self.client_caps.location_link = value;
}
Expand Down
6 changes: 5 additions & 1 deletion crates/rust-analyzer/src/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,11 @@ pub(crate) fn handle_hover(
) -> Result<Option<lsp_ext::Hover>> {
let _p = profile::span("handle_hover");
let position = from_proto::file_position(&snap, params.text_document_position_params)?;
let info = match snap.analysis.hover(position, snap.config.hover.links_in_hover)? {
let info = match snap.analysis.hover(
position,
snap.config.hover.links_in_hover,
snap.config.hover.markdown,
)? {
None => return Ok(None),
Some(info) => info,
};
Expand Down