-
Notifications
You must be signed in to change notification settings - Fork 105
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
Support for measurement arguments. #166
Conversation
This functionality can allow arguments of commands like \raise -0.5em{T} and \kern-2.7 pt J to work, with or without the use of curly braces.
Fixed the exception thrown when inserting the short forms of some space atoms.
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.
To summarize: I don't currently think this PR adds any value to the project. Thanks a lot for your effort on this, but I can't see it being merged in its current state. I'll try to supply my own solution for #139 in my spare time.
@@ -12,6 +12,8 @@ public class SVGConverter | |||
{ | |||
private int m_nestedLevel = 0; | |||
|
|||
public Queue<Brush> GeometryBrushes = new Queue<Brush>(); |
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.
All changes in this file belong to a separate PR #162. They shouldn't be here.
measurementValue = null; | ||
return false; | ||
} | ||
} |
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.
I'm sorry but I can't see how these changes contribute to anything. This method is never called.
if (!this.TempFormulas.ContainsKey(name)) | ||
{ | ||
this.TempFormulas.Add(name, formula); | ||
} |
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.
This code is supposed to fix #139 but I don't think this is the right approach: it deliberately ignores the formulas introduced by different commands and merges them into one. I don't think that's right, even if it accidentally works for simple case with \,
→ \thinspace
.
In other words, <CreateFormula name="f">
in \thinspace
should have no relation to <CreateFormula name="f">
in \,
. Each <Formula>
block should get its own name scope.
var result = this.TempFormulas[name]; | ||
Debug.Assert(result != null); | ||
this.Result = result; | ||
if (this.TempFormulas.ContainsKey(name)) |
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.
I don't think we should have this check here either. If the user have defined a <Return>
block referencing a nonexisting <Formula>
, then it should be an error. Such cases shouldn't be silently ignored.
This functionality can allow arguments of commands like \raise -0.5em{T} and \kern-2.7 pt J to work, with or without the use of curly braces.