echasnovski / mini.nvim

Library of 40+ independent Lua modules improving overall Neovim (version 0.8 and higher) experience with minimal effort
MIT License
4.85k stars 183 forks source link

(misc): setup_termbg_sync reset can spuriously fail #1111

Closed wroyca closed 3 weeks ago

wroyca commented 1 month ago

Contributing guidelines

Module(s)

misc

Description

This is a tracking issue for https://github.com/echasnovski/mini.nvim/issues/999. Currently, we don’t have a reproducible case for this issue, and it is highly timing-sensitive, so its behavior may vary depending on various factors on user machine.

[!NOTE] From the information we've received thus far, it doesn't seem to affect macOS users.

From time to time, resetting the terminal background might fail:

https://github.com/user-attachments/assets/f87fe333-3422-44a5-8c51-05da3b96116d

I confirmed (in my case*) that I could reproduce with:

Testimony

From @echasnovski:

It still works in all terminal emulators I've tried. The difference is that I have zsh

From @gpanders:

I did ask @gpanders earlier to reproduce with fish. Everything works as expected in Ghostty and iTerm2 terminal emulators (on MacOS, I believe).

From @pkazmier: https://github.com/echasnovski/mini.nvim/issues/1111#issuecomment-2267556583

Workaround?

There are currently two workarounds that I know of which work in my case:

  1. Replacing { "UILeave" } with { "VimLeavePre", "VimSuspend" }

    • This is a solution I've been using personally for months before the introduction of setup_termbg_sync.
  2. Wrapping io.write in a defer_fn

    • Although I haven’t tested this extensively, it appears to work with the following code:
      vim.defer_fn(function() 
      io.write('\027]11;' .. H.termbg_init .. '\007')
      end, 10)

Neovim version

0.10.0

Steps to reproduce

  1. TBD
echasnovski commented 1 month ago

Thanks for the screencast!

Here is what I see from it:

I'll wait a bit for more details, but together with the fact that this seems to also require certain system setup, I don't think this is a severe issue which needs to be accounted for.

  1. Replacing { "UILeave" } with { "VimLeavePre", "VimSuspend" }

This indeed might work, but I am not sure about the implications. Maybe there are cases which 'UILeave' cover but { "VimLeavePre", "VimSuspend" } do not.

2. Wrapping io.write in a defer_fn

This is not a solution becauseit might result in the opposite issue of overriding "sync" step if fg is executed less than 10 milliseconds after <C-z>.

wroyca commented 1 month ago

It requires about 15 extremely quick transitions from background to foreground to reproduce this at least once on your setup.

Note that this is non-deterministic; sometimes I can reproduce it on the first, second, or third try, and sometimes it takes 15 tries or more. In some cases, it takes so many attempts that I am led to believe I cannot reproduce it-as was the case with bash :)

I'll wait a bit for more details

This; Since you made a Reddit post about term sync, I've noticed many people beginning to integrate it (both with and without mini). My hope is to find out whether this issue is isolated or if others are also experiencing the same odd edge case :coffee:

pkazmier commented 1 month ago

I was unable to reproduce on MacOS, WezTerm, with bash and zsh. I even programmed my keyboard with a macro to facilitate testing.

{+KC_Y}{-KC_Y}{+KC_U}{-KC_U}{+KC_ENT}{-KC_ENT}{+KC_LCTL}{+KC_SLSH}{-KC_SLSH}{-KC_LCTL}

In short, f, g, ENTER, C-z, which allowed me to test in very quick succession. For some reason, the macro doesn't work in zsh, but even then I was able to break the macro into two separate steps that still allowed me to test in quick succession without being able to reproduce.

wroyca commented 1 month ago

I was unable to reproduce on MacOS, WezTerm, with bash and zsh. I even programmed my keyboard with a macro to facilitate testing.

{+KC_Y}{-KC_Y}{+KC_U}{-KC_U}{+KC_ENT}{-KC_ENT}{+KC_LCTL}{+KC_SLSH}{-KC_SLSH}{-KC_LCTL}

In short, f, g, ENTER, C-z, which allowed me to test in very quick succession. For some reason, the macro doesn't work in zsh, but even then I was able to break the macro into two separate steps that still allowed me to test in quick succession without being able to reproduce.

Interesting, so it just seems to work no matter what on macOS. Thank you for your time, truly appreciate it! 🥳

pkazmier commented 1 month ago

I also tried testing without any macros in both bash/zsh, with no delays, and with 200ms delays. Couldn't reproduce once in any scenario I tried.

wroyca commented 1 month ago

Some observations: I can't seem to reproduce with mini.deps, but I can with lazy.nvim. I can reproduce with lazy = false, but not with event = "VimEnter".

This seems to make sense as having any kind of delay "fix" the issue

echasnovski commented 1 month ago

Some observations: I can't seem to reproduce with mini.deps, but I can with lazy.nvim.

Well, you have your solution then :)

wroyca commented 1 month ago

Some observations: I can't seem to reproduce with mini.deps, but I can with lazy.nvim.

Well, you have your solution then :)

;)

In any case, closing this as unrelated to mini.nvim as a whole. This should probably be addressed either upstream (neovim) or elsewhere, but since we don't have any clear direction other than some volatile observations, it’s best to let it sit for a while

pkazmier commented 1 month ago

Indeed, I, too, use mini.deps. Time to join the other team 😄

wroyca commented 1 month ago

@echasnovski I may have managed to come up with a reproducer actually:

https://github.com/user-attachments/assets/a08664ef-3655-4ccc-90cb-01e3ca7f8dba

In single empty init.lua:

local setup_termbg_sync = function()
  local sync = function()
    local hl = vim.api.nvim_get_hl(0, { name = "Normal" })
    if hl and hl.bg then io.write(string.format("\027]11;#%06x\007", hl.bg)) end
  end
  local reset = function()
    -- Delay a little, notice how neovim end up detaching before sending the
    -- reset.
    --
    -- Increase the delay if you can't reproduce with 1 (it's racy)
    vim.defer_fn(function() io.write "\027]111\007" end, 1)
  end
  vim.api.nvim_create_autocmd({ "UIEnter", "ColorScheme" }, { callback = sync })
  vim.api.nvim_create_autocmd({ "UILeave" }, { callback = reset })
end

setup_termbg_sync()
echasnovski commented 1 month ago

With :quit this is totally expected because Neovim is closed before the function inside vim.defer_fn has a chance to execute.

And I don't any reproduction steps involving manual delay can represent reality because it intentionally simulates different order of "UILeave" and "UIEnter" relative to each other.

echasnovski commented 3 weeks ago

Sooo... I noticed several times in the past days that background was not changed to the original one after fully quitting Neovim. Went down a rabbit hole to try and find the minimal reproducible example, and it turned to essentially be just a single "UILeave" autocommand followed by a :q<CR> after some delay (exact value doesn't appear to matter).

Using a pair of { "VimLeavePre", "VimSuspend" } does indeed fix this issue... But if done in the original setup_termbg_sync(), it introduces the same "not original background" after <C-z> (as in this issue) which did not really need to be done fast (not as in this issue). This is confusing because it seems to have solved the issue, not introduce it :confused:

For possible future use, here is a 'init.lua' for reproducing (with some thoughts during investigation):

Repro 'init.lua' ```lua -- A reference 'init.lua' to reproduce how `MiniMisc.setup_termbg_sync()` -- can not revert back to original terminal background after exiting Neovim. -- And it is a fairly problematic side effect because it needs terminal -- emulator restart as a fix (or executing `echo -e "\033]111\a"`). -- -- Steps: -- 1. Put this file in '~/.config/nvim_uileave/init.lua'. -- 2. Execute `NVIM_APPNAME=nvim_uileave nvim`. -- 3. Wait until it autocloses. -- 4. If after it closes the background is #102030 (and not what it used to be -- earlier), then it reproduced the issue. Otherwise repeat steps 2-4. -- -- I usually reproduce after about 5-10 tries on average. However, it can be -- anywhere from 1 to at least 40. Couldn't find an obvious core factor -- contributing to this. It doesn't look like high CPU or RAM usage, number of -- opened terminal or Neovim instances. -- -- It was reproduced in several terminal emulators (order is from least to most -- tries usually needed to reproduce): Ghostty, WezTerm, Kitty, Alacritty. -- All with zsh on Linux. -- BIG NOTE: replacing `{ "UILeave" }` with `{ "VimLeavePre", "VimSuspend" }` -- resulted into no single reproduction in any of terminal emulators for at -- least 50 tries (each). So it *is* probably some delay issue before closing -- happens. Maybe because "UILeave" event is executed too late (i.e. after -- `VimLeave`) and `io.write` can occasionally not have effect. -- -- **HOWEVER**, when `{ "VimLeavePre", "VimSuspend" }` is used in -- `setup_termbg_sync()` directly, there are instances of occasional not -- restoring original background color after `` (not necessarily quickly -- after `fg`, as in #1111). vim.api.nvim_create_autocmd({ "UILeave" }, { callback = function() -- Can be reproduced with the OSC 11 approach of `setup_termbg_sync()`, -- both with `"UILeave"` plus exiting and `{ "VimLeavePre", "VimSuspend" }` -- plus suspending. OSC 111 approach just has less lines. io.write('\027]111\007') -- Using this approach to register event seems to make everything work -- -- vim.fn.writefile({ "" }, vim.fn.fnamemodify(os.tmpname(), ":t")) end, }) io.write('\027]11;#102030\007') vim.defer_fn( function() vim.api.nvim_input(':q') -- Using this approach to close Neovim seems to make everything work -- vim.cmd('quit') end, -- Actual delay doesn't seem to affect the result, but bigger values (more -- than 1000) tend to need more tries to reproduce. 500 ) ```

My current understanding boils down to these statements:

At this point I think I am at the stage of (severely) diminishing returns with this investigation. Having background not restored once in a while should not be a huge issue:


I'll probably do another short revisit tomorrow and probably replace 'UIEnter' with { 'VimEnter', 'VimResume' } and 'UILeave' with { 'VimLeavePre', 'VimSuspend' }. Basically, this:

diff --git a/lua/mini/misc.lua b/lua/mini/misc.lua
index 6008e3b..c3ebc39 100644
--- a/lua/mini/misc.lua
+++ b/lua/mini/misc.lua
@@ -313,12 +313,12 @@ MiniMisc.setup_termbg_sync = function()
       if normal.background == nil then return end
       io.write(string.format('\027]11;#%06x\007', normal.background))
     end
-    vim.api.nvim_create_autocmd({ 'UIEnter', 'ColorScheme' }, { group = augroup, callback = sync })
+    vim.api.nvim_create_autocmd({ 'VimEnter', 'VimResume', 'ColorScheme' }, { group = augroup, callback = sync })

     -- Set up reset to the color returned from the very first call
     H.termbg_init = H.termbg_init or bg_init
     local reset = function() io.write('\027]11;' .. H.termbg_init .. '\007') end
-    vim.api.nvim_create_autocmd({ 'UILeave' }, { group = augroup, callback = reset })
+    vim.api.nvim_create_autocmd({ 'VimLeavePre', 'VimSuspend' }, { group = augroup, callback = reset })

     -- Sync immediately
     sync()

@wroyca, will this indeed fix this issue of "fast restore-suspend"?

wroyca commented 3 weeks ago

Sooo... I noticed several times in the past days that background was not changed to the original one after fully quitting Neovim. Went down a rabbit hole to try and find the minimal reproducible example, and it turned to essentially be just a single "UILeave" autocommand followed by a :q<CR> after some delay (exact value doesn't appear to matter).

Using a pair of { "VimLeavePre", "VimSuspend" } does indeed fix this issue... But if done in the original setup_termbg_sync(), it introduces the same "not original background" after <C-z> (as in this issue) which did not really need to be done fast (not as in this issue). This is confusing because it seems to have solved the issue, not introduce it 😕

For possible future use, here is a 'init.lua' for reproducing (with some thoughts during investigation): Repro 'init.lua'

-- A reference 'init.lua' to reproduce how `MiniMisc.setup_termbg_sync()`
-- can not revert back to original terminal background after exiting Neovim.
-- And it is a fairly problematic side effect because it needs terminal
-- emulator restart as a fix (or executing `echo -e "\033]111\a"`).
--
-- Steps:
-- 1. Put this file in '~/.config/nvim_uileave/init.lua'.
-- 2. Execute `NVIM_APPNAME=nvim_uileave nvim`.
-- 3. Wait until it autocloses.
-- 4. If after it closes the background is #102030 (and not what it used to be
--    earlier), then it reproduced the issue. Otherwise repeat steps 2-4.
--
-- I usually reproduce after about 5-10 tries on average. However, it can be
-- anywhere from 1 to at least 40. Couldn't find an obvious core factor
-- contributing to this. It doesn't look like high CPU or RAM usage, number of
-- opened terminal or Neovim instances.
--
-- It was reproduced in several terminal emulators (order is from least to most
-- tries usually needed to reproduce): Ghostty, WezTerm, Kitty, Alacritty.
-- All with zsh on Linux.

-- BIG NOTE: replacing `{ "UILeave" }` with `{ "VimLeavePre", "VimSuspend" }`
-- resulted into no single reproduction in any of terminal emulators for at
-- least 50 tries (each). So it *is* probably some delay issue before closing
-- happens. Maybe because "UILeave" event is executed too late (i.e. after
-- `VimLeave`) and `io.write` can occasionally not have effect.
--
-- **HOWEVER**, when `{ "VimLeavePre", "VimSuspend" }` is used in
-- `setup_termbg_sync()` directly, there are instances of occasional not
-- restoring original background color after `<C-z>` (not necessarily quickly
-- after `fg`, as in #1111).
vim.api.nvim_create_autocmd({ "UILeave" }, {
  callback = function()
    -- Can be reproduced with the OSC 11 approach of `setup_termbg_sync()`,
    -- both with `"UILeave"` plus exiting and `{ "VimLeavePre", "VimSuspend" }`
    -- plus suspending. OSC 111 approach just has less lines.
    io.write('\027]111\007')
    -- Using this approach to register event seems to make everything work
    -- -- vim.fn.writefile({ "" }, vim.fn.fnamemodify(os.tmpname(), ":t"))
  end,
})

io.write('\027]11;#102030\007')

vim.defer_fn(
  function()
    vim.api.nvim_input(':q<CR>')
    -- Using this approach to close Neovim seems to make everything work
    -- vim.cmd('quit')
  end,
  -- Actual delay doesn't seem to affect the result, but bigger values (more
  -- than 1000) tend to need more tries to reproduce.
  500
)

My current understanding boils down to these statements:

* It indeed appears to be that `io.write(...)` needs some (small) amount of time to take effect. Adding `io.flush()` did not show any difference.

* It appears to be that `UILeave` event is triggered too late during closing Neovim (after `VimLeave`), hence there is sometimes not enough time for `io.write()` to take effect. Using `VimLeavePre` (together with necessary `VimSuspend`) seems to fix the issue, as it is triggered earlier during Neovim exit.

* The issue with not restoring background with `{ 'VimLeavePre', 'VimSuspend' }` after suspending seems to also be caused by "not enough time" after "VimSuspend" is triggered. I thought that maybe an issue was similar to what is described [here](https://github.com/neovim/neovim/issues/22041) (i.e. `VimResume` and `UIEnter` are triggered immediately after `VimSuspend`), but it doesn't look like it (after reproducing "suspend issue" and going back with `fg`, the total number of `VimSuspend` and `VimResume` is the same as if no reproduction was done).

At this point I think I am at the stage of (severely) diminishing returns with this investigation. Having background not restored once in a while should not be a huge issue:

* It can be reverted with another `fg` - `<C-z>`.

* I tend to "promote" the usage of built-in terminal and with it there is no such issue.

I'll probably do another short revisit tomorrow and probably replace 'UIEnter' with { 'VimEnter', 'VimResume' } and 'UILeave' with { 'VimLeavePre', 'VimSuspend' }. Basically, this:

diff --git a/lua/mini/misc.lua b/lua/mini/misc.lua
index 6008e3b..c3ebc39 100644
--- a/lua/mini/misc.lua
+++ b/lua/mini/misc.lua
@@ -313,12 +313,12 @@ MiniMisc.setup_termbg_sync = function()
       if normal.background == nil then return end
       io.write(string.format('\027]11;#%06x\007', normal.background))
     end
-    vim.api.nvim_create_autocmd({ 'UIEnter', 'ColorScheme' }, { group = augroup, callback = sync })
+    vim.api.nvim_create_autocmd({ 'VimEnter', 'VimResume', 'ColorScheme' }, { group = augroup, callback = sync })

     -- Set up reset to the color returned from the very first call
     H.termbg_init = H.termbg_init or bg_init
     local reset = function() io.write('\027]11;' .. H.termbg_init .. '\007') end
-    vim.api.nvim_create_autocmd({ 'UILeave' }, { group = augroup, callback = reset })
+    vim.api.nvim_create_autocmd({ 'VimLeavePre', 'VimSuspend' }, { group = augroup, callback = reset })

     -- Sync immediately
     sync()

@wroyca, will this indeed fix this issue of "fast restore-suspend"?

Yes, this seems to fix it thus far (though it may just be luck)

wroyca commented 3 weeks ago

Adding to this:

At this point I think I am at the stage of (severely) diminishing returns with this investigation. Having background not restored once in a while should not be a huge issue:

I consider built-in terminal unsuitable for any serious work, it's fine from time to time but anything more than that and it fall apart.

OSC 111

I consider terminals that do not support this properly as an upstream bug that should be reported to the respective terminal developers.

should not be a huge issue:

Being pedantic in integrating neovim well with the system for work, I consider this a huge issue :)

-- BIG NOTE: replacing { "UILeave" } with { "VimLeavePre", "VimSuspend" } -- resulted into no single reproduction in any of terminal emulators for at -- least 50 tries (each). So it is probably some delay issue before closing -- happens. Maybe because "UILeave" event is executed too late (i.e. after -- VimLeave) and io.write can occasionally not have effect.

This perhaps should be reported upstream, doesn't seems normal to me :thinking:

echasnovski commented 3 weeks ago

I consider built-in terminal unsuitable for any serious work, it's fine from time to time but anything more than that and it fall apart.

What do you consider a "serious work" that is not suitable for the built-in terminal?

OSC 111

I consider terminals that do not support this properly as an upstream bug that should be reported to the respective terminal developers.

My main use case is st. Default build doesn't seem to support OSC 111 and there doesn't seem to be any dedicated patches for it, so at least it can happen in the wild. Reporting this to st developers will probably lead to nowhere while I don't want requiring people to write/install special patches.

wroyca commented 3 weeks ago

What do you consider a "serious work" that is not suitable for the built-in terminal?

Heavy usage of any tools or toolchain that are TUI/CLI heavy, really. It's problematic, in particular with the current reflow state upstream

wroyca commented 3 weeks ago

Yes, this seems to fix it thus far (though it may just be luck)

Ok used this for a few hours now and can indeed confirm that it fixed it in my case :)

echasnovski commented 3 weeks ago

Yes, this seems to fix it thus far (though it may just be luck)

Ok used this for a few hours now and can indeed confirm that it fixed it in my case :)

That's both good (it fixes something) and confusing (it introduces very similar issue for me) news. I'll just focus on the first interpretation :)

echasnovski commented 3 weeks ago

Latest main now uses different events, similar to patch in this comment, only without VimEnter (figured it should not be needed as synchronization is done immediately).

Let's all hope that this issue won't again result in countless hours of trying to reproduce and debug it.