andreyorst / fzf.kak

FZF for Kakoune
MIT License
143 stars 33 forks source link

fzf window closes immediatly on x11 #15

Closed TeddyDD closed 6 years ago

TeddyDD commented 6 years ago

Problem description

Running any file related command from fzf mode results in new terminal window blinking for a split of a second. It closes immediately after that. Info box stays open. I did bisect on changes since v0.1.2 and first bad commit is 8ae03b154e3043d8b542672201ba482091b6d61d

Buffer selector, change directory and buffer search works fine.

Steps to reproduce

  1. git checkout 8ae03b154e3043d8b542672201ba482091b6d61d
  2. run kak outide of tmux
  3. run any fzf-mode command

Environment information

Kakoune version: v2018.09.04 OS version: Void 4.18.13_1 x86_64 GenuineIntel uptodate rFFFFFFFF sh executable version: dash-0.5.10.2 fzf version: 0.17.5

Edit:

It seems that fzf_preview was true by default, disabling it fixes the problem.

Okay, I don't have any compatible highlighter installed so it fails because fzf_preview is true by default.

andreyorst commented 6 years ago

Hm. That's weird, because I shouldn't fail if no highlighter is installed. Because it fallbacks to cat. I have no highlighter in my Termux instance, and it works fine there, though inside tmux. I'll investigate it

andreyorst commented 6 years ago

@TeddyDD does fzf-buffer work? It uses different implementation. If It isn't working too, there's nothing to do with preview. Also please check fzf-cd

TeddyDD commented 6 years ago

buffer and cd works. I checked if this might be terminal related but both kitty and xst gives me the same result

andreyorst commented 6 years ago

I've found the issue. Please try with latest master

andreyorst commented 6 years ago

No. Actually not. The problem is that the size calculation isn't happening.

andreyorst commented 6 years ago

I've pushed modified version, in which $pos variable contains default value. Please try to launch fzf with it

andreyorst commented 6 years ago

I've pushed fix_preview branch, can you please try it? It works in my setup, I even symlinked sh to dash. Though It worked for me whole time, so my configuration can't be referenced.

TeddyDD commented 6 years ago

It doesn't close terminal window so you probably fixed it :)

Problem is apparently fzf uses $SHELL in preview command so preview it's broken on Fish shell. I fixed it with this patch

diff --git i/rc/fzf.kak w/rc/fzf.kak
index a038c9..ec160c 100644
--- i/rc/fzf.kak
+++ w/rc/fzf.kak
@@ -334,7 +334,7 @@ define-command -hidden fzf -params 2..3 %{ evaluate-commands %sh{
     elif [ ! -z "${kak_opt_termcmd}" ]; then
         path=$(pwd)
         additional_flags=$(echo $additional_flags | sed "s:\$pos:\\\\\$pos:")
-        cmd="$kak_opt_termcmd \"sh -c \\\"cd $path && $preview_pos $items_command | fzf --expect ctrl-q $additional_flags > $tmp\\\"\""
+        cmd="$kak_opt_termcmd \"sh -c \\\"SHELL=/bin/sh; export SHELL; cd $path && $preview_pos $items_command | fzf --expect ctrl-q $additional_flags > $tmp\\\"\""
     else
         echo "fail termcmd option is not set"
         exit

Not sure if this patch is correct way to do this, but after applying it, it works for me.

andreyorst commented 6 years ago

/bin/sh is not always there. For example in Termux there's no /bin/sh

andreyorst commented 6 years ago

@TeddyDD can you please try the latest fix_preview branch?

TeddyDD commented 6 years ago

It doesn't work, terminal shows up for split of a second and closes.

andreyorst commented 6 years ago

Weird. It works for me. I suspect that we have single thing that doesn't work on your setup and works on my. I'll try to use fish

TeddyDD commented 6 years ago

I tried to run Kakoune with bash and sh on fix_preview branch. I'm getting invalid preview window layout: for a second and terminal closes

andreyorst commented 6 years ago

Thats exactly what I've been fixing all the time, and strangely enough it works for me

andreyorst commented 6 years ago

fish doesn't support $(...)... mother of god why...

TeddyDD commented 6 years ago

yeah, but that shoudn't matter since Kakoune uses posix shell in expansions, right?

andreyorst commented 6 years ago

yeah, but I pass shell command to terminal via $termcmd -e, then I pass it to sh -c to actually do the thing. This way it should be more portable, since we always use sh. Now I try to debug this 3 screen line command in fish, and the syntax is wrong in the places where I least expect it to be.

andreyorst commented 6 years ago

@TeddyDD with latest changes, still doesn't work?

TeddyDD commented 6 years ago

um, you haven't pushed anything new to fix_preview

andreyorst commented 6 years ago

Oh. Sorry. I didn't noticed that my connection was off. I've pushed the changes

TeddyDD commented 6 years ago

nope, still blinking terminal

andreyorst commented 6 years ago

Can I ask you to try with another login shell? Althoughit works for me in KDE and Mate, with bash, zsh, and fish as a login shells...

TeddyDD commented 6 years ago

sure, I'll try and report back

andreyorst commented 6 years ago

I've installed kitty, and now it isn't working for me too. I'm testing this mainly with termite. So I thought that other terminals should work. Turns out that that's not always true. Mate terminal brokes too.

TeddyDD commented 6 years ago

I tried with bash as login shell, it doesn't help. I tried it under st and kitty

andreyorst commented 6 years ago

I've noticed that this doesn't work with kitty, and alacritty when they set as termcmd. If I change it, it works. Try changing termcmd to some common terminal, like gnome terminal? Maybe it is the issue with kitty, as it need some additional argument. Currently, setting termcmd to termite -e gnome-terminal -e and mate-terminal -e makes it work.

andreyorst commented 6 years ago

I see. when alacritty installed termcmd equals to alacritty -e sh -c when kitty installed termcmd equals to kitty sh -c I use termite, and with it termcmd equals to termite -e

Seems likse -e isn't compatible with sh -c in some ways.

andreyorst commented 6 years ago

Ok now I know where to dig. Expect a fix in couple of hours :)

andreyorst commented 6 years ago

@TeddyDD should work now. I've tested with all possible at the moment terminals

TeddyDD commented 6 years ago

it works, awesome job!

andreyorst commented 6 years ago

awesome job

issue solved exactly in 30 comments.

I'll merge this into master now