Skip to content
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

fix hrule thickness argument #905

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

Alex-Jordan
Copy link
Contributor

With horizontal rules of any kind in a niceTables.pl table, you can declare their thickness. You can be explicit with a unit like 2pt or you can just use a number. If you just use a number, it is supposed to come out as that many pixels.

In hardcopy, if you are not using booktabs, this is ignored and you just get the standard hline thickness.

If you are using booktabs, we take advantage of the ability to declare thickness on the booktabs horizontal rule commands. But it turns out that px is not a legal unit of measure. So this change converts that number of pixels to pt, using the convention 1px is 0.75pt.

To test, try

[@ DataTable([[[1, rowbottom => 2]]]) @]*

and make a hardcopy. It should fail without this commit, and work with it.

Side note: When it fails, it takes a long time, and then reports Hardcopy generation error: The operation was aborted. I do not understand why it happens this way, instead of the usual thing when there is bad latex: it more quickly downloads the bundle with the tex file, log, etc.

@pstaabp
Copy link
Member

pstaabp commented Aug 15, 2023

I didn't get an error. Were you running in xelatex? From here: https://tex.stackexchange.com/questions/41370/what-are-the-possible-dimensions-sizes-units-latex-understands

it appears that px is defined in luatex and pdftex.

@Alex-Jordan
Copy link
Contributor Author

Yeah, you are right. It is a XeLaTeX thing. Try:

\documentclass{article}
\usepackage{booktabs}
\begin{document}
\begin{tabular}{c}
a\\\midrule[2px]
c
\end{tabular}
\end{document}

with both pdflatex and xelatex.

I still think this should go in. Supporting xelatex should be a long term goal, and maybe lualatex too.

@pstaabp
Copy link
Member

pstaabp commented Aug 15, 2023

I think it should go also. Just commenting.

What I've read is that lualatex is the way to go for unicode and extra font support these days.

@Alex-Jordan
Copy link
Contributor Author

What I've read is that lualatex is the way to go for unicode and extra font support these days.

There are heavily opinionated statements out there and I'm not convinced that they are all worth taking under consideration. We have short tex files to process compared to theses and books, and a lot of what I see written in support of lualatex is based on efficiency and speed, and so maybe doesn't apply to WeBWorK exercise sets. So I just want to note we should plan to support both, or be really scientific about it if we ultimately choose one over the other.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the issue when $externalPrograms{pdflatex} is set to use xelatex. So this is needed for that case. It still works correctly for pdflatex too.

Note that if I download the TeX source generated by webwork2 with the develop or main branch for the given MWE, and then run xelatex in the terminal the same thing happens. It takes forever to fail. Something to do with xelatex in batchmode with this.

@Alex-Jordan
Copy link
Contributor Author

IIRC if you comment out the batchmode, you will find that it is prompting you for an acceptable dimension to replace the px count. I'm thinking that with batchmode, that prompt is still happening but you don't see it. And it takes forever because actually it's timing out. That's my hunch anyway.

@drgrice1
Copy link
Member

Perhaps we also need the -halt-on-error option. Then when an error occurs it will just stop.

@Alex-Jordan Alex-Jordan mentioned this pull request Aug 18, 2023
@drgrice1
Copy link
Member

I will merge this since the hotfix of this was merged.

@drgrice1 drgrice1 merged commit 58789ff into openwebwork:develop Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants