[PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to saved searches

[Austin Clements]

Quoth Mark Walters on Jul 14 at 10:22 pm:
> 
> On Mon, 14 Jul 2014, Austin Clements <amdragon at MIT.EDU> wrote:
> > This introduces notmuch-jump, which is like a user-friendly,
> > user-configurable global prefix map for saved searches.  This provides
> > a non-modal and much faster way to access saved searches than
> > notmuch-hello.
> >
> > A user configures shortcut keys in notmuch-saved-searches, which are
> > immediately accessible from anywhere in Notmuch under the "j" key (for
> > "jump").  When the user hits "j", the minibuffer immediately shows a
> > helpful table of bindings reminiscent of a completions buffer.
> 
> I am basically happy with this: the only downside compared to dme's
> patch is that this is quite substantially bigger. However, since this is
> all in it's own file that is not really a problem.
> 
> I have a few comments below. In all cases I am happy to go with the
> current code if you think it's better than my suggestion.
> 
> > This code is a combination of work from myself (originally,
> > "notmuch-go"), David Edmondson, and modifications from Mark Walters.
> > ---
> >  emacs/Makefile.local   |   3 +-
> >  emacs/notmuch-hello.el |   2 +
> >  emacs/notmuch-jump.el  | 189 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  emacs/notmuch-lib.el   |   3 +
> >  4 files changed, 196 insertions(+), 1 deletion(-)
> >  create mode 100644 emacs/notmuch-jump.el
> >
> > diff --git a/emacs/Makefile.local b/emacs/Makefile.local
> > index c0d6b19..1109cfa 100644
> > --- a/emacs/Makefile.local
> > +++ b/emacs/Makefile.local
> > @@ -18,7 +18,8 @@ emacs_sources := \
> >  	$(dir)/notmuch-tag.el \
> >  	$(dir)/coolj.el \
> >  	$(dir)/notmuch-print.el \
> > -	$(dir)/notmuch-version.el
> > +	$(dir)/notmuch-version.el \
> > +	$(dir)/notmuch-jump.el \
> >  
> >  $(dir)/notmuch-version.el: $(dir)/Makefile.local version.stamp
> >  $(dir)/notmuch-version.el: $(srcdir)/$(dir)/notmuch-version.el.tmpl
> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> > index 3de5238..061b27d 100644
> > --- a/emacs/notmuch-hello.el
> > +++ b/emacs/notmuch-hello.el
> > @@ -85,6 +85,7 @@ (define-widget 'notmuch-saved-search-plist 'list
> >  		(group :format "%v" :inline t (const :format "  Query: " :query) (string :format "%v")))
> >  	  (checklist :inline t
> >  		     :format "%v"
> > +		     (group :format "%v" :inline t (const :format "Shortcut key: " :key) (key-sequence :format "%v"))
> >  		     (group :format "%v" :inline t (const :format "Count-Query: " :count-query) (string :format "%v"))
> >  		     (group :format "%v" :inline t (const :format "" :sort-order)
> >  			    (choice :tag " Sort Order"
> > @@ -101,6 +102,7 @@ (defcustom notmuch-saved-searches '((:name "inbox" :query "tag:inbox")
> >  
> >    :name            Name of the search (required).
> >    :query           Search to run (required).
> > +  :key             Optional shortcut key for `notmuch-jump-search'.
> >    :count-query     Optional extra query to generate the count
> >                     shown. If not present then the :query property
> >                     is used.
> > diff --git a/emacs/notmuch-jump.el b/emacs/notmuch-jump.el
> > new file mode 100644
> > index 0000000..cb1ae10
> > --- /dev/null
> > +++ b/emacs/notmuch-jump.el
> > @@ -0,0 +1,189 @@
> > +;; notmuch-jump.el --- User-friendly shortcut keys
> > +;;
> > +;; Copyright © Austin Clements
> > +;;
> > +;; This file is part of Notmuch.
> > +;;
> > +;; Notmuch is free software: you can redistribute it and/or modify it
> > +;; under the terms of the GNU General Public License as published by
> > +;; the Free Software Foundation, either version 3 of the License, or
> > +;; (at your option) any later version.
> > +;;
> > +;; Notmuch is distributed in the hope that it will be useful, but
> > +;; WITHOUT ANY WARRANTY; without even the implied warranty of
> > +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +;; General Public License for more details.
> > +;;
> > +;; You should have received a copy of the GNU General Public License
> > +;; along with Notmuch.  If not, see <http://www.gnu.org/licenses/>.
> > +;;
> > +;; Authors: Austin Clements <aclements at csail.mit.edu>
> > +;;          David Edmondson <dme at dme.org>
> > +
> > +(eval-when-compile (require 'cl))
> > +
> > +(require 'notmuch-hello)
> > +
> > +;;;###autoload
> > +(defun notmuch-jump-search ()
> > +  "Jump to a saved search by shortcut key.
> > +
> > +This prompts for and performs a saved search using the shortcut
> > +keys configured in the :key property of `notmuch-saved-searches'.
> > +Typically these shortcuts are a single key long, so this is a
> > +fast way to jump to a saved search from anywhere in Notmuch."
> > +  (interactive)
> > +
> > +  ;; Build the action map
> > +  (let (action-map)
> > +    (dolist (saved-search notmuch-saved-searches)
> > +      (let* ((saved-search (notmuch-hello-saved-search-to-plist saved-search))
> > +	     (key (plist-get saved-search :key)))
> > +	(when key
> > +	  (let ((name (plist-get saved-search :name))
> > +		(query (plist-get saved-search :query))
> > +		(oldest-first
> > +		 (case (plist-get saved-search :sort-order)
> 
> I would probably not do the saved-search-to-plist bit and just use
> notmuch-saved-search-get each time.

What's the downside of notmuch-hello-saved-search-to-plist?  Using
notmuch-saved-search-get everywhere is more verbose.

> > +		   (newest-first nil)
> > +		   (oldest-first t)
> > +		   (otherwise (default-value notmuch-search-oldest-first)))))
> > +	    (push (list key name
> > +			`(lambda () (notmuch-search ',query ',oldest-first)))
> > +		  action-map)))))
> > +    (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."))))
> 
> I would slightly rephrase the error: something more like "... To use
> notmuch-jump please customize notmuch-saved-searches."  So that the user
> who doesn't want to use notmuch-jump doesn't think they are at fault.

Good idea.

> > +(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.
> 
> Maybe say the prompt can be canceled with RET too?

Done.  I also fixed the docstring's mention of a pop-up temporary
buffer, since that's not how the code works any more.

> > +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.
> 
> I find the "j-" prompt a bit weird and would prefer something more like
> "Jump to search: " to be used however the user enters the function.

The intent was to make it act like a prefix keymap.  For example, if
you hit "C-x", Emacs will show "C-x-" in the minibuffer until you hit
the next key.  OTOH, Emacs isn't exactly a paragon of usability, so
you're probably right and this should just use the provided prompt
string regardless.

> > +ACTION-MAP must be a list of triples of the form
> > +  (KEY LABEL ACTION)
> > +where KEY is a key binding, LABEL is a string label to display in
> > +the buffer, and ACTION is a nullary function to call.  LABEL may
> > +be null, in which case the action will still be bound, but will
> > +not appear in the pop-up buffer.
> > +"
> > +
> > +  (let* ((items (notmuch-jump--format-actions action-map))
> > +	 ;; Format the table of bindings and the full prompt
> > +	 (table
> > +	  (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)))
> > +	 ;; 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
> > +	 ;; ourselves) from their labels, so disable the minibuffer's
> > +	 ;; own re-face-ing.
> > +	 (minibuffer-prompt-properties
> > +	  (notmuch-jump--plist-delete
> > +	   (copy-sequence minibuffer-prompt-properties)
> > +	   'face))
> > +	 ;; Build the keymap with our bindings
> > +	 (minibuffer-map (notmuch-jump--make-keymap action-map))
> > +	 ;; The bindings save the the action in notmuch-jump--action
> > +	 (notmuch-jump--action nil))
> > +    ;; Read the action
> > +    (read-from-minibuffer full-prompt nil minibuffer-map)
> > +
> > +    ;; If we got an action, do it
> > +    (when notmuch-jump--action
> > +      (funcall notmuch-jump--action))))
> > +
> > +(defun notmuch-jump--format-actions (action-map)
> > +  "Format the actions in ACTION-MAP.
> > +
> > +Returns a list of strings, one for each item with a label in
> > +ACTION-MAP.  These strings can be inserted into a tabular
> > +buffer."
> > +
> > +  ;; Compute the maximum key description width
> > +  (let ((key-width 1))
> > +    (dolist (action action-map)
> 
> The name "action" is slightly unfortunate when you use ACTION as the
> third item of each element of action-map when describing it
> above. However, no better name springs to mind. (Maybe "triple"?)

Good catch.  Changed to "entry" (for an entry in the action map).

> > +      (setq key-width
> > +	    (max key-width
> > +		 (string-width (format-kbd-macro (first action))))))
> > +    ;; Format each action
> > +    (mapcar (lambda (action)
> > +	      (let ((key (format-kbd-macro (first action)))
> > +		    (desc (second action)))
> > +		(concat
> > +		 (propertize key 'face 'minibuffer-prompt)
> > +		 (make-string (- key-width (length key)) ? )
> > +		 " " desc)))
> > +	    action-map)))
> > +
> > +(defun notmuch-jump--insert-items (width items)
> > +  "Make a table of ITEMS up to WIDTH wide in the current buffer."
> > +  (let* ((nitems (length items))
> > +	 (col-width (+ 3 (apply #'max (mapcar #'string-width items))))
> > +	 (ncols (if (> (* col-width nitems) width)
> > +		    (max 1 (/ width col-width))
> > +		  ;; Items fit on one line.  Space them out
> > +		  (setq col-width (/ width nitems))
> > +		  (length items))))
> > +    (while items
> > +      (dotimes (col ncols)
> > +	(when items
> > +	  (let ((item (pop items)))
> > +	    (insert item)
> > +	    (when (and items (< col (- ncols 1)))
> > +	      (insert (make-string (- col-width (string-width item)) ? ))))))
> > +      (when items
> > +	(insert "\n")))))
> > +
> > +(defvar notmuch-jump-minibuffer-map
> > +  (let ((map (make-sparse-keymap)))
> > +    (set-keymap-parent map minibuffer-local-map)
> > +    ;; Make this like a special-mode keymap, with no self-insert-command
> > +    (suppress-keymap map)
> > +    map)
> > +  "Base keymap for notmuch-jump's minibuffer keymap.")
> > +
> > +(defun notmuch-jump--make-keymap (action-map)
> > +  "Translate ACTION-MAP into a minibuffer keymap."
> > +  (let ((map (make-sparse-keymap)))
> > +    (set-keymap-parent map notmuch-jump-minibuffer-map)
> > +    (dolist (action action-map)
> > +      (define-key map (first action)
> > +	`(lambda () (interactive)
> > +	   (setq notmuch-jump--action ',(third action))
> > +	   (exit-minibuffer))))
> > +    map))
> > +
> > +(defun notmuch-jump--plist-delete (plist property)
> > +  (let* ((xplist (cons nil plist))
> > +	 (pred xplist))
> > +    (while (cdr pred)
> > +      (when (eq (cadr pred) property)
> > +	(setcdr pred (cdddr pred)))
> > +      (setq pred (cddr pred)))
> > +    (cdr xplist)))
> > +
> > +(unless (fboundp 'window-body-width)
> > +  ;; Compatibility for Emacs pre-24
> > +  (defun window-body-width (&optional window)
> > +    (let ((edges (window-inside-edges window)))
> > +      (- (caddr edges) (car edges)))))
> > diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> > index 2941da3..135422d 100644
> > --- a/emacs/notmuch-lib.el
> > +++ b/emacs/notmuch-lib.el
> > @@ -130,9 +130,12 @@ (defvar notmuch-common-keymap
> >      (define-key map "m" 'notmuch-mua-new-mail)
> >      (define-key map "=" 'notmuch-refresh-this-buffer)
> >      (define-key map "G" 'notmuch-poll-and-refresh-this-buffer)
> > +    (define-key map "j" 'notmuch-jump-search)
> >      map)
> >    "Keymap shared by all notmuch modes.")
> >  
> > +(autoload 'notmuch-jump-search "notmuch-jump" "Jump to a saved search by shortcut key." t)
> 
> We don't normally seem to use autoload but instead use
> declare-function. It might be worth being consistent (I am not very sure
> of the pros and cons of autoload). May also be worth having it with the
> other declare-functions to keep it clear how things are loaded.

I used autoload because it won't bother even reading
notmuch-jump.el(c) until the user hits "j" for the first time (and,
hence, won't load it at all if they don't use jump).  This is easy
with notmuch-jump because it's self-contained and has a single clear
entry-point and no customizable variables.

declare-function, on the other hand, still requires you to load the
source containing the function (either eagerly, which is what notmuch
usually does, or with an autoload).

I think we should do *more* autoloading, though maybe it's not
practical for the bulk of notmuch-emacs.

I'm happy to move the autoload call to somewhere else, though
notmuch-lib doesn't actually have any declare-functions (since it's
sort of a root of the dependency tree).  I could put it right below
the requires, since that's where we usually put declare-functions.

> Best wishes
> 
> Mark
> 
> 
> > +
> >  ;; 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