joshmedeski / sesh

Smart session manager for the terminal
MIT License
410 stars 16 forks source link

fix: allow config sessions w/ duplicate paths #125

Open kristoferfannar opened 3 weeks ago

kristoferfannar commented 3 weeks ago

If config sessions defined in sesh.toml have a path equal to an existing tmux session, they are currently not added to a sesh List (when calling sesh list.

This PR adds config sessions, which are explicitly defined by the user, no independent of the user's existing tmux sessions.

kristoferfannar commented 3 weeks ago

The plan was to show the test implemented in 09d5913 fail using the workflow before introducing a fix, but as running the workflow requires approval I'll submit my proposed solution below:

in session/list.go:

...
// do not import a slice of currently listed sessions
func listConfigSessions(c *config.Config) (sessions []Session, err error) {
    var configSessions []Session
    // remove the now unused sessionMap
    for _, sessionConfig := range c.SessionConfigs {
        path := dir.AlternatePath(sessionConfig.Path)
        // Do not check whether path is in session map
        if sessionConfig.Name != "" {
            configSessions = append(configSessions, Session{
                Src:  "config",
                Name: sessionConfig.Name,
                Path: path,
            })
        }
    }
    return configSessions, nil
}
...
kristoferfannar commented 3 weeks ago

this fixes #124 (which is mine)

joshmedeski commented 2 weeks ago

I'm in the middle of a complete rewrite (#99).

Do you mind waiting for that issue to be complete? I believe this is already fixed in my new branch.

kristoferfannar commented 2 weeks ago

Sounds good. Just a couple of questions:

When do you plan to release that version? Do you consider this issue I've mentioned as a bug?

Because if this is a bug and the fix is this simple, then I wouldn't mind having this implemented asap so I can start using this awesome tool! I'd be happy to add a commit with the solution here if you'd like

joshmedeski commented 1 week ago

It's not really a bug IMO, just a preference.

I don't have a release window yet, I've had a lot of personal things come up that have gotten in the way of me spending time working on sesh v2.

You are welcome to make a quick PR if you want to change this functionality, I don't mind reviewing it and merging it before v2 ships (maybe in a month?).

kristoferfannar commented 1 week ago

Okay sounds good. I'll have a go!

kristoferfannar commented 1 week ago

My attempt at a (admittedly, rather expansive) solution is ready.

I ended up spending too much time on this considering you're releasing a new version so soon, but I really enjoyed working on this. You have a really cool project on your hands!

With these changes, config sessions which don't have an equivalent active tmux session are always displayed with sesh list, even though a tmux session with the same path already exists.

To check whether a config session has an active tmux session, the Path+Name attributes of a config session are compared with all tmux sessions. This isn't specified anywhere, but I can't see why anyone would want a config session to be separated from an active tmux session if they have the same name and are in the same location (I feel like even just the same name is enough).

This was fun. I'm excited to continue using sesh and to see what v2 holds! Otherwise I hope you can find use in any of the code I committed here.