ThePrimeagen / harpoon

MIT License
7.2k stars 385 forks source link

Harpoon doesn't save cursor position after quit #441

Open romado77 opened 11 months ago

romado77 commented 11 months ago

I've noticed an issue with Harpoon related to the persistence of cursor positions. When a file is harpooned, the cursor position is saved upon quitting neovim (I could see this from harpoon.json). However, when reopening the harpooned file and moving the cursor to a new position, the Harpoon doesn't preserve the updated cursor position if I quit from neovim again. So every time I open harpooned file it jumps to a position which was initially saved when file was harpooned.

My configuration:

local harpoon = require("harpoon")

harpoon:setup({ settings = {
    save_on_toggle = true,
    sync_on_ui_close = true,
} })

vim.keymap.set("n", "<leader>a", function()
    harpoon:list():append()
end, { desc = "Add current file to harpoon" })
vim.keymap.set("n", "<leader>h", function()
    harpoon.ui:toggle_quick_menu(harpoon:list())
end, { desc = "Toggle harpoon menu" })

vim.keymap.set("n", "<C-1>", function()
    harpoon:list():select(1)
end, { desc = "Select harpoon item 1" })
vim.keymap.set("n", "<C-2>", function()
    harpoon:list():select(2)
end, { desc = "Select harpoon item 2" })
vim.keymap.set("n", "<C-3>", function()
    harpoon:list():select(3)
end, { desc = "Select harpoon item 3" })
vim.keymap.set("n", "<C-4>", function()
    harpoon:list():select(4)
end, { desc = "Select harpoon item 4" })
theKnightsOfRohan commented 11 months ago

I can replicate this issue. Here is a video, in two parts because size requirements:

https://github.com/ThePrimeagen/harpoon/assets/114779675/a3172678-29b3-462f-9ec3-842dc1ba0a8e

https://github.com/ThePrimeagen/harpoon/assets/114779675/f206ed70-56c8-4173-82a0-242874d1ba22

Harpoon isn't updating the global list when you move places. The logger shows the parent list, and not the updated list. The updated list is also not being written. In addition, harpooning at the new position doesn't do anything, and it will still move to the initial harpoon position upon a new session.

theKnightsOfRohan commented 11 months ago

It's also a failing test in harpoon_spec.lua, but I have no idea why the error is ATTENTION, or why utils.create_file is bugging out lol

Testing:        /Users/rohanseth/Documents/GitHub/harpoon/lua/harpoon/test/harpoon_spec.lua
Fail    ||      harpoon when we change buffers we update the row and column
            ./lua/harpoon/test/utils.lua:61: Vim:E325: ATTENTION

            stack traceback:
                ./lua/harpoon/test/utils.lua:61: in function 'create_file'
                ...cuments/GitHub/harpoon/lua/harpoon/test/harpoon_spec.lua:20: in function <...cuments/GitHub/harpoon/lua/harpoon/test/harpoon_spec.lua:16>
roycrippen4 commented 11 months ago

Any updates or workarounds?

ThePrimeagen commented 11 months ago

I'm going to have to think about a way to be able to debug. It works on my machine, but that sucks. I'll have to make the log be able to save to a file. And then have you share the file

theKnightsOfRohan commented 11 months ago

I've isolated it down to the BufLeave function definition in lua/harpoon/config.lua, line 184. The log statement which should be updating position isn't getting called, yet the behaviour is replicated regardless. I've also looked for other places where item.context is updated, but none of those are changed anywhere else, only referenced by the nvim_win_set_cursor function. An explanation I'm thinking of is that this behaviour is being done by neovim inherently, rather than harpoon for whatever reason.

theKnightsOfRohan commented 11 months ago

Additionally, after a but more searching, I'm seeing that the BufLeave function defined by the default config doesn't seem to be called anywhere, as there is no place where an autocmd is created using that function.

roycrippen4 commented 11 months ago

@ThePrimeagen two questions: What is your config; the default with both sync_on_ui_close and save_on_ui_toggle set to false? And do your marks persist if you close a buffer or you exit neovim?

arnevm123 commented 11 months ago

I have the same issue + I don't get line numbers anymore sometimes, not sure how that happened.

ThePrimeagen commented 11 months ago

I'll see if I can spend some time on this tonight.

I'll stream it to

bsolar17 commented 11 months ago

I also have this issue and I thought it was due to how BufLeave is supposed to work as the documentation states the following:

Before leaving to another buffer. Also when leaving or closing the current window and the new current window is not for the same buffer.

Not used for ":qa" or ":q" when exiting Vim.

ThePrimeagen commented 11 months ago

I'm going to rework how the files are stored.

This will prevent the aforementioned problem from happening

maxrzaw commented 11 months ago

I am currently writing a custom list and I think I discovered what is causing this. I think the issue is in BufLeave. It is looking up by the display name and the display name is not always the same as vim.api.nvim_buf_get_name(bufnr).

I added some log statements to BufLeave and it finds the item when the display name is the path from root, but does not find it when it is a relative path.

Having the full file path as part of the context would be helpful for solving this issue and I think it would be helpful for many other use cases.

maxrzaw commented 11 months ago

I have been looking into this a bit and there are at least two issues going on here.

  1. The display name does not match the name of the file so the current BufLeave function does not always update the position. I have recreated this with a unit test and have a fix for this case.
  2. VimLeavePre is called, but in _for_each_list() seen is not true and lists is sometimes nil.

Edit: I have issue 2 narrowed down to a couple things related to _for_each_list():

arnevm123 commented 11 months ago

Workaround that seems to work, but could use (a lot of) improvement:

VimLeavePre = function(_, list)
    for bufnr = 1, vim.fn.bufnr("$") do
        if vim.fn.buflisted(bufnr) == 1 then
            local bufname = vim.api.nvim_buf_get_name(bufnr)
            local item = nil
            for _, it in ipairs(list.items) do
                local value = it.value
                if value == bufname then
                    item = it
                    break
                end
            end
            if item then
                vim.api.nvim_set_current_buf(bufnr)
                local pos = vim.api.nvim_win_get_cursor(0)
                item.context.row = pos[1]
                item.context.col = pos[2]
            end
        end
    end
end,
ThePrimeagen commented 11 months ago

I'm going to be changing how I save files here soon.

Instead of one big configuration, I will have many small files. This is going to allow me to not wait until exit to save

roycrippen4 commented 9 months ago

four-to-six-weeks-later

theKnightsOfRohan commented 9 months ago

Unfortunately, Prime is still APM-Brained. We should probably expect harpoon to be slowed down for at least another month, maybe even two.

roycrippen4 commented 9 months ago

@theKnightsOfRohan I get it. It's open source, it's his project, and he can do as little or as much as he wants with it.

whoop-t commented 9 months ago

Still issue at time of this comment, just bumping for others who end up here

davvid commented 9 months ago

:help restore-cursor provides a stock way to save cursor positions for all files, not just for files known to harpoon. There are references online that suggest that vim shipped with a default.vim that handled this in the past.

Another approach is to leverage the viminfo mkview and loadview features to restore marks and cursors after quitting. The basic idea is sketched out here: https://www.reddit.com/r/vim/comments/ex1vlv/comment/fg63erw/

I took that and minimally wired it into harpoon2 in #525 as a proof of concept. The code's not terrible so it could be merged as-is, but it does call into question whether the context table needs to keep track of row and col anymore now that vim's handling that part.

I'm not sure if this is the best way since it's setup to save/restore on switch and sync unconditionally, but it does work pretty reliably.

GitMurf commented 8 months ago

@davvid I have ran into some weird issues with cwd, folds etc. when using mkview, etc. Depending on your global settings, more stuff than you want could be saved unfavorably and I think the purpose of harpoon is to be pretty explicit and defined about what it is trying to harpoon / focus in on. Just my 2 cents that bringing views in could get messy pretty quickly based on my experience with other plugins.

davvid commented 8 months ago

Thanks @GitMurf +1 to that. Indeed, it'd be better to make it so that the row and col fields are always correct so that a proper and more minimalist approach can be taken.

Another reason why we'd want a more minimalist implementation is that mkview and loadview are slower than setting just setting the cursor directly.

davvid commented 8 months ago

Alrighty @GitMurf take a look at #533 ~ it's behaving perfectly as far as I can tell.