refactor(jump): only do extra search for target if there was no jump

[abeldekat]

Details:

Instead of always checking if target exists in text, only do so if there was no jump.
Additional benefit: The code using the cursor to check if a jump occurred can be replaced by a more direct check

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

[abeldekat]

@echasnovski,

The solution applied to solve issue #688 in February 2024 had a side-effect regarding highlighting.

Relevant part of diff:

@@ -364,6 +365,10 @@ H.make_expr_jump = function(backward, till)
     if target == nil then return '<Esc>' end
     H.update_state(target, backward, till, vim.v.count1)

+    vim.schedule(function()
+      if H.cache.has_changed_cursor then return end
+      vim.cmd('undo' .. (vim.fn.has('nvim-0.8') == 1 and '!' or ''))
+    end)
     return 'v<Cmd>lua MiniJump.jump()<CR>'

Whilst the scheduled undo command restored the character, it also prevented the highlighting.

Scenario with file containing a single word:

mark

Using its previous commit '1d49300d50a2'

• Open file, position cursor on the 'a'
• Type dfm
• Notice that the 'a' disappears, cursor is on 'r'
• Notice that 'm' is highlighted

Using the fix, commit '4f69339'

• Open file, position cursor on the 'a'
• Type dfm
• Notice that text is not modified, as expected
• Notice that m is not highlighted
• Type .
• Notice that 'm' is highlighted, as dot-repeat has no scheduled undo

To restore the original highlighting:

diff --git a/lua/mini/jump.lua b/lua/mini/jump.lua
index 234510b9..7bf50dd6 100644
--- a/lua/mini/jump.lua
+++ b/lua/mini/jump.lua
@@ -239,7 +239,8 @@ MiniJump.jump = function(target, backward, till, n_times)
   -- Ensure to undo "consuming a character" effect if there is no target found
   -- Do it here to also act on dot-repeat
   if has_jumped or not is_expr then return end
-  vim.schedule(function() vim.cmd('undo!') end)
+  -- vim.schedule(function() vim.cmd('undo!') end)
+  vim.api.nvim_create_autocmd('ModeChanged', { once = true, callback = function() vim.cmd('un
do!') end })
 end

 --- Make smart jump

I think it also is more precise to use the ModeChanged event.

[echasnovski]

Thanks for the PR!

I'll take a closer look a bit later (probably next week), as I can not quickly see if the new approach is optimal. Should be, as tests pass and it is negative diff 💪

To restore the original highlighting:
...
I think it also is more precise to use the ModeChanged event.

I don't really understand why ModeChanged event is better or even why it works. Initial reaction here is that executing the undo! command without vim.schedule might be dangerous due to how Neovim works (:h textlock, :h :map-expression, and similar things).

[abeldekat]

I don't really understand why ModeChanged event is better or even why it works. Initial reaction here is that executing the undo! command without vim.schedule might be dangerous due to how Neovim works (:h textlock, :h :map-expression, and similar things).

Me neither. I do think that the 'undo!' is the culprit here. If the command is deferred instead of scheduled, the highlighting is removed when the defer activates.

In addition to 'ModeChanged', 'CursorMoved' also works.

I also tried with 'nvim_feedkeys' and the escape character and did not succeed. In flash.nvim I found a snippet that does work. The diff with that code applied:

diff --git a/lua/mini/jump.lua b/lua/mini/jump.lua
index 234510b9..8fc8cb0e 100644
--- a/lua/mini/jump.lua
+++ b/lua/mini/jump.lua
@@ -239,7 +239,11 @@ MiniJump.jump = function(target, backward, till, n_times)
   -- Ensure to undo "consuming a character" effect if there is no target found
   -- Do it here to also act on dot-repeat
   if has_jumped or not is_expr then return end
-  vim.schedule(function() vim.cmd('undo!') end)
+
+  -- vim.schedule(function() vim.cmd('undo!') end)
+  local t = function(str) return vim.api.nvim_replace_termcodes(str, true, true, true) end
+  vim.api.nvim_feedkeys(t('<C-\\><C-n>'), 'nx', false)
+  vim.api.nvim_feedkeys(t('<esc>'), 'n', false)
 end

 --- Make smart _jump_

The disadvantage of that code is that test "can be dot-repeated if did not jump at first" breaks. I think one can argue that dot-repeating an operation that did not occur lacks intuitiveness. It's not supported by builtin dot-repeat. Surprisingly, flash is able to dot-repeat when initially the jump was not possible. Flash is not using 'expr' mappings though...

[echasnovski]

I also tried with 'nvim_feedkeys' and the escape character and did not succeed. In flash.nvim I found a snippet that does work. The diff with that code applied:

I would rather avoid using feedkeys() whenever possible. It proved to cause problems over time.

The disadvantage of that code is that test "can be dot-repeated if did not jump at first" breaks. I think one can argue that dot-repeating an operation that did not occur lacks intuitiveness. It's not supported by builtin dot-repeat.

Yeah, indeed built-in dot-repeat does not work that way. The current behavior still makes more sense to me as it allows a smoother repeated actions like: dtx -> switch buffer -> . -> etc.

[echasnovski]

Thanks again for the PR! It should now be a part of main after I made couple of small refactors that were bothering me for quite a while. A total diff of -16 for the core function while passing all tests is a very good result!

The solution applied to solve issue #688 in February 2024 had a side-effect regarding highlighting.

Regarding this issue, I am not able to reproduce the steps you describe (maybe anymore or maybe I wasn't able to in the first place, don't remember). With the latest change in how Operator-pending and dot-repeat works, the best approach would be to keep highlighting only in the direction that dot-repeat will repeat, but as far as I can tell there is no existing code for that while adding one would be not concise.

I was thinking about not highlighting when jumping in Operator-pending mode (since it is relatively easy to implement now), but indeed highlighting places where dot-repeat would take the cursor sounds useful. So not yet sure what to do here. One thing I am certain: I'd rather have any highlighting mismatch than add feedkeys() or ModeChanged autocommand workarounds.

[abeldekat]

Thanks!

  if is_expr and not has_jumped then vim.schedule(function() vim.cmd('undo!') end) end

I had this originally and thought you'd like the undo statement outside the if better. But indeed, this is more expressive.

[echasnovski]

I had this originally and thought you'd like the undo statement outside the if better. But indeed, this is more expressive.

Usually - yeah, early return in this kind of situations is nice. But in this case it fits a single line (although pretty convoluted) and reads almost to the point of not needing the comment. Two lines here is more simple though, so the this decision can depend on time of day or day of the week :)

[abeldekat]

jump_no_highlight.mp4

The solution applied to solve issue #688 in February 2024 had a side-effect regarding highlighting.

Regarding this issue, I am not able to reproduce the steps you describe

That's strange. See the video.

With the latest change in how Operator-pending and dot-repeat works, the best approach would be to keep highlighting only in the direction that dot-repeat will repeat, but as far as I can tell there is no existing code for that while adding one would be not concise.

I agree

So not yet sure what to do here. One thing I am certain: I'd rather have any highlighting mismatch than add feedkeys() or ModeChanged autocommand workarounds.

If the highlighting is a requirement, than perhaps the onetime CursorMoved event, as shown in the video, can be considered. I think it might be an appropriate workaround, as indeed the cursor is moved when a character is consumed.

Shall I create a separate issue?

[echasnovski]

That's strange. See the video.

No highlighting after the first dfm I did reproduce. I could not reproduce the dot-repeat part (I saw again no highlighting, but the steps describe yes highlighting), which I thought was the issue.

• Type .

• Notice that 'm' is highlighted, as dot-repeat has no scheduled undo

---

If the highlighting is a requirement, than perhaps the onetime CursorMoved event, as shown in the video, can be considered. I think it might be an appropriate workaround, as indeed the cursor is moved when a character is consumed.

That is such a niche and undocumented behavior (CursorMoved trigger after "consuming" a character in Operator-pending mode if cursor did not move) that I am not fully confident there is a guarantee it will be preserved forever.

Shall I create a separate issue?

Maybe that's a good idea, yeah. At least to (concisely) describe all this behavior. Please make a feature request that is more in terms of "adjust highlighting after jumping in Operator-pending mode", because this is the core problem here. Which my guess would require a fundamental solution: either by not highlighting, or rethinking the whole way of doing expression mapping jumping (and the undo! that it requires).