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: Make not to skip floatwin if it is focusable #659

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

4513ECHO
Copy link
Contributor

@4513ECHO 4513ECHO commented Aug 26, 2023

Problem

One of the difference between Neovim's floating window and Vim's popup window is focusable.
And since Neovim has global statusline (laststatus=3), we can see statusline of floating window.

But currently lightline skipes floating window and the statusline is rendered as inactive.

Solusion

Make not to skip floatwin if it is focusable.

@4513ECHO
Copy link
Contributor Author

I'm thinking about better checking of floatwin. Please wait to merge...

@4513ECHO
Copy link
Contributor Author

I use relative option of nvim_win_get_config() instead of win_gettype() because if floatwin has 'previewwindow' option detection is not working.

@itchyny
Copy link
Owner

itchyny commented Sep 2, 2023

I think autocmd window is not focusable, so we can omit checking win_gettype().

@itchyny
Copy link
Owner

itchyny commented Sep 2, 2023

Also I'd like you to squash the commits.

@4513ECHO
Copy link
Contributor Author

4513ECHO commented Sep 2, 2023

Ok, then I have omitted to check floatwin and only check focusable.

@itchyny
Copy link
Owner

itchyny commented Sep 2, 2023

Could you use with the patch applied for a few days and if it does not cause any problems, let me merge, thanks.

@4513ECHO
Copy link
Contributor Author

4513ECHO commented Sep 2, 2023

I modified the test code from Neovim to try it out and confirmed that the autocommand window is not focusable.
Also this patch has been working properly for several days.

func Test_autocmd_window()
  %bwipeout!
  edit one.txt
  tabnew two.txt
  vnew three.txt
  tabnew four.txt
  tabprevious
  let g:blist = []
  augroup aucmd_win_test1
    autocmd!
    autocmd BufEnter * call add(g:blist, [expand('<afile>'),
          \ win_gettype(bufwinnr(expand('<afile>'))),
          \ nvim_win_get_config(bufwinid(expand('<afile>'))).focusable])
  augroup END

  doautoall BufEnter
  call assert_equal([
        \ ['one.txt', 'autocmd', v:false],
        \ ['two.txt', '', v:true],
        \ ['four.txt', 'autocmd', v:false],
        \ ['three.txt', '', v:true],
        \ ], g:blist)

  augroup aucmd_win_test1
    autocmd!
  augroup END
  augroup! aucmd_win_test1
  %bwipeout!
endfunc
call Test_autocmd_window()

@4513ECHO
Copy link
Contributor Author

4513ECHO commented Sep 3, 2023

Compare Changes

Because of nvim_open_win() always invokes BufEnter autocmd even not focused (neovim/neovim#15300), I have added check for it.

@itchyny
Copy link
Owner

itchyny commented Sep 3, 2023

That's an issue of NeoVim. I liked the previous patch but now I'm hesitant to merge this PR.

@4513ECHO
Copy link
Contributor Author

4513ECHO commented Sep 3, 2023

Reverted. Following code is a workaround for the bug. Can I add this to the troubleshooting section of the help file?

-- from https://github.com/neovim/neovim/pull/15981
local util = require "vim.lsp.util"
local orig = util.make_floating_popup_options
---@diagnostic disable-next-line: duplicate-set-field
util.make_floating_popup_options = function(width, height, opts)
  local orig_opts = orig(width, height, opts)
  orig_opts.noautocmd = true
  return orig_opts
end

@itchyny
Copy link
Owner

itchyny commented Sep 3, 2023

The code should better be written in NeoVim wiki or somewhere, or help them fix the issues completely, but not in lightline doc. Thanks anyway for the patch.

@itchyny itchyny merged commit f11645c into itchyny:master Sep 3, 2023
12 checks passed
@4513ECHO 4513ECHO deleted the fix/neovim-floatwin branch September 3, 2023 11:38
@4513ECHO
Copy link
Contributor Author

4513ECHO commented Sep 3, 2023

Thank you for reviewing!

@UnkwUsr
Copy link

UnkwUsr commented Nov 9, 2023

Hello @4513ECHO. Looks like your commit collides not only with neovim builtin lsp, but also with https://github.com/lewis6991/gitsigns.nvim plugin, which also have floatwin popups. Yes, gitsigns config have an option to pass noautocmd to his floatwins, but I'm afraid about other plugins that also use floatwins. In result, many users have to investigate whats happening and apply fixes to their own configs for each concrete plugin.

At first, I'm proposing either adding a config option to lightline to disable this new behavior, or revert this commit until we find better way detecting focusable windows without damaging existing users setups.

P.S. to be honest, I'm not seeing any benefits of being able to see statusline for floatwins. They are usually(?) not editable, so not more can be done inside them, and if you want to see basic info like buffer line numbers / cursor position / number of words, you can press g<C-g>. Will be interesting if you share your use case.

@4513ECHO
Copy link
Contributor Author

Hi @UnkwUsr. sorry for the delay in replying to you.
I am using a fuzzy finder called ddu.vim and wanted to display the information from it on the status line on the ddu floating window as well. In the end I made a new plugin, which I hope will help.

The bug in the Neovim itself seems to have already been fixed by neovim/neovim#25078. The workaround I presented is no longer needed, please consider upgrading your version of Neovim.

Translated with DeepL.com (free version)

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