Skip to content

style: restore the default group switching behavior#567

Merged
Aloxaf merged 3 commits intomasterfrom
fix/active-group-style
Mar 14, 2026
Merged

style: restore the default group switching behavior#567
Aloxaf merged 3 commits intomasterfrom
fix/active-group-style

Conversation

@Aloxaf
Copy link
Copy Markdown
Owner

@Aloxaf Aloxaf commented Mar 14, 2026

This PR simplifies some of the code and changes the default behavior from "show the first group" to "show all". The reason is that the content of the first group is already displayed at the top by default.

@Aloxaf Aloxaf marked this pull request as ready for review March 14, 2026 09:54
@Aloxaf Aloxaf requested a review from Copilot March 14, 2026 09:54
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts fzf-tab group switching to default to showing the full completion list (rather than filtering to the first group), and simplifies the implementation around large-list handling and header rendering.

Changes:

  • Feed fzf directly from the precomputed completions file to show all results by default.
  • Simplify group marker detection and selection logic in ftb-switch-group, including a large-list fast path.
  • Refactor header styling to apply “active group” formatting via pattern substitution.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
lib/ftb-switch-group Simplifies group marker detection and changes current-group persistence/styling logic.
lib/-ftb-fzf Changes initial fzf input from “filtered-by-script” to “raw completions file” to show all by default.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@alberti42
Copy link
Copy Markdown
Contributor

Hi Aloxaf, thank you for looking into the issue raised by QuarticCat. I'd like to discuss the approach, as I think there's a simpler fix that avoids regressions.

The simpler fix: adding an explicit All group to the initial render. This restores the ability to see all candidates while keeping the group-switching UX intact: users can switch to a specific group and come back to All if needed. It's a minimal, targeted change.

PR #567 as written introduces a few regressions I'd like to flag:

  • macOS/BSD compatibility: the new grep_cmd=${commands[grep]:-$commands[ggrep]} no longer checks for GNU grep. On macOS, this picks up BSD grep, which does not support -P (PCRE). The previous version-check logic was there specifically for this reason.
  • Header style: the new one-liner applies the active group style to trailing spaces as well, making underlines span the full line width rather than just the label text.

Regarding PR #566 reverting the LC_ALL=C change from #557: that flag was introduced to fix a real behavioral issue — without it, filenames containing special characters (e.g., backticks) sort in locale-dependent order, so the first candidate varies by system. This caused test failures and non-deterministic completions across locales. Could you explain why it was reverted? I want to make sure that regression isn't reintroduced.

I'm happy to open a clean PR that fixes QuarticCat's issue without reverting the other changes. Would that be welcome?

@alberti42
Copy link
Copy Markdown
Contributor

One more regression I forgot to mention: the active group header styling is now visually broken.

The previous code used [^]]## to match only the label text inside the brackets, so bold/underline was applied to exactly the label and nothing more:

line=${line/$current_prefix(#b)([^]]##)\]/$current_prefix$sgr_on${match[1]}]$sgr_off}

The new one-liner places sgr_on at the color prefix and sgr_off only before the trailing padding spaces, so the style ends up spanning the entire padded line width:

print -l ${${list[1,header_lines]/(#b)($group_sgr_prefixes[current]*)( ##$'\x1b[00m')/$match[1]$sgr_off$match[2]}/${group_sgr_prefixes[current]}/$sgr_on}

With underline style, this produces a very long underline across all the empty space after the label, which looks visually broken. The original approach was precise for a reason.

Screenshot 2026-03-14 at 12 09 06

One more regression I forgot to mention: the active group header styling is now visually broken.

The previous code used [^]]## to match only the label text inside the brackets, so bold/underline was applied to exactly the label and nothing more:

line=${line/$current_prefix(#b)([^]]##)\]/$current_prefix$sgr_on${match[1]}]$sgr_off}

The new one-liner places sgr_on at the color prefix and sgr_off only before the trailing padding spaces, so the style ends up spanning the entire padded line width:

print -l ${${list[1,header_lines]/(#b)($group_sgr_prefixes[current]*)( ##$'\x1b[00m')/$match[1]$sgr_off$match[2]}/${group_sgr_prefixes[current]}/$sgr_on}

Also, a UX issue with the "show all initially" approach: without an explicit All group, once the user starts cycling through groups there is no way to get back to seeing all candidates. The user is stuck. On top of that, the initial state — where all candidates are shown but the group headers are visible — is confusing: it looks like a group is active, but actually none is. An explicit All group makes the state unambiguous and navigation reversible.

@alberti42
Copy link
Copy Markdown
Contributor

I found another regression related to the header styling. The new one-liner:

print -l ${${list[1,header_lines]/(#b)($group_sgr_prefixes[current]*)( ##$'\x1b[00m')/$match[1]$sgr_off$match[2]}/${group_sgr_prefixes[current]}/$sgr_on}

The second substitution replaces the group's color SGR code (e.g. \e[94m for blue) with sgr_on (e.g. \e[1m for bold). The original color is completely removed from the output. The terminal then renders the label using whatever color was last set in the stream — which is the color of the last candidate in the previously active group. This means the active group label inherits a different color depending on which direction you cycled to reach it (see screenshots above).

The old code kept $current_prefix in the substitution and inserted $sgr_on after it:

line=${line/$current_prefix(#b)([^]]##)\]/$current_prefix$sgr_on${match[1]}]$sgr_off}

This preserved each group's original color and layered the active style on top — the right behavior. Each group keeps its color identity; bold/underline is added only to signal which one is active.

Changing the color on activation is also a UX regression in itself: each group already has a distinct color as a visual identity. Having that color change as you cycle is distracting and visually noisy, especially when the color bleeds unpredictably from the previous group.


Screenshots

Initial state — all groups in their natural colors, no active style applied:

first

After first group switch[external command] is active, but its original color has been replaced instead of having bold/underline layered on top:

second

After second group switch[shell function] is now active; same color replacement issue:

third

Reaching [alias] by cycling forward — the label inherits the color of the last candidate from the previous group:

fourth

Reaching [shell function] by cycling backward — same group position, different inherited color depending on navigation direction:

forth_alt

@Aloxaf
Copy link
Copy Markdown
Owner Author

Aloxaf commented Mar 14, 2026

@alberti42 Thanks for your review. I've updated the code.

  1. I checked it again and -P is actually redundant here, so it can just use the grep command directly here.
  2. [%d] is just a recommended setting. Using ] to determine the string boundary isn't reliable. I changed it to match the trailing space, so it should work fine now.
  3. The color issue has been fixed.

@Aloxaf
Copy link
Copy Markdown
Owner Author

Aloxaf commented Mar 14, 2026

The simpler fix: adding an explicit All group to the initial render. This restores the ability to see all candidates while keeping the group-switching UX intact: users can switch to a specific group and come back to All if needed. It's a minimal, targeted change.

I think the "All" group and the "first" group actually overlap, since the first group is already at the top of the "All" group. So I’m inclined to show all items by default, and when switching, go directly to the second group.

If we introduce an explicit "All" group, the logic here becomes more complicated: on the first switch, we would skip the first group, but on subsequent switches, we wouldn’t.

@Aloxaf
Copy link
Copy Markdown
Owner Author

Aloxaf commented Mar 14, 2026

Regarding PR #566, I think we should avoid changes that alter the current behavior unless the new behavior is clearly more correct.

Since there isn’t really a clear notion of one sort order being better than another, I’d prefer to preserve the existing behavior. If we do want to introduce a change here, it might be better to add a new option to the zstyle sort tag instead.

BTW, the current implementation is also a bit problematic: fzf-tab uses the sort tag from the :completion: context, but its behavior does not fully match zsh’s builtin behavior.

@alberti42
Copy link
Copy Markdown
Contributor

Hi Aloxaf, thank you for the quick update — the latest revision addresses the issues I raised:

  • Underline width is now correct — sgr_off is inserted before the trailing spaces, so the style applies only to the label text.
  • Color bleed is fixed — replacing $current_prefix with $current_prefix$sgr_on preserves each group's original color and layers the active style on top.
  • macOS/BSD grep compatibility is resolved — dropping -P and using plain command grep -a -o works on both GNU and BSD grep. Indeed, we did not need -P in first place.

Regarding the All group: I think the current behavior is confusing from a user perspective. The menu opens showing all candidates, then the first group switch jumps to group 2 — skipping group 1 entirely in the forward direction. Group 1 is only reachable by cycling backward, with no visual indication of this. And once you have switched at all, there is no way to return to the unfiltered view All.

More importantly, All and group 1 are not equivalent views — this has practical consequences when the user starts typing to fuzzy search. In All mode, results from every group appear mixed together. Switching to group 1 narrows the search to that group only, which is exactly what the user wants when they are looking for, say, an external command and do not want results from shell functions or aliases cluttering the list. Without a way to return to All, this filtering becomes a one-way door.

On the implementation side, an explicit [All] at index 1 actually eliminates the special case rather than adding one. The cycle becomes perfectly uniform: 1=All, 2=group 1, 3=group 2, …, wrap back to 1. No index offset, no skipping logic. The only change needed in ftb-switch-group is a single branch in the filter: when current == 1, print all candidates instead of filtering.

For veteran power users who prefer the current behavior (first view shows all, with the first group visible at the top), we can add an option to hide the All group and open directly with all options shown. Or the other way around, we can leave the current behavior as the default one, but offer an option to users who like to have a All group for a simplified mental model / practicality when fuzzy-searching.

Would you be open to this approach? I am happy to submit a PR building on top of yours. Or would you prefer to make the implementation yourself?

@alberti42
Copy link
Copy Markdown
Contributor

Since there isn’t really a clear notion of one sort order being better than another, I’d prefer to preserve the existing behavior. If we do want to introduce a change here, it might be better to add a new option to the zstyle sort tag instead.

Regarding LC_ALL=C: you are absolutely right that respecting the user's locale is the correct behavior, and I fully agree with the reasoning in PR #566.

The reason I had introduced LC_ALL=C was purely to ensure deterministic sorting so that a test would pass reliably across different locales. It was not motivated by a belief that C-locale ordering is inherently better.

I just re-ran the tests on my machine (en_US.UTF-8) without LC_ALL=C and the failure is reproducible:

--- expected
+++ got
-line: {: dir1/}{}
+line: {: dir\`echo\ yes\ \>\ called\` }{}
 QUERY:{dir}
 DESCRIPTION:{file}
+C1:{dir`echo yes > called`}
 C1:{dir1/}
 C1:{dir2/}
-C1:{dir`echo yes > called`}

Under en_US.UTF-8, the backtick filename sorts before dir1/ and dir2/, while the test expects it to sort after them. On your machine the test passes because your locale happens to produce the same order as C collation.

The right fix is not to force LC_ALL=C in the production code—you are correct that fzf-tab should respect the user's locale. The appropriate fix is to set LC_ALL=C inside the test itself, so that the test produces a deterministic, locale-independent result regardless of who runs it. This is standard practice for test suites that involve sorting.

This way the production behavior is untouched, but other developers running the tests locally with different locales won't hit spurious failures. Such a fix would potentially save hours of debugging for future developers and unnecessary regression to LC_ALL into the production code in the future.

@Aloxaf Aloxaf merged commit 9fa8364 into master Mar 14, 2026
10 checks passed
@Aloxaf
Copy link
Copy Markdown
Owner Author

Aloxaf commented Mar 14, 2026

For veteran power users who prefer the current behavior (first view shows all, with the first group visible at the top), we can add an option to hide the All group and open directly with all options shown. Or the other way around, we can leave the current behavior as the default one, but offer an option to users who like to have a All group for a simplified mental model / practicality when fuzzy-searching.

A hidden "All" group is a good idea, I agree with this approach. It can be treated as an "unspecified group" state, which would also mean we don't need to explicitly display an extra "All" group (and worry about what color it should be, haha)

The appropriate fix is to set LC_ALL=C inside the test itself, so that the test produces a deterministic, locale-independent result regardless of who runs it. This is standard practice for test suites that involve sorting.

You're right, but I can't reproduce this issue locally. Welcome to submit a PR.

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.

3 participants