ThePrimeagen / harpoon

MIT License
6.25k stars 348 forks source link

Harpoon2: Wrong state is loaded when working with multiple tabs - having different working dirs #612

Open stroiman opened 1 week ago

stroiman commented 1 week ago

WARNING If this is about Harpoon1, the issue will be closed. All support and everything of harpoon1 will be frozen on master until 4/20 or 6/9 and then harpoon2 will become master

Please use harpoon2 for branch

When opening a tab, and setting a different working folder in the new tab, the harpoon list is not loaded next time I start neovim.

I believe that the issue occurs because the data file uses the hash of the current working directory for to generate the file name.

local function fullpath(config)
    local h = hash(filename(config))
    return string.format("%s/%s.json", data_path, h)
end

When neovim loads, it uses the hash of the initial working folder, but when I open a new tab with a different working folder, it appears that the hash of the new working folder is used when saving the lists. So now lists are saved in a different file than the one loaded at startup.

Is there a reason for the hashing rather than one file?

The data file already contains data for multiple working directories already.

stroiman commented 1 week ago

I made this change in my own fork, https://github.com/stroiman/harpoon/commit/f9a2ff215379fadf1e1e4f781ca2dc31413f1f40

Seems to work nicely (after 2 minutes of testing), but I wouldn't PR it as I don't understand the reasoning behind multiple files - but if you think this is a reasonable change, I'll gladly clean it up (all the callers still pass the config to the function - which really doesn't have to be a function anymore, as it no longer has any arguments)

cosmicboots commented 15 hours ago

I think the deeper issue here is that Harpoon's Data isn't re-read from disc when changing directories. Because harpoon so heavily relies on vim's current working directory (a great decision imo), I believe it would be best to clear out harpoon's state and re-read it from the filesystem whenever vim's working directory changes.

I believe this gives the best of both worlds: allow directory changes and also keeping the separate state files (which I assume are needed for some of the more advanced features of v2).

I'll open a PR soon with the changes.

stroiman commented 15 hours ago

I think that there is something wrong in that statement.

While neovim has a working directory, each tab can have a separate working directory, and even windows can have separate directories.

In my case, I have a "project" open in one tab, and when I open my neovim config in another tab, that new tab has a different working directory, so the idea to "re-read it from the filesystem whenever vim's working directory changes" makes no sense. The directory didn't change. The new tab was just initialised with a different working dir.

cosmicboots commented 15 hours ago

Ah, that's my bad. I misunderstood you're issue and thought it was the same issue I was having. I honestly have never used window local or tab local directory changes, so it hadn't even crossed my mind.

That being said, the changes I proposed should still work. I just tested with tabs (and windows) in different working directories, and the correct harpoon list is shown in relation to the tab/window's location.

stroiman commented 2 hours ago

I looked at your PR, and I think that it would actually ruin my flow.

So what I do is, while working on a project, I detect an common editing pattern that I want to optimize. I open init.lua in a new tab, setting the working directory for that tab to the .config/nvim folder, thus all navigation in that tab works nicely for editing the config. Once I've made that change, I re-source the config and go back to the project.

If I were to use your branch, once I get back to my original tab, the Harpoon marks would be gone.

But try to see if using my fork doesn't fix your problem? I think it should. Rather than detecting directory changes, it just stores all marks in one file rather than multiple files. But the config file is already grouped by working dir. So so far that change seems to work well.

But as I wrote before, I don't understand the intention behind multiple data files, which is why I didn't want to PR it yet.

stroiman commented 2 hours ago

Or maybe it should also react to TabEnter and WinEnter.

Multiple files is probably to avoid loading useless state into memory. When I work on project A, I don't need harpoon marks for project B in memory.

Maybe another solution is, when switching directory, rather than replace the state, merge the state. And maybe that's better than TabEnter, WinEnter - as those would be triggered a lot, and most of the times, there wouldn't be anything to do.

Then, when saving, don't use current dir. The data file is grouped by working dir, so rather than one save, save each working dir from the data separately.