Re: [PATCH] Save and restore point explicitly in `notmuch-wash-toggle-invisible-action'.
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.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'.
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 > 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'.
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 pgp1K87ZrWvZa.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'.
On Tue, 24 May 2011 15:01:04 -0700, Carl Worth wrote: > 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. > 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'.
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.wo...@intel.com pgp5f68urZTgD.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'.
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. ___ 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'.
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 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'.
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 ___ 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'.
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 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 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'.
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 ___ 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'.
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.wo...@intel.com pgpxM97c1bPaF.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
