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

Previous Next

Full log

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)]

Previous Next

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