feat: restore buffers after loading a session

[igorlfs]

Hello,

I'm on a quest to make all of my plugins (vim) "session friendly". For nvim-tree this sounds pretty straightforward, so I went ahead and implemented it myself.

Problem: nvim-tree leaves stale empty buffers after restoring a session

Solution: detect these buffers on SessionLoadPost and reopen them.

[igorlfs]

I went with this location since it was the standard, but it might a good idea have this as an eager autocmd. Otherwise, any means of "lazy loading" may end up calling setup too late (sessions could be loaded relatively early, as in, part of the initilization with nvim -S Session.vim).

[igorlfs]

There's an edge case with this approach: if nvim-tree was the only buf from a tabpage, it will not be restored. I think that's fine, as it's sort of unlikely. Could add a comment as a reference, or otherwise look into it. The vim api is not very tab friendly, but I guess it should be possible to hook an unholy :[N]tabnew...

[alex-courtis]

That's fine. We are very aware of how insufficient the tab api is and how much trouble we've had with it.

Let's stick to the happy path cases to keep it simple and reliable.

Yes please: lots of comments in here.

[igorlfs]

Yes please: lots of comments in here.

Is the current amount enough?

[igorlfs]

Am I missing something or is the CI failure unrelated?

[alex-courtis]

We've had a lot of problems with session integration in the past e.g. #1992, with nodeterministic issues surrounding plogin load and event order etc.

The outcome was that we elected not to support sessions, using a recipe instead.

First question: is an error thrown on startup, or does the session simply fail to correctly load?

Options:

• Publish your solution as a recipe
• Package your solution as an extension plugin

Are the above possible using API?

[alex-courtis]

Am I missing something or is the CI failure unrelated?

Apologies, an upstream Nvim change broke the build, fixed on master #3337

[igorlfs]

We've had a lot of problems with session integration in the past

Thanks, I should have looked this up.

The outcome was that we elected not to support sessions

That's unfortunate to hear. If this decision is final, feel free to close this PR. Else, since the discussion in the other threads is lengthy, could you propose some "up to date" requirements, with, potentially, some compromises? For instance, this comment suggests that folks using session plugins (rmagatti/auto-session in particular) should "be on their own".

The comment also mentions "persisting something about the buffer" (which is also mentioned in this comment). Regarding that, I have an idea which is already employed by some plugins (ref: kulala.nvim, nvim-dap-view): storing the state inside a global variable. Global variables are restored when 'sessionoptions' includes globals (the conditions are slightly stricter and there are other caveats, but I digress).

This approach could be leveraged to restore additional information, besides plainly reopening the buffers. At a most basic level, it could be used to properly restore the cwd. But maybe it could be used to fully restore the tree's state (although I think this would be more complex, since files may have been deleted or whatever).

If we are willing to use features only available on nightly, we can also take advantage of the (recently merged) SessionWritePre, a new event that could also be helpful with the "stateful restore".

A rough outline of what I have in mind for an MVP is:

• Leave "session plugin users on their own"
• When a :mksession is issued, use SessionWritePre to update the global variable that stores nvim-tree's state
• Setup an eager autocmd on SessionLoadPost to restore the state. What I mean by "eager" is that it should be "reachable" from plugin/nvim-tree.lua (potentially "inlined"). Given that session plugins are not interfering with their own SessionLoadPost hooks and that the user isn't using any shenanigans to lazy-load the plugin, this should be much less "race prone".

Of course, that's a really "happy path" plan. If you are uncomfortable with the 'globals' approach, I was thinking about raising a discussion upstream, regarding the "best practices for plugins handling sessions" (aka lua-plugin-sessions). If the other ideas are also "far fetched", I'm open to alternatives. And if we can't settle down on the outline, I'm happy with closing and keeping this as a module on my own config. I could publish it as a recipe as well (current implementation would is easily "copy-pasteable").

Are the above possible using API?

Current implementation is not relying on any internals.

---

First question: is an error thrown on startup, or does the session simply fail to correctly load?

Do you mean if an error is thrown on startup, or something else? Raising an error on VimEnter does not prevent the session from loading. Buffers are restored as usual. SessionLoadPre is also fine. Haven't checked other events (I'm not sure about the event's load order, if you have any specific hints, I can dig into them). I could try a more "destructive" error (e.g., manipulating windows, etc) if needed.

[alex-courtis]

We've had a lot of problems with session integration in the past

Thanks, I should have looked this up.

The outcome was that we elected not to support sessions

That's unfortunate to hear. If this decision is final, feel free to close this PR. Else, since the discussion in the other threads is lengthy, could you propose some "up to date" requirements, with, potentially, some compromises? For instance, this comment suggests that folks using session plugins (rmagatti/auto-session in particular) should "be on their own".

The problems were all around reliability and robustness - many instances of errors thrown on startup. A solution that is very defensive and absolutely does not throw exceptions is acceptable.

The comment also mentions "persisting something about the buffer" (which is also mentioned in this comment). Regarding that, I have an idea which is already employed by some plugins (ref: kulala.nvim, nvim-dap-view): storing the state inside a global variable. Global variables are restored when 'sessionoptions' includes globals (the conditions are slightly stricter and there are other caveats, but I digress).

This approach could be leveraged to restore additional information, besides plainly reopening the buffers. At a most basic level, it could be used to properly restore the cwd. But maybe it could be used to fully restore the tree's state (although I think this would be more complex, since files may have been deleted or whatever).

Setting cwd is quite achievable: nvim_tree.api.tree.open() will set nvim-tree cwd to Nvim pwd. The desired cwd can be explicitly passed via {path}

Full state could be very problematic, due to the great number of configuration options and live state changes (e.g. no-buffer filter) that we have no means of serialising or persisting.

A nvim_tree.api.tree.find_file() might be possible however there may be unforseen problems.

If we are willing to use features only available on nightly, we can also take advantage of the (recently merged) SessionWritePre, a new event that could also be helpful with the "stateful restore".

That changes everything; an official and time predictable mechanism. Yes, new Nvim features can be used via a version check to enable only when available, see Explorer

A rough outline of what I have in mind for an MVP is:

• Leave "session plugin users on their own"
• When a :mksession is issued, use SessionWritePre to update the global variable that stores nvim-tree's state
• Setup an eager autocmd on SessionLoadPost to restore the state. What I mean by "eager" is that it should be "reachable" from plugin/nvim-tree.lua (potentially "inlined"). Given that session plugins are not interfering with their own SessionLoadPost hooks and that the user isn't using any shenanigans to lazy-load the plugin, this should be much less "race prone".

Of course, that's a really "happy path" plan. If you are uncomfortable with the 'globals' approach, I was thinking about raising a discussion upstream, regarding the "best practices for plugins handling sessions" (aka lua-plugin-sessions). If the other ideas are also "far fetched", I'm open to alternatives. And if we can't settle down on the outline, I'm happy with closing and keeping this as a module on my own config. I could publish it as a recipe as well (current implementation would is easily "copy-pasteable").

A predictable SessionWritePre makes a globals approach desirable. The code would be simple on read and write, taking all the guess-work away, thus making it quite robust.

Yes to a global, however please to raise also start a discussion as there may be other unconsidered options or the opportunity for further upstream changes.

Please try this MVP:

• global write
• global read
• open with cwd
  • multiple tabs is up to you at this stage

Are the above possible using API?

Current implementation is not relying on any internals.

First question: is an error thrown on startup, or does the session simply fail to correctly load?

Do you mean if an error is thrown on startup, or something else? Raising an error on VimEnter does not prevent the session from loading. Buffers are restored as usual. SessionLoadPre is also fine. Haven't checked other events (I'm not sure about the event's load order, if you have any specific hints, I can dig into them). I could try a more "destructive" error (e.g., manipulating windows, etc) if needed.

No error right now is good, I was quite concerned. With defensive programming we can keep it that way.

[alex-courtis]

There is an alternative to globals: a json state file.

There is precedent with bookmark persistence: :help nvim_tree.config.bookmarks

[igorlfs]

Latest push now uses the "globals" approach to save and restore the cwd.

start a discussion as there may be other unconsidered options or the opportunity for further upstream changes.

Here it is!

There is an alternative to globals: a json state file.

I can look into that, given the precedent. But we can also wait for some feedback on the upstream discussion, whatever you prefer.

---

CI is failing because the event does exist on stable 🤔
Thoughts on that?

[alex-courtis]

Latest push now uses the "globals" approach to save and restore the cwd.

start a discussion as there may be other unconsidered options or the opportunity for further upstream changes.

Here it is!

There is an alternative to globals: a json state file.

I can look into that, given the precedent. But we can also wait for some feedback on the upstream discussion, whatever you prefer.

That sounds sensible. The global is a good placeholder until we learn more.

We can always refactor / change this at a later date as it is not API.

CI is failing because the event does exist on stable 🤔 Thoughts on that?

That is annoying. It looks like we'll have to use a suppression for the new path instead of the usual old path:

    vim.api.nvim_create_autocmd("SessionWritePre", { ---@diagnostic disable-line: param-type-mismatch

Those suppressions are regularly cleaned up, when we increase the minimum version of Nvim.

[igorlfs]

CI is happy now, should I squash?

[alex-courtis]

Let's run this as an experiment: we can make changes as needed and the user expectations will be correctly set.

• create an experimental config experimental.session_restore_nvim_0.13
• add doc to lua/nvim-tree/_meta/config/experimental.lua
• gate the autocommands behind the experiment flag
• create an issue similar to #2819 to describe the new functionality and give users a place to raise issues and discuss - I'll pin it

[alex-courtis]

That's fine. We are very aware of how insufficient the tab api is and how much trouble we've had with it.

Let's stick to the happy path cases to keep it simple and reliable.

Yes please: lots of comments in here.

[alex-courtis]

CI is happy now, should I squash?

Up to you. The PR will be sqash merged anyway.

[alex-courtis]

neovim/neovim#40336 (comment)

Uppercase globals seem to be a step in the right direction.

Hopefully more guidance follows.

[igorlfs]

Checklist done 🫡 (I can't edit your comment 😭)

Edit: I named the option session_restore_nvim because the doc-gen did not like having to deal with a period inside an option name. Hopefully the name is fine, else a rename is trivial.

[alex-courtis]

Looking good so far...

Apologies for suggesting a . in the name.

I'm not able to achieve session restore for cwd, I think I might be doing something wrong.

Testing with nightly 0.13 3b8c19ea46

VIMRUNTIME=/home/alex/src/neovim/runtime /home/alex/src/neovim/build/bin/nvim ...

Test case:

Added at the end of pre:

print("SessionWritePre set vim.g.NvimTreeState", vim.g.NvimTreeState)

start of post:

print("SessionLoadPost read vim.g.NvimTreeState", vim.g.NvimTreeState)

Set experimental.session_restore_nvim

• :mksession
SessionWritePre set vim.g.NvimTreeState {"cwd":"/home/alex/src/nvim-tree/r/3335"}
• :echo g:NvimTreeState
{"cwd":"/home/alex/src/nvim-tree/r/3335"}
• exit
• start with -S Session.vim

SessionLoadPost read vim.g.NvimTreeState nil
SessionLoadPost path nil
SessionLoadPost read vim.g.NvimTreeState nil
SessionLoadPost path nil

• :echo g:NvimTreeState
E121: Undefined variable: g:NvimTreeState

Multiple SessionLoadPost events on startup is strange - is that expected?

[igorlfs]

I think I might be doing something wrong

I suspect so. sessionoptions must include "globals", which is a non-default flag. Added a note in the documentation.

Multiple SessionLoadPost events on startup is strange - is that expected?

I think that's odd on its own, but unrelated.

[alex-courtis]

I think I might be doing something wrong

I suspect so. sessionoptions must include "globals", which is a non-default flag. Added a note in the documentation.

Got it. After setting it, I am able to restore the session for multiple tabs.

Given the need for that option, and Justin's cautious blessing of using stdpath data, I think it would be best to move to a file.

With a file, it would "just work" out of the box. Following experiment success, this would be a no option / always enabled feature. Nvim-tree state persistence a reasonable expectation for session users and they will be the only ones hitting this codepath.

Multiple SessionLoadPost events on startup is strange - is that expected?

I think that's odd on its own, but unrelated.

SessionLoadPost actions are idempotent, so I reckon it should be OK.

[alex-courtis]

Very close now, this is greatly exceeding my expectations.

Please:

• move to session.lua
• handle json parse errors
• persist in a file, say, nvim-tree-session.json

Remember that this is experimental and not API, hence we can change things like persistence in the future, in a breaking manner.

Move to new module

Apologise, I should have asked for this earlier. This is functionality that is first class and will likely be enhanced in the future. We also want to keep autocmds.lua as minimal as possible.

The callbacks could be something like:

    callback = function()
      require("nvim-tree.session").restore()
    end,

Why require inline inside the callback? Requires at startup time should be minimal, for fastest possible startup.

Invalid json handling

We do need some error handling. Test case:

• create a session and exit
• modify the state in Session.vim to something invalid let NvimTreeState = "{\"cwd\":\"/home/alex/src/nvim-tree/r/3335/foo bar/two three\"} foo bar"
  Error message 4 times:

Error in command line..script /home/alex/src/nvim-tree/r/3335/Session.vim[125]..SessionLoadPost Autocommands for "*":
Lua callback: ...packer/start/nvim-tree.lua.dev/lua/nvim-tree/autocmd.lua:40: Expected the end but found invalid token at character 61
stack traceback:
        [C]: in function 'decode'
        ...packer/start/nvim-tree.lua.dev/lua/nvim-tree/autocmd.lua:40: in function <...packer/start/nvim-tree.lua.dev/lua/nvim-tree/autocmd.lua:32>

Bookmark failures do present any json issues with some context and stop after the first failure.

[NvimTree] Failed to load bookmarks: ...ker/start/nvim-tree.lua.dev/lua/nvim-tree/marks/init.lua:50: Expected comma or array end but found invalid token at character 4

(oversight: bookmark fails should also be printing the path of the bookmark file)

[alex-courtis]

RE nightly check failure, I'll fix that on master. It might be time to remove nightly from all PRs.

Edit: see #3345