Draft: Fix highlight with unicode #77
Conversation
|
At a first glance it looks like this code only fixes the neovim textprop backend. Maybe it works fine without this change on vim's textprop, but could you do a quick check to see if it's necessary? The change "Add default highlight for LocalVariable" is not necessary. I deliberately left that group undefined as it's a very common one that most people don't want custom highlighting for. Adding it results in a lot more highlighting calls which slows things down. Users who want the group can always define it in their own vimrc. |
Kamilcuk
force-pushed
the
master
branch
3 times, most recently
from
87e110e
to
dabc787
Compare
Added support for that, but I didn't test it that much.
Sure! Well, just added a comment then.... :p |
There was a problem hiding this comment.
Looked over it, had some comments, but they are all relatively minor and should be easy to fix up.
I also did some testing locally.
Textprop: yes vim's textprop is affected by unicode problems in the same way. Enabling your change fixes it.
Neovim: Fix works as well.
Match: The matchaddpos backend is actually also affected. But honestly there's no good reason to use this backend at all, it is buggy in many other ways already. Unless you have a vested interest in fixing this as well, I would say just leave it alone.
| " These are equal when each displayed character takes exactly one column, | ||
| " but there's also UTF-8. | ||
| function! lsp_cxx_hl#bytes_to_columns(buf, s_line, s_char, e_line, e_char) | ||
| if get(g:, 'lsp_cxx_hl_use_byteidx', 0) |
There was a problem hiding this comment.
I would prefer this outside the function and each place we need to call it we wrap that. It's a bit more repeated code but we avoid the call which might have less of an impact on the runtime.
| " It should be noted that neovim uses zero based indexing like LSP | ||
| " this is unlike regular vim APIs which are 1 based. | ||
|
|
||
| function! lsp_cxx_hl#textprop_nvim#buf_add_hl_lsrange(buf, ns_id, hl_group, | ||
| \ range) abort | ||
| call lsp_cxx_hl#log('range = ', a:range) |
There was a problem hiding this comment.
needs to be removed before merging.
| " These are equal when each displayed character takes exactly one column, | ||
| " but there's also UTF-8. | ||
| function! lsp_cxx_hl#bytes_to_columns(buf, s_line, s_char, e_line, e_char) | ||
| if get(g:, 'lsp_cxx_hl_use_byteidx', 0) |
There was a problem hiding this comment.
This needs to have it's default in plugin/vim-lsp-cxx-highlight.vim. You can then access it directly via g:lsp_cxx_hl_use_byteidx instead of via get(g:. I think most of the code currently uses get(g: but it's probably unnecessary as it's always defined anyways.
Also a doc entry would be good as well.
Introduce g:lsp_cxx_hl_use_byteidx
This is a draft for now.
Solves issue #51 on neovim and on vim.
The extra
catchare for, as I understand it, a race condition: it may happen, that a line is edited, and in midst editing the highlight function is triggered, then it may happen that it callsgetbufline, while the columns are no longer there, because they were removed when editing. It actually causes nothing, as the column is no longer there, it can just throw and ignore the operation, and highlight it on the next pass. On the other hand, such issue shows that there is a slight delay in the functions, and it causes some slowdown, so it may be a good idea to make it in a feature flag.I am using this for like a month with sources with a lot of
πΩ«»characters, and it works really fine.