Implement \TextOrMath, \@secondoftwo #1024

edemaine · 2017-12-16T15:52:30Z

Partially addressing #1003.

Parser now tells MacroExpander about mode switching. (This seems preferable to a circular reference between Parser and MacroExpander.)
Implement \TextOrMath
Improve when we switch modes so that this actually works, in all cases except single-symbol arguments.
Add \@secondoftwo to match \@firstoftwo.
Add comments documenting all the conditional macros

@FirstOfTwo

* Parser now tells MacroExpander about mode switching. (This seems preferable to a circular reference between Parser and MacroExpander.) * Implement \TextOrMath * Improve when we switch modes so that this actually works, in all cases except single-symbol arguments. * Add \@secondoftwo to match \@FirstOfTwo. * Add comments documenting all the conditional macros

kevinbarabash

Nice work! I left a few comments and questions, but no major concerns.

kevinbarabash · 2017-12-16T22:32:27Z

src/MacroExpander.js

+    /**
+     * Switches between "text" and "math" modes.
+     */
+    switchMode(newMode) {


newMode: Mode

kevinbarabash · 2017-12-16T22:39:52Z

src/Parser.js

            const outerMode = this.mode;
            this.switchMode("math");
+            // Expand next symbol now that we're in math mode.
+            this.consume();


This had to be moved so that the gullet is in the right mode before expansion, cool!

kevinbarabash · 2017-12-16T22:40:29Z

src/Parser.js

-            this.expect("$", true);
+            // We can't expand the next symbol after the $ until after
+            // switching modes back.  So don't consume within expect.
+            this.expect("$", false);


Excellent comment!

kevinbarabash · 2017-12-16T22:52:36Z

src/Parser.js

@@ -746,29 +752,25 @@ export default class Parser {
     *
     * @return {?ParsedFuncOrArgOrDollar}
     */
-    parseGroupOfType(innerMode, optional) {
-        const outerMode = this.mode;
+    parseGroupOfType(type, optional) {


nice change

kevinbarabash · 2017-12-16T22:53:28Z

src/Parser.js

-        return res;
+        // By the time we get here, type is one of "text" or "math".
+        // Specify this as mode to parseGroup.
+        return this.parseGroup(optional, type);


It'll be nice when we can guarantee this with flow.

kevinbarabash · 2017-12-16T23:00:18Z

src/Parser.js

+            // Switch mode back before consuming symbol after close brace
+            if (mode) {
+                this.switchMode(outerMode);
+            }


I guess this is why we have to pass in the mode instead of calling switchMode outside of parseGroup.

kevinbarabash · 2017-12-16T23:03:41Z

src/Parser.js

@@ -976,6 +989,7 @@ export default class Parser {
                    firstToken.range(lastToken, firstToken.text));
        } else {
            // Otherwise, just return a nucleus, or nothing for an optional group
+            // TODO: Not yet switching modes for single-symbol group.


How much work is involved in this?

I'm not actually sure how to do it -- it's pretty challenging, because by the time parseGroup gets called, nextToken already has the single-symbol group, which means that it already got expanded in the wrong mode. In the old way of doing things, we could "unget" the symbol, change modes, and reget it, but that seems challenging/impossible to do if there are side-effects/errors caused by the first execution in the wrong mode. So I think we'd need to call parseGroup before actually reading the first symbol of the group, which might cause a rather big change across all of Parser.js.

So in summary, I'm not sure how much work it'll be. It's surely possible, but hard. I thought I'd commit what I have so far, and open another issue for fixing this further. On the plus side, it took me a lot of playing around with tests before I could even get this situation to arise, and in some sense #924 is even more important than this missing case. So maybe this PR is good to merge for now? Up to you -- I can look at this more when I have time.

kevinbarabash · 2017-12-17T01:14:25Z

test/katex-spec.js

@@ -2672,11 +2672,61 @@ describe("A macro expander", function() {
        expect("\\@ifnextchar!{yes}{no}?!").toParseLike("no?!");
    });

-    it("\\@firstoftwwo should consume star but nothing else", function() {
+    it("\\@ifstar should consume star but nothing else", function() {


kevinbarabash · 2017-12-17T01:15:11Z

test/katex-spec.js

+        compareParseTree("\\mode\\text{\\mode$\\mode$\\mode}\\mode",
+            "math\\text{text$math$text}math",
+            {"\\mode": "\\TextOrMath{text}{math}"});
+    });


Great test suite!

kevinbarabash · 2017-12-17T01:17:56Z

test/katex-spec.js

+    // which doesn't even treat all four letters as an argument.
+    //it("\\TextOrMath should work in a macro passed to \\text", function() {
+    //    compareParseTree("\\text\\mode", "\\text{text}",
+    //        {"\\mode": "\\TextOrMath{text}{math}"});


What does this currently result in?

It results in "m" formatted in text mode followed by "ath" formatted in math mode. So it's wrong in two ways: it thinks it's in math mode when it's actually in text mode, and the argument is just the first character instead of all four. This is a bug with interaction between MacroExpander and Parser, essentially #924 which I'll update.

kevinbarabash · 2017-12-20T03:44:04Z

Code changes look good to me, but there's something causing the Colorbox and DashesAndQuotes screenshot tests to fail.

ColorBox:

The x should be in typewriter.

DashesAndQuotes:

Three dashes in a row should be an em dash.

edemaine · 2017-12-20T18:58:16Z

Wow, the power of tests. DashesAndQuotes was a nice easy fix pointing out incorrect mode detection for the ligatures (obvious in hindsight). Unfortunately, the Colorbox issue is exactly the missing part of this PR, switching to text mode for a single-symbol argument (here, x). So maybe I should try to do that before this PR gets merged; otherwise, this PR would regress behavior a bit.

kevinbarabash · 2017-12-22T04:17:42Z

@edemaine the following seems to be sufficient to fix the ColorBox screenshot test:

        } else {
            // Otherwise, just return a nucleus, or nothing for an optional group
            if (mode) {
                this.switchMode(mode);
            }
            const result = optional ? null : this.parseSymbol();
            if (mode) {
                this.switchMode(outerMode);
            }
            return result;
        }

This fixes the Colorbox screenshot test.

kevinbarabash

LGTM

kevinbarabash · 2017-12-22T04:43:19Z

There may still be other places where we need to switch modes before calling parseSymbol, but we can deal with them in a separate PR as necessary.

edemaine · 2017-12-22T14:59:11Z

@kevinbarabash Thanks for investigating! This partially fixes the single-symbol support, basically whenever the single symbol is not a macro (not expanded by the MacroExpander). I'm glad this makes it mergable (backwards-compatible); now I'll open an issue for the remaining challenge of single-symbol macro.

kevinbarabash reviewed Dec 17, 2017

View reviewed changes

Define type for switchMode

3ebdd30

Fix mode detection for ligatures

cecd839

Switch mode before the call to parseSymbol() in parseGroup

259a8cd

This fixes the Colorbox screenshot test.

kevinbarabash approved these changes Dec 22, 2017

View reviewed changes

Merge branch 'master' into textormath

7bd4f59

kevinbarabash merged commit c30edaa into KaTeX:master Dec 22, 2017

edemaine mentioned this pull request Dec 22, 2017

Single-symbol macro arguments #1027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement \TextOrMath, \@secondoftwo #1024

Implement \TextOrMath, \@secondoftwo #1024

edemaine commented Dec 16, 2017 •

edited

Loading

kevinbarabash left a comment

kevinbarabash Dec 16, 2017

edemaine Dec 19, 2017

kevinbarabash Dec 16, 2017

kevinbarabash Dec 16, 2017

kevinbarabash Dec 16, 2017

kevinbarabash Dec 16, 2017

kevinbarabash Dec 16, 2017

kevinbarabash Dec 16, 2017

edemaine Dec 19, 2017

kevinbarabash Dec 17, 2017

kevinbarabash Dec 17, 2017

kevinbarabash Dec 17, 2017

edemaine Dec 19, 2017 •

edited

Loading

kevinbarabash commented Dec 20, 2017

edemaine commented Dec 20, 2017

kevinbarabash commented Dec 22, 2017

kevinbarabash left a comment

kevinbarabash commented Dec 22, 2017

edemaine commented Dec 22, 2017

Implement \TextOrMath, \@secondoftwo #1024

Implement \TextOrMath, \@secondoftwo #1024

Conversation

edemaine commented Dec 16, 2017 • edited Loading

kevinbarabash left a 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

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

edemaine Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

kevinbarabash commented Dec 20, 2017

edemaine commented Dec 20, 2017

kevinbarabash commented Dec 22, 2017

kevinbarabash left a comment

Choose a reason for hiding this comment

kevinbarabash commented Dec 22, 2017

edemaine commented Dec 22, 2017

edemaine commented Dec 16, 2017 •

edited

Loading

edemaine Dec 19, 2017 •

edited

Loading