Reset dim SGR independently from bold

[pedromfedricci]

This PR fixes #1802 by simply applying the ansi code for normal intensity first and then reapplying bold if appropriate.

Should I write a test for this? I didn't quite find anything close to it to base a implementation on, but would be happy to do so with some help 😁.

dim-effect-fix.mp4

[raphCode]

Very nice work!

We make use of the insta snapshot testing library.
You can search the code for "snapshot", maybe you find an appropriate way of duplicating and modifiying an existing test?
It was quite straightforward when I did it the first time :)

Otherwise @imsnif can surely give you a better hint where to add a test.

[imsnif]

Hey!

About tests: we unfortunately don't have automated tests for styles because we can't take snapshots of them in a Human readable way.

About the change itself: good work on finding this and getting it to work. This is not an easy part of the code-base to thread. I'll admit though that I'm a little concerned about the change. While I understand that it solves this particular use-case, I'm worried other use-cases might not expect it (i.e. developer: "I only need to cancel bold and dim with 22 once and not worry about it").
In general, these two styles (as far as I know) are supposed to cancel each other. Do you have examples from non-Kitty* terminals of this?

*Kitty sometimes adds additional behaviour to terminals that while great for Kitty users, doesn't line up with "convention" (as much as we have convention in this field of ours).

[pedromfedricci]

Hey! thank you @raphCode @imsnif for helping out.

In general, these two styles (as far as I know) are supposed to cancel each other.

I can't really tell if they are supposed to cancel each other or not, though in my experience some term emulators do support these two styles on top of the same text, examples: alacritty and kitty. It might be that there is no agreed upon behavior for this. I found this GNOME/vte brief discussion about faint/bold mutual exclusion.
But I would like to point out that #1802 visual issue occurs when dim is applied on it's own, rather than in conjunction with bold.

Do you have examples from non-Kitty* terminals of this?

The video that I recorded is actually showcasing alacritty. I tested the issue and the fix on: alacritty, kitty and wezterm. For some other term emulators: gnome-terminal, konsole and contour, the dim effect is suppressed entirely, regardless of whether zellij is multiplexing the term or not, and this behavior is not changed after the fix.

To provide some reference, I found that helix applies a similar strategy as this PR, and they are using crossterm to queue the SGR parameters.

[imsnif]

@pedromfedricci - I'm a little busy these weeks with the upcoming release, but I don't want to hold this back. So I'm going to ask for your help here. To be confident in this change, I'd like to test the behavior of different terminals directly. Specifically with the echo command (eg. something like echo -e "\033[22mtext - forgive me, I don't remember the exact ansi codes by heart).

If you'd like to do some test cases here to see exactly how a few popular terminals behave, that would be great. Otherwise I'd be happy to take a deeper look in a week or two.

[pedromfedricci]

ok! so this is the test case I came up with:

printf "\x1b[22m\x1b[1m\x1b[2mABCDEF abcdef Bold+Dim\n\x1b[22m\x1b[1mABCDEF abcdef Bold\n\x1b[22m\x1b[2mABCDEF abcdef Dim\n\x1b[22mABCDEF abcdef Normal\n"

It will print some text lines, each with a set of SGR parameters, in order: bold+dim, bold, dim and normal. For each line, we first reset to normal intensity, and then we apply some other parameters. Each parameter has it's own SGR segment instead of using semicolons.
For all these terminal emulators: gnome-terminal, konsole, xterm, tilix, terminator, tilda, alacritty, kitty, contour; the bold+dim text has a distinguishable visual effect on it in comparison to the others. For wezterm, bold and dim effects are mutually exclusive, only the first parameter seems to be applied (even when using a single, semicolon separated SGR segment).

I'm a little busy these weeks with the upcoming release, but I don't want to hold this back.

I completely understand if you would prefer to look at this only after your next release! 🚀
The issue does not seem to be affecting many users at all.

[imsnif]

Thank you for your extensive tests on this, @pedromfedricci - and for your patience :)
I'm happy to go through with this and merge. Just one last question: the way the code is now, I think if we're resetting Dim and don't have Bold set, we'll be applying it. Or am I misunderstanding?

[pedromfedricci]

the way the code is now, I think if we're resetting Dim and don't have Bold set, we'll be applying it.

If we don't have Bold set then its SGR parameter is not written. Only if the inner statement if let Some(AnsiCode::On) = self.bold matches we rewrite Bold SGR into the Formatter.

[imsnif]

aha! My bad, I misread the comment and didn't remember the ANSI codes by heart :)

Cool, I'm happy to merge this. Thanks again for all your hard work!

[pedromfedricci]

thank you! 😄