-
Notifications
You must be signed in to change notification settings - Fork 401
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
GH-28: Added support for the super- and subtype hierarchies. #823
Conversation
decc6f6
to
3efbce6
Compare
</location> | ||
<location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit"> | ||
<unit id="org.eclipse.xtend.sdk.feature.group" version="2.14.0.v20180523-0937"/> | ||
<unit id="org.eclipse.xtext.sdk.feature.group" version="2.14.0.v20180523-0937"/> | ||
<unit id="org.eclipse.wst.xml_ui.feature.feature.group" version="3.10.0.v201804210200"/> |
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.
should not be necessary anymore, since we now only add the m2e-apt core bundle to the TP 74a0993
I have updated this PR based on the feedback we got for the LSP modifications. I am currently experiencing build issues locally, it might be related to Xtext. |
How can I test it? |
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/TypeHierarchyHandler.java
Show resolved
Hide resolved
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/TypeHierarchyHandler.java
Show resolved
Hide resolved
* The location, encapsulating the range enclosing this symbol not including | ||
* leading/trailing whitespace but everything else like comments. | ||
*/ | ||
public static Location getFullLocation(IJavaElement element) throws JavaModelException { |
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.
similar static methods related to location computation live in JDTUtils. I reckon it's getting bloated over time, so we might get a dedicated LocationUtils class to handle all that
return null; | ||
} | ||
try { | ||
Location fullLocation = DocumentSymbolHandler.getFullLocation(type); |
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.
should not reference DocumentSymbolHandler here. use utility class instead
|
||
public TypeHierarchyHandler(PreferenceManager preferenceManager) { | ||
this.preferenceManager = preferenceManager; | ||
enabled = Suppliers.memoize(() -> this.preferenceManager.getClientPreferences().isTypeHierarchySupported()); |
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.
not convinced this simple call needs to be memoized
you might want to change copyright headers for new files to 2019. |
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/JDTLanguageServer.java
Show resolved
Hide resolved
It's not yet ready, sorry for the noise. I added the [WIP] tag to the title. I had to push to see if the build failure is local or not. I will update this PR once more with some hints on how to try it. |
0b0783f
to
7888ade
Compare
You can build it from source, but you need Python 2.x, yarn, etc.. or you could try to open it in Gitpod.
Note, if the If you have any question, or you stuck, let me know. |
An easier way; here is Gitpod snapshot containing all the steps I have described above. |
*/ | ||
private TypeHierarchyItem shallowCopy(TypeHierarchyItem other) { | ||
TypeHierarchyItem copy = new TypeHierarchyItem(); | ||
other.setName(other.getName()); |
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.
FIXME: must be copy
instead of other
.
…ies. Closes: eclipse-jdtls#28. Signed-off-by: Akos Kitta <[email protected]>
I have rebased this PR from the Please note, although the proposed LSP extension has not been accepted by MS yet, this PR complies the proposed changes. |
wait this feature long time. |
@fbricon can some one handle the confilict files? wait this feature long time? |
@yudar1024 I want to wait for the spec to be finalized before merging this feature. See microsoft/vscode-languageserver-node#426 |
@fbricon can you merge first to let developer can use it in early time like clangd, merge it at this time or after spec be finalize, we both need change the code if the spec make some change. but if we merge at this time , at least it can make vsc more efficient early |
Hi @kittaakos Are you still interested in this feature? I think it's especially useful and I'm interested in it. I found you closed microsoft/vscode-languageserver-node#426 which is a proposal of type hierarchy in LSP. Although it will not be available in LSP recently, VS Code allows extensions to contribute to the tree view and here is a sample. We could put the type hierarchy in the tree view, use a private command to exchange data instead of LSP currently. What do you think about it? |
I'm going to implement this feature and make it available first. In the future, we can move to LSP once it supports type hierarchy. I notice the data models such as In VS Code side, References Viewlet API offers an API to contribute to the reference view. Type hierarchy data can be shown in the reference view with this, just like how call hierarchy works today. Do you have any comment or suggestion on this? |
Superseded by #1656 |
Note: This item depends on eclipse-lsp4j/lsp4j#272 due to required API changes. Still, this PR is ready for review/preview.Done.Closes: #28.
Signed-off-by: Akos Kitta [email protected]
For reviewers:
I start working on a client implementation, once it is available, I will update this PR.A client implementation with Eclipse Theia is available here.Why did I add? The TP related change is not required anymore. See GH-28: Added support for the super- and subtype hierarchies. #823 (review).org.eclipse.wst.xml_ui.feature.feature.group
to the TPhttp://download.eclipse.org/releases/photon/
p2 site.org.jboss.tools.maven.apt.feature.feature.group
depends onorg.eclipse.wst.xml.ui
that we used to pull fromhttp://download.eclipse.org/releases/photon/
under the hood.org.eclipse.wst.xml_ui.feature.feature.group
feature dependency to fulfill the missingo.e.wst.xml.ui
plug-in.Edit: Updated the section for the TP.