Associate font metrics with Options, not Style. #743
Conversation
Font metrics are associated with a given font and size combination. Before KaTeX understood sizing commands, sizes were associated with a Style. That's not true now. So instead of `style.metrics`, use `options.fontMetrics()`, since `options` knows the font and the size. This is a cleanup commit with no visible effects on most tests (there could be some small effect on size + style combinations). It will make other changes possible later.
| // they correspond to the font parameters of the extension fonts (family 3). | ||
| // See the TeXbook, page 441. In AMSTeX, the extension fonts scale; to | ||
| // match cmex7, we'd use cmex7.tfm values for script and scriptscript | ||
| // values. |
There was a problem hiding this comment.
I'm surprised that the third value in each of the arrays isn't determined by cmex5 similar to how cmsy5 is used for the values above.
There was a problem hiding this comment.
There is no cmex5. AMSTeX supplies cmex7, cmex8, cmex9 to TeX's cmex10.
| doubleRuleSep: doubleRuleSep, | ||
| // The space between adjacent `|` columns in an array definition. From | ||
| // `\showthe\doublerulesep` in LaTeX. Equals 2.0 / ptPerEm. | ||
| doubleRuleSep: [0.2, 0.2, 0.2], | ||
| }; |
There was a problem hiding this comment.
Now that this object also includes xis as well as sigmas maybe we should change the name of it. Maybe fontMetrics.
There was a problem hiding this comment.
Called it sigmasAndXis
src/fontMetrics.js
Outdated
| if (sigmas.hasOwnProperty(key)) { | ||
| metrics[key] = sigmas[key][sizeIndex]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Object.keys(sigmas).forEach(key => metrics[key] = sigmas[key][sizeIndex])
is another possibility. I'm okay either way.
| @@ -1,6 +1,3 @@ | |||
| /* eslint no-unused-vars:0 */ | |||
|
|
|||
| const Style = require("./Style"); | |||
There was a problem hiding this comment.
Nice. I ended up removing this dep in another PR. I missed the comment at the top.
| */ | ||
| Options.prototype.fontMetrics = function() { | ||
| if (!this._fontMetrics) { | ||
| this._fontMetrics = fontMetrics.getFontMetrics(this.size); |
There was a problem hiding this comment.
When I first looked at this I thought this might create a lot of objects but then I remembered that getFontMetrics also stores a copy of each unique object it returns which means there's a small number of objects being created.
src/Options.js
Outdated
| @@ -24,6 +26,7 @@ const sizeStyleMap = [ | |||
| ]; | |||
|
|
|||
| const sizeMultipliers = [ | |||
| // fontMetrics.js:getFontMetrics also uses size indexes. | |||
There was a problem hiding this comment.
I'm not sure what this comment means.
There was a problem hiding this comment.
It means that if you change size indexes you should change getFontMetrics
|
Hi! Responded to the review; any other comments? |
|
FYI, merging your #745 ended up making a small conflict here. |
|
Fixed the conflict (assuming a squash-merge later, didn't bother to rebase). |
|
@kohler looks good. Thanks cleaning this up. |
Font metrics are associated with a given font and size combination.
Before KaTeX understood sizing commands, sizes were associated with
a Style. That's not true now. So instead of
style.metrics, useoptions.fontMetrics(), sinceoptionsknows the font and thesize.
This is a cleanup commit with no direct effect; it will make
other changes possible later.