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

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

Closed
wants to merge 1 commit into from

Conversation

keks24
Copy link

@keks24 keks24 commented Oct 6, 2022

@alexghergh
Copy link
Owner

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
Copy link
Author

keks24 commented Oct 9, 2022

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.

@keks24 keks24 closed this Oct 9, 2022
@alexghergh
Copy link
Owner

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
Copy link
Author

keks24 commented Oct 11, 2022

Sure thing!

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.

2 participants