From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 4/6] emacs: Support overriding help and describing prefix
	action
In-Reply-To: <1381029768-11883-5-git-send-email-amdragon@mit.edu>
References: <1381029768-11883-1-git-send-email-amdragon@mit.edu>
	<1381029768-11883-5-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-pc-linux-gnu)
Date: Sun, 06 Oct 2013 21:14:04 +0100
Message-ID: <87wqlqqhrn.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list


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

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).

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