noatpad / obsidian-banners

An Obsidian plugin that adds banners to your notes
MIT License
616 stars 39 forks source link

[Bug] v2.0.1-beta conflicts with other plugins' editor extension #124

Closed RyotaUshio closed 11 months ago

RyotaUshio commented 11 months ago

Hi, I noticed that some editor extensions from Latex Suite don't work after installing v2.0.1-beta. There may be other plugins that have similar issues.

Latex Suite is a plugin that helps type $\LaTeX$ faster with snippets. For example, this snippet

{trigger: "sq", replacement: "\\sqrt{ $0 }$1", options: "mA"}

let us insert \sqrt{ } by just typing sq.

It works well with Banners v.1.3.3 installed.

https://github.com/noatpad/obsidian-banners/assets/72342591/9df6f87d-a1bb-4e5d-8b68-1068850a2ff5

However, it doesn't with v.2.0.1-beta. It inserts \sqrt{ }\sqrt{ $0 }$1.

https://github.com/noatpad/obsidian-banners/assets/72342591/c58c53f0-ab45-4095-b1ba-aa04e24c7fbd

I think the editor extension of Banners v.2.0.1-beta conflicts with that of Latex Suite.

Debug info (sandbox vault):

SYSTEM INFO:
    Obsidian version: v1.4.13
    Installer version: v1.4.13
    Operating system: Darwin Kernel Version 22.3.0: Thu Jan  5 20:53:49 PST 2023; root:xnu-8792.81.2~2/RELEASE_X86_64 22.3.0
    Login status: logged in
    Catalyst license: none
    Insider build toggle: off
    Live preview: on
    Legacy editor: off
    Base theme: dark
    Community theme: none
    Snippets enabled: 0
    Restricted mode: off
    Plugins installed: 2
    Plugins enabled: 2
        1: Latex Suite v1.8.3
        2: Banners v2.0.1-beta

RECOMMENDATIONS:
    Community plugins: for bugs, please first try updating all your plugins to latest. If still not fixed, please try to make the issue happen in the Sandbox Vault or disable community plugins.
RyotaUshio commented 11 months ago

After commenting out bannerExtender from this line, the issue has gone. So my guess seems to be right.

https://github.com/noatpad/obsidian-banners/blob/4fa6043caefbdec5d8f63b6f8c6c5244df29c398/src/editing/index.ts#L18

  plug.registerEditorExtension([
    // bannerExtender,
    bannerField
  ]);
RyotaUshio commented 11 months ago

The opposite has no effect on the result.

  plug.registerEditorExtension([
    bannerExtender,
    // bannerField
  ]);
RyotaUshio commented 11 months ago

I've added some debugging code in here:

https://github.com/noatpad/obsidian-banners/blob/4fa6043caefbdec5d8f63b6f8c6c5244df29c398/src/editing/extensions/bannerExtender.ts#L32-L34

  } else if (hasEffect(effects, refreshEffect) || docChanged) {
    console.log(docChanged ? 'doc changed!' : 'refresh!');
    console.log(shouldDisplayBanner(bannerData) ? 'upsertBannerEffect' : 'removeBannerEffect');
    newEffects.push(effectFromData);
  }

The result was:

https://github.com/noatpad/obsidian-banners/assets/72342591/32bff4c9-0971-4216-ba6f-e946c74f6f7b

So I assume removeBannerEffect is responsible for this. But the thing is, removeBannerEffect is processed by BannerField, but this bug happens even if I comment out BannerField like this:

 plug.registerEditorExtension([
    bannerExtender,
    // bannerField
  ]);

I don't understand why.

noatpad commented 11 months ago

Huh, it shouldn't be affecting other effects it doesn't know about. It isn't an "effect conflict" because that's something unique defined in the Banners plugin. The idea of transactionExtenders, which is what bannerExtender is, is to process all "effects" and add new ones related to Banners when applicable. My best guess now is that maybe it's overriding other plugin's effects because of the return null statement I have in there? Need to test it

noatpad commented 11 months ago

Okay so it is some weird behavior when you don't use return null within an extender. Still not quite sure if this is a general problem with transactionExtenders or particularly the one in the LaTeX plugin, but it seems to be duplicating the effect even when I pass an empty array of effects. Gonna try adding some stricter conditions to account for this

noatpad commented 11 months ago

Should be fixed now in https://github.com/noatpad/obsidian-banners/releases/tag/2.0.2-beta! There may still be conflicts for extensions that specifically change the frontmatter, but I have yet to find a case for it

RyotaUshio commented 11 months ago

@noatpad Wow thank you so much!!! What a quick work...

It surely works, especially for a short note like this:

https://github.com/noatpad/obsidian-banners/assets/72342591/2c9b70c2-9968-4840-b074-b2b2a8f2f882

However, it causes a force-scroll for a longer note:

https://github.com/noatpad/obsidian-banners/assets/72342591/75cc7591-df72-4537-bdc9-88a4cacf5662

Do you have any ideas on this?