-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(zsh): Handle multi-line history selection #4595
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
feat(zsh): Handle multi-line history selection #4595
Conversation
4400e59 to
0f1a086
Compare
LangLangBart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junegunn Annotations were added to clarify the terse Zsh syntax and ease the burden of reviewing.
2823e9a to
dbfef1b
Compare
|
@drelephant Can you test this and see if it works as expected? You can clone this repo, |
style: use long multi flag
for example a single selection: 20194* ls
only useful when =~ operator is being used
1b247b9 to
72aa071
Compare
|
rebased onto master and triggered the CI again; tests now pass, whereas previously it failed:
EDIT1 |
When Perl is not available on the system, fallback to using awk
normally "emulate -L zsh" would take care of it, but bug: junegunn@3a6af27586c6#commitcomment-21136102
Zsh-specific quirks can cause false positives, not the repository's code
1ff419d to
829458a
Compare
|
Sorry, I haven't had the bandwidth to review this yet. I just requested Copilot's review, let's see if it can provide meaningful insights. But feel free to dismiss any comments from it that you consider irrelevant or inaccurate. |
There was a problem hiding this 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 adds multi-line history selection support to zsh's fzf integration, allowing users to select and insert multiple history entries (including multi-line commands) at once, similar to the fish shell implementation in PR #4280.
Key Changes:
- Modified the fzf-history-widget to support multi-select mode (
--multiflag) instead of single selection - Added logic to process multiple selected history entries and join them with newlines
- Implemented handling for both perl-based and awk-based history extraction paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| shell/key-bindings.zsh | Enhanced fzf-history-widget to enable multi-selection, extract multiple history entries, and handle both perl and awk code paths with proper multi-line command preservation |
| test/test_shell_integration.rb | Added comprehensive test coverage including a helper function to test both perl and awk branches, with tests for multi-selection, single-line mode, and edge cases like multi-line entries with leading numbers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shell/key-bindings.zsh
Outdated
| # Join by newline after stripping trailing whitespace from each command | ||
| BUFFER="${(pj:\n:)${(@)cmds%%[[:space:]]#}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering. If I'm not mistaken, in the previous version, this code would only strip trailing new lines, but now it strips [[:space:]]. Could you elaborate on the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During testing, a command had a trailing newline & space character, and since the
original command only removed trailing newlines, it failed to remove anything. For example:
zsh -o extendedglob -fc $'
ORIGINAL="1 foo\nbar\n "
O_WSPACE="${ORIGINAL%%[[:space:]]#}"
O_NEWLIN="${ORIGINAL%%\n#}"
typeset -p ORIGINAL O_WSPACE O_NEWLIN'typeset ORIGINAL=$'1 foo\nbar\n '
typeset O_WSPACE=$'1 foo\nbar'
typeset O_NEWLIN=$'1 foo\nbar\n 'My thinking was nobody wants trailing whitespace on a command, right 😬 ?
(Though there is a chance a user may complain, as it affects their workflow – https://www.explainxkcd.com/wiki/index.php/1172:_Workflow.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was nobody wants trailing whitespace on a command, right 😬 ?
Yeah. One concern is that when:
- User accidentally types in a command with extra spaces.
- User wants to repeat the command, so they use CTRL-R to select the previous command, and press enter.
- Now when they press CTRL-R again, they see two duplicate commands in the list, only differ in the trailing spaces.
100 whoami<invisible spaces>
101 whoami
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was able to reproduce what you described. I am using
'romkatv/zsh4humans', and it has a mechanism1 built in by default that
automatically removes trailing spaces from a command when it is executed, that
is why it took me a bit to reproduce your issue and find out why its not
behaving the same way as you described and only did in 'zsh -f' mode.
Anyway, are you suggesting it’s better to keep the original trailing‑newline
removal?
Because even when reverting back, your original issue still remains: a user sees
two commands.
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, are you suggesting it’s better to keep the original trailing‑newline removal?
Hmm, yeah, I think it's preferable that we don't modify the original command.
Because even when reverting back, your original issue still remains: a user sees two commands.
I didn't test your previous version. I might be missing something, but on zsh, when I repeat the same command multiple times, there's no duplication in the list. Are we on the same page?
$ foo<space><space>
$ foo<space><space>
$ foo<space><space>
# CTRL-R shows only one "foo<space><space>"
$ foo<space>
$ foo
# CTRL-R shows "foo<space><space>", "foo<space>", and "foo"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we on the same page?
Yes, we are. When viewing these commands, they appear to be the same, with the trailing space being the only difference.
Hmm, yeah, I think it's preferable that we don't modify the original command.
ok, will adjust that, and adopt test BUFFER="${(pj:\n:)${(@)cmds%%$'\n'#}}"
junegunn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your hard work and thanks for new tests. Is there anything you want to address?
There is the comment by Copilot. When entering Status quo with the latest release (0.67.0), both Perl and Awk Do you think the I should make the heuristic stricter? At least for Perl it --- a/shell/key-bindings.zsh
+++ b/shell/key-bindings.zsh
@@ -151,5 +151,6 @@ fzf-history-widget() {
if [ -n "$selected" ]; then
# Heuristic to check if the selected value is from history or a custom query
- if [[ $selected == [[:blank:]]#<->\*#[[:blank:]]* ]]; then
+ if (( extracted_with_perl )) && [[ $selected == <->$'\t'* ]] || \
+ (( ! extracted_with_perl )) && [[ $selected == [[:blank:]]#<->\*#[[:blank:]]* ]]; then
# Split at newlines
for line in ${(ps:\n:)selected}; doOf course I could then also add a proper test, which only Perl would actually |
That is unfortunate.
I think it's understandable we can't "perfect" this. fzf can be configured in numerous ways and it's unrealistic for us to make these scripts compatible with every possible setup. That said, if the added complexity is not too significant, we can try to cover more cases. I'll leave the decision up to you. |
Ok, I think we can make the heuristic a bit stricter for Perl/$history, and technically for Awk/fc as well, since by default the history event number (using fc) and the actual command are separated by two spaces or a I will work on that and let you know later today, so that we can also cover the Footnotes |
I did that (using I was previously using the code from here in my Also, before I took that code out of the
Then the command line will now have "vivi file" (ie it kept what was on the line before entering fzf. I don't think it did that before updating to the latest commit? Anyway, I'm on the latest commit now without anything extra in my And I can't multi-select using tab key. |
If you can't build the binary from source, you can just source the script files from your zsh. |
should be fixed in this version; the one from the discussion used the LBUFFER assignment instead of BUFFER
thanks @junegunn @drelephant If you still have trouble testing, one way would be:
I think I am done. updated the test, checked older zsh versions with the command below:
# Requires fzf repo directory and 'zsh-hist-multi-select' branch checked out
# https://hub.docker.com/r/ohmyzsh/zsh/tags
podman run -qit --rm -e ARCH=$(uname -m) -v $PWD:/repo -w /repo ohmyzsh/zsh:5.9 sh -uec '
apt update >/dev/null 2>&1
apt install -y curl jq >/dev/null 2>&1
mkdir /fzf-bin
download_url=$(curl -s https://api.github.com/repos/junegunn/fzf/releases/latest | \
jq --arg arch_string "linux_$ARCH" \
-r '\''.assets[].browser_download_url | select(contains($arch_string))'\'')
curl -sfLo /tmp/fzf.tar.gz "$download_url"
tar -xf /tmp/fzf.tar.gz -C /fzf-bin
{
echo "export PATH=\"/fzf-bin:\$PATH\""
echo "source /repo/shell/key-bindings.zsh"
echo "PS1=\"FZF \$(fzf --version) > \""
echo "RPS1=\"ZSH \$(zsh --version)\""
echo "print -s -- \"ls -la\""
echo "print -s -- \"cat /etc/os-release\""
} > ~/.zshrc
zsh' |
|
Merged, thanks a ton for the great work! |
I just did a So I'm on the latest now. Then And I couldn't multi-select, but then I did I can add that line to my |
|
|
Related discussion #4591
@drelephant
Related PR with implementation in fish: #4280
Add the ability to select multiple zsh history entries