-
Notifications
You must be signed in to change notification settings - Fork 137
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: Use fzf-tmux if fzf options are set #304
Conversation
I can address the lint failures when I’m back on a keyboard. |
Sent up a fixup commit to address the shellcheck lints, I'll squash it down before merge once approved :) |
Don't stress about squashing, we use "squash and merge" I'll hopefully get to test out and review soon! I've never heard about tmux-fzf but it sounds cool. Thanks for the contribution, we will be back to you with comments/approval etc when we can 😀 |
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.
Hey @dljsjr, thanks for your patch! I added some feedback to certain lines.
@carlfriedrich I made some adjustments: |
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.
Great, thanks for your effort. Just two more small change requests from my side.
@carlfriedrich changes made and branch is rebased on top of latest upstream |
The meat of this change is to inspect whether or not we are in a `tmux` pane, and if so, we check the `FZF_TMUX` and `FZF_TMUX_OPTS` variables. If those are set, we use `fzf-tmux` instead of `fzf` and we forward the appropriate options. - Add a `do_fzf` function for abstracting fzf invocation based on shell state as outlined above. - Add a new variable, FORGIT_SHOW_FZF_ON_EMPTY_LIST, for controlling when `--exit-0` is forwarded to `fzf`. - Replace all invocations of `fzf` with `do_fzf`.
Sure. Let me test with and without this plugin and make sure it’s all good! I’ll do that today, and merge if everything looks good.
…On Wed, May 10, 2023 at 11:21 PM, Tim ***@***.***(mailto:On Wed, May 10, 2023 at 11:21 PM, Tim <<a href=)> wrote:
***@***.***(https://github.com/dljsjr) Great, looks good code-wise for me now. Thanks a lot for your contribution!
I Did not do any functional tests, though, since I do not use tmux.
***@***.***(https://github.com/cjappl) Do you want to do any more testing or review? Otherwise I will merge this shortly.
—
Reply to this email directly, [view it on GitHub](#304 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY5ICKMFCCLGEYRRLV3XFSANTANCNFSM6AAAAAAXXPR2WM).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
So far most commands are working well! However Steps to repro:
This does not happen without step 3 (turning on this new FZF_TMUX). I won't be able to fully dig in today to triage. Let me know if you have any "aha's" and I'm happy to test again @dljsjr |
@cjappl Hmm, interesting. I didn't modify any of that code, and I don't use
|
@cjappl do you have anything in your I think what's going on here is that the spawned UpdateThis is an oddity with Any new panes/windows/splits/popups started inside of a Proposed SolutionUse |
@cjappl I've made the proposed change only for |
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.
Blocking merge for now until fish issues resolved
@cjappl Guess this is my excuse to give |
Haha! It’s a blessing and a curse :)
Appreciate you being willing to chase it down. Shout if you need a test subject, happy to run anything you come up with.
…On Mon, May 15, 2023 at 9:16 AM, Douglas Stephen ***@***.***(mailto:On Mon, May 15, 2023 at 9:16 AM, Douglas Stephen <<a href=)> wrote:
***@***.***(https://github.com/cjappl) Guess this is my excuse to give fish a try for the first time in like, 8 years :)
—
Reply to this email directly, [view it on GitHub](#304 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXYY5DNXLWGXLCLF7Z63XGJJFZANCNFSM6AAAAAAXXPR2WM).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@dljsjr Shall we reopen this? What are the chances that you'll get back to this? |
Check list
Description
Overview
The meat of this change is to inspect whether or not we are in a
tmux
pane, and if so, we check theFZF_TMUX
andFZF_TMUX_OPTS
variables. If those are set, we usefzf-tmux
instead offzf
and we forward the appropriate options.This resolves #303
Changes
Add a
do_fzf
function for abstracting fzf invocation based on shell state as outlined aboveOnly hardcode
-0
/--exit-0
forfzf
, as it causes flicker w/ tmux popupReplace all invocations of
fzf
withdo_fzf
Type of change
Test environment