simplify combining chars by k4b7 · Pull Request #1633 · KaTeX/KaTeX

k4b7 · 2018-08-18T03:50:43Z

In preparation of refactoring domTree classes into POJOs as described in my comments in #1618, this PR removes the tryCombine method all of these classes. All of the checks we were doing in symbolNode's tryCombine method weren't actually necessary b/c we were only combining characters within the same group which means that they all had the same style/options to begin with.

This diff also merges mathDefault into makeOrd (fixes #1299) which makes it easier to see more of the font picking logic in one place.

codecov · 2018-08-18T04:07:57Z

Codecov Report

Merging #1633 into master will increase coverage by 8.71%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #1633      +/-   ##
==========================================
+ Coverage   85.13%   93.84%   +8.71%     
==========================================
  Files          78       78              
  Lines        4573     4568       -5     
  Branches      801      801              
==========================================
+ Hits         3893     4287     +394     
+ Misses        585      248     -337     
+ Partials       95       33      -62

Impacted Files	Coverage Δ
src/tree.js	`82.35% <ø> (+26.79%)`	⬆️
src/functions/verb.js	`100% <ø> (+87.5%)`	⬆️
src/functions/text.js	`100% <100%> (ø)`	⬆️
src/domTree.js	`92.59% <100%> (+27.62%)`	⬆️
src/functions/op.js	`96.9% <100%> (+2.06%)`	⬆️
src/buildCommon.js	`98.46% <96.87%> (+7.36%)`	⬆️
src/buildHTML.js	`99.23% <0%> (+1.53%)`	⬆️
src/MacroExpander.js	`95.68% <0%> (+1.72%)`	⬆️
src/functions/operatorname.js	`94.73% <0%> (+2.63%)`	⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ca6f51...518f4cb. Read the comment docs.

k4b7 · 2018-08-18T04:35:32Z

src/functions/text.js

    htmlBuilder(group, options) {
        const newOptions = optionsWithFont(group, options);
+        // TODO(kevinb): split group.body into combinable parts to avoid having
+        // complicated checks in buildCommon.tryCombineChars.


The only things we can combine are textords and spacings so if we create groups of consecutive textords and spacings we can pass each of those groups off to buildExpression and combine the symbolNodes that are returned without doing all of complex checks we're doing right now in tryCombineChars.

I worry that some other functions might return the equivalent of a textord that wouldn't be caught before running the function...

That's a good point. I'll remove this TODO and make sure that the new tryCombineChars covers all the cases that symbolNode.tryCombineChar was checking before.

edemaine

Nice transform.

edemaine · 2018-08-18T12:00:28Z

src/buildCommon.js

+            for (const style in prev.style) {
+                if (prev.style.hasOwnProperty(style)
+                    && prev.style[style] !== next.style[style]) {
+                    skip = true;


Instead of using skip (and should probably use break) here, you can just continue charloop if you label the for loop block with charloop:. (I've never done this myself, but see these docs.)

By the way, odd that there is no domTree.createStyle...

Good call. I too have never done that either. I might move these checks into a separate function.

edemaine · 2018-08-18T12:03:45Z

src/buildCommon.js

+            && next instanceof domTree.symbolNode) {
+
+            if (domTree.createClass(prev.classes) !==
+                domTree.createClass(next.classes)) {


I realize that this check is somewhat oversensitive, because the classes aren't sorted. But I guess that's fine.

edemaine · 2018-08-18T12:05:29Z

src/buildCommon.js

+
+            prev.text += next.text;
+            prev.height = Math.max(prev.height, next.height);
+            prev.depth = Math.max(prev.depth, next.depth);


You seem to be missing prev.italic = next.italic;

edemaine · 2018-08-18T12:07:52Z

src/buildCommon.js

 /**
- * Combine as many characters as possible in the given array of characters
- * via their tryCombine method.
+ * Combine consequetive domTree.symbolNodes into a single symbolNode.


Maybe add a comment that chars is modified in place, as well as returned? The former could be dangerous (and now the code that uses tryCombineChars doesn't make it obvious that it's changing in-place.)

Yeah... I'll ~~made~~make a note that this function mutates the input. Maybe I can make a copy fo the array so that we don't mutate the input.

I guess you could assembly the nodes into a new array and return that. In some ways that might be better -- splice is pretty inefficient. In reality, speed is going to depend on whether there's anything to combine ever. If there is, assembling into a new array would be faster; otherwise, the current approach is probably slightly faster.

k4b7 · 2018-08-18T18:04:51Z

@edemaine I've assigned this to you to indicate that it's ready for review again.

ylemkimon · 2018-08-19T14:43:51Z

There seems to be no commit pushed after #1633 (review).

k4b7 · 2018-08-19T15:05:24Z

I forgot to push. I've pushed the changes I had locally now.

k4b7 · 2018-08-19T15:24:14Z

I'll resolve the merge conflicts later today.

ylemkimon · 2018-08-19T18:50:54Z

src/buildCommon.js

+
+            prev.text += next.text;
+            prev.height = Math.max(prev.height, next.height);
+            prev.depth = Math.max(prev.depth, next.depth);


#1633 (comment) by @edemaine

You seem to be missing prev.italic = next.italic;

The previous code check if this.italic > 0 which felt weird so I left it out. prev.italic = next.italic seems like a better way to go.

ylemkimon · 2018-08-21T04:21:11Z

src/buildCommon.js

+        const prev = chars[i];
+        const next = chars[i + 1];
+        if (prev instanceof SymbolNode
+            && next instanceof SymbolNode


How about moving instanceof checks to canCombine?

I wanted to do that originally, but if we do that flow doesn't know that prev and next are SymbolNodes inside of tryCombineChars which means it doesn't recognize the props that we use on prev and next following this check. It's a bummer b/c the code would look a lot cleaner.

It would be cool if flow had a way to describe "if this function returns true then the args of of these types".

@kevinbarabash AFAIK, after if (!(prev instanceof SymbolNode) || !(next instanceof SymbolNode)) return;, flow would refine them as SymbolNode.

~Ugh, I forgot to change the param types to canCombine to prev: any, next: any. Also, I am impressed with flow ability to make a type refinement based on that check. Thank you for prodding me on this.~Double ugh... the flow plugin in my editor was lagging... this check still doesn't refine the types.

k4b7 · 2018-08-23T01:43:43Z

@edemaine can you have another look when you have a chance?

edemaine

Sorry for the delay. One more comment about where to call tryCombineChars.

edemaine · 2018-08-23T03:50:54Z

src/functions/op.js

        } else {
-            base = buildCommon.makeSpan(["mop"], inner, options);
+            base = buildCommon.makeSpan(
+                ["mop"], buildCommon.tryCombineChars(inner), options);


This is a new call to tryCombineChars, right? This suggests to me that it should be called in buildExpression, not anywhere else. For example, \text{{hello}} probably won't combine the characters in the current code, because of the ordgroup in the way, but would if we called it in buildExpression.

Perhaps it should only be called in text mode, though? How is that currently detected? (Vague worry that that's what the italic > 0 check was for...)

It is a new call. It would be nice to handle cases like \text{{hello}}. I tried collating ordgroups but I had taken some of the checks out. I should try again now that all of the checks are back in. There are situations where I could see us not wanting to combine ordgroups though. As an example: {xyz}... in this situation we might want to keep those characters separate in case someone wants to be able to highlight each variable on hover or click to select an individual variable in the term. I think calling it for this particular case of op makes sense b/c it handles things like sin, cos, etc. and I think we should be treating those things as atomic units as opposed to separate letters.

I don't see a use-case for keeping letters as separate objects in text mode -- but I agree there's a use in math mode where each letter represents a separate variable. See buildMathML.buildExpression which does a similar transform of combining mtexts (but of course MathML is trying to represent semantics here; HTML could have different, more visual purposes). But we can discuss this as a separate PR.

I get what you're saying... try to combine characters inside of ordgroup if we're in text mode. I'll make an issue for it. Having fewer spans should improve performance. The fewer DOM nodes we have to create the better.

* Support \includegraphics * Update screenshots * Fix lint errors * Fix more lint errors * Fix lint errors take 3 * Add documentation, error messages * Improve CSS * Update domTree to coordinate with PR #1633 * Update screenshots * fix flow errors * Use same RegEx as href * Fix lint error * Accept raw ArgType * Fix lint error * fix flow error * Add test

Kevin Barabash added 3 commits August 17, 2018 23:44

simplify comibining chars

36ca6fe

try combining chars in text operators instead of all ordgroups

c99cae7

ensure that adjacent chars have the same classes

3c12ce2

fix phantom tests

352914d

k4b7 commented Aug 18, 2018

View reviewed changes

k4b7 self-assigned this Aug 18, 2018

k4b7 added the GH Review: needs-revision label Aug 18, 2018

edemaine changed the title ~~simplify comibining chars~~ simplify combining chars Aug 18, 2018

edemaine requested changes Aug 18, 2018

View reviewed changes

extract canCombe from tryCombineChars, check for skew and maxFontSize

e6528ff

k4b7 assigned edemaine and unassigned k4b7 Aug 18, 2018

k4b7 added GH Review: review-needed and removed GH Review: needs-revision labels Aug 18, 2018

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Aug 19, 2018

k4b7 assigned k4b7 and unassigned edemaine Aug 19, 2018

Merge branch 'master' into domtree_refactor_1

d8cd62f

ylemkimon removed the GH Review: needs-revision label Aug 19, 2018

ylemkimon reviewed Aug 19, 2018

View reviewed changes

check prev.italic !== next.italic

ffe1d4b

k4b7 removed their assignment Aug 20, 2018

k4b7 added the GH Review: review-needed label Aug 20, 2018

k4b7 assigned edemaine Aug 20, 2018

use the last character's italic correction.

c9fab9f

ylemkimon reviewed Aug 21, 2018

View reviewed changes

ylemkimon approved these changes Aug 21, 2018

View reviewed changes

Merge branch 'master' into domtree_refactor_1

518f4cb

edemaine reviewed Aug 23, 2018

View reviewed changes

edemaine approved these changes Aug 23, 2018

View reviewed changes

k4b7 merged commit 5391720 into master Aug 23, 2018

ylemkimon deleted the domtree_refactor_1 branch September 2, 2018 03:21

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade katex from 0.9.0 to 0.11.1 saurabharch/Rocket.Chat#1

Open

edemaine mentioned this pull request Feb 18, 2026

fix: italic separation in \mathnormal #4143

Merged

Uh oh!

Conversation

k4b7 commented Aug 18, 2018 • edited by ylemkimon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edemaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Aug 18, 2018

Uh oh!

ylemkimon commented Aug 19, 2018

Uh oh!

k4b7 commented Aug 19, 2018

Uh oh!

k4b7 commented Aug 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 commented Aug 23, 2018

Uh oh!

edemaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

k4b7 commented Aug 18, 2018 •

edited by ylemkimon

Loading

codecov bot commented Aug 18, 2018 •

edited

Loading

k4b7 Aug 18, 2018 •

edited

Loading

k4b7 Aug 21, 2018 •

edited

Loading

k4b7 Aug 21, 2018 •

edited

Loading