Repository navigation
Reuse opened windows - #15
Conversation
There was a problem hiding this comment.
Let's make this new behavior the default. I feel strange about adding an option for this, it feels like a bug fix. I think if I already have a window open with the buffer, reusing it is the natural behavior.
Note: don't be discouraged by Request changes, I think the PR does a good thing, I just don't like the option.
Another note:
For disclaimer purposes, I used LLMs to help me with tests and how to run tests since I've never ever ran tests in Lua before.
That's welcome here. Code can be good or bad, and for the purposes of contributing to this repo, it doesn't matter what you used to write it.
| reuse_opened_windows = true -- default: false | ||
| }) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I appreciate the intention of keeping backward compatibility, but I feel like this is more of a bug fix than an optional enhancement. I would probably not make this an option but make reuse_opened_windows = true the default, without an option to configure that. If that breaks someone's workflow, they can open an issue.
Thoughts?
There was a problem hiding this comment.
If we make it a bug fix this can become 1.2.1 instead.
| -- Create a second buffer for a second window | ||
| local bufnr2 = vim.api.nvim_create_buf(true, false) | ||
| set_lines(bufnr2, line_count) | ||
| local temp_path2 = luv.os_tmpdir() .. "/nvim-recall-test2-" .. luv.hrtime() .. ".txt" | ||
| if jit and jit.os == "OSX" then | ||
| temp_path2 = "/private" .. temp_path2 | ||
| end | ||
| table.insert(temp_paths, temp_path2) | ||
| vim.api.nvim_buf_set_name(bufnr2, temp_path2) | ||
| vim.api.nvim_buf_set_option(bufnr2, "modified", false) | ||
| vim.cmd("w") |
There was a problem hiding this comment.
Nit:
I find the readability of this test setup a bit rough. The comment -- Create a second buffer... is a "what" comment, but as-is, I have some "why" questions:
- Why does the test need a proper file in
tmp? The other tests seem to get by with justnvim_create_buf - Why does OSX need to be handled in a special way here?
Suggestion: make a function that returns just what you need for this test, and add "why" comments there.
There was a problem hiding this comment.
This piece of code is pretty much a copy-paste of the before_each block here. The code in the before_each does create a temp file for each test and also handles OSX specially. I wasn't sure why and when looking at that I was having the same questions as you do, but since it was the first time I ever test in Lua I simply assumed this is how it must be done. If that is actually unnecessary I can remove it yes.
There was a problem hiding this comment.
Hm, let's maybe put it into a function so that it's not repeated in multiple places. I can try to understand it at some other point but it should at least not be repeated multiple times.
There was a problem hiding this comment.
If it's already repeated in multiple places then ignore this 😅
There was a problem hiding this comment.
Indeed, I will make it a simple function!
| end | ||
|
|
||
| M.set_cursor_for_mark_in_current_window = function(mark) | ||
| vim.cmd("silent buffer " .. mark.file) |
There was a problem hiding this comment.
Nit:
I'm actually no longer sure why silent buffer is necessary here. I know I added it but it's been a while. It seems to be forcing some indirection because you are trying to only run it in the current_window case.
I think I would rather inline this logic into the only call-site because you already run a check on window_id there: win_id_to_reuse ~= -1, so it would seem natural to use that location to make decisions.
There was a problem hiding this comment.
I think it was necessary in case if the mark.file wasn't loaded in any buffer (which also fallbacks to the current_window case)
|
Thanks for your comments @fnune ! I'm not discouraged :) on the contrary, thanks for the comments, Lua and Neovim is something new to me, so I expected I do/did something wrong. I can address your comments and make a patch on Monday (currently on my phone) |
|
Hey @fnune , I've addressed your comments :) |
|
Once CI passes I'll merge! |
|
The CI failed due to minor stylistic issues, I re-run the Stylua and Prettier locally and commited the edits. I think CI should pass now |
|
Thanks @bvdmitri! |
When enabled, navigation reuses existing windows displaying the target buffer instead of switching buffers in the current window. Useful for split-window workflows. Fixes #14
For disclaimer purposes, I used LLMs to help me with tests and how to run tests since I've never ever ran tests in Lua before. It took a fair amount of time to debug a test since I'm not familiar with that, but I think it works. I also tested it locally by installing the plugin from my personal branch.
The code implementation while inspired by LLM was completely rewritten and improved by me. The README and the CHANGELOG were written by me and polished my LLM since English is not my native language.