[PATCH 1/3] Use different labels for wash buttons when text is visible or hidden.

[PATCH 1/3] Use different labels for wash buttons when text is visible or hidden.

[Dmitry Kurochkin]

Before the change, citation and signature wash buttons used the
same label in both visible and hidden states.  Sometimes it is
very convenient when you can determine if the text is hidden or
shown without reading the context and/or clicking the button.
The patch makes it easy to see if the text is shown or hidden by
explicitly saying what the button does (shows or hides the text).
---
 emacs/notmuch-wash.el |   55 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 0e64eb2..863459e 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -35,16 +35,26 @@
   "\\(^[[:space:]]*>.*\n\\)+"
   "Pattern to match citation lines.")
 
-(defvar notmuch-wash-signature-button-format
-  "[ %d-line signature. Click/Enter to toggle visibility. ]"
+(defvar notmuch-wash-button-signature-hidden-format
+  "[ %d-line signature. Click/Enter to show. ]"
   "String used to construct button text for hidden signatures.
 Can use up to one integer format parameter, i.e. %d")
 
-(defvar notmuch-wash-citation-button-format
-  "[ %d more citation lines. Click/Enter to toggle visibility. ]"
+(defvar notmuch-wash-button-signature-visible-format
+  "[ %d-line signature. Click/Enter to hide. ]"
+  "String used to construct button text for visible signatures.
+Can use up to one integer format parameter, i.e. %d")
+
+(defvar notmuch-wash-button-citation-hidden-format
+  "[ %d more citation lines. Click/Enter to show. ]"
   "String used to construct button text for hidden citations.
 Can use up to one integer format parameter, i.e. %d")
 
+(defvar notmuch-wash-button-citation-visible-format
+  "[ %d more citation lines. Click/Enter to hide. ]"
+  "String used to construct button text for visible citations.
+Can use up to one integer format parameter, i.e. %d")
+
 (defvar notmuch-wash-signature-lines-max 12
   "Maximum length of signature that will be hidden by default.")
 
@@ -69,6 +79,16 @@ collapse the remaining lines into a button.")
     (if (invisible-p invis-spec)
 	(remove-from-invisibility-spec invis-spec)
       (add-to-invisibility-spec invis-spec)))
+  (let* ((new-start (button-start cite-button))
+	 (overlay (button-get cite-button 'overlay))
+	 (button-label (notmuch-wash-button-label overlay))
+	 (inhibit-read-only t))
+    (save-excursion
+      (goto-char new-start)
+      (insert button-label)
+      (let ((old-end (button-end cite-button)))
+	(move-overlay cite-button new-start (point))
+	(delete-region (point) old-end))))
   (force-window-update)
   (redisplay t))
 
@@ -88,13 +108,21 @@ collapse the remaining lines into a button.")
 (defun notmuch-wash-region-isearch-show (overlay)
   (remove-from-invisibility-spec (overlay-get overlay 'invisible)))
 
-(defun notmuch-wash-region-to-button (beg end type prefix button-text)
+(defun notmuch-wash-button-label (overlay)
+  (let* ((type (overlay-get overlay 'type))
+	 (invis-spec (overlay-get overlay 'invisible))
+	 (state (if (invisible-p invis-spec) "hidden" "visible"))
+	 (label-format (symbol-value (intern-soft (concat "notmuch-wash-button-"
+							  type "-" state "-format"))))
+	 (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)
   "Auxilary function to do the actual making of overlays and buttons
 
 BEG and END are buffer locations. TYPE should a string, either
 \"citation\" or \"signature\". PREFIX is some arbitrary text to
-insert before the button, probably for indentation.  BUTTON-TEXT
-is what to put on the button."
+insert before the button, probably for indentation."
 
   ;; This uses some slightly tricky conversions between strings and
   ;; symbols because of the way the button code works. Note that
@@ -108,12 +136,14 @@ is what to put on the button."
     (add-to-invisibility-spec invis-spec)
     (overlay-put overlay 'invisible invis-spec)
     (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)
+    (overlay-put overlay 'type type)
     (goto-char (1+ end))
     (save-excursion
       (goto-char (1- beg))
       (insert prefix)
-      (insert-button button-text
+      (insert-button (notmuch-wash-button-label overlay)
 		     'invisibility-spec invis-spec
+		     'overlay overlay
 		     :type button-type))))
 
 (defun notmuch-wash-excerpt-citations (depth)
@@ -136,11 +166,7 @@ is what to put on the button."
 	  (forward-line (- notmuch-wash-citation-lines-suffix))
 	  (notmuch-wash-region-to-button
 	   hidden-start (point-marker)
-	   "citation" "\n"
-	   (format notmuch-wash-citation-button-format
-		   (- cite-lines
-		      notmuch-wash-citation-lines-prefix
-		      notmuch-wash-citation-lines-suffix)))))))
+	   "citation" "\n")))))
   (if (and (not (eobp))
 	   (re-search-forward notmuch-wash-signature-regexp nil t))
       (let* ((sig-start (match-beginning 0))
@@ -154,8 +180,7 @@ is what to put on the button."
 	      (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
-	       "signature" "\n"
-	       (format notmuch-wash-signature-button-format sig-lines)))))))
+	       "signature" "\n"))))))
 
 ;;
 
-- 
1.7.5.1

[PATCH 2/3] test: fix expected output for emacs tests after the wash button label changes

[Dmitry Kurochkin]

---
 .../notmuch-show-thread-maildir-storage            |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/test/emacs.expected-output/notmuch-show-thread-maildir-storage b/test/emacs.expected-output/notmuch-show-thread-maildir-storage
index 2f24630..241b5b7 100644
--- a/test/emacs.expected-output/notmuch-show-thread-maildir-storage
+++ b/test/emacs.expected-output/notmuch-show-thread-maildir-storage
@@ -26,7 +26,7 @@ with Maildir) or if something else is going on.
 
 Cheers,
 
-[ 5-line signature. Click/Enter to toggle visibility. ]
+[ 5-line signature. Click/Enter to show. ]
 -- 
 Lars Kellogg-Stedman <lars@seas.harvard.edu>
 Senior Technologist, Computing and Information Technology
@@ -34,7 +34,7 @@ Harvard University School of Engineering and Applied Sciences
 
 [ application/pgp-signature ]
 [ text/plain ]
-[ 4-line signature. Click/Enter to toggle visibility. ]
+[ 4-line signature. Click/Enter to show. ]
 _______________________________________________
 notmuch mailing list
 notmuch@notmuchmail.org
@@ -58,12 +58,12 @@ http://notmuchmail.org/mailman/listinfo/notmuch
 
  See the patch just posted here.
 
- [ 2-line signature. Click/Enter to toggle visibility. ]
+ [ 2-line signature. Click/Enter to show. ]
  -- 
  http://fossarchy.blogspot.com/
  [ application/pgp-signature ]
  [ text/plain ]
- [ 4-line signature. Click/Enter to toggle visibility. ]
+ [ 4-line signature. Click/Enter to show. ]
  _______________________________________________
  notmuch mailing list
  notmuch@notmuchmail.org
@@ -88,7 +88,7 @@ http://notmuchmail.org/mailman/listinfo/notmuch
 
   -- Lars
 
-  [ 5-line signature. Click/Enter to toggle visibility. ]
+  [ 5-line signature. Click/Enter to show. ]
   -- 
   Lars Kellogg-Stedman <lars@seas.harvard.edu>
   Senior Technologist, Computing and Information Technology
@@ -96,7 +96,7 @@ http://notmuchmail.org/mailman/listinfo/notmuch
 
   [ application/pgp-signature ]
   [ text/plain ]
-  [ 4-line signature. Click/Enter to toggle visibility. ]
+  [ 4-line signature. Click/Enter to show. ]
   _______________________________________________
   notmuch mailing list
   notmuch@notmuchmail.org
@@ -118,7 +118,7 @@ http://notmuchmail.org/mailman/listinfo/notmuch
 
    Just has been pushed
 
-   [ 10-line signature. Click/Enter to toggle visibility. ]
+   [ 10-line signature. Click/Enter to show. ]
    -- 
    http://fossarchy.blogspot.com/
    -------------- next part --------------
@@ -166,7 +166,7 @@ http://notmuchmail.org/mailman/listinfo/notmuch
     The version of lib/messages.cc in your repo doesn't build because it's
     missing "#include <stdint.h>" (for the uint32_t on line 466).
 
-    [ 5-line signature. Click/Enter to toggle visibility. ]
+    [ 5-line signature. Click/Enter to show. ]
     -- 
     Lars Kellogg-Stedman <lars@seas.harvard.edu>
     Senior Technologist, Computing and Information Technology
@@ -174,7 +174,7 @@ http://notmuchmail.org/mailman/listinfo/notmuch
 
     [ application/pgp-signature ]
     [ text/plain ]
-    [ 4-line signature. Click/Enter to toggle visibility. ]
+    [ 4-line signature. Click/Enter to show. ]
     _______________________________________________
     notmuch mailing list
     notmuch@notmuchmail.org
-- 
1.7.5.1

[PATCH 3/3] test: add test for hiding/showing signature in notmuch-show view

[Dmitry Kurochkin]

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

diff --git a/test/emacs b/test/emacs
index f2983a7..ccf02af 100755
--- a/test/emacs
+++ b/test/emacs
@@ -141,4 +141,15 @@ first_line=$(echo "$expected" | head -n1)
 output=$(test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@mail.gmail.com") (notmuch-show-view-raw-message) (princ (buffer-string))' | sed -ne "/$first_line/,\$ p")
 test_expect_equal "$output" "$expected"
 
+test_begin_subtest "Hiding/showing signature in notmuch-show view"
+maildir_storage_thread=$(notmuch search --output=threads id:20091117190054.GU3165@dottiness.seas.harvard.edu)
+output=$(test_emacs "(notmuch-show \"$maildir_storage_thread\")
+		     (search-forward \"Click/Enter to show.\")
+		     (button-activate (button-at (point)))
+		     (search-backward \"Click/Enter to hide.\")
+		     (button-activate (button-at (point)))
+		     (princ (buffer-string))")
+expected=$(cat $EXPECTED/notmuch-show-thread-maildir-storage)
+test_expect_equal "$output" "$expected"
+
 test_done
-- 
1.7.5.1

[PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

Before the change, save-excursion was used to save the point.
But the restored position is affected by buffer modifications,
which results in jumping cursor.  The patch saves and restores
point explicitly by using a variable instead of save-excursion.
---
 emacs/notmuch-wash.el |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 863459e..b6d791f 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -82,13 +82,14 @@ collapse the remaining lines into a button.")
   (let* ((new-start (button-start cite-button))
 	 (overlay (button-get cite-button 'overlay))
 	 (button-label (notmuch-wash-button-label overlay))
+	 (old-point (point))
 	 (inhibit-read-only t))
-    (save-excursion
-      (goto-char new-start)
-      (insert button-label)
-      (let ((old-end (button-end cite-button)))
-	(move-overlay cite-button new-start (point))
-	(delete-region (point) old-end))))
+    (goto-char new-start)
+    (insert button-label)
+    (let ((old-end (button-end cite-button)))
+      (move-overlay cite-button new-start (point))
+      (delete-region (point) old-end))
+    (goto-char old-point))
   (force-window-update)
   (redisplay t))
 
-- 
1.7.5.1

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

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

On Mon, 23 May 2011 19:29:46 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Before the change, save-excursion was used to save the point.
> But the restored position is affected by buffer modifications,
> which results in jumping cursor.  The patch saves and restores
> point explicitly by using a variable instead of save-excursion.

Dmitry,

Thanks so much for the improvement to the button text! This will be a
nice thing to add.

But this patch confuses me. I can understand how a buffer-position
variable can cause the cursor to jump. That's usually the kind of thing
that can be fixed by switching from an integer position to a marker
instead, (since markers are updated when the corresponding text is
updated).

But in this case, I don't see how:

	(let ((old-point (point)))
        	... code here ...
	  (goto-char old-point))

is distinct from:

	(save-excursion
		... code here ...
	 )

except that save-excursion actually does the right thing in the case of
abnormal exit (throw or error).

Can you help me understand what I'm missing here?

-Carl

-- 
carl.d.worth@intel.com

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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

Hi Carl.

On Tue, 24 May 2011 13:20:56 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Mon, 23 May 2011 19:29:46 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Before the change, save-excursion was used to save the point.
> > But the restored position is affected by buffer modifications,
> > which results in jumping cursor.  The patch saves and restores
> > point explicitly by using a variable instead of save-excursion.
> 
> Dmitry,
> 
> Thanks so much for the improvement to the button text! This will be a
> nice thing to add.
> 
> But this patch confuses me. I can understand how a buffer-position
> variable can cause the cursor to jump. That's usually the kind of thing
> that can be fixed by switching from an integer position to a marker
> instead, (since markers are updated when the corresponding text is
> updated).
> 

So we need to switch from marker to an integer position, right?

> But in this case, I don't see how:
> 
> 	(let ((old-point (point)))
>         	... code here ...
> 	  (goto-char old-point))
> 
> is distinct from:
> 
> 	(save-excursion
> 		... code here ...
> 	 )
> 
> except that save-excursion actually does the right thing in the case of
> abnormal exit (throw or error).
> 
> Can you help me understand what I'm missing here?
> 

Unfortunately, I am not an Emacs lisp expert.  I just noticed that the
cursor jumps and did the first thing that came to mind.  And it worked.

Now, looking at Emacs source code, save_excursion_save() uses
point_marker() to save the point.  As you said above, markers are
updated when the corresponding text is updated.  That explains why the
cursor jumps when using `save-excursion'.

On the other hand, `point' returns position as an integer.  Which is
just what we need.

Regards,
  Dmitry

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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

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

On Wed, 25 May 2011 00:43:20 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Now, looking at Emacs source code, save_excursion_save() uses
> point_marker() to save the point.  As you said above, markers are
> updated when the corresponding text is updated.  That explains why the
> cursor jumps when using `save-excursion'.
> 
> On the other hand, `point' returns position as an integer.  Which is
> just what we need.

Ah. So that explains why your patch is actually making a difference.

But I've usually had "jumping cursor" problems when using an integer,
(and switching to a marker fixes it). For example, imagine the following
operation:

	User positions cursor on some word
	Our code saves point as an integer
	Some operation inserts new text before point
	Our code restores the cursor to the saved integer

This sequence restores point to the same integer position in the buffer,
but logically makes the cursor appear to "jump" to the user, (since the
cursor will no longer be on the word it was on before but will now be
earlier in the buffer).

The fix for the above is to switch from an integer to a marker.

So I'm curious to know the case you're hitting where you getbetter
behavior by switching from a marker to an integer. Can you describe it
in a bit more detail?

-Carl

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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

On Tue, 24 May 2011 15:01:04 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Wed, 25 May 2011 00:43:20 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Now, looking at Emacs source code, save_excursion_save() uses
> > point_marker() to save the point.  As you said above, markers are
> > updated when the corresponding text is updated.  That explains why the
> > cursor jumps when using `save-excursion'.
> > 
> > On the other hand, `point' returns position as an integer.  Which is
> > just what we need.
> 
> Ah. So that explains why your patch is actually making a difference.
> 
> But I've usually had "jumping cursor" problems when using an integer,
> (and switching to a marker fixes it). For example, imagine the following
> operation:
> 
> 	User positions cursor on some word
> 	Our code saves point as an integer
> 	Some operation inserts new text before point
> 	Our code restores the cursor to the saved integer
> 
> This sequence restores point to the same integer position in the buffer,
> but logically makes the cursor appear to "jump" to the user, (since the
> cursor will no longer be on the word it was on before but will now be
> earlier in the buffer).
> 
> The fix for the above is to switch from an integer to a marker.
> 

Ah, I see now.

> So I'm curious to know the case you're hitting where you getbetter
> behavior by switching from a marker to an integer. Can you describe it
> in a bit more detail?
> 

I did not find a better way to update the button label than to:

1. insert the new label
2. move the button overlay from the old label to the new one
3. remove the old label

When a user clicks the button, the cursor is somewhere inside the old
label.  If we save the point as a marker, after step 3 it would end up
at the position where the old label was.  If the new label is inserted
before the old one, that means after the new label.  So the cursor jumps
from inside the button to the position after the button.  Since the new
button is placed at the same position where the old one was, restoring
the point to the same offset it was at the beginning works as we need.

Regards,
  Dmitry

> -Carl
Non-text part: application/pgp-signature

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

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

On Wed, 25 May 2011 02:16:06 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Since the newbutton is placed at the same position where the old one was, restoring
> the point to the same offset it was at the beginning works as we need.

Thanks for explaining this. I'll experiment a bit and push the series,
perhaps with a change to the last commit message.

-Carl

-- 
carl.d.worth@intel.com

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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Austin Clements]

On Tue, May 24, 2011 at 6:16 PM, Dmitry Kurochkin
<dmitry.kurochkin@gmail.com> wrote:
> When a user clicks the button, the cursor is somewhere inside the old
> label.  If we save the point as a marker, after step 3 it would end up
> at the position where the old label was.  If the new label is inserted
> before the old one, that means after the new label.  So the cursor jumps
> from inside the button to the position after the button.  Since the new
> button is placed at the same position where the old one was, restoring
> the point to the same offset it was at the beginning works as we need.

Saving point this way is a bit dangerous, though.  For example, if
you're near the end of the buffer and shorten the label, attempting to
restore the point could result in an error (or, a more benign example:
the cursor could wind up outside the label so pressing RET repeatedly
won't toggle it).

Unfortunately, I don't know of a clean solution to this, but I think I
would rather the cursor move, but stay within the label (probably
moving to the beginning), than have problems like the above.

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

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

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements <amdragon@mit.edu> wrote:
> Saving point this way is a bit dangerous, though.  For example, if
> you're near the end of the buffer and shorten the label, attempting to
> restore the point could result in an error (or, a more benign example:
> the cursor could wind up outside the label so pressing RET repeatedly
> won't toggle it).

Without the patch to change save-excursion to an integer, point is
already moving outside the button, (so that repeatedly pressing RET
doesn't toggle).

I'm exploring a proper fix now to get reliable behavior.

-Carl

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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements <amdragon@mit.edu> wrote:
> On Tue, May 24, 2011 at 6:16 PM, Dmitry Kurochkin
> <dmitry.kurochkin@gmail.com> wrote:
> > When a user clicks the button, the cursor is somewhere inside the old
> > label.  If we save the point as a marker, after step 3 it would end up
> > at the position where the old label was.  If the new label is inserted
> > before the old one, that means after the new label.  So the cursor jumps
> > from inside the button to the position after the button.  Since the new
> > button is placed at the same position where the old one was, restoring
> > the point to the same offset it was at the beginning works as we need.
> 
> Saving point this way is a bit dangerous, though.  For example, if
> you're near the end of the buffer and shorten the label, attempting to
> restore the point could result in an error (or, a more benign example:
> the cursor could wind up outside the label so pressing RET repeatedly
> won't toggle it).
> 
> Unfortunately, I don't know of a clean solution to this, but I think I
> would rather the cursor move, but stay within the label (probably
> moving to the beginning), than have problems like the above.

Good point.  I will send an amended patch that moved to min(old-pos,
new-button-end - 1).  This leaves the cursor in place when possible and
avoids problems with out-of-bounds position (assuming the label is not
empty).

Regards,
  Dmitry

[PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

Before the change, save-excursion was used to save the point.
But the restored position is affected by buffer modifications,
which results in jumping cursor.  The patch saves and restores
point explicitly by using a variable instead of save-excursion.
---
 emacs/notmuch-wash.el |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 863459e..115c3bb 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -82,13 +82,14 @@ collapse the remaining lines into a button.")
   (let* ((new-start (button-start cite-button))
 	 (overlay (button-get cite-button 'overlay))
 	 (button-label (notmuch-wash-button-label overlay))
+	 (old-point (point))
 	 (inhibit-read-only t))
-    (save-excursion
-      (goto-char new-start)
-      (insert button-label)
-      (let ((old-end (button-end cite-button)))
-	(move-overlay cite-button new-start (point))
-	(delete-region (point) old-end))))
+    (goto-char new-start)
+    (insert button-label)
+    (let ((old-end (button-end cite-button)))
+      (move-overlay cite-button new-start (point))
+      (delete-region (point) old-end))
+    (goto-char (min old-point (1- (button-end cite-button)))))
   (force-window-update)
   (redisplay t))
 
-- 
1.7.5.1

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

[-- Attachment #1.1: Type: text/plain, Size: 659 bytes --]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements <amdragon@mit.edu> wrote:
> Saving point this way is a bit dangerous, though.  For example, if
> you're near the end of the buffer and shorten the label, attempting to
> restore the point could result in an error (or, a more benign example:
> the cursor could wind up outside the label so pressing RET repeatedly
> won't toggle it).
> 
> Unfortunately, I don't know of a clean solution to this, but I think I
> would rather the cursor move, but stay within the label (probably
> moving to the beginning), than have problems like the above.

Here's my fix. Let me know what you think.

-Carl


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Carefully-preverse-point-when-changing-button-text-i.patch --]
[-- Type: text/x-diff, Size: 1751 bytes --]

From a32e02bf0d2b57d51695f7d4ea6cdda9acb21322 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth@cworth.org>
Date: Mon, 23 May 2011 19:29:46 +0400
Subject: [PATCH] Carefully preverse point when changing button text in
 `notmuch-wash-toggle-invisible-action'.

Previously, save-excursion was used to attempt to save the point, but
this was unreliable since the region containing the marker saved by
save-excursion was deleted. Instead, we save an integer position
indicating the offset of point within the old button. Then, we restore
point to the same offset within the new button, (but cap the offset to
avoid leaving the button entirely).
---
 emacs/notmuch-wash.el |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
index 8455eee..3dceb8b 100644
--- a/emacs/notmuch-wash.el
+++ b/emacs/notmuch-wash.el
@@ -82,13 +82,14 @@ collapse the remaining lines into a button.")
   (let* ((new-start (button-start cite-button))
 	 (overlay (button-get cite-button 'overlay))
 	 (button-label (notmuch-wash-button-label overlay))
+	 (button-offset (- (point) new-start))
 	 (inhibit-read-only t))
-    (save-excursion
-      (goto-char new-start)
-      (insert button-label)
-      (let ((old-end (button-end cite-button)))
-	(move-overlay cite-button new-start (point))
-	(delete-region (point) old-end))))
+    (goto-char new-start)
+    (insert button-label)
+    (let ((old-end (button-end cite-button)))
+      (move-overlay cite-button new-start (point))
+      (delete-region (point) old-end))
+    (goto-char (min (button-end cite-button) (+ new-start button-offset))))
   (force-window-update)
   (redisplay t))
 
-- 
1.7.5.1


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

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Dmitry Kurochkin]

On Tue, 24 May 2011 16:20:34 -0700, Carl Worth <cworth@cworth.org> wrote:
> On Tue, 24 May 2011 18:43:41 -0400, Austin Clements <amdragon@mit.edu> wrote:
> > Saving point this way is a bit dangerous, though.  For example, if
> > you're near the end of the buffer and shorten the label, attempting to
> > restore the point could result in an error (or, a more benign example:
> > the cursor could wind up outside the label so pressing RET repeatedly
> > won't toggle it).
> > 
> > Unfortunately, I don't know of a clean solution to this, but I think I
> > would rather the cursor move, but stay within the label (probably
> > moving to the beginning), than have problems like the above.
> 
> Here's my fix. Let me know what you think.
> 

(button-end cite-button) would move the point outside the button - to
the next character after it.

Regards,
  Dmitry

> -Carl
> 
> From a32e02bf0d2b57d51695f7d4ea6cdda9acb21322 Mon Sep 17 00:00:00 2001
> From: Carl Worth <cworth@cworth.org>
> Date: Mon, 23 May 2011 19:29:46 +0400
> Subject: [PATCH] Carefully preverse point when changing button text in
>  `notmuch-wash-toggle-invisible-action'.
> 
> Previously, save-excursion was used to attempt to save the point, but
> this was unreliable since the region containing the marker saved by
> save-excursion was deleted. Instead, we save an integer position
> indicating the offset of point within the old button. Then, we restore
> point to the same offset within the new button, (but cap the offset to
> avoid leaving the button entirely).
> ---
>  emacs/notmuch-wash.el |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
> index 8455eee..3dceb8b 100644
> --- a/emacs/notmuch-wash.el
> +++ b/emacs/notmuch-wash.el
> @@ -82,13 +82,14 @@ collapse the remaining lines into a button.")
>    (let* ((new-start (button-start cite-button))
>  	 (overlay (button-get cite-button 'overlay))
>  	 (button-label (notmuch-wash-button-label overlay))
> +	 (button-offset (- (point) new-start))
>  	 (inhibit-read-only t))
> -    (save-excursion
> -      (goto-char new-start)
> -      (insert button-label)
> -      (let ((old-end (button-end cite-button)))
> -	(move-overlay cite-button new-start (point))
> -	(delete-region (point) old-end))))
> +    (goto-char new-start)
> +    (insert button-label)
> +    (let ((old-end (button-end cite-button)))
> +      (move-overlay cite-button new-start (point))
> +      (delete-region (point) old-end))
> +    (goto-char (min (button-end cite-button) (+ new-start button-offset))))
>    (force-window-update)
>    (redisplay t))
>  
> -- 
> 1.7.5.1
> 
Non-text part: application/pgp-signature

Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.

[Carl Worth]

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

On Wed, 25 May 2011 03:27:51 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> (button-end cite-button) would move the point outside the button - to
> the next character after it.

OK. Your fix addresses my off-by-one bug. I've just pushed the whole
series, with your final amended fix, (and some updates I made to its
commit message).

Thanks again, Dmitry. This will be a nice refinement in the interface.

-Carl

-- 
carl.d.worth@intel.com

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