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
Use native virtual-text for vim9 #4281
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I can only verify it does not break things in neovim. Would be great if someone, that is not the PR author, can verify it works as expected in vim.
4db113d
to
661c762
Compare
I am a someone and I can verify it works in vim. ^^ @blayz3r You linked in the issue to the new vim9 implementation, would you mind compiling latest vim from source and verifying that everything works as expected with this PR? |
661c762
to
8b21a84
Compare
Needs bug fixes . vim version : 9.0.228 ale settings let g:ale_virtualtext_cursor = 1 |
Cannot reproduce any of these problems with your config. Please provide a minimal working config to reproduce the problem. The part of your config that you shared, does not reproduce the problem with a clean config as can be tested with: vim --clean -Nu <(cat<<EOF
set rtp+=/path/to/ale/with/my/branch/checkedout
let g:ale_linters = {'rust': ['analyzer']}
let g:ale_fixers = { 'rust': ['rustfmt', 'trim_whitespace', 'remove_trailing_lines'] }
let g:ale_fix_on_save = 1
let g:ale_sign_error = '✗'
let g:ale_sign_warning = ''
let g:ale_sign_column_always = 1
let g:ale_rust_cargo_use_clippy = 1
let g:ale_completion_enabled = 1
let g:ale_virtualtext_cursor = 1
let g:ale_virtualtext_delay = 1
au BufEnter *.rs set ft=rust
EOF
) /path/to/main.rs |
That config is neither minimal, nor does it reproduce the problem on my system. It is not even a valid config, for example the line with There are other problems with that config too so please provide a minimal config, that actually reproduces the problem. Also are you sure that you have checked out my branch? Your problem sounds more like something that could happen with the legacy virtual text implementation. |
Sorry for posting buggy rc. minimal working rc packloadall let g:ale_virtualtext_cursor = 1 |
cloned repo from https://github.com/vimpostor/ale
git branch native_virt_text
packloadall
That is not enough to load the checkout of the plugin.
You still need to add it to the runtimepath, it seems you are loading
the plugin from somewhere else.
And your minimal config still does not reproduce the problem, I have
proven it in the following recording:
https://asciinema.org/a/isJpN089bx4RFZXbHoq9VeVSG
I still think that you are testing with the wrong version of Ale because
"packloadall" does not load my branch if you do not add it to the
runtimepath.
If you still think you are testing it the correct way, then please
attach a recording showing exactly how you start vim with my branch
checked out.
|
Setting rtp fixed the issue. |
Our current virtual text implementation for vim emulates it by abusing the textprop and popupwin feature from vim 8.2 (for more details see commit 708e810). This implementation sometimes is janky, for example the popups may leak into other vim windows next to the current window. Luckily, vim just got native virtual-text support as a proper subtype to the prop_add() function. By using the 'text' option, the text property automatically becomes virtual text that is appended to the current line if col is zero. Note that the prop_add() method now returns negative IDs for virtual text properties. This feature was added in vim 9.0.0067, but it got a lot of bugfixes which is why we only use this new API if vim has at least version 9.0.0214. However, there are still some minor bugs with vim's native virtual text, so we might have to bump the version check again in the future. Also see dense-analysis#3906. Now with proper virtual text support for both vim and neovim available, we can tackle dense-analysis#2962 in the future by simply tracking multiple virt-texts instead of just the last one. In the future we might also want to disable our virtual text emulation support for vim, as it is a total hack, but for now we should keep it for backwards compatibility.
8b21a84
to
f20d296
Compare
Thanks, so it looks like you used the wrong checkout of Ale all along. I just pushed my patch again restricting it to at least vim 9.0.0214, because there were a lot of virtual text bugfixes in vim since its initial implementation. |
Yes, I can confirm it works as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution and testing.
The hardcoded background was a workaround to fix the wrong background for Ale's virtual text. We can remove the workaround now, because Ale uses vim's native virtual text now, which does not have this problem. refs: dense-analysis/ale#4281
Our current virtual text implementation for vim emulates it by abusing the textprop and popupwin feature from vim 8.2 (for more details see commit 708e810). This implementation sometimes is janky, for example the popups may leak into other vim windows next to the current window. Luckily, vim just got native virtual-text support as a proper subtype to the prop_add() function. By using the 'text' option, the text property automatically becomes virtual text that is appended to the current line if col is zero. Note that the prop_add() method now returns negative IDs for virtual text properties. This feature was added in vim 9.0.0067, but it got a lot of bugfixes which is why we only use this new API if vim has at least version 9.0.0214. However, there are still some minor bugs with vim's native virtual text, so we might have to bump the version check again in the future. Also see dense-analysis#3906. Now with proper virtual text support for both vim and neovim available, we can tackle dense-analysis#2962 in the future by simply tracking multiple virt-texts instead of just the last one. In the future we might also want to disable our virtual text emulation support for vim, as it is a total hack, but for now we should keep it for backwards compatibility.
This reimplements virtual text for vim with the new native virtual text API. Please see the commit message for more details.
When this gets merged, I can finally tackle #2962 and implement multiple virtual texts being visible at once.
Screenshot
Before: At the top (implementation is a bit janky)
After: At the bottom (jank is fixed with native virtual text)