Allow evil-repeat to work

[aaronjensen]

See #168

I don't know what the 'exact status was used for. This changes that, and I don't know if that is important.

With this and this, evil-repeat works as intended:

(mapc #'evil-declare-ignore-repeat
      '(corfu-next
        corfu-previous
        corfu-first
        corfu-last))

(mapc #'evil-declare-change-repeat
      '(corfu-insert
        corfu-complete))

There may be other commands I need to register as well.

[minad]

Hmm, I am not convinced that this should be merged here. The change looks quite dubious with the unread-command-events. Is there any advantage to add this beyond better supporting evil-mode? I mean this change could also go into evil-collection.

I should have been more clear about my general policy in #168 - I won't accept any workarounds for third-party packages in Corfu. Integration issues should be solved by integration packages or on the level of the user configuration. Sorry about that!

[minad]

The general idea here is to replace the current pre-command-hook insertion by corfu-insert + repeating the actual operation. Are there more reasons why we would want that? Company also has similar code, but this is not sufficient reason to add this here too. We should at least figure out the precise reasoning for this specific implementation in Company.

[minad]

I don't know what the 'exact status was used for. This changes that, and I don't know if that is important.

Yes, this is important. 'exact ensures that no snippet expansion happens, which is undesired for TNG, since we want to simulate appending to the currently previewed candidate. If a snippet expansion happens, this will disturb the workflow. Such a change alone is reason enough to not make such a change. We could still define a corfu-insert-exact command and use the same unread-command trickery. But overall this would still look like a hack.

[aaronjensen]

Not sure if @dgutov or @nikital could add more color, but it seems to me like having this in place allows for a buffer change to be explicit in terms of the command that are issued. If Emacs had a facility for "run this command and then the original command anytime anything other than this set of bindings is pressed" then we would want to use that, I would think. It doesn't, so this allows us to simulate that by deferring the user's command and replacing it with our own in this situation so that we can "implicitly" evaluate a command.

Here is the commit where company--unread-this-command-keys was renamed and improved from company--unread-last-input:

company-mode/company-mode@91fc865

It looks like it was originally introduced to create hybrid bindings the active company map would conditionally handle and if it didn't, remove its map and fall back to the underlying binding.

I think it boils down to whether or not you want tab and go to invoke an explicit command when ending a completion or if you would like it to be an implicit buffer modification in a precmd.

[aaronjensen]

If we do end up moving it to evil-collection, would you be open to changing the code somewhat to make it easier to override without having to reimplement all of the pre command?

[minad]

If we do end up moving it to evil-collection, would you be open to changing the code somewhat to make it easier to override without having to reimplement all of the pre command?

We could consider that. However I wonder why we need this convoluted implementation for evil in the first place? Is there a reasonable argument why having this two step insertion procedure is "more correct" in some sense? The reason why I am negative about this PR is that it makes the Corfu slightly more convoluted and complicated, it replaces code which looks rather natural otherwise. Or do you consider it wrong to insert in the pre-command-hook? Are we supposed to only ever apply changes to a buffer from within an actual command which was executed by the event loop? And then, how is it justifiable that we redirect the current command to corfu-insert and resend the input? Probably one could argue that it simulates that the user has pressed TAB followed by the input. Also I am generally skeptical of the usage of unread-command-events. I may be wrong, but I believe that this makes things more brittle.

Another more egoistic argument is that I am growing less interested in maintaining workarounds for problems I don't have or maintaining workarounds which address problems due to specific implementations in specific packages. This does not increase overall quality and it also makes it harder for me to keep things in an acceptable state in the long term. I still hope that my packages can be interesting or useful for a wider user base. But for everything that has a workaround-like feeling, I think it is okay to have higher barriers in place.

[minad]

I missed that you have already given more reasoning in #169 (comment). I have to think about this a bit more.

[aaronjensen]

Sounds good, let me know how I can help.

With regard to not maintaining work arounds I completely understand and I don't think that's egoistic at all. You do a lot for this community and I appreciate it greatly. I'm using a lot of your stuff and it's great. I'm happy to maintain my own workarounds as necessary.

[aaronjensen]

If it helps, Emacs itself uses this technique:
https://github.com/emacs-mirror/emacs/blob/2a5e1d8c44e2a8b49135f5ed51f55cfe610ff5ce/lisp/ebuff-menu.el#L215-L217
https://github.com/emacs-mirror/emacs/blob/2a5e1d8c44e2a8b49135f5ed51f55cfe610ff5ce/lisp/term.el#L4352-L4354

They set unread-command-events in many places:

❯ rg -l 'setq unread-command-events' lisp
lisp/vcursor.el
lisp/emacs-lisp/edebug.el
lisp/emacs-lisp/map-ynp.el
lisp/international/isearch-x.el
lisp/international/quail.el
lisp/calculator.el
lisp/progmodes/simula.el
lisp/transient.el
lisp/isearch.el
lisp/wid-edit.el
lisp/replace.el
lisp/xwidget.el
lisp/double.el
lisp/leim/quail/indian.el
lisp/leim/quail/hangul.el
lisp/leim/quail/japanese.el
lisp/simple.el
lisp/comint.el
lisp/calc/calc.el
lisp/help-macro.el
lisp/language/hanja-util.el
lisp/term/ns-win.el
lisp/window.el
lisp/emulation/cua-base.el
lisp/ebuff-menu.el
lisp/obsolete/vip.el
lisp/obsolete/terminal.el
lisp/ehelp.el
lisp/gnus/gnus-art.el
lisp/subr.el
lisp/term.el
lisp/electric.el

[minad]

They set unread-command-events in many places:

Yes, but these are all hacks or places where it may be necessary to use this. Also Embark makes reasonable use of unread-command-events. Here it is not.

[aaronjensen]

Got it. It sounds like you are leaning towards not doing this here then? Would it make sense to extract a function like corfu--insert-preview that could be overridden? I can send a PR if that would help.

[minad]

I have not yet made up my mind. Maybe you can experiment for a while in a personal fork or keep this in your own configuration. I am kind of busy right now...

[aaronjensen]

No rush at all. Thanks for the consideration. I will keep this branch up to date. I have already updated it to have corfu-insert-exact. Thanks.

[aaronjensen]

Hi @minad, I was curious if you've had a chance to give this any more thought? I've been using my patch this whole time and it has worked well for me, for whatever that is worth.

[minad]

I didn't. But rereading through this again it doesn't seem unreasonable to use the event repeating technique. I wonder if dot-mode would be affected positively by this change too, https://github.com/wyrickre/dot-mode/blob/master/dot-mode.el.

[aaronjensen]

I'd not heard of dot-mode. When I played with it just now, it did not work with my patch as I would hope, but without my patch, I was able to cause an infinite recursion somehow when I hit C-. with the corfu popup open. I suspect dot-mode would need to be taught that certain corfu commands were to be ignored, similar to what I have to do with evil.

I've updated my branch to have a new corfu-insert-exact command, if you reopen this PR it should show up.

[aaronjensen]

@minad would you be open to merging this now by chance?

[minad]

Thanks for reminding me. I am still torn. I just reread the discussion and looked around. While the approach to issue changes only in commands is not unreasonable, it is also convoluted and I don't have the impression that this will have a benefit besides fixing evil-repeat. Can we come up with a emacs-Q scenario, maybe with keyboard macros or something? I understand that you are on the pragmatic side and just want this to be fixed. I am sorry to "invent" these blockers. Otoh I already made the same argument in #169 (comment).

I also checked the Emacs source. Unreading this-command-keys is not a common pattern. But it is used heavily in Company. In Company there are many commands which intercept a key, do something else if some conditions are satisfied or unread the events otherwise. I am not fond of this pattern. Therefore we don't have a corfu--unread-this-command-keys function yet.

[aaronjensen]

Right, Emacs only does it in a couple places, though they do set it often: #169 (comment)

I think that the approach may just be more correct from a command hygiene perspective. Corfu is effectively piggy backing on a command to do something entirely different than that command. This is the only facility Emacs has to do that that I know of that still allows the inserted command to have some identity. That identity is what allows evil-repeat to work. I can appreciate that it seems hacky, but I don’t know of another way.

I’ll take a look and see about submitting a PR that just makes it easier for evil-collection to override the behavior.

[minad]

On 8/21/22 10:54, Aaron Jensen wrote: Right, Emacs only does it in a couple places, though they do set it often: #169 (comment) <#169 (comment)>

Yes, setting unread-command-events is very common, but I am talking about the specific pattern corfu--unread-this-command-events you introduced here.

[minad]

I think that the approach may just be more correct from a command hygiene perspective. Corfu is effectively piggy backing on a command to do something entirely different than that command. This is the only facility Emacs has to do that that I know of that still allows the inserted command to have some identity. That identity is what allows evil-repeat to work. I can appreciate that it seems hacky, but I don’t know of another way.

I agree. But I would like to see a more basic (non-Evil) scenario which is positively affected by this change. I don't understand how the repeat mechanism in Evil works, but it does something inaccurate if commands are repeated but relevant hooks are neglected.

I’ll take a look and see about submitting a PR that just makes it easier for evil-collection to override the behavior.

Don't. This will make the situation only more brittle if we scatter workarounds across Corfu and evil-collection.

[minad]

I looked at evil-collection in the hope that there are not too many hacks present. Fortunately it doesn't look bad, except for these normalize-keymap advices which I don't understand.

Funny thing is that it uses menu-item to redirect commands. This is a technique which could be used by Company to conditionally redirect commands instead of the company--unread-this-command-events hack. See https://github.com/emacs-evil/evil-collection/blob/master/modes/corfu/evil-collection-corfu.el#L97-L107.

[aaronjensen]

I don't totally understand that approach. Would it require binding every single key to a conditional menu-item command, or is there a way to have a catch-all map?

[aaronjensen]

Ok, figured out why corfu wasn't working while recording macros, the auto post command is explicitly disabled. When I comment that out, the unread events technique does not make macros work, so afaict it only helps w/ evil (and avoiding the infinite recursion with dot-mode)

[minad]

I don't totally understand that approach. Would it require binding every single key to a conditional menu-item command, or is there a way to have a catch-all map?

The t binding can usually be used as catch-all. But this technique won't work for this issue here. I just mentioned it since it is an alternative to conditionally redirect commands.

[aaronjensen]

So, I can’t think of anything other than evil-repeat that this will help. The macro system works on recording key presses, so it won’t work with auto at all, but it likely would work with a binding regardless because it just emulates the key presses. Evil-repeat records the commands in a post command hook and evaluates them when replayed.

My suggestion for making it easier to patch is just to extract a method to surround the invocation of corfu—insert ‘exact. If that were done, only that method would need to be advised or overridden in evil-collection, rather than the entire precmd.

That all said, how would you like to proceed?

[aaronjensen]

Actually, to clarify how evil-repeat works: For each command that evil-repeat sees in its pre/post command hooks, it looks up repeat info for that command. For many build-in commands, it uses evil-repeat-keystrokes (effectively the same as Emacs macros as it ultimately uses execute-kbd-macro when it comes time to perform a repeat). In order for a corfu insertion to work, there needs to be a command that evil-repeat can see that has the change type of repeat. That effectively records the actual buffer changes, rather than the keys pressed. By replacing this-command, we allow that to be hooked into.

Given that, this actually works when added to the precmd (instead of unreading the command):

(let ((deferred-keys (this-command-keys)))
  (run-at-time 0 nil (lambda ()
                       (execute-kbd-macro deferred-keys))))
(clear-this-command-keys t)

AFAICT, the clear-this-command-keys is not strictly necessary, but it seems like a reasonable thing to do. I personally think this is more of a hack, but that's likely subjective.