Conversation
zorkow
left a comment
zorkow
left a comment
There was a problem hiding this comment.
I am confused about the item (and the subsequent filter method). Please see my comments.
| } | ||
| if (mml.isKind('mspace')) { | ||
| // | ||
| // If the space is in an mstyle, wrap it in an mrow so we can test is scriptlevel. |
| item.Push(mml); | ||
| } | ||
| // | ||
| // Save the item for alter post-processing |
| } | ||
| if (mml.isKind('mspace')) { | ||
| // | ||
| // If the space is in an mstyle, wrap it in an mrow so we can test is scriptlevel. |
There was a problem hiding this comment.
This was the comment was source of confusion for me. Effectively this if will only fire, if the previous unwrapping if has fired.
There was a problem hiding this comment.
No, it can also fire if there is an explicit mspace, e.g., from \hskip or \kern.
There was a problem hiding this comment.
Yes, this if only happens with the first if, true.
| if (item.isKind('mml') && item.Size() === 1) { | ||
| let mml = item.First; | ||
| // | ||
| // Space macros like \, wrap with an mstyle to set scriptlevel=0 (so size is independent of level) |
There was a problem hiding this comment.
Actually, "are wrapped" is probably better.
| // | ||
| this.factory.configuration.addNode('nonscript', item.First); | ||
| } | ||
| } |
There was a problem hiding this comment.
As you can see from my comments above, I find the structure very confusing.
So what do we want to do?
- Any
spaceshould be marked asnonscript - If the
spaceis wrapped in anmstylewe want to give it an extramrowand mark that.
(There is no possibility that it is wrapped in multiplemstyles?)
Can you restructure the code somewhat to reflect this, even if it introduces some duplications? And/or update the comments?
There was a problem hiding this comment.
I think what confused me most here is that they are all called mml.
| for (const mml of data.getList('nonscript')) { | ||
| // | ||
| // If we are in script or script-script style | ||
| // remove the space (either mspace or mrow containing it) |
There was a problem hiding this comment.
If I understand the item correctly, if the item is an mrow then we have added it.
There was a problem hiding this comment.
Right. Since only actual mspace elements or the mrow elements that we added above ever get added to the nonscript list, if we have an mrow, we know it is one that we added.
zorkow
left a comment
zorkow
left a comment
There was a problem hiding this comment.
Thanks for the explanations. I think good commenting/structuring for items is important to preserve understanding what they do. Having refactored many item objects and often spent some time working through all the cases, I know what I am talking about (Of course this is only a sample set of one. Still...)
I make a couple of suggestions in code.
zorkow
left a comment
zorkow
left a comment
There was a problem hiding this comment.
Here's a few suggested comments for the post filter method.
|
@zorkow, OK, I've added more comments that I hope will help the situation. See if that fits the bill. |
zorkow
left a comment
zorkow
left a comment
There was a problem hiding this comment.
Thanks, that makes it a lot clearer.
2 participants
This PR implements the
\nonscriptcommand, which removes the following spacing command (like\hskip,\kern,\hspace, etc.). This is used int macro definitions to handle not use spacing in scripts, when given explicitly.This is handled by using a new StackItem for the
\nonscriptthat checks what is pushed after it, and if anmspace(or anmspaceinside anmstyle, as produced by\,and similar macros), then we add the space into a list to be post-filtered (once thescriptlevelis known). The case of ammstylecontaining anmspaceis handled by wrapping it in anmrowso that we can test thescriptlevel(since themstylewill havescriptlevel="0"set). Themrowis removed in the post-filter if the space is being retained (otherwise themrowtogether with its contents are removed).