Support \hline #1306

ronkok · 2018-05-11T01:14:40Z

Support \hline. Fix #267, part of #269.

Fixes issue #1271

Support `\hline`.

codecov · 2018-05-11T01:20:40Z

Codecov Report

Merging #1306 into master will decrease coverage by 0.31%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##           master    #1306      +/-   ##
==========================================
- Coverage   84.24%   83.92%   -0.32%     
==========================================
  Files          61       61              
  Lines        3973     4001      +28     
  Branches      662      665       +3     
==========================================
+ Hits         3347     3358      +11     
- Misses        532      546      +14     
- Partials       94       97       +3

Impacted Files	Coverage Δ
src/environments/array.js	`42.85% <39.28%> (-0.53%)`	⬇️

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 929b9bf...0543f9e. Read the comment docs.

ronkok · 2018-05-11T01:25:10Z

Here's how they look:

ylemkimon · 2018-05-11T10:23:35Z

The commit Support Unicode \ll and \lll seems to be mistakenly committed into the master branch and preventing merge from KaTeX master.

edemaine · 2018-05-11T12:37:01Z

@ylemkimon I don't think it's actually a problem. @ronkok's last few PRs have been this way, and the old commits get swept up in the squash. At some point it'd be good to start new PRs from the master branch, though. (For me it's git checkout upstream/master; git checkout -b newbranch.)

edemaine · 2018-05-11T12:40:30Z

@ronkok Wow, it's exciting to see how this is relatively easy thanks to stretchy \overline!

There are two natural features that aren't yet in this PR; is it worth adding them now?

\hline can be repeated to make multiple horizontal lines between rows. (I think it also interrupts the vertical lines... that may be harder to do, but could be skipped for now.) This would be good for "full \hline support".
\cline{3-5} does \hline only for columns 3 through 5. I'm guessing you have to render the horizontal line for each column separately anyway (because tables are rendered column-by-column), so this isn't too hard... But also not as important as \hline itself.

ronkok · 2018-05-11T12:52:06Z

@ylemkimon GitH(Hub) has been acting that way since I did manual edit on a merge conflict in the \ll PR. As soon as this PR and PR #1162 are merged, I'll dive in and fix my repository. Until then, I don't want to make any rash moves.

ronkok · 2018-05-11T12:58:03Z

@edemaine Later today, I'll add the double \hline. As you suggest, today's effort will not interrupt any vertical lines. I may also add a dotted \hline.

No promises on \cline. My currrent \hlines are not done column by column. I just let the browser stretch each one the full width of the array. In order to implement \cline, I would have to work my way deeper into the array parser. That may not happen in time for the next release.

ylemkimon · 2018-05-11T16:41:47Z

src/environments/array.js

+                possibleHLine = parser.nextToken.text;
+            }
+            if (possibleHLine === "\\hline") {
+                hlines.push(true);


Can group.value.addHLines = true; be done here?

@ylemkimon Stand by. I've got a code cleanup commit coming in about an hour. It will change this section of the code.

ylemkimon · 2018-05-11T16:44:23Z

src/environments/array.js

+            while (/\s/.test(possibleHLine)) {
+                parser.consume();
+                possibleHLine = parser.nextToken.text;
+            }


Could this be done by parser.consumeSpaces()? (same for above)

ronkok · 2018-05-11T17:19:32Z

Double \hlines:

ronkok · 2018-05-11T17:22:16Z

Now I think this is ready for review. I will also need some help to create screenshots for Hline.

edemaine · 2018-05-11T18:27:13Z

The screenshot renders as 1246x752, but needs to be at most 1024x768. Can you make it a little less wide, e.g., reducing the \quads?

ronkok · 2018-05-11T18:31:26Z

There's no point in cutting it close. One of the arrays was redundant and I took it out.

ylemkimon · 2018-05-12T09:12:18Z

src/environments/array.js

+    while (/^\s$/.test(next)) {
+        parser.consume();
+        next = parser.nextToken.text;
+    }


Could this be done using parser.consumeSpaces()?

Yes. Please use that instead.

ylemkimon · 2018-05-12T09:27:31Z

src/environments/array.js

+        if (nextTokenIsHLine(parser)) {
+            numHLinesBeforeRow[0] = 2;
+        }
+    } else {


I think the following is more readable (same for below), but it's matter of personal preference:

if (nextTokenIsHLine(parser)) { if (nextTokenIsHLine(parser)) { numHLinesBeforeRow.push(2); } else { numHLinesBeforeRow.push(1); } } else {

Is there something magic about 2? Why not a while loop and counter? Does LaTeX not support more than 2 \hlines?

Until just now, I had never tried more than two \hlines. while loop coming right up.

ronkok · 2018-05-12T16:12:08Z

And now, with multiple \hlines:

ylemkimon · 2018-05-12T16:38:53Z

src/environments/array.js

+    for (let i = 2; i <= numHLinesBeforeRow[0]; i++) {
+        totalHeight += 0.25;
+        hlinePos.push(totalHeight);
+    }


This can be simplified to (same for below):

if (numHLinesBeforeRow[0] > 0) { totalHeight += 0.25 * (numHLinesBeforeRow[0] - 1); hlinePos.push(totalHeight); }

@ylemkimon We need a loop in order to handle multiple \hlines. So I think you mean that the code could read something like:

for (let i = 1; i <= numHLinesBeforeRow[0]; i++) { if (i > 1) { totalHeight += 0.25; } hlinePos.push(totalHeight); }

That would be simpler for a human to read. But force of habit makes me try to avoid putting conditional statements inside a loop. Maybe this is not the place for that kind of thing. After all, we're not likely to get very many \hlines.

Would you prefer my suggested revision to the current code?

@ronkok I thought hlinePos is added only once, sorry. I think we can leave as-is.

I thought hlinePos is added only once

Which goes to show that code I suggested above would indeed be easier for a human to read. The speed loss with so few \hlines is negligible. I'll make the change.

ylemkimon · 2018-05-12T16:49:05Z

src/environments/array.js

+        return true;
+    }
+    return false;
+}


How about:

function getNumHLines(parser: Parser): number { let n = 0; parser.consumeSpaces(); while (parser.nextToken.text === "\\hline") { parser.consume(); n++; parser.consumeSpaces(); } return n; }

(or for loop if you prefer) and

ylemkimon · 2018-05-12T16:49:59Z

src/environments/array.js

+    // Test for \hline at the top of the array.
+    while (nextTokenIsHLine(parser)) {
+        numHLinesBeforeRow[0] += 1;
+    }


const numHLinesBeforeRow = []; numHLinesBeforeRow.push(getNumHLines(parser));

(same for below)?

That's an outstanding suggestion. I'll do it.

ronkok · 2018-05-12T17:28:53Z

Once again, I'll need help to generate a screenshot for HLine. Sorry to be a burden.

ronkok · 2018-05-12T17:44:59Z

Thanks, @edemaine!

ronkok added 14 commits April 24, 2018 14:20

Support Unicode \ll and \lll

7e9edf8

Fixes issue #1271

Merge remote-tracking branch 'upstream/master'

b620f16

Merge remote-tracking branch 'upstream/master'

bae86d8

Merge remote-tracking branch 'upstream/master'

3ab032c

Merge remote-tracking branch 'upstream/master'

ee8828e

Merge remote-tracking branch 'upstream/master'

05766f0

Merge remote-tracking branch 'upstream/master'

d658559

Merge remote-tracking branch 'upstream/master'

78dc1f4

Merge remote-tracking branch 'upstream/master'

d6f43a4

Merge remote-tracking branch 'upstream/master'

757ef28

Merge remote-tracking branch 'upstream/master'

28a526c

Merge remote-tracking branch 'upstream/master'

8e7b69d

Support \hline

cb5cd1d

Support `\hline`.

Fix CSS

43b5166

Fix lint errors

e3cbf37

ylemkimon reviewed May 11, 2018

View reviewed changes

Add double \hline. Code Cleanup

21c56b3

Merge branch 'master' into hline

1a1b960

Reduce width of screenshot test

dc9703d

Merge branch 'hline' of https://github.com/ronkok/KaTeX into hline

db4195e

Add screenshots

1c24281

ylemkimon reviewed May 12, 2018

View reviewed changes

ronkok added 2 commits May 12, 2018 09:00

Use consumeSpaces, read multiple \hlines

c36bf2e

Merge branch 'hline' of https://github.com/ronkok/KaTeX into hline

a006679

ylemkimon reviewed May 12, 2018

View reviewed changes

Code cleanup

bd95bcc

ylemkimon approved these changes May 12, 2018

View reviewed changes

edemaine added 2 commits May 12, 2018 13:41

Merge branch 'master' into hline

5511b82

Update screenshots

0543f9e

edemaine merged commit 74422e1 into KaTeX:master May 12, 2018

edemaine mentioned this pull request May 12, 2018

More complete support of the array environment #269

Open

17 tasks

ronkok deleted the hline branch May 12, 2018 18:52

ylemkimon mentioned this pull request Aug 18, 2018

Release v0.10.0-rc #1634

Merged

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support \hline #1306

Support \hline #1306

ronkok commented May 11, 2018 •

edited by edemaine

Loading

codecov bot commented May 11, 2018 •

edited

Loading

ronkok commented May 11, 2018

ylemkimon commented May 11, 2018

edemaine commented May 11, 2018

edemaine commented May 11, 2018

ronkok commented May 11, 2018

ronkok commented May 11, 2018

ylemkimon May 11, 2018

ronkok May 11, 2018

ylemkimon May 11, 2018 •

edited

Loading

ronkok commented May 11, 2018

ronkok commented May 11, 2018

edemaine commented May 11, 2018

ronkok commented May 11, 2018

ylemkimon May 12, 2018

edemaine May 12, 2018

ronkok May 12, 2018

ylemkimon May 12, 2018

edemaine May 12, 2018

ronkok May 12, 2018

ronkok commented May 12, 2018

ylemkimon May 12, 2018

ronkok May 12, 2018

ylemkimon May 12, 2018

ronkok May 12, 2018

ylemkimon May 12, 2018

ronkok May 12, 2018

ylemkimon May 12, 2018

ronkok May 12, 2018

ronkok commented May 12, 2018

ronkok commented May 12, 2018

Support \hline #1306

Support \hline #1306

Conversation

ronkok commented May 11, 2018 • edited by edemaine Loading

codecov bot commented May 11, 2018 • edited Loading

Codecov Report

ronkok commented May 11, 2018

ylemkimon commented May 11, 2018

edemaine commented May 11, 2018

edemaine commented May 11, 2018

ronkok commented May 11, 2018

ronkok commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylemkimon May 11, 2018 • edited Loading

Choose a reason for hiding this comment

ronkok commented May 11, 2018

ronkok commented May 11, 2018

edemaine commented May 11, 2018

ronkok commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronkok commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronkok commented May 12, 2018

ronkok commented May 12, 2018

ronkok commented May 11, 2018 •

edited by edemaine

Loading

codecov bot commented May 11, 2018 •

edited

Loading

ylemkimon May 11, 2018 •

edited

Loading