javalent / obsidian-leaflet

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

allow for relative geojsonFolder Paths #350

Closed SpocWeb closed 1 year ago

SpocWeb commented 1 year ago

2 Features needed to handle large numbers of Geometries:

I hope you will consider merging this PR into this repository, because leaflet becomes very slow with increasing number of Geometries and recoursing arbitrarily deep can easily freeze the whole Application!

valentine195 commented 1 year ago

Thanks, I’ll review this as soon as I can (hopefully tonight but it might not be until Monday)

SpocWeb commented 1 year ago

Hi, it would be nice if you could review this as I am about to publish some maps that would freeze every obsidian for a long time with unlimited recursion.

SpocWeb commented 1 year ago

Hi, I fixed your change requests, would you review it again?

valentine195 commented 1 year ago

Thank you

Sirs0ri commented 1 year ago

Hey, this feature unfortunately broke the markerFolder feature for me - see #366.

I'm beginning to understand reasoning behind the changes, and I'm currently thinking about how it would be possible to restore the original (still documented) functionality of markerFolder scanning all subfolders inside the given folder.

@SpocWeb, can you help me to better understand the issue that the depth limitation is solving? I don't have such a big vault where the recursion is causing issues, where have you found obsidian's limits? What's your setup where a limited depth is needed? I'm not sure if it's possible for you to share your vault, but that would help me a lot.

I've got some ideas about possible solutions, I'd appreciate your input:

My first instinct would be to make the depth parameter optional, eg. if the user specifies a folder ending in one or more slashes (Subfolder//) that'll limit the depth, while a folder without trailing slashes (Subfolder) will still scan the files inside without a depth limit.

A second thought would be to remove the recursion altogether: Obsidian has an index of all files via app.vault.getFiles() or app.vault.getMarkdownFiles() - do do you think it would be a suitable alternative to iterate over those, and perform the filtering on the filenames?

valentine195 commented 1 year ago

Hey, I’ve been thinking about this quite a bit too. I didn’t think through the ramifications of limiting the recursion depth properly before merging this.

My first instinct was to go with what you indicated and allow depth to be optional, or totally remove it altogether.

I hesitate using non-official parts of the api just because of the additional maintenance overhead that incurs, but definitely a possibility. I will prioritize your issue and hopefully take a look early next week.

Sirs0ri commented 1 year ago

Whoops, I didn't even realise the filemap isn't part of the documented API, I adjusted my comment earlier with better suited alternatives.

LMK if I can help, I'll see if I have any other ideas in the meantime.

valentine195 commented 1 year ago

Obsidian has an index of all files via app.vault.getFiles() or app.vault.getMarkdownFiles() - do do you think it would be a suitable alternative to iterate over those, and perform the filtering on the filenames?

The problem with this is you have to iterate through all files. What the plugin was doing before was getting a specific folder (usually) and then recursing down through the children only.

Sirs0ri commented 1 year ago

What about making the previous behavior of an unlimited depth the default, and a limited depth something users can enable explicitly, via trailing slashes at the end of the folder path?

E.g., folderToScan/ would scan just files directly in that folder, folderToScan// would scan two layers deep, etc, whereas folderToScan without a trailing slash defaults to an unlimited depth? This syntax would work with relative paths as well: . would mean to scan from the current folder with an unlimited depth, ./ would be just files directly inside the current folder, etc.

SpocWeb commented 1 year ago

@Sirs0ri I see that this is a breaking change and I'm sorry for that. But I also think that the danger of locking up the Obsidian Application is more severe. Adding the Slashes to the paths is an easy remedy.

Your proposal of using 'no slash' for unlimited Search is possible, but it can always backfire on you in unexpected ways when you add a large File Structure or many geoJSON Files. Explicitly adding one Slash for every level you want to deeper is better controlled.

As a Motivation have a look at my public Vault-Repo: https://github.com/SpocWiki/_public

It contains 6000 geoJSON Files for all States on Earth organized by Continent and Country. If you add an unlimited Depth Search on a Continent or the whole Repo, then the application freezes for days.

You are welcome to clone it, e.g. as a git-submodule in one of your (private) Vaults. Pull Requests are very welcome!

Sirs0ri commented 1 year ago

That's exactly the kind of example I was hoping for, thanks @SpocWeb!

But I also think that the danger of locking up the Obsidian Application is more severe. Adding the Slashes to the paths is an easy remedy.

Both these points come down to the question "how big is your vault?"

My vault's about an order of magnitude smaller than yours, and I'm not using geojson, only notes with coordinates that I want to display on a map. Obsidian's been handling that just fine, in my case there's not really any risk of locking up the vault. We've got two valid use cases for both sides, mine for an unlimited depth and yours for a limited one. At the same time, the workaround of just adding a bunch of slashes to every instance of markerFolder in my vault woks for now, but it'll stop working reliably as soon as the depth of my "locations" folder gets deeper than the amount of slashes I added. Then I'm gonna start missing notes onmy map, and I hope you can see why I don't wanna have to add 20 slashes every time I want to use the markerFolder option to make it reliable. 😅

I want to have a better look at how the originally used Vault.recurseChildren worked - since it takes a callback as parameter there's a very good chance that the usage of this function can be optimised to do heavy loading in the callback, one geojson at a time, while still giving the render thread enough "time to breathe" between the callbacks so that the loading doesn't lock up the app even if it takes a bit longer.

I'll poke around at this a bit more when I find some time, maybe anyone else can come up with other good ideas for a compromise that satisfies both our usecases in the meantime! I appreciate how Obsidian can be used by both casual and power users, and I hope to find a solution that works well for both of us.

SpocWeb commented 1 year ago

@valentine195 Can/Should I update some Documentation or Wiki Pages to explain the Appending of Slashes to limit the Depth of the Search?

sigrunixia commented 1 year ago

I can answer that, @SpocWeb. The majority of our documentation is moved to https://plugins.javalent.com/home

However, we're being slow and methodical with updating the readme's of reach repo because some, like leaflet, have heavy international usage and our Publish is not ready to accept translations yet. That, and formatting needs to be different there.

I threw up a quick .md file, which you can use. Formatting isn't that important, it's more for example. You can also include an update to the readme.md optionally. Else, we'll slot it in where it needs to go.

SpocWeb commented 1 year ago

@sigrunixia , @valentine195 thanks, I rather thought about updating the ReadMe.md in this Repo. but I can as well update in both locations if you don't mind. It's only about the geojsonFolder and geojson Tags. They have a lot of functionality that is not described (or I couldn't find it) I'd explain that you can use a comma-separated list, relative Paths using '.' and the thing about the recursion depth using as many / slashes as you want to go deep.