christoomey / vim-tmux-navigator

Seamless navigation between tmux panes and vim splits
MIT License
5.28k stars 325 forks source link

Optimise `vi` detection logic and use internal macros and commands as much as possible #321

Closed keks24 closed 1 year ago

keks24 commented 1 year ago

Resolves #300 and should also resolve #295.

This pull-request uses:

Also everything is refactored to use internal macros and commands as much as possible to cause less load and invoking less subshells.

I was not sure about the vim-part, but left a comment there with a snippet, which could be used.

I adapted everything by logic and tested only certain parts of the plugin.

-Ramon

1 See CHANGES FROM 2.3 TO 2.4, 20 April 2017 https://github.com/tmux/tmux/blob/master/CHANGES

2 See CHANGES FROM 1.5 TO 1.6, 23 January 2012 https://github.com/tmux/tmux/blob/master/CHANGES

3 See CHANGES FROM 1.1 TO 1.2, 10 March 2010 https://github.com/tmux/tmux/blob/master/CHANGES

4 See CHANGES FROM 2.1 TO 2.2, 10 April 2016 https://github.com/tmux/tmux/blob/master/CHANGES

christoomey commented 1 year ago

Hi @keks24, I certainly appreciate the attempts to clean things up, but unfortunately I believe this would be a regression around the handling of subprocesses (e.g. a git alias / subcommand that spawns vim). The use of ps was introduced a while back to handle the subprocess consideration and I'd not want to break that functionality.

More generally, while I definitely appreciate the attempts to clean things up and make things more efficient, I'm unlikely to merge a sweeping change like this to the core Vim detection logic. My primary goal these days is to keep things stable and a change like this would require a ton of testing (in addition to the more pointed concerns outlined above).

With all that in mind, I'm going to close this now.

keks24 commented 1 year ago

I see!

At least it is documented here and others might still have a use of it. :)

-Keks

christoomey commented 1 year ago

Absolutely! And the nice thing is that this part is easily swappable by folks so they can grab it if they specifically looking for the efficiency gains 🙂