Keypad self insert regression

[Haxxflaxx]

It is no longer possible to insert characters from the keypad.
The regression was introduced in 8ae8b2c.

[DogLooksGood]

I'm curious about the use case on this feature. In which scenario it is useful?

[Haxxflaxx]

It allows for interaction with electric pair mode from normal mode. You can select some thing, then press "SPC (" to surround it with parens.

It is possible that this should be it's own thing, like a self insert next key.

[DogLooksGood]

Probably it's better to just bind self-insert-command to ( in leader to achieve this? I'm thinking maybe we shouldn't support with an option, it's kind pointless to always call a self-insert-command when the key is not defined.

[bplubell]

I thought the intent was to insert a key when it was undefined. Isn't that the behavior that meow-keypad-self-insert-undefined is supposed to enable/disable?

(defcustom meow-keypad-self-insert-undefined t
  "Whether to self-insert a key in keypad mode if it is undefined"
  :group 'meow
  :type 'boolean)

I, too, have become used to using it in the way described by @Haxxflaxx. If we want to change how undefined keys are handled in the leader map, it's important to recognize that it's a breaking change. That would ideally involve some notice of deprecation and clear way to migrate.

Furthermore, I can't seem to achieve the effect you indicate by binding ( to self-insert-command. With the following form, SPC ( just inserts a space at the end of the selection. Am I misunderstanding?

(meow-leader-define-key
 ;; SPC j/k will run the original command in MOTION state.
 '("j" . "H-j")
 '("k" . "H-k")
 '("(" . self-insert-command)
 '("/" . meow-keypad-describe-key)
 '("?" . meow-cheatsheet))

[Haxxflaxx]

It seems like character isn't updated to whatever is pressed inside the prefix, but instead remains the space key.
I also noticed that digit-argument has the same problem.

[DogLooksGood]

I see.

But we need to find a way to make it work again. After the keypad refactor, now the whole key sequences are input in a loop in one command instead of a list of meow-keypad-self-inserts. So even if we call a self-insert-command, it won't insert the last char, instead it inserts the SPC which is the key that used to enter keypad.

[Haxxflaxx]

I've tried adding an explicit self-insert-command to meow--keypad-try-execute

(if meow-keypad-self-insert-undefined
            (self-insert-command (or current-prefix-arg 1) (string-to-char key-str))
          (message "%s is undefined" (meow--keypad-format-keys nil)))

but it doesn't work as expected. The electric-pair-post-self-insert-function uses last-command-event which I then assume just goes back to sending Space.

For this to work as before I think we would need to get last-command-event to be what we pressed in the keypad.

[Haxxflaxx]

(setq last-command-event (string-to-char key-str))
(if meow-keypad-self-insert-undefined
    (call-interactively 'self-insert-command)
  (message "%s is undefined" (meow--keypad-format-keys nil)))

Could be an option if it is save to set the variable.

[Haxxflaxx]

I made a PR with a suggestion on how we can solve this, but I need to research further as it doesn't interact well with beacon state.
#700

[DogLooksGood]

It would be tricky because in kmacro recording, it is the KEY that gets recorded. We could think a better way to achieve the result instead of hacking here.

[Haxxflaxx]

I don't know if I would consider this a hack, but it is an implementation with quite a narrow understanding of the codebase.

After some further research into how keypad execution on beacons work I understand that naturally I need to let meow--keypad-execute handle calling the command.
With this tweaked it works as expected.
I also added an undo-boundary before the call to make the undo ring as expected.

I also now see that my previous assumption that it would work to bind self insert with a disabled meow-keypad-self-insert-undefined. This is due to meow--keypad-lookup-key filtering on the same variable.
We should probably update this and it might also require the same undo-boundary placement.

[Haxxflaxx]

I've also updated meow--keypad-lookup-key to not filter the keypad. I assume the reason we had to do this was that all keys were bound to self insert unless overridden before the refactoring?

After updating meow--keypad-lookup-key it made me think that maybe this is a good time to change the default value of meow-keypad-self-insert-undefined.
While I think it is a good thing to be able to insert on undefined if you want, I don't think it's a good default when we support mapping self insert explicitly in the keypad.

[Haxxflaxx]

I just noticed that keypad no longer triggers repeat maps either. I wonder if the reason is similar?
I would need to explore how repeat-mode works, but I would assume that it checks the last run command and we would report keypad and not the thing run by keypad.

[Haxxflaxx]

After digging into the issue a bit more found that it is indeed the same issue. In the solution which means we can't set last-command-event in a let as the value may be required by functions running after our keypad.

[Kenneth-Mata]

This feels somehow related, after updating using straight (I tend to recompile emacs every few days) I noticed that digit argument is beginning with a -16 prefix when using with leader key, I tried using a clean slate manually loading meow and the issue persists

[Haxxflaxx]

@Kenneth-Mata yes, this is the same thing. It is due to last-command-event not getting updated when executing something from the keypad.

A quick fix could be to do something like:

(defun my/meow-digit-argument ()
    (interactive)
    (let ((last-command-event last-input-event))
      (meow-digit-argument)))

Note that this fix will work poorly for some things as it won't register modifier keys (as you don't press them while using the keypad) but it should work for numbers.

[DogLooksGood]

Just opened another PR #705 for another potential solution.

[Kenneth-Mata]

I tried using that PR but I'm still getting the same -16 prefix behavior, used the default qwerty keymap changing meow-motion-overwrite-define-key to meow-motion-define-key as per the updates