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

bind: add variable and fix function completion #352

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

algorythmic
Copy link
Contributor

This adds completion to the common invocation forms of:

bind 'set variable-name value'

and

bind '"keyseq":function-name'

A little tricky because the whole argument is likely to be quoted and/or has colons in it.

Supported completions (works w/ or w/o : in $COMP_WORDBREAKS):

bind '"\C-r":ab    ==>    '"\C-r":abort'
bind "\C-r":ab     ==>    "\C-r":abort
bind "Con          ==>    "Control-          "Control:
bind 'set ke       ==>    'set keymap        'set keyseq-timeout
bind 'set keymap'  ==>    'set keymap emacs  'set keymap vi       ...

The variable names and values are completed in a case-insensitive and case-preserving manner.

I added some test but they seem to all be getting skipped, unless I put the _bind() function definition first in the file -- not sure what's up with that.

Add completion to the common invocation forms of:
    bind 'set variable-name value'
and
    bind '"keyseq":function-name'
multiple separate readline commands can be supplied, putting in the
check for `[[ $cword -gt 1 ]]` was a mistake.
for v in "${vals[@]}"; do
[[ $v == "$val"* ]] && COMPREPLY+=("${pre}${val}${v/#$val}")
done
eval "$reset"
Copy link
Owner

Choose a reason for hiding this comment

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

I think a plain $reset should work here, no need to eval. And the shopt probably needs protection from nondefault $IFS. See existing similar constructs, commit 5240618 and #143

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking eval "$reset" might be a better/cleaner construct to use in these cases, rather than doing the $IFS switcheroo, since eval "$reset" works the same way regardless of $IFS value and so doesn't require protection from non-default values. Curious what you think.

Copy link
Owner

Choose a reason for hiding this comment

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

Consistency and eval avoidance are good things to pursue.

_bind_vars()
{
local pre=$1 var=$2
local v reset=$(shopt -p nocasematch); shopt -s nocasematch
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above wrt shopt and eval

completions/bind Outdated
local v reset=$(shopt -p nocasematch); shopt -s nocasematch
while IFS=' ' read _ v _; do
[[ $v == "$var"* ]] && COMPREPLY+=("${pre}${var}${v/#$var}")
done < <(bind -v)
Copy link
Owner

Choose a reason for hiding this comment

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

Process substitution doesn't work under posix mode, see commit 153d6d4 and #190 for why simply unsetting it is not a good solution, need to come up with some other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Replaced with a command substitution approach.

_bind()
{
local cur prev words cword
_init_completion || return
local cur prev words cword IFS=$'\n'
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, this might actually provide for the IFS protection I mentioned above. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think IFS would have to contain a space in order for the expansion of $reset to be evaluated as separate words

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.

None yet

2 participants