-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement AMSMath's \dots #599
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edemaine! This looks really awesome! Thanks for going through the effort of reading through that insane implementation. :) I have a suggestion for how to do the spacing in the comments.
Your code looks really good to me! I'll let @gagern or @kevinbarabash do the final review.
src/functions.js
Outdated
} | ||
} | ||
} | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to conditionally add some positive/negative space here, you could do something like this (I tested this locally, it seems to work fine):
const space = new ParseNode("kern", {
dimension: {
number: 1,
unit: "em",
},
}, context.parser.mode);
const bod = new ParseNode("op", {
limits: false,
symbol: true,
body: thedots,
}, context.parser.mode);
return {
type: "op",
limits: false,
symbol: false,
value: [space, bod],
};
Basically, we're creating a "kern" node and sticking that into our op, which accepts arrays of ParseNode
s. Not sure if this is the "right" way to do this, but it certainly beats making a new group type!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks! That's great that value
can be an array, and kern
is the other feature I was looking for. I'll finish the implementation now.
Hmm, I'm having another problem. Even with the PR code as is, which returns a symbol similar to existing
Relatedly, I'm vaguely worried that my |
Seeing macro support in #605, I wonder if it would make more sense to have But currently functions can't return strings. Does this seem like a good direction to go? If so, I can try. |
@edemaine thanks for the PR. I checked out your branch. The wrong font is definitely a problem. A person could fix Running the following using pdflatex:
produces the following (partial) output:
|
Please ignore the suggestion to use |
Yeah, How crazy would it be to add support for returning a string from a defined function, and having that trigger macro expansion? That's exactly what I want here... ( |
Returning a string from a function would work as a shortcut, but calling something else might be better. Doing macro expansion there might be tricky, since built-in functions are working on the level of parse tree nodes, not input text tokens. What we could do is parse a whole string using a separate parser instance, and then extract the root node out of that tree and include it in the current tree. This should work well for some simple functions, but might be problematic once you have arguments or state to deal with. |
@gagern What if I moved this from a function to a macro, adding support for functions in |
Right now, macros don't have access to the next token, but perhaps we should change that. In the long run we'll want to support things like |
I checked the amsmath code, and it uses \let\@xp=\expandafter
\DeclareRobustCommand{\dots}{%
\ifmmode \@xp\mdots@\else \@xp\textellipsis \fi
}
\def\FN@{\futurelet\@let@token}
\def\mdots@{\FN@\mdots@@} In general, though, I think we might need either one -- a plain |
What exactly does I guess |
Ah, you're right about Anyway, I'd like to request that #605 be extended to allow a JavaScript function in |
I'd not modify #605; it already does contain three distinct features, which might make review slower than it would otherwise be. But having a branch build on #605 and posted as a separate PR either immediately or once that got merged sounds like a good idea. Usually I'd say I'll have a look, but I feel we are opening up new areas of development faster than we are closing them right now, so I'm not sure I'll get to that in a timely manner. |
cf1e646
to
08f40ee
Compare
remove 'var's and add screenshot test
I think this should be closed right @edemaine? |
@kohler Nope, still need to revise this to work with new macro parser. |
Definitely not correct. There were various issues, described above, with building the parse structure directly. Macros should let it actually expand to |
@edemaine I'm going to check out your branch and see what's causing the square dots. |
…g group type 'op'
edemaine#2 fixes the issue with square dots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Square dots issue needs to be resolved.
add group type 'dots' so that we get the right font w/o special casing group type 'op'
I'm not sure why the |
Should be replaced by #794. |
I spent a few hours reading AMSMath's source code for
\dots
. Wow, what a beast! This should be a full implementation (fixing #528), except for one issue:I don't know how to add some positive or negative space left or right of a symbol returned by a function, which is what should happen for
\dotsi
anddotso
in some cases (as described in theTODO
comments). Is there an easy way to do this?