Skip to content

[BUG] :SessionSave doesn't actually accept an argument #257

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

Closed
Chloe-H opened this issue Oct 12, 2023 · 17 comments
Closed

[BUG] :SessionSave doesn't actually accept an argument #257

Chloe-H opened this issue Oct 12, 2023 · 17 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Chloe-H
Copy link

Chloe-H commented Oct 12, 2023

Describe the bug

According to the README, an argument can be supplied to :SessionSave to specify a different "directory path" in which to create or save the current session. In reality, it produces one of two errors when an argument is supplied:

  • :SessionSave "~/my/custom/path" (double quotes around the argument) results in Error executing Lua callback: vim/_editor.lua:341: nvim_exec2(): Vim(mksession):E190: Cannot open "<default sessions directory>" for writing (identical to the error described here).
  • :SessionSave '~/my/custom/path' (single quotes around the argument) results in E488: Trailing characters.
  • :SessionSave ~/my/custom/path (no quotes around the argument) also results in E488: Trailing characters.

To Reproduce

  1. Run any of the commands outlined above.
  2. See the error.

Expected behavior

  • :SessionSave ~/custom/directory/ - Save the current session in ~/custom/directory/ with the default session name
  • :SessionSave ~/custom/directory/custom_session_name - Save the current session in ~/custom/directory/ with the name custom_session_name
  • :SessionSave custom_session_name - Save the current session in the default/configured location with the name custom_session_name

Screenshots

The errors:
image

Baseline (please complete the following information):

  • Result of set sessionoptions?: sessionoptions=blank,buffers,curdir,folds,help,tabpages,winsize,terminal
  • OS: I've reproduced the issue exactly as described on Windows 10 Pro and RHEL 7.9
  • URL to your current config (if public) (it's private)
  • Neovim version nvim --version:

On Windows 10 Pro:

$ nvim.exe --version
NVIM v0.9.2
Build type: RelWithDebInfo
LuaJIT 2.1.1694082368
Compilation: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe /MD /Zi /O2 /Ob1  -W3 -wd4311 -wd4146 -DUNIT_TESTING -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_WIN32_WINNT=0x0602 -DMSWIN -DINCLUDE_GENERATED_DECLARATIONS -ID:/a/neovim/neovim/.deps/usr/include/luajit-2.1-ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/build/src/nvim/auto -ID:/a/neovim/neovim/build/include -ID:/a/neovim/neovim/build/cmake.config -ID:/a/neovim/neovim/src -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include

On RHEL 7.9:

NVIM v0.9.1
Build type: Release
LuaJIT 2.0.5

Additional context

I've revisited #51 with lit candles and holy beads in the hopes of getting the functionality I expect. The workaround there is inspiring. I like auto-session a lot, even with this little bug.

@Chloe-H Chloe-H added the bug Something isn't working label Oct 12, 2023
@rmagatti rmagatti added this to the v1.0.0 milestone Nov 21, 2023
@rmagatti
Copy link
Owner

Thanks for the issue submission! I'm adding this to the v2.1.0 milestone, hoping to get a few bug fixes as well as some requested features added in that version. Hoping I can get to it soon!

@Chloe-H
Copy link
Author

Chloe-H commented Nov 24, 2023

Wonderful, I'm looking forward to the update!

@pidgeon777
Copy link

Hello, is there any update about this issue? I'm running:

SessionSave C:\Configurations\Neovim\config\Sessions\TEST.vim

and it returns:

E488: Trailing characters: C:\Configurations\Neovim\config\Sessions\TEST.vim

I'm sure that once it worked, I've been using the above command for a long time, but till last year.

I'm on Windows 11 Pro.

daniprado added a commit to daniprado/auto-session that referenced this issue May 20, 2024
rmagatti added a commit that referenced this issue May 21, 2024
#257 [BUG] :SessionSave doesn't actually accept an argument
@rmagatti
Copy link
Owner

rmagatti commented Jun 6, 2024

Latest referenced merge should fix this.

@rmagatti rmagatti closed this as completed Jun 6, 2024
@Chloe-H
Copy link
Author

Chloe-H commented Jun 6, 2024

Just pulled the latest changes. :SessionSave {path} no longer throws an error, but it doesn't seem to behave as described in the README, either:

:SessionSave ~/my/custom/path " saves or creates a session in the specified directory path.

There doesn't appear to be any completion on the {path} argument, and no matter what I type, it just uses the entire value as the name of the session file instead of parsing out a new directory in which to save the session file.

@rmagatti rmagatti reopened this Jun 7, 2024
@rmagatti
Copy link
Owner

rmagatti commented Jun 7, 2024

@Chloe-H Reopening and will check it out whenever I have some time

@Chloe-H
Copy link
Author

Chloe-H commented Jun 10, 2024

@Chloe-H Reopening and will check it out whenever I have some time

Excellent, I appreciate it!

@pidgeon777
Copy link

Hello, I just wished to ask if there is any update about this.

If I do this on Windows 11:

SessionSave C:\Work\Temp\Test.vim

then, the following file is created:

C:\Users\g.cerqui\AppData\Local\Temp\nvim\sessions\C++-Work-Test.vim.vim

so, the output path is wrong.

@pidgeon777
Copy link

@rmagatti I would also add that this test has been done with the latest commit.

@cameronr
Copy link
Collaborator

cameronr commented Jul 10, 2024

@rmagatti This reproduces for me (where everything passed to SessionSave is used as the session name) but, fwiw, I think that seems like the right behavior. Trying to use the argument to hold both a session name and possible a custom session directory makes things more confusing and introduces extra code complexity, especially since session names are usually paths. For example, how should this be interpreted:

:SessionSave ~/dotfiles/nvim

is ~/dotfiles/nvim the session name or is ~/dotfiles/ a custom session directory where a session called nvim should saved?

Is being able to specify a custom session directory something that's useful for folks? If it's really desired, what about having a second optional argument to SessionSave/Restore that would be the custom session directory to use? That way the code doesn't have to try derive what's intended as it would be clearly specified

@rmagatti
Copy link
Owner

I believe being able to always pass a path to a session file or a dir is desired.
Like the following:

  • :SessionSave ~/.path/to/my-session.vim saves the session with the custom name my-session.vim to the to path.
  • :SessionSave ~/.path/to saves a session with the inferred filename based off of cwd (already what auto-session does) to the to path

We can identify that with the is_directory call.

I think this is how this has worked in the past but changes over the past while likely broke this behaviour.

@pidgeon777
Copy link

I believe being able to always pass a path to a session file or a dir is desired. Like the following:

  • :SessionSave ~/.path/to/my-session.vim saves the session with the custom name my-session.vim to the to path.
  • :SessionSave ~/.path/to saves a session with the inferred filename based off of cwd (already what auto-session does) to the to path

We can identify that with the is_directory call.

I think this is how this has worked in the past but changes over the past while likely broke this behaviour.

Yes, I also think that it worked like that in the past.

Overall, it seems like a good idea to allow users to specify a path to a session file or a directory.

The only thing I would pay a little attention to is the management of Windows paths (unfortunately with other plugins it often happens that the only ones to be managed correctly are Unix-like paths).

Instead, perhaps it could be arranged so that with the commands:

  • :SessionSave my-session.ext : the session file is saved in the directory pointed to by the auto_session_root_dir option with the name my-session.ext.
  • :SessionSave my-session : the session file is saved in the directory pointed to by the auto_session_root_dir option with the name my-session.vim.

These are just ideas, happy to discuss them with you.

@cameronr
Copy link
Collaborator

cameronr commented Jul 11, 2024

To help me understand what i think the best option is, I spent some time digging into how the code works currently (tested on main tonight). Consider this setup:

default session dir: /home/user/sessions/
cwd: /home/user/proj

:SessionSave saves a session file called %home%user%proj.vim to /home/user/sessions/
:SessionRestore finds the session for cwd and reads %home%user%proj.vim from /home/user/sessions/
:AutoSession search (or :Telescope search-lens) will display the session as /home/user/projand let you restore it

:SessionRestore /home/user/proj doesn't actually work currently (fixed in PR #324) because the nargs option is omitted:

vim.api.nvim_create_user_command("SessionRestore", SessionRestore, { bang = true, desc = "Restore Session" })

Fixing that trivial bug, it will then only restore as expected. I'm not sure what the expected behavior would be if the first argument could possibly be a custom session directory.

I would expect SessionSave with no arguments to be equivalent to :SessionSave /home/user/proj. That is currently how it happens to work but it would not work that way if the argument could be treated as a session directory instead. In that case (assuming I'm understanding the proposal correctly), it would save a session for the cwd, which means %home%user%proj.vim, in /home/user/proj. That seems very surprising and confusing. In general, I think a call to SessionSave should be able to be restored by a call to SessionRestore with the same args.

Continuing the testing of how it currently works:

:SessionSave mysession saves a session file called mysession.vim to /home/user/sessions/
:SessionSave mysession.vim saves a session file called mysession.vim.vim to /home/user/sessions/

:SessionRestore mysession Even after the bug fix, that doesn't load the session because it seems to be expecting a full path. Passing in mysession.vim also does not load.
:SessionRestore /home/user/sessions/mysession.vim Will load the session. Same point as above, if :SessionSave mysession created the session, :SessionRestore mysession should restore the session.

:SessionRestoreFromFile mysession I just realized as part of this testing that this doesn't work either because the call to nvim_create_user_command passes just passes in SessionRestore rather than something that would call RestoreSessionFromFile:

vim.api.nvim_create_user_command(
"SessionRestoreFromFile",
SessionRestore,
{ complete = AutoSession.CompleteSessions, bang = true, nargs = "*", desc = "Restore Session from file" }
)

Fixing that simple bug then causes it to load the session correctly (also now in PR #324).

So what to make of all of this? My takeaway is that because session names are usually paths, it's ambiguous to treat the argument to SessionSave/SessionRestore as possibly a custom session directory or a session name. Instead, we should prioritize the use case that is most important and support that well and then make the secondary use case possible.

For me, the more important use case is the ability to save/restore a session consistently, either an automatically created one or an explicitly named one, which means a call to :SessionSave with an argument should be able to be restored by passing that same argument to SessionRestore and those arguments should be consistent with what's displayed in :AutoSession search (or :Telescope search-lens). And because all automatic session names are paths, we can't overload an argument where we take a session name by sometimes trying to interpret it as a custom session directory.

Being able to specify a custom session directory is a secondary use case that should be enabled but not at the cost of disrupting the primary use case of specifying a session name.

With all of that in mind, if it were up to me, I would have SessionSave/SessionRestore always take only an optional session name (cwd is used if no name is provided). That way they're always consistent, a call to SessionSave can be restored with the equivalent call to SessionRestore. To handle a custom session directory, I would have SessionSaveToDir/SessionRestoreFromDir that take the session directory to use and then an optional session name (cwd is used if no name is provided). And SessionRestoreFromFile can be removed as it's not longer necessary. All of that would look like:

:SessionSave saves a session file called %home%user%proj.vim to /home/user/sessions/
:SessionRestore finds the session for cwd and reads %home%user%proj.vim from /home/user/sessions/

:SessionSave /home/user/proj saves a session file called %home%user%proj.vim to /home/user/sessions/
:SessionRestore /home/user/proj restores the session %home%user%proj.vim from /home/user/sessions/

:SessionSave mysession saves a session file called mysession.vim to /home/user/sessions/
:SessionRestore mysession restores the session mysession.vim from /home/user/sessions/

:SessionSaveToDir /other/sessions/ saves a session file called %home%user%proj.vim to /other/sessions/
:SessionRestoreToDir /other/sessions/ finds the session for cwd and reads %home%user%proj.vim from /other/session/

:SessionSaveToDir /other/sessions/ /home/user/proj saves a session file called %home%user%proj.vim to /other/sessions/
:SessionRestoreFromDir /other/sessions/ /home/user/proj restores the session %home%user%proj.vim from /other/session/

:SessionSaveToDir /other/sessions/ mysession saves a session file called mysession.vim to /other/sessions/
:SessionRestoreFromDir /other/sessions/ mysession restores the session mysession.vim from /other/session/

As is probably obvious, the implementation of SessionSave is just a call to SessionSaveToDir with the config session directory and the passed in session name. It'd be the same with SessionRestore/SessionRestoreFromDir.

Lastly, if folks were ok with this proposal, I'm happy to put the PR together. Would be a good chance to leverage the unit tests I've been making. And thanks to everyone that made it this far!

@pidgeon777
Copy link

Thank you so much @cameronr for your detailed analysis.

Let's see what @rmagatti thinks about it.

@rmagatti
Copy link
Owner

I've read your proposal @cameronr. Makes perfect sense to me, much better than what we have currently and what I've proposed 🤝

@cameronr
Copy link
Collaborator

Great, I'll add to my list to work on!

@cameronr
Copy link
Collaborator

cameronr commented Aug 2, 2024

@rmagatti I think this can be closed now

@rmagatti rmagatti closed this as completed Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants