bug: visual mode, closing grug & duplicate buffer names

[folke]

There's four issues I encountered that I'm willing to fix, but I'm not 100% if this was intentional or not:

1. with_visual_selection does not work when still in visual mode. The util.getSelection always returns the last selection. A fix for this is to do norm! v when still in visual mode, before getting the selection.
2. When pressing q, grug closes, but an empty grug buffer stays around
3. When opening grug multiple times, you will eventually get errors about duplicate buffer names
4. --hyperlink-format should probably be added to blacklisted flags (see https://github.com/LazyVim/LazyVim/pull/4099#issuecomment-2236674297)

[folke]

Instead of a separate function with_visual_selection, I think it would be cleaner to always use grug_far(), but add an option to start the search with the visual selection?

[MagicDuck]

Thanks for reporting those! 😄

1. with_visual_selection does not work when still in visual mode. The util.getSelection always returns the last selection. A fix for this is to do norm! v when still in visual mode, before getting the selection.

That seems like a legit bug!

Instead of a separate function with_visual_selection, I think it would be cleaner to always use grug_far(), but add an option to start the search with the visual selection?

I am assuming you mean something like:

require('grug-far').grug_far({ use_visual_selection = true , ... })

I am good with doing that if it enables more elegant keybinding. The reason it's a separate function now is that it affects multiple existing GrugFarOptionsOverride opts (prefills.flags and prefills.search). But am happy with turning it into a GrugFarOptionsOverride option, as long as we deprecate with_visual_selection() and point it to the new call so existing usages are not broken.

Somewhat related but completely optional, we could also make the :GrugFar command work with the current range, eg: '<,'>GrugFar

2. When pressing q, grug closes, but an empty grug buffer stays around

Assuming you mean :q. Yes this is on purpose in case you need to refer back to your search later, but need the screen real estate. I got used to "saved searches" from Intellij Idea 😆. This behaviour is also useful in the case you want to toggle with toggle_instance(). You can close with :bd or <localleader>c (by default). I am not opposed to a "close_on_window_close" (or some better name) option though if people want that type of behaviour.

3. When opening grug multiple times, you will eventually get errors about duplicate buffer names

Hmm, this is what context.count appended to the title in updateBufName() (farBuffer.lua) is supposed to be avoiding. It's incremented for each instance when createContext() (grug-far.lua) is called. I cannot reproduce it myself, but if you can, would definitely appreciate a fix!

4. --hyperlink-format should probably be added to blacklisted flags (see https://github.com/LazyVim/LazyVim/pull/4099#issuecomment-2236674297)

Ah, I see the issue, this just need to be forced to --hyperlink-format=none in getArgs.lua. I'll do that!

[MagicDuck]

FYI, fixed (4) in https://github.com/MagicDuck/grug-far.nvim/pull/123

[folke]

3 is caused by restoring sessions and the fact that q only closes the window and not the grug buffer. When restoring a session, empty grug buffers will also be restored and that's probably why I got name conflicts.

As for 2, no big deal. I LazyVim, I just make it close the buffer as well. Maybe not needed to make that optional, since it's pretty easy to do yourself.

1 In LazyVim, I have just one mapping. In visual mode I use the visual selection, otherwise the normal call. I think this might be a keymap that a lot of users might want to have. In that case, the current way of calling two different methods works fine, but is a bit akward.

Thank you for fixing 4!

[MagicDuck]

(3) Ah, I see, on session restore. I tried setting buftype to nofile on the grug-far buffer, but that does not seem to affect mksession. I guess this is related to https://github.com/neovim/neovim/issues/12242 Might still be valuable to set that option on the grug-far buffer though. I guess the best we can do is check the list of all buffers for GrugFar - <count> until we hit something that is not used.

[MagicDuck]

FYI, "fixed" 3 in the sense that there new grug buffers will always have unique names, even after session restore. I could not find a way to completely exclude them from being saved by mksession. I guess that's something that will have to be handled by custom sesssion plugins if people care.

https://github.com/MagicDuck/grug-far.nvim/pull/129

[folke]

Awesome, thanks. fyi: grug-far is now included in LazyVim :)

[MagicDuck]

wohoo! nice! 🥳

[jacktuck]

@MagicDuck congrats 😆

[MagicDuck]

1 In LazyVim, I have just one mapping. In visual mode I use the visual selection, otherwise the normal call. I think this might be a keymap that a lot of users might want to have. In that case, the current way of calling two different methods works fine, but is a bit akward.

Yes, this makes total sense! That way you don't even need an option to toggle that behaviour, just do it differently in visual mode, dunno why my brain was missing that, haha. I can make a PR to do that in the next few days, depending on when I get the time. Or if somebody else gets to it faster that's good too...

[MagicDuck]

thanks @jacktuck , it's a big milestone for grug-far! 😄

[MagicDuck]

@folke let me know if this branch works for you on the LazyVim side 😄 https://github.com/MagicDuck/grug-far.nvim/pull/134

[folke]

lgtm!

[MagicDuck]

@folke awesome! Thanks, it's merged now!