Skip to content

Conversation

kostabekre
Copy link

In order to implement search by alises, a more faster solution is needed than grep search.
In order to solve this, the cache of the vault is created on the startup as a JSON file and updated every time a note is changed.
The cache is used in search of the notes and their aliases.
For the current moment only telescope search is available.

Solves this issue.

@neo451
Copy link
Member

neo451 commented May 9, 2025

really nice feature! would need to look into the code more in the next few days, meanwhile you can fix the style issue with make style and solve those small things I commented.

@kostabekre
Copy link
Author

@neo451 Maybe I need to add additional configurations to stylelua, but running make style doesn't show anything (I already used the command and made changes before submitting the request)

@neo451
Copy link
Member

neo451 commented May 9, 2025

@neo451 Maybe I need to add additional configurations to stylelua, but running make style doesn't show anything (I already used the command and made changes before submitting the request)

No idea why is that, but you can look at the action result, stylua wants to add a ;, because next line is a bit of a unidiomatic lua, where the start of expression is in (). Maybe you have a global stylua configuration, or there's issue with our makefile and workflow behaving differently. Will look into that.

ps: My local stylua does inform me of this error.

@kostabekre
Copy link
Author

Well, I tried to different options, but my stylua says that ; is not right in this line.
Also my linter (luacheck) removes that semicolon.
image

@sotte
Copy link
Contributor

sotte commented May 11, 2025

Quite interesting. I was actually just playing with porting a sqlite full text search that I used and adapting it for obsidian. While doing that I thought it might be cool to use sqlite as a cache.

This goes into a similar direction.

@kostabekre it would be interesting to see how much faster this is compared to the base case which just uses rg.

@sotte
Copy link
Contributor

sotte commented May 11, 2025

Right now this implementation only works for telescope, right? I think we should have a picker independent way of using a cache.

@kostabekre
Copy link
Author

@sotte I was thinking of using SQLite myself, and I have founded this repository.

But after some time and searching the implementations of other people, I decided that adding a new dependency won't be a good idea.

  1. Using SQLite in our repository would demand both installing the SQLite on the user's computer and adding the plugin.
  2. The plugin I mentioned above is not actively maintained. I don't know the reason but I think it's either people use more appropriate solution for their use case or they use the SQLite without the plugin.
  3. There are list of repositories, that use the SQLite plugin. One of them removed the SQLite completely. Well, it's a weak reason, but when I was researching, I thought using a plugin that is not actively used by others is not a good idea.

Well, I don't think SQLite is bad or anything, it might be the ideal solution for our needs but I decided take a more simple route both for users and for developers - using JSON.

I was actually just playing with porting a sqlite full text search that I used and adapting it for obsidian.

Could you describe how that works?

@kostabekre
Copy link
Author

Right now this implementation only works for telescope, right? I think we should have a picker independent way of using a cache.

Yes, for now I made the implementation only for telescope (and not a very good one, it needs some polishing) because I haven't used other pickers.
I made the basics, and I thought the code can be reviewed by other people.
The other pickers can be done in separate pull requests or in this PR some time later.

@kostabekre
Copy link
Author

kostabekre commented May 11, 2025

it would be interesting to see how much faster this is compared to the base case which just uses rg.

Okay, I can do that.

@neo451
Copy link
Member

neo451 commented Jun 30, 2025

@kostabekre sorry for the long process, I finally addressed some pain points in the codebase #251 also see #237

The thing that kept me from merging this is, you make cache a class, the only valuable info it stores is the client, and then you add it as a field in the client, it is not bad but just did not sit well with me.

And you are only using client to access state, like dir and opts. and now you can do it like Obsidian.dir, and Obsidian.opts.

So now:

  1. cache don't need to be a abc class, just a simple table as a module.
  2. cache don't need to be a field in the client field.
  3. you can update a field in the global Obsidian.cache, the result of Cache.get_cache_notes_from_file, so that users can use them for scripting.
  4. Cache.new's logic can be moved to Workspace.new in the new architecture, if opts.cache.enabled then enable filewatch and etc.
  5. all the fields that is currently in the Cache.xxx, only rebuild_cache makes sense to be exposed for use in the command, others are all for internal use, so we can make them all local, it is efficient and cleaner.

@kostabekre
Copy link
Author

@neo451 We are not in hurry, so I think we shouldn't be worry that it takes long! It's much better to take slow but controlled steps.
I didn't expect that the PR would be merge right away, because I had little experience writing plugins for lua (I wouldn't say that I have much more now, because I don't understand much what is going on in the plugin :D). I just try to repeat the workflow of the code, so I used the abc because I thought I was doing the right thing.

I will make the changes in the following days.

@neo451
Copy link
Member

neo451 commented Jun 30, 2025

thank you for your continued work, I kept being sorry because I am the kind of person whose passion dies down super quickly if I don't get positive feedback lol.

Yes lua is quite a different experience from like for example python which is what the og author have a background in, and why there's an abc class. But lua tables are 90% of the time expressive enough and class is no need. Also plugin development is quite a different mindset.

I guess the rule of thumb is not to use a class if you don't store much state and only use one instance of the class in an neovim session. I think I need to write some of these in the CONTRIBUTING.md.

@kostabekre
Copy link
Author

@neo451 Hi!

Cache.new's logic can be moved to Workspace.new in the new architecture, if opts.cache.enabled then enable filewatch and etc.

Well, I can't really activate cache without exposing a method like activate_cache in the cache module.
But as a concept, filewatch really belongs to a workspace, but the logic is tied to cache. I think I need to create a way, where anyone can subscribe to the workspace update event.

And I can't use opts.cache because the opts are belong to the workspace. Did you mean Obsidian.opts?

The following comment is just my thoughts about the style of the project.
While I was changing my code, I thought about the abc class and the global variable Obsidian, which is a singleton. I come from OOP (C#) and had experience in game development.
From my experience, global variables (singletons) have bad reputation to much easier to break other code and hide dependencies and I think that's why the author used the client class.
But I don't mean the global variable is bad idea! It's really much easier to get the opts variable. I just hope the Obsidian variable will stay slim as possible and won't have much logic.

@neo451
Copy link
Member

neo451 commented Jul 6, 2025

yes Obsidian.opts, it will be properly resolved if the workspace overrides it.

yeah I know global variables have bad rep, but just think of it as some cache for the config values, it should not contain too much logic like you said.

@kostabekre
Copy link
Author

@neo451 Obsidian.opts is empty when creating a workspace, so I placed activate_cache in workspace.set.

For now I didn't create an event handler when workspace is updated.

@neo451
Copy link
Member

neo451 commented Jul 6, 2025

yes I was mistaken, I meant workspace.set

@neo451
Copy link
Member

neo451 commented Jul 9, 2025

@kostabekre I did not catch this part last time

For now I didn't create an event handler when workspace is updated.
meaning it is not working now? or is this a question or issue?

@kostabekre
Copy link
Author

@neo451 sorry, I meant that I didn't implement the event when the workspace was changed. I didn't have time to finish.
I wrote the following 3 days ago:

But as a concept, filewatch really belongs to a workspace, but the logic is tied to cache. I think I need to create a way, where anyone can subscribe to the workspace update event.

Maybe it's not necessary, but I thought it could be more flexible that way.

@neo451
Copy link
Member

neo451 commented Jul 9, 2025

But as a concept, filewatch really belongs to a workspace, but the logic is tied to cache. I think I need to create a way, where anyone can subscribe to the workspace update event.

Since there's like the concept of stuff like dynamic workspaces (which I don't understand now as well), I think we can follow the current approach as it is.

@kostabekre
Copy link
Author

Okay! Now, there are two things that are not implemented:

  • Ignore folders. I use syncthing and it sometimes creates conflict files, which are picked up by filewatch and cache.
  • Only one instance of neovim should create file watch handlers for a workspace. If there are several instances of neovim, each will create file watch handlers and because of that I have seen weird errors.

I will read how to do that and implement the solutions on the weekends.

@neo451 neo451 mentioned this pull request Jul 21, 2025
26 tasks
@kostabekre
Copy link
Author

@neo451 hi! Is there anything that I need to do?

@kostabekre
Copy link
Author

The lock file is created in the root of the workspace, maybe I need to move it to other place?

@neo451
Copy link
Member

neo451 commented Aug 6, 2025

sorry I don't have time to properly review this recently, but I had the idea the other day that we should maybe put cache in vim.fn.stdpath("state") / "obsidian", but how do they correspond to each workspace, (in case of moving and stuff) is a problem, but maybe worth looking into so that we don't put a new file in the vault.

Also, I am working on #339 for limiting jobs (will merge soon), and we don't want to rely on plenary no more on the main, so also look into that.

@GordianDziwis
Copy link

Why don't you use rg instead? It is an external dep, but it is most def fast enough for searching a few 100,000 wiki pages.

@kostabekre
Copy link
Author

Why don't you use rg instead? It is an external dep, but it is most def fast enough for searching a few 100,000 wiki pages.

it's not just use rg aliases: ..., because the value of the field can be in various ways.
A custom logic would be needed to cover each case and it's very error prone.

I'm not advanced user of rg but I think using the nvim.obsidian's API is the better way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants