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

fix(tmux): Add completion for alias functions #12278

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

detroyejr
Copy link
Contributor

@detroyejr detroyejr commented Mar 11, 2024

fix(tmux): Add completion for alias functions

Fixes #12282

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

Other comments:

I barely understand the ZSH completion system, but this seems to get the job done. ta<tab> and tmux attach -t<tab> seem to provide identical results. Any feedback is welcome.

@ohmyzsh ohmyzsh bot added the Area: plugin Issue or PR related to a plugin label Mar 11, 2024
@patrick96
Copy link

For what it's worth, if you do things like ta 0 -f, tab completion doesn't work for that flag. But it's still better than the current state, where completion isn't working at all

@detroyejr
Copy link
Contributor Author

Ah, yes I see what you mean. Alternatively, the ZSH user repository provides a lot of good functionality for completion. I see other plugins have borrowed from them before, so maybe we can just import the relevant parts rather than re-inventing the wheel. I'll see if that's doable.

@detroyejr
Copy link
Contributor Author

@patrick96 thanks for the feedback. I've addressed your comment and now the following should happen:

ta<tab> without arguments completes on session names as before.
ta <session name><tab> will now start completing any of the additional arguments that you can pass to tmux attach.

tmux kill-session takes different arguments so I've added those as well, but it should act like the above.

@detroyejr detroyejr force-pushed the master branch 2 times, most recently from 9a70905 to d38d7e2 Compare March 12, 2024 15:59
@detroyejr
Copy link
Contributor Author

Cleaned this up a bit so that completion for ta <session> -<tab> looks the same as tmux attach -t <session> -<tab>. Same goes for tkss.

Barring any further feedback/cleanup, I think this good to go.

@matthew-nm
Copy link

Is my comment here valid?: #12282 (comment)

Would changing the approach to setting an alias (as in the linked comment) vs defining a function be better? I don't know anything about this codebase but a quick test on my end seems to show that doing so would avoid having to reimplement any autocompletion functionality now or on future changes.

@detroyejr
Copy link
Contributor Author

Does the snippet you wrote provide more than the alias alias ta="tmux attach -t"? I think the only thing it might be missing is being able to run ta without any arguments. That was the main reason that prompted me to open a PR in the first place.

If the maintainers want to support better completion over my workflow preference to attach to the last session withta, that's fine by me. It makes a lot of sense to say this will just no longer work if it makes maintaining this too complex.

@matthew-nm
Copy link

Yes, the snippet replaces the current '_build_tmux_alias' function and can provide the same 4 aliases (ta, tad, tkss, ts) and ta, tad, and ts seem to work without a specified session as well as with. But I don't know if ta is necessarily attaching to the last session or what it's doing exactly (seems kinda random to me).

I don't follow what use case the 'if' statement is designed for so feel free to add the functionality you're looking for and I can always create my own aliases if I feel the need.

I appreciate you figuring out a way to add the autocomplete back in to the new methodology.

@detroyejr
Copy link
Contributor Author

For some reason, I still get the error I had previously. Just to be sure, does your tmux -V return 3.4?

Otherwise, I'm testing as follows:

# Reset
unfunction ta

function _build_tmux_alias {
  eval "if [[ -z \$1 ]] || [[ \${1:0:1} == '-' ]]; then
    alias \$1=\"tmux $2\"
  else
    alias \$1=\"tmux $2 $3\"
  fi"
}

_build_tmux_alias "ta" "attach" "-t"

Everything else works, but I still get command attach-session: -t expects an argument when trying to use ta without a session name.

@matthew-nm
Copy link

matthew-nm commented Mar 26, 2024

Yes, my tmux -V returns tmux 3.4.

And if I enter either of the commands tmux attach-session or tmux attach-session -t (no target specified for -t) it takes me to one of my running sessions. I'm on macOS Sonoma 14.2.1.

However, my Ubuntu workstation on tmux 3.2a doesn't like it when I don't have an argument for -t, but using your version of ta gets around that. I understand what your function is getting at now and it does seem to work properly for lower versions of tmux.

EDIT: And it's clear now that of course my snippet would not solve this problem, only your function addresses the issue for <3.4.

@matthew-nm
Copy link

If you want more work ;) you could perhaps check for the tmux version and set the aliases to tmux attach -t, tmux attach -d -t, etc. if the version is >=3.4, and use the function approach otherwise. That way future autocomplete changes to tmux will come through directly via the aliases (which support empty targets), whereas lower versions get the benefit of empty target commands as well as the current implementation of autocompletes that you've duplicated.

Thanks for making this functional for a wide range of tmux's.

@detroyejr
Copy link
Contributor Author

Thanks for confirming. That's weird because tmux didn't complain when I was on 3.2a (Linux/Nixos) and only started requiring an argument for -t after moving to 3.4. Maybe others can let us know which behavior they're seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: plugin Issue or PR related to a plugin
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Recent update breaks autocomplete for tmux plugin aliases
4 participants