feat(jump)!: make dot-repeat jumping separate from regular jumping

[abeldekat]

Resolve #2054

• I have read CONTRIBUTING.md
• I have read CODE_OF_CONDUCT.md

[abeldekat]

I am not sure if the extra tests in 'works after jump in Operator-pending mode' are actually needed, as the behavior of ; has not been changed.
It still reuses the jump from dot-repeat. The motion is only different because dot-repeat itself has been changed.

[echasnovski]

Thanks for the PR!

The idea and tests overall look good! The large amount of comments is only there because it is possible to focus on little things :)

[echasnovski]

Also, please adjust commit message to fit the new PR title. Dot-repeat is technically not only about changes (yank can be dot-repeated under some circumstances, for example).

[abeldekat]

Also, please adjust commit message to fit the new PR title. Dot-repeat is technically not only about changes (yank can be dot-repeated under some circumstances, for example).

Indeed. However, as I understand this PR, the title should be:

"ensure dot-repeat jumping is not affected by regular jumping"

[echasnovski]

Indeed. However, as I understand this PR, the title should be:

"ensure dot-repeat jumping is not affected by regular jumping"

It goes both ways. And it is also a breaking change. So updated to be "make dot-repeat jumping separate from regular jumping".

[echasnovski]

Why extra check for the forced charwise mode is needed?

[abeldekat]

When the operation is invoked the first time in make_expr_jump we are not dot-repeating and the state should not be restored. Otherwise, after the jump, state.mode is still 'no', as it was in make_expr_jump.

FAIL in tests/test_jump.lua | state | updates `mode`:
  Failed expectation for equality
  Cause: different string length
  Left:  "no"
  Right: "nov"
  Traceback:
    tests/test_jump.lua:185

[echasnovski]

Hmm... Still, the extra check doesn't feel like a good solution to any problem it is meant to solve.

[abeldekat]

Apologies, I misread the question. Indeed, this is an example of doing too much...
Changed into: local needs_restore = MiniJump._is_expr and MiniJump.state.mode ~= 'no'

[echasnovski]

So it is to differentiate initial application of expression mapping from its dot-repeat.

I still don't think that relying on checking the mode is a robust solution. Couldn't immediately find the failing example, though.

There is a more direct approach to distinguish first and second+ calls of expression mapping output: set another flag (like _expr_map_init) in the expression mapping RHS. Then the dot-repeat is MiniJump._expr_map and not MiniJump._expr_map_init.

As it already feels like a nit picking, I'll merge into a temporary branch and adjust myself.

[abeldekat]

Yes...)

I did think about that extra flag. That's also an extra something to reset though... Anyways, no nitpicking. Your feedback is very valuable as always and I look forward to seeing the final version

[abeldekat]

I pushed the new commit

[abeldekat]

Strange, some of my review comments end up as 'pending'.

About the question regarding forced charwise mode:

Why extra check for the forced charwise mode is needed?

if MiniJump.is_expr and not (MiniJump.state.mode == 'no' and vim.fn.mode(1) == 'nov') then

Apologies, I misread the question. Indeed, this is an example of doing too much...
Changed into:

local needs_restore = MiniJump._is_expr and MiniJump.state.mode ~= 'no'

[echasnovski]

Thanks again for the PR! I've merged into a temporary branch to polish a bit and then merge into main.

[echasnovski]

The work from this PR should now be part of the latest main. Thanks again for the PR!

Couple of notes:

• After checking the PR locally, I noticed that this comment was not really addressed. It went unnoticed primarily because there was no dedicated test added for this.

  The reason for this was that created expression hard coded the initial target. So doing something like fe -> d; (hard coded e as target) -> fx -> . (still used e as target although it should use x, as in nvim --clean). Using nil in case of repeat_jump (i.e. ;) solved the issue.

• Adjusted some tests to be more to the point.

• Added small note in documentation to show that the dot-repeat behavior is intentional (i.e. "as in clean Neovim").