Using String as return type for Hover participants #582

Seiphon · 2019-10-22T07:00:25Z

No description provided.

fbricon

Can you please provide tests checking hover aggregation

fbricon · 2019-10-22T07:07:01Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+			}
+			return new MarkupContent(MarkupKind.PLAINTEXT, values.get(0));
+		}
+		String retValue = "";


Please use a StringBuilder

OK, I will uddate

angelozerr · 2019-10-22T07:48:31Z

...eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/extensions/IHoverParticipant.java

 /**
 * Hover participant API.
 *
 */
 public interface IHoverParticipant {

-	Hover onTag(IHoverRequest request) throws Exception;
+	String onTag(IHoverRequest request) throws Exception;


Add javadoc.

angelozerr · 2019-10-22T07:48:40Z

...eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/extensions/IHoverParticipant.java


-	Hover onAttributeName(IHoverRequest request) throws Exception;
+	String onAttributeName(IHoverRequest request) throws Exception;


Add javadoc.

angelozerr · 2019-10-22T07:48:53Z

...eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/extensions/IHoverParticipant.java


-	Hover onAttributeValue(IHoverRequest request) throws Exception;
+	String onAttributeValue(IHoverRequest request) throws Exception;


Add javadoc.

angelozerr · 2019-10-22T07:58:17Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+	 *         of the client.
+	 */
+	public static MarkupContent creatMarkupContent(List<String> values, IMarkupKindSupport request) {
+		if (values.size() ==1) {


MarkupKind kind = request.canSupportMarkupKind(MarkupKind.MARKDOWN) ? MarkupKind.MARKDOWN : MarkupKind.PLAINTEXT;

and after that you use this kind variable in the next code

angelozerr · 2019-10-22T08:00:23Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+			if (request.canSupportMarkupKind(MarkupKind.MARKDOWN)) {
+				return new MarkupContent(MarkupKind.MARKDOWN, values.get(0));
+			}
+			return new MarkupContent(MarkupKind.PLAINTEXT, values.get(0));


return new MarkupContent(kind, values.get(0));

angelozerr · 2019-10-22T08:00:46Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+		if (request.canSupportMarkupKind(MarkupKind.MARKDOWN)) {
+			return new MarkupContent(MarkupKind.MARKDOWN, retValue);
+		}
+		return new MarkupContent(MarkupKind.PLAINTEXT, retValue);


return new MarkupContent(kind, retValue);

angelozerr · 2019-10-22T08:02:37Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLHover.java

 				}
 			} catch (Exception e) {
 				LOGGER.log(Level.SEVERE, "While performing IHoverParticipant#onTag", e);
 			}
 		}
+		if (!contentValues.isEmpty()) {


share this code in private static method createHover to use it with other on* method.

angelozerr · 2019-10-22T08:05:43Z

...a/org/eclipse/lsp4xml/extensions/contentmodel/participants/ContentModelHoverParticipant.java

-				"Cannot process " + (e.isDTD() ? "DTD" : "XML Schema") + " hover: " + e.getMessage(),
-				MarkupKind.MARKDOWN, support);
-		return new Hover(content);
+		return MarkdownConverter.convert("Cannot process " + (e.isDTD() ? "DTD" : "XML Schema") + " hover: " + e.getMessage());


You need to check if client support markdown to convert it to markdown.

String value = "Cannot process " + (e.isDTD() ? "DTD" : "XML Schema") + " hover: " + e.getMessage(); return support.canSupportMarkupKind(MarkupKind.MARKDOWN) ? MarkdownConverter.convert(value) : value;

angelozerr · 2019-10-22T08:11:38Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+		}
+		String retValue = "";
+		for (String value : values) {
+			retValue = retValue + value + System.lineSeparator();


I think we should separate several hover value (only in markdown?) with '___' like vscode does
microsoft/vscode-languageserver-node#417 (comment)

We need to see the result in Eclipse IDE and vscode.

OK, got it. I will update this PR with all above problems resolved.

Hi, I am wandering if we use '___' to separate hover values, we need to handle it on LSP client, right? otherwise it will looks very ugly.

My idea was to generate line breaks (only if client can support markdown) https://www.markdownguide.org/basic-syntax/#horizontal-rules

angelozerr · 2019-10-23T07:32:39Z

@Seiphon could you merge your 3 commits please in one.

angelozerr · 2019-10-23T07:35:28Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+		StringBuilder retValue = new StringBuilder();
+		for (String value : values) {
+			retValue.append(value);
+			retValue.append("___");


What about with plaintext? I think we should generate that only for markdown and generate a System.lineSeparator() in another case.

Sorry just saw your last reply. I will update this PR.

PR has been updated.

angelozerr · 2019-10-23T08:03:03Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+		if (kind.equals(MarkupKind.MARKDOWN)) {
+			for (String value : values) {
+				retValue.append(value);
+				retValue.append("___");


Do this test here if (kind.equals(MarkupKind.MARKDOWN)) {

I think retValue.append(System.lineSeparator() ); must be done for all kind, no?

Yes, you are right. I will modify it.
For the test, it only test when kind is markdown. should I add test for plaintext kind?

we have not this kind of test. I think it's fine now. But please merge your 3 commits in once and after that I think we will able to merge your PR

angelozerr · 2019-10-23T08:03:16Z

org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/MarkupContentFactory.java

+			}
+		}
+		else {
+			for (String value : values) {


remove this code

angelozerr · 2019-10-23T09:31:38Z

...lsp4xml/src/test/java/org/eclipse/lsp4xml/services/extensions/AggregetedHoverValuesTest.java

@@ -0,0 +1,93 @@
+/**
+ *  Copyright (c) 2018 Angelo ZERR.


Add Liferay header or your name

OK, @angelozerr , one more thing.
I found if we only use one System.lineSeparator(), it does'nt work(on Eclipse). And if I use two of them, it will looks much better.

How do you think, should we use two line separator?

It seems LSP4E required that but what about with vscode?

Sorry, just saw it. I believe it won't hurt anything on vscode. If someday we find some problems which caused by it on vscode, we can go back here, and discuss about it.

angelozerr · 2019-10-23T14:10:17Z

Thanks @Seiphon !

Seiphon mentioned this pull request Oct 22, 2019

Aggregate result of Hover participants #577

Closed

fbricon requested changes Oct 22, 2019

View reviewed changes

angelozerr reviewed Oct 22, 2019

View reviewed changes

Seiphon requested a review from fbricon October 23, 2019 07:30

angelozerr reviewed Oct 23, 2019

View reviewed changes

Seiphon force-pushed the hover-return-type-change branch from 31f83d9 to 46d9018 Compare October 23, 2019 07:50

angelozerr reviewed Oct 23, 2019

View reviewed changes

Seiphon force-pushed the hover-return-type-change branch from 46d9018 to fbc8099 Compare October 23, 2019 08:28

angelozerr reviewed Oct 23, 2019

View reviewed changes

Using String as return type for Hover paricipants

2f2dab0

Seiphon force-pushed the hover-return-type-change branch from fbc8099 to 2f2dab0 Compare October 23, 2019 10:10

angelozerr changed the title ~~Using String as return type for Hover paricipants~~ Using String as return type for Hover participants Oct 23, 2019

angelozerr merged commit 0e09b21 into eclipse-lemminx:master Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using String as return type for Hover participants #582

Using String as return type for Hover participants #582

Seiphon commented Oct 22, 2019

fbricon left a comment

fbricon Oct 22, 2019

Seiphon Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019 •

edited

Loading

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

angelozerr Oct 22, 2019

Seiphon Oct 22, 2019

Seiphon Oct 23, 2019

angelozerr Oct 23, 2019

angelozerr commented Oct 23, 2019

angelozerr Oct 23, 2019

Seiphon Oct 23, 2019 •

edited

Loading

Seiphon Oct 23, 2019

angelozerr Oct 23, 2019

angelozerr Oct 23, 2019

Seiphon Oct 23, 2019

angelozerr Oct 23, 2019

angelozerr Oct 23, 2019

angelozerr Oct 23, 2019

Seiphon Oct 23, 2019 •

edited

Loading

Seiphon Oct 23, 2019

angelozerr Oct 23, 2019

Seiphon Oct 23, 2019 •

edited

Loading

angelozerr commented Oct 23, 2019


		Hover onAttributeName(IHoverRequest request) throws Exception;
		String onAttributeName(IHoverRequest request) throws Exception;


		Hover onAttributeValue(IHoverRequest request) throws Exception;
		String onAttributeValue(IHoverRequest request) throws Exception;

Using String as return type for Hover participants #582

Using String as return type for Hover participants #582

Conversation

Seiphon commented Oct 22, 2019

fbricon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelozerr commented Oct 23, 2019

Choose a reason for hiding this comment

Seiphon Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seiphon Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seiphon Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

angelozerr commented Oct 23, 2019

angelozerr Oct 22, 2019 •

edited

Loading

Seiphon Oct 23, 2019 •

edited

Loading

Seiphon Oct 23, 2019 •

edited

Loading

Seiphon Oct 23, 2019 •

edited

Loading