Toggle plugins

[CalebJohn]

With the new change to the renderer (making it much easier to use plugins) I thought it would be a good idea to allow enabling/disabling plugins through the general options. To that end I have created a test branch which adds a couple of plugins and makes one of them able to be enabled/disabled through the interface.

Here is a note with mark syntax and footnotes added.

Here is the plugin screen (I only added mark for now)

And after we hit the toggle and return to the note it looks like this.

With a system like this in place it will allow us to add a lot of plugins without worrying about users will use/like because everything can be turned off if it's annoying. This would solve some of the complaints there have been about katex hijacking the $ symbols.

With that in mind I think there still needs to be some thoughts about which plugins to enable by default, and which should be turned on by users. This would be a good place to discuss that.

For reference, here is a list of markdown-it plugins on npm

[tessus]

This looks great.

However, should @laurent22 agree to adding this, maybe we should create a separate screen and menu item for Plugins... or Plugins

[tessus]

a few ideas for additional plugins:

https://discourse.joplin.cozic.net/t/hiding-passwords-with-markdown/1588/7

[CalebJohn]

Okay this is ready to ship!

@tessus I left it on the config screen for now because that was easiest and it integrates nicely with settings. I also looked into adding spoiler and summary and surprisingly it was a bit more involved than these other plugins so I thought it best to leave for another time.

What this pull adds is a bunch of extra markdown features available for markdown-it and the ability to toggle them all (I also made katex toggle-able because there were some complaints about it).

[tessus]

Nice job. 1 question and 1 comment though:

Question:
How is the data persisted? Is it written into the local sqlite database? I couldn't see any db methods, but I don't know the code too well, so it could be abstracted.

Comment:
We should use markdown-it-toc-done-right ^3.0.1. I asked the dev to allow for a placeholder pattern nagaozen/markdown-it-toc-done-right#7, which he rejected. I then suggested to allow several by default. But for that the placeholder option must be removed.
The line
markdownIt.use(markdownItToc, { placeholder: '[[toc]]' })
should be changed to:
markdownIt.use(markdownItToc, { listType: "ul" });

In that case ${toc}, [[toc]], [toc], [[_toc_]] are all recognized as placehoders. Also, I think an unordered list looks nicer, but that's a matter of opinion. So, you can remove the listType option above, but in a future version we could maybe also allow people to choose between ul and ol.

I really love this change and I hope @laurent22 merges this soon.

[CalebJohn]

I did some testing and didn't find a use for them, it seems like the subsequent restorePercentScroll(); even wasn't triggering a scroll event anyways, so by ignoring this event it introduced a subtle bug.

[CalebJohn]

To answer your question:
The Setting module handles persistence, which means that handling the toggling there (the config screen) we get that for free.

[laurent22]

The footnote plugin looks really nice, thanks for adding this!

I agree enabling/disabling plugins like this is the right approach since that way we don't need to think too much about what's the right plugins to add or not.

I need to review the PR a bit more but I just see few issues at the moment:

• The ignoreNextScrollEvent changes seem unrelated to this PR or am I missing something? If it's unrelated, can it be moved to a separate PR?
• In the README, we should have a table with this list of plugins, with a link to the website or help page as it's difficult to know how each feature would work otherwise.
• How about the quality of each of these plugins? I wonder if we should really enable them all by default? What if they have some bug that crashes or freezes the app under some conditions? Because it's probably difficult to review all of them, maybe we should disable most of them by default? Or should we have some criteria like if it has more than x stars on GitHub, we enable by default?
• Are the MdToHtml tests still passing with these plugins?

[CalebJohn]

1. The ignoreNextScrollEvent was to fix a bug related to markdown anchors. I thought since we were adding the anchor and TOC plugins in this pull it would make sense to have a bug fix related to them tacked on. I can remove those 2 plugins and the bugfix and pull them into another pull request if you want.

2. Good point I'll get on that tomorrow

3. Currently the only plugins enabled by default are katex, mark, footnote, and toc. My thoughts around these were that they were either small enough that I felt limited testing was sufficient (mark), they seem to be used enough by others that it would be stable (footnote) or requested enough that it would be worth enabling by default (toc). All others are disabled by default.

4. I'm not sure, I've tried looking into how to run those tests, but I can't find any tests for MdToHtml. Do you mind giving me a hint about how to run them/ where they are?

[laurent22]

I've read the comment but I don't quite understand the change. Based on the code it means, first we jump to the requested anchor (location.hash = href) but then we moved back to the previous anchor (location.hash = old_hash)? I'm sure I'm missing something but could you clarify the logic here?

[CalebJohn]

That's exactly what's happening, this fixes 2 bugs present in the way webview handles anchors, the first is that when you click on an anchor, the url for the webview changes to reflect that movement, which has the unintended side effect of making the webview un-reactive to subsequent presses of the same anchor link. This fixes that issue by resetting the url after some tiny delay, so the webview will correctly scroll into place then reset to a standard url thus allowing the same link to be pressed again (and get the correct scrolling behavior).

The other bug this fixes is a bit weirder and is why I labelled this "hack". The webview tries to place anchors at the top of the page, which means if you have an anchor near the bottom of your page without much text below it, the webview scrolls to the bottom and would shrink to until it was able to fit the anchor at the top of the page. I suggest you remove the line I've added and give it test for yourself, its very strange. What I found was that by resetting the url the webview would correctly scroll to the bottom but wouldn't shrink to display the anchor.
I don't believe this is absolutely the best fix (hence the hack warning) but I was pretty stuck on how else it could be fixed.

[laurent22]

• The ignoreNextScrollEvent was to fix a bug related to markdown anchors. I thought since we were adding the anchor and TOC plugins in this pull it would make sense to have a bug fix related to them tacked on. I can remove those 2 plugins and the bugfix and pull them into another pull request if you want.

Ok if it's to support these 2 plugins, please leave the changes in then.

• Currently the only plugins enabled by default are katex, mark, footnote, and toc. My thoughts around these were that they were either small enough that I felt limited testing was sufficient (mark), they seem to be used enough by others that it would be stable (footnote) or requested enough that it would be worth enabling by default (toc). All others are disabled by default.

I misread the setting page (was looking at the "public" property instead of "enabled" so I thought they were all enabled). I think it's good as it is then.

• I'm not sure, I've tried looking into how to run those tests, but I can't find any tests for MdToHtml. Do you mind giving me a hint about how to run them/ where they are?

From /CliClient, you would run npm i then ./run_test.sh.

[CalebJohn]

I've run the tests now and nothing related to the rendering is failing, I did get one failure though related to folder sorting? I suspect it's unrelated.

[tessus]

I did get one failure though related to folder sorting

That's maybe because the default sorting changed from ascending to descending at one point (when the change was implemented to sort by last updated).

[laurent22]

Thanks for the fixes @CalebJohn, it's nearly all good. For the tests, it was indeed unrelated and I've now fixed it.

There's one last issue and that's regarding the setting names. I've recently added an internal plugin system, which I intend to use to add new features without touching the core files, to make the code more modular basically.

So far it's just an internal feature but later it could be used to support actual plugins with possibility to enable/disable some of them. Because of this, I think we should use a more specific namespace for the new setting properties. Since they are all viewer plugins, I'm thinking they could go under viewer.plugin.*, what do you think?

[CalebJohn]

There's one last issue and that's regarding the setting names. I've recently added an internal plugin system, which I intend to use to add new features without touching the core files, to make the code more modular basically.

That's awesome.

So far it's just an internal feature but later it could be used to support actual plugins with possibility to enable/disable some of them. Because of this, I think we should use a more specific namespace for the new setting properties. Since they are all viewer plugins, I'm thinking they could go under viewer.plugin.*, what do you think?

There should definitely be a new namespace, I like viewer.plugin.* but I wonder if in the future there might be some other plugins that deal with the view and not necessarily with the markdown transformations. I think something more direct like markdown.plugin.* might be more appropriate since these plugins directly relate to the markdown rendering.
Of course, I'm happy to implement either and it's your decision.

[laurent22]

Right, it's true that viewer is a bit vague so let's go for markdown.plugin.*. If you could just make that change that would be great, and otherwise I think it's ready to be merged.

[CalebJohn]

All done! Thanks @laurent22 .

In light of your recent change should this be changed to integrate better with the new plugin system? Or does it make sense to leave it as is for now?

[laurent22]

Thanks for the update!

In light of your recent change should this be changed to integrate better with the new plugin system? Or does it make sense to leave it as is for now?

Let's leave as it is as they are not really Joplin plugins but markdown-it plugins, so it's good if they remains separated.

So I think it's all good now and ready to merge. Many thanks for your efforts @CalebJohn!

[CalebJohn]

Let's leave as it is as they are not really Joplin plugins but markdown-it plugins, so it's good if they remains separated.

Good point. Thanks @laurent22 !