Skip to content
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

Show package declaration for the binary types in outline #3074

Closed
sandipchitale opened this issue Apr 18, 2023 · 14 comments · Fixed by eclipse-jdtls/eclipse.jdt.ls#2619
Closed

Comments

@sandipchitale
Copy link

The Outline view for java.io.PrintStream does show package node.

image

Environment
  • Operating System: All
  • JDK version: JDK17
  • Visual Studio Code version:

Version: 1.77.3 (system setup)
Commit: 704ed70d4fd1c6bd6342c436f1ede30d1cff4710
Date: 2023-04-12T09:16:02.548Z
Electron: 19.1.11
Chromium: 102.0.5005.196
Node.js: 16.14.2
V8: 10.2.154.26-electron.0
OS: Windows_NT x64 10.0.22621
Sandboxed: No

  • Java extension version: 1.18.2023041704
Steps To Reproduce
  1. Show java.io.PrintStream class
  2. Open Outline view
Current Result

The package node java.io is not shown.

Expected Result

The package node java.io should be shown.

@jdneo
Copy link
Collaborator

jdneo commented Apr 19, 2023

This looks like something by design.

The same behavior can be observed in Eclipse as well.
image

Actually all the IClassFile instance won't have the package declaration information from API level - when we call getChildren().

@jdneo jdneo added the upstream label Apr 19, 2023
@sandipchitale
Copy link
Author

sandipchitale commented Apr 20, 2023

I see. But showing it will be useful. Also it is useful to generate FQNs for setting method (function) breakpoints etc. For example: #1298

@jdneo jdneo changed the title The Outline view for java.io.PrintStream does show package node java.io Show package declaration for the binary types in outline Apr 21, 2023
@jdneo
Copy link
Collaborator

jdneo commented Apr 21, 2023

Marked as a feature request.

@sandipchitale
Copy link
Author

sandipchitale commented Apr 23, 2023

BTW FWIW I was able to figure out the package name by processing the location.uri:

        // Try to figure out package name from location.uri.path '/java.base/java.io/PrintStream.class'
        let packageName: string | undefined;
        if ((documentSymbol as any).location && (documentSymbol as any).location.uri.scheme === 'jdt') {
            const path = (documentSymbol as any).location.uri.path;
            const pathParts = path.split('/');
            if (pathParts.length > 2) {
                packageName = pathParts[2];
            }
        }

@jdneo
Copy link
Collaborator

jdneo commented Apr 23, 2023

Would you like to contribute a pull request for that?

@sandipchitale
Copy link
Author

sandipchitale commented Apr 25, 2023

@jdneo I will give it a try, but not sure what we would like to happen. Basically use the technique above and generate a package node and show it? If so I can give it a try.

@jdneo
Copy link
Collaborator

jdneo commented Apr 25, 2023

I think the above technique looks promising. And it would be even better if the change can happen at the server side so all clients can get it for free.

The code point is at: https://github.com/eclipse/eclipse.jdt.ls/blob/73ccd84d4a0f57312ece7c049c27c6e685a889fe/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/DocumentSymbolHandler.java#L150

Seems that we need some special logic to handle IClassFile and construct a package symbol according to the uri.

@sandipchitale
Copy link
Author

@jdneo Ah, you are suggesting to fix it on eclipse side. I will see if I can create a PR for that, i have done eclipse development in the past.

But it appears that thelocation.uri - jdt:///content/... already has some information about the package name in the uri.path, which the above technic is exploiting. What if we fix this on the client side in the interim by synthetically adding a DocumentSymbol for package using the above technic.

@jdneo
Copy link
Collaborator

jdneo commented Apr 25, 2023

I just took a look at the server-side implementation, and looks like there is no need to parse the uri string.

Even more simple: you can directly call unit.getParent().getElementName() to get the package name.

@sandipchitale
Copy link
Author

@jdneo Are you going to propose the above solution?

@jdneo
Copy link
Collaborator

jdneo commented Apr 25, 2023

unit.getParent().getElementName() makes more sense because the uri sent to client is built from that:

https://github.com/eclipse/eclipse.jdt.ls/blob/06716e21d336f8acbb5c99d29e19f252cfffe83a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java#L825

It would be awesome if you are interested in raising a PR for that. But it's totally fine if you don't have time to do so.

@sandipchitale
Copy link
Author

sandipchitale commented Apr 25, 2023

As a first step filed an issue: eclipse-jdtls/eclipse.jdt.ls#2617

I will look into the PR there.

@sandipchitale
Copy link
Author

Also see: microsoft/vscode-java-debug#1298

@sandipchitale
Copy link
Author

sandipchitale commented Jul 8, 2023

@jdneo @rgrunber Thanks for fixing this. I am able to use it in my VSCode extension VSCode Java Debugger Extras to set method (function) breakpoints in .class files as well (from a custom Outline view). The extension had to implement the custom Outline (Java Debugger) view as the standard Outline view does not allow adding node context actions. See: microsoft/vscode#49925). Hoping to eventually implement Class laoding, CTOR, field access and modification breakpoints once the underlying debugger supports them (See microsoft/vscode-java-debug#1298).

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants