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

[based on 0.4.1] Fix dynamic updates in reading mode & remove targetDisplay in MathLinksRenderChild #37

Closed

Conversation

RyotaUshio
Copy link
Collaborator

@RyotaUshio RyotaUshio commented Aug 15, 2023

I propose two options for resolving #35.

  1. based on the previous release 0.4.1: This PR
  2. based on the latest release 0.4.2: [based on 0.4.2] Fix dynamic updates in reading mode & remove targetDisplay in MathLinksRenderChild #36

Please choose the one you prefer (if exists).


Common modification

Both 1 & 2 have the following modification in common:

In MathLinksRenderChild.update(), targetDisplay should be replaced with this.displayText because the former is overwritten when mathLink is updated but the latter remains untouched.

Having targetDisplay seems to do little harm for mathLinks registered via the YAML frontmatter, but it is problematic for API mathLinks.


Changes specific to this PR

There's not much of it.
I'm not sure why the delay when opening files (the issue you've addressed in the latest release) is happening so there wasn't a lot that I can do for fixing it.

The only possible reason for the delay that I've came up with was that I was awaiting for every piece of inline math in renderTextWithMath:

await finishRenderMath();

So I tried to change it so that it calls finishRenderMath only once:
https://github.com/RyotaUshio/obsidian-mathlinks/blob/a2a5fff8118640bc206811ef30c3c36d0d12ea61/src/links.ts#L104

P.S. In this PR, there is no mathLink-internal-link/``original-internal-link"` classes, unlike in #36 .

@RyotaUshio RyotaUshio changed the title Fix reading mode 0.4.1 [based on 0.4.1] Fix dynamic updates in reading mode & remove targetDisplay in MathLinksRenderChild Aug 15, 2023
@zhaoshenzhai
Copy link
Owner

Yes, the await is the problem. tl;dr: this works for most links, but not for dataview inline fields.

  • When you implemented the dynamic updating feature here, there were two functions for rendering some text (usually, the mathLink) into MathJax: The addMathLink that I had before, and the renderTextWithMath (async) that you wrote recently. Your code is much cleaner, so I decided to remove mine and integrate yours into the rest of the code. The only problem is that toDOM in preview.ts cannot be asynchronous, so I made two versions of renderTextWithMath, one synchronous and the other async. This was the state in 0.4.1, but now links take a short but noticable amount of time to render. This wasn't that big of a deal, so I decided to leave this for later (for 0.4.2).
  • One way to fix it was to simply only keep the synchronous version of renderTextWithMath, but this makes it so that dataview inline fields like (text::[[Main|$\alpha$]]) would not be rendered in reading mode; it would just be blank (in some versions of the code, this won't be rendered if it is under a list like * ..., but would otherwise be ok).
  • After some inspection, it seems that dataview updates the cache again after the inline fields have been processed (or indexed? I'm unfamiliar with the terminology). This causes the links to be rendered twice, which is why I had to include these lines and the check here to prevent the second attempt to somehow remove the link (causing it to appear as blank).

I'm not sure whether this is the exact reason for why those links appear blank in reading mode, but its my best hypothesis so far.

@RyotaUshio
Copy link
Collaborator Author

Thanks for the detailed explanation, now it's much clearer.

I wasn't aware of the Dataview issues so I tested this PR on my vault.
Fortunately, both #36 and #37 (this PR) seem to have no trouble rendering the dataview inline fields in reading mode.

@RyotaUshio
Copy link
Collaborator Author

Also I will take a look at why the dataview issue was happening without async.

@RyotaUshio
Copy link
Collaborator Author

I've finally identified the root cause of the Dataview problem and sent a PR to fix it: blacksmithgu/obsidian-dataview#2098

it was a Dataview's bug. Once the PR is merged, the problem discussed here might be able to be resolved in a different way from the current one, and it will make it easier to resolve the issues #50 & #51.

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.

2 participants