-
Notifications
You must be signed in to change notification settings - Fork 108
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
more correct emulation of \box and \copy primitives #2480
base: master
Are you sure you want to change the base?
Conversation
@@ -227,6 +227,23 @@ sub shiftValue { | |||
assign_internal($self, 'value', $key, [], 'global') unless $$vtable{$key}[0]; | |||
return shift(@{ $$vtable{$key}[0] }); } | |||
|
|||
sub removeValue { |
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.
You could use this code to implement scope => 'samelevel'
assignments (with a better name, if you can find it), which would be equivalent to removeValue
when assigning \undef
. Should you one day discover another assignment 'in place'.
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.
That's also a possibility. My main worry is that assign_internal
is already somewhat entangled, so adding more to it just makes life harder... Will wait for feedback from Bruce.
I enjoy the idea of having a dedicated removal method, for what that's worth. But since the removal isn't global, the name is a bit misleading. It's really UnbindLastValueAssignment
, but this is not a Java project...
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.
That's also a possibility. My main worry is that
assign_internal
is already somewhat entangled, so adding more to it just makes life harder... Will wait for feedback from Bruce.
I was thinking the opposite: this code is very similar to assign_internal
and would be better placed there for ease of maintainance... ?
I am also wondering if removing values has other side effects, e.g. will this make \ifcsname
false? That's not really possible in TeX, is it? (This has no real consequence of course since removeValue
is only used for the box registers.)
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 very similar to assign_internal and would be better placed there for ease of maintainance... ?
Only if it can be DRY ("do not repeat yourself"). Otherwise just growing the body of the function will make it more impenetrable.
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.
Going back to the DRY idea, couldn't this code be folded into the global assignment? If I parse correctly, your new code and global assignment differ by only two lines (last;
and $$frame{value}{$key} = 0
). So a samelevel
scope could be implemented by adding the line last if $samelevel
in assign_internal
(but it wouldn't remove the value as you do here, of course).
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 open to the idea.
Would likely seek a slightly better name ... To fit better with global
and local
, maybe initial
is a good keyword? Fishing for inspiration from CSS initial
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 agree that samelevel
is a terrible name... let's think about how one could document the new scope. The assignment in scope samelevel
means something like:
- in the last active scope where
$name
was assigned a value (solast
?) - in the scope where
$name
received its current value/the current definition of$name
was made (socurrent_def
orcurrent_value
?) - in the same scope where
$name
was assigned the current value (sosame
?) - in the scope where
$name
was initially assigned the current value (soinitial
?)
I am not totally convinced by initial
. It sounds too similar to global.
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.
Well, the primary purpose we'll use this for is still "removing" or "unsetting" the value (CSS has an "unset" property related to initial which also comes to mind). So having an associated scope=>"unset"
could also be intuitive when looked at (+ documented) from the right perspective, if the "initial" is too confusing. The thing that bothers me a little is that such calls really don't take a "value" argument, so unifying the interfaces will be inevitably clunky. So I can also see as reasonable to keep the code paths separate.
lib/LaTeXML/Core/State.pm
Outdated
my @frames = @{ $$self{undo} }; | ||
while (@frames) { | ||
$frame = shift(@frames); | ||
if (my $n = $$frame{value}{$key}) { # Undo the bindings, if $key was bound in this frame |
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.
Is this idempotent? I.e. if applied twice, will it remove the two previous bindings? That would be too much for \box
.
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.
Curious idea - we don't yet have a test for that. Something like:
\newbox\myboxD
\setbox\myboxD\vbox{D won't appear outer.}
\begingroup
\setbox\myboxD\vbox{D will appear inner, once.}
\box\myboxD
\box\myboxD
\endgroup
\bye
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.
Added a test. It appears to work as expected?
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.
Scratch that, the test wasn't precise enough -- there is indeed a bug where unbinding twice will pop the outer frame, nice catch @xworld21
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.
Ok, using exists
and a 0
value for the undo counter becomes idempotent. The new test now has correct timing and matches the PDF.
But the code got extra scary. Maybe good, maybe not?
I really need to think about what TeX is really doing, conceptually, to choose a good name & API. I expect that whatever this is should be kept fairly internal, and not exported to |
Ah, not quite there; As mentioned in some other issue, there's the problem of when
|
I'm suspecting that perhaps Font's |
Fixes #2412 .
Two key upgrades:
\copy
\box
, add a newremoveValue
method in State, which (idempotently) unbinds the nearest binding of a value in the undo table.This gets the tests discussed in the issue to pass, and matches the pgf rendering in the PDF build (really great diagnostic by @xworld21 !)
I would need a second opinion by @brucemiller on whether this kind of "removeValue" method makes sense for latexml's State. Its code is inspired by the global assignment mode of
assign_internal
, which will unbind all values in all frames, and then assign a value on the cleaned up 1-element stack.I am wondering if many of the current uses of
AssignValue($key, undef)
are meant as thisRemoveValue
behavior, which could motivate changing local uses of assign_internal with undef to also unbind from the nearest assigned frame -- without creating a new method.Edit: Another direction for refining the code is to add a
'local'
vs'global'
argument forremoveValue
, to indicate whether one or all bindings should be deleted.But I am feeling confident I am following the
\box
unbind semantics accurately now, as explained in the TeXbook. What was missing was unbinding the actual assignment - we were adding a local undef assignment, while also keeping the original assignment in place, when it happened in a higher frame.Thoughts?