-
Notifications
You must be signed in to change notification settings - Fork 121
Support calc.sty's sneaking redefinitions #2652
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
base: master
Are you sure you want to change the base?
Conversation
…n optional arg and then reparse the contnet as Type
…{Glue},[Glue] which accept calc syntax if calc.sty is loaded
…ally strip braces when reading values:
…onverting options to string
…f several commands behind the scenes
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.
Also tricky to review... Likely good to merge, but I sent some thoughts your way as well.
my $value = $gullet->readArg(); | ||
return $gullet->readingFromMouth($value, sub { | ||
(LookupValue('calc.sty.ltxml_loaded') | ||
? readCalcExpression($gullet, 'Number') |
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 use of readCalcExpression
will be a namespace headache once it reaches Rust.
The readCalcExpression
sub is only available to execute (in the perl sesne) after the calc package has loaded, but it is already used in the Engine, before any bindings load. Ouch. Luckily there is late binding, but it's not easy to port.
Instead, one can have a method defined in Gullet (so always available early) with some generic name describing its behavior, maybe readArithExpression
or such? And again use it only when calc is loaded. You may want to do this here, or I may just use my comment as a porting guide a bit later.
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.
A different perl trick would have been to refactor the Engine to use a named sub for each reader:
sub named_dimension_read_sub {
# regular reader...
}
DefParameterType('[Dimension]', \&named_dimension_read_sub);
and then have calc.sty.ltxml
re-bind the reader sub:
*LaTeXML::Engine::Base_ParameterTypes::named_dimension_read_sub = sub {
# new calc behavior
}
The main benefit of this approach is performance - there is never a need to do a LookupValue
to check if calc is loaded. And since that will happen during every parameter read, I assume it may even be noticeably faster over large numeric files.
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.
agree, it's pretty sucky all round, and all alternatives suck too.
push(@params, LaTeXML::Core::Parameter->new('Optional', $spec, | ||
extra => [undef, parseParameters($inner_spec, $for)])); } | ||
if(LookupMapping('PARAMETER_TYPES', $spec)) { # If specially defined with []? | ||
push(@params, LaTeXML::Core::Parameter->new($spec, $spec, optional => 1)); } |
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 am now starting to get confused about having two ways of specifying these. I recall we also had a prefix keyword Optional
, as with OptionalNumber
, as well as [Number]
.
Shouldn't we be normalizing towards one or the other? Is there a risk that a lookup for OptionalNumber
doesn't find a special mapping for [Number]
or the other way around?
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.
Good point, but there's are Lots of the latter
\makebox[1in]{Foo} | ||
\makebox[{1in}]{Foo} | ||
\makebox[{{1in}}]{Foo} | ||
\makebox[0.5in*2]{Foo} |
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.
How about some +
and -
tests as well?
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.
The point was to verify that it's using the calc method, not to verify every operator in every command
Here's a tricky one! LaTeX defines several utility macros
\setcounter,\setlength
(along with\addtocounter,\addtolength
and others) which basically just assign registers to a Number or Dimension. They are used fairly extensively, and also in several other packages, notably graphicx. Along comes the calc package which supports simple arithmetic calculations of counters and lengths. However, it redefines\setcounter, \setlength
so that automagically, most normal LaTeX commands (eg\makebox, \includegraphics
) suddenly also accept calc syntax to assign numbers and lengths, not to mention that they also ignore extra braces wrapping the quantities.These enhancements should only affect quantities that get passed around as macro arguments, eg.
{Dimension}
, but low-level TeX usage likeDimension
should not be affected. So, the idea here is to define explicitly parameter types{Number}, {Dimension}
. Previously these would have read a regular arg, then reparsed it as a Number, but now if calc has been loaded, will reparse as calc expressions of the correct type. Similarly, optional versions[Number], [Dimension]
are defined.[I thought of having calc simply redefine the parameter types, but internal parameter data will already have been incorporated into Definition's Parameter objects]