javalent / obsidian-leaflet

Adds interactive maps to Obsidian.md using Leaflet.js
481 stars 29 forks source link

[Bug]: Issues caused by another plugin. #385

Closed zhl111 closed 1 year ago

zhl111 commented 1 year ago

https://github.com/nothingislost/obsidian-hover-editor/issues/213

pjeby commented 1 year ago

A description of the underlying problem can be found here -- Obsidian is now adding the setting tab asynchronously, which means that the disable/enable of the core preview plugin by both Hover Editor and Leaflet schedules two addSettingTab() calls that happen together after both HE and L have done their disable+enable. Likely the simple fix will be to patch addSettingTab to refuse duplicate adds; the more complex (but safer) fix would be that both HE and L would need to wait for the setting tab to exist before calling disable+enable.

(The main difference between these approaches is that the latter would be more robust to future changes in the logic of the Page Preview plugin's onEnable() method.)

pjeby commented 1 year ago

Hm. I may have spoken too soon. It looks like maybe Leaflet is doing a bunch of enable/disable stuff that it doesn't actually need to? The around(HoverPopover.prototype) part is all that's really needed to do what it appears to be doing here. That is, it looks like you could just rewrite the method like so:

     patchLinkHover() {
        const uninstaller = around(HoverPopover.prototype, {
            onShow(old: Function) {
                return function () {
                    if (
                        this.parent?.state?.source ==
                        OBSIDIAN_LEAFLET_POPOVER_SOURCE
                    ) {
                        this.hoverEl.addClass("obsidian-leaflet-popover");
                    }
                    return old.call(this);
                };
            }
        });
        this.register(uninstaller);
    }

That is, just literally delete the rest of the method and the type definitions for internalPlugins. This would be safer than any of my previous ideas since it would avoid the conflict altogether rather than working around it.

valentine195 commented 1 year ago

I tried that but I don’t think it was being respected without reloading the page preview plugin - not 100% sure, it was awhile ago.

I did investigate this a bit when I had a few minutes and noticed the new behavior. What confused me is it didn’t happen in English. Do you know why that would be?

pjeby commented 1 year ago

Given that you're only patching the prototype onShow, there shouldn't be anything that would require the enable/disable. HE does that only because it patches methods of the page preview plugin itself and needs the plugin to re-register the event handlers. AFAICT Leaflet is only adding a class to the HTML of created popovers, and this doesn't affect any event handlers.

I can confirm the issue occurs in English, though - you have to start Obsidian with both HE and Leaflet enabled, as the issue is a race condition with both plugins triggering a disable-enable pair at "the same time" from the perspective of the async call.

valentine195 commented 1 year ago

Given that you're only patching the prototype onShow, there shouldn't be anything that would require the enable/disable.

Unfortunately I don’t see my patch occur if I don’t cycle the page preview plugin when HE is enabled. Could be a plugin load order issue there.

AFAICT Leaflet is only adding a class to the HTML of created popovers, and this doesn't affect any event handlers.

correct

I can confirm the issue occurs in English, though - you have to start Obsidian with both HE and Leaflet enabled

I tried several times to get this to occur in English and never replicated it. I was about to just close the issue until I decided to try a different language just to make sure.

pjeby commented 1 year ago

Unfortunately I don’t see my patch occur if I don’t cycle the page preview plugin when HE is enabled.

Maybe I'm misunderstanding things, but AFAICT Leaflet's onShow() patch doesn't do anything when Hover Editor is enabled. Hover Editor uses a subclass of HoverPopover that doesn't use the base class's onShow implementation, so IIUC the patch would never affect Hover Editor, only non-Hover Editor popups.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.