[PATCH 1/1] emacs: Take more care when hiding regions with buttons.

[PATCH 1/1] emacs: Take more care when hiding regions with buttons.

[David Edmondson]

If the region to be hidden with a button by
`notmuch-wash-region-to-button' starts at the beginning of the buffer,
the invisible region will include the inserted button. This is
unfortunate, as it means that it is not possible to see the button to
be pressed.

Make a little space at the start of the buffer before inserting the
button to avoid this, not forgetting to remove the inserted space upon
completion.
---

This is a hack, but I couldn't see another way around it. Can anyone
find a better solution?

 emacs/notmuch-wash.el |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 5c1e830..4afd3b3 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -147,25 +147,44 @@ insert before the button, probably for indentation."
   ;; symbols because of the way the button code works. Note that
   ;; replacing intern-soft with make-symbol will cause this to fail,
   ;; 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 (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
-      (goto-char (1- beg))
-      (insert prefix)
-      (insert-button (notmuch-wash-button-label overlay)
-		     'invisibility-spec invis-spec
-		     'overlay overlay
-		     :type button-type))))
+  (save-excursion
+    ;; If the beginning of the region to be converted to a button is the
+    ;; beginning of the buffer we must move forward a little to avoid
+    ;; creating an overlay that will hide the button intended to be used
+    ;; to reveal the hidden region.
+    (let (scene-of-crime)
+      (when (eq beg (point-min))
+	(goto-char (point-min))
+	(insert "\n")
+	(setq scene-of-crime (point-min)
+	      beg (point)))
+
+      ;; This uses some slightly tricky conversions between strings and
+      ;; symbols because of the way the button code works. Note that
+      ;; replacing intern-soft with make-symbol will cause this to fail,
+      ;; 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 (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- beg))
+	(insert prefix)
+	(insert-button (notmuch-wash-button-label overlay)
+		       'invisibility-spec invis-spec
+		       'overlay overlay
+		       :type button-type))
+
+      (when scene-of-crime
+      	(goto-char scene-of-crime)
+      	(delete-char 1)))))
 
 (defun notmuch-wash-excerpt-citations (msg depth)
   "Excerpt citations and up to one signature."
-- 
1.7.8.3

Re: [PATCH 1/1] emacs: Take more care when hiding regions with buttons.

[Dmitry Kurochkin]

On Wed, 25 Jan 2012 15:05:08 +0000, David Edmondson <dme@dme.org> wrote:
> If the region to be hidden with a button by
> `notmuch-wash-region-to-button' starts at the beginning of the buffer,
> the invisible region will include the inserted button. This is
> unfortunate, as it means that it is not possible to see the button to
> be pressed.
> 
> Make a little space at the start of the buffer before inserting the
> button to avoid this, not forgetting to remove the inserted space upon
> completion.
> ---
> 
> This is a hack, but I couldn't see another way around it. Can anyone
> find a better solution?
> 

I think it would be much easier to understand the problem and probably
suggest a solution if there is a test :)

Regards,
  Dmitry

>  emacs/notmuch-wash.el |   57 ++++++++++++++++++++++++++++++++----------------
>  1 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
> index 5c1e830..4afd3b3 100644
> --- a/emacs/notmuch-wash.el
> +++ b/emacs/notmuch-wash.el
> @@ -147,25 +147,44 @@ insert before the button, probably for indentation."
>    ;; symbols because of the way the button code works. Note that
>    ;; replacing intern-soft with make-symbol will cause this to fail,
>    ;; 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 (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
> -      (goto-char (1- beg))
> -      (insert prefix)
> -      (insert-button (notmuch-wash-button-label overlay)
> -		     'invisibility-spec invis-spec
> -		     'overlay overlay
> -		     :type button-type))))
> +  (save-excursion
> +    ;; If the beginning of the region to be converted to a button is the
> +    ;; beginning of the buffer we must move forward a little to avoid
> +    ;; creating an overlay that will hide the button intended to be used
> +    ;; to reveal the hidden region.
> +    (let (scene-of-crime)
> +      (when (eq beg (point-min))
> +	(goto-char (point-min))
> +	(insert "\n")
> +	(setq scene-of-crime (point-min)
> +	      beg (point)))
> +
> +      ;; This uses some slightly tricky conversions between strings and
> +      ;; symbols because of the way the button code works. Note that
> +      ;; replacing intern-soft with make-symbol will cause this to fail,
> +      ;; 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 (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- beg))
> +	(insert prefix)
> +	(insert-button (notmuch-wash-button-label overlay)
> +		       'invisibility-spec invis-spec
> +		       'overlay overlay
> +		       :type button-type))
> +
> +      (when scene-of-crime
> +      	(goto-char scene-of-crime)
> +      	(delete-char 1)))))
>  
>  (defun notmuch-wash-excerpt-citations (msg depth)
>    "Excerpt citations and up to one signature."
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/1] emacs: Take more care when hiding regions with buttons.

[David Edmondson]

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

On Wed, 25 Jan 2012 19:10:35 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Wed, 25 Jan 2012 15:05:08 +0000, David Edmondson <dme@dme.org> wrote:
> > If the region to be hidden with a button by
> > `notmuch-wash-region-to-button' starts at the beginning of the buffer,
> > the invisible region will include the inserted button. This is
> > unfortunate, as it means that it is not possible to see the button to
> > be pressed.
> > 
> > Make a little space at the start of the buffer before inserting the
> > button to avoid this, not forgetting to remove the inserted space upon
> > completion.
> > ---
> > 
> > This is a hack, but I couldn't see another way around it. Can anyone
> > find a better solution?
> > 
> 
> I think it would be much easier to understand the problem and probably
> suggest a solution if there is a test :)

Wow, and I have that new test infrastructure to use!

> 
> Regards,
>   Dmitry
> 
> >  emacs/notmuch-wash.el |   57 ++++++++++++++++++++++++++++++++----------------
> >  1 files changed, 38 insertions(+), 19 deletions(-)
> > 
> > diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
> > index 5c1e830..4afd3b3 100644
> > --- a/emacs/notmuch-wash.el
> > +++ b/emacs/notmuch-wash.el
> > @@ -147,25 +147,44 @@ insert before the button, probably for indentation."
> >    ;; symbols because of the way the button code works. Note that
> >    ;; replacing intern-soft with make-symbol will cause this to fail,
> >    ;; 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 (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
> > -      (goto-char (1- beg))
> > -      (insert prefix)
> > -      (insert-button (notmuch-wash-button-label overlay)
> > -		     'invisibility-spec invis-spec
> > -		     'overlay overlay
> > -		     :type button-type))))
> > +  (save-excursion
> > +    ;; If the beginning of the region to be converted to a button is the
> > +    ;; beginning of the buffer we must move forward a little to avoid
> > +    ;; creating an overlay that will hide the button intended to be used
> > +    ;; to reveal the hidden region.
> > +    (let (scene-of-crime)
> > +      (when (eq beg (point-min))
> > +	(goto-char (point-min))
> > +	(insert "\n")
> > +	(setq scene-of-crime (point-min)
> > +	      beg (point)))
> > +
> > +      ;; This uses some slightly tricky conversions between strings and
> > +      ;; symbols because of the way the button code works. Note that
> > +      ;; replacing intern-soft with make-symbol will cause this to fail,
> > +      ;; 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 (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- beg))
> > +	(insert prefix)
> > +	(insert-button (notmuch-wash-button-label overlay)
> > +		       'invisibility-spec invis-spec
> > +		       'overlay overlay
> > +		       :type button-type))
> > +
> > +      (when scene-of-crime
> > +      	(goto-char scene-of-crime)
> > +      	(delete-char 1)))))
> >  
> >  (defun notmuch-wash-excerpt-citations (msg depth)
> >    "Excerpt citations and up to one signature."
> > -- 
> > 1.7.8.3
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

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

[PATCH 0/4 v2] more care when hiding regions with buttons

[David Edmondson]

Added a test, which required some improvements to the test harness.

The fix still seems awkward.

David Edmondson (4):
  test: `visible-buffer-substring' should not return text properties.
  test: `notmuch-test-run' should protect against buffer switching.
  test: Add test for Original Message hiding at point-min.
  emacs: Take more care when hiding regions with buttons.

 emacs/notmuch-wash.el                 |   57 ++++++++++++++++++++++-----------
 test/emacs-original-message-hiding.el |   15 +++++++++
 test/emacs-original-message-hiding.sh |   19 +++++++++++
 test/notmuch-test                     |    1 +
 test/test-lib.el                      |    6 ++-
 5 files changed, 77 insertions(+), 21 deletions(-)
 create mode 100644 test/emacs-original-message-hiding.el
 create mode 100755 test/emacs-original-message-hiding.sh

-- 
1.7.8.3

[PATCH 1/4 v2] test: `visible-buffer-substring' should not return text properties.

[David Edmondson]

When using `visible-buffer-substring' to examine a buffer, the text
properties are not useful, so don't include them.
---
 test/test-lib.el |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index bc75f06..36e793a 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -60,7 +60,7 @@ FILENAME is OUTPUT."
     (while (< start end)
       (let ((next-pos (next-char-property-change start end)))
 	(when (not (invisible-p start))
-	  (setq str (concat str (buffer-substring start next-pos))))
+	  (setq str (concat str (buffer-substring-no-properties start next-pos))))
 	(setq start next-pos)))
     str))
 
-- 
1.7.8.3

[PATCH 2/4 v2] test: `notmuch-test-run' should protect against buffer switching.

[David Edmondson]

The body of the test may cause the current buffer to change. Ensure
that the output goes to the correct buffer by switching back before
inserting it.
---
 test/test-lib.el |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index 36e793a..0efb02a 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -89,7 +89,9 @@ nothing."
 (defmacro notmuch-test-run (&rest body)
   "Evaluate a BODY of test expressions and output the result."
   `(with-temp-buffer
-     (let ((result (progn ,@body)))
+     (let ((buffer (current-buffer))
+	   (result (progn ,@body)))
+       (switch-to-buffer buffer)
        (insert (if (stringp result)
 		   result
 		 (prin1-to-string result)))
-- 
1.7.8.3

[PATCH 3/4 v2] test: Add test for Original Message hiding at point-min.

[David Edmondson]

---
 test/emacs-original-message-hiding.el |   15 +++++++++++++++
 test/emacs-original-message-hiding.sh |   20 ++++++++++++++++++++
 test/notmuch-test                     |    1 +
 3 files changed, 36 insertions(+), 0 deletions(-)
 create mode 100644 test/emacs-original-message-hiding.el
 create mode 100755 test/emacs-original-message-hiding.sh

diff --git a/test/emacs-original-message-hiding.el b/test/emacs-original-message-hiding.el
new file mode 100644
index 0000000..bbacbeb
--- /dev/null
+++ b/test/emacs-original-message-hiding.el
@@ -0,0 +1,15 @@
+(defun notmuch-test-original-message-hiding ()
+  (let ((notmuch-show-insert-text/plain-hook '(notmuch-wash-excerpt-citations))
+	(expected "\
+Sender <sender@example.com> (2010-01-05) (inbox)
+Subject: hiding an Original Message
+To: mailinglist@notmuchmail.org
+Date: Tue, 05 Jan 2010 15:43:56 -0000
+
+[ 2-line hidden original message. Click/Enter to show. ]
+
+")
+	output)
+    (notmuch-show "id:\"test_message@test.org\"")
+    (setq output (visible-buffer-string))
+    (notmuch-test-expect-equal output expected)))
diff --git a/test/emacs-original-message-hiding.sh b/test/emacs-original-message-hiding.sh
new file mode 100755
index 0000000..01cf98d
--- /dev/null
+++ b/test/emacs-original-message-hiding.sh
@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+
+test_description="emacs Original Message hiding"
+. test-lib.sh
+
+test_begin_subtest "Hiding an Original Message region at point-min"
+add_message \
+    '[id]="test_message@test.org"' \
+    '[from]="Sender <sender@example.com>"' \
+    '[to]=mailinglist@notmuchmail.org' \
+    '[subject]="hiding an Original Message"' \
+    '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
+    '[body]="-----Original Message-----
+Text here.
+"'
+test_subtest_known_broken
+test_emacs_expect_t \
+    '(load "emacs-original-message-hiding.el") (notmuch-test-original-message-hiding)'
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index 3f1740c..af132fc 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -54,6 +54,7 @@ TESTS="
   argument-parsing
   emacs-test-functions.sh
   emacs-address-cleaning.sh
+  emacs-original-message-hiding.sh
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.8.3

[PATCH 4/4 v2] emacs: Take more care when hiding regions with buttons.

[David Edmondson]

If the region to be hidden with a button by
`notmuch-wash-region-to-button' starts at the beginning of the buffer,
the invisible region will include the inserted button. This is
unfortunate, as it means that it is not possible to see the button to
be pressed.

Make a little space at the start of the buffer before inserting the
button to avoid this, not forgetting to remove the inserted space upon
completion.

Mark the relevant test as no longer broken.
---
 emacs/notmuch-wash.el                 |   57 ++++++++++++++++++++++-----------
 test/emacs-original-message-hiding.sh |    1 -
 2 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 5c1e830..4afd3b3 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -147,25 +147,44 @@ insert before the button, probably for indentation."
   ;; symbols because of the way the button code works. Note that
   ;; replacing intern-soft with make-symbol will cause this to fail,
   ;; 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 (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
-      (goto-char (1- beg))
-      (insert prefix)
-      (insert-button (notmuch-wash-button-label overlay)
-		     'invisibility-spec invis-spec
-		     'overlay overlay
-		     :type button-type))))
+  (save-excursion
+    ;; If the beginning of the region to be converted to a button is the
+    ;; beginning of the buffer we must move forward a little to avoid
+    ;; creating an overlay that will hide the button intended to be used
+    ;; to reveal the hidden region.
+    (let (scene-of-crime)
+      (when (eq beg (point-min))
+	(goto-char (point-min))
+	(insert "\n")
+	(setq scene-of-crime (point-min)
+	      beg (point)))
+
+      ;; This uses some slightly tricky conversions between strings and
+      ;; symbols because of the way the button code works. Note that
+      ;; replacing intern-soft with make-symbol will cause this to fail,
+      ;; 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 (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- beg))
+	(insert prefix)
+	(insert-button (notmuch-wash-button-label overlay)
+		       'invisibility-spec invis-spec
+		       'overlay overlay
+		       :type button-type))
+
+      (when scene-of-crime
+      	(goto-char scene-of-crime)
+      	(delete-char 1)))))
 
 (defun notmuch-wash-excerpt-citations (msg depth)
   "Excerpt citations and up to one signature."
diff --git a/test/emacs-original-message-hiding.sh b/test/emacs-original-message-hiding.sh
index 01cf98d..038ef4f 100755
--- a/test/emacs-original-message-hiding.sh
+++ b/test/emacs-original-message-hiding.sh
@@ -13,7 +13,6 @@ add_message \
     '[body]="-----Original Message-----
 Text here.
 "'
-test_subtest_known_broken
 test_emacs_expect_t \
     '(load "emacs-original-message-hiding.el") (notmuch-test-original-message-hiding)'
 
-- 
1.7.8.3

Re: [PATCH 1/4 v2] test: `visible-buffer-substring' should not return text properties.

[Dmitry Kurochkin]

On Wed, 25 Jan 2012 15:45:25 +0000, David Edmondson <dme@dme.org> wrote:
> When using `visible-buffer-substring' to examine a buffer, the text
> properties are not useful, so don't include them.
> ---
>  test/test-lib.el |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/test/test-lib.el b/test/test-lib.el
> index bc75f06..36e793a 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -60,7 +60,7 @@ FILENAME is OUTPUT."
>      (while (< start end)
>        (let ((next-pos (next-char-property-change start end)))
>  	(when (not (invisible-p start))
> -	  (setq str (concat str (buffer-substring start next-pos))))
> +	  (setq str (concat str (buffer-substring-no-properties start next-pos))))

I would wrap this line.  But would not insist :)

Please update `visible-buffer-substring' and `visible-buffer-string'
docstring accordingly.  Smth like:

  "Same as `buffer-substring-no-properties', but excludes invisible
  text."

  "Same as `buffer-string', but excludes invisible text and text
  properties."

Regards,
  Dmitry

>  	(setq start next-pos)))
>      str))
>  
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/4 v2] test: `notmuch-test-run' should protect against buffer switching.

[Dmitry Kurochkin]

On Wed, 25 Jan 2012 15:45:26 +0000, David Edmondson <dme@dme.org> wrote:
> The body of the test may cause the current buffer to change. Ensure
> that the output goes to the correct buffer by switching back before
> inserting it.

LGTM

Regards,
  Dmitry

> ---
>  test/test-lib.el |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/test/test-lib.el b/test/test-lib.el
> index 36e793a..0efb02a 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -89,7 +89,9 @@ nothing."
>  (defmacro notmuch-test-run (&rest body)
>    "Evaluate a BODY of test expressions and output the result."
>    `(with-temp-buffer
> -     (let ((result (progn ,@body)))
> +     (let ((buffer (current-buffer))
> +	   (result (progn ,@body)))
> +       (switch-to-buffer buffer)
>         (insert (if (stringp result)
>  		   result
>  		 (prin1-to-string result)))
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 3/4 v2] test: Add test for Original Message hiding at point-min.

[Dmitry Kurochkin]

On Wed, 25 Jan 2012 15:45:27 +0000, David Edmondson <dme@dme.org> wrote:
> ---
>  test/emacs-original-message-hiding.el |   15 +++++++++++++++
>  test/emacs-original-message-hiding.sh |   20 ++++++++++++++++++++
>  test/notmuch-test                     |    1 +
>  3 files changed, 36 insertions(+), 0 deletions(-)
>  create mode 100644 test/emacs-original-message-hiding.el
>  create mode 100755 test/emacs-original-message-hiding.sh
> 
> diff --git a/test/emacs-original-message-hiding.el b/test/emacs-original-message-hiding.el
> new file mode 100644
> index 0000000..bbacbeb
> --- /dev/null
> +++ b/test/emacs-original-message-hiding.el
> @@ -0,0 +1,15 @@
> +(defun notmuch-test-original-message-hiding ()
> +  (let ((notmuch-show-insert-text/plain-hook '(notmuch-wash-excerpt-citations))

Do we have to override the default value here?  I thought
notmuch-wash-excerpt-citations was enabled by default.

> +	(expected "\
> +Sender <sender@example.com> (2010-01-05) (inbox)
> +Subject: hiding an Original Message
> +To: mailinglist@notmuchmail.org
> +Date: Tue, 05 Jan 2010 15:43:56 -0000
> +
> +[ 2-line hidden original message. Click/Enter to show. ]
> +
> +")
> +	output)
> +    (notmuch-show "id:\"test_message@test.org\"")
> +    (setq output (visible-buffer-string))
> +    (notmuch-test-expect-equal output expected)))

IMO writing the test in lisp does not give any benefit in this case.
Quite the opposite: a simple test is split in two files and becomes more
complex.

But since we accepted this way of writing tests and you seem to prefer
it, I would not argue about changing it.

> diff --git a/test/emacs-original-message-hiding.sh b/test/emacs-original-message-hiding.sh
> new file mode 100755
> index 0000000..01cf98d
> --- /dev/null
> +++ b/test/emacs-original-message-hiding.sh

Let's rename this file to something a bit more general, e.g.
emacs-message-hiding.  Then we can put new tests (and move all
existing!) related to hiding message parts into it.

> @@ -0,0 +1,20 @@
> +#!/usr/bin/env bash
> +
> +test_description="emacs Original Message hiding"
> +. test-lib.sh
> +
> +test_begin_subtest "Hiding an Original Message region at point-min"
> +add_message \
> +    '[id]="test_message@test.org"' \

The message-id should be more unique to avoid collisions with other
tests.  IMO deriving message-id from the test description is a good
practice.  Also, consider assigning it to a variable and using it
instead of the string.  Since part of the test is in lisp, it is tricky
to use this variable in it.  Perhaps we should pass it as a parameter to
notmuch-test-original-message-hiding?  Or just do not bother and use
string constants :)  Your choice.

Regards,
  Dmitry

> +    '[from]="Sender <sender@example.com>"' \
> +    '[to]=mailinglist@notmuchmail.org' \
> +    '[subject]="hiding an Original Message"' \
> +    '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
> +    '[body]="-----Original Message-----
> +Text here.
> +"'
> +test_subtest_known_broken
> +test_emacs_expect_t \
> +    '(load "emacs-original-message-hiding.el") (notmuch-test-original-message-hiding)'
> +
> +test_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index 3f1740c..af132fc 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -54,6 +54,7 @@ TESTS="
>    argument-parsing
>    emacs-test-functions.sh
>    emacs-address-cleaning.sh
> +  emacs-original-message-hiding.sh
>  "
>  TESTS=${NOTMUCH_TESTS:=$TESTS}
>  
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/4 v2] emacs: Take more care when hiding regions with buttons.

[Dmitry Kurochkin]

On Wed, 25 Jan 2012 15:45:28 +0000, David Edmondson <dme@dme.org> wrote:
> If the region to be hidden with a button by
> `notmuch-wash-region-to-button' starts at the beginning of the buffer,
> the invisible region will include the inserted button. This is
> unfortunate, as it means that it is not possible to see the button to
> be pressed.
> 
> Make a little space at the start of the buffer before inserting the
> button to avoid this, not forgetting to remove the inserted space upon
> completion.
> 
> Mark the relevant test as no longer broken.
> ---

I will try to find some time to play with the test and look for
alternative solutions.

Regards,
  Dmitry

>  emacs/notmuch-wash.el                 |   57 ++++++++++++++++++++++-----------
>  test/emacs-original-message-hiding.sh |    1 -
>  2 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
> index 5c1e830..4afd3b3 100644
> --- a/emacs/notmuch-wash.el
> +++ b/emacs/notmuch-wash.el
> @@ -147,25 +147,44 @@ insert before the button, probably for indentation."
>    ;; symbols because of the way the button code works. Note that
>    ;; replacing intern-soft with make-symbol will cause this to fail,
>    ;; 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 (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
> -      (goto-char (1- beg))
> -      (insert prefix)
> -      (insert-button (notmuch-wash-button-label overlay)
> -		     'invisibility-spec invis-spec
> -		     'overlay overlay
> -		     :type button-type))))
> +  (save-excursion
> +    ;; If the beginning of the region to be converted to a button is the
> +    ;; beginning of the buffer we must move forward a little to avoid
> +    ;; creating an overlay that will hide the button intended to be used
> +    ;; to reveal the hidden region.
> +    (let (scene-of-crime)
> +      (when (eq beg (point-min))
> +	(goto-char (point-min))
> +	(insert "\n")
> +	(setq scene-of-crime (point-min)
> +	      beg (point)))
> +
> +      ;; This uses some slightly tricky conversions between strings and
> +      ;; symbols because of the way the button code works. Note that
> +      ;; replacing intern-soft with make-symbol will cause this to fail,
> +      ;; 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 (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- beg))
> +	(insert prefix)
> +	(insert-button (notmuch-wash-button-label overlay)
> +		       'invisibility-spec invis-spec
> +		       'overlay overlay
> +		       :type button-type))
> +
> +      (when scene-of-crime
> +      	(goto-char scene-of-crime)
> +      	(delete-char 1)))))
>  
>  (defun notmuch-wash-excerpt-citations (msg depth)
>    "Excerpt citations and up to one signature."
> diff --git a/test/emacs-original-message-hiding.sh b/test/emacs-original-message-hiding.sh
> index 01cf98d..038ef4f 100755
> --- a/test/emacs-original-message-hiding.sh
> +++ b/test/emacs-original-message-hiding.sh
> @@ -13,7 +13,6 @@ add_message \
>      '[body]="-----Original Message-----
>  Text here.
>  "'
> -test_subtest_known_broken
>  test_emacs_expect_t \
>      '(load "emacs-original-message-hiding.el") (notmuch-test-original-message-hiding)'
>  
> -- 
> 1.7.8.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 0/4 v3] more care when hiding regions with buttons

[David Edmondson]

Address Dmitry's comments.

David Edmondson (4):
  test: `visible-buffer-substring' should not return text properties.
  test: `notmuch-test-run' should protect against buffer switching.
  test: Add test for Original Message hiding at point-min.
  emacs: Take more care when hiding regions with buttons.

 emacs/notmuch-wash.el        |   57 ++++++++++++++++++++++++++++--------------
 test/emacs-message-hiding.sh |   36 ++++++++++++++++++++++++++
 test/notmuch-test            |    1 +
 test/test-lib.el             |   13 ++++++---
 4 files changed, 84 insertions(+), 23 deletions(-)
 create mode 100755 test/emacs-message-hiding.sh

-- 
1.7.8.3

[PATCH 1/4 v3] test: `visible-buffer-substring' should not return text properties.

[David Edmondson]

When using `visible-buffer-substring' to examine a buffer, the text
properties are not useful, so don't include them.
---
 test/test-lib.el |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index bc75f06..5b32e0a 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -51,16 +51,19 @@ FILENAME is OUTPUT."
     (with-temp-file (or filename "OUTPUT") (insert text))))
 
 (defun visible-buffer-string ()
-  "Same as `buffer-string', but excludes invisible text."
+  "Same as `buffer-string', but excludes invisible text and
+removes any text properties."
   (visible-buffer-substring (point-min) (point-max)))
 
 (defun visible-buffer-substring (start end)
-  "Same as `buffer-substring', but excludes invisible text."
+  "Same as `buffer-substring-no-properties', but excludes
+invisible text."
   (let (str)
     (while (< start end)
       (let ((next-pos (next-char-property-change start end)))
 	(when (not (invisible-p start))
-	  (setq str (concat str (buffer-substring start next-pos))))
+	  (setq str (concat str (buffer-substring-no-properties
+				 start next-pos))))
 	(setq start next-pos)))
     str))
 
-- 
1.7.8.3

[PATCH 2/4 v3] test: `notmuch-test-run' should protect against buffer switching.

[David Edmondson]

The body of the test may cause the current buffer to change. Ensure
that the output goes to the correct buffer by switching back before
inserting it.
---
 test/test-lib.el |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index 5b32e0a..6271da2 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -92,7 +92,9 @@ nothing."
 (defmacro notmuch-test-run (&rest body)
   "Evaluate a BODY of test expressions and output the result."
   `(with-temp-buffer
-     (let ((result (progn ,@body)))
+     (let ((buffer (current-buffer))
+	   (result (progn ,@body)))
+       (switch-to-buffer buffer)
        (insert (if (stringp result)
 		   result
 		 (prin1-to-string result)))
-- 
1.7.8.3

[PATCH 3/4 v3] test: Add test for Original Message hiding at point-min.

[David Edmondson]

---
 test/emacs-message-hiding.sh |   37 +++++++++++++++++++++++++++++++++++++
 test/notmuch-test            |    1 +
 2 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 test/emacs-message-hiding.sh

diff --git a/test/emacs-message-hiding.sh b/test/emacs-message-hiding.sh
new file mode 100755
index 0000000..70bc66d
--- /dev/null
+++ b/test/emacs-message-hiding.sh
@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+test_description="emacs Original Message hiding"
+. test-lib.sh
+
+test_begin_subtest "Hiding an Original Message region at point-min"
+add_message \
+    '[id]="OriginalMessageHiding.1@notmuchmail.org"' \
+    '[from]="Sender <sender@example.com>"' \
+    '[to]=mailinglist@notmuchmail.org' \
+    '[subject]="hiding an Original Message"' \
+    '[date]="Tue, 05 Jan 2010 15:43:56 -0000"' \
+    '[body]="-----Original Message-----
+Text here.
+"'
+
+cat > EXPECTED <<EOF
+Sender <sender@example.com> (2010-01-05) (inbox)
+Subject: hiding an Original Message
+To: mailinglist@notmuchmail.org
+Date: Tue, 05 Jan 2010 15:43:56 -0000
+
+[ 2-line hidden original message. Click/Enter to show. ]
+
+EOF
+
+test_emacs '
+(notmuch-test-run
+ (let ((notmuch-show-insert-text/plain-hook (quote notmuch-wash-excerpt-citations)))
+   (notmuch-show "id:\"OriginalMessageHiding.1@notmuchmail.org\"")
+   (visible-buffer-string)))
+'
+
+test_subtest_known_broken
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index 3f1740c..6631f87 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -54,6 +54,7 @@ TESTS="
   argument-parsing
   emacs-test-functions.sh
   emacs-address-cleaning.sh
+  emacs-message-hiding.sh
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.8.3

[PATCH 4/4 v3] emacs: Take more care when hiding regions with buttons.

[David Edmondson]

If the region to be hidden with a button by
`notmuch-wash-region-to-button' starts at the beginning of the buffer,
the invisible region will include the inserted button. This is
unfortunate, as it means that it is not possible to see the button to
be pressed.

Make a little space at the start of the buffer before inserting the
button to avoid this, not forgetting to remove the inserted space upon
completion.

Mark the relevant test as no longer broken.
---
 emacs/notmuch-wash.el        |   57 ++++++++++++++++++++++++++++--------------
 test/emacs-message-hiding.sh |    1 -
 2 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 5c1e830..4afd3b3 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -147,25 +147,44 @@ insert before the button, probably for indentation."
   ;; symbols because of the way the button code works. Note that
   ;; replacing intern-soft with make-symbol will cause this to fail,
   ;; 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 (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
-      (goto-char (1- beg))
-      (insert prefix)
-      (insert-button (notmuch-wash-button-label overlay)
-		     'invisibility-spec invis-spec
-		     'overlay overlay
-		     :type button-type))))
+  (save-excursion
+    ;; If the beginning of the region to be converted to a button is the
+    ;; beginning of the buffer we must move forward a little to avoid
+    ;; creating an overlay that will hide the button intended to be used
+    ;; to reveal the hidden region.
+    (let (scene-of-crime)
+      (when (eq beg (point-min))
+	(goto-char (point-min))
+	(insert "\n")
+	(setq scene-of-crime (point-min)
+	      beg (point)))
+
+      ;; This uses some slightly tricky conversions between strings and
+      ;; symbols because of the way the button code works. Note that
+      ;; replacing intern-soft with make-symbol will cause this to fail,
+      ;; 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 (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- beg))
+	(insert prefix)
+	(insert-button (notmuch-wash-button-label overlay)
+		       'invisibility-spec invis-spec
+		       'overlay overlay
+		       :type button-type))
+
+      (when scene-of-crime
+      	(goto-char scene-of-crime)
+      	(delete-char 1)))))
 
 (defun notmuch-wash-excerpt-citations (msg depth)
   "Excerpt citations and up to one signature."
diff --git a/test/emacs-message-hiding.sh b/test/emacs-message-hiding.sh
index 70bc66d..36708d7 100755
--- a/test/emacs-message-hiding.sh
+++ b/test/emacs-message-hiding.sh
@@ -31,7 +31,6 @@ test_emacs '
    (visible-buffer-string)))
 '
 
-test_subtest_known_broken
 test_expect_equal_file EXPECTED OUTPUT
 
 test_done
-- 
1.7.8.3

Re: [PATCH 3/4 v2] test: Add test for Original Message hiding at point-min.

[David Edmondson]

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

I've applied changes in accordance with all of the feedback in this mail
and the others, except for...

On Wed, 25 Jan 2012 21:53:08 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > +  (let ((notmuch-show-insert-text/plain-hook '(notmuch-wash-excerpt-citations))
> 
> Do we have to override the default value here?  I thought
> notmuch-wash-excerpt-citations was enabled by default.

It avoids potential confusion over where the problem lies.

> [...]
> 
> IMO writing the test in lisp does not give any benefit in this case.
> Quite the opposite: a simple test is split in two files and becomes more
> complex.
> 
> But since we accepted this way of writing tests and you seem to prefer
> it, I would not argue about changing it.

You're right - it's simpler in the original style here, so v3 switches
to that.

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

Re: [PATCH 0/4 v3] more care when hiding regions with buttons

[David Bremner]

On Thu, 26 Jan 2012 07:19:36 +0000, David Edmondson <dme@dme.org> wrote:
> Address Dmitry's comments.
> 
> David Edmondson (4):
>   test: `visible-buffer-substring' should not return text properties.
>   test: `notmuch-test-run' should protect against buffer switching.
pushed the first two.

>   test: Add test for Original Message hiding at point-min.
>   emacs: Take more care when hiding regions with buttons.

waiting on review.

d

Re: [PATCH 0/4 v3] more care when hiding regions with buttons

[Dmitry Kurochkin]

Hi David.

Here is my attempt to fix the issue in a cleaner way:
id:"1327926286-16680-1-git-send-email-dmitry.kurochkin@gmail.com"

Regards,
  Dmitry