Follow full xparsecolor spec in escape sequences

[rbong]

Escape sequences in xterm are parsed according to xparsecolor. xparsecolor supports 1, 2, 3, and 4 digit hex colors. Previously, only 2 digits were supported.

This also fixes a bug where "fX" was parsed as "0xf" instead of "0xf0". xterm handles invalid hex values as 0, except for '/'.

The response to a request for fg/bg must be a valid escape sequence. Most terminals output a 4-digit hex response on request for fg/bg color. This changes the response to 4-digit, improving compatibility.

• xterm control sequence document referencing xparsecolor
• xparsecolor spec describing the valid colors
• context for why we were using 2-digit color hex responses

[rbong]

Besides the issue above I will add this to the changelog.

[rbong]

Everything looks good from my end now.

[rbong]

Looks like rustfmt failed on CI and it failed a check for cognitive complexity. I will run rustfmt. Any tips for bringing down cognitive complexity is appreciated, since I'm not pretty new to Rust, but I will figure it out if not. I'll run any local checks before pushing next time.

[chrisduerr]

I've only taken a super small peek at this, but this looks like extreme amounts of macros. You'll want to keep macros to a minimum since they add a lot of complexity and compile time, instead you should prefer abstracting over complexity with methods when possible.

[rbong]

Good to know, thanks

[rbong]

Hopefully that should fix rustfmt and clippy problems.

As far as the issues with macros goes, I removed some unnecessary macros. I was able to cut out one macro that was originally in the function by using .map instead. I'm about as far as I can get reducing the complexity of this function while somewhat preserving the original style, if I need to take a step further and refactor parsing colors entirely now that it's a bit more complex, let me know.

[rbong]

I have refactored the function based on the example given above - thank you, that was a much better starting point. All of the issues should be resolved. There are no macros, legacy colors are parsed in the same function, and a simple loop is used.

[rbong]

I am working on the CI issues.

[rbong]

Fixed clippy and rustfmt problems, it's ready to be reviewed again whenever.

[chrisduerr]

I thought I'd answer in a top-level comment for visibility and to make things a bit easier to read.

Regarding the way additional depth should be calculated, you can find the full information I know about here:
https://linux.die.net/man/3/xparsecolor

To go into a bit more detail, the legacy #RRGGBB colors are supposed to define the most significant bits of the color. So #RGB would be the same as #R000G000B000.

The rgb: colors are handled a bit differently. If not enough depth is specified, they're supposed to be scaled. So f[fff] would be equal to 1.0/255 and [0/000] is equal to 0. So a value of 42 would be equal to 25.88/66, if we'd scale that back up it would be 428F instead of 4200 or 4242. At least that's how I understand it based on this documentation.

Full disclosure, my main goal with this PR is really to support 4-digit color query responses, which has already been added to the main branch for an unrelated issue. Improving XParseColor compatibility was just a prerequisite. However I am happy to help and I have added support for more color formats here with tests here

If you're not interested in adding full XParseColor support, we can decide on a subset of it too. Though I feel like it would be nice to get this out of the way. In theory with XLookupColors even colors like red would be valid, however I'd understand if the line would be drawn here.

If we decide to go all the way, it would probably be a nice idea to separate that into a different workspace module (/crate/library) so we could offer an XParseColor crate for the broader Rust community, however that might be a bigger project than anyone's signed up for with this PR. :)

[rbong]

The rgb: colors are handled a bit differently. If not enough depth is specified, they're supposed to be scaled. So f[fff] would be equal to 1.0/255 and [0/000] is equal to 0. So a value of 42 would be equal to 25.88/66

I missed that, thank you. It should now be fixed and tested. I thought you meant that we were supposed to do something with the third and fourth bit.

If you're not interested in adding full XParseColor support, we can decide on a subset of it too.

I think at this point, if I missed a format, I'd be more comfortable with creating an issue than fixing it here. Though obviously, if some color format is parsed incorrectly here, I will fix it.

If we decide to go all the way, it would probably be a nice idea to separate that into a different workspace module

It's a good idea, but I wouldn't be comfortable with maintaining a rust library of any size right now given my skill level with the language.

[chrisduerr]

I've rebased this code and cleaned up some of the remaining style issues.

Thanks for your contribution, I think everything should be good to go now.

[rbong]

Awesome, thank you!