fix: properly handle directory changes#614
Conversation
This change clears out the in-memory harpoon data and re-read it from the disk whenever a `DirChanged` event is triggered. This makes sure the in memory data always reflects the harpoon lists for the current working directory.
|
Tested this and it fixes #612 for me. |
|
I finally got around to test this, and based on the initial testing, it works as expected. (I submitted #612) E.g. I have keyboard shortcuts to open my init.lua in either a new tab, or new window, and set the local dir for better file navigation - e.g. telescope find files show either project-related files or files in my modularised configuration ( When switching between a project-window and config window, the harpoon list shows the relevant marks, i.e. project-related or config related marks, and the list stays persisted after I quit and reopen neovim (I believe that was the original issue). I will drop my own branch, and point to this in my lazy config until it gets merged. |
| pattern = "*", | ||
| callback = function(ev) | ||
| if ev.event == "DirChanged" then | ||
| self:reset() |
There was a problem hiding this comment.
Suggestion for minor technical improvement:
As far as I can tell from reading the code, this change seems to reload the entire state. The data model supports having lists multiple lists by working dir, so perhaps rather than completely reloading state, just load the state for the new dir if it doesn't already exist in memory, to optimize loading? Then the new file is only loaded the first time you switch to a new directory.
Having said that, the harpoon file is so small that reading it in when changing tab or window is not a real problem. I don't experience any delays when using this branch, i.e. changing tabs is instantaneous.
There was a problem hiding this comment.
I'm inclined to leave it reloading from the disk completely. Reading from a file is fast enough the user shouldn't ever notice it, so it seems like a fairly negligible performance cost when considering what it would take to properly store all directory states in memory.
One big issue I could see with caching directory states in memory is garbage collection. When do we remove a directory's state from memory? Keeping them indefinitely is asking for memory leaks (albeit small) for anyone who a long running neovim instance when working with all their projects. It's also not obvious to me what a good heuristic would be to clear them out from memory (only keep 10..?, keep for a time..?).
Ultimately, I don't see the potential performance gains being worth complicating the state management logic further.
|
This doesn't really work as expected (at least under the hood). After switching working directory, there are two lists stored, both with the default name. To make it work, you also need to set self.lists = {} in the reset method. |
|
@mike-jl good catch! Fixed in 09e58f9. Tested and works from a user perspective as before, but now we're not dragging around old lists with us when we change directories. |
|
How do we get this merged 😭 |
Porting ThePrimeagen#614 to harpoon2
|
cc: @ThePrimeagen |
5 participants
This change clears out the in-memory harpoon data and re-read it from the disk whenever a
DirChangedevent is triggered. This makes sure the in memory data always reflects the harpoon lists for the current working directory.Fixes #612