Date: Thu, 7 Nov 2013 22:30:09 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH v2 3/5] emacs: help: split out notmuch-describe-key as a
	function
Message-ID: <20131108033009.GL29298@mit.edu>
References: <1383870096-14627-1-git-send-email-markwalters1009@gmail.com>
	<1383870096-14627-4-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1383870096-14627-4-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Nov 08 at 12:21 am:
> The actual documentation function notmuch-describe-keymap was getting
> rather complicated so split out the code for a single key into its own
> function notmuch-describe-key.
> ---
>  emacs/notmuch-lib.el |   42 +++++++++++++++++++++++++-----------------
>  1 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 2195166..8852703 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -237,6 +237,30 @@ This is basically just `format-kbd-macro' but we also convert ESC to M-."
>  	"M-"
>        (concat desc " "))))
>  
> +
> +(defun notmuch-describe-key (actual-key binding prefix ua-keys tail)
> +  "Prepend cons cells describing prefix-arg ACTUAL-KEY and ACTUAL-KEY to TAIL
> +
> +It does not prepend if ACTUAL-KEY is already listed in TAIL."

This obviously works, but I wonder if the interface would be a bit
clearer if, instead of extending tail, this returned a list of just
the descriptions of ACTUAL-KEY.  It would still have to take the
existing bindings to eliminate duplicates, but that becomes just the
list of existing bindings, and not *also* the tail to prepend bindings
to.  I'm imagining something like,

(defun notmuch-describe-key (actual-key binding prefix ua-keys existing)
  ...
  (let ((key-string (concat prefix (format-kbd-macro actual-key))))
    ;; We don't include documentation if the key-binding is
    ;; over-ridden. Note, over-riding a binding automatically hides the
    ;; prefixed version too.
    (unless (assoc key-string tail)
      (cons
	;; Documentation for command
	(cons key-string
	      (or (and (symbolp binding) (get binding 'notmuch-doc))
		  (notmuch-documentation-first-line binding)))
	;; Documentation for prefixed command
	(when (and ua-keys (symbolp binding)
		   (get binding 'notmuch-prefix-doc))
	  (list
	    (let ((ua-desc (key-description ua-keys)))
	      (cons (concat ua-desc " " prefix (format-kbd-macro actual-key))
		    (get binding 'notmuch-prefix-doc)))))))))

Then below,

  (setq tail (nconc (notmuch-describe-key ...) tail))

This patch is also fine as is.

> +  (let ((key-string (concat prefix (format-kbd-macro actual-key))))
> +    ;; We don't include documentation if the key-binding is
> +    ;; over-ridden. Note, over-riding a binding automatically hides the
> +    ;; prefixed version too.
> +    (unless (assoc key-string tail)
> +      (when (and ua-keys (symbolp binding)
> +		 (get binding 'notmuch-prefix-doc))
> +	;; Documentation for prefixed command
> +	(let ((ua-desc (key-description ua-keys)))
> +	  (push (cons (concat ua-desc " " prefix (format-kbd-macro actual-key))
> +		      (get binding 'notmuch-prefix-doc))
> +		tail)))
> +      ;; Documentation for command
> +      (push (cons key-string
> +		  (or (and (symbolp binding) (get binding 'notmuch-doc))
> +		      (notmuch-documentation-first-line binding)))
> +	    tail)))
> +    tail)
> +
>  (defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
>    "Return a list of cons cells, each describing one binding in KEYMAP.
>  
> @@ -256,23 +280,7 @@ prefix argument.  PREFIX and TAIL are used internally."
>  		  (notmuch-describe-keymap
>  		   binding ua-keys (notmuch-prefix-key-description key) tail)))
>  	   (binding
> -	    (let ((key-string (concat prefix (format-kbd-macro (vector key)))))
> -	      ;; We don't include documentation if the key-binding is
> -	      ;; over-ridden. Note, over-riding a binding
> -	      ;; automatically hides the prefixed version too.
> -	      (unless (assoc key-string tail)
> -		(when (and ua-keys (symbolp binding)
> -			   (get binding 'notmuch-prefix-doc))
> -		  ;; Documentation for prefixed command
> -		  (let ((ua-desc (key-description ua-keys)))
> -		    (push (cons (concat ua-desc " " prefix (format-kbd-macro (vector key)))
> -				(get binding 'notmuch-prefix-doc))
> -			  tail)))
> -		;; Documentation for command
> -		(push (cons key-string
> -			    (or (and (symbolp binding) (get binding 'notmuch-doc))
> -				(notmuch-documentation-first-line binding)))
> -		      tail))))))
> +	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
>     keymap)
>    tail)
>