bug#12208: yes-or-no-p escapes with-current-buffer

bug#12208: yes-or-no-p escapes with-current-buffer

[Steve Hafner]

Open two buffers "buffer1" and  "buffer2" so that both are visible.

Place the following in each buffer

(with-current-buffer "buffer1"
        (goto-char (point-min))
	(yes-or-no-p "")
	(insert "X"))

C-xC-e from buffer1 places "X" at the beginning of the file, while
C-xC-e from buffer2 places an "X" wherever the point was before moving
from buffer1. This doesn't happen if only one buffer is visible; Nor
does it happen if we replace "yes-or-no-p" with "y-or-n-p".
This happens on both 23.3.1 and 24.1.

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

 > Open two buffers "buffer1" and  "buffer2" so that both are visible.
 >
 > Place the following in each buffer
 >
 > (with-current-buffer "buffer1"
 >         (goto-char (point-min))
 > 	(yes-or-no-p "")
 > 	(insert "X"))
 >
 > C-xC-e from buffer1 places "X" at the beginning of the file, while
 > C-xC-e from buffer2 places an "X" wherever the point was before moving
 > from buffer1. This doesn't happen if only one buffer is visible; Nor
 > does it happen if we replace "yes-or-no-p" with "y-or-n-p".
 > This happens on both 23.3.1 and 24.1.

Indeed.  What you see is `save-window-excursion' at work.  Let's remove
`yes-or-no-p' from the example.  With emacs -Q in *scratch* evaluate the
following form:

(let ((form
        "(with-current-buffer \"*buffer1*\"
   (goto-char (point-min))
   (save-window-excursion
     nil)
   (setq x (1+ x))
   (insert (format \"%s\" x)))"))
   (setq x 0)
   (switch-to-buffer (get-buffer-create "*buffer1*"))
   (insert form)
   (split-window)
   (other-window 1)
   (switch-to-buffer (get-buffer-create "*buffer2*"))
   (insert form)
   (other-window 1))

Now in any of the two windows go to the end of the buffer and evaluate
the preceding form.  From the *buffer1* window the number is inserted at
position 1, from the *buffer2* window at the position of *buffer1*
before evaluating the form.  If, instead, you used the slightly
different form

(let ((form
        "(with-selected-window (get-buffer-window \"*buffer1*\")
   (goto-char (point-min))
   (save-window-excursion
     nil)
   (setq x (1+ x))
   (insert (format \"%s\" x)))"))
   (setq x 0)
   (switch-to-buffer (get-buffer-create "*buffer1*"))
   (insert form)
   (split-window)
   (other-window 1)
   (switch-to-buffer (get-buffer-create "*buffer2*"))
   (insert form)
   (other-window 1))

both insertions happen at position 1.

martin

bug#12208: yes-or-no-p escapes with-current-buffer

[Steve Hafner]

On Thu, Aug 16, 2012 at 3:38 AM, martin rudalics <rudalics@gmx.at> wrote:
>> Open two buffers "buffer1" and  "buffer2" so that both are visible.
>>
>> Place the following in each buffer
>>
>> (with-current-buffer "buffer1"
>>         (goto-char (point-min))
>>       (yes-or-no-p "")
>>       (insert "X"))
>>
>> C-xC-e from buffer1 places "X" at the beginning of the file, while
>> C-xC-e from buffer2 places an "X" wherever the point was before moving
>> from buffer1. This doesn't happen if only one buffer is visible; Nor
>> does it happen if we replace "yes-or-no-p" with "y-or-n-p".
>> This happens on both 23.3.1 and 24.1.
>
> Indeed.  What you see is `save-window-excursion' at work.  Let's remove
> `yes-or-no-p' from the example.  With emacs -Q in *scratch* evaluate the
> following form:
>
> (let ((form
>        "(with-current-buffer \"*buffer1*\"
>   (goto-char (point-min))
>   (save-window-excursion
>     nil)
>   (setq x (1+ x))
>   (insert (format \"%s\" x)))"))
>   (setq x 0)
>   (switch-to-buffer (get-buffer-create "*buffer1*"))
>   (insert form)
>   (split-window)
>   (other-window 1)
>   (switch-to-buffer (get-buffer-create "*buffer2*"))
>   (insert form)
>   (other-window 1))
>
> Now in any of the two windows go to the end of the buffer and evaluate
> the preceding form.  From the *buffer1* window the number is inserted at
> position 1, from the *buffer2* window at the position of *buffer1*
> before evaluating the form.  If, instead, you used the slightly
> different form
>
> (let ((form
>        "(with-selected-window (get-buffer-window \"*buffer1*\")
>   (goto-char (point-min))
>   (save-window-excursion
>     nil)
>   (setq x (1+ x))
>   (insert (format \"%s\" x)))"))
>   (setq x 0)
>   (switch-to-buffer (get-buffer-create "*buffer1*"))
>   (insert form)
>   (split-window)
>   (other-window 1)
>   (switch-to-buffer (get-buffer-create "*buffer2*"))
>   (insert form)
>   (other-window 1))
>
> both insertions happen at position 1.
>
> martin

Thanks for the examples. If I understand correctly,
save-window-excursion ends with among other things restoring the
window points; and each time a window point is set, the buffer point
is set as well. And while the current buffer is restored,
save-window-excursion does not restore the point in the current
buffer, leaving it at the position set by the window point restore, if
there was one. What I don't understand is the rationale for not
restoring the current buffer point. Perhaps it's just windows have
precedence in a restore.

Steve

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

 > Thanks for the examples.

I wrote them as a first step in order to uncouple the behavior you
observed from `yes-or-no-p'.

 > If I understand correctly,
 > save-window-excursion ends with among other things restoring the
 > window points; and each time a window point is set, the buffer point
 > is set as well.

Conceptually, for any non-selected window, it should restore
`window-point' but leave `point' for that window's buffer alone.  For
the selected window, it should leave `point' where it is.  The problem
seems to occur only when the selected window's buffer is not current at
the time the configuration is saved.

 > And while the current buffer is restored,
 > save-window-excursion does not restore the point in the current
 > buffer, leaving it at the position set by the window point restore, if
 > there was one.

Did you read the code?  Can you tell me where it does do that "window
point restore" which also sets the buffer's `point'?

 > What I don't understand is the rationale for not
 > restoring the current buffer point. Perhaps it's just windows have
 > precedence in a restore.

I've never been able to fully understand neither the purpose nor the
actual implementation of this.  The patch below seems to make it go away
in your case but I have no idea whether it breaks something else.

martin

=== modified file 'src/window.c'
--- src/window.c	2012-08-16 07:58:24 +0000
+++ src/window.c	2012-08-17 11:56:55 +0000
@@ -5636,8 +5636,9 @@
  	       /* As documented in Fcurrent_window_configuration, don't
  		  restore the location of point in the buffer which was
  		  current when the window configuration was recorded.  */
-	       if (!EQ (p->buffer, new_current_buffer)
-		   && XBUFFER (p->buffer) == current_buffer)
+	       if (EQ (p->buffer, new_current_buffer))
+		 Fgoto_char (make_number (old_point));
+	       else if (XBUFFER (p->buffer) == current_buffer)
  		 Fgoto_char (w->pointm);
  	     }
  	   else if (!NILP (w->buffer)

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

 > Did you read the code?  Can you tell me where it does do that "window
 > point restore" which also sets the buffer's `point'?

... just forgot to look at my beloved enemy.

run_window_configuration_change_hook sets `point' from `window-point'
via select_window_norecord.

martin

bug#12208: yes-or-no-p escapes with-current-buffer

[Steve Hafner]

On Fri, Aug 17, 2012 at 9:18 AM, martin rudalics <rudalics@gmx.at> wrote:
>> Did you read the code?  Can you tell me where it does do that "window
>> point restore" which also sets the buffer's `point'?
>
> ... just forgot to look at my beloved enemy.
>
> run_window_configuration_change_hook sets `point' from `window-point'
> via select_window_norecord.
>

No, I didn't read the code, but I'm glad you found it. The problem
originally showed up in Org-mode, and I had fun chasing it through the
lisp. Then it disappeared into C, and I've never looked at the Emacs C
code before. I'm sorry to say I can't judge whether or not your patch
breaks anything.

Steve

bug#12208: yes-or-no-p escapes with-current-buffer

[Steve Hafner]

On Fri, Aug 17, 2012 at 2:24 PM, Steve Hafner <steve.b.hafner@gmail.com> wrote:
> code before. I'm sorry to say I can't judge whether or not your patch
> breaks anything.


Actually, I'm coming up with a few problems. I applied your patch to
Git sources, and evaluating from buffer1 leaves the point at position
2.  Things seem to work correctly in buffer2, though.

And trying yes-or-no-p again, the behavior is the same as in my original post:


(let ((form
       "(with-current-buffer \"*buffer1*\"
  (goto-char (point-min))
  (yes-or-no-p \"\")
  (setq x (1+ x))
  (insert (format \"%s\" x)))"))
  (setq x 0)
  (switch-to-buffer (get-buffer-create "*buffer1*"))
  (insert form)
  (split-window)
  (other-window 1)
  (switch-to-buffer (get-buffer-create "*buffer2*"))
  (insert form)
  (other-window 1))


Steve

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

 > Actually, I'm coming up with a few problems. I applied your patch to
 > Git sources, and evaluating from buffer1 leaves the point at position
 > 2.  Things seem to work correctly in buffer2, though.
 >
 > And trying yes-or-no-p again, the behavior is the same as in my original post:
 >
 >
 > (let ((form
 >        "(with-current-buffer \"*buffer1*\"
 >   (goto-char (point-min))
 >   (yes-or-no-p \"\")
 >   (setq x (1+ x))
 >   (insert (format \"%s\" x)))"))
 >   (setq x 0)
 >   (switch-to-buffer (get-buffer-create "*buffer1*"))
 >   (insert form)
 >   (split-window)
 >   (other-window 1)
 >   (switch-to-buffer (get-buffer-create "*buffer2*"))
 >   (insert form)
 >   (other-window 1))

Sorry, the patch worked only when the selected window did not change.
Please try the one below.

Thanks, martin


=== modified file 'src/window.c'
--- src/window.c	2012-08-18 06:06:39 +0000
+++ src/window.c	2012-08-18 13:09:15 +0000
@@ -5889,7 +5889,13 @@
      }

    if (!NILP (new_current_buffer))
-    Fset_buffer (new_current_buffer);
+    {
+      Fset_buffer (new_current_buffer);
+      /* If the new current buffer doesn't appear in the selected
+	 window, go to its old point.  */
+      if (!EQ (XWINDOW (data->current_window)->buffer, new_current_buffer))
+	Fgoto_char (make_number (old_point));
+    }

    Vminibuf_scroll_window = data->minibuf_scroll_window;
    minibuf_selected_window = data->minibuf_selected_window;

bug#12208: yes-or-no-p escapes with-current-buffer

[Steve Hafner]

On Sat, Aug 18, 2012 at 7:15 AM, martin rudalics <rudalics@gmx.at> wrote:
>> Actually, I'm coming up with a few problems. I applied your patch to
>> Git sources, and evaluating from buffer1 leaves the point at position
>> 2.  Things seem to work correctly in buffer2, though.

My comment here with respect to the point ending at position 2 being a
problem was just confusion on my part. I think it was the correct
result....


>>
>> And trying yes-or-no-p again, the behavior is the same as in my original
>> post:
>>
>>
>> (let ((form
>>        "(with-current-buffer \"*buffer1*\"
>>   (goto-char (point-min))
>>   (yes-or-no-p \"\")
>>   (setq x (1+ x))
>>   (insert (format \"%s\" x)))"))
>>   (setq x 0)
>>   (switch-to-buffer (get-buffer-create "*buffer1*"))
>>   (insert form)
>>   (split-window)
>>   (other-window 1)
>>   (switch-to-buffer (get-buffer-create "*buffer2*"))
>>   (insert form)
>>   (other-window 1))
>
> Sorry, the patch worked only when the selected window did not change.
> Please try the one below.
>


Thanks, it all seems to work now as I would expect, including in
Org-mode where I initially ran into the problem.

Steve

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

 > Thanks, it all seems to work now as I would expect, including in
 > Org-mode where I initially ran into the problem.

I'll keep my Emacs running with the change for a week or so.  If I don't
see any problems, I'll install it then.  If I forget, please remind me.

Thanks, martin

bug#12208: yes-or-no-p escapes with-current-buffer

[martin rudalics]

> === modified file 'src/window.c'
> --- src/window.c    2012-08-18 06:06:39 +0000
> +++ src/window.c    2012-08-18 13:09:15 +0000
> @@ -5889,7 +5889,13 @@
>      }
> 
>    if (!NILP (new_current_buffer))
> -    Fset_buffer (new_current_buffer);
> +    {
> +      Fset_buffer (new_current_buffer);
> +      /* If the new current buffer doesn't appear in the selected
> +     window, go to its old point.  */
> +      if (!EQ (XWINDOW (data->current_window)->buffer, 
> new_current_buffer))
> +    Fgoto_char (make_number (old_point));
> +    }
> 
>    Vminibuf_scroll_window = data->minibuf_scroll_window;
>    minibuf_selected_window = data->minibuf_selected_window;

Installed in revision 109789 on trunk.  Bug closed.

martin