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

Upgrade to Mathjax 3 (ankitects/help-wanted#19) #809

Merged
merged 5 commits into from
Nov 15, 2020

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Nov 3, 2020

This PR updates MathJax to v3.1.2.

Does the existing functionality in reviewer.ts still work? We need to be able to register a function to be called once rendering is done, so we can defer card rendering

After some small changes, yes. Whereas MathJax 2 implemented its own synchronisation methods, MathJax 3 completely relies on Promises, which seems more stable to me.

Can users still customize MathJax with some JS on their cards?

I don't really know how the user would have done that before with MathJax 2. Could you elaborate?

Will moving to MathJax 3 break things for existing users?

As far as I see, everything works fine. The MathJax API changed quite a bit, so add-ons which rely on the old MathJax 2 API (maybe for synchronization with MathJax output) will surely break.

I also moved the creation of the MathJax files in the mathjax directory to the build process as a new target in ts/Makefile.


The problem mentioned in https://github.com/ankitects/help-wanted/issues/19 still persists though.

MathJax localStorage error: Failed to read the 'localStorage' property from 'Window': Storage is disabled inside 'data:' URLs.

According to GitHub posts I found, MathJax 2 used cookies to save settings whereas MathJax 3 uses localStorage. However neither cookies nor localStorage are supported in data URLs. Did you have to deal with this issue, when you first implemented MathJax support in Anki?

@hgiesel hgiesel mentioned this pull request Nov 3, 2020
@dae
Copy link
Member

dae commented Nov 3, 2020

Thanks for looking into this! I like that the files are being generated at build, as it will make it easier to keep up with point releases.

When you say you're getting a localStorage error, do you mean this is just dumping something to the console, or do you mean it's preventing MathJax from actually working? The plan is to eventually migrate away from data: in setHtml()/stdHtml() in favour of separate HTML page(s), as the current approach places an upper limit on the number of decks that can be shown for example. But it will likely be a while before that move can be made.

Here are some examples of configuration:
https://anki.tenderapp.com/discussions/ankimobile/9705-mathjax-configuration
https://stackoverflow.com/questions/51124109/changing-mathjaxs-font-size-on-anki

@hgiesel
Copy link
Contributor Author

hgiesel commented Nov 4, 2020

The localStorage error is dumped at runtime, but it does not affect output.
In fact, I tracked the error message to the method mergeUserSettings (found here). It saves the settings which the user can configure by right clicking the MathJax output (which does not work in Anki anyway afaik, because the right-click is intercepted by Qt).

Concerning configuration: All the existing configurations would break.
Here are the documents which talk about ways to configure MathJax after being loaded. One example they give is MathJax.startup.output.options.scale, which changes the size of the output, which I can confirm works.

I also tried the following, with which I could switch the inline MathJax delimiters from \[ and \] to \* and */, and can confirm it works.

MathJax.config.tex.displayMath = ['\\*', '*/']
MathJax.startup.getComponents()

So it seems it is sufficiently configurable, however it breaks existing setups.

@dae
Copy link
Member

dae commented Nov 6, 2020

Sorry, the Bazel merge has meant some work will be required to integrate this with master. If you could look into the other files, I can help out with the Makefile equivalent if you need it - probably needs a genrule()

Might be worth excluding the 1.3MB mathmaps_ie.js which we presumably don't need.

@hgiesel hgiesel force-pushed the mathjax3 branch 4 times, most recently from 4794ab2 to 5646707 Compare November 6, 2020 17:52
@hgiesel
Copy link
Contributor Author

hgiesel commented Nov 6, 2020

  1. I looked into writing the genrule(), however I already failed at making a dummy rule just creating a file. I called genrule() call inside qt/aqt/BUILD.bazel. Do I need to do anything else to do this? Some guidance here would be greatly appreciated (or if you want to do it yourself, that's also fine by me).

Also it doesn't seem to be that easy to write a "copy files" target, as you have to specify all the out files in outs. I don't really find a solution for this either (or I don't understand them sufficiently).

I specified the filegroup here, however.

  1. There seems to files you either forgot to add to .gitignore, or deleted by accident:
meta/buildhash
ts/package-lock.json

They both continue to show up in my untracked files.

@dae
Copy link
Member

dae commented Nov 9, 2020

Yeah, this is a bit tricky unfortunately, but should be possible in a .bzl file - either by manually listing the desired files in a list and then constructing the outs list and copy commands, or by doing something like this:

https://stackoverflow.com/a/57983629

...which I have not tested, but the issue with Bazel 2.0 may be fixable by calling to_list() on the depset.

If you find yourself spending too much time on it, please let me know and I'll take a stab - unless @agentydragon wants to give it a try

@dae
Copy link
Member

dae commented Nov 9, 2020

Re the untracked files, those files are no longer used, and you can delete them.

@jolars
Copy link

jolars commented Nov 13, 2020

Could you please see if this fixes this issue: https://forums.ankiweb.net/t/bold-math-does-not-work-in-mathjax-on-the-linux-desktop-client/5073 ?

@hgiesel
Copy link
Contributor Author

hgiesel commented Nov 14, 2020

@dae I spent my whole Saturday trying to get it run, but to no avail. This is code I had at the very end:

def copy_files(ctx):
    includes = ctx.attr.includes
    build_file = ctx.actions.declare_file(".mathjax")

    include_pattern = ' '.join(includes)
    // Rather than echo, this should cp the files to their destiny
    cmd = "echo {}; touch {}".format(
        include_pattern,
        build_file.path,
    )

    ctx.actions.run_shell(
        outputs = [build_file],
        command = cmd,
    )

copy_files = rule(
    implementation = _copy_files_impl,
    attrs = {
        "includes": attr.string_list(
            mandatory = True,
        ),
        "excludes": attr.string_list(
            allow_empty = True,
        ),
        "srcs": attr.label_list(
            allow_empty = True,
        ),
    },
)

// where includes, excludes would be a list of glob patterns

My idea was to create a file ".mathjax" in the ts directory, which is created when the files are copied (kinda how it was done before switching to Bazel), and leave the MathJax files out of the outputs, which is acceptable according to the Bazel docs.

Some difficulties are:

  1. glob() only works for source files, not generated files, so we cannot do this.
  2. The rule above always comes to the conclusion, that it's up-to-date, even though there is no .mathjax file.

@dae
Copy link
Member

dae commented Nov 14, 2020

Thanks for giving it a go, I'll take a stab at this when I have a chance.

@dae dae merged commit afc7bdf into ankitects:master Nov 15, 2020
@dae
Copy link
Member

dae commented Nov 15, 2020

Managed to figure it out, and this is now in master - let's see how it goes in the next beta. Thank you for your work on this!

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