Insert link completion deletes line
AckslD opened this issue · 17 comments
OS type:
- Unix
- Windows
- Other ([SPECIFY])
Vim:
- vim
- neovim
- Other ([SPECIFY])
Vim version:
NVIM v0.7.0-dev+907-gb781a6e50
Reproduce steps:
Example:
Typing an anchor, for example [section](#section) inline deletes the line, see following gif:
Could this be a compatibility issue with cmp?
Expected:
The line not to be deleted.
Actual:
The line is deleted and replaced with the anchor.
Hi @AckslD, thanks for opening an issue :)
I'd like to verify that this is definitely not a compatibility issue before starting to look into it. Can you disable all plugins and only load mkdx and then check if the issue still occurs?
If it still happens even with all plugins disabled then it could be some other configuration which is interfering. In this case I would like to see the smallest vimrc possible which still causes this issue so that I can test it for myself.
If the issue indeed disappears when plugins are disabled then it is likely some plugin compatibility issue. In this case your best bet would be to see if you can disable the conflicting plugin for markdown files.
@SidOfc Thanks for your response! I tried first to just disable cmp at least but the issue is still there. I'm trying to reproduce with a minimal config. I'm trying to use the following:
vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvim/site]]
local package_root = '/tmp/nvim/site/pack'
local install_path = package_root .. '/packer/start/packer.nvim'
local function load_plugins()
require('packer').startup {
{
'wbthomason/packer.nvim',
{
'SidOfc/mkdx',
setup = function()
vim.g["mkdx#settings"] = {
highlight = {enable = 1},
enter = {shift = 1},
links = {
fragment = {complete = 1},
external = {enable = 1},
},
toc = {text = 'Table of Contents', update_on_write = 1},
fold = {enable = 1},
}
end
},
},
config = {
package_root = package_root,
compile_path = install_path .. '/plugin/packer_compiled.lua',
display = { non_interactive = true },
},
}
end
_G.load_config = function()
end
if vim.fn.isdirectory(install_path) == 0 then
print("Installing dependencies.")
vim.fn.system { 'git', 'clone', '--depth=1', 'https://github.com/wbthomason/packer.nvim', install_path }
end
load_plugins()
require('packer').sync()
vim.cmd [[autocmd User PackerComplete ++once echo "Ready!" | lua load_config()]]but when I run this and type # in the link I get the error:
Error detected while processing function mkdx#Complete[2]..61[15]..56[9]..57[5]..44:
line 5:
E716: Key not present in Dictionary: "tokens.header, '') | endif"
Any idea why?
In my own init.lua I do not use the setup function of packer.nvim. Instead I have a small "plugins" section in which I tell packer which plugins I want. Further down in my configuration, separate sections can be found for each plugin.
Feel free to take a look at how I configured mkdx:
Additionally, I had nvim-cmp in there for roughly two months, at the time of writing this comment it's only been 4 days ago since I decided to remove it from my configuration:
During my time with nvim-cmp I did not notice this issue and I did indeed use the same feature you are having issues with (fragment link autocomplete).
My guess is that initializing the configuration in the setup function of packer.nvim is interfering with the "natural loading order" I had in mind when building mkdx.
Hopefully this provides some useful information which you can use to resolve this issue. I'd like to ask you one last time to try out some of the methods I am using in my own configuration. If you still experience issues after that I'll try to produce a working configuration based on your minimal configuration example.
Thank you for the feedback so far and keep me posted 👍
Thanks @SidOfc, if I put the mkdx settings directly in the load_plugins (not in setup) of the minimal config above as you do in your config the error indeed goes away and the completion works as expected. So I started including some more things from my config and finally managed to track down the issue to my setting for :set completeopt=menu,longest. See the following updated minimal example which at least for me reproduces the issue:
vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvim/site]]
local package_root = '/tmp/nvim/site/pack'
local install_path = package_root .. '/packer/start/packer.nvim'
local function load_plugins()
vim.opt.completeopt = {'menu', 'longest'}
require('packer').startup {
{
'wbthomason/packer.nvim',
'SidOfc/mkdx',
},
config = {
package_root = package_root,
compile_path = install_path .. '/plugin/packer_compiled.lua',
display = { non_interactive = true },
},
}
vim.g["mkdx#settings"] = {
highlight = {enable = 1},
enter = {shift = 1},
links = {
fragment = {complete = 1},
external = {enable = 1},
},
toc = {text = 'Table of Contents', update_on_write = 1},
fold = {enable = 1},
}
end
_G.load_config = function()
end
if vim.fn.isdirectory(install_path) == 0 then
print("Installing dependencies.")
vim.fn.system { 'git', 'clone', '--depth=1', 'https://github.com/wbthomason/packer.nvim', install_path }
end
load_plugins()
require('packer').sync()
vim.cmd [[autocmd User PackerComplete ++once echo "Ready!" | lua load_config()]]which I run as nvim -u minimal.lua after first removing ~/.config/nvim/plugin/packer_compiled.lua and ~/.local/share/nvim/site/pack/packer.
Let me know if you can also reproduce it.
Hello again @AckslD, good to hear that configuration is indeed working now. As for the next issue, thank you for supplying the reproduction steps. I did indeed manage to reproduce it.
Offending code in mkdx/markdown.vim.
The above code "appends" values to completeopt as to not overwrite user preferences. So far I've had few issues with it. I'll push a commit with a new g:mkdx#settings.links.fragment.completeopt setting to disable this behaviour as soon as I can 👍
After that we'll just need to check if it works, thank you for all your testing, feedback and patience in the meantime. It is much appreciated!
@AckslD cheers, I try :)
I noticed while playing around that this seems to be caused by the menu,longest configuration. Even when I locally disabled the code which appends to completeopt it still broke. For this reason, I decided to overwrite completeopt in markdown buffers instead of just appending to it.
You should be able to update and verify that the current line is no longer removed now. Despite this patch fixing the issue I do wonder if this is also an acceptable fix for you, or do you really need to configure completeopt to something other than menuone,noinsert in markdown files?
I would really prefer to keep my setting for completeopt since this affects also completion of other things and I like to have the completion insert the match directly if there is only one for example.
I realise now that I misunderstood your earlier comment, I think a better option (although I don't know exactly how to do this) would be to not set completeopt at all but rather try to find out why the current way is incompatible with longest.
I looked into the code a bit and it seems the completefunc is not used in a normal way. Instead of returning the list of matches there is instead a call to Grep here. Why is that? Does Grep manually replace the text instead of passing the results to vim's handling of completion matches? Just trying to understand the behaviour here.
Actually if I just temporarily disable that if statement by applying the following patch it all works again as expected, even with completeopt=menu,longest:
diff --git a/autoload/mkdx.vim b/autoload/mkdx.vim
index 23eb60c..502bd2e 100644
--- a/autoload/mkdx.vim
+++ b/autoload/mkdx.vim
@@ -1246,7 +1246,7 @@ fun! s:util.ContextualComplete()
if (line[start] != '#') | return [start, []] | endif
- if (!s:_testing && s:_can_vimgrep_fmt)
+ if 0
let hashes = {}
let opts = extend(get(s:util.grepopts, s:util.grepcmd, {}), {'pattern': '^(#{1,6} .*|(\-|=)+)$|(name|id)="[^"]+"', 'sync': 1})
let opts['each'] = function(s:util.HeadersAndAnchorsToHashCompletions, [hashes])@AckslD the code you are disabling right now is code which can potentially asynchronously fetch headings (taking a look at the implementation of s:util.Grep will clarify further).
I did this to make sure that the editor does not freeze for too long. Especially in large markdown files walking over each line and performing a regex becomes a bit sluggish.
The file you are testing with is likely not large enough to notice any difference which is why it seems to be just "useless code".
As to why this exactly breaks the menu options I have no clue honestly, but I do prefer keeping this code and then force menuone,insert for markdown files instead of digging deeper myself (currently living a reasonably busy schedule).
Alternatively, you can opt-out of fragment link completion and find some other plugin which does it better. Perhaps nvim-cmp provides some markdown fragment link completion component?
For background context, this is the first Vim plugin I ever developed, it is by no means developed in a pragmatic manner which makes it difficult to comprehend the code years later. It also tries to stay compatible between Vim and Neovim, another thing which further complicates any development of this plugin.
These days I write Lua plugins for Neovim only as that's the version of Vim I use myself and I can't be arsed supporting two "similar" yet "different" APIs just because the two projects have/had some kind of "rivalry".
Finally for some closing notes, I have no intention of porting "all of mkdx" to a Lua Neovim plugin, but I did recently gain some motivation for reviving the good parts of mkdx as a new Lua markdown plugin which would obviously try not to become a "monster" codebase (this is also the first time I'm publicly exposing this idea lol) :)
@SidOfc I completely understand and no problem! :)
I think I now roughly understand the code I miight try to figure out what's actually going on. In the meantime opting out from fragment link completion is indeed an option.
I tried to play around a bit and see what's going on and if I set completeopt=menu,preview and then type # in insert mode it actually crashes nvim 😬
The output says:
-- User defined completion (^U^N^P) -- Searching...munmap_chunk(): invalid pointer
[1] 3184780 IOT instruction (core dumped) nvim README.md
I guess this is some issue in (nightly) nvim. Even if there is something wrong with how completeopt is used it shouldn't crash so I'll probably open an issue in core for this.
Re background. I think that sounds like a nice plan :) I also have some old vimscript plugins and am now only writing plugins in lua which is extremely satisfying. As you say, extracting the core concepts from mkdx might indeed be nice and maybe leave completion etc for nvim-cmp or similar.
Kudos for that mindset @AckslD, I think mkdx.nvim as a Neovim-only Lua plugin will become a reality at some point though no plans yet here :)
One final question remains, what should be done with this issue? Technically mkdx should not remove lines anymore when autocompleting.
Also, I think it's my wonky "programming" from the past which may cause some invalid_pointer error, I'm fairly certain the way I'm doing async stuff in mkdx is just outright garbage and quite unsafe. I definitely stuck with "happy path" programming most of the time back then.
One final question remains, what should be done with this issue? Technically mkdx should not remove lines anymore when autocompleting.
What do you think about having the option to opt-out of the async part for now at least?
On another note, at least for my part, I'd actually prefer to have the option to only set the completefunc and not bind the keys (eg #) in insert mode. This is not actually related to the issue at hand by I prefer to trigger completion manually and don't actually use auto-completion otherwise. I do use cmp but with autocomplete = false :)
@AckslD I've added two new settings (which are currently undocumented) which should give you the control you are looking for. The relevant changes are visible in this commit (I had a reference to this issue included, but it got trimmed somehow).
Setting pumheight to 0 will prevent pumheight from being modified in markdown files. Setting completeopt to 0 has the same effect (e.g. keeps whatever you set).
To ensure some kind of backwards compat, I've set them to the default value they had before they became conditional.
Alternatively, pumheight can be any positive integer (default 15) to determine height for popupmenu and completeopt can be a string value which you would like to use in markdown files (default noinsert,menuone, set 0 to use your own completeopt).
I'm going to write some docs so people know this behavior can be tweaked. Will leave the issue open for you to first confirm whether this fix is good.
Finally, thank you so much for sitting this one through with me so far!
@SidOfc Looks good. Btw, I think the main issue was that complete_add was called on the first call to completefunc which is not the intended way. I opened #160 which fixes this. In #160 I don't have the issue anymore, even using :set completeopt=menu,longest and the minimal example above does not crash nvim anymore.