-
Notifications
You must be signed in to change notification settings - Fork 822
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
[NETBEANS-6425] Provide outline view for Groovy file in VSCode. #3525
Conversation
There are no tests yet, I need to write them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great refactoring, just get ready for evolution of StructureElement
. And write at least one test!
...va.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/TextDocumentServiceImpl.java
Show resolved
Hide resolved
There is now test for the api. |
case KEYWORD: return Kind.Key; | ||
case OTHER: return Kind.Object; | ||
case PACKAGE: return Kind.Package; | ||
case PARAMETER: return Kind.TypeParameter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is PARAMETER
really in csl langugaes used for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on the language.
createTags(item, builder); | ||
|
||
try { | ||
String prefix = doc.getText(lineStart, startOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lineStart
was assigned the value of startOffset
(line 210), but was never changed after that -- maybe swap lines 219 & 220 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code i a little bit misleading. I have change it little bit. I will try to explain the logic:
class A {
| def methodC {} {
}
}
The |
marks text cursor, in this case the logic counts the expanded offset from the beginning of the line, becase on the line starts methodC
definition. So in this case the methodC element is selected. But when you have something like:
class A{
int field1; |int field2
}
the start of the expanded offset will be the same as selected offset of the element field2
and not from the begining of the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is useful for example for yaml files like:
martin:
name: Martin D'vloper
| job: Developer
skill: Elite
The ui will select the job
item, not martin
structure (that will be selected if the job expanded offsets will be the same lake selected offsets.
} else if (lastElement.getExpandedStartOffset() <= lineStart && lineEnd == lastElement.getExpandedEndOffset()) { | ||
// The same line | ||
sElements.remove(lastElement); | ||
Builder leBuilder = StructureProvider.newBuilder(lastElement.getName(), lastElement.getKind()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about adding Builder StructureProvider.copy(StructureElement from)
? All the settings can be copied, the caller will just call builder methods for changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this as well. I will do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be in the next commit.
if (ds != null) | ||
StructureProvider structureProvider = MimeLookup.getLookup(DocumentUtilities.getMimeType(doc)).lookup(StructureProvider.class); | ||
if (structureProvider != null) { | ||
return structureProvider.getStructure(doc).thenApply(structureElements -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementations (java, GSF) in this PR will actually block, not fork; so this will likely block the LSP message thread until the parsing is done. I was about to suggest in the GSF impl that it should fork the .parse() into a RP, but maybe it would be better to do the fork here, centrally (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have put it into the Background taks.
builder.children(jse); | ||
} | ||
} | ||
if (path.getLeaf().getKind() == Tree.Kind.METHOD || path.getLeaf().getKind() == Tree.Kind.VARIABLE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about initializers ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have copied the original code here. I didn't changed the logic.
|
||
private final String name; | ||
private final String detail; | ||
private final int selectionStartOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A weird idea: if javax.swing.text.Position
is used here, the structure could survive some edits between the time StructureElement is created and transofmed into LSP4J wire format -- and during the translation the document could be readlocked to protect the consistency. @JaroslavTulach ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the structure to represent read-only snapshot and be always re-created from scratch? Or do we want the structure to be updated?
I believe we strive for the first approach. As such making the fields final
more properly represents the snapshot nature of the structure. Implementations can of course maintain javax.swing.text.Position
internally.
This is the outlineview implementation not only for Groovy files, but for all GSF languages that are/will be enabled in the Vs Code extension. At the moment it is for Groovy files and YAML files that have
application
orbootstrap
prefix.The implementation requires adding two interfaces to the LSP API. Clients can implement and register a
StructureProvider
intoMimeLookup
for a given mimetype.StructureProvider
provides a list of top symbols to be displayed in the outline view. The symbol structure is composed ofStrctureElements
.StructureElements are then translated into
DocumentSymbol
s on the VSCode API side, which are displayed in the outline view and also in Go to a symbol (CTRL+SHIFT+O by default).DocumentSymbol
has two methodsgetName()
andgetDetail()
, which both returnString
and the resulting compound text is displayed in the outline view, where the text returned bygetName()
is slightly larger than the text returned bygetDetail()
. On the other hand, only the text returned bygetName()
is displayed in Go to a symbol. TheDocumentSymbol
documentation says thatgetName()
should return the name of the symbol andgetDetail()
should return the detail of the symbol, such as the signature of the function.Currently in outline view we display the signature as the symbol name and the type as the symbol detail for java files. This means that in Go to a symbol we display not only the names but the symbol, but also signatures with the types of the function parameters, which can affect the symbol search. You can see on this picture, what I mean: https://issues.apache.org/jira/secure/attachment/13039469/Screenshot%20from%202022-01-28%2011-05-32.png
On the other hand, the java extension offered by Microsoft does the same. However, in my opinion,
getName
should really return only the name of the symbols and this would make the search in Go to symbol easier.This PR also moves the existing functionality that provided outline view for java files outside the LSP server. The JavaStructureProvider implementation is now in the java.editor module, which already has a dependency on the LSP API. Provider should logically belong to java.navigation module, but this module is not enabled for VsCode extension and if we enable it, it brings with it dependency on WindowsSystem.API, which is not sufficient now.