Completing /** should generate Javadoc with params #744

snjeza · 2018-08-03T21:06:19Z

Signed-off-by: Snjezana Peco [email protected]

fbricon · 2018-08-07T15:49:34Z

@snjeza I haven't checked yet but is there a way for the server, through LSP config/requests to perform what the client does in https://github.com/redhat-developer/vscode-java/blob/1f6783957c699e261a33d05702f2da356017458d/src/extension.ts#L263-L303?

Because your solution is to generate code completion items, which gives a different UX than the expected autocompletion

snjeza · 2018-08-07T19:07:46Z

The feature I have created works in a similar way as the VS Code typescript feature does.
See https://github.com/Microsoft/vscode/blob/master/extensions/typescript-language-features/src/features/jsDocCompletions.ts
I think we can't implement this feature on the VS Code client side because the client doesn't know arguments of Java methods.

fbricon · 2018-08-07T19:09:57Z

Of course it can't be implemented on the client side, I just wondered whether we could move that part (API implementation) currently on the client side to be processed in the backend.

fbricon · 2018-08-07T19:52:23Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+				// ignore
+			}
+			final CompletionItem ci = new CompletionItem();
+			ci.setInsertText(buf.toString());


insertText is deprecated, you should use a TextEdit.

Also, according to the spec, for completionItem textEdits:

The range of the edit must be a single line range and it must contain the position at which completion has been requested

That means each extra line must be added by an additional textedit

Only the range must be a single line range.

fbricon · 2018-08-07T19:55:13Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+import org.eclipse.lsp4j.CompletionItemKind;
+import org.eclipse.lsp4j.InsertTextFormat;
+
+public class JavaDocCompletionProposal {


How hard is it to refactor JavaDocAutoIndentStrategy in jdt.ui down to jdt.core?
cc @rgrunber @jjohnstn

There are a few eclipse.jface.text interfaces that would need to move to eclipse.text. There is a reference to StubUtility and CodeGeneration which is still in the works by Roland. There are two methods (isSmartMode() and getCompilationUnit()) that drag in all the eclipse.ui stuff such as PlatformUI and IEditorPart. The class could be split into core and non-core and the smartMode setting plus ICompilationUnit could be set for core so as to avoid eclipse.ui. There are also a few Preferences to reference/set like we have done before (e.g. Member sort order).

There are a lot of changes due to which, I think, we won't be able to use the same code in jdt.ls and jdt.ui

@jjohnstn thanks for looking into it. Given the amount of changes and @snjeza's feedback, I'm fine keeping a customized version of that JavaDocAutoIndentStrategy in jdt.ls.

fbricon · 2018-08-07T20:08:57Z

...clipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java

@@ -319,6 +320,30 @@ public void testCompletion_import_package() throws JavaModelException{
 		//Not checking the range end character
 	}

+	@Test
+	public void testCompletion_javadocComment() throws JavaModelException {


maybe add some more tests for:

already existing javadoc

partially existing javadoc (has only 1 @param doc'ed)

no completion after regular comment /* */

no completion for no-param methods

fbricon · 2018-08-07T20:21:10Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+			ci.setSortText(SortTextHelper.convertRelevance(0));
+			ci.setKind(CompletionItemKind.Snippet);
+			ci.setInsertTextFormat(InsertTextFormat.Snippet);
+			ci.setDocumentation(ci.getInsertText());


example documentation returned: "documentation": " \n * @param foo\n * @param bar\n"
Leading empty lines should be trimmed from documentation

fbricon · 2018-08-07T21:00:46Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

@@ -0,0 +1,235 @@
+/*******************************************************************************
+ * Copyright (c) 2016-2017 Red Hat Inc. and others.


Original copyright should be retained

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+ * Based on org.eclipse.jdt.internal.ui.text.javadoc.JavaDocAutoIndentStrategy
+ *
+ * Contributors:
+ *     IBM Corporation - initial API and implementation


fbricon · 2018-08-08T13:32:04Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+		}
+		List<CompletionItem> result = new ArrayList<>();
+		IDocument d = JsonRpcHelpers.toDocument(cu.getBuffer());
+		if (offset == -1 || d.getLength() == 0) {


fbricon · 2018-08-08T20:09:24Z

Indentation looks weird

In the generated javadoc, I don't see why we're adding trailing spaces on the 1st line:

    /**     
     * @param toto
     * @param tatata
     */
    @Deprecated
    public void name(String toto, String tatata) {

    }

edit:

"newText": "    \n * @param toto\n * @param tatata"

fbricon · 2018-08-08T20:10:46Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+			ci.setSortText(SortTextHelper.convertRelevance(0));
+			ci.setKind(CompletionItemKind.Snippet);
+			ci.setInsertTextFormat(InsertTextFormat.Snippet);
+			String documentation = buf.toString().replaceFirst("\\n", "");


No, this leads to an ugly indentation. 1st empty lines should be removed

fbricon · 2018-08-08T20:14:21Z

All lines should be trimmed on the right.

fbricon · 2018-08-08T23:46:25Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+			ci.setLabel(JAVA_DOC_COMMENT);
+			ci.setSortText(SortTextHelper.convertRelevance(0));
+			ci.setKind(CompletionItemKind.Snippet);
+			ci.setInsertTextFormat(InsertTextFormat.Snippet);


CompletionItemKind.Snippet is fine, but I don't think InsertTextFormat.Snippet is appropriate here, since there will be no placeholders. Clients not supporting Snippets might puke when receiving those items. We should send InsertTextFormat.PlainText

fbricon · 2018-08-08T23:48:01Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+			}
+			final CompletionItem ci = new CompletionItem();
+			Range range = JDTUtils.toRange(unit, offset, 0);
+			ci.setTextEdit(new TextEdit(range, buf.toString()));


textedit should also get the rtrim treatment here.

fbricon · 2018-08-08T23:49:28Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+			buf.append(rtrim(line));
+			buf.append(lineDelimiter);
+		}
+		return buf.toString().replaceFirst(lineDelimiter, "");


only if lineDelimiter position is 0

fbricon · 2018-08-08T23:51:17Z

...clipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java

+		assertEquals(CompletionItemKind.Snippet, item.getKind());
+		assertEquals("999999999", item.getSortText());
+		assertNotNull(item.getTextEdit());
+		assertEquals("	\n * @param i\n * @param s\n", item.getTextEdit().getNewText());


new text should be "\n * @param i\n * @param s\n" (no leading spaces)

fbricon · 2018-08-09T13:23:55Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavaDocCompletionProposal.java

+
+public class JavaDocCompletionProposal {
+
+	public static final String JAVA_DOC_COMMENT = "JavaDoc comment";


s/JavaDoc/Javadoc/g

no camelcase in Javadoc

fbricon · 2018-08-09T21:11:33Z

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavadocCompletionProposal.java

+		return result;
+	}
+
+	private String rtrim(String text) {


what is implemented is a ltrim, i.e. whitespaces on the left are removed. We want to remove the trailing spaces, those on the right

StringUtils.stripEnd(text, null) should work

fbricon · 2018-08-09T23:50:04Z

Trimming is much better BUT I think it now introduces an extra line separator, when completing an existing javadoc block (I think this was not happening in the previous commits)

When completing from the initial opening block, there's no gap in the * delimiters

fbricon · 2018-08-20T22:20:08Z

So the new implementation now adds an extra empty comment line before the parameters :-/
Which made me realize, we could, if the client support snippets, add a $0 placeholder on that 1st empty line, so the user would be able to write javadoc directly after inserting the template.

But if no snippet is supported, then we shouldn't add that extra 1st empty line

Signed-off-by: Snjezana Peco <[email protected]>

snjeza · 2018-08-21T19:47:07Z

test this please

snjeza · 2018-08-21T19:47:47Z

@fbricon I have updated the PR.

fbricon · 2018-08-21T23:37:48Z

Works really nicely now! Nice job @snjeza

snjeza requested review from gorkem and fbricon August 3, 2018 21:06

fbricon reviewed Aug 7, 2018

View reviewed changes

fbricon added this to the End August 2018 milestone Aug 7, 2018

fbricon added enhancement content-assist labels Aug 7, 2018

snjeza force-pushed the issue-228 branch from b365196 to 76ad72b Compare August 7, 2018 22:47

fbricon reviewed Aug 8, 2018

View reviewed changes

snjeza force-pushed the issue-228 branch from 76ad72b to f68bfbb Compare August 8, 2018 19:52

fbricon reviewed Aug 8, 2018

View reviewed changes

snjeza force-pushed the issue-228 branch from f68bfbb to 5953dc7 Compare August 8, 2018 22:43

fbricon reviewed Aug 8, 2018

View reviewed changes

snjeza force-pushed the issue-228 branch 2 times, most recently from a2d9b34 to 4ac94d1 Compare August 9, 2018 00:18

fbricon reviewed Aug 9, 2018

View reviewed changes

snjeza force-pushed the issue-228 branch from 4ac94d1 to 87ba981 Compare August 9, 2018 18:51

fbricon reviewed Aug 9, 2018

View reviewed changes

snjeza force-pushed the issue-228 branch 2 times, most recently from cd62ae9 to 50807c7 Compare August 9, 2018 23:09

snjeza force-pushed the issue-228 branch 3 times, most recently from 03e485f to 0177969 Compare August 13, 2018 22:47

Completing /** should generate Javadoc with params

024f52d

Signed-off-by: Snjezana Peco <[email protected]>

snjeza force-pushed the issue-228 branch from 0177969 to 024f52d Compare August 21, 2018 18:57

fbricon merged commit 13d7e46 into eclipse-jdtls:master Aug 21, 2018

q12321q mentioned this pull request Nov 24, 2018

Completing /** should generate Javadoc with params neoclide/coc-java#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completing /** should generate Javadoc with params #744

Completing /** should generate Javadoc with params #744

snjeza commented Aug 3, 2018

fbricon commented Aug 7, 2018

snjeza commented Aug 7, 2018

fbricon commented Aug 7, 2018

fbricon Aug 7, 2018

snjeza Aug 7, 2018

fbricon Aug 7, 2018 •

edited

Loading

jjohnstn Aug 7, 2018

snjeza Aug 7, 2018

fbricon Aug 7, 2018

fbricon Aug 7, 2018

fbricon Aug 7, 2018

fbricon Aug 7, 2018

This comment was marked as resolved.

fbricon Aug 8, 2018

fbricon commented Aug 8, 2018

fbricon Aug 8, 2018

fbricon commented Aug 8, 2018

fbricon Aug 8, 2018

fbricon Aug 8, 2018

fbricon Aug 8, 2018

fbricon Aug 8, 2018 •

edited

Loading

fbricon Aug 9, 2018

fbricon Aug 9, 2018 •

edited

Loading

fbricon Aug 9, 2018

fbricon commented Aug 9, 2018

fbricon commented Aug 20, 2018

snjeza commented Aug 21, 2018

snjeza commented Aug 21, 2018

fbricon commented Aug 21, 2018

		@@ -0,0 +1,235 @@
		/*******************************************************************************
		* Copyright (c) 2016-2017 Red Hat Inc. and others.


		public class JavaDocCompletionProposal {

		public static final String JAVA_DOC_COMMENT = "JavaDoc comment";

Completing /** should generate Javadoc with params #744

Completing /** should generate Javadoc with params #744

Conversation

snjeza commented Aug 3, 2018

fbricon commented Aug 7, 2018

snjeza commented Aug 7, 2018

fbricon commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Aug 7, 2018 • 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

This comment was marked as resolved.

Choose a reason for hiding this comment

fbricon commented Aug 8, 2018

Choose a reason for hiding this comment

fbricon commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbricon commented Aug 9, 2018

fbricon commented Aug 20, 2018

snjeza commented Aug 21, 2018

snjeza commented Aug 21, 2018

fbricon commented Aug 21, 2018

fbricon Aug 7, 2018 •

edited

Loading

fbricon Aug 8, 2018 •

edited

Loading

fbricon Aug 9, 2018 •

edited

Loading