fix(jump): consumes a char on dot-repeat when target not in text

[abeldekat]

Resolve #2290

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

[abeldekat]

I wonder if the vim.schedule for undo could also be achieved with a one-time mode change event.

[echasnovski]

Thanks for the PR!

My initial impression is that the change does more than is needed to fix the described issue. Or it fixes more than described in the commit or issue.

Because the part of "works on first try but doesn't work on dot-repeat" is probably meant to be fixed by moving the logic from make_expr_jump into jump() itself. Especially since there is a MiniJump._is_exp private indicator.

I think the following should fix the issue:

diff --git a/lua/mini/jump.lua b/lua/mini/jump.lua
index 3ac813d8..7165a1bf 100644
--- a/lua/mini/jump.lua
+++ b/lua/mini/jump.lua
@@ -181,6 +181,14 @@ MiniJump.state = {
 MiniJump.jump = function(target, backward, till, n_times)
   if H.is_disabled() then return end
 
+  -- Ensure to undo "consuming a character" effect if there is no target found
+  -- Do it here to act on dot-repeat
+  local has_changed_cursor = false
+  local undo_no_move = function()
+    if not has_changed_cursor then vim.cmd('undo!') end
+  end
+  if MiniJump._is_expr then vim.schedule(undo_no_move) end
+
   -- Dot-repeat should not change the state, so save it to later restore
   local is_dot_repeat = MiniJump._is_expr and not MiniJump._is_expr_init
   MiniJump._is_expr, MiniJump._is_expr_init = nil, nil
@@ -234,7 +242,7 @@ MiniJump.jump = function(target, backward, till, n_times)
 
   -- Track cursor position to account for movement not caught by `CursorMoved`
   H.cache.latest_cursor = H.get_cursor_data()
-  H.cache.has_changed_cursor = not vim.deep_equal(H.cache.latest_cursor, init_cursor_data)
+  has_changed_cursor = not vim.deep_equal(H.cache.latest_cursor, init_cursor_data)
 
   -- Restore the state if needed
   if is_dot_repeat then
@@ -404,11 +412,6 @@ H.make_expr_jump = function(backward, till)
     if isnt_repeat_jump and target == nil then return '<Esc>' end
     H.update_state(target)
 
-    vim.schedule(function()
-      if H.cache.has_changed_cursor then return end
-      vim.cmd('undo!')
-    end)
-
     -- Set a flag to distinguish first call from dot-repeat
     MiniJump._is_expr_init = true
 

[abeldekat]

My initial impression is...

Yes, you are right. I even started with another version of your approach.

Whilst working on the issue, I encountered behavior which I thought to be a bug. Only on the last moment I saw that it was to be expected, also understanding this test.

Because of that misperception, I thought I needed to get rid of this early return.

I submitted the current version because it worked and I think it has two main benefits:

• Only check if target is present when absolutely necessary
• No need to check if the cursor has changed in order to undo, as that check is a derivative of knowing that a successful jump has occurred. I can schedule a direct undo.

Would you like me to resubmit with your version?

[echasnovski]

I submitted the current version because it worked and I think it has two main benefits:

* Only check if [target](https://github.com/nvim-mini/mini.nvim/blob/0098de999048af0539183d625c52d733318a441b/lua/mini/jump.lua#L200) is present when absolutely necessary

* No need to check if the cursor has changed in order to undo, as that check is a derivative of knowing that a successful jump has occurred. I can schedule a direct undo.

Both of these are nice improvements, but they are not about fixing the issue. It is hard to see if the approach is good if it does several things at once (i.e. fixes the issue and refactors the code to address two points above). I am not opposed to refactor but it needs to be a separate commit that doesn't touch tests and still passes them.

So I think following "as less diff as possible" while fixing the issue is a better approach here. If my above diff works, let's use it at least for now.

[abeldekat]

If my above diff works, let's use it at least for now.

Done.

Would you accept a refactoring pr?

[echasnovski]

Would you accept a refactoring pr?

Maybe after this is merged. But there are some other things in jump() code that caught my eye as worth addressing (it is a fairly old code). So there will be a lot of nit picking.

[echasnovski]

Since tests fail on Nightly (due to upstream neovim/neovim#38137), I've merged into temporary branch to polish and push manually.

Thanks again for noticing the issue and doing the PR!

Edit: Should now be part of the main with a bit adjusted test code and commit message.