supermaven-inc / supermaven-nvim

The official Neovim plugin for Supermaven
https://supermaven.com/
MIT License
304 stars 16 forks source link

fix(completion_preview.lua): fix for cursor position for multi line completions #7

Closed Hashiraee closed 1 month ago

Hashiraee commented 1 month ago

The main change is at the end of the CompletionPreview.on_accept_suggestion(), I calculate the new cursor position based on the number of lines in the completion_text.

So I just:

Setting the cursor position using nvim_win_set_cursor() is a more standard approach, especially if users have certain keybindings for specific keys compared to PR #2.

This fixes Issue #2.

amirhhashemi commented 1 month ago

Using this branch: image

The problem with multi-line suggestions is resolved. Meaning, after accepting a multi-line suggestion the cursor moves correctly. But in other cases where the suggestion is on the line, it's completely broken. 1. image

  1. Accepted with Tab: image
  2. Accepted with Tab again: image
  3. Accepted with Tab again: image
danthomps commented 1 month ago

@amirhhashemi You're right. We have a fix in the discord for it. I'm sure he will add it to the PR soon.

Hashiraee commented 1 month ago

Yep, we already have a fix, it does the completion but moves it to a weird spot. I will make the PR now.

Edit: done, it is fixed now!

amirhhashemi commented 1 month ago

The new commit fixed it. LGTM.

Hashiraee commented 1 month ago

Can be merged now, Issue #2 is resolved @super-jacob!

super-jacob commented 1 month ago

This looks good. I'll wait for @victorw-xyz's review before merging.

Hashiraee commented 1 month ago

Added a fix for get_uft8_length() using neovim's built-in vim.fn.strlen()

b0o commented 1 month ago

Added a fix for get_uft8_length() using neovim's built-in vim.fn.strlen()

I was running into the 8000-char limit as well and ended up on this PR.

I question whether using vim.fn.strlen() or the other string.byte() method is necessary at all. Lua's string.len() / # operator already return the byte length of strings with multi-byte characters, the same as the original string.byte() implementation and vim.fn.strlen():

str = "😊"
print(#str) -- 4
print(str:len()) -- 4
print(vim.fn.strlen(str)) -- 4
print(#{str:byte(1, -1)}) -- 4

The same can be said for other multi-byte characters, like € (3) and ñ (2).

Why not just use #? Should be able to ditch util.get_utf8_length and then do:

diff --git a/lua/supermaven-nvim/binary/binary_handler.lua b/lua/supermaven-nvim/binary/binary_handler.lua
index 7a83f42..d7a0fe7 100644
--- a/lua/supermaven-nvim/binary/binary_handler.lua
+++ b/lua/supermaven-nvim/binary/binary_handler.lua
@@ -71,7 +71,7 @@ function BinaryLifecycle:on_update(buffer, file_name, event_type)
   local cursor = api.nvim_win_get_cursor(0)
   if cursor ~= nil then
     local prefix = self:save_state_id(buffer, cursor, file_name)
-    local offset = u.get_utf8_length(prefix)
+    local offset = #prefix
     updates[#updates + 1] = {
       kind = "cursor_update",
       path = file_name,
3rd commented 1 month ago

Added a fix for get_uft8_length() using neovim's built-in vim.fn.strlen()

I was running into the 8000-char limit as well and ended up on this PR.

I question whether using vim.fn.strlen() or the other string.byte() method is necessary at all. Lua's string.len() / # operator already return the byte length of strings with multi-byte characters, the same as the original string.byte() implementation and vim.fn.strlen():

str = "😊"
print(#str) -- 4
print(str:len()) -- 4
print(vim.fn.strlen(str)) -- 4
print(#{str:byte(1, -1)}) -- 4

The same can be said for other multi-byte characters, like € (3) and ñ (2).

Why not just use #? Should be able to ditch util.get_utf8_length and then do:

diff --git a/lua/supermaven-nvim/binary/binary_handler.lua b/lua/supermaven-nvim/binary/binary_handler.lua
index 7a83f42..d7a0fe7 100644
--- a/lua/supermaven-nvim/binary/binary_handler.lua
+++ b/lua/supermaven-nvim/binary/binary_handler.lua
@@ -71,7 +71,7 @@ function BinaryLifecycle:on_update(buffer, file_name, event_type)
   local cursor = api.nvim_win_get_cursor(0)
   if cursor ~= nil then
     local prefix = self:save_state_id(buffer, cursor, file_name)
-    local offset = u.get_utf8_length(prefix)
+    local offset = #prefix
     updates[#updates + 1] = {
       kind = "cursor_update",
       path = file_name,

Correct, just verified it. I think #str and str:len() are the same thing, and #str is much faster than strlen(): image

Hashiraee commented 1 month ago

I changed vim.fn.strlen(str) to #str as that is indeed much faster (thanks @b0o and @3rd). As for getting rid of the util function itself, I leave that up to @victorw-xyz.

With this everything should be taken care of now. I think it can be merged now @super-jacob?

WilliamWelsh commented 1 month ago

Thanks for fixing this, this issue has made it completely unusable for me in the meantime