Philip Kaludercic <philipk@posteo.net> wrote:
> Susam Pal <susam.pal@gmail.com> writes:
>
> > Hello!
> >
> > I am the author and maintainer of a new package named Devil. This package
> > intercepts keystrokes entered by the user and applies translation rules to
> > translate the keystrokes into Emacs key sequences. It supports three types
> > of rules: special keys that map to custom commands that are invoked
> > immediately prior to any translations, translation rules to translate Devil
> > key sequences to regular Emacs key sequences, and repeatable keys to allow
> > a Devil key sequence to be repeated by typing the last keystroke over and
> > over again using a transient map.
> >
> > See the README at https://github.com/susam/devil for more details.
>
> Looks interesting, here is a diff with a few comments:

Thanks for the review and the diff! The code looks much better with
these changes. Like we discussed a little while ago in the #emacs
channel, if you send me the patches, I'll apply it to the code.

>
> diff -u /home/philip/devil.el.1 /home/philip/devil.el
> --- /home/philip/devil.el.1    2023-05-09 10:37:55.243222106 +0200
> +++ /home/philip/devil.el    2023-05-09 10:39:50.110922443 +0200
> @@ -36,34 +36,42 @@
>  ;; key sequences without using modifier keys.
>
>  ;;; Code:
> +
> +(defgroup devil '()
> +  "Minor mode for Devil-like command entering." ;is there a clearer description?
> +  :prefix "devil-"
> +  :group 'editing)
> +
>  (defconst devil-version "0.2.0"
>    "Devil version number.")

Yes, a possible description could be:

"Minor mode for intercepting and translating key sequences."

>
> -(defun devil-show-version ()
> +(defun devil-show-version ()        ;is this really needed?
>    "Show Devil version number in the echo area."
>    (interactive)
>    (message "Devil %s" devil-version))

Picked this style from markdown-mode:
https://github.com/jrblevin/markdown-mode/blob/master/markdown-mode.el#L9762-L9765

Although not strictly necessary, I thought it does not hurt to provide
a convenient command here.

> -(defvar devil-key ","
> +(defcustom devil-key ","
>    "The key sequence that begins Devil input.
>
>  The key sequence must be specified in the format returned by `C-h
> -k' (`describe-key'). This variable should be set before enabling
> -Devil mode for it to take effect.")
> -
> -(defvar devil-lighter " Devil"
> -  "String displayed on the mode line when Devil mode is enabled.")
> +k' (`describe-key').  This variable should be set before enabling
> +Devil mode for it to take effect."
> +  :type 'key-sequence)
> +
> +(defcustom devil-lighter " Devil"
> +  "String displayed on the mode line when Devil mode is enabled."
> +  :type 'string)
>
>  (defvar devil-mode-map
>    (let ((map (make-sparse-keymap)))
> -    (define-key map (kbd devil-key) #'devil)
> +    (define-key map devil-key #'devil)
>      map)
>    "Keymap to wake up Devil when `devil-key' is typed.
>
>  By default, only `devil-key' is added to this keymap so that
> -Devil can be activated using it. To support multiple activation
> +Devil can be activated using it.  To support multiple activation
>  keys, this variable may be modified to a new keymap that defines
> -multiple different keys to activate Devil. This variable should
> +multiple different keys to activate Devil.  This variable should
>  be modified before loading Devil for it to take effect.")
>
>  ;;;###autoload
> @@ -74,15 +82,16 @@
>
>  ;;;###autoload
>  (define-globalized-minor-mode
> -  global-devil-mode devil-mode devil--on :group 'devil
> +  global-devil-mode devil-mode devil--on ;; :group 'devil (not needed with the defgroup above)
>    (if global-devil-mode (devil-add-extra-keys) (devil-remove-extra-keys)))
>
>  (defun devil--on ()
>    "Turn Devil mode on."
>    (devil-mode 1))
>
> -(defvar devil-logging nil
> -  "Non-nil if and only if Devil should print log messages.")
> +(defcustom devil-logging nil
> +  "Non-nil if and only if Devil should print log messages."
> +  :type 'boolean)
>
>  (defvar devil-special-keys
>    (list (cons "%k %k" (lambda () (interactive) (devil-run-key "%k")))
> @@ -91,12 +100,12 @@
>    "Special Devil keys that are executed as soon as they are typed.
>
>  The value of this variable is an alist where each key represents
> -a Devil key sequence. If a Devil key sequence matches any key in
> +a Devil key sequence.  If a Devil key sequence matches any key in
>  this alist, the function or lambda in the corresponding value is
> -invoked. The format control specifier `%k' may be used to
> +invoked.  The format control specifier `%k' may be used to
>  represent `devil-key' in the keys.")
>
> -(defvar devil-translations
> +(defcustom devil-translations
>    (list (cons "%k z" "C-")
>          (cons "%k %k" "%k")
>          (cons "%k m m" "M-")
> @@ -108,12 +117,13 @@
>  a translation rule that is applied on the Devil key sequence read
>  from the user to obtain the Emacs key sequence to be executed.
>  The translation rules are applied in the sequence they occur in
> -the alist. For each rule, if the key occurs anywhere in the Devil
> +the alist.  For each rule, if the key occurs anywhere in the Devil
>  key sequence, it is replaced with the corresponding value in the
> -translation rule. The format control specifier `%k' may be used
> -to represent `devil-key' in the keys.")
> +translation rule.  The format control specifier `%k' may be used
> +to represent `devil-key' in the keys."
> +  :type '(alist :key-type string :value-type string))
>
> -(defvar devil-repeatable-keys
> +(defcustom devil-repeatable-keys
>    (list "%k p"
>          "%k n"
>          "%k f"
> @@ -128,8 +138,9 @@
>
>  The value of this variable is a list where each item represents a
>  key sequence that may be repeated merely by typing the last
> -character in the key sequence. The format control specified `%k'
> -may be used to represent `devil-key' in the keys.")
> +character in the key sequence.  The format control specified `%k'
> +may be used to represent `devil-key' in the keys."
> +  :type '(repeat string))
>
>  (defun devil-run-key (key)
>    "Execute the given key sequence KEY.
> @@ -140,7 +151,7 @@
>    (dolist (key (split-string key))
>      (if (string= key "%k") (insert devil-key) (execute-kbd-macro (kbd key)))))
>
> -(defvar devil--saved-keys
> +(defvar devil--saved-keys nil        ;otherwise `devil--saved-keys' is assigned a string
>    "Original key bindings saved by Devil.")
>
>  (defun devil-add-extra-keys ()
> @@ -160,8 +171,9 @@
>
>  (defun devil--original-keys-to-be-saved ()
>    "Return an alist of keys that will be modified by Devil."
> -  (list (cons 'isearch-comma (lookup-key isearch-mode-map (kbd devil-key)))
> -        (cons 'universal-u (lookup-key universal-argument-map (kbd "u")))))
> +  ;; Weak suggestions
> +  `((isearch-comma . ,(lookup-key isearch-mode-map (kbd devil-key)))
> +    universal-u . ,(lookup-key universal-argument-map (kbd "u"))))
>
>  (defun devil ()
>    "Wake up Devil to read and translate Devil key sequences."
> @@ -178,27 +190,30 @@
>  after translation to Emacs key sequence.
>
>  The argument KEY is a vector that represents the key sequence
> -read so far. This function reads a new key from the user, appends
> +read so far.  This function reads a new key from the user, appends
>  it to KEY, and then checks if the result is a valid key sequence
> -or an undefined key sequence. If the result is a valid key
> +or an undefined key sequence.  If the result is a valid key
>  sequence for a special key command or an Emacs command, then the
> -command is executed. Otherwise, this function calls itself
> +command is executed.  Otherwise, this function calls itself
>  recursively to read yet another key from the user."
>    (setq key (vconcat key (vector (read-key (devil--make-prompt key)))))
>    (unless (devil--run-command key)
>      (devil--read-key key)))
>
> -(defvar devil-prompt "Devil: %t"
> +(defcustom devil-prompt "Devil: %t"
>    "A format control string that determines the Devil prompt.
>
>  The following format control sequences are supported:
>
>  %k - Devil key sequence read by Devil so far.
>  %t - Emacs key sequence translated from Devil key sequence read so far.
> -%% - The percent sign.")
> +%% - The percent sign."
> +  :type 'string)
>
>  (defun devil--make-prompt (key)
>    "Create Devil prompt based on the given KEY."
> +  ;; If you are interested in adding Compat as a dependency, you can
> +  ;; make use of `format-spec' without raining the minimum version.
>    (let ((result devil-prompt)
>          (controls (list (cons "%k" (key-description key))
>                          (cons "%t" (devil-translate key))
> @@ -210,16 +225,16 @@
>  (defun devil--run-command (key)
>    "Try running the command bound to the key sequence in KEY.
>
> -KEY is a vector that represents a sequence of keystrokes. If KEY
> +KEY is a vector that represents a sequence of keystrokes.  If KEY
>  is found to be a special key in `devil-special-keys', the
>  corresponding special command is executed immediately and t is
>  returned.
>
>  Otherwise, it is translated to an Emacs key sequence using
> -`devil-translations'. If the resulting Emacs key sequence is
> +`devil-translations'.  If the resulting Emacs key sequence is
>  found to be a complete key sequence, the command it is bound to
> -is executed interactively and t is returned. If it is found to be
> -an undefined key sequence, then t is returned. If the resulting
> +is executed interactively and t is returned.  If it is found to be
> +an undefined key sequence, then t is returned.  If the resulting
>  Emacs key sequence is found to be an incomplete key sequence,
>  then nil is returned."
>    (devil--log "Trying to execute key: %s" (key-description key))
> @@ -231,7 +246,7 @@
>
>  If the given key sequence KEY is found to be a special key in
>  `devil-special-keys', the corresponding special command is
> -executed, and t is returned. Otherwise nil is returned."
> +executed, and t is returned.  Otherwise nil is returned."
>    (catch 'break
>      (dolist (entry devil-special-keys)
>        (when (string= (key-description key) (devil-format (car entry)))
> @@ -245,14 +260,14 @@
>
>  After translating KEY to an Emacs key sequence, if the resulting
>  key sequence turns out to be an incomplete key, then nil is
> -returned. If it turns out to be a complete key sequence, the
> -corresponding Emacs command is executed, and t is returned. If it
> -turns out to be an undefined key sequence, t is returned. The
> +returned.  If it turns out to be a complete key sequence, the
> +corresponding Emacs command is executed, and t is returned.  If it
> +turns out to be an undefined key sequence, t is returned.  The
>  return value t indicates to the caller that no more Devil key
>  sequences should be read from the user."
>    (let* ((described-key (key-description key))
>           (translated-key (devil-translate key))
> -         (parsed-key (condition-case nil (kbd translated-key) (error nil)))
> +         (parsed-key (condition-case nil (kbd translated-key) (error nil))) ;or `ignore-errors'
>           (binding (when parsed-key (key-binding parsed-key))))
>      (cond ((string-match "[ACHMSs]-$" translated-key)
>             (devil--log "Ignoring incomplete key: %s => %s"
> @@ -307,27 +322,27 @@
>    "Update variables that maintain command loop information.
>
>  The given KEY and BINDING is used to update variables that
> -maintain command loop information. This allows the commands that
> +maintain command loop information.  This allows the commands that
>  depend on them behave as if they were being invoked directly with
>  the original Emacs key sequence."
>    ;;
>    ;; Set `last-command-event' so that `digit-argument' can determine
> -  ;; the correct digit for key sequences like , 5 (C-5). See M-x
> +  ;; the correct digit for key sequences like , 5 (C-5).  See M-x
>    ;; find-function RET digit-argument RET for details.
>    (setq last-command-event (aref key (- (length key) 1)))
>    ;;
>    ;; Set `this-command' to make several commands like , z SPC , z SPC
> -  ;; (C-SPC C-SPC) and , p (C-p) work correctly. Emacs copies
> -  ;; `this-command' to `last-command'. Both variables are used by
> +  ;; (C-SPC C-SPC) and , p (C-p) work correctly.  Emacs copies
> +  ;; `this-command' to `last-command'.  Both variables are used by
>    ;; `set-mark-command' to decide whether to activate/deactivate the
> -  ;; current mark. The first variable is used by vertical motion
> -  ;; commands to keep the cursor at the `temporary-goal-column'. There
> +  ;; current mark.  The first variable is used by vertical motion
> +  ;; commands to keep the cursor at the `temporary-goal-column'.  There
>    ;; may be other commands too that depend on this variable.
>    (setq this-command binding)
>    ;;
>    ;; Set `real-this-command' to make , x z (C-x z) work correctly.
>    ;; Emacs copies it to `last-repeatable-command' which is then used
> -  ;; by repeat. See the following for more details:
> +  ;; by repeat.  See the following for more details:
>    ;;
>    ;;   - M-x find-function RET repeat RET
>    ;;   - C-h v last-repeatable-command RET
> @@ -337,11 +352,12 @@
>  (defun devil--log-command-loop-info ()
>    "Log command loop information for debugging purpose."
>    (devil--log
> -   (concat "Found "
> -           (format "current-prefix-arg: %s; " current-prefix-arg)
> -           (format "this-command: %s; " this-command)
> -           (format "last-command: %s; " last-command)
> -           (format "last-repeatable-command: %s" last-repeatable-command))))
> +   (format "Found current-prefix-arg: %s; \
> +this-command: %s; last-command: %s; last-repeatable-command: %s"
> +       current-prefix-arg
> +       this-command
> +       last-command
> +       last-repeatable-command)))
>
>  (defun devil--repeatable-key-p (described-key)
>    "Return t iff DESCRIBED-KEY belongs to `devil-repeatable-keys'."
> @@ -379,28 +395,25 @@
>    (when devil-logging
>      (apply #'message (concat "Devil: " format-string) args)))
>
> -(defmacro devil--assert (form)
> -  "Evaluate FORM and cause error if the result is nil."
> -  `(unless ,form
> -     (error "Assertion failed: %s" ',form)))
> -
> -(defun devil--tests ()
> -  "Test Devil functions assuming Devil has not been customized."
> -  (devil--assert (devil--invalid-key-p ""))
> -  (devil--assert (devil--invalid-key-p "C-x-C-f"))
> -  (devil--assert (devil--invalid-key-p "C-x CC-f"))
> -  (devil--assert (not (devil--invalid-key-p "C-x C-f")))
> -  (devil--assert (not (devil--invalid-key-p "C-M-x")))
> -  (devil--assert (string= (devil-translate (vconcat ",")) "C-"))
> -  (devil--assert (string= (devil-translate (vconcat ",x")) "C-x"))
> -  (devil--assert (string= (devil-translate (vconcat ",x,")) "C-x C-"))
> -  (devil--assert (string= (devil-translate (vconcat ",x,f")) "C-x C-f"))
> -  (devil--assert (string= (devil-translate (vconcat ",,")) ","))
> -  (devil--assert (string= (devil-translate (vconcat ",,,,")) ", ,"))
> -  (devil--assert (string= (devil-translate (vconcat ",mx")) "C-M-x"))
> -  (devil--assert (string= (devil-translate (vconcat ",mmx")) "M-x"))
> -  (devil--assert (string= (devil-translate (vconcat ",mmm")) "M-m"))
> -  (message "Done"))
> +(ert-deftest devil-invalid-key-p ()
> +  "Test if `devil--invalid-key-p' words as expected."
> +  (should (devil--invalid-key-p ""))
> +  (should (devil--invalid-key-p "C-x-C-f"))
> +  (should (devil--invalid-key-p "C-x CC-f"))
> +  (should (not (devil--invalid-key-p "C-x C-f")))
> +  (should (not (devil--invalid-key-p "C-M-x"))))
> +
> +(ert-deftest devil-translate ()
> +  "Test if `devil-translate' works as expected."
> +  (should (string= (devil-translate (vconcat ",")) "C-"))
> +  (should (string= (devil-translate (vconcat ",x")) "C-x"))
> +  (should (string= (devil-translate (vconcat ",x,")) "C-x C-"))
> +  (should (string= (devil-translate (vconcat ",x,f")) "C-x C-f"))
> +  (should (string= (devil-translate (vconcat ",,")) ","))
> +  (should (string= (devil-translate (vconcat ",,,,")) ", ,"))
> +  (should (string= (devil-translate (vconcat ",mx")) "C-M-x"))
> +  (should (string= (devil-translate (vconcat ",mmx")) "M-x"))
> +  (should (string= (devil-translate (vconcat ",mmm")) "M-m")))
>
>  (provide 'devil)
>
>
> Diff finished.  Tue May  9 10:39:57 2023
>
> --=-=-=
> Content-Type: text/plain
>
>
> > Also, see https://www.reddit.com/r/emacs/comments/13aj99j/ for some
> > discussion on this new package.
> >
> > Please let me know if this package can be added to NonGNU ELPA. If there
> > are any questions or feedback about this, please let me know.
>
> I see no reason why not.  Thanks for contributing.
>
> One point I have already mentioned on IRC is that a separate manual and
> a shorter README would be nice.  We can generate a TeXinfo manual from
> either .texi or .org files, would you be interested in setting that up?

This is a great idea. I have a busy schedule coming up in the next few
weeks, so it may take me a while to do this though. In the meantime,
if someone is willing to help with this, I will gladly accept the
changes.

>
> > Regards,
> > Susam
> > From 3499d93502b130b1dcb8a648e73e7a614cd24abd Mon Sep 17 00:00:00 2001
> > From: Susam Pal <susam@susam.net>
> > Date: Tue, 9 May 2023 02:36:08 +0100
> > Subject: [PATCH] * elpa-packages (devil): New package
> >
> > ---
> >  elpa-packages | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/elpa-packages b/elpa-packages
> > index c333cc8bb3..f6fd68edaf 100644
> > --- a/elpa-packages
> > +++ b/elpa-packages
> > @@ -112,6 +112,8 @@
> >
> >   (devhelp               :url "https://codeberg.org/akib/emacs-devhelp")
> >
> > + (devil                 :url "https://github.com/susam/devil")
>
> As you have a CHANGES.md file, we can add a :news entry here as well.

Attached an updated patch.

>
> > +
> >   (diff-ansi        :url "https://codeberg.org/ideasman42/emacs-diff-ansi"
> >    :ignored-files ("LICENSE"))

I believe we can continue working on the proposed improvements and
make them available in new versions. Is there anything that is
currently a blocker to include this package into NonGNU ELPA?

Regards,
Susam