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

From: Eli Zaretskii <[email-masked]>
Date: Sun, 07 Apr 2024 10:55:10 +0300

> Date: Sat, 06 Apr 2024 23:26:15 +0000
> From: Okamsn <[email-masked]>
> Cc: [email-masked]
> 
> +@node Kmacro Menu
> +@section Listing Keyboard Macros

"Listing and Editing", I guess?

> +After a command is run, the Kmacro Menu resets to show the new values of
                                           ^^^^^^
"Resets" is not the best word here.  I suggest to rephrase:

  After a command is run, the Kmacro Menu display changes to reflect
  the new values of ...

> +the macro properties and the macro ring.  The usual cursor motion
> +commands can be used in this buffer.

"You can use the usual cursor motion commands in this buffer."  This
avoids passive tense.

> +@item D @r{(Kmacro Menu)}
> +This command deletes macros, removing them from the ring
> +(@code{kmacro-menu-do-delete}).  For example, running this command on
> +the macro at position zero will delete the current macro and then make
> +the first macro in the macro ring (previously at position one) the new
> +current macro, popping it from the ring.
> +
> +  If the region is active, this command deletes the macros in the
> +region.  Otherwise, if there are marked macros, this command deletes the
> +marked macros.  If there is no region nor are there marked macros, this
> +command deletes the macro on the current line.  In all cases, the
> +command prompts for confirmation before duplication.
                                    ^^^^^^^^^^^^^^^^^^
"before deletion", right?

> ++++
> +*** New mode 'kmacro-menu-mode' and new command 'list-keyboard-macros'.
> +The new command 'list-keyboard-macros' the macro version of commands
                                         ^
I think "is" is missing there.

> +(defface kmacro-menu-mark '((t (:inherit font-lock-constant-face)))
> +  "Face used for the Keyboard Macro Menu marks."
> +  :group 'kmacro
> +  :version "30.0.50")

The version should be "30.1", the next released version (here and
elsewhere in the patch).  We never tag options with development
versions.

> +(defun kmacro-menu-mark ()
> +  "Mark macros in the region or, otherwise, on the current line.

I'd remove the "otherwise" part, and explain that in the next lines:

    Mark macros in the region or on the current line.

  If there's an active region, mark macros in the region; otherwise
  mark the macro on the current line.

> +(defun kmacro-menu-flag-for-deletion ()
> +  "Flag macros in the region or, otherwise, on the current line.

Likewise here and in all other similar commands (some of them already
have the "if there's an active region" part).

Thanks.