-
Notifications
You must be signed in to change notification settings - Fork 176
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
disable mathjax fast preview #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me... I don't see a huge benefit to fast preview and if its effect is to slow down the total rendering time, there is a clear disadvantage.
I do think an athenapdf 5 second delay could get be a bit long for local builds, although on CI it makes sense to allow more rendering time. Let me check if there is an easy parameter substitution that would set the delay longer only on Travis... or is this too much complexity?
I think the following implementation of conditionally setting # before athenapdf command
if [ "${CI:-}" = "true" ]; then
echo "continuous integration build detected. Setting athenapdf --delay=${MANUBOT_ATHENAPDF_DELAY:=5000}"
fi
# for athenapdf --delay
--delay=${MANUBOT_ATHENAPDF_DELAY:-1100} https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html |
Default to --delay=5000 (5 seconds) for CI builds, where ensuring rendering is complete is crucial and Travis instances may have high load. Default to --delay=1100 (1.1 seconds) for local builds, where there is a greater chance that users are running the command in real-time. The MANUBOT_ATHENAPDF_DELAY environment variable can now be set to override default values.
@vincerubinetti and @agitter does b45fabc look good to you? I tested the bash locally and it seems to work as intended. I wanted to keep the athenapdf command working even for users that copy and paste it and lose the context of the build.sh script. |
@michaelmhoffman if you have time to look at the shell script changes in this PR that would be appreciated... it's only a few lines. |
build/build.sh
Outdated
if [ "${CI:-}" = "true" ]; then | ||
# Incease --delay for CI builds to ensure the webpage fully renders, even when the CI server is under high load. | ||
# Local builds default to a shorter --delay to minimize runtime, assuming proper rendering is less crucial. | ||
echo >&2 "Continuous integration build detected. Setting athenapdf --delay=${MANUBOT_ATHENAPDF_DELAY:=5000}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the default value of a variable as a side-effect of printing a log message does not lead to understandable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I totally prefer 4457667, but if it will be more widely understood, then OKAY with me.
For readability
This build is based on 579547d. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/121884516 https://travis-ci.com/manubot/rootstock/jobs/222296602 [ci skip] The full commit message that triggered this build is copied below: Reduce chance of incomplete MathJax rendering Merges #255 Closes #252 Disable MathJax fast preview to reduce render time. Enable MANUBOT_ATHENAPDF_DELAY environment variable to set the athenapdf --delay option. Default to --delay=5000 (5 seconds) for CI builds, where ensuring rendering is complete is crucial and Travis instances may have high load. Default to --delay=1100 (1.1 seconds) for local builds, where there is a greater chance that users are running the command in real-time.
This build is based on 579547d. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/121884516 https://travis-ci.com/manubot/rootstock/jobs/222296602 [ci skip] The full commit message that triggered this build is copied below: Reduce chance of incomplete MathJax rendering Merges #255 Closes #252 Disable MathJax fast preview to reduce render time. Enable MANUBOT_ATHENAPDF_DELAY environment variable to set the athenapdf --delay option. Default to --delay=5000 (5 seconds) for CI builds, where ensuring rendering is complete is crucial and Travis instances may have high load. Default to --delay=1100 (1.1 seconds) for local builds, where there is a greater chance that users are running the command in real-time.
Merges manubot/rootstock#255 Closes manubot/rootstock#252 Disable MathJax fast preview to reduce render time. Enable MANUBOT_ATHENAPDF_DELAY environment variable to set the athenapdf --delay option. Default to --delay=5000 (5 seconds) for CI builds, where ensuring rendering is complete is crucial and Travis instances may have high load. Default to --delay=1100 (1.1 seconds) for local builds, where there is a greater chance that users are running the command in real-time.
Merges manubot/rootstock#255 Closes manubot/rootstock#252 Disable MathJax fast preview to reduce render time. Enable MANUBOT_ATHENAPDF_DELAY environment variable to set the athenapdf --delay option. Default to --delay=5000 (5 seconds) for CI builds, where ensuring rendering is complete is crucial and Travis instances may have high load. Default to --delay=1100 (1.1 seconds) for local builds, where there is a greater chance that users are running the command in real-time.
This PR disables MathJax's fast-preview option and increases the Athena print delay in response to #252.
Here's what it looks like now to load equations:
You can briefly see some gray text, which I believe is just the plain text of the latex input. It's then replaced by an SVG when it's ready.
It looks like there is a brief moment where the both the gray text and the svg is there. So I guess even with this change, it's possible that equations could still be doubled if the timer for pdf output is not long enough.
http://docs.mathjax.org/en/latest/options/extensions/fast-preview.html