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

Merged

Hashiraee merged 4 commits from multiline-issue into main 2024-05-13 11:01:29 -04:00

Conversation 12 Commits 4 Files changed 2 +9 -7

Hashiraee commented 2024-05-12 12:02:01 -04:00 (Migrated from github.com)

Copy link

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:

• Split the the completion_text into lines using vim.split()
• Then update the new cursor position using nvim_win_set_cursor()

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.

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: - Split the the completion_text into lines using vim.split() - Then update the new cursor position using nvim_win_set_cursor() 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 2024-05-12 13:11:29 -04:00 (Migrated from github.com)

Copy link

Using this branch:

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.

2. Accepted with Tab:

3. Accepted with Tab again:

4. Accepted with Tab again:

Using this branch:  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.  2. Accepted with Tab:  3. Accepted with Tab again:  4. Accepted with Tab again: 

👍 1

dannyraym commented 2024-05-12 13:13:53 -04:00 (Migrated from github.com)

Copy link

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

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

👍 1

Hashiraee commented 2024-05-12 13:16:49 -04:00 (Migrated from github.com)

Copy link

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!

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 2024-05-12 13:17:20 -04:00 (Migrated from github.com)

Copy link

The new commit fixed it. LGTM.

The new commit fixed it. LGTM.

Hashiraee commented 2024-05-12 13:21:19 -04:00 (Migrated from github.com)

Copy link

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

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

super-jacob commented 2024-05-12 15:24:39 -04:00 (Migrated from github.com)

Copy link

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

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

Hashiraee commented 2024-05-12 17:12:08 -04:00 (Migrated from github.com)

Copy link

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

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

sm-victorw (Migrated from github.com) approved these changes 2024-05-12 19:28:44 -04:00

sm-victorw (Migrated from github.com) left a comment

Copy link

This solves issue #2 without having to check for specific versions of Neovim, which is nice, as we won't have to continue updating this logic

This solves issue #2 without having to check for specific versions of Neovim, which is nice, as we won't have to continue updating this logic

b0o commented 2024-05-12 20:36:55 -04:00 (Migrated from github.com)

Copy link

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,

> 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()`: ```lua 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: ```patch 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 2024-05-13 02:17:09 -04:00 (Migrated from github.com)

Copy link

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():

> > 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()`: > > ```lua > 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 > 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()`: 

Hashiraee commented 2024-05-13 08:42:15 -04:00 (Migrated from github.com)

Copy link

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?

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 2024-05-13 08:52:23 -04:00 (Migrated from github.com)

Copy link

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

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

Sign in to join this conversation.

Reviewers

No reviewers

sm-victorw

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

supermaven-inc/supermaven-nvim!7

No description provided.