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

is_vim does not detect vim running in a subshell #295

Open
jyurek opened this issue May 28, 2021 · 14 comments
Open

is_vim does not detect vim running in a subshell #295

jyurek opened this issue May 28, 2021 · 14 comments

Comments

@jyurek
Copy link

jyurek commented May 28, 2021

I have a project where I am working in Python. I'm using pipenv to manage the environment, which has a feature where if you run pipenv shell it starts a subshell with everything set up right for the environment. This starts a new TTY.

However, that means that ps ... -t "#{pane_tty}" returns the "wrong" process list. It returns

  PID TTY           TIME CMD
92716 ttys001    0:00.85 -bash
96594 ttys001    0:22.72 [/path/to/]Python /usr/local/Cellar/pipenv/2020.11.1

But just ps alone returns (and you can see vim running inside /dev/ttys002)

  PID TTY           TIME CMD
14445 ttys000    0:00.29 -bash
14803 ttys000    0:00.00 /bin/sh /Users/jyurek/bin/tm [my_project]
14804 ttys000    0:00.01 tmux new-session -As [my_project]
92716 ttys001    0:00.85 -bash
96594 ttys001    0:22.72 [/path/to/]Python /usr/local/Cellar/pipenv/2020.11.1
53522 ttys002    0:19.13 vim [source_file].py
96606 ttys002    0:01.01 /bin/bash -i
 8581 ttys003    0:00.33 -bash
61829 ttys003    0:07.61 [/path/to/]Python /usr/local/Cellar/pipenv/2020.11.1
61835 ttys005    0:04.68 /bin/bash -i

The end result is that the is_vim clause in the pane-switching bindings doesn't return true, so it doesn't send the keys to vim, even though it's the program in the foreground.

I'm not really sure how to fix this, since apparently OSX's ps can't show the lineage of processes to know that vim is, in fact, running as an (eventual) sub-process in /dev/ttys001.

@christoomey
Copy link
Owner

Hey John 👋. Well, this is certainly a fun one. I'll be honest that I don't have a great grasp of the ps mechanics at play here (those tweaks were added by a contributor), but I think you might be able to tweak things on your end to get a working solution. The subtlety lies in avoiding false positives so I'm always reticent to change the vim matching logic, but here's two suggestions:

  1. Try out Mislav's original script (I wouldn't be surprised if this didn't work either as it's purposefully a bit less thorough in how it tries to match processes, but could be an option)
  2. Use an alternative matching function. Testing locally against the ps list you provided, something like ps -t '#{pane_tty}' | tail -n +2 | awk '{print $4}' | grep seems to work (although I'm sure there are other edge cases).

Hopefully something in there will help you get back to working.

@jyurek
Copy link
Author

jyurek commented Jun 1, 2021

Hi, Chris! Sadly, no, both of those don't work.

Through a bit more digging, I found ps -o pid= -o ppid= which may lead to some angle, but that's going to take longer than I have at the moment.

@sullivan-sean
Copy link

sullivan-sean commented Aug 10, 2021

Hi, I'm also running into this issue. It seems like '#{pane_tty}' approach alone will not pick up on vim in a pseudoterminal spawned by a process in the original tty (i.e. if I'm using screen, nested tmux, a subshell, or anything that spawns a new pseudoterminal inside of a tmux pane). In my particular case, I'm using https://github.com/withfig/autocomplete which spawns a nested pseudoterminal for each pane in tmux, which unfortunately breaks this extension.

I thought one possible solution would be something like https://stackoverflow.com/a/52544126 which constructs a process tree from a command like what John mentioned above ps -o pid= -o ppid= and then traverses it to find all child processes of a given process.

This approach could be used to get all child processes running in '#{pane_tty}' and check them all for vim.

Wondering if this sounds like a reasonable way to avoid false positives in the full ps list?

@sullivan-sean
Copy link

sullivan-sean commented Sep 17, 2021

Following up on this - a temporary fix for anyone having this issue is to do the following:

  1. Copy the below script into a shell file and make it executable (e.g. touch ~/is_vim.sh && chmod +x ~/is_vim.sh)
#!/usr/bin/env bash
# Usage: is_vim.sh <tty>
#   tty: Specify tty to check for vim process.
tty=$1

# Construct process tree.
children=();
while read -r pid ppid; do
  [[ -n $pid && pid -ne ppid ]] && children[ppid]+=" $pid"
done <<< "$(ps -e -o pid= -o ppid=)"

# Get all descendant pids of processes in $tty with BFS
idx=0
IFS=$'\n' read -r -d '' -a pids < <(ps -o pid= -t $tty && printf '\0')
while (( ${#pids[@]} > idx )); do
  pid=${pids[idx++]}; pids+=( ${children[pid]-} )
done

# Check whether any child pids are vim
ps -o state= -o comm= -p "${pids[@]}" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?)(diff)?$'
exit $?
  1. Replace the is_vim line in this plugin with is_vim="~/is_vim.sh '#{pane_tty}'". If you installed this plugin with tpm you can make this change in your .tmux/plugins folder, and if you installed this plugin by copying lines to your .tmux.conf then you can make this change there

@christoomey I'm happy package this in a PR, but wasn't sure if it was preferable to include the bash script as a separate file or to wrestle with multi-line string escaping inside the tmux file itself.

@sullivan-sean
Copy link

sullivan-sean commented Jan 25, 2022

UPDATE

I found some time to wrestle with tmux's string escaping -- here is a more concise solution that is a drop in for the is_vim variable that robustly accounts for subshells. Rather than making the bash script listed above, you can replace the is_vim function with the following directly:

is_vim="children=(); i=0; pids=( $(ps -o pid= -t '#{pane_tty}') ); \
while read -r c p; do [[ -n c && c -ne p && p -ne 0 ]] && children[p]+=\" $\{c\}\"; done <<< \"$(ps -Ao pid=,ppid=)\"; \
while (( $\{#pids[@]\} > i )); do pid=$\{pids[i++]\}; pids+=( $\{children[pid]-\} ); done; \
ps -o state=,comm= -p \"$\{pids[@]\}\" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?)(diff)?$'"

It does the same thing as the shell script above - a quick explanation line by line:

  1. Initialize a list, pids, of processes running in the current pane_tty
  2. Construct a tree, children, that maps parent to child pids
  3. Loop through descendents of pids running in pane_tty (some of which may be running in descendant ttys)
  4. Apply normal is_vim function across all descendant processes + ttys

For those using this workaround, I'm hoping this approach is more easily adaptable to future updates to the vim-tmux-navigator logic if support for nested ttys is not merged upstream here. For example, if you are encountering the freezing described in #299, this approach can be modified in a similar way as #300 to not use the -t flag of ps in line 1:

is_vim="children=(); i=0; pids=( $(ps -o pid=,tty= | grep -iE '#{s|/dev/||:pane_tty}' | awk '\{print $1\}') ); \
while read -r c p; do [[ -n c && c -ne p && p -ne 0 ]] && children[p]+=\" $\{c\}\"; done <<< \"$(ps -Ao pid=,ppid=)\"; \
while (( $\{#pids[@]\} > i )); do pid=$\{pids[i++]\}; pids+=( $\{children[pid]-\} ); done; \
ps -o state=,comm= -p \"$\{pids[@]\}\" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?)(diff)?$'"

@brendanfalk
Copy link

@christoomey - would you be open to merging this upstream? A lot of our users at Fig would like this to be fixed!

@christoomey
Copy link
Owner

Hi @brendanfalk - apologies for the radio silence here. I've been mulling it over and am hesitant to make a change of this magnitude to the core functionality.

That said, since this change lives in the is_vim check it's much easier for a user to manage themselves (as opposed to adding some configuring deep within the vim plugin side of things). With that in mind, I'd be open to a PR that adds a section to the Troubleshooting section describing the alternative is_vim expression.

@brendanfalk
Copy link

@christoomey thanks for getting back!

Totally understand the concerns about pushing changes that could affect all users. Would you be open to having a chat with me + Sean about the implementation? We've triple checked it and really do think it is just as performant and stable as before, but gives the additional functionality we need to get Fig working.

What happens is people who have vim-tmux-navigator download Fig and it just doesn't work... And most of the time they don't realise it's a conflict between our tools, rather, just assume Fig doesn't work at all. So even if we were in the your troubleshooting documentation, unfortunately I just don't think users would even think to look there.

Open to discussing some other methods like setting an environment variable that makes the change. Or we'd be happy to write a bunch of tests!

@ccope
Copy link

ccope commented Feb 26, 2022

UPDATE

I found some time to wrestle with tmux's string escaping -- here is a more concise solution that is a drop in for the is_vim variable that robustly accounts for subshells. Rather than making the bash script listed above, you can replace the is_vim function with the following directly:

is_vim="children=(); i=0; pids=( $(ps -o pid= -t '#{pane_tty}') ); \
while read -r c p; do [[ -n c && c -ne p && p -ne 0 ]] && children[p]+=\" $\{c\}\"; done <<< \"$(ps -Ao pid=,ppid=)\"; \
while (( $\{#pids[@]\} > i )); do pid=$\{pids[i++]\}; pids+=( $\{children[pid]-\} ); done; \
ps -o state=,comm= -p \"$\{pids[@]\}\" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?)(diff)?$'"

I can't get this to work in tmux 3.2a on Ubuntu 22.04. I'm not sure how to debug it? I think I also ran into an intermittent bug with the old is_vim.sh (I was getting errors from ps about malformed PIDs in the list, and they seemed to have whitespace in them?). For now, I've got a modified version of this inline script working as is_vim.sh:

#!/usr/bin/env bash
# Usage: is_vim.sh <tty>
#   tty: Specify tty to check for vim process.
tty=$1

# Construct process tree.
children=();
pids=( $(ps -o pid= -t $tty) )

while read -r pid ppid
do
  [[ -n pid && pid -ne ppid ]] && children[ppid]+=" $pid"
done <<< "$(ps -Ao pid=,ppid=)"

# Get all descendant pids of processes in $tty with BFS
idx=0
while (( ${#pids[@]} > idx ))
do
  pid=${pids[idx++]}
  pids+=( ${children[pid]-} )
done

# Check whether any child pids are vim
ps -o state=,comm= -p "${pids[@]}" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?)(diff)?$'
exit $?

@brendanator
Copy link

Here's another approach based on #201

  1. Add this to plugin/tmux_navigator.vim
function! s:set_is_vim()
  call s:TmuxCommand("set-option -p @is_vim yes")
endfunction

function! s:unset_is_vim()
  call s:TmuxCommand("set-option -p -u @is_vim")
endfunction

augroup tmux_navigator_is_vim
  au!
  autocmd VimEnter * call s:set_is_vim()
  autocmd VimLeave * call s:unset_is_vim()
  if exists('##VimSuspend')
    autocmd VimSuspend * call s:unset_is_vim()
    autocmd VimResume * call s:set_is_vim()
  endif
augroup END
  1. Add this to tmux.conf
bind -n C-h if-shell -F "#{@is_vim}" "send-keys C-h"  "select-pane -L"
bind -n C-j if-shell -F "#{@is_vim}" "send-keys C-j"  "select-pane -D"
bind -n C-k if-shell -F "#{@is_vim}" "send-keys C-k"  "select-pane -U"
bind -n C-l if-shell -F "#{@is_vim}" "send-keys C-l"  "select-pane -R"
bind -n C-\\ if-shell -F "#{@is_vim}" "send-keys C-\\" "select-pane -l"

The key advantages are:

  • it's portable as it doesn't depend on specific details of bash/ps on a particular system
  • it is really fast as it doesn't spawn a subshell or run any commands

@ccope
Copy link

ccope commented Jul 22, 2022

Here's another approach based on #201

function! s:set_is_vim()
  call s:TmuxCommand("set-option -p @is_vim yes")
endfunction

function! s:unset_is_vim()
  call s:TmuxCommand("set-option -p -u @is_vim")
endfunction

I think this is potentially racy if you move from one tmux split to another and have vim open in both?

@larrybotha
Copy link

larrybotha commented Aug 14, 2022

@brendanator thanks for your approach - works well for me. For the bindings, the C-\ binding needs to be in quotes, otherwise tmux raises a Syntax Error.

For context, <C-\><C-n> is one way to switch from a vim terminal back to vim - for example when using something like vim-floaterm.

Here's a related issue and a proposed solution / workaroud on the tmux repo for the unquoted key binding: tmux/tmux#1827 (comment)

Here are the quoted key bindings I'm using, which are working nicely:

bind -n 'C-h' if-shell -F "#{@is_vim}" "send-keys C-h"  "select-pane -L"
bind -n 'C-j' if-shell -F "#{@is_vim}" "send-keys C-j"  "select-pane -D"
bind -n 'C-k' if-shell -F "#{@is_vim}" "send-keys C-k"  "select-pane -U"
bind -n 'C-l' if-shell -F "#{@is_vim}" "send-keys C-l"  "select-pane -R"
bind -n 'C-\' if-shell -F "#{@is_vim}" "send-keys 'C-\\'" "select-pane -l"

@dagadbm
Copy link

dagadbm commented Jan 3, 2023

If there is someone out there who also needs to have support for fzf the only change you need to do is on the regex. Just add an extra |fzf at the end:

# vim/fzf tmux integration
# https://github.com/christoomey/vim-tmux-navigator/issues/295#issuecomment-1021591011
is_vim_or_fzf="children=(); i=0; pids=( $(ps -o pid= -t '#{pane_tty}') ); \
while read -r c p; do [[ -n c && c -ne p && p -ne 0 ]] && children[p]+=\" $\{c\}\"; done <<< \"$(ps -Ao pid=,ppid=)\"; \
while (( $\{#pids[@]\} > i )); do pid=$\{pids[i++]\}; pids+=( $\{children[pid]-\} ); done; \
ps -o state=,comm= -p \"$\{pids[@]\}\" | grep -iqE '^[^TXZ ]+ +(\\S+\\/)?g?(view|n?vim?x?|fzf)(diff)?$'"

bind -n C-h run "($is_vim_or_fzf && tmux send-keys C-h) || tmux select-pane -L"
bind -n C-j run "($is_vim_or_fzf && tmux send-keys C-j) || tmux select-pane -D"
bind -n C-k run "($is_vim_or_fzf && tmux send-keys C-k) || tmux select-pane -U"
bind -n C-l run "($is_vim_or_fzf && tmux send-keys C-l) || tmux select-pane -R"

conradbeach added a commit to conradbeach/dotfiles that referenced this issue Jan 20, 2023
This reverts commit 7ffe1f2.

I'm removing Fig because it caused issues with vim-tmux-runner [1] [2] [3].
The founders of Fig are trying to work with Chris Toomey to find a
solution though [3], so I could get it working if I really wanted to I bet.

For now, I'm just going to remove Fig; vim and tmux navigation is
more important.

The thing that attracted me to Fig was the command line autocomplete, but
I'm not sure that's worth the cost. Fig does a lot more than I actually
want it to, and it seems to be a fairly complex tool. Dealing with it doesn't
seem worth the value it would create for me. I'll pass.

[1]: christoomey/vim-tmux-navigator#339
[2]: christoomey/vim-tmux-navigator#317
[3]: christoomey/vim-tmux-navigator#295
conradbeach added a commit to conradbeach/dotfiles that referenced this issue Jan 20, 2023
This reverts commit f306816.

I'm removing Fig because it caused issues with vim-tmux-runner [1] [2] [3].
The founders of Fig are trying to work with Chris Toomey to find a
solution though [3], so I could get it working if I really wanted to I bet.

For now, I'm just going to remove Fig; vim and tmux navigation is
more important.

The thing that attracted me to Fig was the command line autocomplete, but
I'm not sure that's worth the cost. Fig does a lot more than I actually
want it to, and it seems to be a fairly complex tool. Dealing with it doesn't
seem worth the value it would create for me. I'll pass.

[1]: christoomey/vim-tmux-navigator#339
[2]: christoomey/vim-tmux-navigator#317
[3]: christoomey/vim-tmux-navigator#295
@JakeTompkins
Copy link

Hey, thanks a bunch for the solution! I just implemented it and it largely fix vim split navigation. The only issue I'm seeing is that if I split nvim horizontally, I can move down a split but not back up.

saifazmi added a commit to saifazmi/dotfiles that referenced this issue Feb 11, 2024
- remove: Fig integration
    - was conflicting with vim-tmux-navigation
    - ref: withfig/fig#460
    - possible solution here:
      christoomey/vim-tmux-navigator#295 (comment)
    - will come back to this later...
- set: a default editor
- add: some aliases
saifazmi added a commit to saifazmi/dotfiles that referenced this issue Feb 11, 2024
- remove: Fig integration
    - was conflicting with vim-tmux-navigation
    - ref: withfig/fig#460
    - possible solution here:
      christoomey/vim-tmux-navigator#295 (comment)
    - will come back to this later...
- set: a default editor
- add: some aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants