BUG: which-func-mode

BUG: which-func-mode

[Le Wang]

Hi,

I just spend a few hours debugging a situation where something was modifying 
my buffer-list behind my back, whenever I had multiple windows in the same 
frame.

It turned out that `which-func-mode' was using `walk-windows' to update the 
mode-lines in all windows.  It selects each window and forces a mode-line 
update in it.  But by selecting the window, it silently disrupts the 
`buffer-list'.  This is the bug.

If I've failed to make the problem clear for everyone, please let me know and 
I'll provide a step by step test case.  But I think this one should be fairly 
obvious.

This is the patch I've come up with, again, please provide corrections where 
required:

*** /usr/local/share/emacs/21.3.50/lisp/which-func.el   Tue Feb  4 19:31:41 2003
--- /tmp/buffer-content-5706yVU Tue Mar  4 19:31:46 2003
***************
*** 158,176 ****
  
  (defun which-func-update-1 (window)
    "Update the Which-Function mode display for window WINDOW."
!   (save-selected-window
!     (select-window window)
!     ;; Update the string containing the current function.
!     (when which-func-mode
!       (condition-case info
!         (progn
!           (setq which-func-current (or (which-function) which-func-unknown))
!           (unless (string= which-func-current which-func-previous)
!             (force-mode-line-update)
!             (setq which-func-previous which-func-current)))
!       (error
!        (which-func-mode -1)
!        (error "Error in which-func-update: %s" info))))))
  
  ;;;###autoload
  (defalias 'which-func-mode 'which-function-mode)
--- 158,175 ----
  
  (defun which-func-update-1 (window)
    "Update the Which-Function mode display for window WINDOW."
!   (set-buffer (window-buffer window))
!   ;; Update the string containing the current function.
!   (when which-func-mode
!     (condition-case info
!       (progn
!         (setq which-func-current (or (which-function) which-func-unknown))
!         (unless (string= which-func-current which-func-previous)
!           (force-mode-line-update)
!           (setq which-func-previous which-func-current)))
!       (error
!        (which-func-mode -1)
!        (error "Error in which-func-update: %s" info)))))
  
  ;;;###autoload
  (defalias 'which-func-mode 'which-function-mode)

Diff finished at Tue Mar  4 19:31:46


--
Le

Re: BUG: which-func-mode

[Stefan Monnier]

> I just spend a few hours debugging a situation where something was modifying 
> my buffer-list behind my back, whenever I had multiple windows in the same 
> frame.
> 
> It turned out that `which-func-mode' was using `walk-windows' to update the
> mode-lines in all windows.  It selects each window and forces a mode-line
> update in it.  But by selecting the window, it silently disrupts the
> `buffer-list'.  This is the bug.

Yes.  We need a lower-level form of `select-window' for such uses.
Actually I'd recommend naming it `with-selected-window'.

> !   (save-selected-window
> !     (select-window window)
[...]

> !   (set-buffer (window-buffer window))

I think the patch is basically more-or-less right, except for the following
issues:

- I'd use with-current-buffer over set-buffer as a matter of style.
  After all, the function's name and docstring doesn't suggest that
  it might change the current buffer.

- I'm wondering whether it's good to update all the windows.
  After all, when you have a hundred windows, it's pretty silly to
  update them all all the time.

- this brings up the old issue of buffers vs windows: the code obviously
  wants to treat each window independently, so that it does TRT if you have
  two windows showing the same buffer (at two different positions), but
  it actually odesn't work because it ends up relying on a buffer-local
  variable (there are no window-local variables in Emacs).
  Your patch thus doesn't really change the behavior but makes
  the code odd since it cycles through windows but only selects the
  corresponding buffers.

The patch below fixes the buffer/window issue and removes the "update all"
behavior.  Maybe the "update all" behvaior should be restored, but it
requires the lower-level select-window function mentioned above.

Also I noticed while hacking it up that I need to set the
risky-local-variable property on both which-func-format and
which-func-current although it seems it should only be
needed on which-func-current.  What do people think ?


	Stefan


--- which-func.el.~1.29.~	Wed Feb  5 10:55:11 2003
+++ which-func.el	Mon Mar 10 13:22:41 2003
@@ -103,6 +103,7 @@
   "Format for displaying the function in the mode line."
   :group 'which-func
   :type 'sexp)
+(put 'which-func-format 'risky-local-variable t)
 
 (defvar which-func-cleanup-function nil
   "Function to transform a string before displaying it in the mode line.
@@ -120,10 +121,11 @@
 ;;;
 (require 'imenu)
 
-(defvar which-func-current  which-func-unknown)
-(defvar which-func-previous which-func-unknown)
-(make-variable-buffer-local 'which-func-current)
-(make-variable-buffer-local 'which-func-previous)
+(defvar which-func-table (make-hash-table :test 'eq :weakness 'key))
+
+(defvar which-func-current
+  '(:eval (gethash (selected-window) which-func-table which-func-unknown)))
+(put 'which-func-current 'risky-local-variable t)
 
 (defvar which-func-mode nil
   "Non-nil means display current function name in mode line.
@@ -153,24 +155,19 @@
      (setq which-func-mode nil))))
 
 (defun which-func-update ()
-  "Update the Which-Function mode display for all windows."
-  (walk-windows 'which-func-update-1 nil 'visible))
-
-(defun which-func-update-1 (window)
-  "Update the Which-Function mode display for window WINDOW."
-  (save-selected-window
-    (select-window window)
-    ;; Update the string containing the current function.
+  ;; "Update the Which-Function mode display for all windows."
+  ;; (walk-windows 'which-func-update-1 nil 'visible))
+  "Update the Which-Function mode display for the current window."
     (when which-func-mode
       (condition-case info
-	  (progn
-	    (setq which-func-current (or (which-function) which-func-unknown))
-	    (unless (string= which-func-current which-func-previous)
-	      (force-mode-line-update)
-	      (setq which-func-previous which-func-current)))
+	(let ((current (which-function)))
+	  (unless (string= current
+			   (gethash (selected-window) which-func-table))
+	    (puthash (selected-window) current which-func-table)
+	    (force-mode-line-update)))
 	(error
 	 (which-func-mode -1)
-	 (error "Error in which-func-update: %s" info))))))
+       (error "Error in which-func-update: %s" info)))))
 
 ;;;###autoload
 (defalias 'which-func-mode 'which-function-mode)

Re: BUG: which-func-mode

[Richard Stallman]

    > It turned out that `which-func-mode' was using `walk-windows' to update the
    > mode-lines in all windows.  It selects each window and forces a mode-line
    > update in it.  But by selecting the window, it silently disrupts the
    > `buffer-list'.  This is the bug.

I tried to fix this in one way last fall, by changing select-window
not to alter the frame-selected-window in some cases.  That caused
other problems.  So in December I undid that change
and tried another fix:

    2002-12-23  Richard M. Stallman  <rms@gnu.org>

	* window.el (save-selected-window): Save and restore
	selected windows of all frames.

If this doesn't fix te problem, why doesn't it?

Re: BUG: which-func-mode

[Stefan Monnier]

>     > It turned out that `which-func-mode' was using `walk-windows' to update the
>     > mode-lines in all windows.  It selects each window and forces a mode-line
>     > update in it.  But by selecting the window, it silently disrupts the
>     > `buffer-list'.  This is the bug.
> 
> I tried to fix this in one way last fall, by changing select-window
> not to alter the frame-selected-window in some cases.  That caused
> other problems.  So in December I undid that change
> and tried another fix:
> 
>     2002-12-23  Richard M. Stallman  <rms@gnu.org>
> 
> 	* window.el (save-selected-window): Save and restore
> 	selected windows of all frames.
> 
> If this doesn't fix te problem, why doesn't it?

In his case, it seems the problem is not that the frame's selected
window is changed (which was the problem you're referring to), but
that the buffer-list is changed because Fselect_window calls
`select_window_1 (window, 1)' where the second arg causes select_window_1
to call `record_buffer (w->buffer)'.


	Stefan

Re: BUG: which-func-mode

[Richard Stallman]

I see two choices: either document that select-window can change the
buffer list order and that save-selected-window does not protect
against that, or else change save-selected-window to save and restore
the buffer list order.  I think the former is better.

Any other ideas?  Does anyone want to write it?

Meanwhile, the patch to which-func-mode seems like the right approach
there.

Re: BUG: which-func-mode

[Stefan Monnier]

> I see two choices: either document that select-window can change the
> buffer list order and that save-selected-window does not protect
> against that, or else change save-selected-window to save and restore
> the buffer list order.  I think the former is better.

I don't necessarily like the latter, but the former doesn't solve the
problem of "how do I temporarily select another window while preventing
buffer-list from being changed?".

I think that it would be better if select-window did not change
the buffer-list.  Why does it do it in the first place ?
It seems it should be the job of things like switch-to-buffer
and other-window.


	Stefan

Re: BUG: which-func-mode

[Richard Stallman]

    I think that it would be better if select-window did not change
    the buffer-list.  Why does it do it in the first place ?

Because it changes the selected buffer.

Re: BUG: which-func-mode

[Stefan Monnier]

>     I think that it would be better if select-window did not change
>     the buffer-list.  Why does it do it in the first place ?
> 
> Because it changes the selected buffer.

I don't understand why one should imply the other.
After all, `set-buffer' also changes the selected buffer but it doesn't
modify the buffer-list.


	Stefan

Re: BUG: which-func-mode

[Robert J. Chassell]

   >     I think that it would be better if select-window did not
   >     change the buffer-list.  Why does it do it in the first place?
   > 
   > Because it changes the selected buffer.

   I don't understand why one should imply the other.
   After all, `set-buffer' also changes the selected buffer but it
   doesn't modify the buffer-list.

`set-buffer' works differently than `switch-to-buffer' or
`select-window'.  As its documentation says, `set-buffer'

    ... does not display the buffer, so its effect ends
    when the current command terminates.

This is in contrast with `switch-to-buffer' or `select-window'.

Moreover, as said in

    (elisp)The Buffer List 

    The "buffer list" is a list of all live buffers.  Creating a
    buffer adds it to this list, and killing a buffer excises it.  The
    order of the buffers in the list is based primarily on how
    recently each buffer has been displayed in the selected window.
    Buffers move to the front of the list when they are selected and
    to the end when they are buried (see `bury-buffer', below).
    Several functions, notably `other-buffer', use this ordering.  A
    buffer list displayed for the user also follows this order.

I did not deal with `select-window', but in my "Introduction to
Programming in Emacs Lisp", I did explain the difference between
`switch-to-buffer' and `set-buffer' like this:

    (eintr)Switching Buffers

    `switch-to-buffer' is designed for humans and does two different
    things: it switches the buffer to which Emacs' attention is
    directed; and it switches the buffer displayed in the window to
    the new buffer.  `set-buffer', on the other hand, does only one
    thing: it switches the attention of the computer program to a
    different buffer.  The buffer on the screen remains unchanged (of
    course, normally nothing happens there until the command finishes
    running).

Thus `set-buffer' should *not* effect the buffer list, which is
intended to list buffers displayed for humans in the selected window.

Perhaps the Emacs manual or Emacs Lisp Reference Manuals are not
sufficiently explicit about the difference among different kinds of
`set', `switch-to', and `select' functions; I don't see the problem,
but maybe the documentation should be improved.

-- 
    Robert J. Chassell                         Rattlesnake Enterprises
    http://www.rattlesnake.com                  GnuPG Key ID: 004B4AC8
    http://www.teak.cc                             bob@rattlesnake.com

Re: BUG: which-func-mode

[Stefan Monnier]

>    >     I think that it would be better if select-window did not
>    >     change the buffer-list.  Why does it do it in the first place?
>    > 
>    > Because it changes the selected buffer.
> 
>    I don't understand why one should imply the other.
>    After all, `set-buffer' also changes the selected buffer but it
>    doesn't modify the buffer-list.
> 
> `set-buffer' works differently than `switch-to-buffer' or
> `select-window'.  As its documentation says, `set-buffer'
> 
>     ... does not display the buffer, so its effect ends
>     when the current command terminates.
> 
> This is in contrast with `switch-to-buffer' or `select-window'.

select-window does not "display the buffer" either.

> Moreover, as said in
> 
>     (elisp)The Buffer List 
> 
>     The "buffer list" is a list of all live buffers.  Creating a
>     buffer adds it to this list, and killing a buffer excises it.  The
>     order of the buffers in the list is based primarily on how
>     recently each buffer has been displayed in the selected window.
>     Buffers move to the front of the list when they are selected and
>     to the end when they are buried (see `bury-buffer', below).
>     Several functions, notably `other-buffer', use this ordering.  A
>     buffer list displayed for the user also follows this order.

This only talks about events where the buffer displayed in a window
is changed rather than events where the selected window is changed.

Furthermore, select-window is not a command and in the case at hand, it
is used to *temporarily* switch to some other window.

> I did not deal with `select-window', but in my "Introduction to
> Programming in Emacs Lisp", I did explain the difference between
> `switch-to-buffer' and `set-buffer' like this:

I'm not comparing switch-to-buffer to set-buffer:
I'm comparing select-window to set-buffer.
I of course agree 100% with the current behavior which does not change
the buffer-list in set-buffer but changes it in switch-to-buffer.
OTOH, I still haven't heard any good reason why select-window should
fiddle with the buffer-list.


	Stefan

Re: BUG: which-func-mode

[Robert J. Chassell]

You are right, there is a confusion regarding `select-window'.

As you point out

    select-window does not "display the buffer"

and as is said in

    (elisp)The Buffer List 

    .... The order of the buffers in the list is based primarily on
    how recently each buffer has been displayed in the selected
    window.  Buffers move to the front of the list when they are
    selected....

Put another way, to be on the buffer list, a buffer must have been:

  * displayed in a window

  * that window selected

But according to its documentation, `select-window' only does the
latter.  It does not do both.

I think the confusion arises because a window always displays
something -- after all that is what a window is.  And in Emacs what a
window displays is a buffer.

Hence, `select-window' must display a buffer of some sort, although
nothing specifies which buffer is displayed.

Consequently, the two criteria for going onto the buffer list are met:
`select-window' must display some buffer in the window, and it also
selects the window.  Unfortunately, the documentation does not make
this clear.

-- 
    Robert J. Chassell                         Rattlesnake Enterprises
    http://www.rattlesnake.com                  GnuPG Key ID: 004B4AC8
    http://www.teak.cc                             bob@rattlesnake.com

Re: BUG: which-func-mode

[Richard Stallman]

    You are right, there is a confusion regarding `select-window'.

I will clarify this in the Lisp manual.  Thanks.

Re: BUG: which-func-mode

[Richard Stallman]

    > Because it changes the selected buffer.

    I don't understand why one should imply the other.
    After all, `set-buffer' also changes the selected buffer but it doesn't
    modify the buffer-list.

set-buffer only changes the current buffer; it doesn't change
the selected buffer, which is the buffer that is displayed
in the selected window.

Re: BUG: which-func-mode

[Stefan Monnier]

>     > Because it changes the selected buffer.
> 
>     I don't understand why one should imply the other.
>     After all, `set-buffer' also changes the selected buffer but it doesn't
>     modify the buffer-list.
> 
> set-buffer only changes the current buffer; it doesn't change
> the selected buffer, which is the buffer that is displayed
> in the selected window.

Oh, I think I now understand the subtle difference.  Thank you.
So now: how should we make it possible to temporarily select another
window without influencing the buffer-list ?


	Stefan

Re: BUG: which-func-mode

[Richard Stallman]

    So now: how should we make it possible to temporarily select another
    window without influencing the buffer-list ?

You can write a macro that would save buffer-list and afterward put it
back the way it was.

Re: BUG: which-func-mode

[Stefan Monnier]

>     So now: how should we make it possible to temporarily select another
>     window without influencing the buffer-list ?
> 
> You can write a macro that would save buffer-list and afterward put it
> back the way it was.

How can I (re)set the buffer-list from elisp ?
If it requires C coding, then I might as well add a NORECORD argument
to select-window and avoid the whole save/restore.


	Stefan


PS: This is also related to the comment I added a while back
to Fpop_to_buffer:

  if (NILP (norecord))
    /* This seems bogus since Fselect_window will call record_buffer anyway. */
    record_buffer (buf);
  Fselect_window (Fdisplay_buffer (buf, other_window, Qnil));

Re: BUG: which-func-mode

[Richard Stallman]

    How can I (re)set the buffer-list from elisp ?

You could select all the buffers in the desired order.
However, that would alter the current frame's buffer list.
So maybe that new arg to select-window is the best idea.