numToStr / Navigator.nvim

:sparkles: Smoothly navigate between neovim and terminal multiplexer(s) :sparkles:
MIT License
391 stars 21 forks source link

feat: add `wezterm` navigation #18

Closed marty-oehme closed 1 year ago

marty-oehme commented 1 year ago

This PR attempts to add wezterm navigation to the plugin in addition to tmux navigation.

It generalizes the general navigation interface to

It uses the added method to automatically set a preferred mux to navigate with if there are multiple running. This behavior can be changed through the setup key mux which defaults to 'auto' but can be set to either 'tmux' or 'wezterm' for now.

The set of included mux navigators and their priority sorting are set at the top of the general navigation interface as the one source of truth.

Navigation under wezterm works well, as does automatic detection of using tmux or wezterm.

There are two remaining issues, however:

I realize this is quite a big PR, we can also attack it bit by bit if it is too large and begin by merging the changes to Navigator interface making it more general and only after adding support for wezterm.

numToStr commented 1 year ago

Thanks for working on this :)

there is no way to infer a window (pane) being maximized in wezterm currently, so the disable_on_zoom configuration option does nothing here for now.

No worries! We can just return true or false depending on what feels right.

detection of a running wezterm instance is being done via extermal pstree command currently, which should work on most Linux installations but may decrease portability and feels a bit clunky still

This, I feel, is a big issue. Because wezterm can also be used on Windows and pstree won't work there. So, we need to come with a solution, maybe with vim.loop, that works well on all the platforms as I don't want to lose windows support.

I realize this is quite a big PR, we can also attack it bit by bit if it is too large and begin by merging the changes to Navigator interface making it more general and only after adding support for wezterm.

I do have some thought regarding a general interface for implementing mux(s) which I still need to do experiment with. I can try to implement that interface here, and let you handle wezterm parts if needed.

marty-oehme commented 1 year ago

We can just return true or false depending on what feels right.

For now we always return false since otherwise the navigation would not work at all when disable_on_zoom option is true.

we need to come with a solution, maybe with vim.loop, that works well on all the platforms as I don't want to lose windows support.

I had not even considered the case of Windows in fact. But I was sleuthing through the wezterm repository and saw that it sets an environment variable for shells running within just like tmux - so the check could be simplified greatly.

I can try to implement that interface here, and let you handle wezterm parts if needed.

Sounds good!

numToStr commented 1 year ago

@marty-oehme I've created a base Vi class and both tmux and wezterm implementation now inherits from it. Now every mux needs to implement :zoomed() and :navigate({dir}) methods and pass that class directly in config.mux (no string value), you can even omit zoomed if you are inheriting from Vi just like WezTerm implementation. I'll update the readme with the instruction later. Meanwhile you can test whether this is working or not for you and let me know your thoughts :)

numToStr commented 1 year ago

I also found something regarding zoom https://wezfurlong.org/wezterm/config/lua/keyassignment/TogglePaneZoomState.html. If there is a way to maximize a pane, so there should be a way to detect zoom state(?)

marty-oehme commented 1 year ago

This is very nice, I especially like the class-based behavior avoiding redundant methods on individual muxes. From what I tested (on linux) it works just as well on wezterm as it did before.

If there is a way to maximize a pane, so there should be a way to detect zoom state(?)

Wezterm can maximize and restore panes just like in tmux, the issue is it can (for now) only be done through the internal lua scripting engine. There is no way to get at it through the cli from outside that I know of. I already thought about a hacky way querying window-size and pane-size but that also does not seem feasible. If I find the time I want to put in a feature request at the repo, since the maintainer seems reasonably open to extending cli capabilities.

pass that class directly in config.mux (no string value)

I really like the idea of unifying the separate option string in the options and the actual implementation, though one thought I had is that this would make it harder for people who wanted to change behavior of vim based on the active mux: previously it was a string comparison, now it would require comparing to the actual class. But in the end that's probably too much bikeshedding and this solution is much cleaner.

The changes look good and make sense to me!

numToStr commented 1 year ago

If I find the time I want to put in a feature request at the repo, since the maintainer seems reasonably open to extending cli capabilities.

This will be great.

I really like the idea of unifying the separate option string in the options and the actual implementation, though one thought I had is that this would make it harder for people who wanted to change behavior of vim based on the active mux: previously it was a string comparison, now it would require comparing to the actual class.

I don't think there is any real class comparison. With auto value it checks the available mux(s) and fallbacks/uses correct mux implementations. If someone wants to override that behavior, they can directly pass that class. Navigator will directly use that class.

require('Navigator').setup({
    mux = require('Navigator.mux.tmux'):new()
})

The real benefit is that it is extensible, user can experiment with different mux(s) like zellij without any hacks or poking into the source code to make this work.


I'll update README with instructions on how to extend the plugin with their own mux. Then I'll merge the changes :)

marty-oehme commented 1 year ago

That is a very nice benefit!

One last thing concerning the README: I have added quite some text for the wezterm side of the setup which may be a little overwhelming in the main readme file. I was wondering if those instructions should be moved into the wiki instead?

Though they would be less connected to the code, right now I feel I am cluttering up your more concise readme so maybe at least the lua code should move behind a foldable or to a different place.

numToStr commented 1 year ago

I was wondering if those instructions should be moved into the wiki instead?

Nice Idea! Yes, definitely we can pull the mux(s) (currently for both lua and wezterm) side of integration in the wiki and just link the wiki back to readme.

numToStr commented 1 year ago

@marty-oehme FYI, I've enabled Wiki in the repo. So now you can go ahead update it :)

numToStr commented 1 year ago

Hey @marty-oehme, I moved all the multiplexer integration code to the wiki. Feel free to check out and update if I miss something.

I am now happy with the state of this PR. If you don't have anything to add then I'd like to merge this :)

numToStr commented 1 year ago

I am going ahead with the merge. Thanks @marty-oehme for working on this :)

marty-oehme commented 1 year ago

Just came back online, thanks @numToStr for doing the hard work of finishing everything!

I took a quick glance at the readme and it looks very nice; I think the integration of individual mux snippets into the wiki makes much more sense. When I get some time over the coming days I'll look into putting in an issue/PR for the wezterm maximized pane cli and perhaps streamlining the wiki snippet on the mux side a little.

Thanks again - and for the wonderful plugin in general of course :-)

numToStr commented 1 year ago

putting in an issue/PR for the wezterm maximized pane cli and perhaps streamlining the wiki snippet on the mux side a little.

That's great! I am also planning to add docs :help navigator-nvim in near future which will include info regarding how to integrate any multiplexer.