Skip to content

Conversation

@mvanderkamp
Copy link
Member

As seen in #236, if the runner was already manually closed when we call VimuxCloseRunner we get the following error:

Error detected while processing function VimuxCloseRunner[2]..VimuxTmux[8]..function VimuxCloseRunner[2]..VimuxTmux:
line    8:
E605: Exception not caught: Tmux command failed with message:can't find pane: %19^@
Error detected while processing function VimuxCloseRunner:
line    2:
E171: Missing :endif

This is an annoyance for people who use VimuxCloseOnExit. The error could spread to other commands as well, since the error prevents vimux from unsetting VimuxRunnerIndex, leading other commands to believe the runner is still available.

The error can occur in other contexts too, as seen in #229.

We generally only check if the VimuxRunnerIndex variable is defined before attempting to interact with the runner. This variable is only unset if vimux closes the runner, so any time anything else loses the runner we will get errors.

We have some options:

  1. Always check for the existence of the runner before interacting with it.
  2. Add try-catch blocks around each command to check for these errors.

I think for now I'll just enhance the checks we already have to make sure that the runner actually exists, rather than just checking for the index variable's existence.

@mvanderkamp mvanderkamp requested a review from alerque January 13, 2025 00:41
@mvanderkamp mvanderkamp requested a review from Copilot May 25, 2025 19:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances runner-existence checks and prevents errors when the tmux runner is closed externally by any method other than Vimux.

  • Introduces a parameterless s:hasRunner() function and replaces ad-hoc exists() checks with it.
  • Adds internal helpers s:sendKeys, s:sendText, and a unified s:echoNoRunner() fallback.
  • Updates all runner-related commands (open, close, toggle, inspect, scroll, interrupt, clear) to use the new helpers.
Comments suppressed due to low confidence (4)

plugin/vimux.vim:379

  • [nitpick] The message uses lowercase “vimux” but the command is VimuxOpenRunner. For clarity and consistency with other messages, capitalize “Vimux”.
  echo 'No vimux runner pane/window. Create one with VimuxOpenRunner'

plugin/vimux.vim:84

  • New branches where s:hasRunner() is false now echo an error rather than throwing. Consider adding or updating tests to cover the no-runner fallback paths and s:echoNoRunner() behavior.
  if s:hasRunner()

plugin/vimux.vim:371

  • [nitpick] Each s:sendKeys invocation now runs a list-… check via s:hasRunner, and then spawns a separate tmux command for send-keys. This could introduce noticeable overhead for rapid key sequences. Consider caching g:VimuxRunnerIndex validity or batching multiple sends to reduce repeated tmux calls.
  call VimuxTmux('send-keys -t '.g:VimuxRunnerIndex.' '.a:keys)

plugin/vimux.vim:375

  • [nitpick] Indentation here uses four spaces while other s: functions use two spaces. Aligning on a single indentation style will improve readability and consistency.
    call s:sendKeys(shellescape(substitute(a:text, '\n$', ' ', '')))

@mvanderkamp
Copy link
Member Author

I've given this another pass to make the behaviour consistent across the commands and externally exposed functions.

@alerque Would you prefer to still get a chance to review my PRs?

Tagging a couple other people who might be interested in reviewing this:

@alerque
Copy link
Member

alerque commented Jul 12, 2025

I've given this another pass to make the behaviour consistent across the commands and externally exposed functions.

@alerque Would you prefer to still get a chance to review my PRs?

No, if I'm pretty satisfied it is in good hands even if I don't have attention to give it at any given moment. Monitoring from the sidelines is good enough by me — if I can find the right settings far that.

Edit: Are you sure you can't override the need for a review already? It looks like I already made you an admin on this repo so you should have that option.

@mvanderkamp
Copy link
Member Author

Are you sure you can't override the need for a review already? It looks like I already made you an admin on this repo so you should have that option.

Yeah I can- I figured I should check before I start merging my work in without review though 😁 Thank you!

@mvanderkamp mvanderkamp merged commit f4bf4a8 into preservim:master Jul 13, 2025
4 checks passed
@alerque
Copy link
Member

alerque commented Jul 13, 2025

No worries, use your judgement, that's why you have the buttons already. If you think something is questionable and want another pair of eyes feel free to explicitly push the "request review from" thing and hope it hits my radar. If it doesn't in a reasonable time frame don't sweat it. Pinging users that reported related issues can help too. But for any cases you're confident something is an clear win, has been tested, and/or you have feedback from other users, just merge at will. 🚀

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