Skip to content

Add sixel backend #154

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

Merged
merged 7 commits into from
Nov 9, 2019
Merged

Add sixel backend #154

merged 7 commits into from
Nov 9, 2019

Conversation

shuni64
Copy link
Contributor

@shuni64 shuni64 commented Nov 8, 2019

Closes #121
This adds decent support for sixel graphics. I only tested it on xterm, but it should work on other terminal emulators too. There seems to be a 16 color limit, so dithering is used to make them look somewhat okay. The kitty backend also needed a fix because the thread workaround caused it to read one more byte from stdin. It wasn't fatal, but would've caused weird behaviour later on, so I replaced it with posix calls.

@o2sh
Copy link
Owner

o2sh commented Nov 8, 2019

@CephalonRho Don't you think, it would be better to let the user choose which image_backend to use instead of having a fallback to the first one that is supported by the terminal?


Still haven't tested it, but I'm sure it works great!!

@o2sh
Copy link
Owner

o2sh commented Nov 8, 2019

mlterm (version 3.8.6) >>
image

xterm(330) >>
image

@o2sh
Copy link
Owner

o2sh commented Nov 9, 2019

Still doesn't work on my xterm, which version do you use?

Beside, I don't think that the two step process of first setting the image backend and then being able to use it is the best way to do it.

Let's make it simpler:

onefetch --sixel ./my-picture.png
onefetch --kitty ./my-picture.png

and no fallback (get_best_backend), if the specified backend isn't supported, just show the regular output (ascii_logo + git_infos), or just the git_info (right column) and no ascii_logo.

@shuni64
Copy link
Contributor Author

shuni64 commented Nov 9, 2019

I'm using xterm(350). It should be launched with "xterm -ti vt340" to enable sixel support, but if it's not enabled then it should've panicked unless --image-backend was specified.

Not sure if requiring the user to know which backend their terminal emulator supports is a great idea. We can detect the backend that will definitely work best on the used emulator automatically, so I don't see a reason to not do that. Right now there shouldn't be a reason to use the --image-backend argument other than the detection failing (for example on yaft), so it wouldn't be a two step process most of the time.

@o2sh
Copy link
Owner

o2sh commented Nov 9, 2019

It should be launched with "xterm -ti vt340" to enable sixel support

That dit it, thx!

it should've panicked unless --image-backend was specified.

No, it didn't panicked, just a funky rendering (see screenshot above)


Ok, for the automatic image_backend detection.


By the way, I don't think the image-backend flag works, when I'm on a Kitty terminal and set the image backend to sixel, it stills render the image using kitty... I'm pretty sure the overriden value is lost in-between two execution.

I feel like this is the kind of setting that should be put into a static file and read at compile time/Run time, not a command line flag.

If we stick to the automatic detection, you can rollback to the first version, unless I'm missing something

@shuni64
Copy link
Contributor Author

shuni64 commented Nov 9, 2019

I forgot to include the part that actually detects sixel support when removing the thread. That's hopefully fixed now.


The image-backend flag works fine for me.
image
It prints lots of garbage to the screen, but that's expected when using the sixel backend on a terminal emulator that doesn't support sixel.
The flag does not persist across executions, and unless I missed something there's no config file to implement that with. It would probably be weird for scripts to use anyway.

@o2sh
Copy link
Owner

o2sh commented Nov 9, 2019

LGTM 👍

Thx a lot,

Looking forward to your next contribution @CephalonRho

@o2sh o2sh merged commit 6b145a5 into o2sh:master Nov 9, 2019
@shuni64 shuni64 deleted the sixel-backend branch November 9, 2019 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Image backend support to SIXEL
2 participants