Use extmarks for highlighting and carets #2099
Conversation
|
Thank you very much for pulling out these changes :) I'll review and test it tomorrow evening. I can't make it before |
|
No worries, take your time. I’m here to answer any questions
…Sent from my iPhone
On Jul 26, 2022, at 12:39 PM, Simon Hauser ***@***.***> wrote:
Thank you very much for pulling out these changes :)
I'll review and test it tomorrow evening. I can't make it before
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
|
@codegangsta you can run stylua over a direction |
|
@tjdevries Yeah I thought I has stylua installed already but it looks like I didn't. Should be all good now |
lua/telescope/pickers/highlights.lua
Outdated
| @@ -56,65 +61,106 @@ end | |||
|
|
|||
| function Highlighter:hi_sorter(row, prompt, display) | |||
| local picker = self.picker | |||
| local sorter = picker.sorter | |||
There was a problem hiding this comment.
there is no need for having sorter in a local var. Its only used in line 74 so we can just do picker.sorter there. esp because line 65 also uses picker.sorter
lua/telescope/pickers/highlights.lua
Outdated
| local line = a.nvim_buf_get_lines(results_bufnr, row, row + 1, false)[1] | ||
| if not line then |
There was a problem hiding this comment.
line 108 also does this check. but it compares with "", I think it doesnt hurt to check for both nil and "" in 108 and here
|
Overall LGTM, just a couple of nits 😆 Thanks again for pulling it out :) One thing i noticed while testing is that on multiselect we change the highlighting for the icon, which we currently dont do. I think priority is causing this and i am currently thinking if DISPLAY should have a higher priority compared to SELECTION. CC @fdschmidt93 |
|
Oh good catch on the highlight. If we want to keep it compatible the priority fix should be easy enough. If we decide later we want to change it to highlight the multi-select cursor we can do that in a followup PR. |
|
Yeah can you update the priorities, and fix the other things. You also need to rebase 😬 Once this is fixed, @fdschmidt93 can you retest and merge. I am leaving this evening for a 10 day vacation and will not take a laptop and actually going to uninstall github from my phone, so yeah thanks :) |
codegangsta
force-pushed
the
JMS-prefix-extmarks
branch
from
ae87e86 to
7972129
Compare
|
@Conni2461 and @fdschmidt93 fixed the feedback in this PR. LMK if there are any outstanding issues |
|
oh right, tests will fail because we no longer overwrite the text in the line -- you can just update the tests to pass @codegangsta they all seem like failures that are OK to just be shifted to new results |
|
Whoops. Missed that one as it looks like that test was disabled on mac. All fixed now @tjdevries |
Co-authored-by: Fabian David Schmidt <[email protected]>
|
Thanks for the suggestions @fdschmidt93. I added those to this PR |
|
Sounds good to me :) |
This reverts commit 8d13f4c.
Description
Pulled out as a subset of the changes from #1491
This PR uses extmarks and virtual text instead of typical highlighting and text replacement to consolidate the highlight/rendering logic to
highlights.lua. This should give us more flexibility in the future when we want to improve rendering of telescope.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list relevant details about your configuration
lua/tests/automated/Configuration:
Checklist: