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

A proper implementation of fzf-tab #293

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions completions/fzf_configure_bindings.fish
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ complete fzf_configure_bindings --long git_status --description "Change the key
complete fzf_configure_bindings --long history --description "Change the key binding for searching history"
complete fzf_configure_bindings --long processes --description "Change the key binding for searching processes"
complete fzf_configure_bindings --long variables --description "Change the key binding for searching variables"
complete fzf_configure_bindings --long completions --description "Change the key binding for searching completions"
189 changes: 189 additions & 0 deletions functions/_fzf_search_completions.fish
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 -co) (commandline -t)
Copy link
Owner

Choose a reason for hiding this comment

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

could you use the long version of these flags please? --cut-at-cursor --tokenize, --current-token. Btw, how come you need the second (commandline -t)?

Copy link
Author

Choose a reason for hiding this comment

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

Good suggestion, will do. Regarding your question, consider ls -l --color=<TAB>

  • commandline --cut-at-cursor --tokenize will yield ls and -l (only tokens before the cursor)
  • commandline --current-token yields --color= (the current token until the cursor)

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.
Copy link
Owner

Choose a reason for hiding this comment

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

WDYM "starts with the expected completion prefix"? why would complete --do-complete ever return a completion the user wouldn't want?

Copy link
Author

@oddlama oddlama Apr 11, 2023

Choose a reason for hiding this comment

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

Sometimes the shell transforms or eliminates quotes, variables, ...
If that happens, I don't want this to silently change the commandline and instead prompt the user with fzf so any change is previewed and can be cancelled if needed.

Consider ls foo"bar" could be completed as foobar, but the user might have intended to place quotes there to e.g. later insert content with spaces more easily. I often do that and it's annoying if deletes the quotes. If there for some reason is only one completion, this shouldn't just replace that.

Copy link
Owner

Choose a reason for hiding this comment

The 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)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is possible? complete --do-complete "fzf_configure_bindings change" and complete --do-complete "fzf_configure_bindings -change" both return nothing even though change matches the description of several fzf_configure_bindings options

Copy link
Author

Choose a reason for hiding this comment

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

The feh example was a real issue I encountered. Not sure if there are other fields used here, but this is definitely necessary to prevent matching non-real matches. Not sure how fish works here internally.

Copy link
Owner

Choose a reason for hiding this comment

The 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
Copy link
Owner

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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]
Copy link
Owner

@PatrickF1 PatrickF1 Jul 10, 2023

Choose a reason for hiding this comment

The 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.
Copy link
Owner

Choose a reason for hiding this comment

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

wdym ~something?

Copy link
Author

Choose a reason for hiding this comment

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

Home directories, such as ~patrick for example

Copy link
Owner

Choose a reason for hiding this comment

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

Oh you mean ~/something then?

Copy link
Author

Choose a reason for hiding this comment

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

No, ~/something is the folder called something in the home directory of the currently logged-in user, while ~something is the home directory of the user named something

Copy link
Owner

Choose a reason for hiding this comment

The 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 = /
Copy link
Owner

Choose a reason for hiding this comment

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

you can use -o instead of ; or test

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
Copy link
Owner

Choose a reason for hiding this comment

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

can you make this into a normal if statement for readability, please?
also can use -a to merge the two tests

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
6 changes: 4 additions & 2 deletions functions/fzf_configure_bindings.fish
Original file line number Diff line number Diff line change
Expand Up @@ -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=?'
Copy link
Owner

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Expand All @@ -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
Expand Down