fish: Fixes and improvements to CTRL-R

[bitraid]

A few more changes to command history:

• SHIFT-DELETE:
  • Improve performance by only calling history delete once and passing the selected items via command substitution.
• Preview window:
  • Handle very long lists of selected commands (using temporary files instead of command arguments).
  • Reformat only the highlighted command. Selected commands are only indented and colorized, so that they match how they are inserted by pressing ENTER (instead of ALT-ENTER).
  • Include trailing whitespace characters, so commands that end with \␊ don't lose the trailing newline and cause a following selected command to be shown as a continuation of the previous one.
  • Also bind the check of whether to display the preview to multi, so it will be triggered when selecting the very last command (which is not triggered by focus).
• Prefix of commands list:
  • Enable changing the timestamp to date/time, or disabling it completely, by pressing ALT-T (uses the new change-with-nth option).
  • Remove the dependence of the external date command for conversion.
  • Set the color to $fish_color_comment, so it is easier distinguished from the actual command.
• Command insertion:
  • Fix an issue with commands that end with \␊: accept-nth strips the trailing newline, but it is important to keep it when inserting multiple commands. Consider the following example: The commands rm -rf ~/trash \␊ (1) and ls ~/ (2) are selected for insertion. What will be inserted instead, is a single rm -rf command with a line break after ~/trash and the last token being /~ (will wipe the home directory!). For that reason, a become action is bind to ENTER, that reads the selected commands from a temporary file (for not hitting the single argument length limit) created with the s flag, and then having to manually delete the file (since it's not possible for fzf to remove the file). This is unnecessary for most cases though, and it would be much better if accept-nth (and maybe also with-nth) had a flag to keep trailing whitespace characters. Let me know what you think.
• Update README:
  • Inform about the ability to select multiple commands.
  • Document the key bindings which are unique to fish shell.
  • Add examples of how to change the default prefix through $FZF_CTRL_R_OPTS.

---

While working on this, I noticed a couple of issues that I think are worth mentioning:

• The become command may fail to execute and won't return to the shell prompt: Because the command is passed to the shell as a single argument, it is possible to exceed the 128K limit of MAX_ARG_STRLEN. This is not so serious for preview or transform (will return argument list too long error), but for become the shell will be left in a broken state. This doesn't affect the script, but maybe fzf should not attempt to execute the command if its final total length is longer than 128 * 1024 - 1 characters.
• Scrolling in preview window with enabled wrap/wrap-word, does not happen line by line for wrapped lines. This means that it's not possible to scroll a long line that doesn't completely fit:

echo (string repeat -n 400 'test ') eol | fzf --preview="echo {}" --preview-window=wrap-word --height=8

                                                ╭─────────────────────────────────────────────╮
                                                │ test test test test test test test test     │
                                                │ ↳ test test test test test test test test   │
                                                │ ↳ test test test test test test test test   │
                                                │ ↳ test test test test test test test test   │
▌ test test test test test test test test tes·· │ ↳ test test test test test test test test   │
  1/1 ───────────────────────────────────────── │ ↳ test test test test test test test test   │
>                                               ╰─────────────────────────────────────────────╯

If follow is also set, it scrolls too far down and no part of the line is visible (notice the info shows 2/1).

echo (string repeat -n 400 'test ') eol | fzf --preview="echo {}" --preview-window=wrap-word,follow --height=8

                                                ╭─────────────────────────────────────────────╮
                                                │                                         2/1 │
                                                │                                             │
                                                │                                             │
                                                │                                             │
▌ test test test test test test test test tes·· │                                             │
  1/1 ───────────────────────────────────────── │                                             │
>                                               ╰─────────────────────────────────────────────╯

Scrolling up, will scroll to the beginning of the line, and the ability to scroll again is lost (it's not possible to view the end of the line).

[bitraid]

I was planning to also add --freeze-left=1 but I forgot. If you agree that this is a good default, let me know and I will update the commit.

EDIT: This will add extra space between the prefix and the command, though. There is also an issue when wrapping is enabled by pressing CTRL-/. Run the following command and press <ctrl-/>:

echo 1\tfoo\n2\t(string repeat -n 100 'bar ') | fzf --freeze-left=1 --no-wrap

▌ 2       bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar
▌ ↳ bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar
▌ ↳ bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar
▌ ↳ bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar bar
▌ ↳ bar bar bar bar bar bar bar bar bar bar bar bar bar
▌ 1        foo
  2/2 ────────────────────────────────────────────────────────────────────────────────────────
>

The space between 2 and bar has decreased, but it hasn't for 1 and foo. If the focus changes, it also decreases for foo.

[junegunn]

Thanks a lot! This looks like a solid improvement. change-with-nth is a nice touch. Let me test it a bit more.

One question: Did you intend to color the first word of a command entry red? Should we append (set_color normal) at the end of --show-time?

• it is possible to exceed the 128K limit

So I see you appropriately adoped the f flag here.

maybe fzf should not attempt to execute the command

I think it's a good suggestion, though I wonder how we should notify the user when that happens.

• Scrolling in preview window with enabled wrap/wrap-word, does not happen line by line for wrapped lines.

This was by design to simplify the implementation. Being able to scroll into the middle of a wrapped line brings complexity. For example, we would have to recalculate the display offset whenever the window is resized. Vim has the same limitation, so I thought it would be acceptable for a "preview" window.

If follow is also set, it scrolls too far down and no part of the line is visible (notice the info shows 2/1).

Ah, it's a bug. Thanks. I'll look into it.

The space between 2 and bar has decreased

Another bug! Thanks!

I was planning to also add --freeze-left=1 but I forgot. If you agree that this is a good default, let me know and I will update the commit.

Sounds like a plan.

[bitraid]

One question: Did you intend to color the first word of a command entry red? Should we append (set_color normal) at the end of --show-time?

This didn't happen for me - $fish_color_command was successfully applied to the command names, but I was not using the default theme colors. Apparently this only happens with the default theme. I tested many other themes by running fish_config theme choose, and they all seem to work as expected. I don't really know why the command color isn't applied for the default theme, but I'm glad we found out so we can fix it. I will update the commit.

[junegunn]

Apparently this only happens with the default theme.

I see, I'm not a fish user, so fortunately I had zero configuration :)

The two commits I pushed to master should fix the issues you reported. Please take a look.

[bitraid]

The two commits I pushed to master should fix the issues you reported. Please take a look.

I tested them and they work great. Thanks!

[junegunn]

Test failure seems unrelated. Let me see.

Confirmed unrelated. It can fail on small screens. Let me fix the test case on master.

[junegunn]

• it would be much better if accept-nth (and maybe also with-nth) had a flag to keep trailing whitespace characters.

Hmm, what do you think about making \n an exception?

diff --git a/src/tokenizer.go b/src/tokenizer.go
index 25448f59..a143636c 100644
--- a/src/tokenizer.go
+++ b/src/tokenizer.go
@@ -161,7 +161,7 @@ func awkTokenizer(input string) ([]string, int) {
 	end := 0
 	for idx := 0; idx < len(input); idx++ {
 		r := input[idx]
-		white := r == 9 || r == 32
+		white := unicode.IsSpace(rune(r))
 		switch state {
 		case awkNil:
 			if white {
@@ -218,6 +218,10 @@ func Tokenize(text string, delimiter Delimiter) []Token {
 	return withPrefixLengths(tokens, 0)
 }
 
+func isSpace(r rune) bool {
+	return r != '\n' && unicode.IsSpace(r)
+}
+
 // StripLastDelimiter removes the trailing delimiter and whitespaces
 func StripLastDelimiter(str string, delimiter Delimiter) string {
 	if delimiter.str != nil {
@@ -231,7 +235,7 @@ func StripLastDelimiter(str string, delimiter Delimiter) string {
 			}
 		}
 	}
-	return strings.TrimRightFunc(str, unicode.IsSpace)
+	return strings.TrimRightFunc(str, isSpace)
 }
 
 func GetLastDelimiter(str string, delimiter Delimiter) string {

It won't affect most use cases. \n can only appear in items when --read0 is specified, and maybe it's not right to treat it as an ordinary white space in that case. But I'm not sure.

EDIT: Well, there can be cases where this is not desirable.

# A new line character after the hash used to be stripped
git log -z --format='%H%n%s%n%b' | fzf --reverse --read0 --gap --highlight-line --with-nth '{1} => {2..}'

[bitraid]

Hmm, what do you think about making \n an exception?

That would work for the script. But it couldn't be used for other cases, for example a field containing filenames, because they could have trailing spaces.

It won't affect most use cases. \n can only appear in items when --read0 is specified, and maybe it's not right to treat it as an ordinary white space in that case. But I'm not sure.

EDIT: Well, there can be cases where this is not desirable.

I think that it would make sense to keep it when --print0 is set. Trailing whitespace can always be trimmed afterwards, but it can't be inserted it isn't known it is missing.

[junegunn]

Thanks for the input. Let me think a bit more about it.

That said, I think this PR is great and already ready as it is. But since it's mentioning the new feature on the README page, we may delay merging it, or merge it to the devel branch instead, so that they're not exposed to the public before a new release.

[junegunn]

How about this approach: when --delimiter is explicitly given, like in this case, only strip the delimiter itself, not trailing whitespace. Only in the default AWK mode (no delimiter) should trailing whitespace be stripped, since whitespace IS the delimiter in that mode. With a custom delimiter, whitespace is a part of the data; stripping it is an assumption we probably shouldn't make.

This actually aligns better with the current man page description of --accept-nth option:

--accept-nth=N[,..] or TEMPLATE
      Define which fields to print on accept. The last delimiter is stripped
      from the output. For advanced transformation, you can provide a template
      containing field index expressions in curly braces. When you use
      a template, the trailing delimiter is stripped from each expression,
      giving you more control over the output. {n} in template evaluates to the
      zero-based ordinal index of the line.

However, while this sounds reasonable, it breaks several contracts:

  1) Failure:
TestCore#test_accept_nth_template [test/test_core.rb:2126]:
--- expected
+++ actual
@@ -1 +1 @@
-["[0] 1st: foo, 3rd: baz, 2nd: bar"]
+["[0] 1st: foo  , 3rd: baz, 2nd: bar"]


  2) Failure:
TestCore#test_accept_nth_regex_delimiter [test/test_core.rb:2108]:
Expected: ["bar,bar,foo  :,:bazfoo"]
  Actual: ["bar,bar,foo  :,:bazfoo  "]

  3) Failure:
TestCore#test_accept_nth_string_delimiter [test/test_core.rb:2099]:
Expected: ["bar,bar,foo  ,bazfoo"]
  Actual: ["bar,bar,foo  ,bazfoo  "]

424 runs, 5063 assertions, 3 failures, 0 errors, 6 skips

I'm not sure how may users rely on the current behavior though.

# Changed behavior
echo 'foo,  bar , baz' | fzf --delimiter , --with-nth '{2} // {1} // {3} // {1}'

# A potential workaround
echo 'foo,  bar , baz' | fzf --delimiter "\s*,\s*" --with-nth '{2} // {1} // {3} // {1}'

[junegunn]

I decided to go that route. I think you can now simplify that part by rebasing onto the latest master.

[bitraid]

Thanks, I made the final change. I also switched the merge branch to devel so it can be merged before the release, and I can start working on a PR for the SHIFT-TAB rewrite (which also modifies key-bindings.fish because common.fish will no longer be used).

[junegunn]

Thanks for the great work!