Skip to content
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

feat(builtin.current_buffer_fuzzy_find): jump to first matched char #2909

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

antonk52
Copy link
Contributor

@antonk52 antonk52 commented Feb 3, 2024

Description

Currently the in buffer fuzzy search always jumps to the first character of the selected line. This PR enhances it to jump to the first same character on the line as the first character from the search. See demo below.

Motivation: I assume people use this helper as an enhanced version of / or ?. Alternatively this is as useful as cmd/ctrlf from modern GUI editors and IDEs. Both jump to the match instead of the beginning of the line.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  1. Open any file
  2. Run :Telescope current_buffer_fuzzy_find
  3. Search for something that is not on the first character on the line that you want to go to
  4. Expect cursor to land on the first character in your search on this line

Before

Screen.Recording.2024-02-03.at.20.26.01.mov

After

Screen.Recording.2024-02-03.at.20.22.51.mov

Configuration:

  • Neovim version (nvim --version): VIM v0.9.5
  • Operating system and version: MacOS 14.2.1

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • NA I have made corresponding changes to the documentation (lua annotations)

lua/telescope/builtin/__files.lua Outdated Show resolved Hide resolved
lua/telescope/builtin/__files.lua Outdated Show resolved Hide resolved
@antonk52
Copy link
Contributor Author

Hi, I have addressed the comments

  • replace entire select behaviour instead of enhancing
  • use minimal value from highlighted instead of the first one

I tried reproducing the off by one error the you mentioned but the current implementation works for me. I tested it on both 0.9.5 and a fresh master build. Do you have a specific case where this did not work for you? Do you use a non default sorter?

@jamestrew
Copy link
Contributor

I tried reproducing the off by one error the you mentioned but the current implementation works for me. I tested it on both 0.9.5 and a fresh master build. Do you have a specific case where this did not work for you? Do you use a non default sorter?

Hmm... that's interesting
I tried with both my config and a minimal config and I'm still getting an off-by-one.
Tried 0.9.5 as well as nightly.

current_buf_fzy_find.mp4

minimal config:

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs({ "config", "data", "state", "cache" }) do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    lazypath,
  })
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  {
    "antonk52/telescope.nvim",
    branch = "feat/buf-fuzzy-column",
    dependencies = {
      "nvim-lua/plenary.nvim",
      "nvim-tree/nvim-web-devicons",
    },
    config = function()
      require("telescope").setup({})
    end,
  },
}

require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

@antonk52
Copy link
Contributor Author

I have updated the PR to handle non 0 based column index returned from highlighted.

Thank you for the repro steps. I tried it out and turns out the last time I tested it I used a shortcut bound to my version of the picker that I was using prior to opening the PR instead of the command. My bad but glad it is resolved now.

@jamestrew
Copy link
Contributor

I noticed that if there's no prompt given and/or there's no highlighting, math.min isn't given any values and errors so I pushed a fix for that.

The rest looks good to me. Thanks!

@jamestrew jamestrew merged commit fac5da8 into nvim-telescope:master Feb 15, 2024
6 checks passed
@b0ae989c
Copy link
Contributor

b0ae989c commented Feb 15, 2024

For telescope-zf-native.nvim users: this commit crashes Telescope when we do :Telescope current_buffer_fuzzy_find. And we have the following error message:

E5108: Error executing lua: ...plugins/telescope.nvim/lua/telescope/builtin/__files.lua:549: bad argument #1 to 'unpack'
 (table expected, got nil)
stack traceback:
        [C]: in function 'unpack'
        ...plugins/telescope.nvim/lua/telescope/builtin/__files.lua:549: in function 'run_replace_or_original'
        ...epro/plugins/telescope.nvim/lua/telescope/actions/mt.lua:65: in function 'key_func'
        ....repro/plugins/telescope.nvim/lua/telescope/mappings.lua:269: in function <....repro/plugins/telescope.nvim/l
ua/telescope/mappings.lua:268>

This is a bug on telescope-zf-native.nvim, not on Telescope.

To reproduce this error:

local root = vim.fn.fnamemodify('./.repro', ':p')
for _, name in ipairs({ 'config', 'data', 'state', 'cache' }) do
  vim.env[('XDG_%s_HOME'):format(name:upper())] = root .. '/' .. name
end
local lazypath = root .. '/plugins/lazy.nvim'
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({
    'git',
    'clone',
    '--filter=blob:none',
    'https://github.com/folke/lazy.nvim.git',
    lazypath,
  })
end
vim.opt.runtimepath:prepend(lazypath)
local plugins = {
  'folke/tokyonight.nvim',
  {
    'nvim-telescope/telescope.nvim',
    commit = 'fac5da839e23e7a4c17a332a640541cd59ebfbd5',
    lazy = false,
    dependencies = {
      'nvim-lua/plenary.nvim',
      'natecraddock/telescope-zf-native.nvim',
    },
    config = function()
      require('telescope').setup({
        extensions = { ['zf-native'] = { generic = { enable = true } } },
      })
      require('telescope').load_extension('zf-native')
    end,
  },
}
require('lazy').setup(plugins, { root = root .. '/plugins' })
vim.cmd.colorscheme('tokyonight')

@b0ae989c
Copy link
Contributor

b0ae989c commented Feb 15, 2024

A temporary fix is to implement

local column = highlighted and math.min(unpack(highlighted) or 1) - 1 or 0

at the line

local column = math.min(unpack(highlighted) or 1) - 1
, and turn off highlight_results in zf-native with

extensions = {
  ['zf-native'] = {
    generic = {
      enable = true,
      highlight_results = false,
    },
  },
},

@b0ae989c
Copy link
Contributor

With some more tests, I find out that this commit causes the same issue with telescope-fzf-native.nvim .

@dmtrKovalenko
Copy link
Contributor

dmtrKovalenko commented Feb 15, 2024

Just tried this and for some reason, it puts the cursor to the end of the highlight, feels a bit strange tbh. Overall, I really like the feature, is there any reason why it is implemented this way?

Screen.Recording.2024-02-15.at.08.54.52.mov

@b0ae989c
Copy link
Contributor

Personally I don't mind this issue since I don't use the builtin fuzzy finder often. But from a design point of view, using highlighting results to compute cursor positions doesn't look like a good idea.

@dmtrKovalenko
Copy link
Contributor

It's just different from using the native search behavior. But we always know where the start of the match happens, aren't we? Why not put the cursor at the start of the match?

@antonk52
Copy link
Contributor Author

I've pushed a follow up PR that I tested with the builtin telescope finder and fzf extension of jumping to the last highlighted character #2922

@jamestrew
Copy link
Contributor

Thanks everyone for the quick response. I've merged in the follow up PR to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants