repeat-help icon indicating copy to clipboard operation
repeat-help copied to clipboard

Use `real-last-command` alongside `last-command`.

Open LemonBreezes opened this issue 3 years ago • 3 comments

Some commands such as evil-scroll-down override this-command (in this case, with next-line). This makes repeat-help show an empty keymap when I use evil-scroll-down. This PR removes that behavior by using real-last-command as well.

I also added a few of Evil's recentering commands to repeat-help-persist-map.

This closes #5.

EDIT: By switching from defsubst to define-inline, we have also fixed #4.

LemonBreezes avatar Jul 26 '22 23:07 LemonBreezes

Thanks, this looks good.

If I understand correctly, the defsubsts were causing errors at native-comp with speed 3?

I'm not sure about modifying real-this-command. The documentation says:

This is like ‘this-command’, except that commands should never modify it.

It's probably safe to change inside a let block, but I'd like to get some input from an Elisp expert first. There might be a valid reason to let it remain 'read-only'. I'll ask on emacs-devel soon.

Lastly, I think I should turn repeat-help-persist-map into a defcustom -- or perhaps create a repeat-help-persist-commands defcustom so the user can set whatever commands they want repeat-mode to ignore when chaining. And the evil-specific configuration can then go into a (with-eval-after-load 'evil) block or just added to the README instead.

karthink avatar Aug 01 '22 22:08 karthink

If I understand correctly, the defsubsts were causing errors at native-comp with speed 3?

Yes. If I use the following config on the left, Emacs patched to native-comp-speed 3 as a default, and the commands on the right (possibly out of order), I get the error Error in post-command-hook (repeat-post-hook): (void-function nil). image This error goes away if I recompile repeat-help with native-comp-speed equal to 2 OR if I patch repeat-help to use define-inline instead of defsubst.

I'm not sure about modifying real-this-command. The documentation says:

This is like ‘this-command’, except that commands should never modify it.

It's probably safe to change inside a let block, but I'd like to get some input from an Elisp expert first. There might be a valid reason to let it remain 'read-only'. I'll ask on emacs-devel soon.

Sure. The real why I modified real-this-command is that evil-scroll-down sets this-command to next-line and from the code of repeat.el, image we can see that repeat looks up repeat maps first with this-command and then with real-this-command. So for a command like evil-scroll-down which overrides this-command, repeat-help will not detect the repeat map without overriding real-this-command as well.

Lastly, I think I should turn repeat-help-persist-map into a defcustom -- or perhaps create a repeat-help-persist-commands defcustom so the user can set whatever commands they want repeat-mode to ignore when chaining. And the evil-specific configuration can then go into a (with-eval-after-load 'evil) block or just added to the README instead.

:+1:

LemonBreezes avatar Aug 01 '22 23:08 LemonBreezes

Thanks, this looks good.

If I understand correctly, the defsubsts were causing errors at native-comp with speed 3?

I'm not sure about modifying real-this-command. The documentation says:

This is like ‘this-command’, except that commands should never modify it.

It's probably safe to change inside a let block, but I'd like to get some input from an Elisp expert first. There might be a valid reason to let it remain 'read-only'. I'll ask on emacs-devel soon.

Lastly, I think I should turn repeat-help-persist-map into a defcustom -- or perhaps create a repeat-help-persist-commands defcustom so the user can set whatever commands they want repeat-mode to ignore when chaining. And the evil-specific configuration can then go into a (with-eval-after-load 'evil) block or just added to the README instead.

Any update in regard to this PR? I still think that we should merge it because messing with real-this-command is necessary for this package to work properly. Even repeat.el does it.

LemonBreezes avatar Aug 09 '23 22:08 LemonBreezes