fix(repeatable_move): repeat_last_move always uses count 1 when used in operator-pending (no) mode.

[illia-shkroba]

I've noticed that after switching to main branch from master branch the ; and , motions are not using [count] in operator pending mode. For example: df/ and then 2d; remove 2 slashes instead of expected 3 slashes (count 2 in d; is ignored).

My nvim version:

NVIM v0.12.0-dev-2063+g8bdfd286e5
Build type: RelWithDebInfo
LuaJIT 2.1.1767980792
Run "nvim -V1 -v" for more info

My bindings nvim-treesitter-textobjects:

-- ;, and fFtT keymaps
local ts_repeat_move = require "nvim-treesitter-textobjects.repeatable_move"

vim.keymap.set({ "n", "v", "o" }, ";", ts_repeat_move.repeat_last_move)
vim.keymap.set(
  { "n", "v", "o" },
  ",",
  ts_repeat_move.repeat_last_move_opposite
)

vim.keymap.set(
  { "n", "v", "o" },
  "f",
  ts_repeat_move.builtin_f_expr,
  { expr = true }
)
vim.keymap.set(
  { "n", "v", "o" },
  "F",
  ts_repeat_move.builtin_F_expr,
  { expr = true }
)
vim.keymap.set(
  { "n", "v", "o" },
  "t",
  ts_repeat_move.builtin_t_expr,
  { expr = true }
)
vim.keymap.set(
  { "n", "v", "o" },
  "T",
  ts_repeat_move.builtin_T_expr,
  { expr = true }
)

[kiyoon]

Thanks.

I rebased it so I can run tests, but if you don't like that I'm in the commit author you can rebase again yourself.

This fixes the fFtT cases but maybe not the other functions? Anyway, this kind of bug is infinite like if we fix one we introduce another.. And I usually use d;. to repeat the deletion which progressively deletes so I don't need to count how many.

I can merge it if you don't mind that it probably still won't work everywhere, but honestly it's more common to count for fFtT commands than to count bigger chunks like function, class etc. thus I didn't even notice it.

[illia-shkroba]

I rebased it so I can run tests, but if you don't like that I'm in the commit author you can rebase again yourself.

All good. Please keep it as-is. I just want to make sure that it works ;-)

This fixes the fFtT cases but maybe not the other functions? Anyway, this kind of bug is infinite like if we fix one we introduce another.. And I usually use d;. to repeat the deletion which progressively deletes so I don't need to count how many.
I can merge it if you don't mind that it probably still won't work everywhere, but honestly it's more common to count for fFtT commands than to count bigger chunks like function, class etc. thus I didn't even notice it.

It fixes ; and , that have M.last_move.func as one of the ftFT. The other motions (e.g.: [m) due to the snippet:

-- we assume other textobjects (move) already handle operator-pending mode correctly
M.last_move.func(opts, unpack(M.last_move.additional_args))

work just fine -- there is no invokation of the force_operator_pending_visual_mode.

I have discovered it when I was doing (something like) dF/ then d; and then 2..
I don't do it that frequently, but I was confused at first that it didn't work.

[kiyoon]

Oh, you're right that it works for others too. This looks good to me. Thank you so much!

Would you be so kind to add a test for this? That would be awesome.

[illia-shkroba]

Great to hear that!

Sure. I have written the test cases for that behavior... Unfortunately these test cases don't pass, but the failures are not related to this fix.

The main branch behavior of the ; and , motions is inconsistent with the master branch:

• On the master branch the ; and , motions match the default behavior:
  • yfn and y; - inclusive.
  • yfn and y, - exclusive.
  • yFn and y; - exclusive.
  • yFn and y, - inclusive.
  • ytn and y; - inclusive.
  • ytn and y, - exclusive.
  • yTn and y; - exclusive.
  • yTn and y, - inclusive.
• On the main branch the ; and , motions are always inclusive and have a "strange" corner case that I found with the test cases:
  • When a cursor is at the beginning of the line use: yfn and y,. The second y, captures a first character in the line. But by default (nvim -u NONE) y, wouldn't capture anything keeping the '0' register intact.

It might be out-of-scope of this PR.

Since the test cases are failing I've pushed them to a separate branch and created this PR: #862.

[kiyoon]

I see. It must be the force_operator_pending_visual_mode() quirk. However, without that it causes another critical issue.

In my opinion, even though I use repeatable move very frequently I didn't know about that difference and I find it very unintuitive. Just like sometimes ; can go forward and sometimes backward which I found awkward. Others may disagree, but I think f and t should make a difference in inclusiveness at the end, but f and F making a difference in inclusiveness at the start is kind of weird. If we were to cover all cases, it would be an unmaintainable infinite loop of fix and introduce another bug..

Thus I can consider fixing other test cases later, but for now I'm happy with the current behaviour unless others think otherwise.

Can you add a test in this PR of the cases it fixes specifically with count, instead of adding all other quirks that don't pass currently?

Thank you!

[illia-shkroba]

I have fixed the repeat_last_move so that it handles inclusive/exclusive behavior of the ; and , motions used after fFtT motions as in plain NeoVim.

Please take a look.

I have also added tests for that.

[kiyoon]

is it necessary to introduce another option, or can it just use if M.last_move.func == 'f' or M.last_move.func == 't' then?

[illia-shkroba]

I have refactored it slightly.
Please take a look.

[kiyoon]

Please add a comment like "this changes operator-pending (no) mode to operator-pending-visual (nov) mode to include last character in the region when going forward. In other words, going forward will include current cursor and found character, and going backward will include the found character but NOT the current cursor."

[kiyoon]

Looks good to me, and it works well, thanks a lot!

[illia-shkroba]

Great to hear that. Thank you for your input!

[kiyoon]

you should use vim.list_contains()

[kiyoon]

Thanks a lot for the awesome work!