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(async): properly trigger callbacks when canceling and fix delays in throttle.sync #1611

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

folke
Copy link
Contributor

@folke folke commented Jun 8, 2023

Fixes #1610 #1608 #1610

@folke folke marked this pull request as draft June 8, 2023 07:42
@folke

This comment was marked as outdated.

@folke

This comment was marked as resolved.

vim.wait(timeout_ or 1000, function()
return not self.running
end)
end, 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the real fix for the delay some users experienced.

Reason is that previsouly the code was not async, so filter would not be running in most cases.
With async, we are hitting the wait in cmp.sync.

Default interval is 200ms for vim.wait which was causing the delays.

Choose a reason for hiding this comment

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

thank you, i will check it now

Choose a reason for hiding this comment

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

@folke delay is greatly reduced, but not completely gone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hrsh7th instead of 10, it could be set to an even lower value, but I personally don't see the delays anymore in the repro.

Copy link

Choose a reason for hiding this comment

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

It's still pretty bad, I tried this:

cmp.visible = cmp.sync(function()
  return cmp.core.view:visible() or vim.fn.pumvisible() == 1
end)

to

cmp.visible = function()
  return cmp.core.view:visible() or vim.fn.pumvisible() == 1
end

and the delay is gone. I have no idea about the architecture of nvim-cmp and why a visibility check would require so much code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgnsk the only repro that was provided to me is here

Those delays are fixed by this PR and multiple people confirmed it.

Are you 100% you're on the latest nvim-cmp version?

If the issue persists, then I really need a repro to check.

Copy link

Choose a reason for hiding this comment

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

I'll look into it more closely in the evening. I'm sure I saw this PR title when I did Lazy sync.

@folke folke changed the title fix(async): properly trigger callbacks when canceling fix(async): properly trigger callbacks when canceling and fix delays in throttle.sync Jun 9, 2023
@folke folke marked this pull request as ready for review June 9, 2023 10:02
@hrsh7th hrsh7th merged commit 6f11816 into hrsh7th:main Jun 9, 2023
@hrsh7th
Copy link
Owner

hrsh7th commented Jun 9, 2023

Thank you so much!

@eeeXun
Copy link

eeeXun commented Jun 9, 2023

I'm still get laggy after this patch.

:version
NVIM v0.9.1
Build type: Release
LuaJIT 2.1.0-beta3

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/share/nvim"

Run :checkhealth for more info
/tmp/mini.lua
vim.cmd([[set runtimepath=$VIMRUNTIME]])
vim.cmd([[set packpath=/tmp/cmp-min/site]])
local package_root = "/tmp/cmp-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
    require("packer").startup({
        {
            "wbthomason/packer.nvim",
            {
                "williamboman/mason.nvim",
                config = function()
                    require("mason").setup({
                        install_root_dir = "/tmp/cmp-min/data",
                    })
                end,
            },
            {
                "WhoIsSethDaniel/mason-tool-installer.nvim",
                config = function()
                    require("mason-tool-installer").setup({
                        ensure_installed = {
                            "typescript-language-server",
                        },
                    })
                end,
            },
            {
                "neovim/nvim-lspconfig",
                config = function()
                    local capabilities = require("cmp_nvim_lsp").default_capabilities()
                    require("lspconfig")["tsserver"].setup({
                        capabilities = capabilities,
                    })
                end,
            },
            {
                "ray-x/lsp_signature.nvim",
                config = function()
                    require("lsp_signature").setup({
                        bind = true,
                        toggle_key = "<C-s>",
                        toggle_key_flip_floatwin_setting = true,
                    })
                end,
            },
            {
                "hrsh7th/nvim-cmp",
                requires = { "hrsh7th/cmp-nvim-lsp" },
                config = function()
                    local cmp = require("cmp")
                    cmp.setup({
                        mapping = {
                            ["<CR>"] = cmp.mapping.confirm({ select = true }),
                        },
                        sources = cmp.config.sources({
                            { name = "nvim_lsp" },
                        }),
                    })
                end,
            },
        },
        config = {
            package_root = package_root,
            compile_path = install_path .. "/plugin/packer_compiled.lua",
            display = { non_interactive = true },
        },
    })
end
if vim.fn.isdirectory(install_path) == 0 then
    vim.fn.system({ "git", "clone", "--depth=1", "https://github.com/wbthomason/packer.nvim", install_path })
end
load_plugins()
require("packer").sync()

Run with nvim -u /tmp/mini.lua. As run it first time, we should quit nvim after plugins are installed. And open it again nvim -u /tmp/mini.lua to let mason-tool-installer.nvim auto install typescript-language-server. Then edit a js file.

2023-06-10_01-21-12.online-video-cutter.com.mp4

As you can see. This doesn't always happen. But quite frequently for me.

BTW. I've checked that after removing lsp_signature.nvim, the input doesn't get stuck.

@mgnsk
Copy link

mgnsk commented Jun 9, 2023

I'm still get laggy after this patch.

@eeeXun See #1613.

Your video is even more clear because it shows which keys were pressed before the lag happened.

@basilgood
Copy link

Same here, it's better but still get laggy: #273

williamboman added a commit to williamboman/nvim-cmp that referenced this pull request Oct 26, 2023
…indow

* upstream/main:
  Expand docs for select_next_item select_prev_item (hrsh7th#1626)
  fix type of the config.autocomplete to accept false (hrsh7th#1624)
  Update ghost_text type to allow true (hrsh7th#1616)
  Format with stylua
  fix: huge single line performance (hrsh7th#1433) (hrsh7th#1615)
  Format with stylua
  restore max_item_count
  fix(async): properly trigger callbacks when canceling and fix delays in `throttle.sync` (hrsh7th#1611)
  Improve types related to mapping definitions (hrsh7th#1604)
  fix: handle godot LSP better (hrsh7th#1592)
  Updating documentation as cmp_git source as been moved to git (hrsh7th#1603)
  fix(entry): fix matches highlight information fixes hrsh7th#1426
  fix(entry): remove offset + 1 for tailwindcss
  Add extmark inline feature detection
  Restore schedule_wrap
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.

6 participants