Skip to content

Comments

feat:autosave session and tmp session for switching between sesssion and non-session#7

Merged
jedrzejboczar merged 9 commits intojedrzejboczar:masterfrom
wasden:merge
May 23, 2022
Merged

feat:autosave session and tmp session for switching between sesssion and non-session#7
jedrzejboczar merged 9 commits intojedrzejboczar:masterfrom
wasden:merge

Conversation

@wasden
Copy link
Contributor

@wasden wasden commented May 21, 2022

No description provided.

@wasden wasden force-pushed the merge branch 2 times, most recently from b5a7056 to d1b20e9 Compare May 21, 2022 14:48
@jedrzejboczar
Copy link
Owner

Thanks, at first glance this looks good. I didn't have much time lately, but I'll try to review this PR soon.
Looks like this partially solves #3.

@jedrzejboczar
Copy link
Owner

Thank you for your work, this is really nice!

I tested this and everything seems to work fine. I modified the config options to use a autosave = { ... } table and added some more options to control the behavior. I also documented the new options in doc/possession.txt and in the README.

I pushed these modifications on this branch, I hope you don't mind (you had "allow changes from maintainers" enabled).
Please tell if you have any comments about the changes I did, and if everything looks fine to you, then I'll merge it.

@jedrzejboczar
Copy link
Owner

I just noticed that when using delete_buffers = true I sometimes get errors from nvim-notify. It seems that it also tries to do something with the buffers later (probably using vim.schedule, similar to what you described in the comment about LSP).

I think it should be fine to merge this even with this issue. After all, it is a bug in these plugins to assume that a buffer will still be valid when the callback from vim.schedule runs.

But I was also thinking about a potential solution. One thing that comes to my mind would be to save current buffers list in before_load and add after_load which would iterate over these buffers and delete them if they are hidden. This should be correct if user doesn't have buffers in their sessionopts. But we would probably have to still do that in vim.schedule so I am not sure if it is so good.

@wasden
Copy link
Contributor Author

wasden commented May 22, 2022

Thank you for your work, this is really nice!

I tested this and everything seems to work fine. I modified the config options to use a autosave = { ... } table and added some more options to control the behavior. I also documented the new options in doc/possession.txt and in the README.

I pushed these modifications (改性) on this branch, I hope you don't mind (you had "allow changes from maintainers" enabled). Please tell if you have any comments about the changes I did, and if everything looks fine to you, then I'll merge it.

It looks fine to me.

I just noticed that when using delete_buffers = true I sometimes get errors from nvim-notify. It seems that it also tries to do something with the buffers later (probably using vim.schedule, similar to what you described in the comment about LSP).

I haven't used it, but I will try to install it later.

But I was also thinking about a potential solution. One thing that comes to my mind would be to save current buffers list in before_load and add after_load which would iterate (迭代) over these buffers and delete (删除) them if they are hidden. This should be correct if user doesn't have buffers in their sessionopts. But we would probably have to still do that in vim.schedule so I am not sure if it is so good.

I use bufferline.nvim to manage hidden buffers. Keeping hidden buffers is pretty important to me.

@wasden
Copy link
Contributor Author

wasden commented May 23, 2022

I just noticed that when using delete_buffers = true I sometimes get errors from nvim-notify. It seems that it also tries to do something with the buffers later (probably using vim.schedule, similar to what you described in the comment about LSP).
Try this commit
472dfd5

Currently delete_buffers has some edge cases that may cause some
plugins like nvim-notify, lspconfig, lsp-status to show errors due
to invalid buffer.
@jedrzejboczar
Copy link
Owner

I use bufferline.nvim to manage hidden buffers. Keeping hidden buffers is pretty important to me.

Ok, I see now. This is a workflow that I didn't think about.

I tested your fix, but I still run into an issue with nvim-notify when I load a session, then load another one while notify window is still visible:

Error running notification service: ...art/nvim-notify/lua/notify/service/buffer/highlights.lua:142: Invalid buffer id: 9

For now I removed your nvim-notify fix as I think it should live in a separate plugin, or maybe we can find a way to modify delete_buffers to not require this fix. I think we can work on this later.

I don't want to hold this PR, as your auto-save function is working without problems. In theory delete_buffers is a different functionality than auto-save and could be separated into another PR, but I don't want to make too much problems, so I think it can be merged with this PR. But for now I changed it do be disabled by default, because currently it may cause unexpected problems for users. Anyone who needs it can enable it in their config. In some future PRs we can try to fix all the problems and then change it to be enabled by default.

@jedrzejboczar jedrzejboczar merged commit ff34c6c into jedrzejboczar:master May 23, 2022
@jedrzejboczar
Copy link
Owner

Thanks for your work. I will try to do some more testing of delete_buffers later and maybe find a solution (or create an issue in nvim-notify if that is actually a bug).

@wasden
Copy link
Contributor Author

wasden commented May 23, 2022

@jedrzejboczar Oh, I forgot you enabled other plugins that can delete buffers. Nvim-notify needs to keep a buffer to display the message, my solution is not to delete the buffer. Obviously, this is not a good solution for other plugins.

@jedrzejboczar
Copy link
Owner

@wasden Currently close_windows plugin is used to close floating windows, because when restoring a session, floating windows will mess up window layout. I noticed that neovim/neovim#18635 has been merged to nightly Neovim. This may solve the problem with nvm-notify - we could disable close_window.match.floating and ignore floating windows in delete_buffers. This way floating windows would just stay there when changing sessions. I must test this when I find a moment.

@wasden
Copy link
Contributor Author

wasden commented May 24, 2022

Glad to hear this. And probably delete_hidden_​​​​buffers needs to ignore it too.

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.

2 participants