-
Notifications
You must be signed in to change notification settings - Fork 83
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
A proper implementation of fzf-tab #293
Changes from all commits
8c8b21a
2d6e6da
8213717
122ff4f
6331eed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
# External limitations: | ||
# - fish-shell/fish-shell#9577: folders ending in '=' break fish's internal path completion | ||
# (It thinks that it has to complete from / because `a_folder=/` looks like an argument to fish. | ||
# - fish cannot give reliable context information on completions. Knowing whether a completion | ||
# is a file or argument can only be determined via a heuristic. | ||
# - Completion is slow. Not because this script is slow but because fish's `complete` command | ||
# can take several seconds even for just 100 entries. Just run `time complete --escape --do-complete l` | ||
# to see for yourself. | ||
set --global _fzf_search_completions_min_description_offset 14 | ||
function _fzf_search_completions --description "Shell completion using fzf" | ||
# Produce a list of completions from which to choose | ||
# and do nothing if there is nothing to select from | ||
set -l cmd (commandline --cut-at-cursor --tokenize) (commandline --current-token) | ||
set -l completions (complete --escape --do-complete "$cmd") | ||
test (count $completions) -eq 0; and return | ||
PatrickF1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
set -l results | ||
set -l first_completion (string split --fields 1 --max 1 --right \t -- $completions[1]) | ||
|
||
# A prefix that will be prepended before each selected completion | ||
set -l each_prefix "" | ||
|
||
# Only skip fzf if there is a single completion and it starts with the expected completion prefix. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WDYM "starts with the expected completion prefix"? why would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes the shell transforms or eliminates quotes, variables, ... Consider There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay this is consistent with how fish currently operates: if only one completion, it will auto-complete while preserving existing quotes and backslashes. Makes sense, good thinking! |
||
# Otherwise, the completion originates from a match in the description which the user might | ||
# want to review first (e.g. man ima<TAB> might match `feh`, an image viewer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is possible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Odd! I'll look into this more. |
||
if test (count $completions) -eq 1; and test (string sub -l (string length -- (commandline -ct)) -- $first_completion) = (commandline -ct) | ||
# If there is only one option then we don't need fzf | ||
set results $first_completion | ||
else | ||
# Preprocess the whole list and extract the actual completion any the corresponding description (if any) | ||
# From here on, make sure to use -- (argument list end) and zero termination everywhere to prevent introducing | ||
# new edge-cases where characters are interpreted wrongly | ||
set -l actual_completions | ||
set -l descriptions | ||
for i in (seq (count $completions)) | ||
# split on \t from the right, at most one time and use the first (left) field. | ||
# This is the actual completion | ||
set --append actual_completions (string split --fields 1 --max 1 --right \t -- $completions[$i]) | ||
# The other field is the description, if it exists (otherwise set empty). | ||
set --append descriptions (string split --fields 2 --max 1 --right \t -- $completions[$i] ; or echo "") | ||
end | ||
oddlama marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Find the common prefix of all completions. Yes this seems to be a hard | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh shoot, I just realized that by overriding the existing tab functionality, we actually need to fill in a lot more than just searching completions, including filling up to the common prefix. Hmm now I'm on the fence again about implementing this UNLESS we use a different key binding and shed this feature, so the user knows when they call the function, they are purely doing completions search and nothing else. I really do not want to be in the business of replicating what fish already does, only to supplement it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do with this PR what you want :) |
||
# thing to do in fish if it should be fast. No external process may be | ||
# spawned as this is too slow. So fish internals it is :/ | ||
set -l common_prefix $completions[1] | ||
set -l common_prefix_length (string length -- $common_prefix) | ||
for comp in $actual_completions[2..] | ||
if test (string sub -l $common_prefix_length -- $comp) != $common_prefix | ||
set -l new_common_prefix_length 0 | ||
set -l try_common_prefix_length (math min $common_prefix_length,(string length -- $comp)) | ||
# Binary search for new common prefix | ||
set -l step $try_common_prefix_length | ||
while test $step != 0 | ||
set step (math --scale 0 $step / 2) | ||
# Adjust range to the left or right depending on whether the current new prefix matches | ||
set -l xs (string sub -l $try_common_prefix_length -- $comp $common_prefix) | ||
set -l op (test $xs[1] = $xs[2]; and printf "+"; and set new_common_prefix_length $try_common_prefix_length; or printf "-") | ||
set try_common_prefix_length (math --scale 0 $try_common_prefix_length $op (math max 1,$step)) | ||
end | ||
set common_prefix_length $new_common_prefix_length | ||
set common_prefix (string sub -l $new_common_prefix_length -- $common_prefix) | ||
# Stop if there is no common prefix | ||
test $common_prefix_length = 0; and break | ||
end | ||
end | ||
|
||
# If the common prefix includes a / we are completing a file path. | ||
# Strip the prefix until the last / completely and later re-add it on the replaced token | ||
set -l path_prefix (string match --regex --groups-only -- '^(.*/)[^/]*$' $common_prefix) | ||
if test $status = 0 | ||
set -l path_prefix_length (string length -- $path_prefix) | ||
set -l new_start (math 1 + $path_prefix_length) | ||
for i in (seq (count $completions)) | ||
set completions[$i] (string sub -s $new_start -- $completions[$i]) | ||
set actual_completions[$i] (string sub -s $new_start -- $actual_completions[$i]) | ||
end | ||
|
||
# We have a path-like prefix and will therefore strip this common prefix from all | ||
# completions to un-clutter the menu. | ||
set each_prefix (string sub -l $path_prefix_length -- $common_prefix) | ||
set common_prefix_length (math $common_prefix_length - $path_prefix_length) | ||
set common_prefix (string sub -s $new_start -- $common_prefix) | ||
end | ||
|
||
# Detect whether descriptions are present and the length of each completion. | ||
set -l has_descriptions false | ||
set -l longest_completion $_fzf_search_completions_min_description_offset | ||
for i in (seq (count $completions)) | ||
if string match --quiet -- "*"\t"*" $completions[$i] | ||
set has_descriptions true | ||
|
||
# Here we additionally remember the longest completion to align the descriptions in fzf later | ||
set longest_completion (math max $longest_completion,(string length -- $actual_completions[$i])) | ||
set completions[$i] $actual_completions[$i]\t(set_color -i yellow)$descriptions[$i](set_color normal) | ||
end | ||
end | ||
|
||
# FIXME Would technically work, but not sure it's worth the effort to match the descriptions | ||
# after filtering. | ||
# Remove tokens from the completion list that are already present on the current commandline. | ||
#set -l all_tokens (commandline -o) | ||
#set -l current_token_index (math 1 + (count (commandline -co))) | ||
#set --erase all_tokens[$current_token_index] | ||
#set -l remaining (comm --zero-terminated -23 (string join0 -- $actual_completions | psub) (string join0 -- $all_tokens | sort | psub) | string split0) | ||
|
||
# TODO pressing / in a completion should add the completion and immediately start a new completion | ||
test $has_descriptions = true; and set -l fzf_complete_description_opts \ | ||
--tabstop=(math 2 + $longest_completion) | ||
set -l fzf_output ( | ||
string join0 -- $completions \ | ||
| _fzf_wrapper \ | ||
--read0 \ | ||
--print0 \ | ||
--ansi \ | ||
--multi \ | ||
--bind=tab:down,btab:up,change:top,ctrl-space:toggle \ | ||
--tiebreak=begin \ | ||
--query=$common_prefix \ | ||
--print-query \ | ||
$fzf_complete_description_opts \ | ||
$fzf_complete_opts \ | ||
| string split0) | ||
|
||
switch $pipestatus[2] | ||
case 0 | ||
# If something was selected, discard the current query | ||
set results $fzf_output[2..-1] | ||
case 1 | ||
# User accepted without selecting anything, thus we will | ||
# use just the current query | ||
set results $fzf_output[1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is better described as "Nothing matched the user's query so just use their query". EDIT: on a second thought, this is not the behavior of any of the other search commands in this plugin, which is to exit without doing anything if you user selects nothing. Not only is that, I would argue, less surprising the the user who intentionally selected nothing at all, it's also important to have consistent UX with all the other search commands. So I think we should remove this. |
||
case '*' | ||
# Fzf failed, do nothing. | ||
commandline -f repaint | ||
return | ||
end | ||
|
||
# Strip anything after last \t (the descriptions) | ||
for i in (seq (count $results)) | ||
set results[$i] (string split --fields 1 --max 1 --right \t -- $results[$i]) | ||
end | ||
end | ||
|
||
set -l prefix "" | ||
set -l suffix " " | ||
# By default we want to append a space to completed options. | ||
# While this is always true when competing multiple things, there | ||
# are some cases in which we don't want to add a space: | ||
if test (count $results) -eq 1 | ||
# When a completion ends in a / we usually want to keep adding to that completion. | ||
# This may be because it is a directory (or link to a directory), or just | ||
# an option that takes a category/name tuple like `emerge app-shells/fish`. | ||
# Also, completions like ~something are directories. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wdym There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Home directories, such as ~patrick for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh you mean ~/something then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, ~/something is the folder called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh shoot I didn't know this syntax existed! |
||
# | ||
# In a similar manner, completions ending in = are usually part of an argument | ||
# that expects a parameter (like --color= or dd if=). The same logic applies here. | ||
set -l first_char (string sub -l 1 -- $results[1]) | ||
set -l last_char (string sub -s -1 -- $results[1]) | ||
if test $last_char = =; or test $last_char = / | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can use |
||
set suffix "" | ||
else if string match --regex --quiet -- '^~[^/]*$' $results[1] | ||
set suffix "/" | ||
end | ||
end | ||
|
||
if not contains -- "--" (commandline -co) | ||
# If a path is being completed and it starts with --, we add -- to terminate argument interpreting. | ||
# Technically not always correct (the program may not accept --), but the alternative is worse | ||
# and this will make the user notice it. | ||
for r in $results | ||
test - = (string sub -l 1 -- $r); and test -e $r; and set prefix "-- "; and break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make this into a normal if statement for readability, please? |
||
end | ||
end | ||
|
||
# If each_prefix is set, we need to apply it to each result | ||
# before replacing the token | ||
if test -n $each_prefix | ||
set -l new_tokens | ||
for r in $results | ||
set -a new_tokens $each_prefix$r | ||
end | ||
commandline -t -- "$prefix$new_tokens$suffix" | ||
else | ||
commandline -t -- "$prefix$results$suffix" | ||
end | ||
|
||
commandline -f repaint | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ function fzf_configure_bindings --description "Installs the default key bindings | |
# no need to install bindings if not in interactive mode or running tests | ||
status is-interactive || test "$CI" = true; or return | ||
|
||
set options_spec h/help 'directory=?' 'git_log=?' 'git_status=?' 'history=?' 'processes=?' 'variables=?' | ||
set options_spec h/help 'directory=?' 'git_log=?' 'git_status=?' 'history=?' 'processes=?' 'variables=?' 'completions=?' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reminder to self (or I suppose you could do it too oddlama), need to update the configure bindings help function. |
||
argparse --max-args=0 --ignore-unknown $options_spec -- $argv 2>/dev/null | ||
if test $status -ne 0 | ||
echo "Invalid option or a positional argument was provided." >&2 | ||
|
@@ -16,13 +16,14 @@ function fzf_configure_bindings --description "Installs the default key bindings | |
else | ||
# Initialize with default key sequences and then override or disable them based on flags | ||
# index 1 = directory, 2 = git_log, 3 = git_status, 4 = history, 5 = processes, 6 = variables | ||
set key_sequences \e\cf \e\cl \e\cs \cr \e\cp \cv # \c = control, \e = escape | ||
set key_sequences \e\cf \e\cl \e\cs \cr \e\cp \cv \t # \c = control, \e = escape | ||
set --query _flag_directory && set key_sequences[1] "$_flag_directory" | ||
set --query _flag_git_log && set key_sequences[2] "$_flag_git_log" | ||
set --query _flag_git_status && set key_sequences[3] "$_flag_git_status" | ||
set --query _flag_history && set key_sequences[4] "$_flag_history" | ||
set --query _flag_processes && set key_sequences[5] "$_flag_processes" | ||
set --query _flag_variables && set key_sequences[6] "$_flag_variables" | ||
set --query _flag_completions && set key_sequences[7] "$_flag_completions" | ||
|
||
# If fzf bindings already exists, uninstall it first for a clean slate | ||
if functions --query _fzf_uninstall_bindings | ||
|
@@ -36,6 +37,7 @@ function fzf_configure_bindings --description "Installs the default key bindings | |
test -n $key_sequences[4] && bind --mode $mode $key_sequences[4] _fzf_search_history | ||
test -n $key_sequences[5] && bind --mode $mode $key_sequences[5] _fzf_search_processes | ||
test -n $key_sequences[6] && bind --mode $mode $key_sequences[6] "$_fzf_search_vars_command" | ||
test -n $key_sequences[7] && bind --mode $mode $key_sequences[7] _fzf_search_completions | ||
end | ||
|
||
function _fzf_uninstall_bindings --inherit-variable key_sequences | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain why you do this thing of breaking up the command into two parts (before cursor and the current token)? is the reason because you want to remove everything after the current token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for generating completions we only want to consider what comes before the cursor. The reason why keep it in an array using the --tokenize flag is that this still allows the code later to access the individual tokens, instead of coercing it to a single string. This is currently not needed technically, but the commented out part below might use it in the future to eliminate completions that are already present (eg. passing
--color
twice makes no sense).So it's not actually two parts, it's just concatenating two arrays as
commandline
doesn't offer this in a single command.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see thanks for explaining.
For the sake of a lower-risk and faster MVP, can you update this to just use normal
complete --do-complete
? I also suspect we'll never build this out because even though for most commands the same option twice doesn't make sense, there will be a few in the wild that breaks that assumption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch my suggestion about doing the completion of the entire commandline vs considering what comes before the cursor. We definitely want to only complete what comes before the cursor rather than at the end of the command line.