[PATCH v2 0/3] Contrib: Pick: Remove horrible hack

[PATCH v2 0/3] Contrib: Pick: Remove horrible hack

[Mark Walters]

This is a trivial rebase of
id:1369551008-30697-1-git-send-email-markwalters1009@gmail.com to
current master.  I have included the dependent patch
id:1369550458-30562-1-git-send-email-markwalters1009@gmail.com

As I said in id:87sj00xapn.fsf@qmul.ac.uk this removes the worst piece
of code in pick (a sleep loop while waiting for the correct message to arrive)

Although the diffstat is relatively large

 1 files changed, 4 insertions(+), 2 deletions(-)
 1 files changed, 12 insertions(+), 25 deletions(-)
 1 files changed, 17 insertions(+), 6 deletions(-)

for the each patch the final one is only 3 lines of code changed.

Many thanks

Mark




Mark Walters (3):
  contrib: pick: if no target specified go to first matching message
  contrib: pick: remove hack notmuch-pick-show-match-message-with-wait
  contrib: pick: fix refresh result

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

-- 
1.7.9.1

[PATCH v2 1/3] contrib: pick: if no target specified go to first matching message

[Mark Walters]

---
 contrib/notmuch-pick/notmuch-pick.el |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 16f8d15..ef16ca7 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -652,8 +652,10 @@ unchanged ADDRESS if parsing fails."
   (save-excursion
     (goto-char (point-max))
     (notmuch-pick-insert-msg msg))
-  (let ((msg-id (notmuch-id-to-query (plist-get msg :id))))
-    (when (string= msg-id notmuch-pick-target-msg)
+  (let ((msg-id (notmuch-id-to-query (plist-get msg :id)))
+	(target notmuch-pick-target-msg))
+    (when (or (and (not target) (plist-get msg :match))
+	      (string= msg-id target))
       (setq notmuch-pick-target-msg "found")
       (goto-char (point-max))
       (forward-line -1))))
-- 
1.7.9.1

Re: [PATCH v2 1/3] contrib: pick: if no target specified go to first matching message

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:
> -  (let ((msg-id (notmuch-id-to-query (plist-get msg :id))))
> -    (when (string= msg-id notmuch-pick-target-msg)
> +  (let ((msg-id (notmuch-id-to-query (plist-get msg :id)))
> +	(target notmuch-pick-target-msg))
> +    (when (or (and (not target) (plist-get msg :match))
> +	      (string= msg-id target))

I can't really figure out from looking at the code what this 'target'
business is about. Possibly unrelated to this patch, but maybe some
documentation is needed?

d

[PATCH v2 2/3] contrib: pick: remove hack notmuch-pick-show-match-message-with-wait

[Mark Walters]

This function was a horrible hack (sleeping while waiting for the
correct message). The new target code can just open the message in the
message window when it arrives.
---
 contrib/notmuch-pick/notmuch-pick.el |   37 +++++++++++-----------------------
 1 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index ef16ca7..5639c7c 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -157,6 +157,8 @@
 (make-variable-buffer-local 'notmuch-pick-query-context)
 (defvar notmuch-pick-target-msg nil)
 (make-variable-buffer-local 'notmuch-pick-target-msg)
+(defvar notmuch-pick-open-target nil)
+(make-variable-buffer-local 'notmuch-pick-open-target)
 (defvar notmuch-pick-buffer-name nil)
 (make-variable-buffer-local 'notmuch-pick-buffer-name)
 ;; This variable is the window used for the message pane. It is set
@@ -349,8 +351,8 @@ Does NOT change the database."
   (notmuch-pick (notmuch-search-find-thread-id)
                 notmuch-search-query-string
 		nil
-                (notmuch-prettify-subject (notmuch-search-find-subject)))
-  (notmuch-pick-show-match-message-with-wait))
+                (notmuch-prettify-subject (notmuch-search-find-subject))
+		t))
 
 (defun notmuch-pick-message-window-kill-hook ()
   (let ((buffer (current-buffer)))
@@ -489,22 +491,6 @@ message will be \"unarchived\", i.e. the tag changes in
   (when (window-live-p notmuch-pick-message-window)
     (notmuch-pick-show-message)))
 
-(defun notmuch-pick-show-match-message-with-wait ()
-  "Show the first matching message but wait for it to appear or search to finish."
-  (interactive)
-  (unless (notmuch-pick-get-match)
-    (notmuch-pick-next-matching-message))
-  (while (and (not (notmuch-pick-get-match))
-	      (get-buffer-process (current-buffer)))
-    (message "waiting for message")
-    (sit-for 0.1)
-    (goto-char (point-min))
-    (unless (notmuch-pick-get-match)
-      (notmuch-pick-next-matching-message)))
-  (message nil)
-  (when (notmuch-pick-get-match)
-    (notmuch-pick-show-message)))
-
 (defun notmuch-pick-refresh-view ()
   "Refresh view."
   (interactive)
@@ -658,7 +644,9 @@ unchanged ADDRESS if parsing fails."
 	      (string= msg-id target))
       (setq notmuch-pick-target-msg "found")
       (goto-char (point-max))
-      (forward-line -1))))
+      (forward-line -1)
+      (when notmuch-pick-open-target
+	(notmuch-pick-show-message)))))
 
 (defun notmuch-pick-insert-tree (tree depth tree-status first last)
   "Insert the message tree TREE at depth DEPTH in the current thread."
@@ -778,13 +766,14 @@ Complete list of currently available key bindings:
 	(notmuch-sexp-parse-partial-list 'notmuch-pick-insert-forest-thread
 					 results-buf)))))
 
-(defun notmuch-pick-worker (basic-query &optional query-context target buffer)
+(defun notmuch-pick-worker (basic-query &optional query-context target buffer open-target)
   (interactive)
   (notmuch-pick-mode)
   (setq notmuch-pick-basic-query basic-query)
   (setq notmuch-pick-query-context query-context)
   (setq notmuch-pick-buffer-name (buffer-name buffer))
   (setq notmuch-pick-target-msg target)
+  (setq notmuch-pick-open-target open-target)
 
   (erase-buffer)
   (goto-char (point-min))
@@ -816,7 +805,7 @@ Complete list of currently available key bindings:
 	  (insert "End of search results.\n"))))))
 
 
-(defun notmuch-pick (&optional query query-context target buffer-name show-first-match)
+(defun notmuch-pick (&optional query query-context target buffer-name open-target)
   "Run notmuch pick with the given `query' and display the results"
   (interactive "sNotmuch pick: ")
   (if (null query)
@@ -830,11 +819,9 @@ Complete list of currently available key bindings:
     ;; Don't track undo information for this buffer
     (set 'buffer-undo-list t)
 
-    (notmuch-pick-worker query query-context target buffer)
+    (notmuch-pick-worker query query-context target buffer open-target)
 
-    (setq truncate-lines t)
-    (when show-first-match
-      (notmuch-pick-show-match-message-with-wait))))
+    (setq truncate-lines t)))
 
 
 ;; Set up key bindings from the rest of notmuch.
-- 
1.7.9.1

Re: [PATCH v2 2/3] contrib: pick: remove hack notmuch-pick-show-match-message-with-wait

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> +(defvar notmuch-pick-open-target nil)
> +(make-variable-buffer-local 'notmuch-pick-open-target)

What do people think about adding a code style suggestion/requirement
for elisp that all variables have docstrings, even if intended for
internal use?  It's true the existing code doesn't really meet this
standard.

>  (defvar notmuch-pick-buffer-name nil)
>  (make-variable-buffer-local 'notmuch-pick-buffer-name)
>  ;; This variable is the window used for the message pane. It is set
> @@ -349,8 +351,8 @@ Does NOT change the database."
>    (notmuch-pick (notmuch-search-find-thread-id)
>                  notmuch-search-query-string
>  		nil
> -                (notmuch-prettify-subject (notmuch-search-find-subject)))
> -  (notmuch-pick-show-match-message-with-wait))
> +                (notmuch-prettify-subject (notmuch-search-find-subject))
> +		t))

I think my previous complaint can be reformulated as (essentially) both
notmuch-pick and notmuch-pick-open-target could use (better) docstrings.

As you say, the hack removed is quite horrible, so I'd be willing to
merge the patches anyway. OTOH, more documentation might make it so that
more than one person can understand the notmuch-pick code.

d

Re: [PATCH v2 2/3] contrib: pick: remove hack notmuch-pick-show-match-message-with-wait

[Mark Walters]

Hi

Many thanks for the review!

On Sun, 30 Jun 2013, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> +(defvar notmuch-pick-open-target nil)
>> +(make-variable-buffer-local 'notmuch-pick-open-target)
>
> What do people think about adding a code style suggestion/requirement
> for elisp that all variables have docstrings, even if intended for
> internal use?  It's true the existing code doesn't really meet this
> standard.

I think this would be a good idea (but see below): I assume this is
anything defined with a defvar?

>>  (defvar notmuch-pick-buffer-name nil)
>>  (make-variable-buffer-local 'notmuch-pick-buffer-name)
>>  ;; This variable is the window used for the message pane. It is set
>> @@ -349,8 +351,8 @@ Does NOT change the database."
>>    (notmuch-pick (notmuch-search-find-thread-id)
>>                  notmuch-search-query-string
>>  		nil
>> -                (notmuch-prettify-subject (notmuch-search-find-subject)))
>> -  (notmuch-pick-show-match-message-with-wait))
>> +                (notmuch-prettify-subject (notmuch-search-find-subject))
>> +		t))
>
> I think my previous complaint can be reformulated as (essentially) both
> notmuch-pick and notmuch-pick-open-target could use (better) docstrings.

I will send a patch to add a docstring to the main notmuch-pick function
as a reply to this message. The exact style was unclear (we seem to do
different things in different places).

I am not sure what the best way to document the variable is: there are
several variables that are essentially buffer local versions of the
arguments passed to notmuch-pick. Should these duplicate the
documentation? Exactly the same situation occurs with notmuch-show in
notmuch-show.el and notmuch-search in notmuch.el and in both those cases
the functions are well documented but the variables are not documented.

Any suggestions?

Best wishes

Mark



> As you say, the hack removed is quite horrible, so I'd be willing to
> merge the patches anyway. OTOH, more documentation might make it so that
> more than one person can understand the notmuch-pick code.
>
> d

[PATCH] contrib: pick: add a docstring for the main notmuch-pick function

[Mark Walters]

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

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 26966e6..a42491e 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -818,7 +818,19 @@ Complete list of currently available key bindings:
 
 
 (defun notmuch-pick (&optional query query-context target buffer-name open-target)
-  "Run notmuch pick with the given `query' and display the results"
+  "Run notmuch pick with the given `query' and display the results.
+
+The arguments are:
+  QUERY: the main query. This can be any query but in many cases will be
+      a single thread. If nil this is read interactively from the minibuffer.
+  QUERY-CONTEXT: is an additional term for the query. The query used
+      is QUERY and QUERY-CONTEXT unless that does not match any messages
+      in which case we fall back to just QUERY.
+  TARGET: A message ID (with the id: prefix) that will be made
+      current if it appears in the pick results.
+  BUFFER-NAME: the name of the buffer to show the pick tree. If
+      it is nil \"*notmuch-pick\" followed by QUERY is used.
+  OPEN-TARGET: If TRUE open the target message in the message pane."
   (interactive "sNotmuch pick: ")
   (if (null query)
       (setq query (notmuch-read-query "Notmuch pick: ")))
-- 
1.7.9.1

Re: [PATCH v2 2/3] contrib: pick: remove hack notmuch-pick-show-match-message-with-wait

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> I am not sure what the best way to document the variable is: there are
> several variables that are essentially buffer local versions of the
> arguments passed to notmuch-pick. Should these duplicate the
> documentation? Exactly the same situation occurs with notmuch-show in
> notmuch-show.el and notmuch-search in notmuch.el and in both those cases
> the functions are well documented but the variables are not documented.

what about just saying something like "this variable is a buffer local
copy of argument FOO to function BAR" ?

d

[PATCH v2 3/3] contrib: pick: fix refresh result

[Mark Walters]

The function notmuch-pick-refresh-result (used to update tag changes)
was not quite correct: sometimes it got the choice between the subject
and " ..." wrong. This was always true but the new code often calls
this (when opening a message in the message pane to remove the unread
tag) while the async pick process is still running and this caused
mistakes which made the tests fail.

Thus we store the previous subject with the message.
---
 contrib/notmuch-pick/notmuch-pick.el |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/contrib/notmuch-pick/notmuch-pick.el b/contrib/notmuch-pick/notmuch-pick.el
index 5639c7c..11b5058 100644
--- a/contrib/notmuch-pick/notmuch-pick.el
+++ b/contrib/notmuch-pick/notmuch-pick.el
@@ -264,8 +264,15 @@ Some useful entries are:
 	(msg (notmuch-pick-get-message-properties))
 	(inhibit-read-only t))
     (beginning-of-line)
-    (delete-region (point) (1+ (line-end-position)))
-    (notmuch-pick-insert-msg msg)
+    ;; This is a little tricky: we override
+    ;; notmuch-pick-previous-subject to get the decision between
+    ;; ... and a subject right and it stops notmuch-pick-insert-msg
+    ;; from overwriting the buffer local copy of
+    ;; notmuch-pick-previous-subject if this is called while the
+    ;; buffer is displaying.
+    (let ((notmuch-pick-previous-subject (notmuch-pick-get-prop :previous-subject)))
+      (delete-region (point) (1+ (line-end-position)))
+      (notmuch-pick-insert-msg msg))
     (let ((new-end (line-end-position)))
       (goto-char (if (= init-point end)
 		     new-end
@@ -628,10 +635,14 @@ unchanged ADDRESS if parsing fails."
 
 (defun notmuch-pick-insert-msg (msg)
   "Insert the message MSG according to notmuch-pick-result-format"
-  (dolist (spec notmuch-pick-result-format)
-    (notmuch-pick-insert-field (car spec) (cdr spec) msg))
-  (notmuch-pick-set-message-properties msg)
-  (insert "\n"))
+  ;; We need to save the previous subject as it will get overwritten
+  ;; by the insert-field calls.
+  (let ((previous-subject notmuch-pick-previous-subject))
+    (dolist (spec notmuch-pick-result-format)
+      (notmuch-pick-insert-field (car spec) (cdr spec) msg))
+    (notmuch-pick-set-message-properties msg)
+    (notmuch-pick-set-prop :previous-subject previous-subject)
+    (insert "\n")))
 
 (defun notmuch-pick-goto-and-insert-msg (msg)
   "Insert msg at the end of the buffer. Move point to msg if it is the target"
-- 
1.7.9.1

Re: [PATCH v2 3/3] contrib: pick: fix refresh result

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> The function notmuch-pick-refresh-result (used to update tag changes)
> was not quite correct: sometimes it got the choice between the subject
> and " ..." wrong. This was always true but the new code often calls
> this (when opening a message in the message pane to remove the unread
> tag) while the async pick process is still running and this caused
> mistakes which made the tests fail.

This looks OK, and independant of the other patches.

removing "needs-review".

d

Re: [PATCH v2 0/3] Contrib: Pick: Remove horrible hack

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> This is a trivial rebase of
> id:1369551008-30697-1-git-send-email-markwalters1009@gmail.com to
> current master.  I have included the dependent patch
> id:1369550458-30562-1-git-send-email-markwalters1009@gmail.com

Pushed,

d