live_grep highlighting shouldn't be fuzzy or include the filenames

ehaynes99 commented 1 year ago

Description

The default behavior of live_grep is to pass it to rg as a regex. Using rg from the cli, its highlighting highlights matches of that regex in the results.

Using the default options for live_grep, the sorter uses the fuzzy highlighter on the result: https://github.com/nvim-telescope/telescope.nvim/blob/master/lua/telescope/sorters.lua#L491-L493.

This results in highlighting that is rather nonsensical. As a visual cue, it actually makes it look like the search is broken (to be clear, it's behaving correctly, it just looks like it's not).

There's a fundamental mismatch between what the highlighter is trying to do and what the search actually does
The highlighter is processing the combined string with filename and content, which gives the impression that the filename had some impact on the search

I'm happy to take a stab at a PR, but it'll be towards the end of the year. I'm curious if it's possible to capture the output of rg with color and treat ansii escapes as "quotes". That would be ideal IMO, as then telescope wouldn't need to attempt any logic around it, simply delegating it to ripgrep. However, if that's not possible, at least a default that splits the string after the filename/index part and does a regex match on that would be an improvement.

Neovim version

NVIM v0.8.1
Build type: Release
LuaJIT 2.1.0-beta3

Operating system and version

Arch Linux - 5.15.81-1-lts

Telescope version / branch / rev

76ea9a898d3307244dce3573392dcf2cc38f340f

checkhealth telescope

telescope: require("telescope.health").check()
========================================================================
## Checking for required plugins
  - OK: plenary installed.
  - WARNING: nvim-treesitter not found. 

## Checking external dependencies
  - OK: rg: found ripgrep 13.0.0
  - OK: fd: found fd 8.5.3

## ===== Installed extensions =====

## Telescope Extension: `fzf`
  - OK: lib working as expected
  - OK: file_sorter correctly configured
  - OK: generic_sorter correctly configured

Steps to reproduce

Any search with regex characters

Expected behavior

Highlighting would highlight the terms within the text that matched the search

Actual behavior

Highlighting highlights... some stuff

Minimal config

-- not configuration specific

vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvim/site]]
local package_root = '/tmp/nvim/site/pack'
local install_path = package_root .. '/packer/start/packer.nvim'
local function load_plugins()
  require('packer').startup {
    {
      'wbthomason/packer.nvim',
      {
        'nvim-telescope/telescope.nvim',
        requires = {
          'nvim-lua/plenary.nvim',
          { 'nvim-telescope/telescope-fzf-native.nvim', run = 'make' },
        },
      },
      -- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
    },
    config = {
      package_root = package_root,
      compile_path = install_path .. '/plugin/packer_compiled.lua',
      display = { non_interactive = true },
    },
  }
end
_G.load_config = function()
  require('telescope').setup()
  require('telescope').load_extension('fzf')
  -- ADD INIT.LUA SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
end
if vim.fn.isdirectory(install_path) == 0 then
  print("Installing Telescope and dependencies.")
  vim.fn.system { 'git', 'clone', '--depth=1', 'https://github.com/wbthomason/packer.nvim', install_path }
end
load_plugins()
require('packer').sync()
vim.cmd [[autocmd User PackerComplete ++once echo "Ready!" | lua load_config()]]

-- for convenience
vim.keymap.set('n', '<space>ft', '<cmd>Telescope live_grep<CR>')
 -- to avoid burning your retinas on search ;-)
vim.cmd('colorscheme habamax')

dasupradyumna commented 1 year ago

+1 for this issue. As shown the screenshots posted by @ehaynes99, especially when there is no regex in the search string, the file paths in the resulting entries are highlighted over the actual detected string in the files, when the search string is a substring of the file paths.

jamestrew commented 1 year ago

Rather than taking the colored rg output and parsing ansi color codes, we could use either the --json to get the output as a json or --replace to use a custom delimiter for matches.

--json would probably be over kill. There's a lot to parse.

$ rg --json --smart-case foo
{"type":"begin","data":{"path":{"text":"foo.md"}}}
{"type":"match","data":{"path":{"text":"foo.md"},"lines":{"text":"foo\n"},"line_number":1,"absolute_offset":0,"submatches":[{"match":{"text":"foo"},"start":0,"end":3}]}}
{"type":"end","data":{"path":{"text":"foo.md"},"binary_offset":null,"stats":{"elapsed":{"secs":0,"nanos":14727,"human":"0.000015s"},"searches":1,"searches_with_match":1,"bytes_searched":4,"bytes_printed":221,"matched_lines":1,"matches":1}}}
{"type":"begin","data":{"path":{"text":"foobar.md"}}}
{"type":"match","data":{"path":{"text":"foobar.md"},"lines":{"text":"foobar\n"},"line_number":1,"absolute_offset":0,"submatches":[{"match":{"text":"foo"},"start":0,"end":3}]}}
{"type":"end","data":{"path":{"text":"foobar.md"},"binary_offset":null,"stats":{"elapsed":{"secs":0,"nanos":2815,"human":"0.000003s"},"searches":1,"searches_with_match":1,"bytes_searched":7,"bytes_printed":230,"matched_lines":1,"matches":1}}}
{"type":"begin","data":{"path":{"text":"test.lua"}}}
{"type":"match","data":{"path":{"text":"test.lua"},"lines":{"text":"  args = {\"foo\"},\n"},"line_number":8,"absolute_offset":168,"submatches":[{"match":{"text":"foo"},"start":11,"end":14}]}}
{"type":"end","data":{"path":{"text":"test.lua"},"binary_offset":null,"stats":{"elapsed":{"secs":0,"nanos":5463,"human":"0.000005s"},"searches":1,"searches_with_match":1,"bytes_searched":317,"bytes_printed":245,"matched_lines":1,"matches":1}}}
{"data":{"elapsed_total":{"human":"0.002163s","nanos":2163299,"secs":0},"stats":{"bytes_printed":696,"bytes_searched":328,"elapsed":{"human":"0.000023s","nanos":23005,"secs":0},"matched_lines":3,"matches":3,"searches":3,"searches_with_match":3}},"type":"summary"}

--replace or -r for short would be my guess for how we'd achieve this.

$ rg --vimgrep --smart-case -r '<<<$0>>>' foo                  
foo.md:1:1:<<<foo>>>
foobar.md:1:1:<<<foo>>>bar
test.lua:8:12:  args = {"<<<foo>>>"},

What we choose for the delimiter would matter though to prevent over matching on the telescope side when parsing the entries.

I can work on this.

matu3ba commented 1 year ago

--json would probably be over kill. There's a lot to parse.

vim.json is fast and makes parsing very simple and now also has documentation on master HEAD. Performance for non-luajit should be quickly benched though, but I dont see (yet) how it would make things slower than the current approach.

jamestrew commented 1 year ago

Yeah I'm reconsidering and leaning towards using --json especially after learning that's basically what burntsushi recommends. I'm not concerned so much about the performance but rather that reworking live_grep to consume the json data would be a lot more work than using --replace.

And frankly, this repo's been a bit cold in terms of maintainer activity so I don't have a ton of motivation to spend a good chunk of time on something only for it to be forgotten in the stack of PRs.

jamestrew commented 1 year ago

Started working on this. Still have to work on some edge cases and do some clean up but overall fairly straight forward. ^notice the filename foo is not getting highlighted.

fdschmidt93 commented 1 year ago

You can also refer to the entry maker of https://github.com/fdschmidt93/telescope-egrepify.nvim as well which uses vim.json to parse info from rg.

jamestrew commented 1 year ago

I did take a peek actually when working on the entry maker :grin:

The tricky part is how to deal with certain options that will conflict with --json (rg will throw an error that isn't propagated to telescope), namely --files, --files-with-matches, --files-without-match, --count and --count-matches.

For the first three file related flags, we should be able to use the gen_from_file entry maker and not use the --json flag.

The count flags we won't be able to support (and we never did) without it's own entry maker I think. I'm not going to bother with that but I probably want to throw an error message at least.

nvim-telescope / telescope.nvim