Re: [PATCH 4/6] emacs: Support overriding help and describing prefix action

[Tomi Ollila <tomi.ollila@iki.fi>]

On Sun, Oct 06 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> This whole series looks good to me. If you are rolling another version
> for any reason I have one trivial comment
>
> On Sun, 06 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> Traditionally, function documentation strings are intended primarily
>> for programmers, rather than users.  They're written from the
>> perspective of calling the function, not interactively invoking it.
>> They're only ever displayed along with the function prototype (and
>> often refer to argument names).  And built-in help commands like
>> `describe-bindings' show the name of the command, not its
>> documentation.
>>
>> The notmuch help system is like `describe-bindings', but tries to be
>> more user-friendly by displaying documentation strings, rather than
>> Elisp command names.  For most commands, this is fine, but for some
>> the "programmer description" is inappropriate for interactive use.
>> This is particularly noticeable for commands that take an optional
>> prefix argument.
>>
>> This patch adds support for two symbol properties: notmuch-doc and
>> notmuch-prefix-doc, which let a command override its interactive
>> documentation and provide separate documentation for its prefixed
>> invocation.  If notmuch-prefix-doc is present, we add an extra line to
>> the help giving the prefixed key sequence along with the documentation
>> for the prefixed command.
>> ---
>>  emacs/notmuch.el | 29 ++++++++++++++++++++++++-----
>>  1 file changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> index a36849f..278bd35 100644
>> --- a/emacs/notmuch.el
>> +++ b/emacs/notmuch.el
>> @@ -140,7 +140,7 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
>>  	"M-"
>>        (concat desc " "))))
>>  
>> -(defun notmuch-describe-keymap (keymap &optional prefix tail)
>> +(defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
>>    "Return a list of strings, each describing one key in KEYMAP.
>>  
>>  Each string gives a human-readable description of the key and the

This series Looks Good To Me, too.

> It would be nice to document the ua-keys variable here. It took me some
> time to work out what was going in (and I worked out based on the
> caller).

I did not have any problems understanding this -- probably due to me
having played so much with where-is-internal and such commands...
... just that this looks to me as 'first-ua-key' (ua being unversal 
argument, not user agent ;)

If there is need for new version then documenting this would not hurt...
if not followup-patch to add this documentation (w/ one line commit
message for least review effort! ;) could do)

Tomi

>
> Best wishes
>
> Mark
>
>> @@ -151,10 +151,19 @@ first line of documentation for the bound function."
>>  	   ((keymapp binding)
>>  	    (setq tail
>>  		  (notmuch-describe-keymap
>> -		   binding (notmuch-prefix-key-description key) tail)))
>> +		   binding ua-keys (notmuch-prefix-key-description key) tail)))
>>  	   (t
>> +	    (when (and ua-keys (symbolp binding)
>> +		       (get binding 'notmuch-prefix-doc))
>> +	      ;; Documentation for prefixed command
>> +	      (let ((ua-desc (key-description ua-keys)))
>> +		(push (concat ua-desc " " prefix (format-kbd-macro (vector key))
>> +			      "\t" (get binding 'notmuch-prefix-doc))
>> +		      tail)))
>> +	    ;; Documentation for command
>>  	    (push (concat prefix (format-kbd-macro (vector key)) "\t"
>> -			  (notmuch-documentation-first-line binding))
>> +			  (or (and (symbolp binding) (get binding 'notmuch-doc))
>> +			      (notmuch-documentation-first-line binding)))
>>  		  tail))))
>>     keymap)
>>    tail)
>> @@ -165,14 +174,24 @@ first line of documentation for the bound function."
>>      (while (string-match "\\\\{\\([^}[:space:]]*\\)}" doc beg)
>>        (let* ((keymap-name (substring doc (match-beginning 1) (match-end 1)))
>>  	     (keymap (symbol-value (intern keymap-name)))
>> -	     (desc-list (notmuch-describe-keymap keymap))
>> +	     (ua-keys (where-is-internal 'universal-argument keymap t))
>> +	     (desc-list (notmuch-describe-keymap keymap ua-keys))
>>  	     (desc (mapconcat #'identity desc-list "\n")))
>>  	(setq doc (replace-match desc 1 1 doc)))
>>        (setq beg (match-end 0)))
>>      doc))
>>  
>>  (defun notmuch-help ()
>> -  "Display help for the current notmuch mode."
>> +  "Display help for the current notmuch mode.
>> +
>> +This is similar to `describe-function' for the current major
>> +mode, but bindings tables are shown with documentation strings
>> +rather than command names.  By default, this uses the first line
>> +of each command's documentation string.  A command can override
>> +this by setting the 'notmuch-doc property of its command symbol.
>> +A command that supports a prefix argument can explicitly document
>> +its prefixed behavior by setting the 'notmuch-prefix-doc property
>> +of its command symbol."
>>    (interactive)
>>    (let* ((mode major-mode)
>>  	 (doc (substitute-command-keys (notmuch-substitute-command-keys (documentation mode t)))))
>> -- 
>> 1.8.4.rc3
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch