Skip to content
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.

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

feat: allow repeating the last completion with . #870

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mikavilpas
Copy link
Contributor

This is a bit work in progress, but could implement dot-repeat (.) for simple completions. All cases might not work yet.

repeat-demo.mov

The algorithm is basically the same as in nvim-cmp. The way it works here is the following:

  • when a completion is selected, use the first text edit as the thing that can be repeated
  • simulate pressing keys so that it's possible to write to the repeat register (., dot). This register gets executed when the user presses the dot key later on. Note that neovim does not allow cleanly replacing text in a way that supports dot-repeat (nvim_buf_set_text in insert mode should update dot repeat neovim/neovim#19806 (comment))
  • press backspace repeatedly to erase the current word
  • add the new word
  • undo these changes to the buffer so that all the text edits can be applied cleanly (this is the previous behaviour)
  • allow the user to continue writing text (these are part of the repeating, as seen in the demo)

next steps

  • find out what cases need to be supported
  • idea: could use async instead off callbacks
  • idea: could eventually release this with an experimental (opt-in) status

Solves #182

@mikavilpas mikavilpas marked this pull request as draft January 3, 2025 15:32
@Saghen
Copy link
Owner

Saghen commented Jan 3, 2025

I'd be open to merging this early and making it opt-in: completion.accept.dot_repeatable = true

@mikavilpas
Copy link
Contributor Author

Nice! I just rebased and accounted for context.get_keyword() being deleted - it looks like it should now be done on the rust side. It seems to work for these simple cases.

To be honest, I haven't had the time to test any more cases than the ones in the demo. I think automatic parenthesis insertion could still be supported (might not work right now) - snippets might be too difficult (I don't know how they are implemented).

I'm leaving for a mini holiday today for a day or two, but if you want to merge this in sooner, it might work fine as an opt-in feature. In general, I have found that merging opt-in features often tends to speed things up quite a bit 👍🏻

@mikavilpas
Copy link
Contributor Author

Just quickly tested a case where parentheses are inserted - it seems to have an issue with positioning the cursor (although repeating does work surprisingly)

bug.mov

@mikavilpas
Copy link
Contributor Author

I think I found out the cause. It's outlined here (neovim/neovim#16571 (comment)), but I'll give the short version: if I understand correctly, using feedkeys adds keys to be executed into a buffer, and the keys are executed later. Previously I think I might have run them too soon.

I think I was able to fix the positioning bug by simply waiting until the keys have been scheduled (I think it runs later in the event loop which works), but I had to convert some code to use blink.cmp.Task. This has now caused a strange thing to happen:

position.mov

As you can see, the cursor goes too far to the right now. I can dig into why that happens later on.

But I also wanted to hear your thoughts on using blink.cmp.Task. I now noticed that some code is a mixture of passing callbacks and using tasks.

@mikavilpas
Copy link
Contributor Author

I also noticed that in my ripgrep plugin, blink now calls the source for every new character. This causes a lot of ripgrep searches, while previously the source was only called once. 😦

Maybe my async implementation has some issues?

@xzbdmw
Copy link
Contributor

xzbdmw commented Jan 6, 2025

This patch seems works for me

blink.cmp git:(main) ✗ git --no-pager diff  HEAD                                                        0 [04:37:51]
diff --git a/lua/blink/cmp/completion/accept/init.lua b/lua/blink/cmp/completion/accept/init.lua
index c4041d3..3811de5 100644
--- a/lua/blink/cmp/completion/accept/init.lua
+++ b/lua/blink/cmp/completion/accept/init.lua
@@ -78,6 +78,14 @@ local function accept(ctx, item, callback)
       -- OR Normal: Apply the text edit and move the cursor
       else
         table.insert(all_text_edits, item.textEdit)
+
+        local row = vim.api.nvim_win_get_cursor(0)[1]
+        local old_line = vim.api.nvim_get_current_line()
+        -- emulate builtin completion (dot repeat).
+        vim.fn.complete(item.textEdit.range.start.character + 1, { item.textEdit.newText })
+        -- restore old content
+        vim.api.nvim_buf_set_lines(0, row - 1, row, false, { old_line })
+
         text_edits_lib.apply(all_text_edits)
         -- TODO: should move the cursor only by the offset since text edit handles everything else?
         ctx.set_cursor({ ctx.get_cursor()[1], item.textEdit.range.start.character + #item.textEdit.newText + offset
 })

nvim-cmp has some heuristics to determine a good item.textEdit.newText (word) here.

item.textEdit.range.start.character + 1 should be the col offset at the time when we send request, I don't know how to get it.

The performance cost is 1ms ~ 4ms, the same as feedkey's cost in nvim-cmp.

This is the relevant coc.nvim dot-repeat pr using complete.

I'm just giving some idea here, IMO nvim-cmp uses feedkeys extensively, the hack may be something convenient for it but not for blink, you may want to go this way down.

@Saghen
Copy link
Owner

Saghen commented Jan 6, 2025

Whenever you feel this is ready for merging, mark it as ready to review and I'll add the bits to make it opt in. Thanks for all your work on this!

@mikavilpas
Copy link
Contributor Author

Nice, I appreciate it! There's one major bug left - I think that should be fixed before merging:

I created this demo to clarify what I meant. Each time the text flashes with the bright color, a new search is done (this is wrong).

Incorrect

invocations.mov

I think I have broken the old behaviour of only searching once, and using the following keypresses as filters. That can be seen here (text flashes only once)

Correct old behaviour 👍🏻

invocations-working.mov

@mikavilpas
Copy link
Contributor Author

@xzbdmw did you apply that patch on top of this PR, or did you apply it on top of the newest version (nightly blink)? Meaning, it that all that is needed for it to work? I could try it out. It looks much simpler, but I can't tell if it has some limitations or not.

@xzbdmw
Copy link
Contributor

xzbdmw commented Jan 7, 2025

Yeah, it apply on top of main branch. (there should be some edge case)

If you use auto_insert, this approach surprisingly triggers CursorMovedI, even if I set even_ignore = "all", I see
there are some ways to suppress some autocmds, but you guys should be much more familiar with the code base than me.

I'm listing a newest path:

blink.cmp git:(dot-repeat) git --no-pager diff  HEAD~1 HEAD                                             0 [01:20:30]
diff --git a/lua/blink/cmp/lib/text_edits.lua b/lua/blink/cmp/lib/text_edits.lua
index 2ce76fe..e1bd6c6 100644
--- a/lua/blink/cmp/lib/text_edits.lua
+++ b/lua/blink/cmp/lib/text_edits.lua
@@ -7,7 +7,26 @@ local text_edits = {}
 --- @param edits lsp.TextEdit[]
 function text_edits.apply(edits)
   local mode = context.get_mode()
-  if mode == 'default' then return vim.lsp.util.apply_text_edits(edits, vim.api.nvim_get_current_buf(), 'utf-8') en
d
+  if mode == 'default' then
+    local edit = edits[1]
+    local row = vim.api.nvim_win_get_cursor(0)[1]
+    local old_line = vim.api.nvim_get_current_line()
+    vim.o.eventignore = 'all'
+    -- emulate builtin completion (dot repeat).
+    vim.fn.complete(edit.range.start.character + 1, { edit.newText })
+
+    local feedkeys = function(keymap, mode)
+      vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes(keymap, true, false, true), mode, false)
+    end
+    -- exit completion mode
+    feedkeys('<c-x><c-z>', 'in')
+    -- restore old content, use nvim_buf_set_text to preserve extmark.
+    vim.api.nvim_buf_set_text(0, row - 1, 0, row - 1, -1, { old_line })
+    vim.lsp.util.apply_text_edits(edits, vim.api.nvim_get_current_buf(), 'utf-8')
+    vim.o.eventignore = ''
+
+    return
+  end
 
   assert(mode == 'cmdline', 'Unsupported mode for text edits: ' .. mode)

@mikavilpas
Copy link
Contributor Author

I tried that implementation. I like that it's so short and simpler to understand. It passed all my tests in blink-ripgrep, including the cases in my repeat demo.

But a more complicated case seems to fail:

multiple.mov

I noticed my implementation also fails to work in this case, so I will fix that next.

@xzbdmw
Copy link
Contributor

xzbdmw commented Jan 7, 2025

Seems work for me (I have empty config)

iShot_2025-01-08_03.50.59.mp4

@mikavilpas
Copy link
Contributor Author

mikavilpas commented Jan 9, 2025

I fixed the bug with many subsequent invocations, and that (basic) case works now

many-repeats.mov

There is one remaining bug that I know of: selecting other completion items now just inserts them instead of just highlighting them. I'm a bit stressed for time, but will take a look at this later.

edit: also, the repeated invocations bug from earlier is not a bug after all. My bad on that one. 🙂

@mikavilpas
Copy link
Contributor Author

Spent a little bit more time on this, but was not able to fix the bug.

scroll-inserts.mov

@Saghen would you have some idea on what could be wrong?

I looked into how events are handled in lua/blink/cmp/lib/buffer_events.lua, but I think no new events are handled by accident. Maybe it's possible that keeping track of the last inserted character is somehow messed up?

@Saghen
Copy link
Owner

Saghen commented Jan 13, 2025

We update the context on every CursorMoved and TextChanged event, so during auto_insert, to prevent the menu from updating, we suppress all events. Sounds like for some reason we're not suppressing one of the events (likely TextChanged).

P.S. Sorry I haven't been more active to help you with this, work has been busy lately

@mikavilpas
Copy link
Contributor Author

No worries - a normal day in open source 🙂

I think you're right about the event stuff. I wasn't able to cleanly solve all of my test cases, but I found out not applying the preview for the selected completion fixes the bug in my previous message:

diff --git a/lua/blink/cmp/completion/list.lua b/lua/blink/cmp/completion/list.lua
index eeae0fe..40853c5 100644
--- a/lua/blink/cmp/completion/list.lua
+++ b/lua/blink/cmp/completion/list.lua
@@ -225,6 +225,7 @@ function list.undo_preview()
 end
 
 function list.apply_preview(item)
+  if true then return end
   -- undo the previous preview if it exists
   list.undo_preview()
   -- apply the new preview

Maybe you have an idea what a clean way to do this could look like?

@Saghen
Copy link
Owner

Saghen commented Jan 13, 2025

It should be ignored because the changedtick of the buffer changes, so I'm wondering if for some reason it's no longer changing. What happens if you set self.ignore_next_text_changed = true and ignore the changed tick in the function I sent?

@mikavilpas
Copy link
Contributor Author

Just found out that additional text edits don't work yet 😄 I can take a look at this problem in the coming days.

oops.mov

@Saghen
Copy link
Owner

Saghen commented Jan 13, 2025

Yeah they should be applied via vim.lsp.utils.apply_text_edits like normal, before applying the main text edit

@mikavilpas
Copy link
Contributor Author

Yeah, applying the additional text edits before the actual (now repeatable) text edit seems to work great

nice.mov

Just pushed this change. But this still needs applying the if true then return end hack to disable list.apply_preview(item). I could not see any change when adding the following change:

Details

diff --git a/lua/blink/cmp/lib/buffer_events.lua b/lua/blink/cmp/lib/buffer_events.lua
index dcca8b8..d2f7ee6 100644
--- a/lua/blink/cmp/lib/buffer_events.lua
+++ b/lua/blink/cmp/lib/buffer_events.lua
@@ -131,7 +131,8 @@ function buffer_events:suppress_events_for_callback(cb)
 
   local is_insert_mode = vim.api.nvim_get_mode().mode == 'i'
 
-  self.ignore_next_text_changed = changed_tick_after ~= changed_tick_before and is_insert_mode
+  -- self.ignore_next_text_changed = changed_tick_after ~= changed_tick_before and is_insert_mode
+  self.ignore_next_text_changed = true
 
   -- HACK: the cursor may move from position (1, 1) to (1, 0) and back to (1, 1) during the callback
   -- This will trigger a CursorMovedI event, but we can't detect it simply by checking the cursor position

@mikavilpas
Copy link
Contributor Author

Without disabling list.apply_preview(item) the behaviour now looks like some type of desync. I think the previous preview is not cleared correctly, and it results in extra text being added in this case.

weird.mov

Maybe you could take a look at it? The event handling logic looks a bit intricate to me right now.

@Saghen
Copy link
Owner

Saghen commented Jan 18, 2025

I spent a couple hours trying to get this working but wasn't successful. This adds more complexity than I was hoping for since making text edits async means downstream also needs to be async. It's odd though, since the feedkeys implementation doesn't seem like it needs to be async, it appears to be running synchronously.

I'm toying around with a vim.fn.completion based solution, recommended by @xzbdmw, in the repeat-via-completion branch, which keeps everything sync. #1033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants