dajva / rg.el

Emacs search tool based on ripgrep
https://rgel.readthedocs.io
GNU General Public License v3.0
484 stars 41 forks source link

rg-mode shouldn't bind C-n and C-p #72

Closed hmelman closed 4 years ago

hmelman commented 4 years ago

I think rg-mode should stop binding C-n and C-p in the *rg* buffer.

The keymap is rg-mode-map which inherits from grep-mode-map which inherits from compilation-minor-mode-map which all seems reasonable.

compilation-minor-mode-map being a minor mode doesn't bind the most common keys. So it uses M-n and M-p for next and previous error and M-} and M-{ for next and previous file.

grep-mode-map being a map for major mode binds easier keys for these things. n and p are next and previous error and } and { are next and previous file and of course the parent bindings still work.

rg-mode-map has similar needs, needing motion by error (aka match) and by file, as well as forward and back in search history which are bound to C-c> and C-c< which is fine. It uses the inherited bindings for n and p for next and previous error which seems reasonable. Those are the most obvious keys for motion and moving by error is the most obvious motion.

But it also binds C-n and C-p to file motion commands rg-next-file and rg-previous-file. I tend to just use these keys for motion since they work everwhere else in emacs and having them skip over lines in *rg* buffers is jarring. Also unlike compilation-next-file and compilation-previous-file they don't move you to the first or last match in a file, but to the filename line itself (if rg-group-result is non-nil which is its default value).

As near as I can tell, when results are grouped, when point is on a file name line there don't seem to be useful key bindings. RET doesn't take you to the file, o doesn't display it in another window, etc. So I don't think the next and previous file commands should put you on those lines.

Also they don't seem to play well with the next and previous error commands. If I use n to move to say the 4th error of the 5th file and then use C-p to move up to the 2nd file; if I then hit n I move not to the first error of the 2nd file, but to the 5th error of the 5th file, as if I never hit C-p at all. Navigating with compilation-next-file and compilation-previous-file seem to have the same issue (I notice they don't move a bullet in the left gutter), even if results aren't grouped by files, but since they always leave me on an error, I can hit RET to move to that error and then n and p work. Using rg-next-file I end up on a file and then can't move to it's first error because C-n is bound to rg-next-file.

So I don't understand what rg-next-file and rg-previous-file are buying you over compilation-next-file and compilation-previous-file and regardless I don't think they should be bound to C-n and C-p.

dajva commented 4 years ago

The n, p, M-n and M-p are all from upstream as you say so the behavior is defined there. The C-p and C-n is consistent with those in the sense that n and p has it's own positional state while the other keys works from point.

rg-next-file and rg-previous-file are really move to next "header" for the grouped results mode so very useful there IMO. The upstream function moves to the closest match which doesn't always (previous navigation) give you the file information immediately. Of course, if you don't find this useful you don't have to use it. ;)

I don't think I will change the bindings. It's a bit of an unfortunate clash but It's commonly used keys in this mode and I don't want to break such use cases. Could possibly add an option for it for users that find it hard to rebind keys.

hmelman commented 4 years ago

rg-next-file and rg-previous-file are really move to next "header" for the grouped results mode so very useful there IMO.

Could you be more specific of their utility? I tried to be specific because I found them unusable in grouped mode. They moved me to the "header" line but then I couldn't do anything there because I didn't find bindings that did anything when point was on a header line, and I couldn't move off a header line because C-n and C-p were changed and behaved as if I hadn't moved to the header line in the first place. This is what I tried to describe.

I can see their utility in ungrouped mode, but I think in this case they just behave the same as the "upstream" { and } do. If you use group mode with evil bindings I think rg-next-file and rg-previous-file would still be useful because you can use j and k to move to an error, but AFAICT the non-evil user can't.

dajva commented 4 years ago

Could you be more specific of their utility?

I don't think I can actually. As I wrote, the benefit is moving between files to immediately identify the file (name) with matches. There isn't really much more to it I am afraid but very useful I think. Compare that to moving backwards with { where the file name may not be visible.

I couldn't move off a header line

M-n and M-p should work there as usual. If they are broken, there is a bug.

hmelman commented 4 years ago

M-n and M-p do work. I see how they're different from n and p in that the meta versions don't move the locus (to use compilation-mode's term) and the single keys do. Still the control versions would be my obvious choice for this, but the meta versions are emacs standard bindings.

I'll be doing the following in my config:

(define-key rg-mode-map (kbd "C-n") nil)
(define-key rg-mode-map (kbd "C-p") nil)
(define-key rg-mode-map (kbd "M-e") 'rg-next-file)
(define-key rg-mode-map (kbd "M-a") 'rg-prev-file)

I see the utility of getting to the filename if it's not visible (though maybe it could be displayed in the modeline), but it feels closer to moving by paragraphs so I'm trying M-a and M-e for them. They're otherwise pretty useless bindings in a compilation buffer.

nverno commented 4 years ago

FWIW, I also bind those (C-n, ...) to nil in rg-mode-map, and tend to agree with @hmelman analysis

garyo commented 4 years ago

I just found this ticket after trying to unbind C-n and C-p in rg-mode. I also find that behavior unusual and a bit jarring, so I agree with @mhelman that there should at least be a config to turn off those bindings.