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.

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

Use a map instead of a hook to jump to grep result #5149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lobre
Copy link
Contributor

@lobre lobre commented Apr 19, 2024

This has been discussed a little bit on Discord, but I think we do not need a hook for the <ret> mapping in the grep module.

I guess this was something that was necessary in the past, but I am not sure that we need it anymore.

Also, it prevents from overriding the <ret> key easily at the user level with:

map global normal <ret> "..."

@@ -45,8 +45,7 @@ hook -group grep-highlight global WinSetOption filetype=grep %{
}

hook global WinSetOption filetype=grep %{
hook buffer -group grep-hooks NormalKey <ret> jump
hook -once -always window WinSetOption filetype=.* %{ remove-hooks buffer grep-hooks }
map buffer normal <ret> :jump<ret>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was also surprised why we use a hook here instead of a mapping.

Turns out there is a behavioral difference; consider

map global normal <c-j> <ret>

This mapping will fire NormalKey <ret> hooks, thus triggering the existing mapping.
It would not trigger the proposed mapping.

I wonder if we should move away from relying on this subtle difference.
One could use execute-keys -with-maps instead.

There is a related problem; this remapping is currently broken in that it doesn't add to prompt history:

map global prompt <c-j> %{<a-semicolon>:execute-keys -with-hooks -with-maps <lt>ret><ret>}

luckily I don't need maps/hooks in prompt mode so I can use the simple remapping.

There is the argument that if we used NormalKey hooks everywhere,
then the simple remapping (map global normal <c-j> <ret>) would work, no need for execute-keys.
But of course it's surprising that normal mode key-aliases are mapped differently than for other modes.
I'm not yet sure what we should do.
Of course we should be less inconsistent in the end.


Either way we still want to remove the mapping when the filetype changes.
A naive

hook -once -always window WinSetOption filetype=.* %{ unmap buffer <ret> :jump<ret> }

will have problems; particularly if we switch from filetype=grep to another
filetype that adds this mapping via a filetype hook.
Since hooks run in definition order, the unmap hook will run last and remove
the wrong mapping.

We can solve that by making the maps unique (map buffer <ret> ':jump<ret> # grep').


I wonder if instead of mapping <ret> everywhere, we should define a buffer-scoped option (or alias).
That might make it easier to map keys like <c-j> to do the same as <ret> but it kinda goes against our conventions (<ret> is just that, there is no other name), so I'd not go there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants