Fully replace evil-integration
noctuid opened this issue · 31 comments
From emacs-evil/evil#992:
I'd like to think of evil-collection as serving as an alternative evil-integration.el here, so the all or nothing approach should be fine.
Currently, evil-collection is mainly focused on keybindings and is not an alternative to evil-integration. I'm not suggesting doing any opinionated configuration by default, only adding "fixes" that it's unlikely anyone would object to (e.g. advising show-paren-function to make show-paren-mode work correctly in normal state, adding evil command properties such as :repeat, :type, :keep-visual, and :suppress-operator, etc.). Anything considered potentially intrusive could be optional. Unfortunately, since evil-integration is all or nothing and will stay that way, its useful, non-intrusive functionality needs to be replicated in another package for users who want to sanely only use those parts of evil-integration. Maybe evil-collection is not the suitable place to do this, but I'm interested to hear your thoughts.
Yup, as mentioned in the same thread.
Once that is in, I'll backport relevant changes to evil-collection instead so most of the integration can live in one place.
I'm fully ok with copy-pasting from evil-integration and disabling what doesn't make sense and was planning to once a version of evil with that change is out in the wild.
Planning on attempting this in the next week (or few weeks).
I think a solid peacemeal plan would be to:
- Copy over all/most of evil-integration and disable evil-integration in evil. (Achieve parity)
- Remove all overriding maps in evil-integration-copy that are already handled here or at least gate it with a defcustom.
- Refactor the integration into their separate packages (if there is a separate package for it). (Clean up)
- If there's no suitable (doubt it) place for a binding in evil-integration-copy, we can wrap it with a defcustom to conditionally enable/disable it.
@Ambrevar @noctuid Sound good?
If anyone else has the time or energy, feel free to kick start the process too.
Looks good to me.
Next few weeks will be busy ones on my end :/
I wonder how we want to handle something like this in evil-integration.
(eval-after-load 'company
'(progn
(mapc #'evil-declare-change-repeat
'(company-complete-mouse
company-complete-number
company-complete-selection
company-complete-common))
(mapc #'evil-declare-ignore-repeat
'(company-abort
company-select-next
company-select-previous
company-select-next-or-abort
company-select-previous-or-abort
company-select-mouse
company-show-doc-buffer
company-show-location
company-search-candidates
company-filter-candidates))))
We can put it in the company file but it'd be reasonable for a user to disable the company integration (assuming they have their own setting). But doing that would mean they'd disable this nice integration too.
The want here is that these integrations should go with their integration file but I could see a user wanting this 'integration' but not wanting the bindings.
The changes in evil-integration just around bindings are easier because they're just bindings.
It might be better to leave this type of change in evil-collection-integration but that seems unsatisfying.
Found this on reddit: https://www.reddit.com/r/emacs/comments/7obhve/evil_and_avy/
Any idea on how we want to handle this? This might be one of those needs that would suggest tweaking the init system for these packages to handle the case where a user wants 'integration' but no bindings.
We could always just go for the absolutely simple route and consider integration to be the same as bindings and let the user recreate whatever setup is needed.
Relevant PR to evil.
This might be one of those needs that would suggest tweaking the init system for these packages to handle the case where a user wants 'integration' but no bindings.
I think it would be best to give the user a way to opt out of getting keybindings. For example, another argument could be added to evil-collection-init that would disable binding keys (and this could be passed to each setup function). The user could then run evil-collection-init for modes individually or twice (once for the modes they want keybindings and once for other modes). Alternatively, there could be another list added of modes to make keybindings in that is checked in each setup function. I personally like the first way since each setup function takes an argument which is more direct (and simple I think).
Edit: evil-collection doesn't consistently do any intrusive/opinionated configuration/keybindings, so I think it would be fine to configure that with per-file settings. For example, I think it might be better if the minibuffer was included in evil-collection-mode-list, and there was a setting for whether it should make the minibuffer modal (off by default). By default, it could bind escape directly in the relevant minibuffer keymaps instead of in normal state.
Yeah, I've been noodling about how to do this. I think that part is what's blocking the rest of evil-collection-integration being dispersed into separate modules.
@noctuid If you had a good idea on how to handle this, I think a PR would be awesome.
I'd think we'd want to be able to allow the user to disable either the integration and/or the keybindings. It may or may not look similar to what @Ambrevar was playing around with a while back on tweaking the init list.
I can potentially make a pull request at some point if I have time, but I'd rather everyone agree on how to do it before doing any work. To elaborate on my previous suggestion, each setup function would now takes two optional arguments. For example: (cl-defun evil-collection-ag-setup (&optional (bind t) (integrate t))). That or the setup functions could be split (one for integration and one for keybindings).
As for evil-collection-init, either it could have extra arguments like the other setup functions, or evil-collection-mode-list could be extended, maybe with keyword arguments (e.g. (list ag avy (bookmark :bind nil) (mode :integrate nil))). The latter might be more convenient if the user wanted keybindings for most modes. On the other hand, if they just wanted integration for all modes and keybindings for only a few, it would require a lot of :bind nils. Maybe both methods could be supported.
was playing around with a while back on tweaking the init list.
Can you point me to that?
Regardless of the method used, work could potentially be done just to split integration from keybindings in each file (even if it still always runs both for now). If not for old files, it may be a good idea to do this for any new files to make things less work later on.
I haven't looked through everything, but we may want to discuss any special cases beforehand. There are some cases where keybindings should be considered integration (e.g. for the avy remap commands and for my suggested change to the minibuffer keybindings). I think integration should be only for any "fixes" that pretty much every user will want, but we can still have it be optionally disabled.
That or the setup functions could be split (one for integration and one for keybindings).
Don't have a strong opinion either way. @Ambrevar
On the other hand, if they just wanted integration for all modes and keybindings for only a few, it would require a lot of :bind nils. Maybe both methods could be supported.
I'm erring on the side of someone that wants this package would probably use it mostly for the keybindings so maybe we can assume the case where the user wants 0 bindings is an edge-case.
Open to suggestions here if we can have our cake and eat it too.
Can you point me to that?
It was on a separate branch. I forget what the goal there was.
https://github.com/emacs-evil/evil-collection/tree/init
There are some cases where keybindings should be considered integration (e.g. for the avy remap commands and for my suggested change to the minibuffer keybindings). I think integration should be only for any "fixes" that pretty much every user will want, but we can still have it be optionally disabled.
I don't have a strong opinion where we draw the line.
so maybe we can assume the case where the user wants 0 bindings is an edge-case.
I think anyone who would normally want to just use evil-integration but doesn't because of its extra keybindings/use of override maps may use this package mainly for the integration. I think we can probably cater to this use case without making things too much more complicated.
On the other hand, if they just wanted integration for all modes and keybindings for only a > few, it would require a lot of :bind nils.
It's Lisp after all, users are free to use loops to simplify the setup. I don't think this would be a problem at all.
Fair point. Use keywords in evil-collection-mode-list then?
Not completely sure about what you have in mind, but if I understood correctly, then I suggest that the default behaviour when no key argument is specified is to include both the bindings and the integration. In short:
- bind nil & integration nil: include both.
- bind nil & integration t: include integration.
- bind t & integration nil: include bindings.
- bind t & integration t: include both.
Hey guys, what's the impacts of what you are planning for users that don't use evil-collection ? I hope you think about users that don't use it, I fear more and more that you'll end up imposing it to all evil user :-(
Seriously guys, evil-collection is a set of key-bindings, not a rewrite of evil that forces to use evil-collection! I really fear what you are doing here...
Is that possible to improve evil-integration.el and keep it decouple from evil-collection and keep evil-collection as an opinionated optional set of key-bindings ?
Thank you!
evil-integration.el has intrusive keybinding configuration that conflicts with how evil-collection does things (e.g. overriding maps). The evil maintainers did not want to split evil-integration.el, so there is no choice but to either attempt to undo some of it or to replace all of it. I don't really understand your concerns and don't see why evil-collection should be limited to just keybindings, especially if the behavior is configurable.
Spacemacs does not use evil-collection so I beg you to not introduce a breaking change in evil that requires evil-collection to be installed to be fixed.
By breaking change I mean loss of features like avy integration, key-bindings etc...
My apologies it seems that I made a mistake about what's going on here. Seeing breaking evil-ediff recently and conversations about evil-magit started to make me feel nervous. :-)
So I agree with you about the split and it's unfortunate that you have to duplicate the integration to support them in evil-collection although you just wanted to make your own key bindings.
@wasamasa @TheBB I know that we don't want to touch evil too much but I guess we should improve evil-integration.el to the point where evil-collection can easily set its own key-bindings. If we need to split evil-integration.el into evil-additional-key-bindings.el and evil-integration.el let's do this. But for this we first need your approval about it.
Ideally we should make what it takes to keep evil-collection to be a set of key-bindings. Integration closer to the core by supporting motions etc...should remain in core as it does not involved new key bindings per se. For backward compatibility issue we must retain the current additional key bindings in evil core though with appropriate comments.
Sorry @syl20bnr but I'm pretty much out of the project. I've never expected to contribute much to it (see https://www.reddit.com/r/emacs/comments/5g9d53/evil_needs_a_new_maintainer/daqij57/ for my announcement), ended up being the one doing most of the work and now I don't have any time left for it at all. You've heard my opinion on this, I consider a small bit of code duplication far better than bikeshedding over how to make code reusable/modular. However, it's ultimately @TheBB's decision.
Seriously guys, evil-collection is a set of key-bindings, not a rewrite of evil that forces to use evil-collection!
evil-collection started (partially) as a replacement for evil-integration.el. This, includes keybindings as well as integrations.
Ideally we should make what it takes to keep evil-collection to be a set of key-bindings.
See above.
Integration closer to the core by supporting motions etc...should remain in core as it does not involved new key bindings per se.
I agree somewhat. If there can be an improvement to evil, it should go there. For any keybinding/integration change, I 'd prefer it to be in evil-collection.
Again, evil-collection isn 't just a set of key bindings, it was meant (at least from me, not sure about @Ambrevar) to be a replacement for evil-integration.
For backward compatibility issue we must retain the current additional key bindings in evil core though with appropriate comments.
I agree but evil-collection as this point is still expected to be more bleeding-edge.
I know that we don't want to touch evil too much but I guess we should improve evil-integration.el to the point where evil-collection can easily set its own key-bindings. If we need to split evil-integration.el into evil-additional-key-bindings.el and evil-integration.el let's do this. But for this we first need your approval about it.
For what it 's worth, I do think this is a good idea but will settle for the one flag that needs to be set to nil.
Thank you for the clear answers.
Not sure we agree on the meaning of integration here. In contrast to key bindings which are opinionated by nature, integrations are not a subjective matter so it would be not efficient for us to duplicate the effort on them. For instance, using avi motions is straightforward and there is only one possible result (I would say POLA) for the end user. Also all the declare should obviously be core.
Clearly evil-integration.el is a mess as its scope is all over the place. We should really start from there before moving on.
By breaking change I mean loss of features like avy integration, key-bindings etc...
There are no plans to remove anything from evil-integration.el or to try to replace it completely with evil-collection. I agree with you that evil-integration.el should not be removed from evil.
In contrast to key bindings which are opinionated by nature, integrations are not a subjective matter so it would be not efficient for us to duplicate the effort on them.
I think that we are mostly on the same page. Most everything in evil-integration.el that doesn't have to do with keybindings (except for command remappings like for avy) or altering the initial state is non-controversial. There is some configuration that not all users would necessarily want (e.g. enabling undo tree mode and the subsequent command remappings). The only other configuration that could possibly be considered controversial is already enabled conditionally (evil-respect-visual-line-mode and evil-want-abbrev-expand-on-insert-exit).
I agree that it would be preferable to have all non-controversial "integration" done in evil. Anything more could be left for other packages like evil-collection.
Also, I really don't think that there is much room for bikeshedding over how evil-integration.el could be split. I think everyone who has suggested splitting it in the past could probably reach an agreement on how to split it. @TheBB Would you reconsider splitting evil-integration.el if this was this case?
@wasamasa Even with the "potentially controversial" configuration I mentioned left in, removing the keybinding configuration alone (which everyone who has wanted to split the file has already agreed on) would make evil-integration.el easily usable by evil-collection without any bikeshedding/discussion necessary. I also think any discussion could be had once only. While I agree that the code duplication is not a huge deal, I wouldn't be surprised if in the future someone made a PR to evil-collection adding some basic non-controversial integration improvement and did not also make a PR to evil, which is what I think that @syl20bnr is worried about. Maybe this is also not a huge deal, but I think it's a problem that's not difficult to prevent.
Not sure we agree on the meaning of integration here.
In contrast to key bindings which are opinionated by nature, integrations are not a subjective matter so it would be not efficient for us to duplicate the effort on them.
For instance, using avi motions is straightforward and there is only one possible result (I would say POLA) for the end user.
Also all the declare should obviously be core.
My intent was just to provide context, as in evil-collection was meant to be an alternative to the entire evil-integration.el file.
I digress, my point is that evil-collection can house both keybindings and integration changes.
For example, I don 't think term, minibuffer, company, eshell, helm, wdired type changes in evil-collection would make it into evil (or is even appropriate there).
Clearly evil-integration.el is a mess as its scope is all over the place. We should really start from there before moving on.
Agreed.
There is some configuration that not all users would necessarily want (e.g. enabling undo tree mode and the subsequent command remappings). The only other configuration that could possibly be considered controversial is already enabled conditionally (evil-respect-visual-line-mode and evil-want-abbrev-expand-on-insert-exit).
Agreed, if @TheBB is ok with splitting, we can start looking at what the list looks like.
would make evil-integration.el easily usable by evil-collection without any bikeshedding/discussion necessary.
I think this would be good. I also don 't like the thought of having to port over changes to evil-integration.el to evil-collection. As of right now, it's a necessary evil.
I wouldn't be surprised if in the future someone made a PR to evil-collection adding some basic non-controversial integration improvement and did not also make a PR to evil, which is what I think that @syl20bnr is worried about. Maybe this is also not a huge deal, but I think it's a problem that's not difficult to prevent.
I think that 's a fair scenario. Right now, the reverse is happening (evil -> evil-collection). Of course, if we can avoid it, lets go for it.
This is in now (we're back on leveraging evil's evil-integration)!
Hopefully users will be able reset their configuration without a hitch.
Do you want to have a dedicated issue open for selectively enabling keybindings vs. Integration?
Yeah, I'll look into adding it soon.
Since I didn't see a solution on here and the error is still showing up and referring to this github issue I wanted to share my solution.
Here's what I did to solve it:
Add this before loading evil-leader:
(setq evil-want-keybinding nil)
Then add it again before loading evil. The warning disappeared after this.