Skip to content

fix: builtin find not working with dot repeat #622

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 1, 2024
Merged

fix: builtin find not working with dot repeat #622

merged 4 commits into from
Jun 1, 2024

Conversation

@kiyoon
Copy link
Collaborator

@kiyoon kiyoon commented May 21, 2024 •
edited
Loading

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 kiyoon changed the title fix!: builtin find not working with dot repeat fix: builtin find not working with dot repeat May 21, 2024
@kiyoon kiyoon requested a review from theHamsta May 21, 2024 13:12
@kiyoon kiyoon mentioned this pull request May 21, 2024
Closed
sahinakkaya
sahinakkaya approved these changes May 21, 2024
View reviewed changes
Copy link

@sahinakkaya sahinakkaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and can confirm that it works as expected.

@kiyoon
Copy link
Collaborator Author

kiyoon commented May 21, 2024

@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
Copy link

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

@Mithrandir2k18
Copy link

Also just tested it and can confirm it works :)

@kiyoon
Copy link
Collaborator Author

kiyoon commented Jun 1, 2024

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

@clason
Copy link
Collaborator

clason commented Jun 1, 2024

If it applies cleanly to the main branch.

@kiyoon
Copy link
Collaborator Author

kiyoon commented Jun 1, 2024

@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
Copy link
Collaborator

clason commented Jun 1, 2024

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
Copy link
Collaborator Author

kiyoon commented Jun 1, 2024

I can rebase and make the PR.

@clason
Copy link
Collaborator

clason commented Jun 1, 2024

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.

@kiyoon kiyoon merged commit fd41b7c into nvim-treesitter:master Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@theHamsta theHamsta Awaiting requested review from theHamsta

1 more reviewer

@sahinakkaya sahinakkaya sahinakkaya approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mapping repeating movements as recommended breaks some repeated commands

4 participants

@kiyoon @sahinakkaya @Mithrandir2k18 @clason