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

Adds \notni character ∌ #710

Merged
merged 3 commits into from
Nov 24, 2017
Merged

Conversation

SuzanneSoy
Copy link
Contributor

No description provided.

@khanbot
Copy link

khanbot commented May 16, 2017

Hey @jsmaniac,

Thanks for the PR! Mind signing our Contributor License Agreement?

When you've done so, go ahead and comment [clabot:check] and I'll check again.

Yours truly,
khanbot

@SuzanneSoy
Copy link
Contributor Author

[clabot:check]

@khanbot
Copy link

khanbot commented May 16, 2017

CLA signature looks good 👍

@ronkok
Copy link
Collaborator

ronkok commented May 17, 2017

I don’t think this approach will work, because the KaTeX-Main font does not contain a glyph for U+220C. What could work is:

defineMacro("\\notni", "\\mathrel{\\rlap{\\kern{0.04em}/}{\\ni}}");

That results in a character that fits in pretty well:
notni

@SuzanneSoy
Copy link
Contributor Author

@ronkok Thanks a lot for the solution! I hadn't thought about the font issue.

@SuzanneSoy SuzanneSoy reopened this May 17, 2017
@SuzanneSoy
Copy link
Contributor Author

Note that the best might be to support \not\ni, which works with AMSMath IIRC, but I'm not sure if KaTeX supports defining macros with arguments. So \notni seems a good compromise (its meaning is clear, it exists in (non-AMS) LaTeX packages).

@ronkok
Copy link
Collaborator

ronkok commented May 17, 2017

@jsmaniac's original code would give a MathML result that is much superior to the macro that I proposed. Perhaps the thing to do is accept the macro for now, but put U+220C on to the font To Do list, along with \euro and \colonequals. Also, the macro code should include a TODO comment that suggests that the macro is temporary and that the font glyph should be used when it is available.

@SuzanneSoy
Copy link
Contributor Author

@ronkok I added the TODO.

@edemaine
Copy link
Member

edemaine commented May 17, 2017

@ronkok Thanks for the input! @jsmaniac Thanks for being responsive to the alternative approach!

If it's not too much trouble, could we see here output from texcmp (available in dockers/texcmp) comparing this symbol to one of the LaTeX versions of the symbol? (This involves adding a tiny test to test/screenshotter/ss_data.yaml.) Let me know if you have trouble running texcmp.

Incidentally, general \not support is in process/almost complete in #140. (Hi @kevinbarabash!) But I think there's still value to this PR, given that it is a supported command in some packages. I guess we could later change the macro to expand to just \not\ni... though that might have slightly inferior spacing?

P.S. You're currently failing tests because of long lines. Run npm test locally to see this output.

@kevinbarabash
Copy link
Member

I'll have a look at what's left to do in #140 this evening.

@SuzanneSoy
Copy link
Contributor Author

SuzanneSoy commented May 17, 2017

@edemaine I know about docker but don't use it in my regular development so I tend to forget the syntax, and google is proving itself unhelpful at telling me how to start a Dockerfile. I tried docker start dockers/texcmp and docker exec -ti dockers/texcmp /bin/sh, but both fail. Care to remind me what's the one-liner?

@edemaine
Copy link
Member

@jsmaniac You can try sudo dockers/texcmp/texcmp.sh YourTestName which does the launching for you. (But see #708) Personally I've had more success with just running node dockers/texcmp/texcmp.js YourTestName.

@SuzanneSoy
Copy link
Contributor Author

@edemaine Sorry, I tried running sudo dockers/texcmp/texcmp.sh Sqrt on my local machine, and inside a fresh Ubuntu 14.04 machine (with the default NodeJS and with NodeJS 6 too), and it fails in both cases with SyntaxError: Use of const in strict mode..

I also tried with the node dockers/texcmp/texcmp.js Sqrt command directly, and it throws module.js:471 throw err;.

You'll have to run texcmp, as I cannot.

@edemaine
Copy link
Member

Regarding your latter error, you probably need to run npm install (which installs all dependencies) first. (Though hard to tell without seeing the actual error message, which is in the line after throw err;.)

I forgot you need to run screenshotter first, though, which I've only gotten to run using Docker. So you might have had trouble anyway.

Anyway, here is what I got with \usepackage{newtxmath} added to test/screenshotter/test.tex:

notni

There's a difference here, but it seems like a consistent difference. Here's a comparison of \in\ni\notin\notni:

notni 1

@edemaine
Copy link
Member

@kevinbarabash Great! Perhaps we should wait to see if #140 is easy to finalize, and then modify this PR accordingly...

@kevinbarabash
Copy link
Member

@jsmaniac I'm getting close with #140. With any luck it should be merged sometime this week and that will unblock this PR. Thanks for your patience.

@kevinbarabash
Copy link
Member

@jsmaniac I finally got \not merged. Can you update your diff to us \not instead of /?

@SuzanneSoy
Copy link
Contributor Author

@kevinbarabash Sure! I'll look into this next week if you don't mind. Thanks for the update!

@edemaine
Copy link
Member

@jsmaniac A reminder on this -- should be straightforward. Also, you can use { and } instead of \bgroup and \egroup.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for updating the diff. Sorry for making you wait so long for \not to be added.

src/symbols.js Outdated
@@ -527,6 +527,7 @@ defineSymbol(math, main, rel, "\u2190", "\\gets");
defineSymbol(math, main, rel, ">", "\\gt");
defineSymbol(math, main, rel, "\u2208", "\\in", true);
defineSymbol(math, main, rel, "\u2209", "\\notin", true);
defineSymbol(math, main, rel, "\u220c", "\\notni", true);
Copy link
Member

@kevinbarabash kevinbarabash Nov 24, 2017

Choose a reason for hiding this comment

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

I think this line should be removed b/c we don't have that symbol in the font yet.

// TODO: The unicode character U+220C ∌ should be added to the font, and this
// macro turned into a propper defineSymbol in symbols.js. That way, the
// MathML result will be much cleaner.
defineMacro("\\notni", "\\not\\ni");
Copy link
Member

Choose a reason for hiding this comment

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

Should the definition be wrapped in a \mathrel?

Copy link
Member

Choose a reason for hiding this comment

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

After doing some checking this doesn't look to be necessary as \ni is already a mathrel and the \not has no effect on the atom type.

reason: we don't have the glyph yet
@kevinbarabash kevinbarabash merged commit d773b17 into KaTeX:master Nov 24, 2017
@kevinbarabash
Copy link
Member

@jsmaniac thanks for the PR. Sorry this took so long to get merged.

@SuzanneSoy
Copy link
Contributor Author

Hi @kevinbarabash, sorry I didn't remove the defineSymbol quick enough. I thought it would allow typing the unicode character in the TeX source, and wanted to ask why it wasn't working when testing with the npm test server. I hadn't realised it was meant to translate the other way round (from the TeX command to the unicode char for MathML).

Thanks for fixing this and merging!

@kevinbarabash
Copy link
Member

@jsmaniac sorry for being over eager to merge your PR. I think in order to support the unicode char for this you'll have to define a macro, see https://github.com/Khan/KaTeX/pull/950/files for some examples.

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

Successfully merging this pull request may close these issues.

5 participants