GNU bug report logs - #70208
[PATCH] Add command `list-keyboard-macros`

Previous Next

To reply to this bug, email your comments to 70208 AT debbugs.gnu.org.
There is no need to reopen the bug first.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Okamsn <okamsn <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add command `list-keyboard-macros`
Date: Fri, 05 Apr 2024 03:34:59 +0000

[Message part 1 (text/plain, inline)]

Hello,

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.

Please let me know what you think. I followed the naming scheme used by 
Dired for a few of the commands, but don't know if that's best.

Thank you.

[0001-Add-command-list-keyboard-macros-that-works-like-lis.patch (text/x-patch, attachment)]

Message #8 received at 70208 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Okamsn <okamsn <at> protonmail.com>
Cc: 70208 <at> debbugs.gnu.org
Subject: Re: bug#70208: [PATCH] Add command `list-keyboard-macros`
Date: Fri, 05 Apr 2024 09:16:12 +0300

> 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 <at> 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.

Message #11 received at 70208 <at> debbugs.gnu.org (full text, mbox):

From: Okamsn <okamsn <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70208 <at> debbugs.gnu.org
Subject: Re: bug#70208: [PATCH] Add command `list-keyboard-macros`
Date: Sat, 06 Apr 2024 23:26:15 +0000

[Message part 1 (text/plain, inline)]

Eli Zaretskii wrote:
>> --- 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.

I moved the description to its own section. How is it now? I copied part 
of the Texinfo documentation of `list-buffers` and the Buffer Menu node.

>> +*** 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 "+++".

Done.

>> +(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.

Removed.

>> +(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.

Done.

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

Done.

>> +(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".

Changed.

>> +(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".

Changed.

>> +(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.

Changed.

>> +(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?

I changed the functions to use "replace" instead of "update".  What I 
meant was that only existing keyboard macro at that position would be 
replaced. The others would be re-used.

>> +  (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?

Yes. Changed.

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

That function wasn't meant to be a command. I removed the `interactive` use.

> ... 

I changed the wording of the commands that act on the region when there 
is one.  Please check them again.

> 
>> +(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.

I added that.

>> +(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?

Sometimes, I want to fix a mistake in a keyboard macro and then re-run 
it with a previous counter value.  Another possibility is duplicating a 
macro, changing the definition somewhat for a different context, and 
then setting the counter back to 0 or another value.

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

That is what I meant to do by giving the mode in the `declare` form. I 
added the mode for the `interactive` form too. Is that what you mean?

Thank you.

[v2-0001-Add-command-list-keyboard-macros-that-works-like-.patch (text/x-patch, attachment)]

Message #14 received at 70208 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Okamsn <okamsn <at> protonmail.com>
Cc: 70208 <at> debbugs.gnu.org
Subject: Re: bug#70208: [PATCH] Add command `list-keyboard-macros`
Date: Sun, 07 Apr 2024 10:55:10 +0300

> Date: Sat, 06 Apr 2024 23:26:15 +0000
> From: Okamsn <okamsn <at> protonmail.com>
> Cc: 70208 <at> debbugs.gnu.org
> 
> +@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.

Message #17 received at 70208 <at> debbugs.gnu.org (full text, mbox):

From: Okamsn <okamsn <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 70208 <at> debbugs.gnu.org
Subject: Re: bug#70208: [PATCH] Add command `list-keyboard-macros`
Date: Sat, 13 Apr 2024 19:24:45 +0000

[Message part 1 (text/plain, inline)]

Eli Zaretskii wrote:
>> Date: Sat, 06 Apr 2024 23:26:15 +0000
>> From: Okamsn <okamsn <at> protonmail.com>
>> Cc: 70208 <at> debbugs.gnu.org
>>
>> +@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.

Please see the attached.

Thank you.

[v4-0001-Add-command-list-keyboard-macros-that-works-like-.patch (text/x-patch, attachment)]

Message #22 received at 70208-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Okamsn <okamsn <at> protonmail.com>
Cc: 70208-done <at> debbugs.gnu.org
Subject: Re: bug#70208: [PATCH] Add command `list-keyboard-macros`
Date: Sun, 14 Apr 2024 12:41:30 +0300

> Date: Sat, 13 Apr 2024 19:24:45 +0000
> From: Okamsn <okamsn <at> protonmail.com>
> Cc: 70208 <at> debbugs.gnu.org
> 
> Please see the attached.

Thanks, installed, and closing the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.