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

Fish Shell breaks Kitty image previews with ranger #6994

Closed
purxiz opened this issue May 11, 2020 · 28 comments
Closed

Fish Shell breaks Kitty image previews with ranger #6994

purxiz opened this issue May 11, 2020 · 28 comments
Labels
bug Something that's not working as intended
Milestone

Comments

@purxiz
Copy link

purxiz commented May 11, 2020

Please tell us which fish version you are using by executing the following:
fish, version 3.1.1

Please tell us which operating system and terminal you are using. The output of uname -a and echo $TERM may be helpful in this regard although other commands might be relevant in your specific situation.

Ubuntu 20.04 with Kitty Terminal

Please tell us if you tried fish without third-party customizations by executing this command and whether it affected the behavior you are reporting:

sh -c 'env HOME=$(mktemp -d) fish'

Occurs on a fresh install of fish in a VM, with the only 3rd party program being ranger's rc.conf to enable image previews. This bug is documented on the ranger page here, but I believe it to be an error(?) in fish shell, rather than with ranger:

ranger/ranger#1919

Steps to reproduce are install ranger, enable image previews and set image_preview_method=kitty.

Image previews work properly in BASH, but in FISH, only a portion of the image is shown. The image is not resized to fit the pane, and the ranger panes are no longer drawn while the portion of the image is being previewed.
I have tried downgrading to 3.1.0-1.2 (the only other version available in apt for Ubuntu 20.04), and the issue persists, though slightly differently. The picture is still rendered incorrectly, with only a portion of it being drawn, but only the text in the ranger panes disappear. The outlines remain.

I also tried with version 3.1.2, issue still persists.

Here is a picture of the issue:
image

The expected behavior is that I can still see the bounding boxes on ranger, as well as the contents, and the image is resized to fit into the right pane. Here is an image just before the image preview finishes loading. Once it is displayed, the terminal looks like above.
image

These images were produced on fish 3.1.2.

Here is the expected behavior:
image

@ammgws
Copy link
Contributor

ammgws commented May 11, 2020

I get this from time to time as well, didn't think it might be due to fish.

@purxiz
Copy link
Author

purxiz commented May 12, 2020

@ammgws for me it's working with bash, if you get a chance, it would be useful to test on fish 3.0.2, as the other thread claims that it's not broken in that version.

@zanchey zanchey added the bug Something that's not working as intended label May 14, 2020
@zanchey zanchey added this to the fish-future milestone May 14, 2020
@zanchey
Copy link
Member

zanchey commented May 14, 2020

Marking this as a bug though I think it needs more characterisation; I can't get kitty to run on my machine for whatever reason.

@purxiz
Copy link
Author

purxiz commented May 14, 2020

@zanchey I'm happy to provide whatever test output or additional info is relevant/needed. I updated the post with some images to describe the problem, but I really don't know much if anything about how shell programming works, so I'm not sure about a solution at all.

@faho
Copy link
Member

faho commented May 14, 2020

I've managed to get the previews to work, but I can't see a difference between fish and bash here. Both resize the image in what seems to be an aspect-ratio preserving manner?

The obvious thing to check are the environment variables (e.g. is there a difference in $PATH, so that this executes a different thing in the background). It is also possible fish turns something on in kitty or prints something at some point.

@purxiz
Copy link
Author

purxiz commented May 14, 2020

@faho Did you set preview_images_method = kitty in your ranger.conf? For me, w3m (the default) works with both, but kitty does not work with fish while bash does, path's identical, empty kitty.conf and empty fish config.

@ammgws
Copy link
Contributor

ammgws commented May 14, 2020

I'm getting the same behaviour in bash as well, however I'm running bash from fish so it might be due to something in my environment.

@faho
Copy link
Member

faho commented May 14, 2020

Okay, I spoke too soon.

It's only really visible with a large image and I was testing with small ones. The weird part if I launch fish from bash, exit it immediately and then start ranger from bash it reproduces. So there is some state in kitty that gets triggered from fish, and it's not terminal modes (which would be the obvious candidate, but 1. the only difference is ixon and 2. changing it doesn't change anything).

Tbh this seems quite likely to be an issue in kitty at this point.

@callahad
Copy link

I can confirm that Fish 3.0.2 works as expected, and 3.1.0 fails.

There were only 2,210 commits between the two releases, so we should be able to bisect the regression in about a dozen builds.

...not that I'm volunteering. But not that I'm not volunteering, either.

@callahad
Copy link

Git bisect reports that 8031fa3 is the first bad commit.

I manually re-verified that its parent 3d25ce1 works as expected.

@callahad
Copy link

(I do not have the experience or time to effectively pursue this further, but hopefully the bisect is helpful. I'm more than happy to test patches.)

@ammgws
Copy link
Contributor

ammgws commented May 23, 2020

Probably not a coincidence that the code changed in that commit had to do with termsize.

@ridiculousfish does anything look suss here?

@callahad
Copy link

callahad commented May 23, 2020

Looks like an easier way to reproduce this is by passing a large image to Kitty's built-in icat.

Example invocations and screenshots over at ranger/ranger#1919 (comment), but generally speaking, it's kitty +kitten path/to/image.jpg from within an existing Kitty window.

If I run kitty bash, then icat works.
If I run fish from inside of kitty bash, then icat does not work.

If I run kitty fish, then icat does not work.
If I run bash from inside of kitty fish, then icat does not work unless I first resize the window.

So it seems like starting up Fish at all somehow breaks Kitty's sense of the window size, and that breakage persists into Bash subshells, but is temporarily fixed if the window is resized while the Bash subshell is active. Exiting Bash and returning to Fish re-breaks things.

@callahad
Copy link

For a temporary workaround, calling git revert on 277db64 and 8031fa3 yields the following patch against Fish 3.1.2, which I'm happily using locally.

(Click to show patch)
diff --git a/src/common.cpp b/src/common.cpp
index f76445fc0..57d66695a 100644
--- a/src/common.cpp
+++ b/src/common.cpp
@@ -103,7 +103,7 @@ static relaxed_atomic_t<pid_t> initial_fg_process_group{-1};
 /// This struct maintains the current state of the terminal size. It is updated on demand after
 /// receiving a SIGWINCH. Use common_get_width()/common_get_height() to read it lazily.
 static constexpr struct winsize k_invalid_termsize = {USHRT_MAX, USHRT_MAX, USHRT_MAX, USHRT_MAX};
-static owning_lock<struct winsize> s_termsize{k_invalid_termsize};
+static relaxed_atomic_t<struct winsize> s_termsize(k_invalid_termsize);
 
 static relaxed_atomic_bool_t s_termsize_valid{false};
 
@@ -1749,8 +1749,9 @@ bool unescape_string(const wcstring &input, wcstring *output, unescape_flags_t e
 void invalidate_termsize(bool invalidate_vars) {
     s_termsize_valid = false;
     if (invalidate_vars) {
-        auto termsize = s_termsize.acquire();
-        termsize->ws_col = termsize->ws_row = USHRT_MAX;
+        struct winsize termsize = s_termsize;
+        termsize.ws_col = termsize.ws_row = USHRT_MAX;
+        termsize = s_termsize;
     }
 }
 
@@ -1818,42 +1819,27 @@ static void export_new_termsize(struct winsize *new_termsize, env_stack_t &vars)
 #endif
 }
 
-/// Get the current termsize, lazily computing it. Return by reference if it changed.
-static struct winsize get_current_winsize_prim(bool *changed, const environment_t &vars) {
-    auto termsize = s_termsize.acquire();
-    if (s_termsize_valid) return *termsize;
+/// Updates termsize as needed, and returns a copy of the winsize.
+struct winsize get_current_winsize() {
+    struct winsize termsize = s_termsize;
+    if (s_termsize_valid) return termsize;
 
     struct winsize new_termsize = {0, 0, 0, 0};
 #ifdef HAVE_WINSIZE
     errno = 0;
     if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &new_termsize) != -1 &&
-        new_termsize.ws_col == termsize->ws_col && new_termsize.ws_row == termsize->ws_row) {
+        new_termsize.ws_col == termsize.ws_col && new_termsize.ws_row == termsize.ws_row) {
         s_termsize_valid = true;
-        return *termsize;
+        return termsize;
     }
 #endif
+    auto &vars = env_stack_t::globals();
     validate_new_termsize(&new_termsize, vars);
-    termsize->ws_col = new_termsize.ws_col;
-    termsize->ws_row = new_termsize.ws_row;
-    *changed = true;
+    export_new_termsize(&new_termsize, vars);
+    termsize.ws_col = new_termsize.ws_col;
+    termsize.ws_row = new_termsize.ws_row;
+    s_termsize = termsize;
     s_termsize_valid = true;
-    return *termsize;
-}
-
-/// Updates termsize as needed, and returns a copy of the winsize.
-struct winsize get_current_winsize() {
-    bool changed = false;
-    auto &vars = env_stack_t::globals();
-    struct winsize termsize = get_current_winsize_prim(&changed, vars);
-    if (changed) {
-        // TODO: this may call us reentrantly through the environment dispatch mechanism. We need to
-        // rationalize this.
-        export_new_termsize(&termsize, vars);
-        // Hack: due to the dispatch the termsize may have just become invalid. Stomp it back to
-        // valid. What a mess.
-        *s_termsize.acquire() = termsize;
-        s_termsize_valid = true;
-    }
     return termsize;
 }
 
diff --git a/src/common.h b/src/common.h
index d453e4b1d..a83a3de15 100644
--- a/src/common.h
+++ b/src/common.h
@@ -494,7 +494,6 @@ class owning_lock {
 
    public:
     owning_lock(Data &&d) : data(std::move(d)) {}
-    owning_lock(const Data &d) : data(d) {}
     owning_lock() : data() {}
 
     acquired_lock<Data> acquire() { return {lock, &data}; }

@dsanson
Copy link
Contributor

dsanson commented May 25, 2020

Good to see that it isn't just me. An easy way to check for the problem is running:

kitty icat --print-window-size

when things are working properly, you get a reasonable response. But when things are broken, you get:

65535x65535

For an old report of a similar (the same?) problem, see kovidgoyal/kitty#2026

@mqudsi
Copy link
Contributor

mqudsi commented May 26, 2020

I thought I'd tackle this but I can't get it to reproduce. I first tried whatever ranger I had installed via debian packages then switched to the latest ranger from pip3 (1.9.3). I'm running kitty 0.12.3 and fish 3.1.2-630-gad020e84d-dirty

image

image

image

image

Note that I had to set TERM to xterm-kitty for image preview to show as non-textual in ranger.

@krobelus
Copy link
Member

@mqudsi maybe try with a newer version of kitty, With 0.17.4 I get 65535x65535 when running kitty icat --print-window-size inside kitty. TERM is already set to xterm-kitty by default for me.

@ammgws
Copy link
Contributor

ammgws commented May 26, 2020

@mqudsi perhaps try some higher resolution photos?

@mqudsi
Copy link
Contributor

mqudsi commented May 27, 2020

@krobelus same with 0.17.4 (kitty git master), which did indeed set xterm-kitty on its own:

image

@ammgws that was a high resolution image, but more importantly, the underlying cause is not reproducing for me.

@mqudsi
Copy link
Contributor

mqudsi commented May 31, 2020

I have no real reason to think it matters, but are you guys maybe running under wayland?

@ammgws
Copy link
Contributor

ammgws commented May 31, 2020

Yes, using sway

@krobelus
Copy link
Member

Xorg here.

@dsanson
Copy link
Contributor

dsanson commented May 31, 2020

Mac OS X.

@mqudsi
Copy link
Contributor

mqudsi commented Jun 8, 2020

@ridiculousfish just pushed a lot of changes to term size; perhaps give it another try?

@ridiculousfish
Copy link
Member

I'll give it a try too

@ammgws
Copy link
Contributor

ammgws commented Jun 8, 2020

Seems to be fixed for me. Top is before updating fish.
image

@faho
Copy link
Member

faho commented Jun 8, 2020

Yeah, seems to be working, pretty sure it's d5a239e that fixed it.

I think we were stomping on Kitty's stuff and it broke by forgetting the window size, which would also explain why it remains broken after quitting fish.

If someone cares enough, that's a thing kitty could be more resilient against.

@faho faho closed this as completed Jun 8, 2020
@faho faho modified the milestones: fish-future, fish 3.2.0 Jun 8, 2020
@callahad
Copy link

callahad commented Jun 8, 2020

I bisected and can confirm that d5a239e resolved this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

No branches or pull requests

9 participants