From: Austin Clements Date: Tue, 15 Jul 2014 03:46:45 +0000 (+2000) Subject: Re: [PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to saved searches X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=0f8af615626d362a02a03b7d654d0b901a8ac8e7;p=notmuch-archives.git Re: [PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to saved searches --- diff --git a/0a/1c39492c1e288ae47d0cd0d119fe1bae589ed8 b/0a/1c39492c1e288ae47d0cd0d119fe1bae589ed8 new file mode 100644 index 000000000..d2ffed16e --- /dev/null +++ b/0a/1c39492c1e288ae47d0cd0d119fe1bae589ed8 @@ -0,0 +1,425 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 2062C431FC4 + for ; Mon, 14 Jul 2014 20:46:59 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id Bk-mPmHeZqu4 for ; + Mon, 14 Jul 2014 20:46:50 -0700 (PDT) +Received: from dmz-mailsec-scanner-2.mit.edu (dmz-mailsec-scanner-2.mit.edu + [18.9.25.13]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 34598431FBC + for ; Mon, 14 Jul 2014 20:46:50 -0700 (PDT) +X-AuditID: 1209190d-f79c06d000002f07-43-53c4a429729b +Received: from mailhub-auth-3.mit.edu ( [18.9.21.43]) + (using TLS with cipher AES256-SHA (256/256 bits)) + (Client did not present a certificate) + by dmz-mailsec-scanner-2.mit.edu (Symantec Messaging Gateway) with SMTP + id 13.B9.12039.924A4C35; Mon, 14 Jul 2014 23:46:49 -0400 (EDT) +Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) + by mailhub-auth-3.mit.edu (8.13.8/8.9.2) with ESMTP id s6F3klvJ004257; + Mon, 14 Jul 2014 23:46:48 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id s6F3kjEY026768 + (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); + Mon, 14 Jul 2014 23:46:46 -0400 +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) + (envelope-from ) + id 1X6thV-0005Vg-4p; Mon, 14 Jul 2014 23:46:45 -0400 +Date: Mon, 14 Jul 2014 23:46:45 -0400 +From: Austin Clements +To: Mark Walters +Subject: Re: [PATCH 1/2] emacs: Introduce notmuch-jump: shortcut keys to + saved searches +Message-ID: <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> +MIME-Version: 1.0 +Content-Type: text/plain; charset=iso-8859-1 +Content-Disposition: inline +Content-Transfer-Encoding: 8bit +In-Reply-To: <87egxnd4aq.fsf@qmul.ac.uk> +User-Agent: Mutt/1.5.21 (2010-09-15) +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFtrFKsWRmVeSWpSXmKPExsUixCmqrau55EiwwctfChY3WrsZLfbd2cJk + sXouj8X1mzOZHVg8dj3/y+Sxc9Zddo9nq24xe2w59J45gCWKyyYlNSezLLVI3y6BK+PV7aCC + 5qKK1w0LmRsYV0V0MXJySAiYSBw/1MUMYYtJXLi3nq2LkYtDSGA2k8T+f5fYQBJCAhsZJX7/ + 4YRInGaSOPRkPyOEs4RR4mDPBLB2FgFVifvbnzCC2GwCGhLb9i8Hs0UEdCRuH1rADmIzC1hL + vFv1DywuLBAp8fn1VVYQm1dAW2L3rrlQq6cySpz8u5YNIiEocXLmExaIZh2JnVvvAMU5gGxp + ieX/OCDC8hLNW2eD3cAJtPdN41Ow+aICKhJTTm5jm8AoPAvJpFlIJs1CmDQLyaQFjCyrGGVT + cqt0cxMzc4pTk3WLkxPz8lKLdI30cjNL9FJTSjcxgqNFkncH47uDSocYBTgYlXh4T3w6HCzE + mlhWXJl7iFGSg0lJlLdg6pFgIb6k/JTKjMTijPii0pzU4kOMEhzMSiK8HouBcrwpiZVVqUX5 + MClpDhYlcd631lbBQgLpiSWp2ampBalFMFkZDg4lCV5VkEbBotT01Iq0zJwShDQTByfIcB6g + 4dPBhhcXJOYWZ6ZD5E8x6nJ0XT/WxiTEkpeflyolzvt1EVCRAEhRRmke3BxYknvFKA70ljBv + BMgoHmCChJv0CmgJE9CS8prDIEtKEhFSUg2MZeeObpp8bFf/7FuuD0+LbI2xubnLjWcfU3f7 + lY/7Hk5ddKx0/7/vMY/8a6JeLBTr4FxvJWB8P5XHT0lrwuzvjIE10SyK0fw/3E0n1Wd/Xv7T + +32ETELVvQZ3jmzuVcdrLu6dFLXyz9Xl1k/ymblrFkhIOrg9VPi999r1Z34zhB7LrPJ7zBUv + qsRSnJFoqMVcVJwIADugN4dNAwAA +Cc: notmuch@notmuchmail.org +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Tue, 15 Jul 2014 03:46:59 -0000 + +Quoth Mark Walters on Jul 14 at 10:22 pm: +> +> On Mon, 14 Jul 2014, Austin Clements 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 . +> > +;; +> > +;; Authors: Austin Clements +> > +;; David Edmondson +> > + +> > +(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