Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: properly handle directory changes #614

Open
wants to merge 3 commits into
base: harpoon2
Choose a base branch
from
Open

fix: properly handle directory changes #614

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db7749e
fix: properly handle directory changes
cosmicboots Jul 2, 2024
e58645d
make pr-ready
cosmicboots Jul 2, 2024
09e58f9
clear harpoon lists on directory change
cosmicboots Jul 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions lua/harpoon/init.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ function Harpoon:_for_each_list(cb)
end
end

---Resets harpoon data by reading from disk
function Harpoon:reset()
self.data = Data.Data:new(self.config)
self.lists = {}
end

function Harpoon:sync()
local key = self.config.settings.key()
self:_for_each_list(function(list, _, list_name)
Expand Down Expand Up @@ -152,22 +158,28 @@ function Harpoon.setup(self, partial_config)
---TODO: should we go through every seen list and update its config?

if self.hooks_setup == false then
vim.api.nvim_create_autocmd({ "BufLeave", "VimLeavePre" }, {
group = HarpoonGroup,
pattern = "*",
callback = function(ev)
self:_for_each_list(function(list, config)
local fn = config[ev.event]
if fn ~= nil then
fn(ev, list)
end

if ev.event == "VimLeavePre" then
self:sync()
vim.api.nvim_create_autocmd(
{ "BufLeave", "VimLeavePre", "DirChanged" },
{
group = HarpoonGroup,
pattern = "*",
callback = function(ev)
if ev.event == "DirChanged" then
self:reset()
Copy link

@stroiman stroiman Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end)
end,
})
self:_for_each_list(function(list, config)
local fn = config[ev.event]
if fn ~= nil then
fn(ev, list)
end

if ev.event == "VimLeavePre" then
self:sync()
end
end)
end,
}
)

self.hooks_setup = true
end
Expand Down