Date: Thu, 7 Nov 2013 22:35:51 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH v2 4/5] emacs: help: add a special function to deal with
	remaps
Message-ID: <20131108033551.GM29298@mit.edu>
References: <1383870096-14627-1-git-send-email-markwalters1009@gmail.com>
	<1383870096-14627-5-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-5-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

I like the way you separated this out.

Quoth Mark Walters on Nov 08 at 12:21 am:
> remaps are a rather unusual keymap consisting of "first key" 'remap
> and then "second-key" the remapped-function. Thus we do the
> documentation for it separately.
> ---
>  emacs/notmuch-lib.el |   25 ++++++++++++++++++++++---
>  1 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 8852703..ef616d5 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -261,6 +261,21 @@ It does not prepend if ACTUAL-KEY is already listed in TAIL."
>  	    tail)))
>      tail)
>  
> +(defun notmuch-describe-remaps (remap-keymap ua-keys prefix tail)
> +  ;; Remappings are represented as a binding whose first "event" is
> +  ;; 'remap.  Hence, if the keymap has any remappings, it will have a
> +  ;; binding whose "key" is 'remap, and whose "binding" is itself a
> +  ;; keymap that maps not from keys to commands, but from old (remapped)
> +  ;; functions to the commands to use in their stead.
> +  (map-keymap
> +   (lambda (command binding)
> +     (mapc
> +      (lambda (actual-key)
> +	(setq tail (notmuch-describe-key actual-key binding prefix ua-keys tail)))
> +      (where-is-internal command)))
> +   remap-keymap)
> +  tail)
> +

Hmm.  Maybe holistically the interface change I suggested to the
previous patch isn't the right thing to do, since there ends up being
a lot of symmetry between these three functions.

>  (defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail)
>    "Return a list of cons cells, each describing one binding in KEYMAP.
>  
> @@ -276,9 +291,13 @@ prefix argument.  PREFIX and TAIL are used internally."
>     (lambda (key binding)
>       (cond ((mouse-event-p key) nil)
>  	   ((keymapp binding)
> -	    (setq tail
> -		  (notmuch-describe-keymap
> -		   binding ua-keys (notmuch-prefix-key-description key) tail)))
> +	    (if (eq key 'remap)
> +		(setq tail
> +		      (notmuch-describe-remaps
> +		       binding ua-keys prefix tail))
> +	      (setq tail
> +		    (notmuch-describe-keymap
> +		     binding ua-keys (notmuch-prefix-key-description key) tail))))

Maybe swap the if and the setq?

(setq tail (if (eq key 'remap) ...))

>  	   (binding
>  	    (setq tail (notmuch-describe-key (vector key) binding prefix ua-keys tail)))))
>     keymap)