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

Support MarkupContent[] in a Hover #417

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
4 participants
@DJMcNab
Copy link
Contributor

DJMcNab commented Sep 23, 2018

Fixes #374 and Microsoft/language-server-protocol#518.

Supported by Microsoft/language-server-protocol#572

Note that StringOrContent is used to ease type-checking, as TS doesn't seem to be able to implicitely convert ls.MarkedString[] | ls.MarkupContent[] to (ls.MarkedString | ls.MarkupContent)[] for Array#map. This does change the semantic meaning of asHoverContent, but it's an internal function anyway, so ¯\_(ツ)_/¯.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Sep 23, 2018

CLA assistant check
All CLA requirements met.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Nov 20, 2018

@DJMcNab can you elaborate why this is necessary. The reason why I am asking is that this is a breaking change for a client and we need to provide capabilities for this.

If the reason for this is the rendering in the VS Code UI (horizontal lines) I think we are better off treating --- special and split the content on the client side into an array there.

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Nov 20, 2018

Certainly - the reason is for that UI effect. I created this PR mostly because you labelled #374 as 'feature-request', without raising this issue, which then seemed dormant and unactioned. I also encountered this limitation in a language server I am developing, where I had to fall back on the old deprecated API, which seemed wrong. However, seeing @felixfbecker's solution as laid out in Microsoft/language-server-protocol#518 (comment) makes me realise that splitting on the --- is a good idea.

However, the usual visual style of the --- is ugly, so perhaps it would be good to clarify that client implementations should use minimal styling for a --- in markdown for a hover.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Nov 21, 2018

@DJMcNab I think the styling of --- is very specific to VS Code and how it combines hovers from different providers. In VS Code for a language X there can be two hover providers which are separated using a hline. The API allows to separate section with a different styled hline by providing an array. IMO this shouldn't leak into the LSP API. So we should simple here https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/protocolConverter.ts#L257 convert the hover content into an array based on ---.

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Nov 21, 2018

So, just to clarify, I should close Microsoft/language-server-protocol#572 and instead make the change in https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/protocolConverter.ts#L257.

What would be the best approach to this - should I force push an update? Is it worth the slight jankiness of splitting based on the raw string ---. Alternatively, the actual vscode hover markdown support replace Horizontal Break with the thinner line style.

@DJMcNab DJMcNab force-pushed the DJMcNab:master branch from abb0f6f to 0779538 Nov 21, 2018

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Nov 21, 2018

There, I have added the suggested changes.

Note that Sourcegraph uses css rules to make the markdown content with horizontal seperators look the same as multiple array items: see sourcegraph/codeintellify@2ab2bf6.

@felixfbecker, do you think we should use this approach in vscode, although (I think) that would require a PR against Microsoft/vscode.

@dbaeumer dbaeumer merged commit 9049954 into Microsoft:master Dec 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@@ -257,7 +263,8 @@ export function createConverter(uriConverter?: URIConverter): Converter {
let result: code.MarkdownString;
switch (value.kind) {
case ls.MarkupKind.Markdown:
return new code.MarkdownString(value.value);
// https://github.com/Microsoft/vscode-languageserver-node/issues/374
return value.value.split(/\s*---\s*/g).map(asMarkdownString);

This comment has been minimized.

@felixfbecker

felixfbecker Dec 13, 2018

Contributor

Is this correct? What if --- appears in a code snippet, or in the middle of a sentence?

This comment has been minimized.

@dbaeumer

dbaeumer Dec 13, 2018

Member

I am currently fixing it :-) We need to consider *** and ___ as well and ignore html comments

dbaeumer added a commit that referenced this pull request Dec 13, 2018

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 13, 2018

@DJMcNab I reverted the merge. The think is more complicated than looking for ---

  • things we need to do: consider *** and ___ as well.
  • handle HTML comments correctly
  • test showed that the hline needs to start at the beginning of a line or has less than three spaces in front. But I am not sure this is standard.

Are you willing to continue on this?

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 13, 2018

Actually this is getting too complicated. We would need to handle hline as well and ignore --- in inline html. Doing this right would almost mean to convert the whole markdown. Will think about another solution.

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Dec 13, 2018

OK! Well I'm glad we're thinking about a solution! Don't worry about it.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 14, 2018

Fixed via: Microsoft/vscode#65094

The following code:

connection.onHover((textPosition): Hover => {
	// connection.tracer.log('A trace message from the server', 'Verbose content');
	return {
		contents: {
			kind: MarkupKind.Markdown,
			value: [
				'```typescript',
				'function validate(document: TextDocument): Diagnostic[]',
				'```',
				'___',
				'Some doc',
				'',
				'_@param_ `document` '
			].join('\n')
		}
	};
});

produces the following hover:

capture

@felixfbecker

This comment has been minimized.

Copy link
Contributor

felixfbecker commented Dec 14, 2018

Nice, that's the solution we chose in Sourcegraph too 👍

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Dec 14, 2018

Awesome work! Thanks.

Should we have a note about this in the protocol document - I suppose Microsoft/language-server-protocol#518 should track that?

Also, does tsserver currently use this or does it use the deprecated MarkedString[] approach.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 14, 2018

TSServer does use the deprecated MarkedString[] appraoch.

I also updated Microsoft/language-server-protocol#518

@DJMcNab what do you suggest to add to the protocol document ?

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Dec 14, 2018

Something like:

It is recommended that implementations display markdown seperators (such as denoted by --- or <hr/>) as a minimal horizontal line, as can be seen in vscode.

Maybe in slightly better English 😃.

@dbaeumer

This comment has been minimized.

Copy link
Member

dbaeumer commented Dec 17, 2018

I think this is not a good idea since the LSP purposely stayed away from defining how something should be rendered. If there is an editor that renders <hr/> as a bold thick line and does this consistently I have no problems with that from a LSP perspective.

@DJMcNab

This comment has been minimized.

Copy link
Contributor Author

DJMcNab commented Dec 17, 2018

Yeah, that's fine. It might be worth noting that --- (or <hr />) is the standard separator (such as between definitions and documentation or conflicting definitions), to clarify the situation for servers. Otherwise, it can just be left.

kenkangxgwe added a commit to kenkangxgwe/lsp-wl that referenced this pull request Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment