JuliaIO / Sixel.jl

The Julia wrapper of libsixel
MIT License
41 stars 5 forks source link

support transparency #18

Open t-bltg opened 2 years ago

t-bltg commented 2 years ago

Fix https://github.com/JuliaIO/Sixel.jl/issues/17.

PR (mlterm) eqn

master schrodinger

codecov[bot] commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@07011d3). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #18   +/-   ##
=========================================
  Coverage          ?   41.19%           
=========================================
  Files             ?       10           
  Lines             ?      352           
  Branches          ?        0           
=========================================
  Hits              ?      145           
  Misses            ?      207           
  Partials          ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

t-bltg commented 2 years ago

@johnnychen94 ?

t-bltg commented 2 years ago

Regarding the tests, there is something fundamentally fishy with src/terminaltools.jl: query_terminal requires stdin fd to be opened, and under the tests, stdin is closed since tests are ran under non interactive mode, so testing is not representative. ==> side effect of https://github.com/JuliaLang/Pkg.jl/pull/3065.

I couldn't find a way to fake stdin when querying sixel support, so I'm a bit stuck here @johnnychen94.

johnnychen94 commented 2 years ago

Why would you need the interactive mode to run the test? I mean, it's more or less like how https://github.com/JuliaIO/Sixel.jl/blob/07011d31c9308a29d58390fb83aa14268d1a21e1/test/backend/libsixel.jl#L60-L63 does for RGBA, right?

img = rand(RGBA, 5, 5)
io = IOBuffer()
sixel_encode(io, img)
@test ref == String(take!(io))
t-bltg commented 2 years ago

I got confused by https://github.com/JuliaIO/Sixel.jl/blob/07011d31c9308a29d58390fb83aa14268d1a21e1/test/runtests.jl#L9

johnnychen94 commented 2 years ago

sixel_encode supports three outputs:

It's okay to use IOBuffer to get the encoded sequence and test against it.

t-bltg commented 2 years ago

Last commit is optional, let me know I you don't want it.

t-bltg commented 2 years ago

Let's put this on hold since the real fix would (hopefully) be brought by https://github.com/libsixel/libsixel/pull/23.

I'm planning of bumping libsixel to 1.10.3 (https://github.com/JuliaPackaging/Yggdrasil/pull/5432 - might require running gen/generator.jl here).

ctrlcctrlv commented 2 years ago

@t-bltg Perhaps we should work together instead of separately, as I got a bit scared off by all the memory safety issues in libsixel so have been afraid to make big changes to the C code, especially because we know for a fact the current API's aren't even able to be made safe as a few of them lack critical information like data size.

I don't know Julia, sorry, but my longterm plan upstream has been to:

t-bltg commented 2 years ago

That's certainly a good idea to transition to rust if you want something similar to current c interface. I must admit I never wrote rust code (lots of Fortran and C, and obviously julia) ;). But if I can help in any way ...