zed-industries
Fix autocomplete completions being cut in half
Release Notes:
- Fixed LSP completions being cut in half (#8126).
Previously, autocomplete suggestions were covered by autocomplete documentation, which only appeared after a short delay.
Now, when an autocomplete suggestion is too long to fit, the name is truncated with ellipses like how VSCode does it:
Additionally completion_documentation_secondary_query_debounce's default was changed from 300ms to 0ms, which makes the editor feel significantly faster (in my opinion).
Before:
https://github.com/zed-industries/zed/assets/50590465/6443670b-fe25-4428-9a39-54405d9a7cec
After:
https://github.com/zed-industries/zed/assets/50590465/72572487-3eb4-4a96-a2f9-608e563a1f05
I’d love to see the results of testing this on the typescript LSP. I believe that debounce was added because autocomplete while typing in typescript would constantly flicker back and forth
@mikayla-maki I tested with TS and had no (new) issues.
However
[2024-02-24T12:55:02-07:00 ERROR util] crates/project/src/project.rs:5107: Request completionItem/resolve failed with message: Cannot read properties of undefined (reading 'file')
was printed to console multiple times (regardless of completion_documentation_secondary_query_debounce's value). That said, this was also occurring in Zed-Preview, so it seems unrelated. I've only seen this error while editing TypeScript.
That said, I did notice some spacing issues I need to fix before this can be merged. Some LSP documentation should be left-justified, and others right-justified, but this only works for right-justified documentation.
Passing by, but would like to notice that I would prefer splitting the debounce out of this PR: mixing different UI and functional changes seem dangerous, esp. given that either of this may spoil something for other languages and have to be reverted.
Thank you for the debounce removal: if you plan to submit another PR with that, I'd propose to make it configurable, since whatever forced value we pick, there will be people who would not like it.
I'd ask @ForLoveOfCats to look at it as the one involved in the original design and having most of the context, but feel free to reassign it on me if needed.
@SomeoneToIgnore I agree it seems a little dangerous, removed.
So now languages that use it still have the pop-in, but text is truncated with ellipses:
Rust
Before:
After:
Python
Before:
After:
@SomeoneToIgnore just checked and completion_documentation_secondary_query_debounce is already a user-configurable setting
I realized this fix relies on the user having text and UI sizes within a pretty narrow range of ratios. I'm going to work on it and come back with a better solution.
@maxbrunsfeld I started work on a cleaner implementation. During the step where it calculates how many characters should be included in the label, I want to use the max length of the text area divided by the width of each letter. However, font width is not currently available through GPUI. Before I go on from here, I just wanted to ask a few style/implementation questions:
- I'm currently implementing this logic in
editor.rs. I believe this makes sense, as this is the last place the label is touched before being sent to GPUI. However, I can also see an argument to create anOverflow::Truncatediv style and do this in GPUI. My hesitation with putting this logic in GPUI is that to preserve labels being truncated asmysuperlon...: Stringrather thanmysuperlongstring_m..., I am truncating using the length of therunsinStyledText, and that seems to me to be pretty task specific. Which of these locations seems better to you? - Getting the font width isn't currently a feature of GPUI. Would it make sense to add it, or is there a better way to solve this task?
- If I keep the truncation logic in
editor.rsand add font_width to GPUI, should that happen through two separate PRs, or is it ok to edit both editor and GPUI in this PR?
A few comments, but this mostly makes sense to me.
Looking at the original report, it seems like that user was running into this when the autocomplete was too narrow (in Python maybe based on the del keyword). I'm not sure if it was this change, but it seems like on this branch I don't see the documentation at all for Python anymore:
main as of (effc317a06126d4a3516c1f582e5215677c6cc73):
On your branch (60123809045d7fb859717379d6939a884aafb016).
(Perhaps a simpler approach to this would be to make the autocomplete wider for python :D).
@ConradIrwin Oh haha, I didn't run into that issue during testing because in my user settings, I have completion_documentation_secondary_query_debounce set to 0, so on my machine the documentation label is filled during the sizing calculation, while by default it isn't. (Setting this to 0 wouldn't be a fix because it would still cause issues if the lsp ever lags).
I was already setting the width to max on Python, but again, this was relying on a filled documentation label when the completion width gets calculated. I could alternatively just check if the language is in a list of languages that use the documentation label rather than styled runs, but that solution feels more like a hack than an actual fix. I'll take another look and get back to you.
@ConradIrwin Actually the true most straightforward approach would be setting min and max window width to always be the same size, which is what both VSCode and JetBrains IDEs do.
JetBrains:
VSCode:
Would this be a valid solution or would the Zed team prefer the completion popup scales width with the text? (I personally lie somewhere between "I wouldn't notice either way" and thinking the popup width changing is a little odd).
Can we tell in advance whether we're likely to get documentation (e.g. for a particular language)?
If so it seems reasonable to always use the larger size in that case. In the case where we just get simple strings, 510 is very wide.
Either way it seems like truncation should work with the size that was actually chosen.
@effdotsh I'm going to close this for now, but please feel free to re-open if you get it working!
@ConradIrwin I've had a crazy busy past two weeks, so haven't been working on this. But funnily enough, today was the day my schedule cleared up I got it working earlier. (I don't have permissions to re-open the pr, I think that's something you need to do).
The issue ended up being the pre-existing code for the widest_completion_ix. Everything works now, even without the hack of just setting languages with documentation to the max width.
https://github.com/zed-industries/zed/assets/50590465/265d91c1-a2c4-477c-833e-31e48a422c4b
Current Behavior (Zed Preview 0.128.1)
Rust:
Python:
With Changes
Rust:
Python:
Behavior is as expected with large font sizes
or large UI sizes
or both
or small font and UI sizes
or a non-mono font
@effdotsh It looks like this introduced a regression with the syntax highlighting of completion labels. The bug is somewhat unpredictable, by I could reproduce it by opening the following C file:
int main() {
printˇ
}
When typing printf, I get autocompletions that were incorrectly highlighted, in a way that varied from frame to frame:
good frame:
bad frame:
I'm reverting this in https://github.com/zed-industries/zed/pull/10084. But if you have time to follow up with a fix, maybe we can re-land this in a later release.
@effdotsh I'll also say, I worry about how complex your code is that truncates the completions. I was hoping I could fix forward without reverting, but I wasn't able to quickly understand what that code is doing. In particular, I think we should avoid computing the styled ranges twice, and laying out the text twice.
Zooming out, to me it seems like a lot of the problem is that our Python completions have very long "detail/inline document" elements, which often redundantly contain the label text (the name of the variable/method etc). If we processed completions from Pyright the way that we do for Rust, Go, and C, this problem wouldn't come up as much, and we'd actually provide even better looking completions.
Specifically, I think we could provide the best possible Python completions by changing the PythonLspAdapter::label_for_completion method so that it inspected the label, detail (and possibly documentation) in the LSP completions, and created a unified completion label that looked like this:
the_method_name(param1: T1, param2: T2) -> T3
Where the_method_name is emphasized, and the other text is slightly faded out. We do exactly this for other languages servers.
The one additional challenge with Pyright is that the detail is not present until completions are "resolved", so the method signature / variable type information would appear after a brief delay. But I don't think that's any worse than what currently happens (a separate right-justified detail appears later).
Interesting, I'll take another look in a ~~week or two~~month-ish when my schedule clears up.
@maxbrunsfeld just want to check before I go back to working on this. For python completions would you imagine basically discarding the Pyright's first completion and only rendering after the secondary completion's returned?