Skip to content
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.

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

Mathtools update #697

Merged
merged 22 commits into from
Jun 8, 2021
Merged

Mathtools update #697

merged 22 commits into from
Jun 8, 2021

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented May 11, 2021

This PR incorporates more of the mathtools package into the current mathtools_package branch, but since I merged develop into this branch, it doesn't make a good PR to mathtools_package, so I'm making it to develop instead. (Even if it had gone to mathtools_package, it wouldn't really have helped, since I ended up breaking the single MathtoolsConfiguration.ts into separate files, so the diff would not really show anything valuable anyway.)

This branch includes the files to make a MathJax Component for mathtools, and adds it into the dependencies and source listings, as well as the AllPackages file (so it will show up in the lab, in particular). If we are going to separate out extra packages, this will need to be changed.

I ended up resetting the cramped style in tables and for explicit style changes (e.g., \scriptstyle), as in actual TeX, but did not do it for boxes, since that is more of a side-effect of how some macros are implemented using hboxes rather than a meaningful difference (as we discussed F2F). I also added a data-cramped attribute to control it on mstyle and table elements so that the \cramped macros could be implemented without much overhead (and I made the mathtools \math*lap macros force non-cramped styles, as they do in actual TeX, since mathtools expects that distinction. But it won't affect other boxes, just these mathtools macros. I think that is a good compromise.

I also moved some code from BaseMethods to ParseUtil that I woudl have had to duplicate for some mathools macros (i.e., handling of under-over constructs, and checking the maximum macro counts). Similarly, I moved the code for adjusting the row spacing in an alignment out of the implementation of \\ (via CrLatex) and into the ArrayItem StackItem object, so that it could be used in mathtools' \shortvdotswithin macro.

I fixed an error with vertical centering (the 'c' option should center on the math axis), and normalized the spacing for some of the xArrows for consistency (and so that all packages define them the same way, as they are defined in more than one package).

I added \boxed and \framboxed macros (which are defined in AMSmath, but were not in MathJax), as they are needed for mathtools' \Aboxed macro.

I added a lookup() utility function that gets a value from an object literal while checking that it is not an inherited value, and using a default value if not found. E.g., {l: 'left', r:'right'}[align] || 'center' would not produce the expected result if align is toString. Using lookup(align, {l: 'left', r: 'right'}, 'center') resolves that problem. (There are a number of places elsewhere in the code that are susceptible to this problem, and should be recoded using lookup(), but that can wait until our next code cleanup).

The BaseMethods.Entry function has been modified to handle text mode in the cases* environment (as it does for the \cases macro). It looks like more changes here than there actually are, since reduced the indenting by exiting the function earlier for one conditional). Viewing the difference with spacing changes ignored might help.

I refactored some code from the newcommand extension to move the parsing of a control sequence name and of optional argument counts into NewcommandUtil, as they are needed in mathtools as well.

Finally, I fixed a problem with CHTML tables where extra cells added to add out the rows wasn't getting the proper CSS if the column spacing was specified. I also simplified some of the CSS for the cells.

And, of course, there are the mathtools files themselves.

I'm sorry that this is such a big set of changes. If you want me to break out some of the changes to the non-mathtools files into separate PR's, I can do that.

@dpvc dpvc added this to the 3.2 milestone May 11, 2021
@dpvc dpvc requested a review from zorkow May 11, 2021 19:50
@dpvc
Copy link
Member Author

dpvc commented May 11, 2021

Oh, I meant to say that the \mathtoolsset macro (which allows changing the mathtools options on the fly) is an interesting idea that could be used to allow changing options in general. For example, \setOptions[package]{options} (some thought would need to be given for a "safe" environment version of this).

@pkra
Copy link
Contributor

pkra commented May 12, 2021

This branch does not compile for me. Here's what I get:

$ npm run compile

> mathjax-full@3.1.4 precompile
> npm run --silent clean:js


> mathjax-full@3.1.4 compile
> npx tsc

ts/input/tex/mathtools/MathtoolsMethods.ts:219:35 - error TS2345: Argument of type 'MmlNode' is not assignable to parameter of type 'MmlMstyle'.
  Type 'MmlNode' is missing the following properties from type 'MmlMstyle': setChildInheritedAttributes, texclass, updateTeXclass, getPrevClass, and 4 more.

219     MathtoolsUtil.setDisplayLevel(mml, style);
                                      ~~~

ts/input/tex/mathtools/MathtoolsMethods.ts:233:35 - error TS2345: Argument of type 'MmlNode' is not assignable to parameter of type 'MmlMstyle'.

233     MathtoolsUtil.setDisplayLevel(mml, style);
                                      ~~~

ts/input/tex/mathtools/MathtoolsMethods.ts:732:38 - error TS2550: Property 'entries' does not exist on type 'ObjectConstructor'. Do you need to change your target library? Try changing the `lib` compiler option to 'es2017' or later.

732     for (const [id, value] of Object.entries(keys)) {
                                         ~~~~~~~


Found 3 errors.

@dpvc
Copy link
Member Author

dpvc commented May 12, 2021

It compiles for me. What version of tsc are you using? I'm at 4.2.4, as listed in the package.json (we updated all dependencies during the 3.1.4 release).

@pkra
Copy link
Contributor

pkra commented May 12, 2021

It compiles for me. What version of tsc are you using? I'm at 4.2.4, as listed in the package.json (we updated all dependencies during the 3.1.4 release).

I had cloned a fresh copy, checked out this branch, run npm i and npm run compile to get the above error.

@pkra
Copy link
Contributor

pkra commented May 12, 2021

I had cloned a fresh copy, checked out this branch, run npm i and npm run compile to get the above error.

I get same result after deleting the lockfile beforehand.

@dpvc
Copy link
Member Author

dpvc commented May 12, 2021

OK, I'm seeing it now. I had made a local change to tsconfig.json to test something and forgot to revert it. Sorry about that. I have pushed a new commit to take care of the issues.

@pkra
Copy link
Contributor

pkra commented May 13, 2021

Thanks, Davide. I can now compile this branch. Unfortunately, the develop branch no longer compiles for me which seems due to #653. I'll leave some details there since I can't think of a better place to put it.

Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

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

Some minor issues and typos. Otherwise OK.

ts/core/MmlTree/MmlNodes/mstyle.ts Show resolved Hide resolved
ts/core/MmlTree/MmlNodes/mtable.ts Outdated Show resolved Hide resolved
throw new TexError('MaxMacroSub1',
'MathJax maximum macro substitution count exceeded; ' +
'is here a recursive macro call?');
} else {
Copy link
Member

Choose a reason for hiding this comment

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

No need for the else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know that you prefer to reduce indentation when possible, but the indentation sometimes has meaning. In this case, the function will produce one of two error messages, neither of which is more important than the other. The indentation of both shows that they are comparable alternatives. Leaving out the "else" would make the second be the "main" message and the first be a subordinate one. It makes the isMacro variable a break0out condition rather than a toggle that determines which of two equal things is being selected. This structure preserves the logic of the function better than having the two messages at different indentations, which gives the second a prominence it doesn't deserve.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unless you strongly object, I'd like to keep it as is.

ts/input/tex/mathtools/MathtoolsConfiguration.ts Outdated Show resolved Hide resolved
ts/input/tex/mathtools/MathtoolsMethods.ts Outdated Show resolved Hide resolved
ts/input/tex/mathtools/MathtoolsMethods.ts Outdated Show resolved Hide resolved
ts/input/tex/mathtools/MathtoolsMethods.ts Outdated Show resolved Hide resolved
ts/input/tex/mathtools/MathtoolsMethods.ts Show resolved Hide resolved
NArrow(parser: TexParser, _name: string, c: string, dy: string) {
parser.Push(
parser.create('node', 'TeXAtom', [
parser.create('token', 'mtext', {}, c),
Copy link
Member

Choose a reason for hiding this comment

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

What is c?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the unicode character for either an up arrow or a down arrow (as provided by the MathtoolsMappings.ts file).

], {width: 0, lspace: '-.5width', voffset: dy}),
parser.create('node', 'mphantom', [
parser.create('token', 'mtext', {}, c)
])
Copy link
Member

Choose a reason for hiding this comment

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

So you are overlaying the same arrow twice? That is a bit like poor man's bold (or whatever it is called).
Is there some future way we could do that "properly" that is also SRE friendly?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not overplayed twice. Notice that the second is inside an mphantom, so it is not visual. It is only used for positioning purposes (the space: '-.5width' below means we are shifting the preceding (mpadded) menclose to the left by half the width of the arrow given in c. That is how the "not" line is centered on the arrow (so that it will work in any font, regardless of the width of the arrow).

Copy link
Member Author

Choose a reason for hiding this comment

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

As for making it SRE friendly, there don't seem to be unicode positions for up or down arrows with a diagonal slash (otherwise I'd just have called on those). The closest is U+2908 and U+2909, which are explicit about having a horizontal slash.

@pkra
Copy link
Contributor

pkra commented Jun 2, 2021

The mathtools_update and the amsmath_update branches have a merge conflict (in ParseUtils due to the change to Em()).

@dpvc
Copy link
Member Author

dpvc commented Jun 2, 2021

@pkra, yes, the two branches do produce a conflict. That will have to be resolved after one of them in merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants