From: Austin Clements <aclements@csail.mit.edu>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/4] emacs: help: remap support
In-Reply-To: <1382821479-23384-4-git-send-email-markwalters1009@gmail.com>
References: <1382821479-23384-1-git-send-email-markwalters1009@gmail.com>
	<1382821479-23384-4-git-send-email-markwalters1009@gmail.com>
User-Agent: Notmuch/0.16+110~g45444ee (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Thu, 07 Nov 2013 14:47:05 -0500
Message-ID: <87habokn9i.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

I originally wanted to see these concerns separated -- to have a
separate function that took care of resolving remaps to keep the logic
out of notmuch-describe-keymap -- but then I tried to write that
function and gave up.  If you can think of a way to do that, I think it
would make this code clearer, but if not that's fine, too.

On Sat, 26 Oct 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> If a user or mode uses remap to rebind a keybinding then it appears in
> the help as a line <remap><function> New function docstring. Special
> case these remapping lines so that we print the actual binding.
> ---
>  emacs/notmuch.el |   21 ++++++++++++++-------
>  1 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index b9db9ba..c354b05 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 ua-keys &optional prefix tail)
> +(defun notmuch-describe-keymap (keymap ua-keys &optional prefix tail remap)
>    "Return a list of cons cells, each describing one binding in KEYMAP.
>  
>  Each cons cell consists of a string giving a human-readable
> @@ -150,16 +150,23 @@ documentation is extracted.
>  
>  UA-KEYS should be a key sequence bound to `universal-argument'.
>  It will be used to describe bindings of commands that support a
> -prefix argument.  PREFIX and TAIL are used internally."
> +prefix argument.  PREFIX, TAIL and REMAP are used internally."
>    (map-keymap
>     (lambda (key binding)
>       (cond ((mouse-event-p key) nil)
>  	   ((keymapp binding)

I think it would be worth documenting what's going on with remaps here.
I didn't get it until I wrote some of my own test code *and* chatted
with you on IRC.  Perhaps something like:

  ;; Remapping 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.

> -	    (setq tail
> -		  (notmuch-describe-keymap
> -		   binding ua-keys (notmuch-prefix-key-description key) tail)))
> +	    (if (equal key 'remap)

`eq' would be more standard here.

> +		(setq tail
> +		      (notmuch-describe-keymap
> +		       binding ua-keys prefix tail t))
> +	      (setq tail
> +		    (notmuch-describe-keymap
> +		     binding ua-keys (notmuch-prefix-key-description key) tail))))
>  	   (t
> -	    (let ((key-string (concat prefix (format-kbd-macro (vector key)))))
> +	    (let* ((actual-key (if remap
> +				   (where-is-internal key nil t)

Is it okay to use nil for the keymap to `where-is-internal' here?  It
seems like maybe we have to pass down a "root" keymap from
`notmuch-substitute-command-keys' so it knows what to resolve the
remappings relative to.  Arguably the original caller should provide
just the root keymap, while the "current" sub-keymap should be one of
the internal recursive arguments.

> +				 (vector key)))
> +		   (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.
> @@ -168,7 +175,7 @@ prefix argument.  PREFIX and TAIL are used internally."
>  			   (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)))
> +		    (push (cons (concat ua-desc " " prefix (format-kbd-macro actual-key))
>  				(get binding 'notmuch-prefix-doc))
>  			  tail)))
>  		;; Documentation for command
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch