search

emacs-evil / evil-magit

Black magic or evil keys for magit
https://github.com/justbur/evil-magit
GNU General Public License v3.0
273 stars 16 forks source link

with-editor bindings #18

Closed wbolster closed 8 years ago

wbolster commented 8 years ago

i've filed a request for better evil mode bindings in with-editor buffers here: magit/magit#2561

perhaps this makes better sense in this project instead? feedback welcome. :)

justbur commented 8 years ago

I like the idea and it's appropriate here, but since with-editor may be used outside of magit I think it should be an option that is disabled by default. I'd accept a PR for this

wbolster commented 8 years ago

actually i think it should not be an option, and it should be enabled by default. the with-editor package itself tries really hard to "do what i mean" by overriding any key bindings that close the buffer, including kill-buffer itself. evil-magit should act in exactly the same spirit and also just "do what i mean". the current behaviour is extremely error-prone, e.g. pressing ZZ just loses the buffer and leaves a git process lingering in the background. the default should not allow users to shoot themselves in the foot by default. ;)

justbur commented 8 years ago

I agree with the spirit of what you are saying, but consider the person who uses with-editor but does not have magit. If they load evil-magit, the behavior of their existing non-magit config would change, which is surprising to me. Making it an option makes them explicitly acknowledge that they want this behavior outside of magit as well.

I can see your argument though. If you want to make a pr, I'll definitely do something about this.

On Mon, Feb 22, 2016 at 11:25 AM Wouter Bolsterlee notifications@github.com wrote:

actually i think it should not be an option, and it should be enabled by default. the with-editor package itself tries really hard to "do what i mean" by overriding any key bindings that close the buffer, including kill-buffer itself. evil-magit should act in exactly the same spirit and also just "do what i mean". the current behaviour is extremely error-prone, e.g. pressing ZZ just loses the buffer and leaves a git process lingering in the background. the default should not allow users to shoot themselves in the foot by default. ;)

— Reply to this email directly or view it on GitHub https://github.com/justbur/evil-magit/issues/18#issuecomment-187253074.

justbur commented 8 years ago

In my opinion, with-editor should treat any method of saving and closing the buffer as being finished with the buffer instead of requiring C-c C-c. That would fix the issue you described here in a non-evil dependent way. I don't know if there are issues with doing it that way, but it would certainly be more consistent with the command line.

wbolster commented 8 years ago

i really think there is no problem at all to do the right thing by default, since what you describe seems exactly what with-editor tries to achieve:

https://github.com/magit/with-editor/blob/580f225a6c4476feb36b707c6c705b027339717b/with-editor.el#L322-L351

i argue that the use case you described, namely with-editor without magit, while using evil is an uncommon case, and should not be used as a baseline at all. note that with-editor was only recently split off from magit itself, so use in the wild will be limited. and even in that case, i would still expect evil to integrate in the "right way", i.e. in the same spirit as with-editor itself.

practical example: do you think that someone using evil and with-editor intends to lose data and leave a stray background process when they press ZZ in a with-editor buffer? of course not, since that behaviour is a non-existent use case.

fwiw, a separate evil-with-editor (on which evil-magit would depend) would perhaps address your concerns from a technical perspective, but i argue that such complexity is overkill for simply adding "do the right thing" behaviour.

justbur commented 8 years ago

fwiw, a separate evil-with-editor (on which evil-magit would depend) would perhaps address your concerns from a technical perspective, but i argue that such complexity is overkill for simply adding "do the right thing" behaviour.

That would be the strictly correct thing to do I think, but I'm convinced by your argument that it should be the default and I think it would make a lot of people happy who are accustomed to ZZ or :wq.

Do you want to make a PR?

Also, I never use them but what are your thoughts about :wqall and :qall?

wbolster commented 8 years ago

i personally never use :wqall and :qall. not sure whether it's possible to override ex commands only for a specific minor mode. there is no (evil-define-key 'STATE some-minor-mode-map ...) equivalent i presume.

i can have a stab at a pr later on; for now my ZZ and ZQ remapping mentioned in magit/magit#2561 solve the immediate pain of me losing data (commit message... GONE!). please bear with me, i am only a ~3 week old emacs convert and not really familiar with elisp and emacs internals yet. :)

wbolster commented 8 years ago

btw, same question applies to overriding :wq only when with-editor-mode is active...

on second thought, disabling :wqall and :qall makes sense to me. i mean, Write all changed buffers and exit Vim (from :help wqall in vim) while operating on an external process (well, a with-editor "external process), does not make a lot of sense to me.

justbur commented 8 years ago

I'd be happy to do this. I just thought I'd offer. Let me know

wbolster commented 8 years ago

go for it! :)

anyway i had some untested ideas on magit/magit#2561, maybe you can use those as a starting point.

the remapping of ZZ and ZQ feels 'hackier' than remapping any key combo ([remap ...]) calling a certain function, as with-editor apparently does.

about the ex commands (:wq), i have no idea.

justbur commented 8 years ago

ok, I used the remaps like you had. ZZ and ZQ should work now, but the evil ex stuff is going to be more complicated. I'll put that on my todo list.

wbolster commented 8 years ago

awesome, thanks.

only three weeks with emacs (and evil), and my first contribution has been integrated into a tool that forms part of my core workflow. yay.

justbur commented 8 years ago

:+1:

tarsius commented 8 years ago

In https://github.com/magit/magit/issues/2561 @kyleam only said that that nothing can be done in Magit. As I understand him, he didn't say that adding some remappings to with-editor.el is out of question.

So if you Evil folks come to the conclusion that it would be best to just remap two Evil commands directly in with-editor.el, then I think we would be fine merging such a pull request.

justbur commented 8 years ago

@tarsius Thanks. I made a PR

justbur commented 8 years ago

@wbolster Good news. I made the necessary changes in evil and they got merged, so soon ZZ, ZQ, :wq and :q will be supported for evil users even without evil-magit.

wbolster commented 8 years ago

great. i presume you meant "in with-editor"?

justbur commented 8 years ago

Right :)

yorickvP commented 6 years ago

@justbur it seems like :wq and :q don't work. ZZ and ZQ do. Did something evil change?

justbur commented 6 years ago

@yorickvP No and they work for me. This functionality is not dependent on evil-magit, so you should try figure out whether it's a with-editor problem, an evil problem, or something with your configuration. You may open an issue with the corresponding package if you find something.