[PATCH 0/2] Contrib: Pick: Remove horrible hack

[PATCH 0/2] Contrib: Pick: Remove horrible hack

[Mark Walters]

Pick used to have one horrible hack: if the user asked it to open the
first matching message it had to check whether that had arrived (as
the search is asynchronous) and if not wait and try again. Now the
opening of the first matching message is called via the pick process
filter this hack can be removed.

This did reveal the followibg small bug.  Pick shows the subject line
in the output but if it is the same as the previous line (ignoring re:
etc) it shows ... If a single message is refreshed (eg for a tag
update) this was got wrong. The change above triggered this and made
the test fail as the unread tag was removed from the first matching
message when it was displayed.

Patch 2/2 fixes this by storing the previous subject with the search result.

Best wishes

Mark
 

Mark Walters (2):
  contrib: pick: remove hack notmuch-pick-show-match-message-with-wait
  contrib: pick: fix refresh result

 contrib/notmuch-pick/notmuch-pick.el |   60 ++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 31 deletions(-)

-- 
1.7.9.1

[PATCH 1/2] 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 b7e9515..9112989 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 @@ will be reversed."
   (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."
@@ -779,13 +767,14 @@ Complete list of currently available key bindings:
 					 'notmuch-pick-show-error
 					 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))
@@ -817,7 +806,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)
@@ -831,11 +820,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

[PATCH 2/2] 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 9112989..46d9503 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 0/2] Contrib: Pick: Remove horrible hack

[Mark Walters]

Would anyone be able to review (or just test) this pair of patches: they
are smaller and simpler than the diffstat suggests: the second patch is
just 3 extra lines of code (with some whitespace change and commments).

This pair of patches does remove the worst piece of code in pick: a
sleep loop waiting for the correct message to arrive.

I should have said that it applies on top of
id:1369550458-30562-1-git-send-email-markwalters1009@gmail.com

(which is also very simple)

Many thanks

Mark




On Sun, 26 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Pick used to have one horrible hack: if the user asked it to open the
> first matching message it had to check whether that had arrived (as
> the search is asynchronous) and if not wait and try again. Now the
> opening of the first matching message is called via the pick process
> filter this hack can be removed.
>
> This did reveal the followibg small bug.  Pick shows the subject line
> in the output but if it is the same as the previous line (ignoring re:
> etc) it shows ... If a single message is refreshed (eg for a tag
> update) this was got wrong. The change above triggered this and made
> the test fail as the unread tag was removed from the first matching
> message when it was displayed.
>
> Patch 2/2 fixes this by storing the previous subject with the search result.
>
> Best wishes
>
> Mark
>  
>
> Mark Walters (2):
>   contrib: pick: remove hack notmuch-pick-show-match-message-with-wait
>   contrib: pick: fix refresh result
>
>  contrib/notmuch-pick/notmuch-pick.el |   60 ++++++++++++++++-----------------
>  1 files changed, 29 insertions(+), 31 deletions(-)
>
> -- 
> 1.7.9.1

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

[Mark Walters]

Actually I will post a new version as this version doesn't apply anymore
(the context which overlaps an unrelated function has changed)

Best wishes

Mark


On Sun, 30 Jun 2013, Mark Walters <markwalters1009@gmail.com> wrote:
> Would anyone be able to review (or just test) this pair of patches: they
> are smaller and simpler than the diffstat suggests: the second patch is
> just 3 extra lines of code (with some whitespace change and commments).
>
> This pair of patches does remove the worst piece of code in pick: a
> sleep loop waiting for the correct message to arrive.
>
> I should have said that it applies on top of
> id:1369550458-30562-1-git-send-email-markwalters1009@gmail.com
>
> (which is also very simple)
>
> Many thanks
>
> Mark
>
>
>
>
> On Sun, 26 May 2013, Mark Walters <markwalters1009@gmail.com> wrote:
>> Pick used to have one horrible hack: if the user asked it to open the
>> first matching message it had to check whether that had arrived (as
>> the search is asynchronous) and if not wait and try again. Now the
>> opening of the first matching message is called via the pick process
>> filter this hack can be removed.
>>
>> This did reveal the followibg small bug.  Pick shows the subject line
>> in the output but if it is the same as the previous line (ignoring re:
>> etc) it shows ... If a single message is refreshed (eg for a tag
>> update) this was got wrong. The change above triggered this and made
>> the test fail as the unread tag was removed from the first matching
>> message when it was displayed.
>>
>> Patch 2/2 fixes this by storing the previous subject with the search result.
>>
>> Best wishes
>>
>> Mark
>>  
>>
>> Mark Walters (2):
>>   contrib: pick: remove hack notmuch-pick-show-match-message-with-wait
>>   contrib: pick: fix refresh result
>>
>>  contrib/notmuch-pick/notmuch-pick.el |   60 ++++++++++++++++-----------------
>>  1 files changed, 29 insertions(+), 31 deletions(-)
>>
>> -- 
>> 1.7.9.1