Date: Mon, 7 Oct 2013 18:49:57 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 4/6] emacs: Support overriding help and describing prefix
	action
Message-ID: <20131007224957.GL21611@mit.edu>
References: <1381029768-11883-1-git-send-email-amdragon@mit.edu>
	<1381029768-11883-5-git-send-email-amdragon@mit.edu>
	<87wqlqqhrn.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87wqlqqhrn.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Oct 06 at  9:14 pm:
> 
> 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).

Just sent a follow-up patch that improves this function documentation.

> 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)))))