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-map should inherit from grep-mode-map #59

Closed hmelman closed 5 years ago

hmelman commented 5 years ago

rg-mode-map shouldn't just copy grep-mode-map but should inherit it via set-keymap-parent so that my customations to grep-mode-map apply to it too.

Also, while it's unresolved, see this emacs bug report for possible unified keybindings between wdired, occur, and grep modes.

dajva commented 5 years ago

Thanks for the report. The goal hasn't really been to unify the bindings with grep-mode, but rather to have a good familiar starting point. Especially since I essentially started with forking grep-mode which then became a more standalone package.

With that said, your suggestion sounds like a good idea. And also reminds me that I should pay more attention not to override grep bindings if not needed.

It might of course be that rg.el override some of your grep-mode modifications but those would not be available with the current setup anyway so would just be a minor annoyance when the expected bindings doesn't work I guess.

hmelman commented 5 years ago

Yeah I've recently been annoyed about different ways of entering wdired, occur-edit and wgrep and found that emacs bug report. I wish that old bug was resolved but it's not. I hadn't realized the different ways of going to the file of a line (f in the same window and o in another window). I setup grep-mode-map to bind o and C-o and have liked that (which unifies those modes). Then I was surprised to find rg didn't get those, hence this bug.

I included the emacs bug in part because w is unusual (though I agree logical) to enter wgrep particularly since e is also unbound in rg-mode-map.

On an only slightly related note, today I was bit by rg-mode-map rebinding C-b and C-f for history movement (which is great) but I'm not sure yet what binding I'd prefer. I had wanted to move the cursor between two quotes in a result to see if there was a space or if they were an empty string and had to resort to arrow keys.

dajva commented 5 years ago

Good suggestions. I am personally an evil user so loosing a bit of touch of the standard emacs bindings. Overriding C-b and C-f actually seems like a bad idea. :)

I'll see what I can do to improve this.

dajva commented 5 years ago

After consideration, I will not go all in and align with the suggestions in the emacs bug. Simply, since it has not been implemented and no point in following something that may change. What I have decided to do is to add e as a binding for wgrep to align with occur (IIRC). w will still functioning with a warning for the time being. I also plan to move History navigation to C-c b and C-c f to avoid clashing with elementary movements keys. Those bindings are particularily hard since there are some other clashing bindings in the neigbourhood. Please comment if you find any of this particularily bad.

hmelman commented 5 years ago

I agree with not implementing everything in that bug, things are too inconsistent. e sounds fine. I might add a customization for C-x C-q.

C-c LETTER is supposed to be reserved for users, not packages, see "Major Mode Conventions" in the elisp manual. C-c C-b/f would be better but at least I have bound (I'm not sure from what source) thing that conflict with both the C-c C-f/b and C-c C-n/p possibilities.

C-c C-f     next-error-follow-minor-mode
C-c C-p     wgrep-change-to-wgrep-mode

M-n/M-p would be good choices (mapping to history in mode line) but that would overwrite compilation-minor-mode-map setting them to next/previous error, which is probably ok since I think C-n/C-p already do the equivalent in rg.

dajva commented 5 years ago

Hmm, this is non trivial stuff apparently.

M-n/M-p differs from C-n/C-p since the first is moving between matches and the latter is moving between files. So we are really running out of the b/f and n/p possibilities here.

So not really any good alternatives here. I will let this sink in a bit. Might be that I move this to C-c > and C-c <, although I don't really like the SHIFT modifier on american querty.

hmelman commented 5 years ago

Another option is to leave them unbound with a comment/suggestion to bind them to C-c b/f if you would find them useful. I've seen other packages do that. I don't love it, as you don't find out about the feature unless you read the comment (you can put it in the readme too), but it gets around the restriction of modes note defining C-c LETTER.

muffinmad commented 5 years ago

Maybe it's ok to sacrifice next-error-follow-minor-mode. I never enable this mode in rg results buffer. But I often use C-c C-b and C-c C-f in help buffer to to go back/forward. For me it would be perfect default bindings for moving through search history.

hmelman commented 5 years ago

Maybe. But for what it's worth, I do use next-error-follow-minor-mode and only used rg's history movement once. That's only one factor in what the "defaults" should be. Everyone uses different features and emacs is customizable for this very reason. Other factors are principle of least surprise, consistency, discoverability, etc. And still, choosing key bindings seems to be one of the most difficult thing in emacs :)

dajva commented 5 years ago

I thought about sacrifice that too but them I realised that this is one binding that is actually consistent between modes (diff, grep, compile, occur). Similar binding in help mode is a plus though.

muffinmad commented 5 years ago

Might be that I move this to C-c > and C-c <, although I don't really like the SHIFT modifier on american querty.

Maybe C-c C-, and C-c C-.? No need in SHIFT.

dajva commented 5 years ago

Maybe C-c C-, and C-c C-.? No need in SHIFT.

Yeah, unfortunately those bindings are reserved for minor modes. I am not sure how strict I would be here. My current plan is to follow the emacs mode convention pretty strictly. It's always possible to override and I will soon add a transient menu interface that would provide better alternative bindings IMO.

hmelman commented 5 years ago

What about the arrow keys with some modifier? Like M-right, M-left? I don't expect to use them frequently so I don't think I'd mind moving my hands from the home keys (aka main part of the keyboard). And again, people could bind C-f/b if they wanted to.

dajva commented 5 years ago

Moving this to 2.0.0 milestone which will contain breaking changes. I already commited the inheritance part which will be in 1.8.0

dajva commented 5 years ago

I went with the C-c </C-c > in the end. Also providing a rg-use-old-defaults defun to restore original settings.

Thanks a lot for all the input guys.