Skip to content

Conversation

mbilykov
Copy link

Hi there,

First of all i want to say that like your plugin very much. You did a great job! Thank you a lot.

I hope you'll continue to support this plugin 🙏

I did add live preview for theme. I think it can be very convenient.
shiki_live_preview

I'm not sure if i should to did this but i updated dependencies 😌

Copy link
Owner

@mProjectsCode mProjectsCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually don't like PRs messing with dependencies, since it can easily lead to merge conflicts, but it's fine. You can keep the updates there.

I am not sure that using the markdown renderer is the best idea, especially since you passed the wrong arguments to the last two arguments (the second last should be an empty string and the last should be a component created in this function that is unloaded when you rerender).

A better approach would be to render the code block content directly to HTML (via the plugin methods). The heading should be omitted and the warning can be integrated directly into the settings UI (and created via the DOM API).

@mbilykov
Copy link
Author

@mProjectsCode thanks for your fast feedback.

I must admit that I am not very knowledgeable about creating plugins for Obsidian. Moreover I started learning obsidian's plugin development by "reverse engineering" your plugin ☺

I am not sure that using the markdown renderer is the best idea

Why do u think to render the code block content directly to HTML it is better approach? Can we face performance or security issues with MarkdownRenderer.render?

the last should be a component created in this function that is unloaded when you rerender

Could you please explain what the last argument should be?

In documentation says that this is component that managing lifecycle. So plugin instance looks for me like correct value.

A better approach would be to render the code block content directly to HTML (via the plugin methods).

I have doubts about how to do this. I means which html i should use? Because as i noticed there are a lot html for showing code block. Each line is standalone block, etc. Like

<div class="HyperMD-codeblock HyperMD-codeblock-bg cm-line" dir="ltr"><span class="cm-hmd-codeblock" spellcheck="false">// Example TypeScript code</span></div>
<div class="HyperMD-codeblock HyperMD-codeblock-bg cm-line" dir="ltr"><span class="cm-hmd-codeblock" spellcheck="false">function greet(name: string): string {</span></div>

So i choose to render Markdown way because it was much faster and easier 😌

Or could we use more simple HTML for this? Something like

<pre>
   <code>
        function greet(name: string): string {
            return `Hello, ${name}!`;
        }

        const result = greet('World');
        console.log(result);
   </code>
</pre>

the warning can be integrated directly into the settings UI (and created via the DOM API).

I'm not sure that understood this.
I've noticed that settings UI uses two column layout that not very helpful in this case ( i've tried first to use Setting constructor).

the warning can be integrated directly into the settings UI (and created via the DOM API).

For the test i also did improve code to get autoreload Highlighter when theme is changed and for some other options.

It works like a charm. But maybe is there potential performance issue?

@mProjectsCode
Copy link
Owner

Why do u think to render the code block content directly to HTML it is better approach? Can we face performance or security issues with MarkdownRenderer.render?

Rendering directly to HTML is going to be slightly faster and why do extra work when you don't have to?

Could you please explain what the last argument should be?
In documentation says that this is component that managing lifecycle. So plugin instance looks for me like correct value.

The plugin instance is a component, yes. But the plugin instance lives way longer than we need. The plugin instance lives as long as the plugin is loaded. Thus using the plugin as a component here would be bad, as the memory used by the markdown rendering gets cleaned up when the plugin unloads (usually when the user closes obsidian). This is equivalent to a memory leak and not unloading the markdown at all. Thus we want a component that lives as short as possible. The general idea in lifecycle management is to clean up things ASAP when they are no longer needed.

I means which html i should use?

You should use the plugin.highligher.renderWithEc(...) method.

I'm not sure that understood this.
I've noticed that settings UI uses two column layout that not very helpful in this case ( i've tried first to use Setting constructor).

You can use this.containerEl.createEl("p", { text: "some texthere" }) (not tested, arguments might be slightly different).

It works like a charm. But maybe is there potential performance issue?

Reloading the highlighter can be slow. If you are doing this I would reccoment using the Obsidian API provided helper function debounce with a timeout between 200ms - 1000ms.

@mbilykov
Copy link
Author

You should use the plugin.highligher.renderWithEc(...) method.

You can use this.containerEl.createEl("p", { text: "some texthere" }) (not tested, arguments might be slightly different).

Thanks for information. I did rework code.

But there is small issue with live update. Live code block does not updates own styles properly on reload highlighter.

I did notice that you update activeCodeBlocks on the reloadHighlighter(). And my block does not attached to this map. Is it a good idea to add my block to activeCodeBlocks to force rerender on reloadHighlighter?

Reloading the highlighter can be slow. If you are doing this I would reccoment using the Obsidian API provided helper function debounce with a timeout between 200ms - 1000ms.

I did not notice some issues with that but this is good advice. But i did notice warning issue from shiki lib that plugin did not release hiki resources and that i need use method shiki.dispose()

I added this code to the unloadEC() and issue is gone.

@mbilykov
Copy link
Author

Hi mProjectsCode,

I hope you in a good mood.

I did update code. I added live update for preview code block. Just clear block container and recreate inner content.

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