javalent / obsidian-leaflet

Adds interactive maps to Obsidian.md using Leaflet.js
499 stars 30 forks source link

Define custom marker types in json files #400

Closed nathonius closed 1 year ago

nathonius commented 1 year ago

Slightly WIP, there's one issue I've yet to solve. Was hoping to get some feedback on this. First, a summary:

Still to-do / feedback requested:

Currently, the marker file is read after the map is created:

    // renderer.ts
    async buildMap() {
        if (this.options.type === "real") {
            this.map = new RealMap(this, this.options);
        } else {
            this.map = new ImageMap(this, this.options);
            // ...
        }
        this.map.localMarkerTypes = await this.plugin.getLocalFileMarkers(this.file);

This causes markers of types defined in the json file to not render correctly until they are edited or otherwise re-rendered, since the marker types aren't available when the map is first constructed. Originally I tried to read the file before creating the map and then passing any additional markers as one of the option parameters, but something about awaiting ANY async call before the map is created causes the plugin to error out. This code throws this error:

    // renderer.ts
    async buildMap() {
        const somePromise = Promise.resolve('some value');
        const theValue = await somePromise;
        if (this.options.type === "real") {
            this.map = new RealMap(this, this.options);
        } else {
            this.map = new ImageMap(this, this.options);
plugin:obsidian-leaflet-plugin:2 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'contentEl')
    at uo.registerMapEvents (plugin:obsidian-leaflet-plugin:2:1558633)
    at uo.postprocessor (plugin:obsidian-leaflet-plugin:2:1556132)
    at t.initDOM (app.js:1:1406811)
    at t.toDOM (app.js:1:1403089)
    at t.sync (app.js:1:389594)
    at e.sync (app.js:1:365245)
    at app.js:1:406709
    at e.ignore (app.js:1:480071)
    at t.updateInner (app.js:1:406505)
    at t.update (app.js:1:406266)

I've really got no clue why awaiting any promise (even an already resolved promise) would cause this, was hoping to get a second pair of eyes on that. My assumption is that somewhere someone is trying to access the map synchronously and introducing the await causes it to be instantiated in a later cycle. But I wasn't able to find the source of the issue.

valentine195 commented 1 year ago

Marker file name setting? I wasn't sure if this would be overkill.

I agree, this is overkill. Forcing people to use a markers.json filename is good with me.

valentine195 commented 1 year ago

Fix the bug described below.

This happens because the post processor assumes the renderer creates the map directly in the same event loop call.

By adding in the asynchronous call before the map creation, the registerMapEvents call happens before the renderer's map is created (because the await takes it out of the event loop, to be executed later, after the promise returns).

I think the best solution would be to move the registerMapEvents call to the renderer instead, and register them in the buildMap flow. There's nothing about them that can't be done in the renderer and then it's more explicit.

Honestly, the whole post processor could be cleaned up a lot. The code is a mess unfortunately, kudos for being willing to dive into it lol

nathonius commented 1 year ago

I think the best solution would be to move the registerMapEvents call to the renderer instead, and register them in the buildMap flow. There's nothing about them that can't be done in the renderer and then it's more explicit.

Honestly, the whole post processor could be cleaned up a lot. The code is a mess unfortunately, kudos for being willing to dive into it lol

Well that wasn't the only place trying to access the map synchronously. šŸ˜ It was also being accessed inside the onload of the renderer itself. Rather than trying a large scale refactor, I added a safety measure to allow those places (which are luckily already async) to wait on the map to be built first.

nathonius commented 1 year ago

As a test, I replaced all my marker types defined in settings (10 types), with a local file. And my map with all those markers still rendered just fine. šŸ˜ image