Info bug

Info bug

[Chong Yidong]

Due to a recent change to Info, I see the following bug:

emacs -Q
M-x info RET -> Point is on the last info link, instead of point-min.

Could anyone who changed Info in the last few days please double-check
your work and see if you introduced this bug?

Re: Info bug

[Juri Linkov]

> Due to a recent change to Info, I see the following bug:
>
> emacs -Q
> M-x info RET -> Point is on the last info link, instead of point-min.
>
> Could anyone who changed Info in the last few days please double-check
> your work and see if you introduced this bug?

This is caused by the following change in `Info-complete-menu-item':

@@ -2271,8 +2262,7 @@
   ;; Note that `Info-complete-menu-buffer' could be current already,
   ;; so we want to save point.
-  (save-excursion
-    (set-buffer Info-complete-menu-buffer)
+  (with-current-buffer Info-complete-menu-buffer
     (let ((completion-ignore-case t)
 	  (case-fold-search t)
 	  (orignode Info-current-node)

In theory, these constructs should be equivalent, but they are not.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Info bug

[Nick Roberts]

 > This is caused by the following change in `Info-complete-menu-item':
 > 
 > @@ -2271,8 +2262,7 @@
 >    ;; Note that `Info-complete-menu-buffer' could be current already,
 >    ;; so we want to save point.
 > -  (save-excursion
 > -    (set-buffer Info-complete-menu-buffer)
 > +  (with-current-buffer Info-complete-menu-buffer
 >      (let ((completion-ignore-case t)
 >  	  (case-fold-search t)
 >  	  (orignode Info-current-node)
 > 
 > In theory, these constructs should be equivalent, but they are not.

with-current-buffer is equivalent to

(save-current-buffer
  (set-buffer...

but I can't see any mention of Info-complete-menu-item in the ChangeLog.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Info bug

[Chong Yidong]

Juri Linkov <juri@jurta.org> writes:

>> Due to a recent change to Info, I see the following bug:
>>
>> emacs -Q
>> M-x info RET -> Point is on the last info link, instead of point-min.
>
> This is caused by the following change in `Info-complete-menu-item':
>
> @@ -2271,8 +2262,7 @@
>    ;; Note that `Info-complete-menu-buffer' could be current already,
>    ;; so we want to save point.
> -  (save-excursion
> -    (set-buffer Info-complete-menu-buffer)
> +  (with-current-buffer Info-complete-menu-buffer
>      (let ((completion-ignore-case t)
>  	  (case-fold-search t)
>  	  (orignode Info-current-node)

Could you revert it?  Thanks.

Re: Info bug

[Stefan Monnier]

>> Due to a recent change to Info, I see the following bug:
>> 
>> emacs -Q
>> M-x info RET -> Point is on the last info link, instead of point-min.
>> 
>> Could anyone who changed Info in the last few days please double-check
>> your work and see if you introduced this bug?

> This is caused by the following change in `Info-complete-menu-item':

> @@ -2271,8 +2262,7 @@
>    ;; Note that `Info-complete-menu-buffer' could be current already,
>    ;; so we want to save point.
> -  (save-excursion
> -    (set-buffer Info-complete-menu-buffer)
> +  (with-current-buffer Info-complete-menu-buffer
>      (let ((completion-ignore-case t)
>  	  (case-fold-search t)
>  	  (orignode Info-current-node)

> In theory, these constructs should be equivalent, but they are not.

I'll take care of it,


        Stefan

Re: Info bug

[David Kastrup]

Juri Linkov <juri@jurta.org> writes:

>> Due to a recent change to Info, I see the following bug:
>>
>> emacs -Q
>> M-x info RET -> Point is on the last info link, instead of point-min.
>>
>> Could anyone who changed Info in the last few days please double-check
>> your work and see if you introduced this bug?
>
> This is caused by the following change in `Info-complete-menu-item':
>
> @@ -2271,8 +2262,7 @@
>    ;; Note that `Info-complete-menu-buffer' could be current already,
>    ;; so we want to save point.
> -  (save-excursion
> -    (set-buffer Info-complete-menu-buffer)
> +  (with-current-buffer Info-complete-menu-buffer
>      (let ((completion-ignore-case t)
>  	  (case-fold-search t)
>  	  (orignode Info-current-node)
>
> In theory, these constructs should be equivalent, but they are not.

Read the comment above the change.  It describes the rationale for the
original code and does not jibe with the new code.

You are probably thinking of save-current-buffer rather than
save-excursion.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Compilation auto-jump bug (was: Info bug)

[Juri Linkov]

>  > @@ -2271,8 +2262,7 @@
>  >    ;; Note that `Info-complete-menu-buffer' could be current already,
>  >    ;; so we want to save point.
>  > -  (save-excursion
>  > -    (set-buffer Info-complete-menu-buffer)
>  > +  (with-current-buffer Info-complete-menu-buffer
>  >      (let ((completion-ignore-case t)
>  >  	  (case-fold-search t)
>  >  	  (orignode Info-current-node)
>  >
>  > In theory, these constructs should be equivalent, but they are not.
>
> with-current-buffer is equivalent to
>
> (save-current-buffer
>   (set-buffer...

BTW, there is also the opposite bug: when `compilation-scroll-output' is
`first-error', but `compilation-auto-jump-to-first-error' is nil, when
the current buffer is not the *compilation* buffer, then it doesn't keep
point on the first error.  This means when I switch to the *compilation*
buffer, point is at the beginning of the *compilation* buffer instead of
the position of the first error.  But when the current buffer is the
*compilation* buffer at the time when compilation moves point, then it
keeps the correct position of the first error.  This is due to the code:

(defun compilation-auto-jump (buffer pos)
  (with-current-buffer buffer
    (goto-char pos)
    (if compilation-auto-jump-to-first-error
	(compile-goto-error))))

It doesn't use `save-excursion', so it is strange that it doesn't keep
point on the new position after `goto-char'.

But when `compilation-auto-jump-to-first-error' is t, then
after calling `compile-goto-error' and switching to the source buffer,
point keeps its new position on the first error in the *compilation* buffer.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Compilation auto-jump bug

[Stefan Monnier]

> BTW, there is also the opposite bug: when `compilation-scroll-output' is
> `first-error', but `compilation-auto-jump-to-first-error' is nil, when
> the current buffer is not the *compilation* buffer, then it doesn't keep
> point on the first error.  This means when I switch to the *compilation*
> buffer, point is at the beginning of the *compilation* buffer instead of
> the position of the first error.  But when the current buffer is the
> *compilation* buffer at the time when compilation moves point, then it
> keeps the correct position of the first error.  This is due to the code:

> (defun compilation-auto-jump (buffer pos)
>   (with-current-buffer buffer
>     (goto-char pos)
>     (if compilation-auto-jump-to-first-error
> 	(compile-goto-error))))

> It doesn't use `save-excursion', so it is strange that it doesn't keep
> point on the new position after `goto-char'.

I guess the problem is that we need to affect the window-point of the
window that's showing the compilation buffer and this only happens when
we do `goto-char' if that window is the selected one.
So maybe something like:

 (defun compilation-auto-jump (buffer pos)
   (with-current-buffer buffer
     (goto-char pos)
     (let ((win (get-buffer-window buffer 0)))
       (if win (set-window-point win pos)))
     (if compilation-auto-jump-to-first-error
         (compile-goto-error))))


-- Stefan

Re: Compilation auto-jump bug

[Juri Linkov]

>> BTW, there is also the opposite bug: when `compilation-scroll-output' is
>> `first-error', but `compilation-auto-jump-to-first-error' is nil, when
>> the current buffer is not the *compilation* buffer, then it doesn't keep
>> point on the first error.  This means when I switch to the *compilation*
>> buffer, point is at the beginning of the *compilation* buffer instead of
>> the position of the first error.  But when the current buffer is the
>> *compilation* buffer at the time when compilation moves point, then it
>> keeps the correct position of the first error.  This is due to the code:
>
>> (defun compilation-auto-jump (buffer pos)
>>   (with-current-buffer buffer
>>     (goto-char pos)
>>     (if compilation-auto-jump-to-first-error
>> 	(compile-goto-error))))
>
>> It doesn't use `save-excursion', so it is strange that it doesn't keep
>> point on the new position after `goto-char'.
>
> I guess the problem is that we need to affect the window-point of the
> window that's showing the compilation buffer and this only happens when
> we do `goto-char' if that window is the selected one.
> So maybe something like:
>
>  (defun compilation-auto-jump (buffer pos)
>    (with-current-buffer buffer
>      (goto-char pos)
>      (let ((win (get-buffer-window buffer 0)))
>        (if win (set-window-point win pos)))
>      (if compilation-auto-jump-to-first-error
>          (compile-goto-error))))

Yes, it works.  Maybe we should add a primitive for this additional code
with a name like `save-window-point'?  If this name can be confused with
a function that keeps the old window point before entering its body, then
maybe some other name would be more clear.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Compilation auto-jump bug

[Stefan Monnier]

>> (defun compilation-auto-jump (buffer pos)
>> (with-current-buffer buffer
>> (goto-char pos)
>> (let ((win (get-buffer-window buffer 0)))
>> (if win (set-window-point win pos)))
>> (if compilation-auto-jump-to-first-error
>> (compile-goto-error))))

> Yes, it works.  Maybe we should add a primitive for this additional code
> with a name like `save-window-point'?  If this name can be confused with
> a function that keeps the old window point before entering its body, then
> maybe some other name would be more clear.

We could provide

  (defun goto-char-window (pos)
    "Like `goto-char' but also moves `window-point'."
    (goto-char pos)
    (let ((win (get-buffer-window (current-buffer) 0)))
      (if win (set-window-point win pos))))

This kind of thing is needed at various places, so it might be
a good idea.  If someone could grep through the Elisp code to try and
find the various places where we do something similar and try to see if
the above function is a good fit, that would be helpful,


        Stefan