LaTeXImage.pm #606
LaTeXImage.pm #606
Conversation
There was a problem hiding this comment.
There are a few issues here.
Also, please remove the "legacy" comments. The tikz code is not legacy. That is an invalid declaration as that code is still valid and has not been replaced with new code that takes care of its functionality. If anything the new code is experimental.
lib/LaTeXImage.pm
Outdated
| if ($self->environment->[0] eq 'tikzpicture') { | ||
| return &$self('texPackages', ['tikz',@$_[0]]) if ref($_[0]) eq "ARRAY"; | ||
| return &$self('texPackages', ['tikz']); | ||
| } |
There was a problem hiding this comment.
This code does not work. When the problem author calls this method for the object with an array of packages the texPackages is set to be a reference to the array containing the 'tikz' tex package as well as the additional tex packages requested. However, when this method is later called by this perl package in the header subroutine the additional packages are wiped out and replaced with only the 'tikz' tex package as at that point the array argument is not present.
I recommend that you delete this added code, and add the 'tikz' package as before on what is now line 151 conditional on $self->environment->[0] eq 'tikzpicture'.
There was a problem hiding this comment.
Should there be any attempt to avoid loading tikz twice? We will automatically load it if environment->[0] eq 'tikzpicture'. But the author might include tikz in texPackages() too. The complication is they might load it as a string or as an array ref that includes package options. So it's a little harder to check if the author is explicitly loading it too.
If it means anything, environment only exists so that PGtikz.pl can use it to maintain automatic wrapping of the tikzpicture environment. If I were making this from scratch, I would just expect authors to write \begin{...}...\end{...} in the tex() part. So I don't expect authors to use environment directly. But it's there, so they might.
There was a problem hiding this comment.
It would not be a bad idea to check to avoid loading it twice.
There was a problem hiding this comment.
I did as you suggested, and then also implemented a check for if tikz is already used. Maybe you can see a simpler way. I turned the array of texPackages (a mix of strings and array refs) into a hash. Each key is a tex package name the author provided. The value from each key is 0 or 1, depending on if it matches 'tikz'. Then the check is boolean on $istikzused{'tikz'}. The result will either be undefined if the author did not include tikz, or 1 if they did.
|
The two examples provided works, however, I'm trying to run some existing problems that worked with the tikzImage code before, and am unsuccessful. Clearly, I need to adapt to the new API, however, this will need to be done with any tikzImage code currently written, correct? Are we just hoping that there aren't a lot of problems out there yet? @Alex-Jordan, do you have a plan for this? |
|
@pstaabp: No, existing problems should not need to be changed. They should just work. |
|
@drgrice1 Are your original tikzImage problems working under this branch? |
|
Most of them are working. However, any problem that adds an additional tex package fails. See my review comments. |
|
The ones that are failing are adding If we include this package, then there are two ways of including tikz images. That seems confusing to authors. If we are keeping the current tikz image macro ( |
|
I believe it is a good move. The PGtikz.pl macro takes care of adding the things needed for tikz images, and so is more convenient for is purpose. Also, it is certainly needed for backwards compatibility. |
|
Even though it's not in the nature of webwork to deprecate things, perhaps we can deprecate PGtikz.pl then. |
|
No. I completely disagree. |
|
@pstaabp Can you post a problem that was working before, but does not work now? I'll include it in revising things today. The objective here is to (a) open up image-making to LaTeX tools beyond tikz while (b) not messing up problem files (or macros like parserGraphTool.pl) that use PGtikz.pl. And (c) trying to do it without having two .pm files that have 95% code overlap. I think it will work once the issues raised are ironed out. |
|
@Alex-Jordan: Here is a problem that uses pgfplots that worked before this pull request, but does not now. |
|
Ready for more review. I also added a 3rd test problem in |
Co-authored-by: Glenn Rice <47527406+drgrice1@users.noreply.github.com>
|
Oh wait. I need to test those that add packages yet. |
|
Nevermind, I did. Merging. |
This renames
TikZImage.pmasLaTeXImage.pm, and makes it so other kinds of images (not necessarily tikz) can be used. It changesPGtikz.placcordingly, and unless I made a mistake, any problem or macro that relies onPGtikz.plshould still work with no changes.It adds
PGlateximage.plas the more general version ofPGtikz.pl. There are two sample problems that use it int. One makes anxypicimage, one makes acircuittikzimage.There will be a
webwork2PR to go along with this.