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: autocenter: avoid use of feedkeys #1632

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

emmanueltouzery
Copy link
Contributor

I'm using the center feature, I was tracking down why I was unexpectedly getting some 'z' somewhere, then I realized that this feedkeys on focus was causing it. Calling through norm should achieve the same outcome but in a more controlled manner.

@emmanueltouzery emmanueltouzery force-pushed the no_feed_keys branch 2 times, most recently from fa6cf7f to 660fdae Compare October 8, 2022 12:56
@emmanueltouzery emmanueltouzery changed the title autocenter: avoid use of feedkeys fix: autocenter: avoid use of feedkeys Oct 8, 2022
@emmanueltouzery
Copy link
Contributor Author

emmanueltouzery commented Oct 8, 2022

To be clear it was a plugin changing window then asking for user input and the feedkeys inserted zz as if I had typed it in the input.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

zz is working on the branch.

I'm not able to reproduce the problem on master; can you please list replication steps. An nvt-min.lua would be great.

@emmanueltouzery
Copy link
Contributor Author

emmanueltouzery commented Oct 9, 2022

ok, here is the min script:

vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvt-min/site]]
local package_root = "/tmp/nvt-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
  require("packer").startup {
    {
      "wbthomason/packer.nvim",
      "nvim-tree/nvim-tree.lua",
      "nvim-tree/nvim-web-devicons",
      -- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
    },
    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
  print "Installing nvim-tree and 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 setup()]]
vim.opt.termguicolors = true
vim.opt.cursorline = true

-- MODIFY NVIM-TREE SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
_G.setup = function()
  require("nvim-tree").setup {
    view = {
      centralize_selection = true,
    }
  }
  vim.cmd("NvimTreeToggle")
end

function _G.repro()
  vim.cmd("wincmd l")
  vim.cmd("NvimTreeFocus")
  vim.fn.input("what say you?")
end

run it with nvim -nu repro.lua
let it launch, set up, then once nvim-tree is opened, run: :lua repro().

Without the fix, at the "What say you?" prompt, zz will be inserted.
With the fix, there'll be no zz.

That's the problem with feedkeys... it just.. feeds keys.. no matter the context, it'll do that. but there's no guarantee where these keys will be sent.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looks good. Reproduced failed and fixed case.

@kylo252 why did you choose feedkeys instead of a regular cmd? Is there an edge case we are not seeing?

Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

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

I don't recall exactly why I used feedkeys() but I know that vim.schedule() can be tricky!

hopefully it's enough to use nvim_buf_call()

@@ -389,8 +389,7 @@ local function setup_autocommands(opts)
pattern = "NvimTree_*",
callback = function()
vim.schedule(function()
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you try this instead

  if opts.view.centralize_selection then
    create_nvim_tree_autocmd("BufEnter", {
      pattern = "NvimTree_*",
      callback = function()
        local bufnr = api.nvim_get_current_buf()
        vim.schedule(function()
          local keys = api.nvim_replace_termcodes("zz", true, false, true)
          api.nvim_buf_call(bufnr, function()
            api.nvim_feedkeys(keys, "n", true)
          end)
        end)
      end,
    })
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but how would that be better than norm! zz? isn't the norm solution strictly simpler?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@emmanueltouzery, what was missing from my implementation is ensuring that feedkeys() only fires in the specified buffer, so nvim_buf_call() should be called regardless.

I wasn't able to reproduce the issue, but using the feedkeys() vs vim.cmd could be debatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i'll try your solution asap, but most likely i won't manage today 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @kylo252

Copy link
Contributor Author

@emmanueltouzery emmanueltouzery Oct 11, 2022

Choose a reason for hiding this comment

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

so I tried @kylo252 's solution and.. it didn't work for me.

this is the test startup file for @kylo252 's solution:

vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvt-min/site]]
local package_root = "/tmp/nvt-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
  require("packer").startup {
    {
      "wbthomason/packer.nvim",
      {"emmanueltouzery/nvim-tree.lua", branch = 'zz_kylo'},
      "nvim-tree/nvim-web-devicons",
      -- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
    },
    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
  print "Installing nvim-tree and 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 setup()]]
vim.opt.termguicolors = true
vim.opt.cursorline = true

-- MODIFY NVIM-TREE SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
_G.setup = function()
  require("nvim-tree").setup {
    view = {
      centralize_selection = true,
    }
  }
  vim.cmd("NvimTreeToggle")
end

function _G.repro()
  vim.cmd("wincmd l")
  vim.cmd("NvimTreeFocus")
  vim.fn.input("what say you?")
end

and this is for my solution:

vim.cmd [[set runtimepath=$VIMRUNTIME]]
vim.cmd [[set packpath=/tmp/nvt-min/site]]
local package_root = "/tmp/nvt-min/site/pack"
local install_path = package_root .. "/packer/start/packer.nvim"
local function load_plugins()
  require("packer").startup {
    {
      "wbthomason/packer.nvim",
      {"emmanueltouzery/nvim-tree.lua", branch = 'no_feed_keys'},
      "nvim-tree/nvim-web-devicons",
      -- ADD PLUGINS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
    },
    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
  print "Installing nvim-tree and 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 setup()]]
vim.opt.termguicolors = true
vim.opt.cursorline = true

-- MODIFY NVIM-TREE SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
_G.setup = function()
  require("nvim-tree").setup {
    view = {
      centralize_selection = true,
    }
  }
  vim.cmd("NvimTreeToggle")
end

function _G.repro()
  vim.cmd("wincmd l")
  vim.cmd("NvimTreeFocus")
  vim.fn.input("what say you?")
end

I'm not sure how to proceed, @kylo252, maybe you can test this and prepare a patch that fixes that issue? Note that this also fails in my real-world scenario, not only in this "artificial" reproduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

again. run the test files with nvim -nu filename.lua.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And again, the instructions to reproduce are:

run it with nvim -nu repro.lua
let it launch, set up, then once nvim-tree is opened, run: :lua repro().

Without the fix, at the "What say you?" prompt, zz will be inserted.
With the fix, there'll be no zz.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I didn't account for the use of input() 😅

we can go with this version then

    callback = function()
      local bufnr = api.nvim_get_current_buf()
      vim.schedule(function()
        api.nvim_buf_call(bufnr, function()
          vim.cmd [[norm! zz]]
        end)
      end)
    end,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that works! force-pushed so that's now what the MR does.

btw it wasn't only a problem with input, in fact my original issue was with the "lightspeed" plugin, which waits for user input, and was "fed" that zz, therefore leading to unpredictable behavior.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Tested OK with repro

@alex-courtis alex-courtis merged commit 187388b into nvim-tree:master Oct 15, 2022
@alex-courtis
Copy link
Member

Thanks @emmanueltouzery @kylo252

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.

None yet

3 participants