style: restore the default group switching behavior#567
Conversation
There was a problem hiding this comment.
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.
|
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 PR #567 as written introduces a few regressions I'd like to flag:
Regarding PR #566 reverting the I'm happy to open a clean PR that fixes QuarticCat's issue without reverting the other changes. Would that be welcome? |
|
One more regression I forgot to mention: the active group header styling is now visually broken. The previous code used line=${line/$current_prefix(#b)([^]]##)\]/$current_prefix$sgr_on${match[1]}]$sgr_off}The new one-liner places 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 One more regression I forgot to mention: the active group header styling is now visually broken. The previous code used line=${line/$current_prefix(#b)([^]]##)\]/$current_prefix$sgr_on${match[1]}]$sgr_off}The new one-liner places 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 |
|
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. The old code kept 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. ScreenshotsInitial state — all groups in their natural colors, no active style applied: After first group switch — After second group switch — Reaching Reaching |
|
@alberti42 Thanks for your review. I've updated the code.
|
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. |
|
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. |
|
Hi Aloxaf, thank you for the quick update — the latest revision addresses the issues I raised:
Regarding the More importantly, On the implementation side, an explicit 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 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? |
Regarding The reason I had introduced I just re-ran the tests on my machine ( Under The right fix is not to force 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. |
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)
You're right, but I can't reproduce this issue locally. Welcome to submit a PR. |






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.