Fix all AMS mathord symbols #618
Conversation
|
@edemaine thanks for the PR. How did you determine the list of broken chars? Is it just all mathords? |
|
It is all symbols in symbols.js of font "ams" and type "textord". |
| utils.contains(mainitLetters, value)) { | ||
| // For greek capitals, we have a choice, but Math-Italic | ||
| // seems closer to LaTeX. | ||
| //|| utils.contains(greekCapitals, value)) { |
There was a problem hiding this comment.
Is this code supposed to be commented out?
There was a problem hiding this comment.
I meant to ask you about this. Yes, it seems so:
Greek capital letters are in both fonts, Main-Italic and Math-Italic. So which should we use? The master code claims to use Main-Italic for Greek capital letters. But if you try the demo with \mathit{\Omega\imath}, and inspect, you'll see that the \Omega gets class mathit while \imath gets mainit:
<span class="mord mathit" style="margin-right: 0.05017em;">Ω</span>
<span class="mord mainit">ı</span>In fact, the master code ends up calling mathit() with a single-unicode-character string (Ω), while greekCapitals is defined in terms of the macro name ("\Omega"). When my code fixed this, I ended up changing Greek capital letters to use Main-Italic font, which caused some tests to fail. I confirmed with texcmp that Math-Italic seems slightly closer, so kept it there.
Let me know if you'd like this code changed at all...
There was a problem hiding this comment.
In that case, can you remove the commented out code? TBH, I'm not sure why we have the same glyphs duplicated in different fonts. It would be nice if we could avoid this and save a few bytes. I'm curious to find out why the rendering looks different even though the glyphs are the same. Maybe the advance or skew metrics for the Greek capitals are differ between fonts. I think it's safe to investigate this separate from this pull request.
There was a problem hiding this comment.
Should I remove the list of greekCapitals as well? It's currently commented out (to prevent lint from complaining).
| return { | ||
| fontName: "Math-Italic", | ||
| fontClass: "mathit" | ||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can you update the doc string for mathit now that it no longer creates a symbol?
kevinbarabash
added
GH Review: needs-revision
and removed
GH Review: review-needed
labels
|
@edemaine a few minor nits and this'll be ready to merge. Thanks for fixing all of these symbols. |
|
Cool! I just merged with master and moved to ES2016 style. |
|
@edemaine thanks for cleaning up the commented out code. ^_^ |
Here is a fix for #600 (and #609 and #512 and #374). In fact, all AMS textord symbols were similarly broken. Here is a complete list of symbols fixed by this PR:
\hslash, \triangledown, \lozenge, \circledS, \circledR, \measuredangle, \nexists, \mho, \Finv, \Game, \Bbbk, \backprime, \blacktriangle, \blacktriangledown, \blacksquare, \blacklozenge, \bigstar, \sphericalangle, \complement, \eth, \diagup, \diagdown, \square, \Box, \Diamond, \yen, \checkmark, \beth, \daleth, \gimel, \digamma, \varkappa, \varnothing, \malteseBasically, in
buildCommon.js,mathsymhad logic for choosing between font familiesMain-RegularandAMS-Regular, but this applies to typesrel, bin, open, close, inner, punctonly.makeOrdis used specifically formathordandtextord, and needed similar logic. To complicate things,makeOrdwas also losing track of the original macro name and thus AMS vs. Main, replacing it with the unicode symbol. So I had to refactor code a bit so thatmathDefaultcould figure it out.