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

GH-707: Introduced the @theia/typehierarchy extension. #3148

Closed
wants to merge 0 commits into from

Conversation

kittaakos
Copy link
Contributor

  • Extended the LSP with the type hierarchy.
  • From now on, a tree node can carry decoration data.
  • Added editor access for the current/active editors.
  • Wired the JDT LS type hierarchy into the application.

Closes: #707.

Signed-off-by: Akos Kitta kittaakos@typefox.io

Ready for preview:
screencast 2018-10-11 15-18-32

@AlexTugarev
Copy link
Contributor

screen shot 2018-10-11 at 15 55 08

just started testing... looks awesome!

@kittaakos
Copy link
Contributor Author

When restarting the Theia backend without refreshing the browser:

root ERROR RangeError: Maximum call stack size exceeded
    at http://localhost:3000/11.bundle.js:3892:19
    at Map.forEach (<anonymous>)
    at SubTypeHierarchyFeature.push.../../node_modules/vscode-base-languageclient/lib/client.js.TextDocumentFeature.dispose (http://localhost:3000/11.bundle.js:3891:25)
    at SubTypeHierarchyFeature.push.../../packages/languages/lib/browser/typehierarchy/typehierarchy-feature.js.TypeHierarchyFeature.dispose (http://localhost:3000/36.bundle.js:381:34)
    at http://localhost:3000/36.bundle.js:628:42
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:100223:39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:100303:36)
    at SubTypeHierarchyFeature.push.../../packages/languages/lib/browser/typehierarchy/typehierarchy-feature.js.TypeHierarchyFeature.dispose (http://localhost:3000/36.bundle.js:380:32)
    at http://localhost:3000/36.bundle.js:628:42
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:100223:39)
root ERROR RangeError: Maximum call stack size exceeded
    at http://localhost:3000/11.bundle.js:3892:19
    at Map.forEach (<anonymous>)
    at SubTypeHierarchyFeature.push.../../node_modules/vscode-base-languageclient/lib/client.js.TextDocumentFeature.dispose (http://localhost:3000/11.bundle.js:3891:25)
    at SubTypeHierarchyFeature.push.../../packages/languages/lib/browser/typehierarchy/typehierarchy-feature.js.TypeHierarchyFeature.dispose (http://localhost:3000/36.bundle.js:381:34)
    at http://localhost:3000/36.bundle.js:628:42
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:100223:39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:100303:36)
    at SubTypeHierarchyFeature.push.../../packages/languages/lib/browser/typehierarchy/typehierarchy-feature.js.TypeHierarchyFeature.dispose (http://localhost:3000/36.bundle.js:380:32)
    at http://localhost:3000/36.bundle.js:628:42
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:100223:39)

Possible related: #3150, #3145. Something is odd with the reconnection in Java.


child.unbind(TreeImpl);
child.bind(TypeHierarchyTree).toSelf();
child.rebind(Tree).toDynamicValue(ctx => ctx.container.get(TypeHierarchyTree));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: #3153

toDynamicValue -> toService.

@simark
Copy link
Contributor

simark commented Oct 12, 2018

Could you point me to the latest version of the LSP proposal for type hierarchy? I would like to test this with clangd, if I have the time.

@kittaakos
Copy link
Contributor Author

latest version of the LSP proposal for type hierarchy

microsoft/vscode-languageserver-node#346

Note, this implementation differs from the proposed one.


Extracted from the source:

Or in Java/Xtend: eclipse-lsp4j/lsp4j#273


registerMenus(menus: MenuModelRegistry): void {
super.registerMenus(menus);
const menuPath = [...EDITOR_CONTEXT_MENU, 'navigation'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should go into its own 'type-hierarchy' whatever menu group, instead of the 'navigation'

return <div className={'symbol-icon ' + this.icons.get(node.kind) || 'unknown'}></div>;
}
// tslint:disable-next-line:no-null-keyword
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be undefined. See: #3181

@kittaakos
Copy link
Contributor Author

kittaakos commented Oct 17, 2018

I have noticed another UX issue, we should not reveal the document symbol, when expanding the nodes in the type hierarchy tree.

Update:
This is a generic issue with the tree widgets without "multi-selection" support. We fire a selection change event when we expand the node. Perhaps, it is out of the context of this PR.

@AlexTugarev
Copy link
Contributor

I have noticed another UX issue, we should not reveal the document symbol, when expanding the nodes in the type hierarchy tree.

I see the same behavior with the call hierarchy. And after a quick comparison to Rider, I tend to agree that this is an UX issue. Personally I'd expect to reveal the symbol on selection. Unfortunately, expansion is equal to selection in trees.

We fire a selection change event when we expand the node. Perhaps, it is out of the context of this PR.

... but an interesting topic.

@simark
Copy link
Contributor

simark commented Oct 18, 2018

One thing to look out for: make sure this plays well with restarting language servers.

If you rebase on top of #3215, you can try restarting a language server (I tested with Java, so you should be able to reproduce with that). I get infinite recursion during dispose:

ss

@kittaakos
Copy link
Contributor Author

One thing to look out for: make sure this plays well with restarting language servers.

#3148 (comment)

If you rebase on top of #3215,

Why to rebased? The reconnecting issue happens from the master branch too: #3145 (comment)

@simark
Copy link
Contributor

simark commented Oct 18, 2018

Oh sorry, I missed that comment. Well, it gives you a more straightforward way to reproduce :).

@kittaakos
Copy link
Contributor Author

The PR: #3802.

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

Successfully merging this pull request may close these issues.

3 participants