Skip to content
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

Add a "exist_in_session" config for #121 #122

Merged
merged 1 commit into from
May 24, 2024
Merged

Add a "exist_in_session" config for #121 #122

merged 1 commit into from
May 24, 2024

Conversation

Parsifa1
Copy link
Contributor

as title

@Shatur
Copy link
Owner

Shatur commented May 24, 2024

What is the difference between autosave_only_in_session?

@Parsifa1
Copy link
Contributor Author

In my opinion, autosave_only_in_session does not seem to work, but I am not willing to delete your original interface. You can modify it at will, or design it yourself :)

@Shatur
Copy link
Owner

Shatur commented May 24, 2024

If it doesn't work, it probably a bug. autosave_only_in_session wasn't submitted by me.

@Parsifa1
Copy link
Contributor Author

If it doesn't work, it probably a bug. autosave_only_in_session wasn't submitted by me.

autosave_only_in_session uses a local variable to control whether the current file is already in session. This design obviously has many problems, but it seems that this is_session variable is used in many places. I can try to reconstruct it, but there is no guarantee.

@Parsifa1
Copy link
Contributor Author

I did a simple refactor, you can see if it can be merged

@Shatur
Copy link
Owner

Shatur commented May 24, 2024

I like it, thanks!

@Shatur Shatur linked an issue May 24, 2024 that may be closed by this pull request
@Shatur Shatur merged commit b552ee8 into Shatur:master May 24, 2024
2 checks passed
@ranebrown
Copy link
Contributor

This change seems to have broken the behavior of autosave_only_in_session. This new check looks if there is a session file that matches the current directory and if so then performs and autosave. Previously you had to manually activate the session before the autosave would kick in which was the desired behavior. Can this be reverted?

@Shatur
Copy link
Owner

Shatur commented Jun 17, 2024

I think this new behavior is more expected... What was your workflow?

ranebrown added a commit to ranebrown/neovim-session-manager that referenced this pull request Jun 17, 2024
@ranebrown
Copy link
Contributor

Create a session in a directory and use it. Close the session and open a different file in the same directory but don't want to overwrite the old session. Close that one file and then be able to restore the old session. The new behavior will autosave a new session with just the one file and lose the previous session that I want to go back to.

I don't want an autosave unless I have activated the session which is the old behavior.

@Shatur
Copy link
Owner

Shatur commented Jun 17, 2024

I would suggest to implement this behavior locally. You can have your custom autosave via autocmd that saves only if a special variable is set.

@ranebrown
Copy link
Contributor

Seems like it would make more sense to restore the old behavior and add a new option like the suggested exist_in_session if something different was needed. Probably can get something working using one of the User autocmds though.

@Shatur
Copy link
Owner

Shatur commented Jun 17, 2024

My concern is having many options with similar names :(
I already having regrets about adding a customization via parameters like this. I should have made them minimalistic and encourage Autocmds. Autocmds are much more powerful then all these autosave_* that we have now.

And I'm sorry it's caused a breaking change for you. I though that it was a bug. Reminds me this 😅 But I personally think that this new behavior is more expected. Although I would remove all these parameters altogether... But users will hate me.

@ranebrown
Copy link
Contributor

Haha, make sense. Thanks.

@svanharmelen
Copy link
Contributor

It causes the same issue for me, but I have quite a valid use case (IMHO of course 😉) which (I hope) is a good reason to revert this change after all...

First some context; I'm using a wide screens with 4 windows next to each other, so being able to resume a session saves me a ton of time searching and opening all the files I was working on.

Now lately I noticed that suddenly one of my session configs was gone and it now tried to open single file in a single window called /private/var/folders/n6/bqfb_xf94tg8pbxyvrj2gkq00000gn/T/kubectl-edit-2644556663.yaml . I hoped it was somehow a one-time (user)error and manually restored and saved my session. Unfortunately since then the same thing keeps happening every now and then 😞

When debugging and reading this issue I now understand what is happening. I open k9s from wherever I am in my terminal to connect to Kubernetes. And every now and then I then open a deployment or secret from within k9s to see or edit the content. k9s in turn opens the content in my configured editor (see where this is going...)

So nvim is started by 'k9s' with a specific one-time temp-file to see/edit some resource in Kubernetes and if that happens to be done from within one of my saved session folders (which is somethings that happens a lot as the related files are in there) it overwrites my session when I quit nvim and return to k9s.

I am pretty sure I can think of more of these kind of uses. I also have a similar workflow when editing a file from Lazygit which then also opens the file in nvim during the Lazyvim session. I didn't test it by I guess this now has the same unwanted and unexpected side-effect.

So can you please reconsider if this indeed the expected behavior, as to me this feels like a very unexpected side effect that I would actually call a bug.

Thanks!

@ranebrown
Copy link
Contributor

If you set autosave_last_session = false, and add this to your config it should give you the old behavior.

local au_group =                                                        
    vim.api.nvim_create_augroup("NvimSessionAuGroup", { clear = true }) 
vim.g.session_active = false                                            
                                                                        
vim.api.nvim_create_autocmd({ "User" }, {                               
    pattern = "SessionLoadPost",                                        
    group = au_group,                                                   
    callback = function()                                               
        vim.g.session_active = true                                     
    end,                                                                
})                                                                      
vim.api.nvim_create_autocmd({ "BufWritePre", "VimLeavePre" }, {         
    group = au_group,                                                   
    callback = function()                                               
        if vim.g.session_active == true then                            
            require("session_manager").save_current_session()           
        end                                                             
    end,                                                                
})                                                                      

@svanharmelen
Copy link
Contributor

@ranebrown thanks for the snippet! At least I now have a way around it, yet I'd still argue the current behavior has a bug that should be fixed. Both k9s and Lazygit are super popular and anyone using them together with this plugin would encounter the same unexpected behavior...

@Shatur
Copy link
Owner

Shatur commented Jun 24, 2024

@svanharmelen hmm... But it saves the session after k9s or Lazygit? It should save only if you already have a session with this name.

@svanharmelen
Copy link
Contributor

Is saves the session using the folder name from which I started one of those tools (which will be the CWD for the tools).

Say I'm working on myapp in path ~/code/applications/myapp. Then in my terminal (using tmux) I will likely be in that folder as well so I have access to (for example) the Makefile and can execute a make command every now and then.

So if from that terminal I now start one of those tools it overwrites the session that I have for that folder every time I do open or edit in file in one of the tools.

@Shatur
Copy link
Owner

Shatur commented Jun 24, 2024

Got it!
I think we can fix this behavior. We should not only check the session file exists, but also check if the application was launched with arguments (i.e. if a session was actually loaded). I think this should solve the problem with such tools.

@svanharmelen
Copy link
Contributor

That would be a great solution indeed!

@Shatur
Copy link
Owner

Shatur commented Jun 24, 2024

Any chance you could open a PR for it?
I currently a little busy with another project. We already have a similar check when we decide if we should autoload session. We just need to add the same check before saving a session if autosave_only_in_session is enabled.

@svanharmelen
Copy link
Contributor

If you can point me to that check I can make a PR for it.

@Shatur
Copy link
Owner

Shatur commented Jun 24, 2024

Sure, it's this line:

if vim.fn.argc() > 0 or vim.g.started_with_stdin then

@svanharmelen
Copy link
Contributor

svanharmelen commented Jun 24, 2024

Hmm... That seems a bit limiting as I tent to use an alias like nvim +'SessionManager load_session' to start nvim which then immediately asks me which session to load (so I can open a session easily when I'm not in the folder and do not want to cd to the path first).

Hoped this was a way to tell if a session was loaded by using something that the plugin sets when it finished loading a session. But I guess that is more or less the in_session var that was there before 😞

Yet I do wonder what @Parsifa1 actually wanted to fix? As I've read (and read again) the info given in #121 but I fail to fully understand if the changes made are indeed needed for his use case.

I'm using a small plugin to call this one, and I'd like to have the option to not add new sessions
except when I call save_current_session on my own initiative, but at the same time not prevent
updates to already saved sessions.

If I read and understand this comment correctly then I don't see the issue with adding something that checks if a session was actually loaded. That will still allow you to only manually create sessions (I only manually create sessions as well) and it also doesn't prevent to save updates to already existing sessions.

The only thing that is needed is that you have loaded a session. But I'm getting the impression @Parsifa1 doesn't want to load the session but does want the (not-loaded) session to be updated automatically. While I appreciate you (@Parsifa1) may feel otherwise, to me that feels weird and unexpected and I wonder if that is indeed the intention of this setting @Shatur?

But besides that, am I correct in my assumptions @Parsifa1? And if so, do you agree that maybe using one of the many ways to load a session make more sense than the current behavior (also given the issues we discussed above)?

@Shatur
Copy link
Owner

Shatur commented Jun 24, 2024

Good points. Now I think that we should just revert the change.

What I disliked about the variable is that you may change your current directory and it will save the session despite you are no longer inside the session. I wondering if we could store the last loaded session name and on autosave check if it matches the current?..

@Parsifa1
Copy link
Contributor Author

@svanharmelen In my opinion autosave_only_in_session should be automatically saved only when you are already in a session that has been save before. Otherwise, when you modify cwd inside nvim, the autosave will still be active. will work to autosave some cwds that you don't need to autosave at all

I'm using it in my config, I used it just like a cli in neovim, which maybe really strange :X

@Parsifa1
Copy link
Contributor Author

But I think we should provide a variety of possible implementations of autosave. In other words, these things should be implemented by users using autocmd and only provide sample examples in the README. After all, everyone may have their own How to use session :)

@Parsifa1
Copy link
Contributor Author

Parsifa1 commented Jun 25, 2024

My use case will be automatically saved when you exit the session you have saved. Because my target is on saving the session of a single folder automatically when you leave this folder or nvim itself.
I have an idea:
You can set up a whitelist that does not automatically save, so that those who need to manually maintain a sisson working directory for persistence can keep it as it is.

@Parsifa1
Copy link
Contributor Author

I think it is reasonable to fall back to, or at least retain, the old behavior. After all, there are many other plugins based on the old behavior such as this
But I still hope to retain at least one behavior that I currently want :D

@Parsifa1
Copy link
Contributor Author

I think it is reasonable to fall back to, or at least retain, the old behavior. After all, there are many other plugins based on the old behavior such as this But I still hope to retain at least one behavior that I currently want :D

It seems that it's still works in other plugins :)

@svanharmelen
Copy link
Contributor

Thanks for your replies @Parsifa1! And reading this:

@svanharmelen In my opinion autosave_only_in_session should be automatically
saved only when you are already in a session that has been save before. Otherwise,
when you modify cwd inside nvim, the autosave will still be active. will work to
autosave some cwds that you don't need to autosave at all

It feels like you and @Shatur mean the same thing here:

What I disliked about the variable is that you may change your current directory and
it will save the session despite you are no longer inside the session. I wondering if 
we could store the last loaded session name and on autosave check if it matches the
current?

I think I can achieve that by restoring the "old" behavior based on the code in this PR work and adding a bit of adjusted logic back from the code that was there before. Looking at your dotfiles I see you are actually loading a session, so checking for a loaded session should not impact your workflow but it should fix other workflows.

Do need to find some time for it as I need to do some actual work as well today 😉 But I will try to create a PR for it so we can all have a look if that might be the correct solution that will fit both use cases.

@Parsifa1
Copy link
Contributor Author

store the last loaded session name and on autosave check if it matches the current

this may be a good idea ,can be called only autosave last loaded session

@svanharmelen
Copy link
Contributor

Yeah I'm thinking about something in that direction. Yet I don't intent to add a new config option but instead "fix" the one we already have and then suggest we never tough it again 😂

And if people do want additional uses, I'm with @Shatur that just keep adding very specific config options isn't a good idea and people should instead create an autocmd for any custom use cases.

@svanharmelen
Copy link
Contributor

@Shatur please checkout (and maybe test) #134 as I believe this will solve the issues discussed here (and fix a small additional bug), including that it will now only auto save things related to the loaded session. So if you leave the directory related to your loaded session, it will not auto save from there as well.

I believe this combines and solves the two issues/goals, so this should be a working solution for @Parsifa1 (and others) as well.

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.

request: Only automatically update and save previously saved sessions
4 participants