bug#70208: [PATCH] Add command `list-keyboard-macros`

[Eli Zaretskii]

> Date: Fri, 05 Apr 2024 03:34:59 +0000
> From:  Okamsn via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The attached patch adds the command `list-keyboard-macros`, which works 
> like `list-buffers` using `tabulated-list-mode`.  It allows for 
> re-arranging the macros in the ring; editing their counters, counter 
> formats, and macro keys; deleting macros in the ring; and duplicating 
> macros for further editing.

Thanks, please see some comments below.

> --- a/doc/emacs/kmacro.texi
> +++ b/doc/emacs/kmacro.texi
> @@ -24,6 +24,12 @@ Keyboard Macros
>  keyboard macro is defined and also has been, in effect, executed once.
>  You can then do the whole thing over again by invoking the macro.
>  
> +  The list of defined keyboard macros can be seen via @kbd{M-x
> +list-keyboard-macros @key{RET}}.  This command can be used to re-order
> +the list of defined macros (the @dfn{keyboard macro ring}) and to edit
> +the properties of those keyboard macros, which are described in the
> +following subsections.

Please rewrite this not to use passive tense so much ("can be seen",
"can be used").

Also, I think this command should be documented in more detail,
including the commands in kmacro-menu-mode-map, later in the manual.
In any case, each documented command should be indexed, with an
explicit @findex.

> +*** New mode and new command 'list-keyboard-macros'.

You say "new mode", but don't mention the mode or its name.

Also, since the manuals have been updated by the patch, this entry
should be marked with "+++".

> +(defvar tabulated-list-format)
> +(defvar tabulated-list-entries)
> +(defvar tabulated-list-sort-key)
> +(declare-function tabulated-list-init-header  "tabulated-list" ())
> +(declare-function tabulated-list-print "tabulated-list"
> +                  (&optional remember-pos update))

tabulated-list is preloaded, so I don't think these are needed.

> +(defface kmacro-menu-mark '((t (:inherit font-lock-constant-face)))
> +  "Face used for the Keyboard Macro Menu marks."
> +  :group 'kmacro)
> +
> +(defface kmacro-menu-flagged '((t (:inherit error)))
> +  "Face used for keyboard macros flagged for deletion."
> +  :group 'kmacro)
> +
> +(defface kmacro-menu-marked '((t (:inherit warning)))
> +  "Face used for keyboard macros marked for duplication."
> +  :group 'kmacro)

Please add a :version tag to new faces.

> +(define-derived-mode kmacro-menu-mode tabulated-list-mode
> +  "Keyboard Macro Menu"
> +  "Major mode for listing keyboard macros."
                     ^^^^^^^
"listing and editing", I think?

> +(defun kmacro-menu--kmacros ()
> +  "Return a list of the existing keyboard macros."
             ^^^^^^
"the list"

Also, I think this should say "or nil, if none are defined".

> +(defun kmacro-menu--map-ids (function)
> +  "Map a FUNCTION to the current table entry IDs in order.

Our style is to say "Map FUNCTION", without "a".

Better yet, say "Apply FUNCTION to current table's entries in order."

> +Returns a list of the output of FUNCTION."

"Return", to be consistent with "Map".

> +(defun kmacro-menu--update (kmacros)
> +  "Update the variables for the current and previous keyboard macros.

This doc string doesn't say what are the "variables" to which it
alludes.

> +(defun kmacro-menu--update-at (kmacro n)
> +  "Update to KMACRO at position N."

Not sure I understand what you mean by "Update to" here.  Update what?

> +  (kmacro-menu--update
> +   (kmacro-menu--map-ids (lambda (id)
> +                           (if (= n (kmacro-menu--id-position id))
> +                               kmacro
> +                             (kmacro-menu--id-kmacro id))))))
> +
> +(defun kmacro-menu--query-revert ()
> +  "When the table differs from the existing macros, ask whether to revert 
> table."
      ^^^^
Not "When", but "If", right?

> +  (interactive)

Interactive functions (i.e. commands) should never be internal, so the
double-dash in the name is inappropriate.

> +  (when (and (not (equal (kmacro-menu--kmacros)
> +                         (kmacro-menu--map-ids #'kmacro-menu--id-kmacro)))
> +             (yes-or-no-p "Table does not match existing keyboard macros.  
> Stop and revert table?"))
> +    (tabulated-list-revert)
> +    (signal 'quit nil)))
> +
> +(defun kmacro-menu--assert-row (&optional id)
> +  "Signal an error if point is not on a table row.
> +
> +ID is the tabulated list id of the supposed entry at point."
> +  (unless (or id (tabulated-list-get-id))
> +    (user-error "Not on a table row")))
> +
> +(defun kmacro-menu--propertize-keys (face)
> +  "Redisplay the macro at point with FACE."
> +  (let ((data (tabulated-list-delete-entry)))
> +    (setf (aref (cadr data) 4) (propertize (aref (cadr data) 4) 'face face))
> +    (tabulated-list-print-entry (car data) (cadr data)))
> +  (forward-line -1))
> +
> +(defun kmacro-menu--do-region (function &optional no-region-halt)
> +  "Run FUNCTION on macros in the region or on the current line.

This doesn't explain when it operates on region and when on the
current line.

> +(defun kmacro-menu-flag-for-deletion ()
> +  "Flag the keyboard macro for deletion by `kmacro-menu-do-flagged-delete'.
> +
> +If the region is active, then flag all macros in the region."
   ^^^^^^^^^^^^^^^^^^^^^^^
And if not?

> +(defun kmacro-menu-unmark ()
> +  "Unmark and unflag the keyboard macro at point.
> +
> +If the region is active, then unmark all macros in the region."

The last sentence should say "instead" at the end, to make it clear
that this is alternative to the previous sentence.

> +(defun kmacro-menu-edit-format ()
> +  "Edit the counter format of the keyboard macro at point."

Should the doc string say more about what is a valid format that the
user can type.

> +(defun kmacro-menu-edit-counter ()
> +  "Edit the counter of the keyboard macro at point."

Any motivation? why would a user want to edit the counter?

Last, but not least: please consider making at least some of the
commands in this patch specific to kmacro-menu-mode.