Conversation
… adjusting a used box later, it should only modify colors that had been the defaults, and it should create a copy of the boxes rather than modify them in place
…with respect to color and background color adjustment
| elsif ($type =~ /^LaTeXML::Core::(?:Box|List|Whatsit|Alignment)$/) { # blessed hashes! | ||
| $$cache{$box} = $adj = bless {%$box}, $type; # Cache BEFORE recursion! | ||
| map { $$adj{$_} = adjustBoxColor_rec($fg, $bg, $cache, $$box{$_}); } keys %$box; } | ||
| elsif($type eq 'LaTeXML::Common::Font'){ |
There was a problem hiding this comment.
A Font shouldn't make it as an argument to a box-oriented subroutine, surely? This makes porting this to typed languages rather painful, but it is also rather confusing to think about. Why is a font object passed in as $box?
There was a problem hiding this comment.
You're right, bad naming. It's oriented to (digested) objects, not just boxes. Fixed.
There was a problem hiding this comment.
Is a Font object a Digested object?
I do not understand the design here.
There was a problem hiding this comment.
A Font object is inside of a Digested object, and it is exactly the Font objects inside of the digested objects that we're adjusting.
dginev
left a comment
dginev
left a comment
There was a problem hiding this comment.
I have read this little function five times now and I still find it confusing. I'll approve the PR, but I am really not looking forward to porting it.
Did you check if the tests from my older PR all continued to pass? I noticed that you didn't carry them over.
|
Sure, I did, and more! (there were other tests in your PR, but those were covered in #2527) |
|
Maybe this improved comment helps a little? |
| {\color{red}\colorbox{green}{\copy\mybox}} | ||
|
|
||
| \setbox\mybox=\hbox{Red on Green, but \color{green}\colorbox{blue}{Green on Blue!}} | ||
| {\color{red}\colorbox{green}{\copy\mybox}} |
There was a problem hiding this comment.
I suggest adding a test where the color is set outside of the box, before \setbox, and likewise before using \copy.
3 participants
In TeX,
\setboxdoesn't record the current color or background in the box it saves, but when the box is used, it inherits the then current colors. In LaTeXML, we do record those colors (perversely, within the font). So, we have to be careful to preset them to defaults before\setboxcreates the box content. When using the box we should update only those colors which were the inherited defaults. Moreover, we must create a copy of the boxes, rather than modifying them in place, since some commands (eg.\copy) still keeps the box in the register. This PR achieves those aims.Fixes #2455
And is a better alternative to the approaches in:
Closes #2480
Closes #2456