Type hierarchy support

[HighCommander4]

This is a first draft which I'm posting for feedback.

Currently, the type hierarchy view only shows derived classes (Children direction), which is the more useful one (since base classes can be navigated to from the base clauses in the class definition).

We will need to make a UX choice around how to incorporate the Parents direction as well. My suggestion is:

• Stick to just one view and one command to activate it.
• The view shows one of the directions at a time, and Indicates which one it's currently showing.
• There is a button to cause the view to switch to showing the other direction.

This is how vscode's built-in call hierarchy view works (for showing callers vs. callees), and given the similarity in the protocols, I am venturing a guess that this is how vscode's built-in type hierarchy view (which doesn't exist yet, but will eventually replace this when it does) will work as well.

Thoughts / feedback on this direction is welcome.

[sam-mccall]

Oh, this is neat!

Random observations from playing with it:

• having it display test.c:3 is much less useful than having it jump there (I think single-click is fine, like VSCode's call hierarchy). Once this happens we can probably drop the filename.
• mirroring call hierarchy for the directions seems like a reasonable call, though I didn't find that UI particularly intuitive. It's really tempting to include nodes for the parents (in the case where there's exactly one parent)
• the "reveal" isn't working for me: it focuses the left panel, but doesn't expand or focus the type hierarchy section.
• the context menu item should probably be in the references section if possible

I haven't looked deeply at the code but it looks reasonable.
This really makes me want to export "show AST" as a real feature...

[HighCommander4]

Thanks for the feedback! I pushed an update that addresses many of the comments.

• having it display test.c:3 is much less useful than having it jump there (I think single-click is fine, like VSCode's call hierarchy). Once this happens we can probably drop the filename.

I implemented single-click navigation.

The filename and line number are still there. I'm somewhat ambivalent about removing them, as they can still be useful (e.g. you could have different derived classes by the same name in different files). Perhaps keeping the filename only would make sense?

• mirroring call hierarchy for the directions seems like a reasonable call

Haven't implemented this yet, will do in the next update.

It's really tempting to include nodes for the parents (in the case where there's exactly one parent)

I agree it's really tempting. But what do you do when there are multiple parents? If you show just one, or none, we might be misleading the user.

• the "reveal" isn't working for me: it focuses the left panel, but doesn't expand or focus the type hierarchy section.

Yeah, the logic in the first draft only caused the view to be shown (i.e. have it appear in the panel), but not expanded or focused.

I tried harder in this draft, should expand + focus now.

• the context menu item should probably be in the references section if possible

Fixed.

This really makes me want to export "show AST" as a real feature...

Just as another consumer of the tree view facility? Or is there some other connection?

[HighCommander4]

• mirroring call hierarchy for the directions seems like a reasonable call

Haven't implemented this yet, will do in the next update.

This is done now as well. I think the patch should be ready for review.

[sam-mccall]

The filename and line number are still there. I'm somewhat ambivalent about removing them, as they can still be useful

The filename will be truncated most of the time: the panel is by default quite narrow, with indentation followed by the arrow indicator, followed by the name of the class, and finally the filename. If either the classname or the filename are long at all, it's going to be truncated. Once the extension is truncated it loses a lot of value as you can't tell it's a filename.

I really think this is just going to be visual noise. It's also inconsistent with other places where we show classnames (like the workspace symbols panel) where the small grey text is the namespace rather than the filename.

I agree it's really tempting. But what do you do when there are multiple parents? If you show just one, or none, we might be misleading the user.

I think we could show a dummy node "multiple parents" - it's not really elegant, but this is a rare case and it'd be great to avoid the awkward "reverse direction" interaction in the common case.

[HighCommander4]

Addressed review comments.

[HighCommander4]

I agree it's really tempting. But what do you do when there are multiple parents? If you show just one, or none, we might be misleading the user.

I think we could show a dummy node "multiple parents" - it's not really elegant, but this is a rare case and it'd be great to avoid the awkward "reverse direction" interaction in the common case.

I implemented this in a separate PR: https://github.com/HighCommander4/vscode-clangd/pull/1/commits

[HighCommander4]

I implemented this in a separate PR: https://github.com/HighCommander4/vscode-clangd/pull/1/commits

Er, whoops, didn't mean to create a PR against my own fork... let's try that again: #44

[HighCommander4]

I implemented this in a separate PR: https://github.com/HighCommander4/vscode-clangd/pull/1/commits

Er, whoops, didn't mean to create a PR against my own fork... let's try that again: #44

Not quite wanted I wanted either, as that PR contains both commits.

Sorry, the GitHub isn't strong with me: what's the proper way to create a PR against this main fork which depends on another PR (i.e. this one), and only contains that second commit?

[Trass3r]

Not possible. isaacs/github#959 (comment)

[HighCommander4]

Review ping :)

[hokein]

sorry, I planned to do the review, but somehow got distracted and forgot it. Will try to review it today or tomorrow.

Sorry, the GitHub isn't strong with me: what's the proper way to create a PR against this main fork which depends on another PR (i.e. this one), and only contains that second commit?

I think you could create two branches in "clangd/vscode-clangd" repo (you should have permission), and send a PR against two branches.

[HighCommander4]

I think you could create two branches in "clangd/vscode-clangd" repo (you should have permission), and send a PR against two branches.

Thanks, will try that next time. For now, if you'd like to see the parts of the patch that have been split out to implement the "try to show parents as well" suggestion, you can look at just the second commit in #44.

[hokein]

please also resolve the clang-format issue on the CI.

btw, I saw an exception in the debug console when playing around this patch locally, no sure which operation caused it.

Clang Language Server is now active!
extension.js:135
rejected promise not handled within 1 second: Error: TreeError [clangd.typeHierarchyView] Data tree node not found: [object Object]
extensionHostProcess.js:976
stack trace: Error: TreeError [clangd.typeHierarchyView] Data tree node not found: [object Object]
    at V.getDataNode (file:///Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:1125:302)
    at V.reveal (file:///Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:1124:883)
    at q.reveal (file:///Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:5414:769)
    at p.reveal (file:///Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:4248:298)
    at file:///Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/workbench.desktop.main.js:4247:442
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)

[HighCommander4]

Thanks for the review, I believe I addressed all of the comments.

btw, I saw an exception in the debug console when playing around this patch locally, no sure which operation caused it.

Based on the stack trace, it is probably caused by the call to this.treeView.reveal() in TypeHierarchyProvider.reveal(). However, I have not been able to trigger it.

It's worth noting that #44 makes changes that could fix this (because we now have to support reveal() for any node, not just the root, and accordingly it implements getParent() for all nodes).

[hokein]

Thanks, I think it is good to go. just a few more nits.

btw, I saw an exception in the debug console when playing around this patch locally, no sure which operation caused it.
Based on the stack trace, it is probably caused by the call to this.treeView.reveal() in TypeHierarchyProvider.reveal(). However, I have not been able to trigger it.

I saw it again today, but no luck to get stable reproduce steps (and it doesn't seem to hurt the functionality). Let's see whether it'll disappear after #44.

btw, please don't force-push commits, it makes reviewers harder to see the difference, I'd just append new commits.

[HighCommander4]

btw, please don't force-push commits, it makes reviewers harder to see the difference, I'd just append new commits.

Sorry, still getting used to the GitHub workflow which seems quite different from the Phabricator one (where I do --amend after each set of comments and resubmit).

I assume I should squash the commits together before merging, though, to avoid polluting the repo history with "Address review comments" commits.