in evil-mode, the evil-repeat is not compatible with company-complete #15

Closed
redguardtoo opened this Issue Apr 24, 2013 · 12 comments

Projects

None yet

2 participants

@redguardtoo
Contributor

input "hell", say there is a candidate "helloworld" pop up. So I press tab key to complete the word.

press esc to come back to evil-normal-mode.

Run command "M-x evil-repeat" (hot key "."), I expect the "helloworld" is input but actually only "hell" is input.

hippie-expand does not have this problem.

I use evil-repeat a lot

@dgutov
Member
dgutov commented Apr 24, 2013

Apparently, you need to call evil-declare-change-repeat on every applicable company-mode command (company-complete-common in this case). Evil does that for hippie-expand at the top of evil-integration.el.

Not sure if third-party packages are supposed to do that themselves, or leave that to the users.

@redguardtoo
Contributor

Thanks. problem solved.

It's up to you how to handle the relationship with third party packages. It's a business issue instead of technical issue. I've no opinion.

@dgutov
Member
dgutov commented Apr 24, 2013

I think what matters most here is the opinion of evil-mode maintainer(s). As a user, you might want to find it out.

@redguardtoo
Contributor

see redguardtoo/emacs.d@f5d167f , I need customize more functions. maybe you can add this tip to wiki or somewhere.

@dgutov
Member
dgutov commented Apr 24, 2013

maybe you can add this tip to wiki or somewhere, at least.

Feel free to put that somewhere on the EmacsWiki page yourself, it's publicly editable. I'll proof-read the change later.

@ghost
ghost commented Aug 11, 2013

The opinion of the evil maintainers (I'm one of them) is that it's perfectly ok to have some integration code for widely used packages within evil (that's more or less the whole point of evil-integration.el), e.g. there's some code for integration with auto-complete. However, nobody asked us about integration with company or submitted a patch, that's why there no such code there in Evil, yet.

But the problem with third-party packages is that there are so many of them, so it will be virtually impossible to have integration code for all of them. In the end, we rely on users providing patches (or maybe some additional package) that do the integration (of course we would be happy to help with the code, but quite often people ask about packages that we do not use ourselves). Maybe, if most Emacs users will be converted to Evil some day, it will be so standard that the maintainers of other packages are responsible for integration, but I doubt that this will ever happen ;)

@dgutov
Member
dgutov commented Aug 11, 2013

Ah, so this should've been reported to Evil after all.

@lyro Would you add @redguardtoo's snippet to the integration code? Or is there something seemingly wrong with it?

@ghost
ghost commented Aug 12, 2013

Ah, so this should've been reported to Evil after all.

Yes and no. If something goes wrong then some knowledge about both, evil and the other packages is usually needed. But of course, at least someone should tell us about the problem ;)

Regarding the snippet: I don't think that this works (it does not in my tests). The snippet changes company functions to be recorded by buffer change (more or less by using after-change-functions). However, the popup probably uses overlays or so, and those (may) count as buffer changes and it is also possible to move point around during completion. So if one types some characters until the popup occurs, then uses the cursor keys to select some different completion, this whole operation (along with possible buffer changes and motions of point) will trigger evil's recording and easily cause trouble. A succeeding repeat of this insertion (i.e. using the "dot" command) will typically insert some text at some random position.

However, evil can be configured to record a command to be repeated in a very specific way. In evil-integration.el there's an example how to integrate auto-complete, and I suspect that something similar should work for company. The idea is as follows: ac-next and ac-previous (the equivalent to company-select-next and company-select-previous) are configured to be ignored completely by evil, i.e. selecting a different completion candidate triggers exactly nothing in evil. The functions ac-complete and ac-expand are configured to use a special repeat recording function evil-ac-repeat. This functions is called once before the command is executed and once after it is completed (analogous to pre-/post-command-hook). In the 'pre case the original buffer content (that part that is about being completed) is saved. In the 'post case two record informations are stored, one that removes the original text to be completed, the second to insert the full completion. I don't know what the corresponding variables/functions are in company, but I suppose it should not be too difficult to adapt this function.

@dgutov
Member
dgutov commented Aug 12, 2013

I'm not quite sure how to test this, but shouldn't this work, then?

(evil-declare-change-repeat 'company-complete-common)
(evil-declare-change-repeat 'company-complete-selection)
(evil-add-command-properties 'company-select-next :repeat 'ignore)
(evil-add-command-properties 'company-select-previous :repeat 'ignore)

Unlike auto-complete, company doesn't do anything out of the ordinary with buffer-undo-list. It's not referenced anywhere in the code.

Calling undo-tree-undo undoes the completion, calling undo-tree-undo re-applies it. Plain undo also seems to work as expected. So I'm not sure why company-complete-common and company-complete-selection would need to be treated differently from hippie-expand.

and those (may) count as buffer changes

Unlike text properties, overlays and changes in them explicitly don't count as buffer changes.

@ghost
ghost commented Aug 12, 2013

I think the problem is not undo but repeat, i.e. the dot command. And this has nothing to do with undo or undo-tree (also the evil-declare-change-... functions do not influence undo but only the repeat system). The problem are commands that are executed during the completion process and that are recorded although they should be ignored. It seems as if <up> and <down> are bound to company-select-next-or-abort and -previous-or-abort. Because these commands are not marked as "ignored" they are recorded by keystroke -- which means evil records the down and up keys that are used to select a completion, and when the command is repeated these keystrokes are repeated as well. I looked a little bit into company's source and collected everything that I think should be ignored or recorded by change:

(mapc #'evil-declare-change-repeat
      '(company-complete-mouse
        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))

Please check if this looks right (everything that could be executed during completion but does not really insert anything should be ignored, all completion functions that insert text should be recorded by change). This seems to work well in my tests.

Btw, you were right, in contrast to auto-complete company does not trigger the buffer-change hooks, which makes the change-based recording work out of the box (if the functions are marked correctly). In fact, the change-based stuff had been invented for auto-complete in the first place, and I remember my frustration when I realized that this was not sufficient ... ;)

@dgutov
Member
dgutov commented Aug 12, 2013

And this has nothing to do with undo or undo-tree

Ah, okay. I guess I just meant that the default facilities understand the trail of buffer changes we create, so Evil should, too, without special support.

It seems as if and are bound to company-select-next-or-abort and -previous-or-abort.

Yes, sorry. I haven't been using the arrow keys much lately (on a laptop), so just tested this with M-n and M-p.

Please check if this looks right

The lists look right. Thanks for spending time on this. 👍

I remember my frustration when I realized that this was not sufficient

I can sympathize. :)

@ghost
ghost commented Aug 13, 2013

Thanks, I've added the integration code to evil. Let's see if it works ... ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment