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

Add support to view inherited members #2342 #2590

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gayanper
Copy link
Contributor

The reference view API is used to show the document outline with inherited
symbols using the new java.ls extension method.

Signed-off-by: Gayan Perera [email protected]

@gayanper
Copy link
Contributor Author

@rgrunber please have a look at the patch and let me know what you think ?

@gayanper gayanper force-pushed the extended_outline branch 2 times, most recently from dad7afa to 8d52ed3 Compare July 22, 2022 20:09
@jdneo
Copy link
Collaborator

jdneo commented Jul 27, 2022

Is it possible to display them in the outline view?

My thought is: we can register a switch button at the navigation bar of the outline view like this:

"menus": {
            "view/title": [
                {
                    "command": "<Enable Inherited Members Command>",
                    "when": "view == outline",
                    "group": "navigation"
                },
                ...
            ],
}

Then JDT.LS will provide different content based on current mode.

@gayanper
Copy link
Contributor Author

@jdneo You mean using the new ls extension method ?

@jdneo
Copy link
Collaborator

jdneo commented Jul 28, 2022

@gayanper

Not exactly. I'm thinking that we should use the existing LSP request for document symbols. At JDT.LS side, it will store a preference saying that which mode is enabled now (show inherited members or not) and return the response accordingly.

@gayanper
Copy link
Contributor Author

@jdneo ahhh i see, thats sounds neat. I can have a look at it this weekend.

@jdneo
Copy link
Collaborator

jdneo commented Jul 28, 2022

BTW, one thing might be tricky and risk: I'm not sure if it's possible to let outline view do a refresh programmatically. Since when the mode is changed, the content needs to do a refresh.

Seems impossible now: microsoft/vscode#108722

@gayanper
Copy link
Contributor Author

gayanper commented Jul 31, 2022

@jdneo the refreshing is a issue. One possibility is try to trigger a file change event, but I couldn’t find a way to that either.

Another issue is when we add the menu item into view/title it is also shown for other languages as well, I couldn’t find away to remove or disable it based on the active editor which is presented by outline.

Even finding a way to disable it, it will be shown for all languages. Therefore it seems to be better that showing inherited members comes from vscode it self so we have more control over the action/menus.

@jdneo
Copy link
Collaborator

jdneo commented Aug 1, 2022

Another issue is when we add the menu item into view/title it is also shown for other languages as well

Will adding editorLangId == typescript to the when clause works in this case?

But the 'refresh' blocks us anyway. What if we do not expose the shortcut at the navigation bar of the outline view? Instead, we just exposing a setting first.

Because when user is changing the setting's value, the user must switch between editors and the change of the editor will kick off a new document symbol request (if I remeber it correctly) and thus refresh the outline view?

@gayanper
Copy link
Contributor Author

gayanper commented Aug 1, 2022

Another issue is when we add the menu item into view/title it is also shown for other languages as well

Will adding editorLangId == typescript to the when clause works in this case?

No it doesn't work. Seems like editorLangId is not present I this view context.

But the 'refresh' blocks us anyway. What if we do not expose the shortcut at the navigation bar of the outline view? Instead, we just exposing a setting first.

Because when user is changing the setting's value, the user must switch between editors and the change of the editor will kick off a new document symbol request (if I remeber it correctly) and thus refresh the outline view?

I can try that out and see if it solves.

@gayanper
Copy link
Contributor Author

gayanper commented Aug 6, 2022

I tried the approach you mentioned @jdneo. We have few issues

  • When opening the file the symbols are cached, so basically you need to close and open editor to send another request to LS to consider the preference when loading symbols or the file must change.
  • The vscode outline UI always use the document uri to open the symbol. This is because it assume what is shown are only part of current document. So even we send SymbolInformation which has the support to send the URI it doesn't work when trying to select a symbol

Therefore I guess for now we can only have our own outline UI with our own LS extension since otherwise the document symbol quick picker will have symbols which cannot be navigated to.

WDYT ?

@jdneo
Copy link
Collaborator

jdneo commented Aug 8, 2022

@gayanper, thank you for trying that approach!

I filed an issue at VS Code repo about the uri support for document symbol: microsoft/vscode#157461

Therefore I guess for now we can only have our own outline UI with our own LS extension since otherwise the document symbol quick picker will have symbols which cannot be navigated to.

Yes, looks like in short term we have to forget about the integrated outline view 😥. The reference view in your initial commit might be an option.

@testforstephen @rgrunber What do you think? Should we go forward to use reference view to show the inherited members, or we wait for VS Code team's opinion a little bit then decide where will we go?

@manojbaishya
Copy link

I want to give some inputs on how Eclipse shows inherited fields and members:

Eclipse

Outline View

image

Type Hierarchy (with "Show all inherited members" enabled)

image

Navigation from Type Hierarchy panel

image

Visual Studio Code

Outline and References: Class Hierarchy

image

Navigation from Class Hierarchy

image

@jdneo
Copy link
Collaborator

jdneo commented Sep 7, 2022

The upstream request is in the stage of collecting upvotes now. For anyone want to have this capability, please vote (👍) at microsoft/vscode#157461.

@rgrunber
Copy link
Member

rgrunber commented Sep 8, 2022

The change works pretty well for me. I would be fine with reviewing the 2 contributions (JDT-LS + vscode-java) to at least get this merged until we can use the outline view instead.

We could then also add some of the options available to the outline view. (eg. sort elements, exclude fields, static members, non-public members, etc.)

Overall though, it would be nice to expose the "extended members" from the symbol outline of the command palette This is the closest thing to ctrl+o within Eclipse, and a faster way to navigate.

@manojbaishya
Copy link

Hello, can we please have this merged? Thanks!

The reference view API is used to show the document outline with inherited
symbols using the new java.ls extension method.

Signed-off-by: Gayan Perera <[email protected]>
@jdneo
Copy link
Collaborator

jdneo commented Nov 22, 2022

Looks like the API change for outline view won't happen in the near future, we can go with the reference view now. I'll review this PR recently.

package.json Show resolved Hide resolved
}

const params: DocumentSymbolParams = {
textDocument: TextDocumentIdentifier.create(this.location.uri.toString())
Copy link
Collaborator

@jdneo jdneo Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we do not persist the document location as a member, but always get the active document's uri (if it's available and is a java-like file).

By doing this, after user changes focus to another java file, he can click the refresh button to get the extended outline for the new file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do so will it be violating the intended API usage ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the API of the reference view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reference view's API does not ban us to do so?

What we need is just change textDocument: TextDocumentIdentifier.create(this.location.uri.toString()) to textDocument: TextDocumentIdentifier.create(window.activeEditor.document.uri.toString()) with some checks before to make sure the document is available and is a java-like file.

@@ -543,6 +544,17 @@ export class StandardLanguageClient {
}
}));

context.subscriptions.push(commands.registerCommand(Commands.SHOW_EXTEND_OUTLINE, (location: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides register a command to let user trigger manually. Is it possible that when we received an active editor change event (window.onDidChangeActiveTextEditor), we help user to auto refresh the extended outline view if it's expanded now. - need to check if the reference view api has such capability to let us check its expand status.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CsCherrYY Do you know how to get the expand status of the reference view?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no such API in specific reference view extension. Since reference view is a tree view isself, maybe there is some status we can get from that tree view?

@gayanper
Copy link
Contributor Author

@CsCherrYY @rgrunber @jdneo @testforstephen I would like to reiterate on this. I was looking into vscode quick outline and outline view are more build toward the document outline. Therefore my new suggestion is, we introduce outline hierarchy similar to call hierarchy or type hierarchy as a separate command. We can use the same reference view to start with.

WDYT ?

@jdneo
Copy link
Collaborator

jdneo commented Mar 27, 2023

@gayanper I'm fine with that approach. It looks like the VS Code team will expose the required APIs for the outline view. We can go with the reference view first.

@manojbaishya
Copy link

May this feature please be merged? :)

@rgrunber rgrunber self-assigned this May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants