[PATCH 00/11] contrib: pick: keybindings

[PATCH 00/11] contrib: pick: keybindings

[Mark Walters]

This series adds lots of keybindings and funtionality to pick: in
particular it adds the stash keymap, the ability to tab between and
activate buttons in the message pane, and it reduces a lot of code
duplication between pick and show.

It is a large series: but most of it is a lot of small changes. These
small changes are mostly logically independent but as they add
keybindings their contexts all clash. I have made most of the
keybindings as single separate patches to make discussion of them
individually easier.

The key patches for review/discussion (apart from bike-shedding on
key-bindings!) are patches 1, 5 and 8.

Patch 1 is the most "controversial": it over-rides
notmuch-show-get-prop so that whether it uses
notmuch-show-get-message-properties or
notmuch-pick-get-message-properties depends on the major-mode (ie
whether it is called from pick or show).

This means that functions from show which just use message properties
(most often just the message id) "just work" when called from pick. In
particular it gives us access to lots of functions without having to
duplicate the code.

In the longer term it would be better to have some show/pick common
file and migrate the common functions there.

Patch 5 and 8 add in functions for creating fucntions ready to be used
in keybindings. The one in patch 5 takes a show fucntion and creates a
function which switches from pick to the message pane, applies the
function and then switches back to pick. The one in patch 8 takes a
function and creates a function which closes the message pane and the
calls this function.

Both of these make the keybinding section clearer. They also have the
advantage that the user can use them easily to create their own
keybindings which do this.

This completes all the keybindings I use and I think means that pick
doesn't have any glaring omissions.

Finally, this will clash with the thread archive patches
id:1371195472-441-1-git-send-email-markwalters1009@gmail.com

Best wishes

Mark

Mark Walters (11):
  contrib: pick: override notmuch-show-get-prop
  contrib: pick: Link in notmuch-show-pipe-message
  contrib: pick: Link in attachment functions straight from
    notmuch-show
  contrib: pick: Link in stash map straight from notmuch-show
  contrib: pick: add in to-message-window function
  contrib: pick: add button press helper
  contrib: pick: pass tab through to the message pane
  contrib: pick: close window function
  contrib: pick: make help close the message pane first
  contrib: pick: add in binding to view raw message
  contrib: pick: use close-message-pane for reply etc

 contrib/notmuch-pick/notmuch-pick.el |  139 +++++++++++++++++-----------------
 1 files changed, 70 insertions(+), 69 deletions(-)

-- 
1.7.9.1

[PATCH 01/11] contrib: pick: override notmuch-show-get-prop

[Mark Walters]

We override notmuch-show-get-prop so that many of the show functions
can be used in notmuch-pick without modification. The main use is that
it means notmuch-show-get-message-id `works' in pick. Thus we get all
the stash functions and several other `for free' in pick.
---
 contrib/notmuch-pick/notmuch-pick.el |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index fbd7c0b..d07f393 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -238,6 +238,22 @@ Some useful entries are:
     (beginning-of-line)
     (get-text-property (point) :notmuch-message-properties)))
 
+;; This should really be a lib function but we are trying to reduce
+;; impact on the code base.
+(defun notmuch-show-get-prop (prop &optional props)
+  "This is a pick overridden version of notmuch-show-get-prop
+
+It gets property PROP from PROPS or, if PROPS is nil, the current
+message in either pick or show. This means that several functions
+in notmuch-show now work unchanged in pick as they just need the
+correct message properties."
+  (let ((props (or props
+		   (cond ((eq major-mode 'notmuch-show-mode)
+			  (notmuch-show-get-message-properties))
+			 ((eq major-mode 'notmuch-pick-mode)
+			  (notmuch-pick-get-message-properties))))))
+    (plist-get props prop)))
+
 (defun notmuch-pick-set-message-properties (props)
   (save-excursion
     (beginning-of-line)
-- 
1.7.9.1

[PATCH 02/11] contrib: pick: Link in notmuch-show-pipe-message

[Mark Walters]

Since we can now use show functions directly in pick we can drop pick-pipe-message.
---
 contrib/notmuch-pick/notmuch-pick.el |   33 ++++-----------------------------
 1 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index d07f393..08bdc11 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -190,6 +190,10 @@ if the user has loaded a different buffer in that window.")
 (defvar notmuch-pick-mode-map
   (let ((map (make-sparse-keymap)))
     (define-key map [mouse-1] 'notmuch-pick-show-message)
+    ;; these use notmuch-show functions directly
+    (define-key map "|" 'notmuch-show-pipe-message)
+
+    ;; The main pick bindings
     (define-key map "q" 'notmuch-pick-quit)
     (define-key map "x" 'notmuch-pick-quit)
     (define-key map "?" 'notmuch-help)
@@ -205,7 +209,6 @@ if the user has loaded a different buffer in that window.")
     (define-key map "p" 'notmuch-pick-prev-matching-message)
     (define-key map "N" 'notmuch-pick-next-message)
     (define-key map "P" 'notmuch-pick-prev-message)
-    (define-key map "|" 'notmuch-pick-pipe-message)
     (define-key map "-" 'notmuch-pick-remove-tag)
     (define-key map "+" 'notmuch-pick-add-tag)
     (define-key map " " 'notmuch-pick-scroll-or-next)
@@ -586,34 +589,6 @@ message will be \"unarchived\", i.e. the tag changes in
   (notmuch-pick-close-message-window)
   (notmuch-mua-new-reply (notmuch-pick-get-message-id) prompt-for-sender nil))
 
-;; Shamelessly stolen from notmuch-show.el: maybe should be unified.
-(defun notmuch-pick-pipe-message (command)
-  "Pipe the contents of the current message to the given command.
-
-The given command will be executed with the raw contents of the
-current email message as stdin. Anything printed by the command
-to stdout or stderr will appear in the *notmuch-pipe* buffer.
-
-When invoked with a prefix argument, the command will receive all
-open messages in the current thread (formatted as an mbox) rather
-than only the current message."
-  (interactive "sPipe message to command: ")
-  (let ((shell-command
-	 (concat notmuch-command " show --format=raw "
-		 (shell-quote-argument (notmuch-pick-get-message-id)) " | " command))
-	 (buf (get-buffer-create (concat "*notmuch-pipe*"))))
-    (with-current-buffer buf
-      (setq buffer-read-only nil)
-      (erase-buffer)
-      (let ((exit-code (call-process-shell-command shell-command nil buf)))
-	(goto-char (point-max))
-	(set-buffer-modified-p nil)
-	(setq buffer-read-only t)
-	(unless (zerop exit-code)
-	  (switch-to-buffer-other-window buf)
-	  (message (format "Command '%s' exited abnormally with code %d"
-			   shell-command exit-code)))))))
-
 (defun notmuch-pick-clean-address (address)
   "Try to clean a single email ADDRESS for display. Return
 AUTHOR_NAME if present, otherwise return AUTHOR_EMAIL. Return
-- 
1.7.9.1

[PATCH 03/11] contrib: pick: Link in attachment functions straight from notmuch-show

[Mark Walters]

We can use the attachment functions straight from
notmuch-show. notmuch-show-view-all-mime-parts might be deprecated so
we either want to undeprecate it or not have this binding.
---
 contrib/notmuch-pick/notmuch-pick.el |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 08bdc11..62c4ffc 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -192,6 +192,8 @@ if the user has loaded a different buffer in that window.")
     (define-key map [mouse-1] 'notmuch-pick-show-message)
     ;; these use notmuch-show functions directly
     (define-key map "|" 'notmuch-show-pipe-message)
+    (define-key map "w" 'notmuch-show-save-attachments)
+    (define-key map "v" 'notmuch-show-view-all-mime-parts)
 
     ;; The main pick bindings
     (define-key map "q" 'notmuch-pick-quit)
-- 
1.7.9.1

[PATCH 04/11] contrib: pick: Link in stash map straight from notmuch-show

[Mark Walters]

These functions all now work straight from their notmuch-show
implementation so link them in.

Stash functionality was one of the key missing things in notmuch-pick.
---
 contrib/notmuch-pick/notmuch-pick.el |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 62c4ffc..7313100 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -194,6 +194,7 @@ if the user has loaded a different buffer in that window.")
     (define-key map "|" 'notmuch-show-pipe-message)
     (define-key map "w" 'notmuch-show-save-attachments)
     (define-key map "v" 'notmuch-show-view-all-mime-parts)
+    (define-key map "c" 'notmuch-show-stash-map)
 
     ;; The main pick bindings
     (define-key map "q" 'notmuch-pick-quit)
-- 
1.7.9.1

[PATCH 05/11] contrib: pick: add in to-message-window function

[Mark Walters]

This would be very simple but we want to get a useful doc string (so
we get useful help messages).

We use this for tabbing between and activate buttons in the message
pane but it is trivial for pick or the user to link in other
functions. For example

(define-key map "h" (notmuch-pick-to-message-pane #'notmuch-show-toggle-visibility-headers))

Other plausible functions for linking are

notmuch-show-toggle-elide-non-matching
notmuch-show-toggle-process-crypto
notmuch-show-toggle-thread-indentation
toggle-truncate-lines
---
 contrib/notmuch-pick/notmuch-pick.el |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 7313100..8251b35 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -187,6 +187,19 @@ if the user has loaded a different buffer in that window.")
 (make-variable-buffer-local 'notmuch-pick-message-buffer)
 (put 'notmuch-pick-message-buffer 'permanent-local t)
 
+(defun notmuch-pick-to-message-pane (func)
+  "Execute FUNC in message pane.
+
+This function returns a function (so can be used as a keybinding)
+which executes function FUNC in the message pane if it is
+open (if the message pane is closed it does nothing)."
+  `(lambda ()
+      ,(concat "(In message pane) " (documentation func t))
+     (interactive)
+     (when (window-live-p notmuch-pick-message-window)
+       (with-selected-window notmuch-pick-message-window
+	 (funcall #',func)))))
+
 (defvar notmuch-pick-mode-map
   (let ((map (make-sparse-keymap)))
     (define-key map [mouse-1] 'notmuch-pick-show-message)
-- 
1.7.9.1

[PATCH 06/11] contrib: pick: add button press helper

[Mark Walters]

We will want to be able to activate buttons not in the current
buffer (ie in the message pane) so it is helpful to have a way of
activating a button without signalling error if there is no button.
---
 contrib/notmuch-pick/notmuch-pick.el |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 8251b35..775b783 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -200,6 +200,14 @@ open (if the message pane is closed it does nothing)."
        (with-selected-window notmuch-pick-message-window
 	 (funcall #',func)))))
 
+(defun notmuch-pick-button-activate (&optional button)
+  "Activate BUTTON or button at point
+
+This function does not give an error if there is no button."
+  (interactive)
+  (let ((button (or button (button-at (point)))))
+    (when button (button-activate button))))
+
 (defvar notmuch-pick-mode-map
   (let ((map (make-sparse-keymap)))
     (define-key map [mouse-1] 'notmuch-pick-show-message)
-- 
1.7.9.1

[PATCH 07/11] contrib: pick: pass tab through to the message pane

[Mark Walters]

This makes tab move to next button in the message pane and binds
button activate (in message pane) to "/". This means that is easy to
toggle hidden parts or hidden citations etc in the message pane.
---
 contrib/notmuch-pick/notmuch-pick.el |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 775b783..77b257d 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -217,6 +217,12 @@ This function does not give an error if there is no button."
     (define-key map "v" 'notmuch-show-view-all-mime-parts)
     (define-key map "c" 'notmuch-show-stash-map)
 
+    ;; these apply to the message pane
+    (define-key map (kbd "M-TAB") (notmuch-pick-to-message-pane #'notmuch-show-previous-button))
+    (define-key map (kbd "<backtab>")  (notmuch-pick-to-message-pane #'notmuch-show-previous-button))
+    (define-key map (kbd "TAB") (notmuch-pick-to-message-pane #'notmuch-show-next-button))
+    (define-key map "/" (notmuch-pick-to-message-pane #'notmuch-pick-button-activate))
+
     ;; The main pick bindings
     (define-key map "q" 'notmuch-pick-quit)
     (define-key map "x" 'notmuch-pick-quit)
-- 
1.7.9.1

[PATCH 08/11] contrib: pick: close window function

[Mark Walters]

---
 contrib/notmuch-pick/notmuch-pick.el |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 77b257d..4ad6ef6 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -208,6 +208,18 @@ This function does not give an error if there is no button."
   (let ((button (or button (button-at (point)))))
     (when button (button-activate button))))
 
+(defun notmuch-pick-close-message-pane-and (func)
+  "Close message pane and execute FUNC.
+
+This function returns a function (so can be used as a keybinding)
+which closes the message pane if open and then executes function
+FUNC."
+  `(lambda ()
+      ,(concat "(Close message pane and) " (documentation func t))
+     (interactive)
+     (notmuch-pick-close-message-window)
+     (funcall #',func)))
+
 (defvar notmuch-pick-mode-map
   (let ((map (make-sparse-keymap)))
     (define-key map [mouse-1] 'notmuch-pick-show-message)
-- 
1.7.9.1

[PATCH 09/11] contrib: pick: make help close the message pane first

[Mark Walters]

Previously pressing "?" for help when the message pane was open meant
the help window was very small. Close the message pane before
displaying help.
---
 contrib/notmuch-pick/notmuch-pick.el |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 4ad6ef6..c3c8b20 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -235,10 +235,12 @@ FUNC."
     (define-key map (kbd "TAB") (notmuch-pick-to-message-pane #'notmuch-show-next-button))
     (define-key map "/" (notmuch-pick-to-message-pane #'notmuch-pick-button-activate))
 
+    ;; bindings from show (or elsewhere) but we close the message pane first.
+    (define-key map "?" (notmuch-pick-close-message-pane-and #'notmuch-help))
+
     ;; The main pick bindings
     (define-key map "q" 'notmuch-pick-quit)
     (define-key map "x" 'notmuch-pick-quit)
-    (define-key map "?" 'notmuch-help)
     (define-key map "a" 'notmuch-pick-archive-message-then-next)
     (define-key map "=" 'notmuch-pick-refresh-view)
     (define-key map "s" 'notmuch-pick-to-search)
-- 
1.7.9.1

[PATCH 10/11] contrib: pick: add in binding to view raw message

[Mark Walters]

Note this does rely on the fact that we have over-ridden notmuch-show-get-properties
---
 contrib/notmuch-pick/notmuch-pick.el |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index c3c8b20..15c7e74 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -236,6 +236,7 @@ FUNC."
     (define-key map "/" (notmuch-pick-to-message-pane #'notmuch-pick-button-activate))
 
     ;; bindings from show (or elsewhere) but we close the message pane first.
+    (define-key map "V" (notmuch-pick-close-message-pane-and #'notmuch-show-view-raw-message))
     (define-key map "?" (notmuch-pick-close-message-pane-and #'notmuch-help))
 
     ;; The main pick bindings
-- 
1.7.9.1

[PATCH 11/11] contrib: pick: use close-message-pane for reply etc

[Mark Walters]

We can save some code duplication by using the new close-message-pane
functionality for reply, forward, and new mail.
---
 contrib/notmuch-pick/notmuch-pick.el |   43 +++------------------------------
 1 files changed, 4 insertions(+), 39 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 15c7e74..9587dda 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -236,6 +236,10 @@ FUNC."
     (define-key map "/" (notmuch-pick-to-message-pane #'notmuch-pick-button-activate))
 
     ;; bindings from show (or elsewhere) but we close the message pane first.
+    (define-key map "m" (notmuch-pick-close-message-pane-and #'notmuch-mua-new-mail))
+    (define-key map "f" (notmuch-pick-close-message-pane-and #'notmuch-show-forward-message))
+    (define-key map "r" (notmuch-pick-close-message-pane-and #'notmuch-show-reply-sender))
+    (define-key map "R" (notmuch-pick-close-message-pane-and #'notmuch-show-reply))
     (define-key map "V" (notmuch-pick-close-message-pane-and #'notmuch-show-view-raw-message))
     (define-key map "?" (notmuch-pick-close-message-pane-and #'notmuch-help))
 
@@ -246,10 +250,6 @@ FUNC."
     (define-key map "=" 'notmuch-pick-refresh-view)
     (define-key map "s" 'notmuch-pick-to-search)
     (define-key map "z" 'notmuch-pick-to-pick)
-    (define-key map "m" 'notmuch-pick-new-mail)
-    (define-key map "f" 'notmuch-pick-forward-message)
-    (define-key map "r" 'notmuch-pick-reply-sender)
-    (define-key map "R" 'notmuch-pick-reply)
     (define-key map "n" 'notmuch-pick-next-matching-message)
     (define-key map "p" 'notmuch-pick-prev-matching-message)
     (define-key map "N" 'notmuch-pick-next-message)
@@ -599,41 +599,6 @@ message will be \"unarchived\", i.e. the tag changes in
 			 target
 			 (get-buffer buffer-name))))
 
-(defmacro with-current-notmuch-pick-message (&rest body)
-  "Evaluate body with current buffer set to the text of current message"
-  `(save-excursion
-     (let ((id (notmuch-pick-get-message-id)))
-       (let ((buf (generate-new-buffer (concat "*notmuch-msg-" id "*"))))
-         (with-current-buffer buf
-	    (call-process notmuch-command nil t nil "show" "--format=raw" id)
-           ,@body)
-	 (kill-buffer buf)))))
-
-(defun notmuch-pick-new-mail (&optional prompt-for-sender)
-  "Compose new mail."
-  (interactive "P")
-  (notmuch-pick-close-message-window)
-  (notmuch-mua-new-mail prompt-for-sender ))
-
-(defun notmuch-pick-forward-message (&optional prompt-for-sender)
-  "Forward the current message."
-  (interactive "P")
-  (notmuch-pick-close-message-window)
-  (with-current-notmuch-pick-message
-   (notmuch-mua-new-forward-message prompt-for-sender)))
-
-(defun notmuch-pick-reply (&optional prompt-for-sender)
-  "Reply to the sender and all recipients of the current message."
-  (interactive "P")
-  (notmuch-pick-close-message-window)
-  (notmuch-mua-new-reply (notmuch-pick-get-message-id) prompt-for-sender t))
-
-(defun notmuch-pick-reply-sender (&optional prompt-for-sender)
-  "Reply to the sender of the current message."
-  (interactive "P")
-  (notmuch-pick-close-message-window)
-  (notmuch-mua-new-reply (notmuch-pick-get-message-id) prompt-for-sender nil))
-
 (defun notmuch-pick-clean-address (address)
   "Try to clean a single email ADDRESS for display. Return
 AUTHOR_NAME if present, otherwise return AUTHOR_EMAIL. Return
-- 
1.7.9.1

Re: [PATCH 00/11] contrib: pick: keybindings

[Tomi Ollila]

On Fri, Jul 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> This series adds lots of keybindings and funtionality to pick: in
> particular it adds the stash keymap, the ability to tab between and
> activate buttons in the message pane, and it reduces a lot of code
> duplication between pick and show.
>
> It is a large series: but most of it is a lot of small changes. These
> small changes are mostly logically independent but as they add
> keybindings their contexts all clash. I have made most of the
> keybindings as single separate patches to make discussion of them
> individually easier.
>
> The key patches for review/discussion (apart from bike-shedding on
> key-bindings!) are patches 1, 5 and 8.

Now that you "asked", I'd ask what was used as a reason to decide "/"
as keybinding for notmuch-pick-button-activate -- something common
or as a convenience. In US/UK keyboard "/" is left to Right Shift,
but for example in my keyboard it is Shift-7...

> Patch 1 is the most "controversial": it over-rides
> notmuch-show-get-prop so that whether it uses
> notmuch-show-get-message-properties or
> notmuch-pick-get-message-properties depends on the major-mode (ie
> whether it is called from pick or show).

At the moment the code comments could have something like XXX to
emphasize the situation. IMHO it is ok to have this in contrib
code -- when this is going to be "official" part of emacs MUA
then this code needs to be ... well, at least moved to other place :D

Apart from these code LGTM.

Tomi


> This means that functions from show which just use message properties
> (most often just the message id) "just work" when called from pick. In
> particular it gives us access to lots of functions without having to
> duplicate the code.
>
> In the longer term it would be better to have some show/pick common
> file and migrate the common functions there.
>
> Patch 5 and 8 add in functions for creating fucntions ready to be used
> in keybindings. The one in patch 5 takes a show fucntion and creates a
> function which switches from pick to the message pane, applies the
> function and then switches back to pick. The one in patch 8 takes a
> function and creates a function which closes the message pane and the
> calls this function.
>
> Both of these make the keybinding section clearer. They also have the
> advantage that the user can use them easily to create their own
> keybindings which do this.
>
> This completes all the keybindings I use and I think means that pick
> doesn't have any glaring omissions.
>
> Finally, this will clash with the thread archive patches
> id:1371195472-441-1-git-send-email-markwalters1009@gmail.com
>
> Best wishes
>
> Mark
>
> Mark Walters (11):
>   contrib: pick: override notmuch-show-get-prop
>   contrib: pick: Link in notmuch-show-pipe-message
>   contrib: pick: Link in attachment functions straight from
>     notmuch-show
>   contrib: pick: Link in stash map straight from notmuch-show
>   contrib: pick: add in to-message-window function
>   contrib: pick: add button press helper
>   contrib: pick: pass tab through to the message pane
>   contrib: pick: close window function
>   contrib: pick: make help close the message pane first
>   contrib: pick: add in binding to view raw message
>   contrib: pick: use close-message-pane for reply etc
>
>  contrib/notmuch-pick/notmuch-pick.el |  139 +++++++++++++++++-----------------
>  1 files changed, 70 insertions(+), 69 deletions(-)
>
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 00/11] contrib: pick: keybindings

[Mark Walters]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Fri, Jul 05 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>
>> This series adds lots of keybindings and funtionality to pick: in
>> particular it adds the stash keymap, the ability to tab between and
>> activate buttons in the message pane, and it reduces a lot of code
>> duplication between pick and show.
>>
>> It is a large series: but most of it is a lot of small changes. These
>> small changes are mostly logically independent but as they add
>> keybindings their contexts all clash. I have made most of the
>> keybindings as single separate patches to make discussion of them
>> individually easier.
>>
>> The key patches for review/discussion (apart from bike-shedding on
>> key-bindings!) are patches 1, 5 and 8.

Hi 

Thanks for the review.

> Now that you "asked", I'd ask what was used as a reason to decide "/"
> as keybinding for notmuch-pick-button-activate -- something common
> or as a convenience. In US/UK keyboard "/" is left to Right Shift,
> but for example in my keyboard it is Shift-7...

That is just my not knowing (and I regret to say even thinking of) other
keyboard layouts. I agree that makes it a silly key. What about 'e'
(Enter as the mnemonic)?
>
>> Patch 1 is the most "controversial": it over-rides
>> notmuch-show-get-prop so that whether it uses
>> notmuch-show-get-message-properties or
>> notmuch-pick-get-message-properties depends on the major-mode (ie
>> whether it is called from pick or show).
>
> At the moment the code comments could have something like XXX to
> emphasize the situation. IMHO it is ok to have this in contrib
> code -- when this is going to be "official" part of emacs MUA
> then this code needs to be ... well, at least moved to other place :D

Yes that is a good point. I will add this in the next version.

Best wishes

Mark


> Apart from these code LGTM.
>
> Tomi
>
>
>> This means that functions from show which just use message properties
>> (most often just the message id) "just work" when called from pick. In
>> particular it gives us access to lots of functions without having to
>> duplicate the code.
>>
>> In the longer term it would be better to have some show/pick common
>> file and migrate the common functions there.
>>
>> Patch 5 and 8 add in functions for creating fucntions ready to be used
>> in keybindings. The one in patch 5 takes a show fucntion and creates a
>> function which switches from pick to the message pane, applies the
>> function and then switches back to pick. The one in patch 8 takes a
>> function and creates a function which closes the message pane and the
>> calls this function.
>>
>> Both of these make the keybinding section clearer. They also have the
>> advantage that the user can use them easily to create their own
>> keybindings which do this.
>>
>> This completes all the keybindings I use and I think means that pick
>> doesn't have any glaring omissions.
>>
>> Finally, this will clash with the thread archive patches
>> id:1371195472-441-1-git-send-email-markwalters1009@gmail.com
>>
>> Best wishes
>>
>> Mark
>>
>> Mark Walters (11):
>>   contrib: pick: override notmuch-show-get-prop
>>   contrib: pick: Link in notmuch-show-pipe-message
>>   contrib: pick: Link in attachment functions straight from
>>     notmuch-show
>>   contrib: pick: Link in stash map straight from notmuch-show
>>   contrib: pick: add in to-message-window function
>>   contrib: pick: add button press helper
>>   contrib: pick: pass tab through to the message pane
>>   contrib: pick: close window function
>>   contrib: pick: make help close the message pane first
>>   contrib: pick: add in binding to view raw message
>>   contrib: pick: use close-message-pane for reply etc
>>
>>  contrib/notmuch-pick/notmuch-pick.el |  139 +++++++++++++++++-----------------
>>  1 files changed, 70 insertions(+), 69 deletions(-)
>>
>> -- 
>> 1.7.9.1
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 00/11] contrib: pick: keybindings

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> This series adds lots of keybindings and funtionality to pick: in
> particular it adds the stash keymap, the ability to tab between and
> activate buttons in the message pane, and it reduces a lot of code
> duplication between pick and show.

pushed,

d