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

[WIP] cmd/govim: experiment with call hierarchy support #1015

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

Conversation

leitzler
Copy link
Member

@leitzler leitzler commented Jan 12, 2021

This is an experiment for how Call Hierarchy UX could be implemented. Don't mind the ugly code, it is a quick hack to just to try out different things.

It introduces a new command, :GOVIMCallHierarchy that must be called with one argument. The argument must be either in, out or goto.

Calling :GOVIMCallHierarchy out with the cursor on a function will bring up all outgoing calls from that function. The same way goes for :GOVIMCallHierarchy in, but for incoming calls to that function.

The list of calls is populated in a newly created buffer named govim-callhierarchy that acts as a tree. Each line is a call and lines prefixed with + can be further expanded by calling the same :GOVIMCallHierarchy again.

The tree view looks much like the screenshots from VSCode from the CL that added support in gopls, CL 247699: https://i.imgur.com/tHzx8yn.png & https://i.imgur.com/gjshuaj.png. I did put some effort into how the UX should look like and ended up with this. Any ideas of improvements are very welcome!

You can also call the "other" :GOVIMCallHierarchy command on one of the lines in the call hierarchy buffer, e.g. if it was opened with to show all outgoing requests you can call :GOVIMCallHierarchy in to set the current entry as a new root for incoming calls.

:GOVIMCallHierarchy goto will only have an effect when called from the call hierarchy buffer, and it jumps to the definition of the current entry.

Highlighting has been added mostly for debugging, but could be used if they add anything.

@leitzler
Copy link
Member Author

Simple example:
asciicast

Slightly more complex example:
asciicast

@myitcv
Copy link
Member

myitcv commented Jan 14, 2021

Thanks very much for investigating this. I'll take a look as soon as I can.

@myitcv
Copy link
Member

myitcv commented Jan 24, 2021

This looks fantastic. Some drive-by comments (please bear in mind that I have not looked at the code):

  1. presumably it would be possible to mouse enable stuff here? For those setups that support mouse, how about single click means expand (i.e. repeat previous call), double click means goto, control-click (or similar) means "do the other action" (i.e. do out in case we previous did in, and vice versa)
  2. in terms of UI/UX I have no strong opinions. Leveraging/copying existing patterns of presentation of tree data is probably a good step - what have you done here? Might be nice to add some detail in the PR description of the approach you've followed, in case others want to contribute ideas/suggestions etc (edited)
  3. the filenames being included in the detail panel doesn't feel right. Does pkg.Identifier not suffice? Again, there is overlap here from a presentation perspective with what symbol search does
  4. the highlighting is excellent
  5. the order of the results in the detail window does not seem to follow the execution order? (at least in my local test) (edited)

Have you messaged in the govim channel to ask for feedback? I can see plenty of people wanting this functionality.

@leitzler
Copy link
Member Author

Thanks for taking a look! And the code is still hack-ish so it is a good think you didn't waste time on it yet, this was more if the concept holds.

  1. presumably it would be possible to mouse enable stuff here? For those setups that support mouse, how about single click means expand (i.e. repeat previous call), double click means goto, control-click (or similar) means "do the other action" (i.e. do out in case we previous did in, and vice versa)

I haven't looked into mouse integration much in vim at all, I'll take a look. I don't think it should be any issues adding that.

  1. in terms of UI/UX I have no strong opinions. Leveraging/copying existing patterns of presentation of tree data is probably a good step - what have you done here? Might be nice to add some detail in the PR description of the approach you've followed, in case others want to contribute ideas/suggestions etc (edited)

The tree view looks much like the screenshots from VSCode that was included in the CL that added support in gopls, CL 247699: https://i.imgur.com/tHzx8yn.png & https://i.imgur.com/gjshuaj.png. Would be nice with other suggestions, I'll add something about this to the description.

  1. the filenames being included in the detail panel doesn't feel right. Does pkg.Identifier not suffice? Again, there is overlap here from a presentation perspective with what symbol search does

I fully agree. The reason to why the filename is there is that it is bundled with the package identifier:

{
    To: protocol.CallHierarchyItem{
        Name:   "NewEncoder",
        Kind:   12,
        Tags:   nil,
        Detail: "encoding/json • stream.go",
        URI:    "file:///Users/leitzler/sdk/gotip/src/encoding/json/stream.go",
        Range:          // .. stripped 
        SelectionRange: // .. stripped
        Data: nil,
    },
    FromRanges:         // .. stripped
}

Probably worth raising an issue to gopls regardless, even if it is a part of aDetail field it is redundant when URI is there as well.

  1. the highlighting is excellent

Thanks. It has been very helpful for debugging and as I already know how it works it is a nice subtle aid in general.
Since it was tricky to describe in text I didn't know if it was helpful when now knowing how it works up front.

  1. the order of the results in the detail window does not seem to follow the execution order? (at least in my local test) (edited)

I recall the response from gopls not being consistent so I added an alphabetic sort, execution order is probably more intuitive for "Outgoing calls". Any suggestions for how to sort "Incoming calls" would be appreciated. I have no strong opinions at all, I guess the good thing about alphabetic is that it is consistent for both in/out.

@myitcv
Copy link
Member

myitcv commented Jan 24, 2021

Thanks for taking a look! And the code is still hack-ish so it is a good think you didn't waste time on it yet, this was more if the concept holds.

The perfect way of evaluating an idea to my mind!

I haven't looked into mouse integration much in vim at all, I'll take a look. I don't think it should be any issues adding that.

Great. For some reason I suspect I would use the mouse in this instance - I don't know why, especially because I'm generally 100% keyboard in Vim.

The tree view looks much like the screenshots from VSCode that was included in the CL that added support in gopls, CL 247699: https://i.imgur.com/tHzx8yn.png & https://i.imgur.com/gjshuaj.png. Would be nice with other suggestions, I'll add something about this to the description.

Great. It honestly seems to work really well as it is... but I'm all for making it even better.

I fully agree. The reason to why the filename is there is that it is bundled with the package identifier:

{
    To: protocol.CallHierarchyItem{
        Name:   "NewEncoder",
        Kind:   12,
        Tags:   nil,
        Detail: "encoding/json • stream.go",
        URI:    "file:///Users/leitzler/sdk/gotip/src/encoding/json/stream.go",
        Range:          // .. stripped 
        SelectionRange: // .. stripped
        Data: nil,
    },
    FromRanges:         // .. stripped
}

Probably worth raising an issue to gopls regardless, even if it is a part of aDetail field it is redundant when URI is there as well.

Agreed.

I recall the response from gopls not being consistent so I added an alphabetic sort, execution order is probably more intuitive for "Outgoing calls". Any suggestions for how to sort "Incoming calls" would be appreciated. I have no strong opinions at all, I guess the good thing about alphabetic is that it is consistent for both in/out.

I thought about this a bit more. It's quite a tricky one - because I'm sure my brain will expect to see things in execution order, but given there could be multiple calls to a given function/method for an out query, and those will be grouped, I'm not really sure what to expect. Hence perhaps lexicographic order makes most sense. But then something else jarring will be what to order by - the method/function name? If so, won't that jar with the package/receiver name?

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.

None yet

2 participants