Support multiple selection for generate accessors

[CsCherrYY]

This PR

• extracts some utility methods into SourceAssistUtility.java
• As for generating accessors:
  • for a field declaration has more than one field, if the focus is on the declaration itself, the quick assist will generate accessors for all fields, if the focus in on a VariableDeclarationFragment, the quick assist will generate the accessor for it only. (See the demo video)
  • for a selection contains more than one field declaration, the quick assist will generate accessors for all of them.

Signed-off-by: Shi Chen [email protected]

multiple-acceccor.mp4

[CsCherrYY]

test this please

[jdneo]

There is one problem about the insert position:

position.mp4

You can see that the inserted code locates before the cursor instead of after it. Meanwhile, the selection looks wired too. Only select the inserted part should be better, I think.

[CsCherrYY]

@jdneo I think I found the bug here. findElementAfterPosition() should use DiagnosticsHelper.getEndOffset(), not DiagnosticsHelper.getStartOffset(). https://github.com/eclipse/eclipse.jdt.ls/blob/e804ec31ecb8909384c253a49f2f2a4a1fdda4ee/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeGenerationUtils.java#L124-L135

[CsCherrYY]

now it looks like

selectbug.mp4

[jdneo]

Much better after using getEndOffset.

Just one more corner case:

I think the inserted code should before private String a in this case.

[CsCherrYY]

see https://github.com/eclipse/eclipse.jdt.ls/blob/e804ec31ecb8909384c253a49f2f2a4a1fdda4ee/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeGenerationUtils.java#L113

The start position of a is equal with the end of the selection offset, so we ignore it here. if we can make it <= this case could be handled properly.

[jdneo]

LGTM!

[rgrunber]

@CsCherrYY , @jdneo . Just a question about this. @jasonhy-wang rightly pointed out that on recent versions, one can no longer activate code action from pri|vate String someField; and expect to see the Generate Getter and Setter for 'someField'. I found this PR and figured it probably became tricky to distinguish between single and multi-variable declarations, which would be needed. Is this a bug or a known limitation ?

[CsCherrYY]

@rgrunber It's a known limitation. In this PR we introduced three cases to get the field names from a given AST node: https://github.com/eclipse/eclipse.jdt.ls/blob/1226891c0f5002cb8b49ab0794f4616a525e2ef0/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/CodeActionUtility.java#L89-L106, the case when the node is a Modifier is not supported here.

A potential fix is to check the parent of the node, when it's a FieldDeclaration we can directly return the result of getFieldNamesFromASTNode(parent). This would also fix other unexpected cases IMO. If that's the case, I can create a new PR to fix this.

[rgrunber]

It's a bit of a corner case given how many ways there are to generate the getters/setters, so it's lower priority. The only thing is you'd also have to check the value of java.quickfix.showAt.

Update: I've created an issue to at least track that we're aware of this.