Skip to content

allow sizing commands inside optional groups #885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
k4b7 merged 3 commits into from
Oct 3, 2017
Merged

allow sizing commands inside optional groups #885

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bfc125
allow sizing commands inside optional groups
Sep 15, 2017
326f17f
allow color, old font, and style commands inside optional groups
Sep 30, 2017
be8c0c9
Merge branch 'master' into sizing_in_option_groups
k4b7 Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"jest": "^20.0.4",
"jest-serializer-html": "^4.0.0",
"js-yaml": "^3.3.1",
"json-stable-stringify": "^1.0.1",
"jspngopt": "^0.2.0",
"less": "~2.7.1",
"less-plugin-clean-css": "^1.5.1",
Expand Down
20 changes: 11 additions & 9 deletions src/Parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export default class Parser {
if (breakOnInfix && functions[lex.text] && functions[lex.text].infix) {
break;
}
const atom = this.parseAtom();
const atom = this.parseAtom(breakOnTokenText);
if (!atom) {
if (!this.settings.throwOnError && lex.text[0] === "\\") {
const errorNode = this.handleUnsupportedCmd();
Expand Down Expand Up @@ -306,12 +306,13 @@ export default class Parser {
/**
* Parses a group with optional super/subscripts.
*
* @param {"]" | "}"} breakOnTokenText - character to stop parsing the group on.
* @return {?ParseNode}
*/
parseAtom() {
parseAtom(breakOnTokenText) {
Copy link
Copy Markdown
Collaborator

@marcianx marcianx Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Mind adding a @param documenting the type of the new parameter?

// The body of an atom is an implicit group, so that things like
// \left(x\right)^2 work correctly.
const base = this.parseImplicitGroup();
const base = this.parseImplicitGroup(breakOnTokenText);
Copy link
Copy Markdown
Collaborator

@marcianx marcianx Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth adding a comment somewhere as to why it's okay not to pass this parameter down to other methods below. The reason seems to be subtle: as handleSupSubscript can call parseGroup which can call parseExpression, but parseExpression is only called in parseGroup only if a { or [ is encountered, opening a deeper layer. But I haven't yet done an exhaustive check.

I'm also fine with completely deferring this if we intend to formalize the various non-obvious invariants in this file some point to aid comprehensibility so that they aren't violated in the future.

Copy link
Copy Markdown
Member Author

@k4b7 k4b7 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment. handleSupSubscript should always be parsing non-optional groups afaict because x^[2] isn't valid (or rather it's valid it's treated as x^{[}2]).


// In text mode, we don't have superscripts or subscripts
if (this.mode === "text") {
Expand Down Expand Up @@ -423,9 +424,10 @@ export default class Parser {
* small text {\Large large text} small text again
* It is also used for \left and \right to get the correct grouping.
*
* @param {"]" | "}"} breakOnTokenText - character to stop parsing the group on.
* @return {?ParseNode}
*/
parseImplicitGroup() {
parseImplicitGroup(breakOnTokenText) {
Copy link
Copy Markdown
Collaborator

@marcianx marcianx Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, please.

const start = this.parseSymbol();

if (start == null) {
Expand Down Expand Up @@ -484,7 +486,7 @@ export default class Parser {
} else if (utils.contains(Parser.sizeFuncs, func)) {
// If we see a sizing function, parse out the implicit body
this.consumeSpaces();
const body = this.parseExpression(false);
const body = this.parseExpression(false, breakOnTokenText);
Copy link
Copy Markdown
Collaborator

@marcianx marcianx Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this only apply to this block and not to the rest below: styleFuncs, oldFontFuncs, and \color? Are they not similarly affected? Or do none of our functions currently want to allow these in optional groups?
Otherwise, this PR mostly LGTM.

Copy link
Copy Markdown
Member Author

@k4b7 k4b7 Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I was fixing only the issue, but I'm sure there are places where KA also uses \color inside of optional groups (apparently LaTeX supports this too). LaTeX also supports style functions within optional blocks too.

return new ParseNode("sizing", {
// Figure out what size to use based on the list of functions above
size: utils.indexOf(Parser.sizeFuncs, func) + 1,
Expand All @@ -493,7 +495,7 @@ export default class Parser {
} else if (utils.contains(Parser.styleFuncs, func)) {
// If we see a styling function, parse out the implicit body
this.consumeSpaces();
const body = this.parseExpression(true);
const body = this.parseExpression(true, breakOnTokenText);
return new ParseNode("styling", {
// Figure out what style to use by pulling out the style from
// the function name
Expand All @@ -504,7 +506,7 @@ export default class Parser {
const style = Parser.oldFontFuncs[func];
// If we see an old font function, parse out the implicit body
this.consumeSpaces();
const body = this.parseExpression(true);
const body = this.parseExpression(true, breakOnTokenText);
if (style.slice(0, 4) === 'text') {
return new ParseNode("text", {
style: style,
Expand All @@ -522,7 +524,7 @@ export default class Parser {
if (!color) {
throw new ParseError("\\color not followed by color");
}
const body = this.parseExpression(true);
const body = this.parseExpression(true, breakOnTokenText);
return new ParseNode("color", {
type: "color",
color: color.result.value,
Expand Down Expand Up @@ -834,7 +836,7 @@ export default class Parser {
if (this.nextToken.text === (optional ? "[" : "{")) {
// If we get a brace, parse an expression
this.consume();
const expression = this.parseExpression(false, optional ? "]" : null);
const expression = this.parseExpression(false, optional ? "]" : "}");
Copy link
Copy Markdown
Member Author

@k4b7 k4b7 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this was null.

Copy link
Copy Markdown
Member

@edemaine edemaine Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing always ends on }, so I'm not sure specifying "}" is necessary... hence the null. (It's been a while since I've looked at this code, so I forget the exact details...)

Copy link
Copy Markdown
Member Author

@k4b7 k4b7 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels nice to have symmetry the ? "[" : "{" check above.

const lastToken = this.nextToken;
// Make sure we get a close brace
this.expect(optional ? "]" : "}");
Expand Down
178 changes: 178 additions & 0 deletions test/__snapshots__/katex-spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`An implicit group parser within optional groups should work style commands \\sqrt[\\textstyle 3]{x} 1`] = `
[
{
"type": "sqrt",
"mode": "math",
"value": {
"type": "sqrt",
"body": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "mathord",
"mode": "math",
"value": "x"
}
]
},
"index": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "styling",
"mode": "math",
"value": {
"style": "text",
"value": [
{
"type": "textord",
"mode": "math",
"value": "3"
}
]
}
}
]
}
}
}
]
`;

exports[`An implicit group parser within optional groups should work with \\color: \\sqrt[\\color{red} 3]{x} 1`] = `
[
{
"type": "sqrt",
"mode": "math",
"value": {
"type": "sqrt",
"body": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "mathord",
"mode": "math",
"value": "x"
}
]
},
"index": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "color",
"mode": "math",
"value": {
"type": "color",
"color": "red",
"value": [
{
"type": "textord",
"mode": "math",
"value": "3"
}
]
}
}
]
}
}
}
]
`;

exports[`An implicit group parser within optional groups should work with sizing commands: \\sqrt[\\small 3]{x} 1`] = `
[
{
"type": "sqrt",
"mode": "math",
"value": {
"type": "sqrt",
"body": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "mathord",
"mode": "math",
"value": "x"
}
]
},
"index": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "sizing",
"mode": "math",
"value": {
"size": 5,
"value": [
{
"type": "textord",
"mode": "math",
"value": "3"
}
]
}
}
]
}
}
}
]
`;

exports[`An implicit group parser within optional groups should work wwith old font functions: \\sqrt[\\tt 3]{x} 1`] = `
[
{
"type": "sqrt",
"mode": "math",
"value": {
"type": "sqrt",
Copy link
Copy Markdown
Collaborator

@marcianx marcianx Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) I don't know whether an arbitrary strict order is enforced here, but if not, moving this up would make it much easier to see which level it is in.

Copy link
Copy Markdown
Member Author

@k4b7 k4b7 Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can set up a custom formatter for the snapshots to always put "type" first.

"body": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "mathord",
"mode": "math",
"value": "x"
}
]
},
"index": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "font",
"mode": "math",
"value": {
"body": {
"type": "ordgroup",
"mode": "math",
"value": [
{
"type": "textord",
"mode": "math",
"value": "3"
}
]
},
"font": "mathtt"
}
}
]
}
}
}
]
`;
44 changes: 44 additions & 0 deletions test/katex-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/* global expect: false */
/* global it: false */
/* global describe: false */
import stringify from 'json-stable-stringify';

import buildMathML from "../src/buildMathML";
import buildTree from "../src/buildTree";
Expand All @@ -13,6 +14,27 @@ import Options from "../src/Options";
import Settings from "../src/Settings";
import Style from "../src/Style";

const typeFirstCompare = (a, b) => {
if (a.key === 'type') {
return -1;
} else if (b.key === 'type') {
return 1;
} else {
return a.key < b.key ? -1 : 1;
}
};

const serializer = {
print(val) {
return stringify(val, {cmp: typeFirstCompare, space: ' '});
},
test() {
return true;
},
};

expect.addSnapshotSerializer(serializer);

const defaultSettings = new Settings({});
const defaultOptions = new Options({
style: Style.TEXT,
Expand Down Expand Up @@ -520,6 +542,28 @@ describe("An implicit group parser", function() {
expect(sizing.type).toEqual("sizing");
expect(sizing.value.value.length).toBe(1);
});

describe("within optional groups", () => {
it("should work with sizing commands: \\sqrt[\\small 3]{x}", () => {
const tree = stripPositions(getParsed("\\sqrt[\\small 3]{x}"));
expect(tree).toMatchSnapshot();
});

it("should work with \\color: \\sqrt[\\color{red} 3]{x}", () => {
const tree = stripPositions(getParsed("\\sqrt[\\color{red} 3]{x}"));
expect(tree).toMatchSnapshot();
});

it("should work style commands \\sqrt[\\textstyle 3]{x}", () => {
const tree = stripPositions(getParsed("\\sqrt[\\textstyle 3]{x}"));
expect(tree).toMatchSnapshot();
});

it("should work wwith old font functions: \\sqrt[\\tt 3]{x}", () => {
const tree = stripPositions(getParsed("\\sqrt[\\tt 3]{x}"));
expect(tree).toMatchSnapshot();
});
});
});

describe("A function parser", function() {
Expand Down
Binary file modified test/screenshotter/images/Sizing-chrome.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/screenshotter/images/Sizing-firefox.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion test/screenshotter/ss_data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ SizingBaseline:
pre: x
post: M
Sizing: |
{\Huge x}{\LARGE y}{\normalsize z}{\scriptsize w}
{\Huge x}{\LARGE y}{\normalsize z}{\scriptsize w}\sqrt[\small 3]{x+1}
Smash: \left( X^{\smash 2} \right) \sqrt{\smash[b]{y}}
Spacing: ^3+[-1][1-1]1=1(=1)\lvert a\rvert~b
Sqrt: |
Expand Down