change-(with-)nth: apply value after current one

- Find current value in bound list and apply next valid,
  non-current one
- Keep rotating bound list as a position hint, so repeated
  values in the list still reach every position
- Treat empty --nth and '..' as the same value
- Ignore invalid nth values, also in transform-nth, as with-nth
  does. Previously, they restored default --nth
- Previously, first value of rotated list was applied blindly
    - Key press spent on value already applied, e.g. first value
      equal to initial --nth
    - Stuck on change-nth printed by transform (new action on
      each run), as it always applied first value
- Add nextValue helper for reuse by change-layout

CHANGELOG.md

@@ -9,6 +9,19 @@ CHANGELOG
     - Only `change-nth` and `change-with-nth` rotate through `|`-separated values
     - Fixed the bound command being replaced with the output. e.g. With `transform-nth(echo '1|2')`, the next key press ran `2|1` as a shell command.
     - A `--with-nth` template in the output can now contain `|`. e.g. `{2}|{1}` was cut to `{2}`.
+- An invalid value for `change-nth` and `transform-nth` no longer restores the default `--nth`. It is ignored, as with `change-with-nth` and `transform-with-nth`.
+- `change-nth` and `change-with-nth` now apply the value after the current one in the `|`-separated list
+    - A value equal to the current one is skipped, e.g. the first value when it is the initial `--nth`
+    - Invalid values in the list are skipped
+    - The rotation follows the current value even when another binding changed it, or when the action is printed by `transform` (unless the list contains the same value more than once)
+    - You can list the values in their natural order, starting with the initial one
+      ```sh
+      # Previously, you would have to write:
+      ls -al | fzf --with-nth .. --bind 'space:change-with-nth:1|2|..'
+
+      # Now you can write:
+      ls -al | fzf --with-nth .. --bind 'space:change-with-nth:..|1|2'
+      ```
 - nushell
     - The completion integration now requires nushell 0.116.1 or above, where an interactive completer can take the terminal through the `@interactive` attribute (#4934) (@sim590)
 

man/man1/fzf.1

@@ -2060,8 +2060,8 @@ A key or an event can be bound to one or more of the following actions.
     \fBchange\-list\-label(...)\fR       (change \fB\-\-list\-label\fR to the given string)
     \fBchange\-multi\fR                 (enable multi-select mode with no limit)
     \fBchange\-multi(...)\fR            (enable multi-select mode with a limit or disable it with 0)
-    \fBchange\-nth(...)\fR              (change \fB\-\-nth\fR option; rotate through the multiple options separated by '|')
-    \fBchange\-with\-nth(...)\fR         (change \fB\-\-with\-nth\fR option; rotate through the multiple options separated by '|')
+    \fBchange\-nth(...)\fR              (change \fB\-\-nth\fR option; rotate through the multiple options separated by '|', starting from the one after the current option)
+    \fBchange\-with\-nth(...)\fR         (change \fB\-\-with\-nth\fR option; rotate through the multiple options separated by '|', starting from the one after the current option)
     \fBchange\-pointer(...)\fR          (change \fB\-\-pointer\fR option)
     \fBchange\-preview(...)\fR          (change \fB\-\-preview\fR option)
     \fBchange\-preview\-label(...)\fR    (change \fB\-\-preview\-label\fR to the given string)

src/terminal.go

@@ -6304,6 +6304,43 @@ func (t *Terminal) startTimers(ctx context.Context) {
 	}
 }
 
+// nextValue returns the first valid, non-current value after the current one
+// in a '|'-separated list, and the list rotated to end with it.
+// isValid can be nil.
+func nextValue(expr string, isCurrent func(string) bool, isValid func(string) bool) (string, string) {
+	valid := func(value string) bool {
+		return isValid == nil || isValid(value)
+	}
+	// An invalid value is never current, even if it resolves to the current one
+	current := func(value string) bool {
+		return valid(value) && isCurrent(value)
+	}
+	values := strings.Split(expr, "|")
+	start := 0
+	// Last value is the one applied by the previous call
+	if !current(values[len(values)-1]) {
+		for i, value := range values {
+			if current(value) {
+				start = i + 1
+				break
+			}
+		}
+	}
+	for i := range values {
+		pick := (start + i) % len(values)
+		if value := values[pick]; valid(value) && !current(value) {
+			return value, strings.Join(slices.Concat(values[pick+1:], values[:pick+1]), "|")
+		}
+	}
+	// Nothing to pick
+	for _, value := range values {
+		if valid(value) {
+			return value, expr
+		}
+	}
+	return values[0], expr
+}
+
 // Loop is called to start Terminal I/O
 func (t *Terminal) Loop() error {
 	// prof := profile.Start(profile.ProfilePath("/tmp/"))
@@ -7304,25 +7341,38 @@ func (t *Terminal) Loop() error {
 				req(reqList, reqInfo)
 			case actChangeNth, actTransformNth, actBgTransformNth:
 				capture(true, func(expr string) {
+					empty := len(expr) == 0
+					// Empty value means all fields if it is the whole
+					// expression, otherwise the default --nth
+					resolve := func(value string) ([]Range, bool) {
+						if nth, err := splitNth(value); err == nil || empty {
+							return nth, true
+						} else if len(value) == 0 {
+							return t.nth, true
+						}
+						return nil, false
+					}
+					// Empty or a single '..' means all fields
+					allFields := func(nth []Range) bool {
+						return len(nth) == 0 || len(nth) == 1 && nth[0].IsFull()
+					}
+					isCurrent := func(nth []Range) bool {
+						return compareRanges(t.nthCurrent, nth) || allFields(t.nthCurrent) && allFields(nth)
+					}
 					// Only change-nth cycles through '|'-separated values
-					tokens := []string{expr}
 					if a.t == actChangeNth {
-						tokens = strings.Split(expr, "|")
+						expr, a.a = nextValue(expr, func(value string) bool {
+							nth, _ := resolve(value)
+							return isCurrent(nth)
+						}, func(value string) bool {
+							_, ok := resolve(value)
+							return ok
+						})
 					}
-					if nth, err := splitNth(tokens[0]); err == nil || len(expr) == 0 {
-						// Changed
+					if nth, ok := resolve(expr); ok && !isCurrent(nth) {
 						newNth = &nth
-					} else {
-						// The default
-						newNth = &t.nth
-					}
-					// Cycle
-					if len(tokens) > 1 {
-						a.a = strings.Join(append(tokens[1:], tokens[0]), "|")
-					}
-					if !compareRanges(t.nthCurrent, *newNth) {
 						changed = true
-						t.nthCurrent = *newNth
+						t.nthCurrent = nth
 						t.forceRerenderList()
 					}
 				})
@@ -7331,18 +7381,22 @@ func (t *Terminal) Loop() error {
 					break Action
 				}
 				capture(true, func(expr string) {
-					tokens := []string{expr}
+					resolve := func(value string) string {
+						// Empty value restores the default --with-nth
+						if len(value) == 0 {
+							return t.withNthDefault
+						}
+						return value
+					}
 					if a.t == actChangeWithNth {
-						tokens = strings.Split(expr, "|")
-					}
-					withNthExpr := tokens[0]
-					if len(tokens) > 1 {
-						a.a = strings.Join(append(tokens[1:], tokens[0]), "|")
-					}
-					// Empty value restores the default --with-nth
-					if len(withNthExpr) == 0 {
-						withNthExpr = t.withNthDefault
+						expr, a.a = nextValue(expr, func(value string) bool {
+							return resolve(value) == t.withNthExpr
+						}, func(value string) bool {
+							_, err := nthTransformer(resolve(value))
+							return err == nil
+						})
 					}
+					withNthExpr := resolve(expr)
 					if withNthExpr != t.withNthExpr {
 						if factory, err := nthTransformer(withNthExpr); err == nil {
 							newWithNth = &withNthSpec{fn: factory(t.delimiter)}

src/terminal_test.go

@@ -927,3 +927,56 @@ func TestSplitOnIND(t *testing.T) {
 		}
 	}
 }
+
+func TestNextValue(t *testing.T) {
+	tests := []struct {
+		expr    string
+		current string
+		want    string
+		rotated string
+	}{
+		{"a", "x", "a", "a"},
+		{"a", "a", "a", "a"},
+		{"a|b|c", "x", "a", "b|c|a"},
+		{"a|b|c", "a", "b", "c|a|b"},
+		{"a|b|c", "b", "c", "a|b|c"},
+		{"a|b|c", "c", "a", "b|c|a"},
+		{"a|a|b", "a", "b", "a|a|b"},
+		{"a|b|b", "b", "a", "b|b|a"},
+		{"a|a", "a", "a", "a|a"},
+		{"a|b|", "", "a", "b||a"},
+		{"|a", "", "a", "|a"},
+		{"a|bad|b", "a", "b", "a|bad|b"},
+		{"bad|a", "x", "a", "bad|a"},
+		{"a|bad", "a", "a", "a|bad"},
+		// An invalid value is never current
+		{"a|bad|b", "bad", "a", "bad|b|a"},
+		// Nothing to pick: the first valid value
+		{"bad|a", "a", "a", "bad|a"},
+		{"bad", "x", "bad", "bad"},
+		// The last value is checked first
+		{"b|a|c|a", "a", "b", "a|c|a|b"},
+	}
+	isValid := func(v string) bool { return v != "bad" }
+	for _, tc := range tests {
+		got, rotated := nextValue(tc.expr, func(v string) bool { return v == tc.current }, isValid)
+		if got != tc.want || rotated != tc.rotated {
+			t.Errorf("nextValue(%q) with current %q: got (%q, %q), want (%q, %q)",
+				tc.expr, tc.current, got, rotated, tc.want, tc.rotated)
+		}
+	}
+}
+
+func TestNextValueRepeated(t *testing.T) {
+	// Presses with a rotated list reach every position, even with a repeated value
+	expr := "1|2|1|3"
+	current := "1"
+	got := []string{}
+	for range 6 {
+		current, expr = nextValue(expr, func(v string) bool { return v == current }, nil)
+		got = append(got, current)
+	}
+	if want := "2 1 3 1 2 1"; strings.Join(got, " ") != want {
+		t.Errorf("got %q, want %q", strings.Join(got, " "), want)
+	}
+}

test/test_core.rb

@@ -2342,6 +2342,94 @@ class TestCore < TestInteractive
     end
   end
 
+  def test_change_nth_next_value
+    tmux.send_keys %(echo 'a b c d' | #{FZF} --nth 1 --bind 'space:change-nth(1|2|3|4),x:change-nth(3),result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('[2]>') }
+    # Changed by another binding. The next value is the one after '3'.
+    tmux.send_keys :x
+    tmux.until { |lines| assert lines.any_include?('[3]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('[4]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+  end
+
+  def test_change_nth_all_fields_is_current
+    # '--nth ..' is stored as an empty list, which must match '..'
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth .. --bind 'space:change-nth(..|1|2)' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+  end
+
+  def test_change_nth_repeated_value
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth 1 --bind 'space:change-nth(1|2|1|3)' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+    %w[2 1 3 1 2].each do |nth|
+      tmux.send_keys :Space
+      tmux.until { |lines| assert lines.any_include?("[#{nth}]>") }
+    end
+  end
+
+  def test_change_nth_skips_invalid_value
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth 2 --bind 'space:change-nth(1|x|3)' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[2]>') }
+    %w[1 3 1].each do |nth|
+      tmux.send_keys :Space
+      tmux.until { |lines| assert lines.any_include?("[#{nth}]>") }
+    end
+  end
+
+  def test_change_nth_only_invalid_values_left
+    # 'x' is invalid and '3' is current, so nothing changes
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth 1 --bind 'a:change-nth(3),space:change-nth(x|3)+transform-prompt:echo "S[$FZF_NTH]> "' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+    tmux.send_keys :a
+    tmux.until { |lines| assert lines.any_include?('[3]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('S[3]>') }
+  end
+
+  def test_change_nth_invalid_value_ignored
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth 1 --bind 'a:change-nth(3),space:change-nth(bad)+transform-prompt:echo "S[$FZF_NTH]> "' --bind 'b:transform-nth(echo bad)+transform-prompt:echo "T[$FZF_NTH]> "' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+    tmux.send_keys :a
+    tmux.until { |lines| assert lines.any_include?('[3]>') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('S[3]>') }
+    tmux.send_keys :b
+    tmux.until { |lines| assert lines.any_include?('T[3]>') }
+  end
+
+  def test_change_nth_from_transform
+    # A new action is created on each run, so the next value is found from the current state
+    tmux.send_keys %(echo 'a b c' | #{FZF} --nth 1 --bind 'space:transform:echo "change-nth(1|2|3)"' --bind 'result:transform-prompt:echo "[$FZF_NTH]> "'), :Enter
+    tmux.until { |lines| assert lines.any_include?('[1]>') }
+    %w[2 3 1].each do |nth|
+      tmux.send_keys :Space
+      tmux.until { |lines| assert lines.any_include?("[#{nth}]>") }
+    end
+  end
+
+  def test_change_with_nth_skips_default_value
+    tmux.send_keys %(echo 'a b c' | #{FZF} --with-nth 1 --bind 'space:change-with-nth(|2)'), :Enter
+    tmux.until { |lines| assert lines.any_include?('> a') }
+    # Empty value is the default (field 1), which is current
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('> b') }
+  end
+
+  def test_change_with_nth_skips_invalid_value
+    tmux.send_keys %(echo 'a b c' | #{FZF} --with-nth 1 --bind 'space:change-with-nth(1|foo|2)'), :Enter
+    tmux.until { |lines| assert lines.any_include?('> a') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('> b') }
+    tmux.send_keys :Space
+    tmux.until { |lines| assert lines.any_include?('> a') }
+  end
+
   def test_transform_with_nth_search
     input = [
       'alpha bravo charlie',