[PATCH 1/3] test: `notmuch-show-advance-and-archive' with invisible signature

[PATCH 1/3] test: `notmuch-show-advance-and-archive' with invisible signature

[Dmitry Kurochkin]

Add Emacs test to check that `notmuch-show-advance-and-archive'
works for the last message in thread with invisible signature.
---

This patch series fixes the bug reported by Sebastien in [1].  I
was able to reproduce it and confirm that the second patch from
this series fixes the problem.  Unfortunately, I can not explain
why it fixes it.  The patch uses a cleaner approach for visible
text search.  But the old approach should work fine as well.
Apparently, it does not work when `invisible' property is not a
single symbol but a list (which was changed in
95ef8da29439f2e79115c36ab4d2a80aef1a1462).  I suspect that it is
an Emacs bug.  I plan to look at it later.

Another issue is that the test does not demonstrate the bug.
Again, I do not really know why.  It passes both before and after
the fix.  Although if I run the test commands by hand I hit the
bug.  I guess it has something to do with emacs daemon mode when
the buffer is not visible.  I hope someone with a better elisp
knowledge can tell what is going on and how to make the test
work.

I believe patches 2 and 3 can be pushed after review even without
a working test.

Regards,
  Dmitry

[1] id:"8739j5rn2d.fsf@cern.ch"

 test/emacs |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/test/emacs b/test/emacs
index e59de47..65a96a5 100755
--- a/test/emacs
+++ b/test/emacs
@@ -347,4 +347,16 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
 	    (test-visible-output)'
 test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-thread-with-hidden-messages
 
+test_begin_subtest 'notmuch-show-advance-and-archive with invisible signature'
+message1='id:20091118010116.GC25380@dottiness.seas.harvard.edu'
+message2='id:1258491078-29658-1-git-send-email-dottedmag@dottedmag.net'
+test_emacs "(notmuch-search \"$message1 or $message2\")
+	    (notmuch-test-wait)
+	    (notmuch-search-show-thread)
+	    (notmuch-show-advance-and-archive)
+	    (test-output)"
+test_emacs "(notmuch-show \"$message2\")
+	    (test-output \"EXPECTED\")"
+test_expect_equal_file OUTPUT EXPECTED
+
 test_done
-- 
1.7.5.4

[PATCH 2/3] emacs: improve hidden signatures handling in notmuch-show-advance-and-archive

[Dmitry Kurochkin]

Use `previous-single-char-property-change' instead of going
through each character by hand and testing it's visibility.  This
fixes `notmuch-show-advance-and-archive' to work for the last
message in thread with hidden signature.
---
 emacs/notmuch-show.el |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 6685717..ad3cc7b 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1113,17 +1113,18 @@ thread, (remove the \"inbox\" tag from each message). Also kill
 this buffer, and display the next thread from the search from
 which this thread was originally shown."
   (interactive)
-  (let ((end-of-this-message (notmuch-show-message-bottom)))
+  (let* ((end-of-this-message (notmuch-show-message-bottom))
+	 (visible-end-of-this-message (1- end-of-this-message)))
+    (while (invisible-p visible-end-of-this-message)
+      (setq visible-end-of-this-message
+	    (previous-single-char-property-change visible-end-of-this-message
+						  'invisible)))
     (cond
      ;; Ideally we would test `end-of-this-message' against the result
      ;; of `window-end', but that doesn't account for the fact that
-     ;; the end of the message might be hidden, so we have to actually
-     ;; go to the end, walk back over invisible text and then see if
-     ;; point is visible.
-     ((save-excursion
-	(goto-char (- end-of-this-message 1))
-	(notmuch-show-move-past-invisible-backward)
-	(> (point) (window-end)))
+     ;; the end of the message might be hidden.
+     ((and visible-end-of-this-message
+	   (> visible-end-of-this-message (window-end)))
       ;; The bottom of this message is not visible - scroll.
       (scroll-up nil))
 
-- 
1.7.5.4

[PATCH 3/3] emacs: remove no longer used functions from notmuch-show.el

[Dmitry Kurochkin]

Remove `notmuch-show-move-past-invisible-backward' and
`notmuch-show-move-past-invisible-forward' functions which are
unused.
---
 emacs/notmuch-show.el |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index ad3cc7b..dcaea65 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -977,14 +977,6 @@ All currently available key bindings:
     (notmuch-show-move-to-message-top)
     t))
 
-(defun notmuch-show-move-past-invisible-forward ()
-  (while (point-invisible-p)
-    (forward-char)))
-
-(defun notmuch-show-move-past-invisible-backward ()
-  (while (point-invisible-p)
-    (backward-char)))
-
 ;; Functions relating to the visibility of messages and their
 ;; components.
 
-- 
1.7.5.4

[PATCH] emacs: remove unused `point-invisible-p' function

[Dmitry Kurochkin]

`point-invisible-p' does not work correctly when `invisible'
property is a list.  There are standard `invisible-p' and related
functions that should be used instead.
---
 emacs/notmuch-lib.el |   15 ---------------
 1 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index f93c957..0f856bf 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -105,21 +105,6 @@ the user hasn't set this variable with the old or new value."
 
 ;;
 
-;; XXX: This should be a generic function in emacs somewhere, not
-;; here.
-(defun point-invisible-p ()
-  "Return whether the character at point is invisible.
-
-Here visibility is determined by `buffer-invisibility-spec' and
-the invisible property of any overlays for point. It doesn't have
-anything to do with whether point is currently being displayed
-within the current window."
-  (let ((prop (get-char-property (point) 'invisible)))
-    (if (eq buffer-invisibility-spec t)
-	prop
-      (or (memq prop buffer-invisibility-spec)
-	  (assq prop buffer-invisibility-spec)))))
-
 (defun notmuch-remove-if-not (predicate list)
   "Return a copy of LIST with all items not satisfying PREDICATE removed."
   (let (out)
-- 
1.7.5.4

Re: [PATCH 1/3] test: `notmuch-show-advance-and-archive' with invisible signature

[Dmitry Kurochkin]

On Wed, 29 Jun 2011 05:48:50 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Add Emacs test to check that `notmuch-show-advance-and-archive'
> works for the last message in thread with invisible signature.
> ---
> 
> This patch series fixes the bug reported by Sebastien in [1].  I
> was able to reproduce it and confirm that the second patch from
> this series fixes the problem.  Unfortunately, I can not explain
> why it fixes it.  The patch uses a cleaner approach for visible
> text search.  But the old approach should work fine as well.
> Apparently, it does not work when `invisible' property is not a
> single symbol but a list (which was changed in
> 95ef8da29439f2e79115c36ab4d2a80aef1a1462).  I suspect that it is
> an Emacs bug.  I plan to look at it later.
> 

Turns out that `point-invisible-p' is a function from notmuch-lib.el, I
did not realize that before.  It implements a custom visibility check
which is incomplete and does not work correctly when `invisible'
property is a list.  That is why the previous code (which used
`point-invisible-p') had the bug.  I sent another patch that removes
`point-invisible-p' function.

> Another issue is that the test does not demonstrate the bug.
> Again, I do not really know why.  It passes both before and after
> the fix.  Although if I run the test commands by hand I hit the
> bug.  I guess it has something to do with emacs daemon mode when
> the buffer is not visible.  I hope someone with a better elisp
> knowledge can tell what is going on and how to make the test
> work.
> 

Now it is clear where the bug was.  Remaining question is how to test
it.

Regards,
  Dmitry

> I believe patches 2 and 3 can be pushed after review even without
> a working test.
> 
> Regards,
>   Dmitry
> 
> [1] id:"8739j5rn2d.fsf@cern.ch"
> 
>  test/emacs |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/test/emacs b/test/emacs
> index e59de47..65a96a5 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -347,4 +347,16 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
>  	    (test-visible-output)'
>  test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-thread-with-hidden-messages
>  
> +test_begin_subtest 'notmuch-show-advance-and-archive with invisible signature'
> +message1='id:20091118010116.GC25380@dottiness.seas.harvard.edu'
> +message2='id:1258491078-29658-1-git-send-email-dottedmag@dottedmag.net'
> +test_emacs "(notmuch-search \"$message1 or $message2\")
> +	    (notmuch-test-wait)
> +	    (notmuch-search-show-thread)
> +	    (notmuch-show-advance-and-archive)
> +	    (test-output)"
> +test_emacs "(notmuch-show \"$message2\")
> +	    (test-output \"EXPECTED\")"
> +test_expect_equal_file OUTPUT EXPECTED
> +
>  test_done
> -- 
> 1.7.5.4
> 

Re: [PATCH 1/3] test: `notmuch-show-advance-and-archive' with invisible signature

[Jani Nikula]

On Wed, 29 Jun 2011 09:10:19 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Wed, 29 Jun 2011 05:48:50 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > This patch series fixes the bug reported by Sebastien in [1].  I
> > was able to reproduce it and confirm that the second patch from
> > this series fixes the problem.  Unfortunately, I can not explain
> > why it fixes it.  The patch uses a cleaner approach for visible
> > text search.  But the old approach should work fine as well.
> > Apparently, it does not work when `invisible' property is not a
> > single symbol but a list (which was changed in
> > 95ef8da29439f2e79115c36ab4d2a80aef1a1462).  I suspect that it is
> > an Emacs bug.  I plan to look at it later.
> > 
> 
> Turns out that `point-invisible-p' is a function from notmuch-lib.el, I
> did not realize that before.  It implements a custom visibility check
> which is incomplete and does not work correctly when `invisible'
> property is a list.  That is why the previous code (which used
> `point-invisible-p') had the bug.  I sent another patch that removes
> `point-invisible-p' function.
> 
> > Another issue is that the test does not demonstrate the bug.
> > Again, I do not really know why.  It passes both before and after
> > the fix.  Although if I run the test commands by hand I hit the
> > bug.  I guess it has something to do with emacs daemon mode when
> > the buffer is not visible.  I hope someone with a better elisp
> > knowledge can tell what is going on and how to make the test
> > work.
> > 
> 
> Now it is clear where the bug was.  Remaining question is how to test
> it.

Hi, I applied the series, and I can confirm it fixes the bug. Hiding of
messages also seems to work as expected, including the un-hidden
signatures, which is what the commit that introduced this bug originally
fixed. Many thanks. I have no insights on the automated tests, though.

Jani

Re: [PATCH 1/3] test: `notmuch-show-advance-and-archive' with invisible signature

[Dmitry Kurochkin]

On Wed, 29 Jun 2011 09:10:19 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Wed, 29 Jun 2011 05:48:50 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Add Emacs test to check that `notmuch-show-advance-and-archive'
> > works for the last message in thread with invisible signature.
> > ---
> > 
> > This patch series fixes the bug reported by Sebastien in [1].  I
> > was able to reproduce it and confirm that the second patch from
> > this series fixes the problem.  Unfortunately, I can not explain
> > why it fixes it.  The patch uses a cleaner approach for visible
> > text search.  But the old approach should work fine as well.
> > Apparently, it does not work when `invisible' property is not a
> > single symbol but a list (which was changed in
> > 95ef8da29439f2e79115c36ab4d2a80aef1a1462).  I suspect that it is
> > an Emacs bug.  I plan to look at it later.
> > 
> 
> Turns out that `point-invisible-p' is a function from notmuch-lib.el, I
> did not realize that before.  It implements a custom visibility check
> which is incomplete and does not work correctly when `invisible'
> property is a list.  That is why the previous code (which used
> `point-invisible-p') had the bug.  I sent another patch that removes
> `point-invisible-p' function.
> 
> > Another issue is that the test does not demonstrate the bug.
> > Again, I do not really know why.  It passes both before and after
> > the fix.  Although if I run the test commands by hand I hit the
> > bug.  I guess it has something to do with emacs daemon mode when
> > the buffer is not visible.  I hope someone with a better elisp
> > knowledge can tell what is going on and how to make the test
> > work.
> > 
> 
> Now it is clear where the bug was.  Remaining question is how to test
> it.
> 

I have posted a new version of this patch series [1].  It fixes the test
to actually demonstrate the bug.

Regards,
  Dmitry

[1] id:"1309376558-26284-1-git-send-email-dmitry.kurochkin@gmail.com"

> Regards,
>   Dmitry
> 
> > I believe patches 2 and 3 can be pushed after review even without
> > a working test.
> > 
> > Regards,
> >   Dmitry
> > 
> > [1] id:"8739j5rn2d.fsf@cern.ch"
> > 
> >  test/emacs |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/test/emacs b/test/emacs
> > index e59de47..65a96a5 100755
> > --- a/test/emacs
> > +++ b/test/emacs
> > @@ -347,4 +347,16 @@ test_emacs '(notmuch-show "id:f35dbb950911171438k5df6eb56k77b6c0944e2e79ae@mail.
> >  	    (test-visible-output)'
> >  test_expect_equal_file OUTPUT $EXPECTED/notmuch-show-thread-with-hidden-messages
> >  
> > +test_begin_subtest 'notmuch-show-advance-and-archive with invisible signature'
> > +message1='id:20091118010116.GC25380@dottiness.seas.harvard.edu'
> > +message2='id:1258491078-29658-1-git-send-email-dottedmag@dottedmag.net'
> > +test_emacs "(notmuch-search \"$message1 or $message2\")
> > +	    (notmuch-test-wait)
> > +	    (notmuch-search-show-thread)
> > +	    (notmuch-show-advance-and-archive)
> > +	    (test-output)"
> > +test_emacs "(notmuch-show \"$message2\")
> > +	    (test-output \"EXPECTED\")"
> > +test_expect_equal_file OUTPUT EXPECTED
> > +
> >  test_done
> > -- 
> > 1.7.5.4
> >