Skip to content
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

Add use-fzf-default-opts #479

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

PrayagS
Copy link
Contributor

@PrayagS PrayagS commented Oct 6, 2024

Ref: #475

0b49f3e introduces a change that makes the plugin ignore FZF_DEFAULT_OPTS by default.

This commit adds a flag to disable this behavior.

Ref: Aloxaf#475

Aloxaf@0b49f3e introduces a change that makes the plugin ignore
`FZF_DEFAULT_OPTS` by default.

This commit adds a flag to disable this behavior.

Signed-off-by: PrayagS <[email protected]>
FZF_DEFAULT_OPTS='' SHELL=$ZSH_NAME $fzf_command \
local fzf_default_opts=''
if [[ "$use_fzf_default_opts" == "yes" ]]; then
fzf_default_opts=$FZF_DEFAULT_OPTS
Copy link

Choose a reason for hiding this comment

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

I was wondering if fzf_default_opts couldn't be set outside like this:
zstyle ':fzf-tab:*' fzf-default-opts $FZF_DEFAULT_OPTS
?

I that case you could get ride off the use_fzf_default_opts option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was going for a quick change so didn't try that. I thought zstyle might parse things differently and lead to unintended behaviors.

Let me try that and get back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I remember it now. Since FZF_DEFAULT_OPTS is a bunch of flags, we need to pass it as such and not as an environment variable.

And fzf-flags already exists in the plugin. But when I tried setting that equal to the flags in FZF_DEFAULT_OPTS (see below), it wasn't working. I think the reason is the same as what I mentioned in #475 (comment).

# this converts the string to a string array which is expected by fzf-flags.
export parsed_fzf_default_opts=(${=FZF_DEFAULT_OPTS})

zstyle ':fzf-tab:*' fzf-flags $parsed_fzf_default_opts

Copy link

Choose a reason for hiding this comment

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

Your suggestion doesn't seem to be working for my case:

export FZF_DEFAULT_OPTS="          \
  --exact                          \
  --height ${FZF_TMUX_HEIGHT:-30%} \
  --layout=reverse                 \
  --info=hidden                    \
  --prompt='$  '                   \
  --pointer='=>'                   \
  --marker='+'                     \
  --cycle                          \
  --select-1                       \
  --color=dark                     \
  --bind '?:toggle-preview,ctrl-a:select-all,ctrl-d:half-page-down,ctrl-u:half-page-up,ctrl-l:clear-query,home:first,end:last,enter:accept-non-empty,alt-down:down,alt-up:up,right:accept-non-empty,alt-right:accept-non-empty,alt-left:close,left:close' \
  --color='fg:-1,bg:-1,hl:#fb8aa4,fg+:-1,bg+:-1,hl+:#55E579' \
  --color='info:#af87ff,prompt:#fb8aa4,pointer:#55E579,marker:#55E579,spinner:#55E579'
"

I think the --height ${FZF_TMUX_HEIGHT:-30%} is not correctly handle.

Copy link
Owner

@Aloxaf Aloxaf Oct 10, 2024

Choose a reason for hiding this comment

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

@pablospe can you try this?

zstyle ':fzf-tab:*' fzf-flags ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}}

Choose a reason for hiding this comment

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

This is magic! It is working, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are my flags,

# Set fzf options
export FZF_DEFAULT_OPTS=$FZF_DEFAULT_OPTS'
    --color=bg:#1b1b1b
    --color=preview-bg:#1b1b1b
    --color=hl+:#fb4934
    --color=gutter:#1b1b1b
    --color=pointer:#f9f5d7,marker:#f9f5d7
    --color=info:#ebdbb2,spinner:#f9f5d7
    --color=query:#ebdbb2,prompt:#ebdbb2
    --color=border:#ebdbb2
    --border="rounded" --prompt="> "
    --marker="" --pointer="->" --layout="reverse"
'

While echo ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}} looks as expected, when I try to open fzf-tab, it fails with an eval error.

@pablospe
Copy link

pablospe commented Oct 8, 2024

Adding @Aloxaf for awareness. I think it is good to add an option so the user can decide about this behavior but taking care of leaving a safe default, as this PR proposes.

README.md Outdated
zstyle ':fzf-tab:*' fzf-flags --color=fg:1,fg+:2 --bind=tab:accept
# To make fzf-tab follow FZF_DEFAULT_OPTS
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should warn user that this may cause unexpected behavior.

Copy link

@pablospe pablospe Oct 10, 2024

Choose a reason for hiding this comment

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

@PrayagS I think you can mention here there are other plugins that overrides the value of fzf-flags directly, creating bugs.

Now I was wondering if overriding the fzf-flags is the right thing to do for an external plugin, it should do something like: fzf-flags = fzf-flags + <extra_things>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Aloxaf Updated the README. Please review 380daee (#479).

@pablospe My comment on Freed-Wu/fzf-tab-source causing a conflict is a hunch. Because when I was reviewing its code, it overrides it specifically for a completion type (see https://github.com/Freed-Wu/fzf-tab-source/blob/aabde06d1e82b839a350a8a1f5f5df3d069748fc/fzf-tab-source.plugin.zsh#L31). The more likely reason for it to not work could be just a parsing issue with the flags.

@Aloxaf
Copy link
Owner

Aloxaf commented Oct 10, 2024

Umm, quite reasonable.

zstyle ':fzf-tab:*' fzf-flags ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}} or zstyle ':fzf-tab:*' use-fzf-default-opts yes, which one do you prefer?

@pablospe
Copy link

pablospe commented Oct 10, 2024

Umm, quite reasonable.

zstyle ':fzf-tab:*' fzf-flags ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}} or zstyle ':fzf-tab:*' use-fzf-default-opts yes, which one do you prefer?

I don't have a strong preference:
zstyle ':fzf-tab:*' fzf-flags ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}}
doesn't touch the code, only the documentation. Although, it is more complicated to understand, and my guess it is more susceptible to errors (maybe something in the FZF_DEFAULT_OPTS will break the parsing). In the other hand, the 2nd option seems more elegant, there is no parsing and self-explanatory: use-fzf-default-opts yes. Notice that implementing the 2nd solution still allows to do use the first one.

@PrayagS
Copy link
Contributor Author

PrayagS commented Oct 10, 2024

Umm, quite reasonable.

zstyle ':fzf-tab:*' fzf-flags ${(Q)${(Z:nC:)FZF_DEFAULT_OPTS}} or zstyle ':fzf-tab:*' use-fzf-default-opts yes, which one do you prefer?

In reference to #479 (comment), if the solution that you shared is dependent on how flags are written and parsed, it is highly likely to break frequently as more users start using it. It could also break when fzf adds some new flag.

So I think adding an explicit setting sounds more reasonable.

@PrayagS PrayagS force-pushed the add-flag-to-follow-fzf-default-opts branch 2 times, most recently from de9f287 to 1ef7832 Compare October 10, 2024 08:15
@PrayagS PrayagS force-pushed the add-flag-to-follow-fzf-default-opts branch from 1ef7832 to 380daee Compare October 10, 2024 08:16
@Aloxaf Aloxaf merged commit b6e1b22 into Aloxaf:master Oct 11, 2024
10 checks passed
PrayagS added a commit to PrayagS/dotfiles that referenced this pull request Oct 12, 2024
Upstream PR was merged. Ref: Aloxaf/fzf-tab#479

Signed-off-by: PrayagS <[email protected]>
kevinhughes27 added a commit to kevinhughes27/dotfiles that referenced this pull request Jan 3, 2025
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