KaTeX / KaTeX

Fast math typesetting for the web.
https://katex.org
MIT License
18.24k stars 1.17k forks source link

Code duplication and maintainability ambiguities in font and metric generation #3702

Open IvoWingelaar opened 2 years ago

IvoWingelaar commented 2 years ago

Before reporting a bug

I've been rewriting the code responsible for font, and the associated metrics, generation from the Perl scripts to a (IMHO) more readable (and therefore maintainable) collection of Python scripts. The font generation scripts are finished, but I've ran into some situations that point to possible issues when rewriting the metric generation.

I've tried so search the archived katex-fonts repository for more information about these design choices, but I couldn't find it, so are these decisions intentional?

Environment (please complete the following information):

edemaine commented 2 years ago

I believe the messyness in the code (and those duplicate tables) are inherited from MathJax code that this was originally based on. But it predates my time so I'm not sure.

I agree that it would be best to remove this risky duplicated code (in particular resolving the discrepancies), and to avoid duplicates (and maybe detect duplicates to avoid this in the future?). Or to otherwise rewrite this code in a better way.

IvoWingelaar commented 2 years ago

The issue is not as bad as I thought. I re-used the font generation mapping to also generate the metrics, and get the following diff on my newly generated fontMetricsData.js:

I'm confident my work's in a state that can be reviewed, so how granular do you want my PR to be? A single PR for both the font-generation and the metrics generation changes? Or two separate ones?

Afterwards, it's not that difficult to add a sanity check to detect duplicate mappings. My next goal though is to add the pair-kerning tables to the fonts.

IvoWingelaar commented 2 years ago

I've taken the initiative to prepare to split it in multiple PR's, as the changes can be semantically grouped, with the goal to make it easier to review. A huge delta of multiple KLOC's added/deleted seems like bad etiquette, hopefully a chain of PR's is less bad. This chain is #3713, #3714, #3715. When this chain is approved and merged I can start working on implementing pair-kerning, but this is feature where a separate issue to track progress seems more appropriate.