search

PgBiel / typst-tablex

More powerful and customizable tables in Typst
MIT License
370 stars 12 forks source link

utilities: fix issues related to conversion of lengths to pt #84

Closed dixslyf closed 8 months ago

dixslyf commented 9 months ago

This PR fixes some issues regarding the conversion of lengths to pt in hopes of progressing #74. Please see each commit's message for details.

I don't see any regressions in tablex-test.typ (compiled it to PNG before and after this PR's commits, then checked their hashes).

I'm marking this as a draft for now as I haven't added any tests. I did some quick testing with negative line expansion, and surprisingly, it seems to work, so I'd like to investigate this further before readying this PR.

EDIT: should resolve #74.

PgBiel commented 9 months ago

Oh, I didn't realize that the problem occurred even when you were reducing the stroke below a single cell (not between cells, which is expected to fail). That is interesting. Thanks for looking into this!

I can't take a look at the proper logic right now, but I'll give some initial feedback.

dixslyf commented 9 months ago

I've rewritten the functions to use regex (yeah, quite ugly) and added some tests (still need to add more for ratio and fraction types).

One concern I have: the repr for the abs component of a length rounds to a precision of 2 (src), so there will be a loss of precision. This is true for at least Typst 0.9.0, but I haven't looked at previous versions and other length types.

PgBiel commented 9 months ago

One concern I have: the repr for the abs component of a length rounds to a precision of 2 (src), so there will be a loss of precision. This is true for at least Typst 0.9.0, but I haven't looked at previous versions and other length types.

That seems to be the case since before Typst was released... In this case, I think a good workaround might be to keep the old algorithm (just measure the line) if there are no negative numbers in the length's parts (checking if either of the "minus" symbols is present in the repr() should be enough). That way, we only lose a bit of precision when dealing with the specific case of negative numbers. Of course, this is bound to be improved if/when we switch to 0.7.0's constructs.

dixslyf commented 9 months ago

One concern I have: the repr for the abs component of a length rounds to a precision of 2 (src), so there will be a loss of precision. This is true for at least Typst 0.9.0, but I haven't looked at previous versions and other length types.

Seems like this is true for the other length types as well (and angle). The em component seems to be the only exception. The rounding also seems inconsistent, so I've filed a bug report in Typst's repo (https://github.com/typst/typst/issues/2836).

dixslyf commented 9 months ago

I've gone ahead and implemented what I came up with in https://github.com/PgBiel/typst-tablex/pull/84#discussion_r1413024123. I've also managed to come up with a solution for the same precision issue for relative lengths (we don't use repr for fraction and ratio, so those are unaffected).

On the testing side:

I think we're nearly done with this PR. All that's left is another round of review and ensuring that this PR works with older versions of Typst (still trying to work out the best way to do this).

I've left the commit history mostly intact because I didn't want to accidentally remove commits when force pushing. However, I can squash some of them if you prefer.

EDIT: Forgot to check for regressions; will do tomorrow.

PgBiel commented 9 months ago

Thanks, you're doing great work! I'll try to provide a final round of review soon.

Regarding commit history, don't worry too much about it, I'll probably squash before merging.

dixslyf commented 9 months ago

Re-ordered the commits so that changes to tablex-test.typ come later. Makes it easier to check for regressions before tablex-test.typ got modified.

I managed to hack together a bunch of scripts using Nix and diff-pdf to semi-automate regression testing for the different Typst versions. (Didn't include them here since they're out of scope and pretty makeshift, but they can be found at: https://github.com/dixslyf/typst-tablex/commit/7d078ebff5f91010395103aa624af068686fcac8.) I don't see any regressions for any of the supported Typst versions.

PgBiel commented 8 months ago

Thank you! The result looks great. I also appreciate very much that you tested much more than I could ever ask someone to! 😂 Great job 👍

PgBiel commented 8 months ago

I plan on having a better testing system in the future. I'll probably consider using PNG instead of PDF though as it's a pain to work with :p

I'm particularly interested in Lau's efforts towards a standard testing solution for packages, although I might also try out something like CeTZ's system. We'll see...

dixslyf commented 8 months ago

I'll probably consider using PNG instead of PDF though as it's a pain to work with :p

Indeed, PDFs are a huge pain to work with. I initially wanted to go with PNG for my tests, but PNG export was only added in Typst v0.5.0. What I tried at first was to convert the PDFs to PNGs using ImageMagick (I also tried pdftoppm), but the conversion took a while and you need to tune the options to get decent quality output. I later realized you could just use magick compare on two PDFs (it'll do the conversion for you, but you'll still need to tune the options) to generate a diff image (the exit code also tells you whether the PDFs were similar). I went with diff-pdf (which does more or less the same thing) in the end just because it was easier to use.

Another thing I tried was to compare the output across different Typst versions (e.g., compare the outputs for Typst v0.2.0 and v0.9.0). The outputs were slightly different — mostly differences in spacing. However, even for pages that look the same to our eyes, if you compared them using magick compare, there would be differences in the anti-aliasing (I think) of rendered text. I figured it wasn't worth it to test across Typst versions.

I'm particularly interested in Lau's efforts towards a standard testing solution for packages,

Same!

although I might also try out something like CeTZ's system. We'll see...

I took a look at CeTZ's testing when I was trying to test this PR, and it did look nice and promising. My only concern was it tests only a single version of Typst as far as I can tell. It's probably not too difficult to expand on that, however (e.g., modifying PATH to use a different Typst version, though fetching the different Typst versions and caching them somewhere sounds like a bit of a pain).