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

Fix autosnippets #709

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

carbon-steel
Copy link

Hey @L3MON4D3, this PR is in regards to #706.

You'll have to forgive my code and inexperience. I've only ever contributed to C++ codebases. I believe the tests were meant for an Ubuntu environment. I am running arch so I'll have to figure out how to run them tomorrow (unless you know what I should do?).

I'd be happy to hear any notes you may have :)

@L3MON4D3
Copy link
Owner

Oh, I'm also running them on arch, they should work as-is.
Could you outline why this works?
Does it work if only a single character is inserted?

@L3MON4D3
Copy link
Owner

Oh and a big thank you already for taking a look, awesome! :)

@carbon-steel
Copy link
Author

Oh, I'm also running them on arch, they should work as-is.

Ah, I got confused because of the use of apt-get in the github workflows.

Could you outline why this works?

b:changedtick is a monotonically increasing per-buffer count of the number of changes made to the buffer.

You can observe its behavior by using:
:autocmd TextChangedI * echom b:changedtick

Then type some stuff on the buffer you called the command from. You can then see how b:changedtick changes with each character you insert by looking at the printed results:
:messages

You can see how b:changedtick gets incremented by one with each character you type in. If you yank some string with multiple characters and you paste it (using in insert mode), you'll notice that b:changedtick increases by the number of characters that were pasted. However, the TextChangedI event only occurs once after the paste is complete. By this, we can distinguish when a TextChangedI event was triggered by single character from a normal keypress vs multiple characters from . I use this to prevent autosnippets from expanding when text was pasted using .

Does it work if only a single character is inserted?

No, this will not work if you paste a single character with (I checked) since b:changedtick's behavior will be the same as if you had typed that single character in yourself.

@carbon-steel
Copy link
Author

carbon-steel commented Dec 30, 2022

As a side note, I believe remapping <C-r> to just use the paste command from the vim.api could fix this problem for all cases:

local function insertModePaste()
  local register = vim.cmd([[
  let s:register = getchar()
  echo  s:register
  ]], true)
  vim.api.nvim_paste(vim.fn.getreg(register), true, -1)
end

map('i', '<C-r>', insertModePaste)

I don't know if you think this should go in the plugin. If not, users could manually put this code in their configs to completely fix this issue for themselves for all cases (if the fix from this pull request is not sufficient for them).

@L3MON4D3
Copy link
Owner

Thanks for the clarification👍
Seems like an okay solution, it's a bit involved, and doesn't work for all cases (if the first action is such a paste, luasnip_changedtick would be nil and we would auto-expand (not 100% sure this is possible but it might be)), but it is better that the status quo.

Changing C-R seems dangerous 😅😅

AFAICT ultisnips could have had this same issue, would you be interested in figuring out how they handled it?

@carbon-steel
Copy link
Author

I've fixed the test that I broke and I added a test to document and preserve the new behavior. I've been using Luasnip with my changes and it seems to work well. I think this pull request is ready to go.

What do you think about adding the code from my above post to the wiki to let people know they can fully fix this issue for themselves if the add it to their configs?

@carbon-steel
Copy link
Author

I just tested the pasting triggers in insert mode with ultisnips and their autosnippets just expand in every case. It looks like they did not solve this problem.

@L3MON4D3
Copy link
Owner

Thank you for taking a look at ultisnips, in that case this seems to be the best way to do it

I've fixed the test that I broke and I added a test to document and preserve the new behavior. I've been using Luasnip with my changes and it seems to work well. I think this pull request is ready to go.

What do you think about adding the code from my above post to the wiki to let people know they can fully fix this issue for themselves if the add it to their configs?

Yes! Would you mention that in the commit-message as well? Or put it into the doc (since it fixes an issue, and is not just a new feature), though I'm not sure which section would be appropriate

Comment on lines +193 to +194
-- Luasnip_just_inserted makes sure we only trigger autosnippets after we inserted characters in insert mode.
-- vim.b.Luasnip_last_changedtick allows us to compute how many changes occurred in the file between two TextChanged* events. We use this information to distinguish typing characters by hand vs using <C-r> to paste multiple characters in insert mode. We want to allow expanding autosnippets in the first case and not in the second. After some investigation, this is the only way I found to distinguish the two cases.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% happy with this comment, I think it could be more descriptive, ie:

  1. what is the issue
  2. how can typing characters and pasting multiple characters be distinguished
  3. the solution

Also, could you make the comment 80-100 characters wide? Normally stylua should take care of this, but here it doesn't 🤷

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Actually, maybe stylua isn't supposed to..)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and in a similar vein, could you shorten the comments in the tests?

autocmd InsertCharPre * lua Luasnip_just_inserted = true
autocmd TextChangedI,TextChangedP * lua if Luasnip_just_inserted then require("luasnip").expand_auto() Luasnip_just_inserted=nil end
]] or "")
autocmd TextChangedI,TextChangedP * lua if Luasnip_just_inserted then Luasnip_just_inserted=nil if vim.b.Luasnip_last_changedtick == nil or vim.b.changedtick==vim.b.Luasnip_last_changedtick + 1 then require("luasnip").expand_auto() end vim.b.Luasnip_last_changedtick = vim.b.changedtick end
Copy link
Owner

@L3MON4D3 L3MON4D3 Jan 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, could you spread this over multiple lines to increase readability?
Alternative would be to switch to lua-autocommands, but I don't think this alone warrants it.

@L3MON4D3
Copy link
Owner

Or put it into the doc (since it fixes an issue, and is not just a new feature), though I'm not sure which section would be appropriate

Actually, this behaviour might be unexpected, would you add a new section on it in DOC.md?

@@ -291,6 +303,22 @@ describe("add_snippets", function()
{2:-- INSERT --} |]],
})

-- Test to make sure that autosnippets do not get triggered while pasting in insert mode.
feed("<ESC>dd") -- clear line
helpers.feed_command("set paste") -- disable autosnippets
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, why does set paste disable autosnippets? Had a cursory read of the help-section on it, but nothing in there caught my eye

@L3MON4D3
Copy link
Owner

Hey, are you still up for this? I could also take over, but I don't understand the set paste-part, I'd appreciate if you can explain that to me 😅

@leiserfg
Copy link
Contributor

I don't get it either, also paste is deprecated it may stop working at any point.

@L3MON4D3
Copy link
Owner

Oh good to know, I guess we can ignore that then.
Still, important to know what's going on there, maybe it does not only happen when paste is set, but under other circumstances as well

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