search

alexghergh / nvim-tmux-navigation

Easy Neovim-Tmux navigation, completely written in Lua
MIT License
273 stars 21 forks source link

optimise `vi` detection logic and use internal macros and commands as much as possible #11

Closed keks24 closed 1 year ago

keks24 commented 1 year ago

See https://github.com/christoomey/vim-tmux-navigator/pull/321.

alexghergh commented 1 year ago

Hey, and thanks for opening a pull request!

Couple of things: 1) Some of the changes are not exactly backwards compatible (for example, #?{version} was introduced in tmux 2.4, some versions of Debian still use Tmux 2.3). 2) I fail to see why you want to duplicate the Vim check command in so many places. I'm not quite sure if that is even 'better' or faster in any way (I notice that Tmux itself is now checking if a pane is Vim or not, instead of spawning a shell process, however this needs to be tested, might not work in all cases, one such case might be over ssh, though, again, it's just pure speculation without trying anything). I do like that we wouldn't have to rely on the external command, but I am not really willing to switch to it unless tested (since the benefits to the user would literally be close to zero). 3) Is there any noticeable performance improvement/any bugs that the PR solves? The PR you mentioned additionally mentioned a 30 second freeze bug. Is this pull request also fixing that? If that's the case, I would kindly ask of you to open a separate pull request just with the fix of the bug, and then we can discuss the changes in this PR further.

Very much appreciated for all the help!

keks24 commented 1 year ago

Thank you for quick reply and taking your time!

To answer your concerns:

  1. I did not know, that it should be backwards compatible this far.
  2. Everything defined in $is_vim is always executed and spawns at least four avoidable subshells, when pressing CTRL+hjkl. Therefore, I optimised it to use only two subshells and a pseudo substring match, using internal macros only. All commands are executed in a /bin/sh subshell, so [[ <some_string> == *<some_substring>* ]], which is Bash only does not work. It is working via ssh, but I did not test any edge cases.
  3. It is indeed saving a 1 to 2 percent of CPU usage and it should fix the freezing issue, since the ps command caused issues.

Anyway, I am going to close this pull-request and let it be for educational purposes.

alexghergh commented 1 year ago

Hey,

Again, thank you for taking the time to write the improvement. I like the idea of having tmux do all the work, however it needs thorough testing. I might return to this in the future at some point, when I get some free time to test.

Have a good one!

keks24 commented 1 year ago

Sure thing!