nvim-treesitter / nvim-treesitter-textobjects

Apache License 2.0
2.2k stars 200 forks source link

fix: builtin find not working with dot repeat #622

Closed kiyoon closed 5 months ago

kiyoon commented 6 months ago

fixes #519

The current implementation of builtin f, F, t, T is a re-implementation to match the behaviour of the nvim upstream. Although it works with ; and , movement repeat, unfortunately it probably wouldn't work with complex combinations (e.g. dfe and . repeat wouldn't work).

Instead, we can make builtin_f_expr, builtin_F_expr, ... just returns "f", "F", "t", "T" character so you can use it to map with {expr = true}. See the implementation below. This will almost likely resemble the upstream behaviour in most of the situations.

M.builtin_f_expr = function()
  M.last_move = {
    func = "f",
    opts = { forward = true },
    additional_args = {},
  }
  return "f"
end

Before returning the string, note that it also maps the last movement so the ; and , mapping can handle it as well.

Important: users must change their keybindings to include { expr = true }. I updated the README.md as follows:

-- Optionally, make builtin f, F, t, T also repeatable with ; and ,
vim.keymap.set({ "n", "x", "o" }, "f", ts_repeat_move.builtin_f_expr, { expr = true })
vim.keymap.set({ "n", "x", "o" }, "F", ts_repeat_move.builtin_F_expr, { expr = true })
vim.keymap.set({ "n", "x", "o" }, "t", ts_repeat_move.builtin_t_expr, { expr = true })
vim.keymap.set({ "n", "x", "o" }, "T", ts_repeat_move.builtin_T_expr, { expr = true })

The original functions builtin_f etc. are left unchanged, except it will notify the deprecation warning once.

kiyoon commented 6 months ago

@sahinakkaya Thanks for testing! Would it be possible to keep it in your config and try it for some time? Because the last thing we want to do is to ask all users to change their config only to see another issue that we have to roll back.

sahinakkaya commented 6 months ago

Yeah, sure. I was already planning to keep it until this is merged.

Mithrandir2k18 commented 6 months ago

Also just tested it and can confirm it works :)

kiyoon commented 5 months ago

@ribru17 I've also been using this and had no problem so far. Do you think it's ready to merge?

clason commented 5 months ago

If it applies cleanly to the main branch.

kiyoon commented 5 months ago

@clason Yes it does, it's separated from the module structure.

I can make a separate PR for the main branch if it's needed. I don't know if it can be just rebased as main branch has a little bit of refactoring.

clason commented 5 months ago

Rebasing would be preferred (also to keep main up-to-date with respect to query changes). This will require some manual conflict resolution, but hopefully nothing too involved.

kiyoon commented 5 months ago

I can rebase and make the PR.

clason commented 5 months ago

No, no need to make a PR -- just git checkout main; git rebase master, resolve any conflicts, then git rebase --continue. Lather, rinse, repeat, and then just git push -f.