From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 0/2] emacs: Shortcut keys to saved searches
In-Reply-To: <1405433166-2198-1-git-send-email-amdragon@mit.edu>
References: <1405353735-26244-1-git-send-email-amdragon@mit.edu>
	<1405433166-2198-1-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.15.2+615~g78e3a93 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Tue, 15 Jul 2014 21:18:48 +0100
Message-ID: <87lhru5qaf.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Tue, 15 Jul 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> This is version 2 of
> id:1405353735-26244-1-git-send-email-amdragon@mit.edu and addresses
> Mark's comments in id:87egxnd4aq.fsf@qmul.ac.uk.
>
> The diff from v1 is below.

This version gets a +1 from me. I would have a slight preference for
making the prompt "Jump to search: " rather than "Search: " to emphasise
that you can't type an actual search, but am definitely happy with this
version.

Best wishes

Mark




>
> diff --git a/emacs/notmuch-jump.el b/emacs/notmuch-jump.el
> index cb1ae10..9cb1e6a 100644
> --- a/emacs/notmuch-jump.el
> +++ b/emacs/notmuch-jump.el
> @@ -53,20 +53,19 @@ (defun notmuch-jump-search ()
>      (setq action-map (nreverse action-map))
>  
>      (if action-map
> -	(notmuch-jump action-map "Search ")
> -      (error "No shortcut keys for saved searches.  Please customize notmuch-saved-searches."))))
> +	(notmuch-jump action-map "Search: ")
> +      (error "To use notmuch-jump, please customize shortcut keys in notmuch-saved-searches."))))
>  
>  (defvar notmuch-jump--action nil)
>  
>  (defun notmuch-jump (action-map prompt)
>    "Interactively prompt for one of the keys in ACTION-MAP.
>  
> -Displays a pop-up temporary buffer with a summary of all bindings
> -in ACTION-MAP, reads a key from the minibuffer, and performs the
> -corresponding action.  The prompt can be canceled with C-g.
> -PROMPT must be a string to use for the prompt if this command was
> -not invoked directly by a key binding (e.g., it was invoked
> -through M-x).  PROMPT should include a space at the end.
> +Displays a summary of all bindings in ACTION-MAP in the
> +minibuffer, reads a key from the minibuffer, and performs the
> +corresponding action.  The prompt can be canceled with C-g or
> +RET.  PROMPT must be a string to use for the prompt.  PROMPT
> +should include a space at the end.
>  
>  ACTION-MAP must be a list of triples of the form
>    (KEY LABEL ACTION)
> @@ -82,16 +81,9 @@ (defun notmuch-jump (action-map prompt)
>  	  (with-temp-buffer
>  	    (notmuch-jump--insert-items (window-body-width) items)
>  	    (buffer-string)))
> -	 (prompt-text
> -	  (if (eq this-original-command this-command)
> -	      ;; Make it look like we're just part of any regular
> -	      ;; submap prompt (like C-x, C-c, etc.)
> -	      (concat (format-kbd-macro (this-command-keys)) "-")
> -	    ;; We were invoked through something like M-x
> -	    prompt))
>  	 (full-prompt
>  	  (concat table "\n\n"
> -		  (propertize prompt-text 'face 'minibuffer-prompt)))
> +		  (propertize prompt 'face 'minibuffer-prompt)))
>  	 ;; By default, the minibuffer applies the minibuffer face to
>  	 ;; the entire prompt.  However, we want to clearly
>  	 ;; distinguish bindings (which we put in the prompt face
> @@ -121,14 +113,14 @@ (defun notmuch-jump--format-actions (action-map)
>  
>    ;; Compute the maximum key description width
>    (let ((key-width 1))
> -    (dolist (action action-map)
> +    (dolist (entry action-map)
>        (setq key-width
>  	    (max key-width
> -		 (string-width (format-kbd-macro (first action))))))
> +		 (string-width (format-kbd-macro (first entry))))))
>      ;; Format each action
> -    (mapcar (lambda (action)
> -	      (let ((key (format-kbd-macro (first action)))
> -		    (desc (second action)))
> +    (mapcar (lambda (entry)
> +	      (let ((key (format-kbd-macro (first entry)))
> +		    (desc (second entry)))
>  		(concat
>  		 (propertize key 'face 'minibuffer-prompt)
>  		 (make-string (- key-width (length key)) ? )
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 135422d..b338aaa 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -25,6 +25,9 @@
>  (require 'mm-decode)
>  (require 'cl)
>  
> +(autoload 'notmuch-jump-search "notmuch-jump"
> +  "Jump to a saved search by shortcut key." t)
> +
>  (defvar notmuch-command "notmuch"
>    "Command to run the notmuch binary.")
>  
> @@ -134,8 +137,6 @@ (defvar notmuch-common-keymap
>      map)
>    "Keymap shared by all notmuch modes.")
>  
> -(autoload 'notmuch-jump-search "notmuch-jump" "Jump to a saved search by shortcut key." t)
> -
>  ;; By default clicking on a button does not select the window
>  ;; containing the button (as opposed to clicking on a widget which
>  ;; does). This means that the button action is then executed in the