[READY] Implement type/call hierarchy handling #4221
Conversation
This makes the popup disappear if you keep typing or enter inser mode or move the cursor etc. I found it jarring that previously it would just move the cursor behind the popup and such until you hit escape. Makes the popup more "modal" but without actually stopping you from continueing. Also: - Use simliar up/down keys as the symbol finder (c-p, c-k, c-n, c-j etc.) - SetupFoo -> SetUpFoo because the verb is "To set up" (rather than the noun "a setup". - purge barbaric 80+ character lines
We try to be a bit consistent with the finder poppup, but implementation-wise this is simpler. The idea is that there are 3 columns, each having 1/3 of the popup width. We fix the width of the popup (like we do for the finder) and set the tabstop to 1/3 of the internal width (core_width). Then when displaying text, we truncate "columnns" according to that tabstop (to avoid mess). To do this, we pass structured data from the python layer to vimscript and construct the line text there. This will also help later when we add in the syntax highlight (text properties) like we have for the finder popup.
This basically involves moving the properties mapping to its own place, and creating text properties to overlap the various parts of the popup columns. Style matches, and feels correct. Fiddly part is that we have to (for some reason) set cursorline every time we move the cursor, but ain't nobody gonna be able to explain why, and why that only necessary after launching the finder popup window once. Some wierd vim bug is my guess.
This makes the columns seem less broken, particularly for leading whitespace. This is particularly problematic due to our use of tabstop: if the leading whitespace was a tab, it would go nuts. It would still go nuts for any other literal tab in the description. Perhaps we should fix that too.
bstaletic
force-pushed
the
hierarchies
branch
3 times, most recently
from
7e16c1f
to
9d8d267
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):
if a:kind == 'call' let lines_and_handles = py3eval( \ 'ycm_state.InitializeCurrentHierarchy( ycm_state.SendCommandRequest( ' .
Rather than having magic in SendCommandRequest to skip the "result" handler, shouldn't we just call "GetCommandResponse" which is ... for that exact purpose (get the result without handling it?) same below
python/ycm/client/command_request.py line 222 at r2 (raw file):
buffer_command = DEFAULT_BUFFER_COMMAND, extra_data = None, skip_post_command_action = False ):
what's the difference between SendCommandRequest with skip_post_command_action=True and GetCommandResponse ?
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
There was a problem hiding this comment.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
a discussion (no related file):
Need to update README
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)
Previously, when changing direction of hierarchy and re-rooting the hierarchy tree, we would retain the old node as the new root. That works most of the time, but fails in the face of multiple inheritance and switching to supertypes. In that, problematic, case, the resulting hierarchy will be missing some supertypes. Worse, if a call hierarchy contains a node with multiple locations, re-rooting to that node behaved in a way that defies explanation. The solution is to throw away everything and start a new hierarchy completely every time a user requests re-rooting. This trades some performance for correctness, but this is not at all performance sensitive.
There was a problem hiding this comment.
I have some tests locally, but...
not quite LGTM. There's one more bug to solve.
Re-rooting the call hierarchy , such that we go from asking for foo's callers to foo's callees is broken.
Solving this will require a slight API modification.
The "incoming calls" response looks something like this:
{
'from': hierarchy_item
'fromRanges': locations
}locations is what ycmd API currently provides.
hierarchy_item['range'] is what we need when going from callers to callees.
Example:
int f();
int g() {
return f() + f();
}Asking for callers of f returns something like this:
{
'from': hierarchy_item # hierarchy_item['range'] represents `g` location
'fromRanges': [
first_f,
second_f
]
}If g should be the new root, we definitely want the from.range.start for the new request. Currently, instead, we get fromRanges[N].
All that to say... what should the additional location be called in the ycmd API?
from is just an awful property name.
parent_location? Seems very specific to incoming calls. (we have no use for it, but we can provide the location of to in outgoing calls.)
root_location? Seems like it should only be used when changing root. (That's the only current purpose.)
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)
a discussion (no related file):
Previously, puremourning (Ben Jackson) wrote…
Need to update README
Done, thanks to your pull request.
autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Rather than having magic in SendCommandRequest to skip the "result" handler, shouldn't we just call "GetCommandResponse" which is ... for that exact purpose (get the result without handling it?) same below
Magic is gone, but like I said, I think we want another utility in command_request.py.
python/ycm/youcompleteme.py line 147 at r4 (raw file):
response = self.GetCommandRequest( request_id ).Response() self.FlushCommandRequest( request_id ) return response
Here's an example of how I'm currently performing sync requests.
This is the need for a new utility.
Locally I have implemented that new utility and called it GetRawCommandResponse().
Would that be a good name?
python/ycm/youcompleteme.py line 441 at r4 (raw file):
final_arguments = [] for argument in arguments: if isinstance( argument, str ):
Now that I'm passing hierarchy items (dict) as an argument for /run_completer_command, we now need this check as well.
Yet, this function will never add any extra_data to these requests.
Taking into account the GetRawCommandResponse(), we could just skip calling _GetCommandResponseArguments() and then revert this change.
python/ycm/client/base_request.py line 263 at r4 (raw file):
'column_num': column, 'working_dir': GetCurrentDirectory(), 'file_data': file_data
Really not sure if file_data is correct here. I think it is...
The point of this change is, when re-rooting the hierarchy, we do not want a request for the cursor location, but whatever is in the hierarchy node.
Also, location being a (file, line, column) tuple seems ugly.
At one point I have named it HierarchyLocation, but that's even worse - why would BuildRequestDataForLocation know about hierarchy types!
python/ycm/client/command_request.py line 222 at r2 (raw file):
Previously, puremourning (Ben Jackson) wrote…
what's the difference between SendCommandRequest with skip_post_command_action=True and
GetCommandResponse?
GetCommandResponse returns a string and returns an empty string for lists. I.e. request.Response() vs request.StringResponse().
Right now, I have done away with it, but am using SendCommandRequestAsync and immediately calling Response() on it.
We probably want an additional utility in our command_request.py.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)
autoload/youcompleteme/hierarchy.vim line 42 at r2 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Magic is gone, but like I said, I think we want another utility in
command_request.py.
As agreed, I have added GetRawCommandResponse.
python/ycm/client/base_request.py line 263 at r4 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Really not sure if
file_datais correct here. I think it is...The point of this change is, when re-rooting the hierarchy, we do not want a request for the cursor location, but whatever is in the hierarchy node.
Also,
locationbeing a(file, line, column)tuple seems ugly.
At one point I have named itHierarchyLocation, but that's even worse - why wouldBuildRequestDataForLocationknow about hierarchy types!
This is definitely broken when location refers to a buffer we have never opened.
python/ycm/client/command_request.py line 222 at r2 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
GetCommandResponsereturns a string and returns an empty string for lists. I.e.request.Response()vsrequest.StringResponse().Right now, I have done away with it, but am using
SendCommandRequestAsyncand immediately callingResponse()on it.We probably want an additional utility in our
command_request.py.
Added GetRawCommandResponse().
In order to send a ycmd request for an unloaded buffer one must first... load the buffer. That is the only way to reliably determine the buffer's filetype. However, we do not want to switch to the buffer, so everything needs to be done "in the background". `:badd` adds a buffer as unloaded, if it were not present before. `bufload()` loads a buffer that has previously not been loaded.
Previously all new nodes were counted as 1. With call hierarchies, there may be more locations/lines per hierarchy node. In that case, the cursorline would be set to the wrong line. Instead, we need to add up the lengths of all the item locations.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4221 +/- ##
==========================================
+ Coverage 89.67% 89.78% +0.11%
==========================================
Files 34 37 +3
Lines 4445 4779 +334
==========================================
+ Hits 3986 4291 +305
- Misses 459 488 +29 |
Changelog: 1. Upgrade all subservers 2. Hierarchy support
bstaletic
changed the title
There was a problem hiding this comment.
Updated to the latest ycmd.
Still need help with the tests. Manually performing the steps in the tests works.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)
Previously the tests were using async result checking, but this is intended for insert mode only. In fact we were running off the end of the test and then the check callbacks were firing. Simplified by just making it the same but sequential, and replacding FeedAndCheck* with direct calls to feedkeys(..., 'xt').
indexof was added so recently that even vim-helptag-versions website doesn't have it. Replace with a loop.
There were some unnecessary `WaitForAssert()` calls and unnecessary `silent` commands.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic)
python/ycm/client/base_request.py line 253 at r5 (raw file):
def BuildRequestDataForLocation( location ): file, line, column = location
any reason not to accept file, line, location as arguments here rather than a tuple? Callers can always do *location
python/ycm/client/base_request.py line 255 at r5 (raw file):
file, line, column = location current_buffer = vim.current.buffer vim.command( f'badd {file }' )
may be slightly more consistent to use vim.eval( 'bufadd(...)' ) rather than cmd/function mix.
I'm also fairly sure that either this or the buflload can throw vim.error E325 ATTENTION! if there is a swap file.
python/ycm/client/base_request.py line 257 at r5 (raw file):
vim.command( f'badd {file }' ) vim.eval( f'bufload( "{ file }" )' ) buffer_number = vimsupport.GetBufferNumberForFilename( file )
Looking at this, I recall that this function has a create_buffer_if_needed argument. We could do that (first) and then call bufload, skipping badd call above.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r4, 3 of 7 files at r5, 3 of 3 files at r6, 3 of 4 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)
python/ycm/client/base_request.py line 253 at r5 (raw file):
Previously, puremourning (Ben Jackson) wrote…
any reason not to accept
file, line, locationas arguments here rather than a tuple? Callers can always do*location
No good reason. Fixed.
python/ycm/client/base_request.py line 255 at r5 (raw file):
Previously, puremourning (Ben Jackson) wrote…
may be slightly more consistent to use
vim.eval( 'bufadd(...)' )rather than cmd/function mix.I'm also fairly sure that either this or the buflload can throw vim.error E325 ATTENTION! if there is a swap file.
Dropped badd in favour of GetBufferNumberForFile() with create_if_needed = True.
E325 is handled. It can only be thrown from bufload(). I hope I have handled it correctly.
python/ycm/client/base_request.py line 257 at r5 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Looking at this, I recall that this function has a
create_buffer_if_neededargument. We could do that (first) and then call bufload, skippingbaddcall above.
Good catch.
Done.
- `badd` is unnecessary since we have `GetBufferNumberForFilename()` - Instead of passing `location`, we can pass `file`, `line` and `column` separately.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)
|
Thanks for sending a PR! |
PR Prelude
Thank you for working on YCM! :)
Please complete these steps and check these boxes (by putting an
xinsidethe brackets) before filing your PR:
rationale for why I haven't.
actually perform all of these steps.
Why this change is necessary and useful
This is the client side pull request related to ycm-core/ycmd#1733
Again, tests and documentations are missing. Documentation is the easy part, but tests would be dreadful...
popup look
I have not paid much attention to the look of the popup.
The look is slightly off - for deep hierarchies the location of references in the file (
foo.cpp:8for example) becomes misaligned.The context is is not right aligned either.
Basically, it's functional but ugly. If someone would be up for it, please send patches my way.
How to use this thing?
The only publicly available function here is
youcompleteme#hierarchy#StartRequest( kind ). It takes'call'or'type'as the argyment. From there, YCM will fire a "prepare hierarchy" request and then open a popup with the results.While the popup is open, the following keys are used:
<Tab>to resolve a hierarchy item in the "subtypes" / "incoming" direction.<S-Tab>to resolve a hierarchy item in the "supertypes"/"outgoing" direction.<Up>/<Down>to select items in the popup.<CR>to jump to the selected item and close the popup.<Esc>to close the popup without jumping.Maybe we want more key bindings here?
<C-n>/<C-p>andj/kcome to mind...Oh yes, there's also
<plug>(YCMCallHierarchy)and<plug>(YCMTypeHierarchy)that just callyoucompletem#hierarchy#StartRequest(). Users should be using these, instead of directly calling the function.This change is