RyotaUshio commented 1 year ago

Hi @zhaoshenzhai! Let me propose a few updates to the API.

Generalize enableFileNameBlockLinks: boolean to prefixer: (sourceFile, targetFile) => string | null.
Change MathLinksMetadataSet from Record<string, MathLinksMetadata> to Map<TFile, MathLinksMetadata> in order to better react to file renaming/deletion.

I'll explain these changes below. What are your thoughts? I'd love to hear your feedback when you have time.

Prefixer

This update is about the prepended note title before a heading/block MathLink (note > heading). I believe it gives the API users much more flexibility.

Firstly, getMathLinkFromSubpath's last parameter is now prefix: string | null.

https://github.com/RyotaUshio/obsidian-mathlinks/blob/3cd5bc8011ec5572925e393394d6cf3d97058ff9/src/links/helper.ts#L77-L97

If prefix is null, the standard prefix rule is applied. That is, the note title (or mathLink if it exists) + > is used as the prefix.
On the other hand, if prefix is string, then it is interpreted as a custom prefix. In other words, the resulting mathLink is prefix + subMathLink.

Secondly, each API account's enableFileNameBlockLinks: boolean property has been replaced with a function prefixer: (sourceFile: TFile, targetFile: TFile) => string | null, which determines the value of prefix (getMathLinkFromSubpath's last parameter) depending on the relationship between sourceFile (where the link exists) and targetFile (where the link goes).

https://github.com/RyotaUshio/obsidian-mathlinks/blob/3cd5bc8011ec5572925e393394d6cf3d97058ff9/src/links/helper.ts#L59-L62

The default behavior can be recovered by setting prefixer to be() => null: https://github.com/RyotaUshio/obsidian-mathlinks/blob/3cd5bc8011ec5572925e393394d6cf3d97058ff9/src/main.ts#L52

Moreover, it can be used to realize much more flexible behavior. For example,

https://github.com/RyotaUshio/obsidian-math-booster/blob/refactor/docs/projects.md

this feature (unreleased, but you can try using the refactor branch of Math Booster if you are interested) can be realized with the following prefixer:

https://github.com/RyotaUshio/obsidian-math-booster/blob/44881ee182e3194dbc5b0fb30ba135fd0cc2dc37/src/project.ts#L123-L141

As for the ordinary mathlinks (given in frontmatter), the current behavior can be recovered by setting getMathLinkFromSubpath's prefix parameter to be plugin.settings.enableFileNameBlockLinks ? null : "":

https://github.com/RyotaUshio/obsidian-mathlinks/blob/3cd5bc8011ec5572925e393394d6cf3d97058ff9/src/links/helper.ts#L39-L43

Record to Map

Currently, each MathLinksAPIAccount's metadataSet: MathLinksMetadataSet is implemented by Record<string, MathLinksMetadata> i.e. a dictionary that maps a file path to the corresponding metadata.

The problem is that, even after a file is renamed or deleted, its metadata remains as is because there isn't anything that handles this kind of event.

Although it's not a big issue at least for my plugin, it can be problematic for other plugins that will use the API in the future.

There are at least two possible solutions for this issue:

Add event handlers for file renaming & deletion
Replace file paths (string) with TFile objects (which keep pointing to the same file after renaming or deletion)

Here I chose the second one because it eliminates the need for manual implementation of event handlers, which can be buggy.

Record doesn't accept TFile as a key value, so I had to use Map instead.

RyotaUshio commented 1 year ago

Oh, I found a bug regarding heading mathLinks that is not directly related to this PR but affects both API users and non-API users (sorry...). It might be better to make a new PR, but it will cause a conflict with this PR, so I'll include this bug fix in this PR for now (I'm not sure if this is the right procedure, though...). Let me know if you don't like it.

Bug description

The problem was that heading mathLinks are not correctly rendered when there's no YAML frontmatter (or should I call it properties now?).

Live preview	Reading view

It happens because the getMathLinkFromSubpath() for non-API mathlinks is only called when cache.frontmatter exists:

https://github.com/zhaoshenzhai/obsidian-mathlinks/blob/375dd73f599642f3c342b81eeb89eb60ee7d6598/src/links/helper.ts#L37

I've never noticed this bug until now because it doesn't occur when Math Booster is enabled:

Math Booster enabled

In this case,

mathLink remains to be "" right after this if block because there's no frontmatter and this if block never runs.
Therefore, the next if block for API mathlinks runs. The heading mathlink is finally processed here.

Solution

To fix this bug, I removed if (cache.frontmatter) and modified getMathLinkFromSubpath to accept possibly undefined metadata. After this change, the heading mathlink seems to be working well without Math Booster.

Live preview	Reading view

zhaoshenzhai commented 1 year ago

Hi! Apologies for the late reply. I'll need some time to over this and get back to you (probably this weekend, as school started).

RyotaUshio commented 1 year ago

Thanks, you don't have to rush at all!

RyotaUshio commented 1 year ago

Update: subpathResult: HeadingSubpathResult | BlockSubpathResult has been added to the prefixer's parameter list, allowing adaptive prefixing depending on the block ID, for example.

zhaoshenzhai commented 1 year ago

Hi! Just wanted to let you know that I have not forgotten about this, but this week proved to be a lot more busy than I thought so I might need more time to go through this PR.

If you want, I can actually add you as a collaborator for this plugin so that you can add more features/bug fixes as you wish. Let me know if this is something that you are interested in!

RyotaUshio commented 1 year ago

I might need more time to go through this PR.

That's totally okay!

If you want, I can actually add you as a collaborator for this plugin

Oh, that would be great, I'd love it! Thanks a lot!

RyotaUshio commented 1 year ago

Thanks for adding me as a collaborator! Can I merge this PR if no problem is found after testing?

zhaoshenzhai commented 1 year ago

Just as an initial test, there are errors that are thrown by Math Booster (using the mathlinks api) as:

MathLinks API: Math Booster passed a non-markdown file undefined to update().

I have math booster version 0.6.11 installed from obsidian (congrats btw!)

RyotaUshio commented 1 year ago

congrats btw!

Thanks! Obviously I couldn't make it without your help, I truly appreciate it!

Math Booster 0.6.11 is not ready for this new version of MathLinks API, so it passes path: string to update(), while update() expects file: TFile. In the refactor branch, I'm developing 0.7.0 (or maybe 1.0.0?) that has adapted to the API update. I guess 0.7.0 doesn't throw such errors.

zhaoshenzhai commented 1 year ago

Ahh I see, thats what you meant by this:

Moreover, it can be used to realize much more flexible behavior. For example,

https://github.com/RyotaUshio/obsidian-math-booster/blob/refactor/docs/projects.md

this feature (unreleased, but you can try using the refactor branch of Math Booster if you are interested) can be realized with the following prefixer:

Other than this, I don't notice any bugs with my existing tests. Feel free to merge this PR and make a new release when you are ready!

RyotaUshio commented 1 year ago

@zhaoshenzhai I can make a new release at this point as long as you are okay (there are unresolved issues: #46, #47), but I still have to ask you to update the npm package:pray: (You don't have to wait until I make a release)

zhaoshenzhai commented 1 year ago

I've published the npm package just now, and I can release 0.4.6 now. Regarding the issues, I've solved #46, but not #47 yet, which seems to be a bigger issue. Let's leave it for a future release?

RyotaUshio commented 1 year ago

issues

47 seems to be a hard one... I feel it comes from the inherent limitation of markdown links (i.e. `[]()`).

It might take some time to resolve it, so we will have to leave it unresolved for now.

I've published the npm package just now

Thanks, but I noticed that the main & types fields of package.json are incorrect. I've fixed this in the PR #49. So let me know after you re-publish the npm package. I will then make new releases of MathLinks and MathBooster at the same time (otherwise MathBooster will throw errors and the users might come to this repo to report issues...).

RyotaUshio commented 1 year ago

By the way, I'm going to use this as the release note/changelog:

Fixed bugs regarding heading links (#45) and block links with custom display names containing escaped square brackets (#46).

Let me know if you want to add or correct something (especially my English!).

zhaoshenzhai commented 1 year ago

I've published the npm package just now. Feel free to release 0.4.6 with your release notes!

zhaoshenzhai / obsidian-mathlinks

API updates & bugfix around heading mathlinks #45

Prefixer

Record to Map

Bug description

Solution

47 seems to be a hard one... I feel it comes from the inherent limitation of markdown links (i.e. `[]()`).

zhaoshenzhai / obsidian-mathlinks

API updates & bugfix around heading mathlinks #45

Prefixer

Record to Map

Bug description

Solution

47 seems to be a hard one... I feel it comes from the inherent limitation of markdown links (i.e. []()).

47 seems to be a hard one... I feel it comes from the inherent limitation of markdown links (i.e. `[]()`).