pandoc / lua-filters

A collection of lua filters for pandoc
MIT License
611 stars 166 forks source link

math2svg.lua filter #118

Closed stroobandt closed 3 years ago

stroobandt commented 4 years ago

This Lua filter converts LaTeX math to MathJax generated SVG in any of the available MathJax fonts.

tarleb commented 4 years ago

Thank you for the PR. The documentation looks nice, tests are good, and the filter seems useful.

There are a couple of questions and issues which we should check to make the filter maintainable as well as useful for other users.

A problem I see is that the docs are focused on Ubuntu/Debian and the path to tex2svg is hard-coded in the filter. We should either have clear instructions for all major platforms, or just link to instructions provided elsewhere.

The lua-filters repo is under the MIT license; I'd like to prevent a jumble of filter licenses. For that reason we can't include code licensed under the GPL-3.

The addition of the additional LaTeX commands is opinionated; can we find a way to make that more general?

Could you also add an explanation to the docs why only display math equations are handled, but not inline math? I assume it is because of baseline alignment issues? How does Mathjax handle that normally?

The filter currently adds raw SVG code to all formats, which is most likely not what we want. Maybe we could store the images in the mediabag in these cases?

Cheers!

stroobandt commented 4 years ago

Thank you for the very constructive comments. I will work on these.

stroobandt commented 4 years ago

Dear Albert,

I think I have been able to attend to all your concerns:

tarleb commented 4 years ago

Thank you. This is looking really good now!

The tests currently fail, partly because I need to adapt existing tests to the latest pandoc release, and also because tex2svg isn't being installed on the CI system. You could try to do the latter yourself by editing .travis.yml if you'd like; otherwise just let me know and I'll have a look once the other tests are fixed. I'll be holding off merging until we have all lights green again.

Some tweaks we could look at until then:

stroobandt commented 4 years ago

You could try to do the latter yourself by editing .travis.yml if you'd like

Done. It seems to be working. I also cleaned up a bit the YAML indenting as per Travis CI documentation.

  • Couldn't we set the default for math2svg_path to just tex2svg? This would allow users to simply add the executable into their PATH and would make the filter truly platform independent. What do you think about naming that parameter tex2svg_path to make it more obvious what it's used for?

Done. The key is now called math2svg_tex2svg. When absent, the filter will now just try to execute tex2svg; rendering it truly platform independent.

  • Some lines are very long, both in the filter and the readme. Lines should stay below 80 chars if possible.

Reducing the lines below 80 columns is simply not possible because some of the hyperlinks in README.md are longer than that. However, I was able to reduce README.md and math2svg.lua to below 120 columns, which also happens to be a terminal standard. (Hey, we are not 1984 anymore! — Although it certainly feels like it nowadays.)

To set the record straight: More than a year before John was even considering this, others and myself were already playing with math SVG. Credits for suggesting the current solution go out to Nikolay "Lierdakil" Yakimov.

Anyhow, no, I do not think this needs mentioning. What I do think is that our Lua filters deserve BETTER MARKETING. I will open a separate issue about this for discussion. Currently. Lua filters look more like an afterthought at the bottom of the Extras page, when they are actually easier to maintain than Haskell filters. When one eventually lands on this README.md, it becomes a GitHub dumpster dive to find out which Lua filter does what. I would suggest adding one paragraph descriptions to the general README.md and do something similar on the Extras page.

  • If we were to set math2svg_inline2svg to true by default, then the filter could serve as a full drop-in replacement for pandoc-tex2svg. Personally, I'd also find this more consistent, but it is your choice.

Won't fix. In the README.md I promise the user sensible defaults. Even John complains about the filter being slow when both display and inline math are being converted:

"The filter is rather slow, and it adds significantly to file size, but the resulting HTML renders quickly and does not depend on an internet connection or JavaScript."

  • I remembered that HTML does not allow <div> elements within <p> elements. The filter currently produces invalid markup and should probably use <span> for both "display" and "inline" elements. We could think about adding style="display:block" to "display" spans, but maybe it's better to leave that to a global CSS rule.

Work in progress. I already fixed it with a <span class="math display">. However, since I eat my own dog food, I discovered this is not ideal for CSS paged media processing. I will run some tests to see what works best, whilst keeping the HTML valid. I will also document this in the README.md.

stroobandt commented 4 years ago

I opened #121 concerning better Lua filter marketing.

tarleb commented 4 years ago

The additional, non-essential changes to .travis.yml broke the config. PRs should only have one topic. Changing indentation in a config file is a separate concern and should be treated accordingly.

The 80 char-per-line rule is rooted in two principles: (1) The README should be easy to read, no matter whether viewed as rendered HTML in a browser, or as plaintext on a console. (2) research showed that text is easiest to read if there a 60 to 66 characters per line. 80 is still acceptable, anything above 100 is seriously bad. If you don't like how links look when forcing shorter lines, use reference links.

tarleb commented 3 years ago

We had to switch from Travis CI, so now all tests are run in GitHub Actions. This led to a merge conflict which has to be resolved before we can finally merge. The additional packages now need to be added in .tools/Dockerfile. Could you update the PR?

stroobandt commented 3 years ago

@tarleb Dear Albert, I have modified .tools/Dockerfile, but apparently I cannot test it because of lacking write permissions.

Only those with write access to this repository can merge pull requests.

The packages nodejs and npm are required for testing. There is also an additional RUN statement to download an npm package.

stroobandt commented 3 years ago

All open issues have been resolved. If an administrator can take care of the suggested .tools/Dockerfile, this new Lua filter should be ready to go.

tarleb commented 3 years ago

Thanks!