AtlasEngine: Fix ligature splitting for JetBrains Mono #15810
Conversation
lhecker
added
Issue-Bug
| @@ -5,6 +5,7 @@ | |||
|
|
|||
| #include <array> | |||
| #include <cassert> | |||
| #include <cstdio> | |||
There was a problem hiding this comment.
You remember how we had a bug report a long time ago about how _vsnwprintf_s was an undefined symbol? Well as it turns out they were right about it, but it simply didn't happen with the back-then-stable version of VS. In the latest version of VS (or rather MSVC) this code doesn't compile anymore unless we explicitly include cstdio, which we probably should've always done.
There was a problem hiding this comment.
(I fixed it because I used the app to test my change.)
| // Our loop below will split the ligature by processing it from left to right. | ||
| // Some fonts however implement ligatures by replacing a string like "&&" with a whitespace padding glyph, | ||
| // followed by the actual "&&" glyph which has a 1 column advance width. In that case the originalQuad | ||
| // will have the .color of the 2nd column and not of the 1st one. We need to fix that here. |
There was a problem hiding this comment.
The comment mentions 2-column glyphs. Has the code been tested with 3-column and 4-column wide glyphs like <-- and <!--?
There was a problem hiding this comment.
The code doesn't seem to be specific to any column sized glyphs, instead it fixes the scenario where same cell is shared by two adjacent glyphs.
There was a problem hiding this comment.
In this repository I added a tool called benchcat (bc.exe) a little while ago. Among others it's able to print a text file with a randomized color per extended grapheme. This is the file I use for testing:
__ -- --- -~ ;; :: ::: ::=
:// := !: !! !!. !. != !==
?: ?? ??= ?. ?= .- .? ..
... ..< ..= .= (* [| ]# {|
}# *) ** *** */ *> /* //
/// /\ /> \/ \\ && #_ #_(
#: #! #? #( #[ #{ #= %%
^= ++ +++ +> <!-- <* <*> </
</> <+ <+> << <<< <<= <= <>
<| <|> <|| <||| <~ <~> <~~ <$
<$> =<< == === =>> >= >> >>=
>>> |] |} |> || ||> ||| |||>
~- ~@ ~= ~> ~~ ~~> $> www
which then looks like this:
There was a problem hiding this comment.
Cool and useful tool! It might be worth having a fixed-color (non-random) mode as well, so that it's possible for a human to verify not only that colors are different, but also that they're the expected color. Having 3-4 preselected colors with high contrast/hue differences from each other could enhance the ability to quickly detect issues at a glance.
Also, for example the := and ::= and !! and \/ and <> and <~~ and =<< cases got adjacent random colors that are very similar to each other. It might be useful to skew the random colors to avoid similarities to adjacent colors. (I have no colorblindness, and I'm able to see differences in all of the cases I listed -- but (a) not everyone will be able to, and (b) the RGB values are pretty close to each other and are thus inherently harder to visually detect quickly.)
There was a problem hiding this comment.
The code doesn't seem to be specific to any column sized glyphs, instead it fixes the scenario where same cell is shared by two adjacent glyphs.
@tusharsnx I was asking about the testing validation, not about the code. The comment in the code explicitly stated "1st" and "2nd" column; there was no mention of 3- or 4-character ligatures. It was ambiguous what the scope of testing had been, so I asked whether 3- and 4-character ligatures had been tested. It's an important question to ask, under the circumstances. Leonard's response is exactly what I was hoping for, and covers the broad spectrum of cases. ❤️
Co-authored-by: Tushar Singh <[email protected]>
lhecker
added
the
AutoMerge
|
/azp run |
microsoft-github-policy-service
bot
merged commit a7a4490
into
main
microsoft-github-policy-service
bot
deleted the
dev/lhecker/atlas-engine-fixup
branch
Some fonts implement ligatures by replacing a string like "&&" with a whitespace padding glyph, followed by the actual "&&" glyph which has a 1 column advance width. In that case the algorithm in `_drawTextOverlapSplit` will get confused because it strictly scans the input from left to right, searching for color changes. The initial color is the glyph's color and so it breaks for such fonts because then the first split will retain the last column's color. ## Validation Steps Performed * Use JetBrains Mono * Print ``"`e[91m`&`e[96m&`e[m"`` * Red and blue `&` appear ✅ --------- Co-authored-by: Tushar Singh <[email protected]> Co-authored-by: Dustin L. Howett <[email protected]> (cherry picked from commit a7a4490) Service-Card-Id: 90153416 Service-Version: 1.18
Some fonts implement ligatures by replacing a string like "&&" with
a whitespace padding glyph, followed by the actual "&&" glyph which
has a 1 column advance width. In that case the algorithm in
_drawTextOverlapSplitwill get confused because it strictly scansthe input from left to right, searching for color changes.
The initial color is the glyph's color and so it breaks for such fonts
because then the first split will retain the last column's color.
Validation Steps Performed
"`e[91m`&`e[96m&`e[m"&appear ✅