Breaking Change in Keybinding Behavior due to Commit 6b4358f

LangLangBart commented 1 month ago

Checklist

[X] I have read through the manual page (man fzf)
[X] I have searched through the existing issues
[X] For bug reports, I have checked if the bug is reproducible in the latest version of fzf

Output of `fzf --version`

0.52.1 (7aa88aa1)

OS

[ ] Linux
[X] macOS
[ ] Windows
[ ] Etc.

Shell

[ ] bash
[X] zsh
[ ] fish

Problem / Steps to reproduce

Issue

When executing the command below and pressing ⌃ Control + Y, I observe a different result since commit 6b4358f (Ref: #3810).

# Current situation with 'devel' branch
seq 10 | fzf --bind 'ctrl-y:toggle+down' --expect 'ctrl-y' --bind 'load:pos:4'
# ctrl-y
# 3

# Old behavior prior to 6b4358f
seq 10 | fzf --bind 'ctrl-y:toggle+down' --expect 'ctrl-y' --bind 'load:pos:4'
# ctrl-y
# 4

This seems like a breaking change. If a user has set some custom configuration in their FZF_DEFAULT_OPTS and runs a script where a user uses this keybind with --expect, it will cause unintended issues.

Background Info

In my situation, I had a list of PRs and set ctrl-y in my FZF_DEFAULT_OPTS to perform a toggle-down operation. In the script, the ctrl-y key was configured with --expect to checkout the selected PR. This has led to the wrong PR being checked out every time.

Unsetting FZF_DEFAULT_OPTS in the script serves as a solution, however, it results in the loss of numerous custom configurations set by the user. What is the best course of action here ?

junegunn commented 1 month ago

I see, that's unfortunate.

--expect hasn't been compatible with --bind for a very long time, so while if it's conceptually right to make them compatible, some users may have come to rely on the current behavior.

So since we have the new print(...) action, how about if we

Officially declare that --expect is not compatible with --bind, and that it supersedes --bind for the same key when both are used. (Or maybe we can take "last one wins" strategy, e.g. --expect ctrl-y --bind ctrl-y:up)

And suggest using print(...) when they need to combine the two.

fzf --bind 'enter:print()+accept,ctrl-y:print(ctrl-y)+accept'

/cc @ibhagwan

ibhagwan commented 1 month ago

@junegunn

While I find it logically sound to have expect compatible with bind, I sympathize with the will to avoid breaking changes, especially for behaviors that have been relied upon for a long time, whatever you decide will be lovingly accepted.

junegunn / fzf