From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to saved
	searches
In-Reply-To: <1405353735-26244-2-git-send-email-amdragon@mit.edu>
References: <1405353735-26244-1-git-send-email-amdragon@mit.edu>
	<1405353735-26244-2-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: Mon, 14 Jul 2014 22:22:05 +0100
Message-ID: <87egxnd4aq.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list


On Mon, 14 Jul 2014, Austin Clements <amdragon@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 :=3D \
>  	$(dir)/notmuch-tag.el \
>  	$(dir)/coolj.el \
>  	$(dir)/notmuch-print.el \
> -	$(dir)/notmuch-version.el
> +	$(dir)/notmuch-version.el \
> +	$(dir)/notmuch-jump.el \
>=20=20
>  $(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) (stri=
ng :format "%v")))
>  	  (checklist :inline t
>  		     :format "%v"
> +		     (group :format "%v" :inline t (const :format "Shortcut key: " :ke=
y) (key-sequence :format "%v"))
>  		     (group :format "%v" :inline t (const :format "Count-Query: " :cou=
nt-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" :q=
uery "tag:inbox")
>=20=20
>    :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 =C2=A9 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@csail.mit.edu>
> +;;          David Edmondson <dme@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-se=
arch))
> +	     (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.

> +		   (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 not=
much-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.

> +(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?

> +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.

> +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"?)

> +      (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 "=3D" '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.")
>=20=20
> +(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.

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
> --=20
> 2.0.0