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 for markdown equation formatting #11

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

robbotorigami
Copy link
Contributor

I was having similar issues to those reported in #7 and #3 , and dug in a bit to see what the problem was. I found that the way the MutationObserver architecture was set up for triggering MathJax.typesetPromise did not match the DOM layout I was getting from jupyterlab. I'm not sure why this seems to be different for different users - presumably an underlying jupyterlab reason. I rewrote the architecture so that it should be much less fragile to changes in the document structure, and this seems to resolve the issue.

@robbotorigami
Copy link
Contributor Author

@dragonstyle or @cscheid Is there any feedback that can be given on this PR? I would really like to use this plugin when editing notebooks for use with quarto, but rendering issues with mathjax is a deal-breaker.

@dragonstyle
Copy link
Contributor

Apologies for not getting to this sooner. If I understand the change correctly, it seems like the biggest change is that the whole document is observed (rather than attempting to observe the cells). This definitely seems like it would be more robust, but will mean that we typeset the math each time any part of the document is mutated - does that also seem correct to you? In your observation does this typesetting cause any latency of have observable downsides in normal / large notebooks?

@robbotorigami
Copy link
Contributor Author

robbotorigami commented Nov 24, 2024

@dragonstyle no worries, and thank you for taking a look (and all your other work on this project).

The whole document is being observed, but it's only looking through the addedNode list for each mutation for the .quarto-<inline|display>-math elements. In theory this would mean that only those in a cell getting executed (and thus with a newly added output node) would be provided to Mathjax.typesetPromise. In practice, it seems that there's a number of other events causing mutations: scrolling, selecting cells, etc, but it doesn't seem to be egregious. It's also worth noting that calling typesetPromise should be fairly low overhead for math that is already typeset (though presumably there is some amount of overhead).

I just made a notebook with a couple hundred equations to try the plugin both with and without these changes. I think performance is actually slightly better with this change, but they're pretty similar so it could easily be due to something else. Both are much slower than in base jupyterlab, but I imagine that's an unrealistic goal post.

For context, (and hopefully I'm remembering this correctly), I had issues with the existing two-tier mutation observer approach since the element containing jp-MarkdownCell was getting recreated each time the cell was executed, and I couldn't find a way to make it work with code cells display markdown output (with IPython.Markdown).

@dragonstyle
Copy link
Contributor

Ok, thank you for your diligence. I think that two tier approach was in part my attempt to thread the needle and avoid all those mutations triggering typesetting, but I also recall having issues with recreated cells and sorting out how to observe those with that approach.

@cscheid cscheid merged commit e90c2e0 into quarto-dev:main Dec 11, 2024
1 check passed
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