hackerb9 / lsix

Like "ls", but for images. Shows thumbnails in terminal using sixel graphics.
GNU General Public License v3.0
4.03k stars 127 forks source link

Problems with st-sixel #32

Open ghost opened 4 years ago

ghost commented 4 years ago

Hi

I'm using st-sixel (https://github.com/galatolofederico/st-sixel) which lsix doesn't realise has sixel capabilities. I got lsix working on it by brute force (setting hassixel=yup) but I don't the clever (correct) way to do this.

Hope you're able to help

Ta

smhmd commented 4 years ago

Seconded.

Reproduce:

git clone https://github.com/charlesdaniels/st.git
cd st 
# change font in config.h if you want a working font
make
./st

This will launch new st. Run the command convert foo.jpg -geometry 800x480 sixel:-. image

hackerb9 commented 4 years ago

Thank you for reminding me of this issue.

The problem is that st is not responding correctly to the Send Device Attributes query. If it supports sixels, it should include the number "4" (delimited by semicolons) in its response. You can see that it responded with "6c" which means it only supports VT102 escape sequences, not sixel. This is an easy problem to fix — the response string can be changed to "63;4" and it'd work — but it should be done by the developers of st (@charlesdaniels and @galatolofederico). I'll file a bug report to let them know.

smhmd commented 4 years ago

Because sixel support is added through a patch, I'm not sure if filing a bug to upstream would result in a fix unless what you are talking about is of concern to vanilla st. Maybe this should be taken up with @saitoha.

hackerb9 commented 4 years ago

In the discussion of the patch, @charlesdaniels says,

“I have not implemented the escape sequences which would allow sixel support to be correctly detected by programs running within the terminal, so many programs must be ‘forced’ into their relevant sixel modes.”

Maybe Charles does indeed wish to implement a fix and just needs a nudge?

charlesdaniels commented 4 years ago

I don't use st anymore, and instead use uxterm in VT430 mode (hint: add UXTerm*decTerminalID: vt340 to ~/.Xresources). I don't have any plans to resume working on my st fork at this time. I think st is a worthy project, but UXterm is ubiquitous, well supported, and well tested -- xterm is the standard against which other terminal emulators are judged. Not looking to start a flame war though -- if st solves the problems you want solved, that's great and you should use it.

When I was using it actively, I simply patched my copy of lsix to disable the check for sixel support.

Perhaps lsix should check for an environment variable like LSIX_FORCE_SIXEL_SUPPORT or something and disable the relevant check if so.

data-man commented 4 years ago

Yet another active fork: LukeSmithxyz/st But:

2496: / TODO: render sixel /; 2505: / TODO: implement sixel mode /

charlesdaniels commented 4 years ago

I would like to note that my fork of st is probably not a good starting point for adding sixel support. It more or less worked, but was flaky and had some odd behaviors. Additionally, the diff I started with is unclear on licensing, so the actual license of my fork is questionable (as noted in the README) too. Finally, st has continued to be updated, and I have not kept up with it. I think I tried bringing it forward to a newer base version of st a while ago but there were merge conflicts that were too much for me to resolve at the time.

If someone really wants to do this right, it would be best to start with a fresh copy of st and contribute your patches upstream (the st homepage tracks community patches). I never cleaned mine up properly to contribute it there, but that would be the ideal.

Sixel is not a terribly complicated format. I think that a C programmer with a good knowledge of xlib could do this right in a weekend or two. My xlib skills are not so good though, hence why I didn't do that myself.

Good luck!

hackerb9 commented 4 years ago

Perhaps lsix should check for an environment variable like LSIX_FORCE_SIXEL_SUPPORT or something and disable the relevant check if so.

Done.

I understand you're not working with st any more, but do you think you could add just the small fix I suggested? Basically, instead of returning 6, return 12;4 to Send Device Attributes. I see that your repository is the upstream for five other repositories that use your sixel implementation. It'd be easiest to get this fixed in your repo than in each of those.

Thanks.

charlesdaniels commented 4 years ago

I'll see what I can do. It's been a long time since I looked the handling for escape codes, but this shouldn't be too hard.

I should probably also update the README to indicate it's not active anymore while I'm at it.

smhmd commented 4 years ago

@bakkeby, could you look into adding this into st-flexipatch (and perhaps a community patches)?

charlesdaniels commented 4 years ago

I think I have fixed it, hope this resolves your troubles!