Conversation
itsjunetime
left a comment
itsjunetime
left a comment
There was a problem hiding this comment.
I really like this and think it's a great idea that I'd also probably end up using most of the time. Thanks for sending it in :)
However, I'm a bit apprehensive about the cost of maintaining the parsing code, which (afaict) doesn't cover all the different ways that this OSC thing can work. If you could poke around to try to find an alternative crate for that parsing, that would be very appreciated :)
| let mut prev = None::<u8>; | ||
| let mut byte = [0u8; 1]; | ||
| loop { | ||
| handle.read_exact(&mut byte).ok()?; |
There was a problem hiding this comment.
Imo we should probably ready into a larger buffer instead of just reading byte-by-byte, right? Less syscalls and such. I think you can essentially just wrap handle in a BufReader and that'll do it for you - kinda how we do it in the get_font_size_through_stdio fn.
| (fg.unwrap_or(MUPDF_BLACK), bg.unwrap_or(MUPDF_WHITE)) | ||
| } | ||
|
|
||
| fn query_osc_color(osc: u8, handle: &mut std::io::StdinLock<'_>) -> Option<i32> { |
There was a problem hiding this comment.
Reading through this function in general is making me a bit apprehensive, mainly due to reading some descriptions of the spec and seeing how many variants in the response specification there can be. If such a crate exists, I think I'd prefer for this functionality to be pulled in from a different crate that maintains more compatibility between the different standards. I worry that if we ship this as-is, we'll get bug reports from people whose terminals implement some other variant of this spec, and that'll just be really annoying to try to keep up with.
Would you be able to poke around and see if you can find a different crate that does this? If not, obviously, we can build in the functionality, it's just not ideal.
There was a problem hiding this comment.
Those variants really only need to be supported by terminals when they receive a request to set the colors. When a terminal is reporting colors, it should only be responding with an rgb format (e.g. something like rgb:1212/3434/5656). There are a few terminals that have an option to respond with two-digit rgb values (e.g. rgb:12/34/56), but I think the default should always be four-digit rgb. Personally I'd consider it a terminal bug if they respond with anything else.
3 participants
That was something useful to me - automatically picking up my terminal bg/fg colors and using those instead of manually specifying them as the command line arguments.
Now I can just run
tdf my.pdf -tand it will render the pdf using the terminal's colors.Not sure if this something anyone else needs, but anyways :)