romgrk / barbar.nvim

The neovim tabline plugin.
2.26k stars 85 forks source link

[Discussion] Automatic session save with barbar #477

Closed otavioschwanck closed 8 months ago

otavioschwanck commented 1 year ago

Hi, on the last PR's @Iron-E fixed the problem with the session, now we can save and load sessions correctly.
Doing some edge cases i found some issues (i managed to fix) with the session that i like to share here. Using the persistence.nvim.

Disclaimer: This post is only from who use automatic session save / load.

1 - If we open a instance of neovim, open a lot of buffers in it, send it to foreground (Ctrl + z), and open another instance and do another changes. If i close the neovim and the terminal, the persisted session that will be saved is from the foreground one (not the last instance that i was using)

2 - If we force kill vim, we lost all our work.

How i managed to fixed?

First, we don't have to use the pre_save from persistence, it will make the foreground neovim save when we don't want. So my persistence:

First, we need an autocmd to save session and an autocmd to see if vim is exiting

  vim.g.exiting = false

  local autocommands = {
    { { "VimLeavePre" },    { "*" },              function() vim.g.exiting = true end }, -- set to true the vim.g.exiting
    { { "BufReadPost", "BufDelete" },      { "*" },               function(ft) require("scripts.auto-save-session").save_session(ft) end },
  }

  for i = 1, #autocommands, 1 do
    vim.api.nvim_create_autocmd(autocommands[i][1], { pattern = autocommands[i][2], callback = autocommands[i][3] })
  end

With the BufReadPost and BufDelete everytime there is a buffer change we will save teh session, lets take a look on the save_session function:

local M = {}

function M.save_session(ft)
  if vim.g.exiting == false and ft.file and ft.file ~= "" then
    vim.fn.timer_start(100, function()
      vim.api.nvim_exec_autocmds('User', { pattern = 'SessionSavePre' })

      require('persistence').save()
    end)
  end
end

return M

first we don't want to call it when vim is exiting (BufDelete is called when exiting, it would mess our foreground vim instance). Second we don't want to do it for ALL Buffers (like telescope, etc), just for files. if all true, we save the session. Any change that we do will save the session in real time and make the buffers right.

At last, we have to modify the PinBuffer function:

function()
        vim.cmd("BufferPin")
        require("mood-scripts.auto-save-session").save_session({ file = "yeees" })
      end

Instead of just calling the pin, we also want to save the session.

The points of this post:

Iron-E commented 1 year ago
  1. If we open a instance of neovim, open a lot of buffers in it, send it to foreground (Ctrl + z), and open another instance and do another changes. If i close the neovim and the terminal, the persisted session that will be saved is from the foreground one (not the last instance that i was using)

  2. If we force kill vim, we lost all our work.

romgrk might have a different opinion, but I think this is more of an issue with automatic session management than barbar.nvim. Any plugin which relies on a hook for session integration will be vulnerable to this sort of problem, because killing Vim can interrupt the normal shutdown procedure.

Also, there may soon be an official autocmd event to replace User SessionSavePre, in which case even more plugins may start having issues with killing vim + automatic session management.

I can see how if you use backgrounding it may be easy to forget that an instance is still alive. In this case, I might suggest these alternatives instead:

The FocusLost event doesn't currently respond when neovim is about to be backgrounded (which is why I suggest terminal tabs), but if it did that would solve this issue much more easily. I wonder if there has been any talk upstream of adding that feature

otavioschwanck commented 1 year ago
  1. If we open a instance of neovim, open a lot of buffers in it, send it to foreground (Ctrl + z), and open another instance and do another changes. If i close the neovim and the terminal, the persisted session that will be saved is from the foreground one (not the last instance that i was using)
  2. If we force kill vim, we lost all our work.

romgrk might have a different opinion, but I think this is more of an issue with automatic session management than barbar.nvim. Any plugin which uses relies on a hook for session integration will be vulnerable to this sort of problem, because killing Vim can interrupt the normal shutdown procedure.

Also, there may soon be an official autocmd event to replace User SessionSavePre, in which case even more plugins may start having issues with killing vim + automatic session management.

I can see how if you use backgrounding it may be easy to forget that an instance is still alive. In this case, I might suggest these alternatives instead:

  • your terminal emulator's tab support (e.g. kitty, alacritty, wezterm, lxterminal).

    • The FocusLost event + a session manager that lets you manually save (e.g. mini.sessions) will be helpful here.
  • Neovim's tab support (+ :term when necessary).

The FocusLost event doesn't currently respond when neovim is about to be backgrounded (which is why I suggest terminal tabs), but if it did that would solve this issue much more easily. I wonder if there has been any talk upstream of adding that feature

for now i could implement something on barbar to have this feature, like:

integrations = { persistence = true }

That do those fixes i had to make, make sense? If yes, i can start the development of it.

EDIT:

Just to be clear, its not a barbar problem, but we need to call the SessionSavePre to work well.

Iron-E commented 1 year ago

@otavioschwanck I just found the VimSuspend event which runs whenever neovim goes into the background. Does that help simplify your use case? e.g. autocmd VimSuspend * lua require('persistence').save() or something like that


If it were up to me, I'd say we not add workarounds for problems other plugins also face into barbar, but we could certainly document this particular edge case. Partially because of the reasons I said before, and also because integrating all of these checks would have to touch a lot of places and be checked in any future code changes.

If romgrk feels otherwise though, we can take a shot at it.

Edit: I'd be open to adding an api.save_barbar or api.load_barbar command if that would help with the process

otavioschwanck commented 1 year ago

@otavioschwanck I just found the VimSuspend event which runs whenever neovim goes into the background. Does that help simplify your use case? e.g. autocmd VimSuspend * lua require('persistence').save() or something like that

If it were up to me, I'd say we not add workarounds for problems other plugins also face into barbar, but we could certainly document this particular edge case. Partially because of the reasons I said before, and also because integrating all of these checks would have to touch a lot of places and be checked in any future code changes.

If romgrk feels otherwise though, we can take a shot at it.

Edit: I'd be open to adding an api.save_barbar or api.load_barbar command if that would help with the process

@Iron-E take a look:

https://github.com/romgrk/barbar.nvim/pull/478/files

Just a quick draft, with this, the issue is easily fixed

Iron-E commented 1 year ago

The only problem is, this doesn't only affect persistence. We'd also have to hard-code workarounds for all the other automatic session managers as well (and any that come in the future) to fully work around this problem

Did VimSuspend not work?

otavioschwanck commented 1 year ago

The only problem is, this doesn't only affect persistence. We'd also have to hard-code workarounds for all the other automatic session managers as well (and any that come in the future) to fully work around this problem

Did VimSuspend not work?

Let me try

EDIT: @Iron-E

VimSuspend and VimResume works, i could set a variable to when suspend and when resume, and verify if suspended = true, don't save. But looks like we don't have much control anyway (need to manually set for each session manager, etc).

And also, it doesn't fix the force quit propose.

This save_session doesn't cost much, i can't even notice while using.

EDIT 2: We could just get the "save" function for each session manager and do a generic for all, just changing the save function.

otavioschwanck commented 1 year ago

Tried a way with VimResume and VimSuspend, but the VimResume is called at same time at VimSuspend, so i cannot set the variable properly.

Iron-E commented 1 year ago

We could just get the "save" function for each session manager and do a generic for all, just changing the save function

At that point I think it becomes like its own plugin, which manages other session managers and ensures all pre_save hooks (not just ours) are handled properly with suspends/vim kills. That way it doesn't just fix barbar, but all other plugins that use hooks as well.

@romgrk what is your opinion?

Iron-E commented 8 months ago

Closing as #479 was also closed. Feel free to reopen!