search

emacs-evil / evil-collection

A set of keybindings for evil-mode
GNU General Public License v3.0
1.2k stars 275 forks source link

Binding (kbd "<tab>") in outline-minor-mode overrides org-cycle #53

Closed dieggsy closed 4 years ago

dieggsy commented 6 years ago

I'd argue org-cycle is way more useful when you're actually in org-mode.

Ambrevar commented 6 years ago

Strange, I never experienced it... Recipe to reproduce? I'll try harder on my end.

dieggsy commented 6 years ago

It may have something to do with the fact that i'm using org-mode from master? I'll try and come up with a minimal repro sometime soon.

dieggsy commented 6 years ago

Ok, try this:

curl -L https://git.io/vbR3d -o /tmp/minimal-repro.el && emacs --batch -q -l /tmp/minimal-repro.el

This will use this gist to:

Here's the output on my computer, with emacs 25.3 and default org:

Importing package-keyring.gpg...
Importing package-keyring.gpg...done
Contacting host: elpa.gnu.org:80
Contacting host: elpa.gnu.org:80
Contacting host: melpa.org:443
Package refresh done
Installing evil...
Cloning evil-collection...

EMACS VERSION: GNU Emacs 25.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.19)
 of 2017-09-16
ORG VERSION: 8.2.10
BUFFER NAME: test.org
BUFFER MODE: org-mode
M-x describe-key TAB RET:
<tab> runs the command outline-toggle-children, which is an
interactive compiled Lisp function in ‘outline.el’.

It is bound to <tab>, <motion-state> <tab>.

(outline-toggle-children)

Show or hide the current subtree depending on its current state.
Ambrevar commented 6 years ago

Wow, you've really put in a lot of effort to automate the repro! :)

On the one hand, I'd like to say you did not have to, a simple recipe would have been enough. On the other hand, it's a great script that's definitely going to prove handy in the future. Thanks for sharing!

(Please keep the gist online :p)

The problem is actually very simple: you don't use evil-org, do you? If not, then Org will inherit from the outline-mode-map keys, including <tab>.

I don't think there is anything we should change to Evil Collection:

Let me know your current setup.

dieggsy commented 6 years ago

I disagree with this solution. Even though I may use evil-org in the future, I don't now (I tried it and didn't like or ever really use it's bindings). I do still use evil-mode in org-mode however, with some minimal bindings of my own. Changing to Emacs state isn't an option - I don't get vim-like modal editing (and motion) that way. I think assuming if someone doesn't want a particular evil integration that hey'd rather not use evil at all in that mode is a mistaken train of thought.

That said, I could just rebind tab myself (it's one key), but I don't think that's a permanent solution, as anyone else who decides not to use evil-org but wants evil-collections will run into this unexpected behavior. Guess I'm saying that since org-mode inherits outline-mode-map keys, evil-collection should be a little more cautious about these.

re: the script - it's no problem at all, I had trouble coming up with a minimal recipe "by hand" so this was the easier route. I don't use package.el and so I was tripping up a bit too much on the right steps for an isolated minimal repro (that truly were repeatable). I realized I work better with elisp anyway.

dieggsy commented 6 years ago

To reword, it seems to me like the reasoning is "this binding is fine because this other package (which is optional), when installed, solves it" and I'm not sure how I feel about that.

Ambrevar commented 6 years ago

I totally agree with you: it's not a desirable behaviour. But what's your suggested solution then? I'm afraid we're being hit by the fundamental drawback of inheritance: not sure there is anything we can do about it.

It's the same story with image-mode and it's inherited modes, PDF-tools and doc-view.

Slightly off-topic: what don't you like about evil-org? (I co-authored evil-agenda :p)

Ambrevar commented 6 years ago

Well, one thing we could definitely do: document the issue! ;)

Ambrevar commented 6 years ago

To reword, it seems to me like the reasoning is "this binding is fine because this other package (which is optional), when installed, solves it" and I'm not sure how I feel about that.

My initial comment was poorly phrased: the real issue here is that of inheritance and there is not much we can do but using evil-org or org-evil.

dieggsy commented 6 years ago

Agreed about documenting. A note: interestingly, S-<tab> doesn't give me the same issue, but that might be because my emacs is registering shift-tab as <S-iso-lefttab>.

I think there are solutions, though maybe not very pretty ones. For example, one could use menu item bindings a la general-predicate-dispatch (The following is just a macro expansion of that):

(evil-define-key 'motion outline-mode-map
  (kbd "<tab>") '(menu-item "" nil :filter
                            (lambda
                              (&optional _)
                              (cond
                               ((derived-mode-p 'org-mode)
                                'org-cycle)
                               (t outline-toggle-children)))))

Which would do org-cycle in org-mode and fallback to outline-toggle-children.

(Side note: have you considered using general as a dependency/helper in this project? It has some nifty features - the above becomes simply:

(evil-define-key 'motion outline-mode-map
  (kbd "<tab>") (general-predicate-dispatch 'outline-toggle-children
                  (derived-mode-p 'org-mode) 'org-cycle))

)

With regard to evil-org, I can't remember all the details, except that it "wasn't for me" - maybe I liked to bind too many keys that evil-org overrode (I think I don't now), or just didn't find it that useful at the time. Maybe I should have said "I'm looking to try it again soon" - It was a while ago that I tried it and it seems like several additons/improvements have been made.

Ambrevar commented 6 years ago

While your workaround would work, I'm not too sure about implementing it... It's not very general: What if another mode inherits from outline-mode? What if other bindings have similar issues?

@jojojames Any take on this issue?

have you considered using general as a dependency/helper in this project?

See #48.

It was a while ago that I tried it and it seems like several additons/improvements have been made.

Went through the same route here and I'm happy with the latest versions. I highly recommend you give it another try. evil-org has a rather flexible configuration process.

dieggsy commented 6 years ago

@Ambrevar I think if another mode inherits from outline-mode, that wouldn't be an issue - it wouldn't pass the check for (derived-mode-p 'org-mode) and would fall back to outline-toggle-children. Or are you saying we'd have to add a check for every mode? Could you clarify your question on other bindings?

Ambrevar commented 6 years ago

Or are you saying we'd have to add a check for every mode?

Yes, the other derived mode would also have its <tab> key overridden.

Could you clarify your question on other bindings?

Not just <tab>, any other key that could potentially conflict :/

dieggsy commented 6 years ago

Well, it could be something like this instead:

(evil-define-key 'motion outline-mode-map
  (kbd "<tab>") (general-predicate-dispatch nil
                  (eq major-mode 'outline-mode) 'outline-toggle-children))

Which expands to

(evil-define-key 'motion outline-mode-map
  (kbd "<tab>") '(menu-item "" nil :filter
                            (lambda
                              (&optional _)
                              (cond
                               ((eq major-mode 'outline-mode)
                                'outline-toggle-children)
                               (t nil)))))

Course, if you wanted the binding to be inherited, this would prevent that. And yeah, I guess you'd have to do this for other conflicting bindings. While I agree it's not perfect, I'm just still not convinced that evil-collection's default should be what we go with for the stated reasons.

On the other hand, if documented well enough, maybe the solution is "If you don't want to use evil-org, you may want to disable evil-collection-outline.el" and be done with it. I guess evil-collection can't be expected to attend to every edge case.

jojojames commented 6 years ago

This does raise the question of 'is there a value-add general adds' that we could leverage to solve this problem. Curious on your thoughts @noctuid.

As for evil-org and not using it, I wonder if it'd be easier to have a 'simple-evil-org' version that will rebind TAB for you? I think every mode working reasonably even if other modes are disabled (evil-org) in this case is a worthy goal..

This kind of runs into the defcustom/init discussions we had before but I think for now, it'd be okay to have another defcustom for org with 'none/simple/evil-org etc that swaps out the one before evil-collection-init.

@Ambrevar It does run into the same init discussions, I think we can still leave it in its 'simple' state and just add defcustoms for edge cases. If we get 10 or more 'edge cases', we can probably start visualizing a proper pattern to it.

Ambrevar commented 6 years ago

Done: 28eb46bd4439630bcb3ef4d65c2bff7d6fc7ffe4.

The bindings are still on by default, but the behaviour is now documented.

I think we should expect Evil-Collection users to want some sort of Evil bindings for Org mode anyways, so the tab issue would be fixed on their end one way or another.

Let me know if that suits you.

noctuid commented 6 years ago

This does raise the question of 'is there a value-add general adds' that we could leverage to solve this problem. Curious on your thoughts @noctuid.

If the functionality of general-predicate-dispatch is ever needed/wanted in evil-collection, the expansion is extremely simple and could be used directly I think.

Edit: Alternatively it could be redefined in evil-collection. I don't think general.el in its current state is suitable for use as a library (i.e. being required by other packages for internal use for key definitions). I would consider splitting general.el to create a utility library for this purpose though. I'm not sure whether I want to encourage the behavior of packages requiring keybinding utility library though. Feel free to make an issue in the general.el repository about this.

jojojames commented 6 years ago

@noctuid I'd be curious about the utility library. I'll raise an issue in general at some point..

@Ambrevar I was thinking more along the lines of creating a 'simple org' evil version that rebinds TAB but your way seems ok too unless @dieggsy has another opinion.

dieggsy commented 6 years ago

I'm ok with this for now, though maybe because I haven't noticed other collisions?

Ambrevar commented 6 years ago

I think it's better to have no "dummy" org file as it sends a clear message to the user: "Evil org bindings are not here."

Adding evil-collection-org would probably raise complaints as to why the bindings are so limited.

Ambrevar commented 6 years ago

Closing now, feel free to re-open if you'd like to improve on anything.

jupart commented 5 years ago

New emacs user here: I don't really understand the issue completely, but I did remove evil-collection because it seemed to change "\<tab>" to 'outline-toggle-children when I wanted to keep 'org-cycle.

Ambrevar commented 5 years ago

Actually I have been having a similar issue for a while now, but with <backtab> bound to outline-show-all instead of org-shifttab in normal state. Note that insert state gets it right.

cvdub commented 5 years ago

Actually I have been having a similar issue for a while now, but with <backtab> bound to outline-show-all instead of org-shifttab in normal state. Note that insert state gets it right.

I had this same issue, even with evil-org installed. I believe it's because evil-org binds org-shifttab to <S-tab> instead of <backtab>. <S-tab> is bound to <backtab>, so the evil-collection binding to <backtab> takes precedence.

Setting evil-collection-outline-bind-tab-p to nil fixed this issue for me, but it would probably be better if evil-org updated their binding to <backtab>.

jojojames commented 5 years ago

@noctuid I'd be curious about the utility library. I'll raise an issue in general at some point..

I probably never did this but noticed https://github.com/noctuid/general.el/issues/144 . I'm be happy with either general or a NIH evil-collection version of the same functionality. (Tangential point).

Setting evil-collection-outline-bind-tab-p to nil fixed this issue for me, but it would probably be better if evil-org updated their binding to .

Can you PR @cvdub . @Somelauw is pretty responsive from what I've seen.

noctuid commented 5 years ago

Feel free to leave suggestions on that issue for what functionality you think would be useful to have. I still have to think more about what it would actually look like.

jojojames commented 4 years ago

@noctuid

I'm not super familiar with all the goodies general has for keybinding but I like the one @dieggsy mentioned.

(evil-define-key 'motion outline-mode-map
  (kbd "<tab>") (general-predicate-dispatch nil
                  (eq major-mode 'outline-mode) 'outline-toggle-children))

Which is way better than what it would expand to.

Adding evil-collection-org would probably raise complaints as to why the bindings are so limited.

After some time, I agree but I also think it's weird we don't have support for org. If we could absorb evil-org, that would be nice and fix the issue nicely, IMO.

There's probably nothing left for this topic but if @Somelauw (or someone else with an evil org type package) wanted to merge evil-org into evil-collection, I think that'd be a great idea too.

jojojames commented 4 years ago

@dieggsy Your curl command to do the repro is pretty cool. If there was a way to generalize that so issue reporters can use something similar, that'd be awesome.

dieggsy commented 4 years ago

@jojojames heh, i agree it's pretty cool :P I'd forgotten about it, tbh. It's flawed right now - it doesn't install the annalist dependency. How would you want to generalize it? Maybe wrap the whole thing in a shell script? What would you want to test for?

I suppose instead of opening an org file it could take command-line args , e.g. $1 would be one of evil-collection's supported modes, $2 would be the keybinding to test. So you'd use it like ./minimal-repro.sh org-mode TAB or something, and it would open a file in that mode and describe the binding. ....version checking would be little more code, i guess, for the modes that have a [mode]-version. We could support evil-collection versions (melpa default, git releases, git master) through another arg too, i suppose.

jojojames commented 4 years ago

I think for starters, the "how to set up emacs -Q with evil-collection + evil installed" is a good one.

From what I've seen, most reporters are hesitant to create one because it's time consuming.

After that, it'd be interesting from a "evil-collection+evil+whatever mode they're having problems with" approach to be even more automatic.

Your $1 and $2 seems interesting too, but I didn't look too closely at your black magic there, so don't have an opinion. @dieggsy