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(finder): don't trigger twice w/ inital_mode="insert" #1886

Merged
merged 4 commits into from Apr 27, 2022

Conversation

fdschmidt93
Copy link
Member

@fdschmidt93 fdschmidt93 commented Apr 27, 2022

Closes nvim-telescope/telescope-file-browser.nvim#128

Sorry guys, but initial_mode="insert" wants another rodeo.

It seems to work appropriately with visual and insert mode in my tests, but can you please also take this once for a spin as I'm aware at least some of you have use cases that would stress test this? That would be very much appreciated :)

nvim_input seems to appropriately fit into the event-loop of the finder rather than nvim_feedkeys.

@Conni2461 @kylo252 @clason

E: Summary

  • This fixes that finder loop currently is triggered twice with initial_mode="insert" (relevant for on_complete callbacks)
  • Requires users haven't remapped <ESC> and A to anything materially otherwise what they are doing by default

Copy link
Contributor

@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.

Interesting, I thought it might've been caused by this: d346dc6 (#1600), but that doesn't work either.

this one seems to work tho

diff --git a/lua/telescope/pickers.lua b/lua/telescope/pickers.lua
index fd3558a..80d1af3 100644
--- a/lua/telescope/pickers.lua
+++ b/lua/telescope/pickers.lua
@@ -438,7 +438,10 @@ function Picker:find()
       vim.schedule(function()
         -- startinsert! did not reliable do `A` no idea why, i even looked at the source code
         -- Example: live_grep -> type something -> quit -> Telescope pickers -> resume -> cursor of by one
-        if vim.fn.mode() ~= "i" then
+        local mode = a.nvim_get_mode()
+        if mode == "n" then
+          vim.api.nvim_feedkeys("A", "n", true)
+        elseif mode == "v" then
           vim.api.nvim_feedkeys(vim.api.nvim_replace_termcodes("<ESC>A", true, false, true), "n", true)
         end
       end)

could it be related to using eval inside vim.schedule()? also, is neovim/neovim#16891 a concern?

@fdschmidt93
Copy link
Member Author

fdschmidt93 commented Apr 27, 2022

I'm not entirely sure I understand what problem your patch supposedly fixes induced with this PR? And your patch I'm afraid that runs into

local builtin = require("telescope.builtin")

local index = 0
builtin.find_files({
  on_complete = {
    function()
      -- echoes 2 as nvim_feedkeys triggers finder loop twice
      index = index + 1
      require "telescope.log".warn(index)
    end,
  },
})

because nvim_feedkeys "doesn't fit" (in lack of a better description) into the finder event loop like nvim_input (which seems to be scheduled executed more correctly appropriately).

Visual mode seems to be handled appropriately with this PR in my tests.

also, is neovim/neovim#16891 a concern?

My PR would seem to work just as well with vim.fn.mode in my tests.

E: I guess one downside of nvim_input is that if <ESC> or A were whyever remapped, this wouldn't work appropriately..

@clason
Copy link
Contributor

clason commented Apr 27, 2022

LGTM from a brief test (at least no regressions for symbol insertion)!

@kylo252
Copy link
Contributor

kylo252 commented Apr 27, 2022

My PR would seem to work just as well with vim.fn.mode in my tests.

that's all good then, I was worried we'd run into some weird interaction later, hence why I pointed out another test that I tried, sorry I didn't make it clearer.

because nvim_feedkeys "doesn't fit" (in lack of a better description) into the finder event loop like nvim_input (which seems to be scheduled more correctly).

TIL! thanks for the explanation.

Copy link
Member

@Conni2461 Conni2461 left a comment

Choose a reason for hiding this comment

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

LGTM

@fdschmidt93
Copy link
Member Author

Thanks to you all!

Let's see what other problems this will have 😆

@fdschmidt93 fdschmidt93 merged commit 4449f70 into nvim-telescope:master Apr 27, 2022
@fdschmidt93 fdschmidt93 deleted the fix/finder-trigger branch April 27, 2022 10:33
@ryoppippi
Copy link

Because of this pr, telescope would start up every time in normal mode even though default.initial_mode = "insert" is specified.

@Conni2461
Copy link
Member

insert is default mode so technically you dont need to specify it. I can't reproduce your issue, so i ask you to please file a new bug report. Thanks :)

@ryoppippi
Copy link

ok
and when I rollback the previous commit, it works

@vinzd
Copy link

vinzd commented Apr 27, 2022

Hi there,

Thanks for your hard work on this plugin!

Same issue than ryoppippi here, since this commit telescope starts on normal mode on my side. Additionnally, it breaks my global key mapping and makes nvim a bit crazier each time I open Telescope.

@fdschmidt93
Copy link
Member Author

fdschmidt93 commented Apr 28, 2022

Please open an issue with minimal reproduction steps as opposed to commenting on this PR, which worked as expected prior to merging for four people. That is to say unless you provide sufficient information with an minimal_init.lua, it's very hard to help you I'm afraid.

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.

select_buffer=true with cwd=%:p:h not working
6 participants