[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  wrote:
> On Tue, 24 May 2011 18:43:41 -0400, Austin Clements  
> 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 
> 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

[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

[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  wrote:
> On Tue, May 24, 2011 at 6:16 PM, Dmitry Kurochkin
>  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]

On Tue, 24 May 2011 15:01:04 -0700, Carl Worth  wrote:
> On Wed, 25 May 2011 00:43:20 +0400, 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

[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  wrote:
> On Mon, 23 May 2011 19:29:46 +0400, 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 at intel.com
Non-text part: application/pgp-signature

[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
 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.

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

[Carl Worth]

On Wed, 25 May 2011 03:27:51 +0400, Dmitry Kurochkin  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 at intel.com
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements  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

-- next part --
A non-text attachment was scrubbed...
Name: 0001-Carefully-preverse-point-when-changing-button-text-i.patch
Type: text/x-diff
Size: 1707 bytes
Desc: not available
URL: 

-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements  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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Wed, 25 May 2011 02:16:06 +0400, Dmitry Kurochkin  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 at intel.com
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Wed, 25 May 2011 00:43:20 +0400, Dmitry Kurochkin  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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Mon, 23 May 2011 19:29:46 +0400, Dmitry Kurochkin  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 at intel.com
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: 

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

[Carl Worth]

On Mon, 23 May 2011 19:29:46 +0400, Dmitry Kurochkin 
dmitry.kuroch...@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.wo...@intel.com


pgpjWjqFStd2F.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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 cwo...@cworth.org wrote:
 On Mon, 23 May 2011 19:29:46 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@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.wo...@intel.com
Non-text part: application/pgp-signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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 cwo...@cworth.org wrote:
 On Wed, 25 May 2011 00:43:20 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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.kuroch...@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.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Carl Worth]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements amdra...@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


pgpiqVNCj7RTl.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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 amdra...@mit.edu wrote:
 On Tue, May 24, 2011 at 6:16 PM, Dmitry Kurochkin
 dmitry.kuroch...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Carl Worth]

On Tue, 24 May 2011 18:43:41 -0400, Austin Clements amdra...@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

From a32e02bf0d2b57d51695f7d4ea6cdda9acb21322 Mon Sep 17 00:00:00 2001
From: Carl Worth cwo...@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



pgpZUpuiAlgqJ.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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 cwo...@cworth.org wrote:
 On Tue, 24 May 2011 18:43:41 -0400, Austin Clements amdra...@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 cwo...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Carl Worth]

On Wed, 25 May 2011 03:27:51 +0400, Dmitry Kurochkin 
dmitry.kuroch...@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.wo...@intel.com


pgpxM97c1bPaF.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[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

[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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch