From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to saved
	searches
In-Reply-To: <20140715034645.GE4660@mit.edu>
References: <1405353735-26244-1-git-send-email-amdragon@mit.edu>
	<1405353735-26244-2-git-send-email-amdragon@mit.edu>
	<87egxnd4aq.fsf@qmul.ac.uk> <20140715034645.GE4660@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 08:02:28 +0100
Message-ID: <8738e3xfy3.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
Precedence: list

On Tue, 15 Jul 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Jul 14 at 10:22 pm:
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> > 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) (s=
tring :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")
>> >=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=
-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)
>>=20
>> 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.

The only downside is that I sort of intended
notmuch-hello-saved-search-to-list to be a private function (I am not
sure why I gave notmuch-hello--saved-searches-to-plist the double dash
bit not this). But I am definitely happy with it as is: I wasn't sure
whether to make any comment when first reviewing.

>> > +		   (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."))))
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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)
>>=20
>> 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-comm=
and
>> > +    (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)
>>=20
>> 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.

I think having the autoload  at the top sounds good. With the changes
you suggest it gets my +1.

Best wishes

Mark


>
>> Best wishes
>>=20
>> Mark
>>=20
>>=20
>> > +
>> >  ;; 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