[PATCH] Fix hiding a message while some citations are visible

[PATCH] Fix hiding a message while some citations are visible

[Dmitry Kurochkin]

This patch series does some fixes and improvements for hiding and
showing messages in notmuch-show.  However it comes with a
regression: isearch is broken for hidden overlays when
`invisible' property is a list.  I have opened emacs bug #8721
[1] and sent a patch.  It has been committed to emacs trunk
r104356.

I do not know if this should be pushed or wait until emacs with
the fix is released.

Regards,
  Dmitry

[1] http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8721

[PATCH 1/5] Pass message to the `notmuch-show-insert-text/plain-hook' hook.

[Dmitry Kurochkin]

Before the change, the `notmuch-show-insert-text/plain-hook' was
given only the `depth' argument.  The patch adds another one -
the message.  Currently, the new message argument is not used by
any on the hooks.  But it will be used later to get access to
message invisibility specs when wash buttons are inserted.
---
 emacs/notmuch-show.el |    2 +-
 emacs/notmuch-wash.el |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index bd348e1..786debf 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -531,7 +531,7 @@ current buffer, if possible."
     (save-excursion
       (save-restriction
 	(narrow-to-region start (point-max))
-	(run-hook-with-args 'notmuch-show-insert-text/plain-hook depth))))
+	(run-hook-with-args 'notmuch-show-insert-text/plain-hook msg depth))))
   t)
 
 (defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type)
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 115c3bb..7d87e85 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -147,7 +147,7 @@ insert before the button, probably for indentation."
 		     'overlay overlay
 		     :type button-type))))
 
-(defun notmuch-wash-excerpt-citations (depth)
+(defun notmuch-wash-excerpt-citations (msg depth)
   "Excerpt citations and up to one signature."
   (goto-char (point-min))
   (beginning-of-line)
@@ -185,7 +185,7 @@ insert before the button, probably for indentation."
 
 ;;
 
-(defun notmuch-wash-elide-blank-lines (depth)
+(defun notmuch-wash-elide-blank-lines (msg depth)
   "Elide leading, trailing and successive blank lines."
 
   ;; Algorithm derived from `article-strip-multiple-blank-lines' in
@@ -213,7 +213,7 @@ insert before the button, probably for indentation."
 
 ;;
 
-(defun notmuch-wash-tidy-citations (depth)
+(defun notmuch-wash-tidy-citations (msg depth)
   "Improve the display of cited regions of a message.
 
 Perform several transformations on the message body:
@@ -244,7 +244,7 @@ Perform several transformations on the message body:
 
 ;;
 
-(defun notmuch-wash-wrap-long-lines (depth)
+(defun notmuch-wash-wrap-long-lines (msg depth)
   "Wrap any long lines in the message to the width of the window.
 
 When doing so, maintaining citation leaders in the wrapped text."
@@ -263,7 +263,7 @@ When doing so, maintaining citation leaders in the wrapped text."
 
 (defvar diff-file-header-re) ; From `diff-mode.el'.
 
-(defun notmuch-wash-convert-inline-patch-to-part (depth)
+(defun notmuch-wash-convert-inline-patch-to-part (msg depth)
   "Convert an inline patch into a fake 'text/x-diff' attachment.
 
 Given that this function guesses whether a buffer includes a
-- 
1.7.5.1

[PATCH 2/5] Set message invisibility spec properties before inserting the body.

[Dmitry Kurochkin]

This would allow body-inserting code (in particular, wash
button-inserting code) to use message invisibility specs.
---
 emacs/notmuch-show.el |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 786debf..34c0b79 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -693,6 +693,9 @@ current buffer, if possible."
 
     (setq content-start (point-marker))
 
+    (plist-put msg :headers-invis-spec headers-invis-spec)
+    (plist-put msg :message-invis-spec message-invis-spec)
+
     ;; Set `headers-start' to point after the 'Subject:' header to be
     ;; compatible with the existing implementation. This just sets it
     ;; to after the first header.
@@ -730,10 +733,7 @@ current buffer, if possible."
     ;; message.
     (put-text-property message-start message-end :notmuch-message-extent (cons message-start message-end))
 
-    (plist-put msg :headers-invis-spec headers-invis-spec)
     (overlay-put (make-overlay headers-start headers-end) 'invisible headers-invis-spec)
-
-    (plist-put msg :message-invis-spec message-invis-spec)
     (overlay-put (make-overlay body-start body-end) 'invisible message-invis-spec)
 
     ;; Save the properties for this message. Currently this saves the
-- 
1.7.5.1

[PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

Before the change, message and citation invisibility overlays
conflicted: if some citation is made visible and then the whole
message is hidden, that citation remained visible.  This happened
because the citation's overlay has an invisible property which
takes priority over the message overlay.  The message
invisibility spec does not affect citation visibility, it is
determined solely by the citation overlay invisibility spec.
Hence, if citation is made visible, it is not hidden by message
invisibility spec.

The patch changes citation overlay invisibility property to be a
list which contains both the citation and the message
invisibility specs.  This makes the citation invisible if either
of them is added to the `buffer-invisibility-spec'.  Note that
all citation visibility states are "restored" when the message
hidden and shown again.
---
 emacs/notmuch-wash.el |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 7d87e85..bd521ba 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -107,7 +107,8 @@ collapse the remaining lines into a button.")
   :supertype 'notmuch-wash-button-invisibility-toggle-type)
 
 (defun notmuch-wash-region-isearch-show (overlay)
-  (remove-from-invisibility-spec (overlay-get overlay 'invisible)))
+  (dolist (invis-spec (overlay-get overlay 'invisible))
+    (remove-from-invisibility-spec invis-spec)))
 
 (defun notmuch-wash-button-label (overlay)
   (let* ((type (overlay-get overlay 'type))
@@ -118,7 +119,7 @@ collapse the remaining lines into a button.")
 	 (lines-count (count-lines (overlay-start overlay) (overlay-end overlay))))
     (format label-format lines-count)))
 
-(defun notmuch-wash-region-to-button (beg end type prefix)
+(defun notmuch-wash-region-to-button (msg beg end type prefix)
   "Auxilary function to do the actual making of overlays and buttons
 
 BEG and END are buffer locations. TYPE should a string, either
@@ -131,11 +132,12 @@ insert before the button, probably for indentation."
   ;; since the newly created symbol has no plist.
 
   (let ((overlay (make-overlay beg end))
+	(message-invis-spec (plist-get msg :message-invis-spec))
 	(invis-spec (make-symbol (concat "notmuch-" type "-region")))
 	(button-type (intern-soft (concat "notmuch-wash-button-"
 					  type "-toggle-type"))))
     (add-to-invisibility-spec invis-spec)
-    (overlay-put overlay 'invisible invis-spec)
+    (overlay-put overlay 'invisible (list invis-spec message-invis-spec))
     (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
     (overlay-put overlay 'type type)
     (goto-char (1+ end))
@@ -166,7 +168,7 @@ insert before the button, probably for indentation."
 	  (goto-char cite-end)
 	  (forward-line (- notmuch-wash-citation-lines-suffix))
 	  (notmuch-wash-region-to-button
-	   hidden-start (point-marker)
+	   msg hidden-start (point-marker)
 	   "citation" "\n")))))
   (if (and (not (eobp))
 	   (re-search-forward notmuch-wash-signature-regexp nil t))
@@ -180,7 +182,7 @@ insert before the button, probably for indentation."
 	      (set-marker sig-end-marker (point-max))
 	      (overlay-put (make-overlay sig-start-marker sig-end-marker) 'face 'message-cited-text)
 	      (notmuch-wash-region-to-button
-	       sig-start-marker sig-end-marker
+	       msg sig-start-marker sig-end-marker
 	       "signature" "\n"))))))
 
 ;;
-- 
1.7.5.1

[PATCH 4/5] Set higher priority for headers and hidden citation overlays.

[Dmitry Kurochkin]

Before the patch, message, headers and hidden citation overlays
had zero priority.  All these overlay have `invisible' property.
Emacs documentation says that we should not make assumptions
about which overlay will prevail when they have the same priority
[1].  It happens to work as we need, but we should not rely on
undocumented behavior.

[1] http://www.gnu.org/s/emacs/manual/html_node/elisp/Overlay-Properties.html
---
 emacs/notmuch-show.el |    4 +++-
 emacs/notmuch-wash.el |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 34c0b79..e1846bc 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -733,7 +733,9 @@ current buffer, if possible."
     ;; message.
     (put-text-property message-start message-end :notmuch-message-extent (cons message-start message-end))
 
-    (overlay-put (make-overlay headers-start headers-end) 'invisible headers-invis-spec)
+    (let ((headers-overlay (make-overlay headers-start headers-end)))
+      (overlay-put headers-overlay 'invisible headers-invis-spec)
+      (overlay-put headers-overlay 'priority 10))
     (overlay-put (make-overlay body-start body-end) 'invisible message-invis-spec)
 
     ;; Save the properties for this message. Currently this saves the
diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index bd521ba..992fa8f 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -139,6 +139,7 @@ insert before the button, probably for indentation."
     (add-to-invisibility-spec invis-spec)
     (overlay-put overlay 'invisible (list invis-spec message-invis-spec))
     (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
+    (overlay-put overlay 'priority 10)
     (overlay-put overlay 'type type)
     (goto-char (1+ end))
     (save-excursion
-- 
1.7.5.1

[PATCH 5/5] Simplify message and headers visibility code in notmuch-show view.

[Dmitry Kurochkin]

Before the change, headers and message visibility functions took
extra care to correctly set `buffer-invisibility-spec'.  This was
needed because headers overlay `invisible' property had only
headers' invisibility spec.  So visibility of headers was
determined only by the headers invisibility spec.  The patch sets
headers overlay `invisible' property a list with both the headers
and the message invisibility spec.  This makes headers invisible
if either of them is added to the `buffer-invisibility-spec' and
allows to simplify the code.
---
 emacs/notmuch-show.el |   18 +++++-------------
 1 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index e1846bc..2f7154e 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -733,8 +733,9 @@ current buffer, if possible."
     ;; message.
     (put-text-property message-start message-end :notmuch-message-extent (cons message-start message-end))
 
-    (let ((headers-overlay (make-overlay headers-start headers-end)))
-      (overlay-put headers-overlay 'invisible headers-invis-spec)
+    (let ((headers-overlay (make-overlay headers-start headers-end))
+          (invis-specs (list headers-invis-spec message-invis-spec)))
+      (overlay-put headers-overlay 'invisible invis-specs)
       (overlay-put headers-overlay 'priority 10))
     (overlay-put (make-overlay body-start body-end) 'invisible message-invis-spec)
 
@@ -974,20 +975,11 @@ All currently available key bindings:
       (add-to-invisibility-spec spec))))
 
 (defun notmuch-show-message-visible (props visible-p)
-  (if visible-p
-      ;; When making the message visible, the headers may or not be
-      ;; visible. So we check that property separately.
-      (let ((headers-visible (plist-get props :headers-visible)))
-	(notmuch-show-element-visible props headers-visible :headers-invis-spec)
-	(notmuch-show-element-visible props t :message-invis-spec))
-    (notmuch-show-element-visible props nil :headers-invis-spec)
-    (notmuch-show-element-visible props nil :message-invis-spec))
-
+  (notmuch-show-element-visible props visible-p :message-invis-spec)
   (notmuch-show-set-prop :message-visible visible-p props))
 
 (defun notmuch-show-headers-visible (props visible-p)
-  (if (plist-get props :message-visible)
-      (notmuch-show-element-visible props visible-p :headers-invis-spec))
+  (notmuch-show-element-visible props visible-p :headers-invis-spec)
   (notmuch-show-set-prop :headers-visible visible-p props))
 
 ;; Functions for setting and getting attributes of the current
-- 
1.7.5.1

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

On Thu, 26 May 2011 02:10:14 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Before the change, message and citation invisibility overlays
> conflicted: if some citation is made visible and then the whole
> message is hidden, that citation remained visible.

That sounds like quite a bug. I'd love to see this series also add a
test case for that.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Wed, 25 May 2011 15:23:47 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 26 May 2011 02:10:14 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Before the change, message and citation invisibility overlays
> > conflicted: if some citation is made visible and then the whole
> > message is hidden, that citation remained visible.
> 
> That sounds like quite a bug. I'd love to see this series also add a
> test case for that.
> 

I am not sure how it is best to test this.  The common `printc' method
for emacs tests does not work, because it prints invisible parts as
well.  We need either to find a way to print only visible text on the
console, or test it inside emacs somehow.  Any suggestions?

Note that this is exactly the patch that hits the isearch emacs bug.  Do
I understand correctly that you are ready to push the series despite of
it (given that we have a test)?

Regards,
  Dmitry

> -Carl
> 
> -- 
> carl.d.worth@intel.com
Non-text part: application/pgp-signature

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]

On Thu, 26 May 2011 02:34:28 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I am not sure how it is best to test this.  The common `printc' method
> for emacs tests does not work, because it prints invisible parts as
> well.  We need either to find a way to print only visible text on the
> console, or test it inside emacs somehow.  Any suggestions?

Unfortunately, I don't have a good plan here. I delayed implementing any
automated testing at all of the emacs interface precisely because of
this problem. It's seems to me that surely emacs must have some built-in
mechanism for copying the visible portion of a block of text, but I've
not been able to find it.

We could do something cheesy (and slow) by marching through the buffer
character-by-character in elisp and testing for visibility, but the
emacs tests are already the slowest part of "make test"[*] so that would
be obnoxious.

> Note that this is exactly the patch that hits the isearch emacs bug.  Do
> I understand correctly that you are ready to push the series despite of
> it (given that we have a test)?

Breaking isearch would be really unfortunate. That's a really nice
feature of the emacs frontend currently.

So I would notice that breakage, (while I've apparently never before
noticed the breakage of having visible citations in a hidden message).

So no, I'm not saying I'm ready to push the series while emacs is broken.

-Carl

[*] Maybe the performance of the emacs testing could be significantly
improved by sharing a single invocation of emacs? Perhaps this wouldn't
even be hard by just using emacsclient?


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Wed, 25 May 2011 15:46:40 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 26 May 2011 02:34:28 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I am not sure how it is best to test this.  The common `printc' method
> > for emacs tests does not work, because it prints invisible parts as
> > well.  We need either to find a way to print only visible text on the
> > console, or test it inside emacs somehow.  Any suggestions?
> 
> Unfortunately, I don't have a good plan here. I delayed implementing any
> automated testing at all of the emacs interface precisely because of
> this problem. It's seems to me that surely emacs must have some built-in
> mechanism for copying the visible portion of a block of text, but I've
> not been able to find it.
> 

Me too.

> We could do something cheesy (and slow) by marching through the buffer
> character-by-character in elisp and testing for visibility, but the
> emacs tests are already the slowest part of "make test"[*] so that would
> be obnoxious.
> 

Indeed.

> > Note that this is exactly the patch that hits the isearch emacs bug.  Do
> > I understand correctly that you are ready to push the series despite of
> > it (given that we have a test)?
> 
> Breaking isearch would be really unfortunate. That's a really nice
> feature of the emacs frontend currently.
> 
> So I would notice that breakage, (while I've apparently never before
> noticed the breakage of having visible citations in a hidden message).
> 
> So no, I'm not saying I'm ready to push the series while emacs is broken.
> 

Well, emacs trunk is not broken :)  The bug is in lisp code, so you can
fix it in .emacs by redefining `isearch-range-invisible' function.  I do
that now.

I do not think I will make myself work on the test until it is likely to
be pushed.  I will try to not to forget about it, so sometime later I
may be back to it :)

Please consider pushing other patches from the series.  They do not fix
any bug, but do simplify the code.  The last patch uses list for
invisible overlay property as well.  But it does not break isearch
because we do not search in hidden messages.

BTW would be nice to have a set of known-to-fail tests, i.e. bugs that
are not fixed yet.  If we had it, the above test could be implemented
and committed before we have the fix pushed.

> -Carl
> 
> [*] Maybe the performance of the emacs testing could be significantly
> improved by sharing a single invocation of emacs? Perhaps this wouldn't
> even be hard by just using emacsclient?
> 

This is possible as long as tests do not affect each other.  Would be a
nice improvement.

Regards,
  Dmitry

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

On Thu, 26 May 2011 03:10:11 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Wed, 25 May 2011 15:46:40 -0700, Carl Worth <cworth@cworth.org> wrote:
> Well, emacs trunk is not broken :)  The bug is in lisp code, so you can
> fix it in .emacs by redefining `isearch-range-invisible' function.  I do
> that now.

Oh, in that case we can fix this is notmuch emacs lisp by just defining
and using a fixed function. Is the broken function something we're
calling directly? Or is it being called indirectly? (being called by
other emacs lisp code that we are calling)?

If we can incorporate the fix, that would be great.

> Please consider pushing other patches from the series.  They do not fix
> any bug, but do simplify the code.  The last patch uses list for
> invisible overlay property as well.  But it does not break isearch
> because we do not search in hidden messages.

Hmmm... we should probably do that. I'd like isearch in notmuch to
search anything that is hidden.

> BTW would be nice to have a set of known-to-fail tests, i.e. bugs that
> are not fixed yet.  If we had it, the above test could be implemented
> and committed before we have the fix pushed.

We do! Use test_expect_equal_failure (yes, the name is horrible!)
instead of test_expect_equal and you should get what you want.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Wed, 25 May 2011 18:02:49 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 26 May 2011 03:10:11 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > On Wed, 25 May 2011 15:46:40 -0700, Carl Worth <cworth@cworth.org> wrote:
> > Well, emacs trunk is not broken :)  The bug is in lisp code, so you can
> > fix it in .emacs by redefining `isearch-range-invisible' function.  I do
> > that now.
> 
> Oh, in that case we can fix this is notmuch emacs lisp by just defining
> and using a fixed function. Is the broken function something we're
> calling directly? Or is it being called indirectly? (being called by
> other emacs lisp code that we are calling)?
> 

It is called indirectly.  What is the best way to fix it?  I imagine
that we can replace `isearch-range-invisible' function with another one,
which would call the fixed function if some special variable is set, or
if we are searching in a notmuch-show view.  Otherwise it would call the
original function.

> If we can incorporate the fix, that would be great.
> 
> > Please consider pushing other patches from the series.  They do not fix
> > any bug, but do simplify the code.  The last patch uses list for
> > invisible overlay property as well.  But it does not break isearch
> > because we do not search in hidden messages.
> 
> Hmmm... we should probably do that. I'd like isearch in notmuch to
> search anything that is hidden.
> 

That is easy to fix.  I can do that once we are done with this stuff.

> > BTW would be nice to have a set of known-to-fail tests, i.e. bugs that
> > are not fixed yet.  If we had it, the above test could be implemented
> > and committed before we have the fix pushed.
> 
> We do! Use test_expect_equal_failure (yes, the name is horrible!)
> instead of test_expect_equal and you should get what you want.
> 

I should look at it, thanks.

Regards,
  Dmitry

> -Carl
> 
> -- 
> carl.d.worth@intel.com

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

On Thu, 26 May 2011 14:26:34 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> It is called indirectly.  What is the best way to fix it?  I imagine
> that we can replace `isearch-range-invisible' function with another one,
> which would call the fixed function if some special variable is set, or
> if we are searching in a notmuch-show view.  Otherwise it would call the
> original function.

I'm not sure what the best approach would be. I'd like to be a good
"emacs citizen" but I don't know what the best way to do that is here.

If we can predict what the first emacs release will be that contains the
fix, then we could restrict our kludge here to older, unfixed
emacs. That would help avoid some problems with "bad citizenship here".

I'm willing to take whatever you this is best here.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 1/2] test: add tests for hiding messages in notmuch-show view

[Dmitry Kurochkin]

---
 test/emacs                                         |   16 ++++++++++++++++
 .../notmuch-show-thread-with-hidden-messages       |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 test/emacs.expected-output/notmuch-show-thread-with-hidden-messages

diff --git a/test/emacs b/test/emacs
index ccf02af..7421650 100755
--- a/test/emacs
+++ b/test/emacs
@@ -152,4 +152,20 @@ output=$(test_emacs "(notmuch-show \"$maildir_storage_thread\")
 expected=$(cat $EXPECTED/notmuch-show-thread-maildir-storage)
 test_expect_equal "$output" "$expected"
 
+test_begin_subtest "Hiding message in notmuch-show view"
+output=$(test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com")
+		     (notmuch-show-toggle-message)
+		     (princ (visible-buffer-string))')
+expected=$(cat $EXPECTED/notmuch-show-thread-with-hidden-messages)
+test_expect_equal "$output" "$expected"
+
+test_begin_subtest "Hiding message with visible citation in notmuch-show view"
+output=$(test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.gmail.com")
+		     (search-forward "Click/Enter to show.")
+		     (button-activate (button-at (point)))
+		     (notmuch-show-toggle-message)
+		     (princ (visible-buffer-string))')
+expected=$(cat $EXPECTED/notmuch-show-thread-with-hidden-messages)
+test_expect_equal "$output" "$expected"
+
 test_done
diff --git a/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages b/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages
new file mode 100644
index 0000000..5df6606
--- /dev/null
+++ b/test/emacs.expected-output/notmuch-show-thread-with-hidden-messages
@@ -0,0 +1,3 @@
+Jan Janak <jan@ryngle.com> (2009-11-17) (inbox unread)
+ Jan Janak <jan@ryngle.com> (2009-11-17) (inbox)
+ Carl Worth <cworth@cworth.org> (2009-11-18) (inbox unread)
-- 
1.7.5.1

[PATCH 2/2] Workaround for Emacs bug #8721.

[Dmitry Kurochkin]

The patch adds `notmuch-isearch-range-invisible' function which
is the same as `isearch-range-invisible' but with fixed Emacs bug
`notmuch-isearch-range-invisible' instead of the original
`isearch-range-invisible' when in `notmuch-show-mode'.
---
 emacs/notmuch-wash.el |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 992fa8f..f37fd95 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -298,4 +298,71 @@ for error."
 
 ;;
 
+;; Temporary workaround for Emacs bug #8721
+;; http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8721
+
+(defun notmuch-isearch-range-invisible (beg end)
+  "Same as `isearch-range-invisible' but with fixed Emacs bug #8721."
+  (when (/= beg end)
+    ;; Check that invisibility runs up to END.
+    (save-excursion
+      (goto-char beg)
+      (let (;; can-be-opened keeps track if we can open some overlays.
+	    (can-be-opened (eq search-invisible 'open))
+	    ;; the list of overlays that could be opened
+	    (crt-overlays nil))
+	(when (and can-be-opened isearch-hide-immediately)
+	  (isearch-close-unnecessary-overlays beg end))
+	;; If the following character is currently invisible,
+	;; skip all characters with that same `invisible' property value.
+	;; Do that over and over.
+	(while (and (< (point) end) (invisible-p (point)))
+	  (if (get-text-property (point) 'invisible)
+	      (progn
+		(goto-char (next-single-property-change (point) 'invisible
+							nil end))
+		;; if text is hidden by an `invisible' text property
+		;; we cannot open it at all.
+		(setq can-be-opened nil))
+	    (when can-be-opened
+	      (let ((overlays (overlays-at (point)))
+		    ov-list
+		    o
+		    invis-prop)
+		(while overlays
+		  (setq o (car overlays)
+			invis-prop (overlay-get o 'invisible))
+		  (if (invisible-p invis-prop)
+		      (if (overlay-get o 'isearch-open-invisible)
+			  (setq ov-list (cons o ov-list))
+			;; We found one overlay that cannot be
+			;; opened, that means the whole chunk
+			;; cannot be opened.
+			(setq can-be-opened nil)))
+		  (setq overlays (cdr overlays)))
+		(if can-be-opened
+		    ;; It makes sense to append to the open
+		    ;; overlays list only if we know that this is
+		    ;; t.
+		    (setq crt-overlays (append ov-list crt-overlays)))))
+	    (goto-char (next-overlay-change (point)))))
+	;; See if invisibility reaches up thru END.
+	(if (>= (point) end)
+	    (if (and can-be-opened (consp crt-overlays))
+		(progn
+		  (setq isearch-opened-overlays
+			(append isearch-opened-overlays crt-overlays))
+		  (mapc 'isearch-open-overlay-temporary crt-overlays)
+		  nil)
+	      (setq isearch-hidden t)))))))
+
+(defadvice isearch-range-invisible (around notmuch-isearch-range-invisible-advice activate)
+  "Call `notmuch-isearch-range-invisible' instead of the original
+`isearch-range-invisible' when in `notmuch-show-mode' mode."
+  (if (eq major-mode 'notmuch-show-mode)
+      (setq ad-return-value (notmuch-isearch-range-invisible beg end))
+    ad-do-it))
+
+;;
+
 (provide 'notmuch-wash)
-- 
1.7.5.1

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Thu, 26 May 2011 14:31:30 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 26 May 2011 14:26:34 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > It is called indirectly.  What is the best way to fix it?  I imagine
> > that we can replace `isearch-range-invisible' function with another one,
> > which would call the fixed function if some special variable is set, or
> > if we are searching in a notmuch-show view.  Otherwise it would call the
> > original function.
> 
> I'm not sure what the best approach would be. I'd like to be a good
> "emacs citizen" but I don't know what the best way to do that is here.
> 

Agreed on being a good "emacs citizen".

> If we can predict what the first emacs release will be that contains the
> fix, then we could restrict our kludge here to older, unfixed
> emacs. That would help avoid some problems with "bad citizenship here".
> 
> I'm willing to take whatever you this is best here.
> 

I have just send two more patches to this thread.  One with new tests.
Another with a workaround for the bug.  The workaround should not break
anything since it affects only notmuch-show mode.

Regards,
  Dmitry

> -Carl
> 
> -- 
> carl.d.worth@intel.com
Non-text part: application/pgp-signature

Re: [PATCH 2/2] Workaround for Emacs bug #8721.

[Dmitry Kurochkin]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Fri, 27 May 2011 01:38:35 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The patch adds `notmuch-isearch-range-invisible' function which
> is the same as `isearch-range-invisible' but with fixed Emacs bug
> `notmuch-isearch-range-invisible' instead of the original
> `isearch-range-invisible' when in `notmuch-show-mode'.

I've screwed up the commit message because of a line starting with '#'.

Attach is an amended patch.

Regards,
  Dmitry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Workaround-for-Emacs-bug-8721.patch --]
[-- Type: text/x-diff, Size: 3416 bytes --]

From ead308fb53725d593562d7d4e3cd4aa82412aa70 Mon Sep 17 00:00:00 2001
From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Date: Fri, 27 May 2011 01:35:09 +0400
Subject: [PATCH] Workaround for Emacs bug #8721.

The patch adds `notmuch-isearch-range-invisible' function which
is the same as `isearch-range-invisible' but with fixed Emacs bug
 #8721.  Advice added for `isearch-range-invisible' which calls
`notmuch-isearch-range-invisible' instead of the original
`isearch-range-invisible' when in `notmuch-show-mode'.
---
 emacs/notmuch-wash.el |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 992fa8f..f37fd95 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -298,4 +298,71 @@ for error."
 
 ;;
 
+;; Temporary workaround for Emacs bug #8721
+;; http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8721
+
+(defun notmuch-isearch-range-invisible (beg end)
+  "Same as `isearch-range-invisible' but with fixed Emacs bug #8721."
+  (when (/= beg end)
+    ;; Check that invisibility runs up to END.
+    (save-excursion
+      (goto-char beg)
+      (let (;; can-be-opened keeps track if we can open some overlays.
+	    (can-be-opened (eq search-invisible 'open))
+	    ;; the list of overlays that could be opened
+	    (crt-overlays nil))
+	(when (and can-be-opened isearch-hide-immediately)
+	  (isearch-close-unnecessary-overlays beg end))
+	;; If the following character is currently invisible,
+	;; skip all characters with that same `invisible' property value.
+	;; Do that over and over.
+	(while (and (< (point) end) (invisible-p (point)))
+	  (if (get-text-property (point) 'invisible)
+	      (progn
+		(goto-char (next-single-property-change (point) 'invisible
+							nil end))
+		;; if text is hidden by an `invisible' text property
+		;; we cannot open it at all.
+		(setq can-be-opened nil))
+	    (when can-be-opened
+	      (let ((overlays (overlays-at (point)))
+		    ov-list
+		    o
+		    invis-prop)
+		(while overlays
+		  (setq o (car overlays)
+			invis-prop (overlay-get o 'invisible))
+		  (if (invisible-p invis-prop)
+		      (if (overlay-get o 'isearch-open-invisible)
+			  (setq ov-list (cons o ov-list))
+			;; We found one overlay that cannot be
+			;; opened, that means the whole chunk
+			;; cannot be opened.
+			(setq can-be-opened nil)))
+		  (setq overlays (cdr overlays)))
+		(if can-be-opened
+		    ;; It makes sense to append to the open
+		    ;; overlays list only if we know that this is
+		    ;; t.
+		    (setq crt-overlays (append ov-list crt-overlays)))))
+	    (goto-char (next-overlay-change (point)))))
+	;; See if invisibility reaches up thru END.
+	(if (>= (point) end)
+	    (if (and can-be-opened (consp crt-overlays))
+		(progn
+		  (setq isearch-opened-overlays
+			(append isearch-opened-overlays crt-overlays))
+		  (mapc 'isearch-open-overlay-temporary crt-overlays)
+		  nil)
+	      (setq isearch-hidden t)))))))
+
+(defadvice isearch-range-invisible (around notmuch-isearch-range-invisible-advice activate)
+  "Call `notmuch-isearch-range-invisible' instead of the original
+`isearch-range-invisible' when in `notmuch-show-mode' mode."
+  (if (eq major-mode 'notmuch-show-mode)
+      (setq ad-return-value (notmuch-isearch-range-invisible beg end))
+    ad-do-it))
+
+;;
+
 (provide 'notmuch-wash)
-- 
1.7.5.1

Re: [PATCH 2/2] Workaround for Emacs bug #8721.

[Dmitry Kurochkin]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]

This is an amended patch which uses `isearch-range-invisible' from Emacs
trunk r104393 (includes the second patch from #8721).

Regards,
  Dmitry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Workaround-for-Emacs-bug-8721.patch --]
[-- Type: text/x-diff, Size: 3430 bytes --]

From d56f2c308c995639aad04f0e5388b52912e603b0 Mon Sep 17 00:00:00 2001
From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Date: Fri, 27 May 2011 01:35:09 +0400
Subject: [PATCH] Workaround for Emacs bug #8721.

The patch adds `notmuch-isearch-range-invisible' function which
is the same as `isearch-range-invisible' but with fixed Emacs bug
 #8721.  Advice added for `isearch-range-invisible' which calls
`notmuch-isearch-range-invisible' instead of the original
`isearch-range-invisible' when in `notmuch-show-mode'.
---
 emacs/notmuch-wash.el |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 992fa8f..e046d62 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -298,4 +298,71 @@ for error."
 
 ;;
 
+;; Temporary workaround for Emacs bug #8721
+;; http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8721
+
+(defun notmuch-isearch-range-invisible (beg end)
+  "Same as `isearch-range-invisible' but with fixed Emacs bug #8721."
+  (when (/= beg end)
+    ;; Check that invisibility runs up to END.
+    (save-excursion
+      (goto-char beg)
+      (let (;; can-be-opened keeps track if we can open some overlays.
+	    (can-be-opened (eq search-invisible 'open))
+	    ;; the list of overlays that could be opened
+	    (crt-overlays nil))
+	(when (and can-be-opened isearch-hide-immediately)
+	  (isearch-close-unnecessary-overlays beg end))
+	;; If the following character is currently invisible,
+	;; skip all characters with that same `invisible' property value.
+	;; Do that over and over.
+	(while (and (< (point) end) (invisible-p (point)))
+	  (if (invisible-p (get-text-property (point) 'invisible))
+	      (progn
+		(goto-char (next-single-property-change (point) 'invisible
+							nil end))
+		;; if text is hidden by an `invisible' text property
+		;; we cannot open it at all.
+		(setq can-be-opened nil))
+	    (when can-be-opened
+	      (let ((overlays (overlays-at (point)))
+		    ov-list
+		    o
+		    invis-prop)
+		(while overlays
+		  (setq o (car overlays)
+			invis-prop (overlay-get o 'invisible))
+		  (if (invisible-p invis-prop)
+		      (if (overlay-get o 'isearch-open-invisible)
+			  (setq ov-list (cons o ov-list))
+			;; We found one overlay that cannot be
+			;; opened, that means the whole chunk
+			;; cannot be opened.
+			(setq can-be-opened nil)))
+		  (setq overlays (cdr overlays)))
+		(if can-be-opened
+		    ;; It makes sense to append to the open
+		    ;; overlays list only if we know that this is
+		    ;; t.
+		    (setq crt-overlays (append ov-list crt-overlays)))))
+	    (goto-char (next-overlay-change (point)))))
+	;; See if invisibility reaches up thru END.
+	(if (>= (point) end)
+	    (if (and can-be-opened (consp crt-overlays))
+		(progn
+		  (setq isearch-opened-overlays
+			(append isearch-opened-overlays crt-overlays))
+		  (mapc 'isearch-open-overlay-temporary crt-overlays)
+		  nil)
+	      (setq isearch-hidden t)))))))
+
+(defadvice isearch-range-invisible (around notmuch-isearch-range-invisible-advice activate)
+  "Call `notmuch-isearch-range-invisible' instead of the original
+`isearch-range-invisible' when in `notmuch-show-mode' mode."
+  (if (eq major-mode 'notmuch-show-mode)
+      (setq ad-return-value (notmuch-isearch-range-invisible beg end))
+    ad-do-it))
+
+;;
+
 (provide 'notmuch-wash)
-- 
1.7.5.1

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Fri, 27 May 2011 01:42:22 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I have just send two more patches to this thread.  One with new tests.
> Another with a workaround for the bug.  The workaround should not break
> anything since it affects only notmuch-show mode.

Thanks!

I love committing tests that demonstrate broken code before committing
fixes. As it happened here, I committed these two new patches thinking I
had previously committed the earlier patches in the series. Fortunately,
the failure of the test pointed out that I was missing the actual fix.

I also really like the workaround to avoid regressing functionality
because of an emacs bug.

Well done, Dmitry. I've now pushed out everything in this series.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Wed, 15 Jun 2011 07:06:40 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Fri, 27 May 2011 01:42:22 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I have just send two more patches to this thread.  One with new tests.
> > Another with a workaround for the bug.  The workaround should not break
> > anything since it affects only notmuch-show mode.
> 
> Thanks!
> 
> I love committing tests that demonstrate broken code before committing
> fixes. As it happened here, I committed these two new patches thinking I
> had previously committed the earlier patches in the series. Fortunately,
> the failure of the test pointed out that I was missing the actual fix.
> 

I know you prefer tests to go before patches and I agree with that.  But
most of the time I do tests after coding.  I do not know an easy way to
reorder patches in git.  (Also I do not know how to amend an old patch,
wish more darcs features in git.)  Hopefully it is not a big trouble for
you to reorder the patches when applying.

> I also really like the workaround to avoid regressing functionality
> because of an emacs bug.
> 

indeed

> Well done, Dmitry. I've now pushed out everything in this series.
> 

Thanks.

Regards,
  Dmitry

> -Carl
> 
> -- 
> carl.d.worth@intel.com

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I know you prefer tests to go before patches and I agree with that.  But
> most of the time I do tests after coding.  I do not know an easy way to
> reorder patches in git.  (Also I do not know how to amend an old patch,
> wish more darcs features in git.)  Hopefully it is not a big trouble for
> you to reorder the patches when applying.

Hey, Dmitry.  I usually just pop in to a new branch rooted before my new
patch series, and then cherry-pick the new patches onto the branch in
the order I want (eg. tests first, etc.).

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]

On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> I know you prefer tests to go before patches and I agree with that.

Great!

> But most of the time I do tests after coding.

Yes, I do that order almost exclusively as well.

> I do not know an easy way to reorder patches in git.  (Also I do not
> know how to amend an old patch

Fortunately, git has a great feature here for both use cases, (git
rebase -i). Here's the simple recipe:

    * Find a bug, fix a bug, commit

    * Write a test case, commit

    * Run the following command:

	git rebase -i origin/master

At this point you'll be presented with an editor window giving one line
for each commit that you have made since origin/master. You can reorder
these lines however you'd like. When you save and exit the editor, the
commits will be applied in the order you saved.

If there are any conflicts due to the re-ordering, then git rebase will
stop and tell you what to do, which will be:

    * Resolve the conflict

    * Run "git add" on the files you edited

    * Run "git rebase --continue"

Also, back when editing the original list of commits, you can change the
word "apply" next to any particular commit to change what happens when
applying it. If you change that to "reword" you'll be given an editor
window to edit the commit message. If you use "edit" then you'll be
dropped to a shell where you can:

    * Edit the code

    * Test as necessary

    * Run "git commit --amend"

    * Run "git rebase --continue"

I absolutely love "git rebase -i". It's one of my favorite
user-interface features in git.

> wish more darcs features in git.

I don't know about "git rebase -i", but I think I heard that "git add
-i", (interactively add some portions of the dirty working tree to the
index to be committed). I think the menu-based interface of "git add -i"
is particularly clunky. But I love the trimmed-down interface of "git
add -p" which simply prompts one-patch-hunk-at-a-time for pieces to add
to the next commit. It even supports splitting a hunk, (or even manually
editing the patch to trim it down!). It's pretty slick stuff.

So there are some git tips that might be useful.

> Thanks.

You're quite welcome. Thanks for all the great work. Please keep it up!

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.

[Dmitry Kurochkin]

On Wed, 15 Jun 2011 10:00:36 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > I know you prefer tests to go before patches and I agree with that.
> 
> Great!
> 
> > But most of the time I do tests after coding.
> 
> Yes, I do that order almost exclusively as well.
> 
> > I do not know an easy way to reorder patches in git.  (Also I do not
> > know how to amend an old patch
> 
> Fortunately, git has a great feature here for both use cases, (git
> rebase -i). Here's the simple recipe:
> 
>     * Find a bug, fix a bug, commit
> 
>     * Write a test case, commit
> 
>     * Run the following command:
> 
> 	git rebase -i origin/master
> 
> At this point you'll be presented with an editor window giving one line
> for each commit that you have made since origin/master. You can reorder
> these lines however you'd like. When you save and exit the editor, the
> commits will be applied in the order you saved.
> 
> If there are any conflicts due to the re-ordering, then git rebase will
> stop and tell you what to do, which will be:
> 
>     * Resolve the conflict
> 
>     * Run "git add" on the files you edited
> 
>     * Run "git rebase --continue"
> 
> Also, back when editing the original list of commits, you can change the
> word "apply" next to any particular commit to change what happens when
> applying it. If you change that to "reword" you'll be given an editor
> window to edit the commit message. If you use "edit" then you'll be
> dropped to a shell where you can:
> 
>     * Edit the code
> 
>     * Test as necessary
> 
>     * Run "git commit --amend"
> 
>     * Run "git rebase --continue"
> 
> I absolutely love "git rebase -i". It's one of my favorite
> user-interface features in git.
> 

Thanks for this.  I did not know about interactive mode in rebase.

This is some sort of replacement for darcs amend (which allows editing
any patch, not just the last one).

> > wish more darcs features in git.
> 
> I don't know about "git rebase -i", but I think I heard that "git add
> -i", (interactively add some portions of the dirty working tree to the
> index to be committed). I think the menu-based interface of "git add -i"
> is particularly clunky. But I love the trimmed-down interface of "git
> add -p" which simply prompts one-patch-hunk-at-a-time for pieces to add
> to the next commit. It even supports splitting a hunk, (or even manually
> editing the patch to trim it down!). It's pretty slick stuff.
> 

Yes, "add -i" is ugl... confusing, but "add -p" is very nice.  A great
feature of darcs picked up by git.

> So there are some git tips that might be useful.
> 

They will be useful indeed.  Thanks!

Regards,
  Dmitry

> > Thanks.
> 
> You're quite welcome. Thanks for all the great work. Please keep it up!
> 
> -Carl
> 
> -- 
> carl.d.worth@intel.com