Changed the results window highlight to always be full width #1312
Conversation
TC72
force-pushed
the
full_width_select
branch
from
427be8e
to
81ee0b5
Compare
TC72
force-pushed
the
full_width_select
branch
from
81ee0b5
to
fb8bdc7
Compare
|
I'm personally happy to have full line highlighting, though as you've said, better to use extmarks. would have to be changed to a.nvim_buf_set_extmark(
results_bufnr,
ns_telescope_selection,
row,
#caret,
{ end_line = row + 1, hl_eol = true, hl_group = "TelescopeSelection" }
)In |
|
@fdschmidt93 Thanks for explaining how to use extmark. So if I start in the directory Sometimes it throws some error info on the screen: |
|
That's strange, doesn't seem to be happening for me and works just as expected.. mhm, could you paste the output of |
|
I can fix it using either Here's the version info: |
|
Ok luajit shouldn't be an issue.
Could you please push the commit? Then I can more easily follow 😅 |
|
possibly related: neovim/neovim#15940 |
Sorry, that's done. |
|
Just to make sure, I've just now cleanly rebuild neovim on latest master (with clean runtime files etc) and checked out your PR. Still works for me as expected. I guess must be something upstream that has unexpected local interactions ala above linked issue? |
|
Thanks for checking, I'll change to using that one. In general should we be clearing the |
TC72
force-pushed
the
full_width_select
branch
from
b7cff7d
to
f3b1982
Compare
At first glance, I'm not so concerned about this. Extmarks are attached to buffers and the telescope buffers get wiped (brr, actually they just get deleted, something I think we should change, as in why only delete and not wipe? why not with api? commented that on another PR as well). I guess for previewers it's more that we have to clean up a lot more buffers, and therefore we have a dedicated clean up function. Hopefully your segfault gets fixed with: neovim/neovim#15932 |
|
I think the relevant PRs got merged, does the error get fixed for you after clean recompilation? |
|
Hi @fdschmidt93 yes, the latest build fixes the issue. |
|
I've been testing this for the preview window but there's an issue. |
|
@fdschmidt93 would it be possible to merge this and I'll add the same for the preview window in a separate PR? |
Did you wrap it in a you'd be happy to push your WIP :) then I can more easily comment.
It's a cosmetic change I personally like. Not sure how other team members feel about this ;) I'd also prefer to merge it just at once as it otherwise might have an odd look and feel to be honest. |
|
Yes, it's in a I've pushed it now, it's just the |
| -- nvim_buf_add_highlight({buffer}, {ns_id}, {hl_group}, {line}, {col_start}, {col_end}) | ||
| -- nvim_buf_set_extmark({buffer}, {ns_id}, {line}, {col}, {*opts}) | ||
|
|
||
| pcall( |
There was a problem hiding this comment.
Slight syntactical error.
It should be
pcall(
vim.api.nvim_buf_set_extmark,
bufnr,
ns_previewer,
lnum - 1,
0,
{ end_line = lnum, hl_eol = true, hl_group = "TelescopePreviewLine"}
)(hoping to have set the commas etc here correctly). Compare it to how it was before in the diff.
pcall(function, ...) where ... are you function arguments. What happens in your case that you call pcall(extmark_id) essentially.
That explains why it works when it should and doesn't call nvim_buf_set_extmark in protected fashion, when it shouldn't work :)
There was a problem hiding this comment.
DOH!!
Thanks for setting me back on track.
|
Can someone get me up to date? I think this PR achieves what it tries to achieve: "Changed the results window highlight to always be full width". Changing every Also i dont think its worth the effort to do it for every Previewer line highlight. Its one line for each selection change. I dont think this is bottle necking. We could talk about making the full line optional or we just merge full line and then see if anyone complains. |
|
@Conni2461 the highlight for the results window works and I'd done the same just for the vimgrep previewer. I'd be happy to just do this for the results window now and come back to the previews. |
|
I felt like this previewer work is unrelated and just "blocking" this PR so we can do it in two separate PRs. Can you clean up this PR after that we decide what we should do regarding the preferences from users. full line vs only highlight text. |
|
@Conni2461 I've pushed a clean version with just the single change in |
|
Are you sure? 0ca7c58 |
|
@Conni2461 that's done. |
|
@Conni2461 I've tried pushing this to run the tests again but it's failing with issues due to tar and zip that I can't do anything about. |
|
Sorry. Busy week (which isnt really a excuse i know, i am still sorry). Code LGTM now thanks :) I gonna try it out and generate the docs :) Thanks for the PR :) i probably merge in the next 10 minutes Edit: CI failure is unrelated. Please ignore that failure. Its most likely github doing weird things because its happening only sometimes. |
|
Excuse me, I have updated to the newest version of tel and I have found that my nvim will crash after I select an item from selections. And after I use last commit, neovim become normal(means that the appearance above disappear) return require('packer').startup({function(use)
-- Packer can manage itself
use 'wbthomason/packer.nvim'
use {
'nvim-treesitter/nvim-treesitter',
run = ':TSUpdate'
}
-- stdlib for lua
use 'nvim-lua/plenary.nvim'
use {
'nvim-telescope/telescope.nvim',
commit = "02a02f7bcdfb1f207de6649c00701ee1fe13a420",
requires = { {'nvim-lua/plenary.nvim'} },
config = function ()
require("lua.plugin_config.telescope")
end
}
use {
'EdenEast/nightfox.nvim',
config = function ()
require('lua.plugin_config.nightfox')
end
}
use {
'goolord/alpha-nvim',
config = function ()
require("lua.plugin_config.alpha")
end
}
use {'neoclide/coc.nvim', branch = 'master', run = 'yarn install --frozen-lockfile'}
end,
config = {
display = {
open_fn = function()
return require('packer.util').float({ border = 'single' })
end,
}
}
}) |
|
Can you update to neovim 0.5.1 (if that segfault is still happening there. I will revert). Edit: I know the segfault is fixed in nightly. |
|
I was having that segfault when I 1st started working on this. Sorry but I can see this being a pain in the ass. |
Thx. The issue was gone when I use neovim@0.5.1 |
|
Sorry tc. I have to revert this :| I am not happy about this decision. We should try to implement this feature without relying on |
|
@Conni2461 no problem, it has to be done right. |
Fwiw, this is the right solution. Extmarks are a critical API and segfaults were fixed super swiftly once they occurred upstream. Maybe a working compromise would be to set |
|
FWIW, 0.6.0 is planned for the end of the month (if all goes according to plan, which it never does...) And Neovim itself supports only two versions:
|
i know that. Just segfaulting 0.5.0 I didnt bump the version because |
Yeah, but there's no indication that it will be updated anytime soon, if ever (great talk at vimconf notwithstanding). An unmaintained (or at least on hiatus) GUI should not hold back telescope development... |
@fdschmidt93 just to be clear, I meant the way it gets released has to be right. Something so vital to telescope can't be causing problems for people running 0.5.0. |
|
Disagree; people should be using 0.5.1; otherwise you have to keep supporting every release until eternity. The right thing to do for a plugin would be to tag a commit before using 0.5.1 specifics so people who can't update can freeze the plugin at the last compatible state. |
Yeah i agree, people should do that. If you guys wanna bump it for this feature then feel free. But we have to expect a lot of tickets. We still get tickets from people that have issues with the recent I just dont have the time and will to deal with it. |
|
Put a clear notice at the top of the readme and be done with it :) I'll be happy to step in and close any issues from people on unsupported versions :] |
|
@TC72 wanna do a new PR with a version bump ? |
|
@Conni2461 #1403 |
… width (nvim-telescope#1312)" (nvim-telescope#1398) This reverts commit 3b9ac8e.
Thought it was easier to submit this to show the effect working than describe it.
Making the selection movement easier on the eye by staying full width, not sticking to the content width.
The code I've written is a very quick hack, it just adds spaces to the end of a line to pad it out.
This can probably be done in a much nicer way using virtual text which could allow it to happen on the previewer as well.
I've also had to add an extra 2 to the width for the math to work correctly, still looking at why.
I'm looking into what's going on with the
=not being highlighted, don't think it's related to this code.