Closed IanLindsley closed 4 months ago
Very cool contribution! Will review and hopefully incorporate into Map View soon enough!
Thanks!
I've been a software developer for almost 18 years, but my JavaScript/TypeScript skills may be a bit rusty. This is also my first time working on an "official" Obsidian plugin. Please let me know if anything needs to be cleaned up, reworked, etc. I greatly appreciate all your suggestions!
Here's an idea of what it looks like.
@IanLindsley do you think you'll be able to continue working on this? If you had enough and don't feel like continuing to work on it, just let me know, and I'll take your PR and continue it, as I think your improvements will be highly beneficial for the plugin :pray:
@esm7, yes, I will be able to continue working on this. Sorry for the delayed response. I was out of town until yesterday evening for work. I will respond to your comments today, and push fixes/updates for you to review by the end of the day tomorrow. Thank you for all your comments and great feedback.
Over the weekend I worked on some issues, and I converted the utils.verifyOrAddFrontMatter
to be using the formal Obsidian front matter API. I think it can help in this PR too, for the purpose of modifying locations, without needing for any external package.
Here's the updated function (not pushed yet):
// Creates or modifies a front matter that has the field `fieldName: fieldValue`.
// Returns true if a change to the note was made.
export async function verifyOrAddFrontMatter(
app: App,
editor: Editor,
file: TFile,
fieldName: string,
fieldValue: string
): Promise<boolean> {
let locationAdded = false;
await app.fileManager.processFrontMatter(file, (frontmatter: any) => {
if (fieldName in frontmatter) {
locationAdded = false;
return;
}
frontmatter[fieldName] = fieldValue;
locationAdded = true;
});
return locationAdded;
}
I think you can do a similar modifyFrontMatterLocation
method that uses app.fileManager.processFrontMatter
.
@esm7, I completed about 70% of the changes above today. I'll finish up the remaining 30% tomorrow (2/13) and push for your review.
Thanks a million for this contribution, users will love it! I might make some usability changes in the upcoming days while playing with it (e.g. I'm thinking the various link settings might be worth making a map state, with a direct map control, rather than a global setting), but either way, the whole credit is yours! :pray: :sunglasses:
Thank you @esm7 for collaborating with me on this. I will pay attention to the usability changes you make in the days ahead; this was my first time working on an official Obsidian plugin, and I still have much to learn. Thank you for your time and patience. I'm particularly interested in seeing what you do with the map state.
I look forward to the future updated release of MapView with these changes! I hope to collaborate with you again in the future!
@esm7, just curious, what do you think the rough timeline is on your future v5.0 release?
@esm7, just curious, what do you think the rough timeline is on your future v5.0 release?
Hopefully no more than a week or two. Basically everything is ready except the functionality in this PR, which I'll spend some time on testing and ironing out.
For starters, I want to:
Also, I haven't tried it on mobile yet.
@esm7, just curious, what do you think the rough timeline is on your future v5.0 release?
Hopefully no more than a week or two. Basically everything is ready except the functionality in this PR, which I'll spend some time on testing and ironing out.
For starters, I want to:
- Make it a map state, which means it will be controlled from the 'View' drop down of the map, can be configured individually for different views, etc,
- Add a behavior that when the user hovers on a marker with links, it grays-out all the unlinked markers and temporarily keeps only the connected ones visible. I noticed that even with the lowest configuration of 1, on my existing maps the links aren't very usable, showing as a huge mess of red lines that I can't understand anything from 😄
Also, I haven't tried it on mobile yet.
Brilliant. All those changes sound awesome. I also understand the purpose of map state now. Pretty cool. Thanks again for this collaboration.
Hi, I'm wondering, how important do you think is the functionality of "resizable markers"? I think that the edges/link display fit really nicely into Map View, but the way the resizable markers work, seems a bit limited and hacky to me. Although it's true to be configurable, I think I rather drop it for now, as the user experience there may be a little rough... What's your take on this?
Hi, I'm wondering, how important do you think is the functionality of "resizable markers"? I think that the edges/link display fit really nicely into Map View, but the way the resizable markers work, seems a bit limited and hacky to me. Although it's true to be configurable, I think I rather drop it for now, as the user experience there may be a little rough... What's your take on this?
@esm7, feel free to remove the the resizable marker functionality if you think it’s hacky and will negatively affect user experience. This was a proof of concept for something we’re modeling at work. If you don’t think it fits in the Map View as it’s currently implemented then nix it. Maybe we can revisit the idea in a future version if it makes sense.
@esm7, does this mean there will be no circular markers at all (not even non-resizable)? I only ask because that functionality is required for the use case at my job. We need circular markers to model non-city nodes (like people).
I understand if this doesn’t fit your conception of the map view. It just means I’ll need to keep my fork around for this use case at work (which I was hoping not to do). The resizing of the circles we can live without. But the circular marker shape is required for this use case.
Oh thank you for pointing that out. I'll make sure the circular markers remain.
Thanks!
Hi, would you mind testing the beta version in the v5.0 branch to verify it suits your needs, and that I didn't ruin anything beyond what I intended ruining? :smile:
The circle marker is now called simple-circle
.
No problem. I'll test the beta version in the 5.0 branch today @esm7.
@esm7, in general, everything looks/works amazing.
One issue I encountered is that after enabling dragging, and then dragging, and completing the drag, the edges (or links) all disappear. To get the links to reappear, you need to toggle the link depth in the dropdown. Take a look at the screenshots below.
Is this a bug? Also, the links used to update/redraw as the marker was being dragged. Did you intend to keep that functionality? Or redraw the edges only after dragging concluded? It looks a little odd for the links to stay in place with the marker being dragged detached from them (screenshot #2).
Oh yes definitely a bug, thanks! I didn't intend to keep the code that moves the edges while the markers are being used because I thought too much is going on there for the visual treat that it is, but I intended the edges to come back once the update is complete... Will sort this out!
Should be fixed now, any chance you give it another test run?
Yep. I'll give it another run now.
BTW, as a side note (but an important one), I'm a bit concerned about the performance of the links mechanism when it comes to resolving updates on a single file. The fact we allow the process to be recursive with arbitrary levels makes it impractical (=quite complicated) to optimize the process of figuring out what links need to be updated when just one marker is modified. As a result, for every single change in a file that contains a geolocation, Map View needs to recalculate all the links (of markers that are included in the filter). This happens repeatedly when users type inside notes, and when many (e.g. hundreds or thousands) are links are displayed, noticeably slows down typing. It might just be enough to warn users about it, but it makes me think -- do you think that the recursion feature ("link level") is really useful? I assume you did it this way because you needed it... Can you shed light on it a little bit? If the feature is reduced to displaying simple links, it will be very fast to resolve which files need to be traversed when a single marker changes. But of course, if that's not what users want, better keep the more complex version.
@esm7, could you explain in a little more detail what you mean by "reduced to displaying simple links" in contrast to the current functionality?
Again, I just coded this up fast for a POC (in like a day or 2). So some of my decisions were made from a purely "just get something working quick" perspective. It's quite possible that my recursive implementation is not optimal or ideal. But I need to know a little more about what you mean by "simple links" to know if this new implementation would break any of our orgs use cases. Thanks.
My retesting confirms that the bug is fixed by the way. Looks great!
@esm7, could you explain in a little more detail what you mean by "reduced to displaying simple links" in contrast to the current functionality?
Again, I just coded this up fast for a POC (in like a day or 2). So some of my decisions were made from a purely "just get something working quick" perspective. It's quite possible that my recursive implementation is not optimal or ideal. But I need to know a little more about what you mean by "simple links" to know if this new implementation would break any of our orgs use cases. Thanks.
If I would have implemented this on my own, I would have made edges represent direct links -- markers A and B would be connected by an edge if either A linked to B or the other way around. Just like the Obsidian graph works.
You took another step and made it recursive: markers A and B are connected by an edge if there is a path of links between them in the length of N, where we made N configurable.
I'm wondering -- what are the considerations and use cases for N > 1
?
@esm7, although I'm still not fully following, my gut says that making "edges represent direct links -- markers A and B would be connected by an edge if either A linked to B or the other way around" would be fine. I think that would still satisfy our use cases. I merely implemented it recursively because that's the first thing that came to my mind.
However, I'm still not sure how this could be true: "A and B are connected by an edge if there is a path of links between them in the length of N, where we made N configurable." --- given the current implementation. But I must be misunderstanding something basic. For example, given this simple vault:
shouldn't there be a link from a to c (forming a triangle) if what you stated is correct? But there's not.
What am I missing? I've attached this simple vault as well.
Nevertheless, feel free to implement the direct link approach if that improves the performance while typing. I can look at the code and see what you changed. I have a feeling that our use cases will be unaffected. Maybe that's the easiest way for me to see what you mean.
Another approach would be to delay the rebuild until the user has stopped typing for some set amount of time (along with the warning you mentioned). But, in my experience, this comes with its own set of complications.
Wow, it's funny how I didn't even think questioning you about the use case for the recursion before, assuming you needed it for a use case I just didn't know :laughing: It seems from your test that "depth 5" is not quite working as I'd expect, but nonetheless, if it was not needed in the first place, I'd be more than happy to simplify the mechanism! I pushed an update to the v5.0 branch that leaves just "off" and "show links". Mind verifying it works as you'd expect, just to make sure we didn't miss anything?
No problem. Thanks for everything @esm7. Testing now.
Everything looks good to me, @esm7. The graphs produced by both the recursive and iterative solutions are the same, which is what I expected. The iterative solution should be faster, of course, minus the (unnecessary) overhead of recursion. I was trying to get this out so quickly, I didn't even consider that an iterative solution would be adequate (thinking "graph"...immediately jumps to recursion in my mind). Since we have a list of all the "vertices" in the graph and can obtain their edges, a recursive traversal is obviously not needed to build the graph.
Thanks for all your work on this! I know it ended up being more work than expected.
I had no illusions and knew it would take a few iterations to finalize, but it's well worth it, as several users asked in the past to have this feature. Thanks for all the help along the way!
Functionality added on top of MapView functionality:
Note: The driver for this functionality is a specific use case within the organization I work for. The idea is to visualize connections (represented by lines) between important people, locations, church networks, etc., overlaid on a map. My feelings won't be hurt if this functionality doesn't fit with your vision for the MapView plugin. Nevertheless, I wanted to submit this PR just in case you thought it might be useful to other users.