omerxx / tmux-sessionx

A Tmux session manager, with preview, fuzzy finding, and MORE
GNU General Public License v3.0
556 stars 51 forks source link

Add fzf options #59

Closed ttdotsh closed 3 months ago

ttdotsh commented 4 months ago

This PR adds a binding to be able to pass in FZF options, addressing #54. I put them first in the list of arguments so that any options that are core to the plugin functionality or already exist as separate bindings will override user-provided options, preventing the user from breaking the plugin.

ttdotsh commented 3 months ago

I am using this setup locally and things are working as intended, however I do find that passing in --bind <keybind>:<command> args appears to break the plugin where the popup flashes briefly and then disappears, similar to what is described in #22, #46, and #58

My suspicion is that the arguments being passed in as string values are not being properly evaluated. I'll be looking at tmux-fzf-url as inspiration for this since the works properly in that plugin.

ttdotsh commented 3 months ago

I am using this setup locally and things are working as intended, however I do find that passing in --bind <keybind>:<command> args appears to break the plugin where the popup flashes briefly and then disappears, similar to what is described in #22, #46, and #58

My suspicion is that the arguments being passed in as string values are not being properly evaluated. I'll be looking at tmux-fzf-url as inspiration for this since the works properly in that plugin.

Okay, I have spent probably too long on this. The problem is indeed that the fzf options passed to the plugin as a string are being parsed as fzf-tmux "--bind '<some-binding>'" rather than the desirable fzf-tmux --bind '<some-binding>'. In tmux-fzf-url this is solved by using the eval command.

For some reason this plugin does not work when either the fzf-tmux command is either called in eval like so:

    RESULT=$(eval "fzf-tmux $bind_fzf_options $args[@]" <<< ${INPUT// /})

or if it's given arguments that are parsed from a string in eval like so:

    bind_fzf_options=$(tmux_option_or_fallback "@sessionx-fzf-options" "--color pointer:9,spinner:92,marker:46")
    eval "fzf_opts=($bind_fzf_options)"
    ...
    RESULT=$(echo -e "${INPUT// /}" | fzf-tmux "$fzf_opts[@] $args[@]")

I have also tried things like:

args=(
    ...
)

eval "fzf_opts=($bind_fzf_options)"

combined=()
combined+=$fzf_opts
combined+=$args

RESULT=$(echo -e "${INPUT// /}" | fzf-tmux "${combined[@]}")

All of these yielded the same result. The kicker is that all of these options worked just fine for me running fzf-tmux directly in my terminal. I am not sure what could be happening inside the script that would so drastically change the functionality. Wondering if there is something obvious I am missing here (bash isn't my strong suit).

ProgmRuanSilva commented 3 months ago

Hi @ttdotsh thanks for this pull request, and good idea.

Can you push these alterations that you have locally?

I'll take a look tonight

ttdotsh commented 3 months ago

Thanks @ProgmRuanSilva! I've done that

ProgmRuanSilva commented 3 months ago

💾 Changes

  1. The evaluation process
  2. Change the additional_fzf_options to the inside of handle_binds()
  3. Format code, I'll format all file on the next PR

@ttdotsh I've made some changes, check if works on your machine to be able to merge

ttdotsh commented 3 months ago

💾 Changes

  1. The evaluation process
  2. Change the additional_fzf_options to the inside of handle_binds()
  3. Format code, I'll format all file on the next PR

@ttdotsh I've made some changes, check if works on your machine to be able to merge

I just gave it a shot and this works great! Thanks for taking a look at this @ProgmRuanSilva

ProgmRuanSilva commented 3 months ago

@ttdotsh can I merge?

ttdotsh commented 3 months ago

@ttdotsh can I merge?

All good on my end, assuming you're good with the default values as is! Happy to change them to empty if that's your preference.

ProgmRuanSilva commented 3 months ago

@ttdotsh can I merge?

All good on my end, assuming you're good with the default values as is! Happy to change them to empty if that's your preference.

I've changed it, my fear is the other users

Thanks for the contribution @ttdotsh (and sorry for replying too late)

ttdotsh commented 3 months ago

@ttdotsh can I merge?

All good on my end, assuming you're good with the default values as is! Happy to change them to empty if that's your preference.

I've changed it, my fear is the other users

Thanks for the contribution @ttdotsh (and sorry for replying too late)

Appreciate the help debugging the issue I had! @ProgmRuanSilva