bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Markus Triska]

When you do:

   $ emacs -Q -f linum-mode

and then:

   C-x 2

you obtain one frame with two windows, the one on top being selected.

When you then do:

   M-: (switch-to-buffer-other-window "test") RET

the selected window is unexpectedly reused to display the new buffer.

In GNU Emacs 23.0.60.11 (i386-apple-darwin8.11.1, GTK+ Version 2.12.9)
 of 2008-10-14 on mt-computer.local
Windowing system distributor `The XFree86 Project, Inc', version 11.0.40400000
Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: nil
  default-enable-multibyte-characters: t

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > When you do:
 >
 >    $ emacs -Q -f linum-mode
 >
 > and then:
 >
 >    C-x 2
 >
 > you obtain one frame with two windows, the one on top being selected.
 >
 > When you then do:
 >
 >    M-: (switch-to-buffer-other-window "test") RET
 >
 > the selected window is unexpectedly reused to display the new buffer.

FWIW, the problem seems that `linum-mode' indirectly changes the value
of windows' use_time so the value returned by `get-lru-window' is messy.
use_time is updated by `select-window', hence a potential culprit is
`walk-windows' which is used by `linum-mode'.  Could you try to find out
whether my guess is correct and/or where and how use_time is set when
`linum-mode' is active?

Thanks, martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Markus Triska]

martin rudalics <rudalics@gmx.at> writes:

> of windows' use_time so the value returned by `get-lru-window' is messy.
> use_time is updated by `select-window', hence a potential culprit is
> `walk-windows' which is used by `linum-mode'.

You are right that `get-lru-window' seems not to do the right thing in
effect (it evaluates to the currently selected window even though there
is another one in the frame). `select-window' seems not to be called in
`walk-windows' as used by linum though, since the third argument is not
a frame. I found that when you set `window-configuration-change-hook' to
nil in *scratch* before `switch-to-buffer-other-window', it chooses the
right buffer. I do not yet know if/where use_time changes.

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > I found that when you set `window-configuration-change-hook' to
 > nil in *scratch* before `switch-to-buffer-other-window', it chooses the
 > right buffer.

Does it change anything when you remove the `walk-windows' from your
function?  Then we would know that `walk-windows' is responsible for
triggering it.  Maybe run_window_configuration_change_hook messes it up
after `walk-windows'.

 > I do not yet know if/where use_time changes.

Could you try to run this with GDB, set a breakpoint in select_window at
the

       XSETFASTINT (w->use_time, window_select_count);

line, and get a backtrace when(ever) this is hit for the lower window?

martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Markus Triska]

I constructed the following self-contained case that I hope helps you:

   (defun my-walk () (walk-windows 'identity))

   (add-hook 'window-configuration-change-hook 'my-walk nil t)

   (split-window)

   (switch-to-buffer-other-window "test")

If you put these forms in the *scratch* buffer of "emacs -Q" and then
evaluate them one after another, you should be able to see the problem.
The problem does not occur if the hook is NOT made buffer-local.

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 >    (defun my-walk () (walk-windows 'identity))
 >
 >    (add-hook 'window-configuration-change-hook 'my-walk nil t)
 >
 >    (split-window)
 >
 >    (switch-to-buffer-other-window "test")
 >
 > If you put these forms in the *scratch* buffer of "emacs -Q" and then
 > evaluate them one after another, you should be able to see the problem.
 > The problem does not occur if the hook is NOT made buffer-local.

Yes.  Replacing `split-window' by

(progn
   (split-window)
   (message "sel %s .. lru %s" (selected-window) (get-lru-window)))

shows the problem immediately.  Everything seems alright when you put
that message into my-walk.

run_window_configuration_change_hook selects each window before running
the hook buffer-locally which makes any reasoning (within the function
run by the hook) about which window will be selected when the command
completes void.  In addition it unwinds with select_window_norecord
which won't restore the current buffer when the selected window gets
reselected.  Both of these don't explain the problem that somewhere
`select-window' must get called with norecord nil :-(

martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > run_window_configuration_change_hook selects each window before running
 > the hook buffer-locally which makes any reasoning (within the function
 > run by the hook) about which window will be selected when the command
 > completes void.  In addition it unwinds with select_window_norecord
 > which won't restore the current buffer when the selected window gets
 > reselected.  Both of these don't explain the problem that somewhere
 > `select-window' must get called with norecord nil :-(

OK.  The reason may be twofold: `save-selected-window' may twice
select a window with norecord nil:

		(set-frame-selected-window (car elt) (cdr elt)))) ; <--- here
	 (if (window-live-p save-selected-window-window)
	     (select-window save-selected-window-window)))))) <--- and here

Fixing these two fixes the bug.  I have to find out how to do this in an
unobtrusive way though.

martin, who'd never ever run `window-configuration-change-hook'
buffer-locally in the first place ;-)

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Markus Triska]

martin rudalics <rudalics@gmx.at> writes:

> 		(set-frame-selected-window (car elt) (cdr elt)))) ; <--- here
> 	 (if (window-live-p save-selected-window-window)
> 	     (select-window save-selected-window-window)))))) <--- and here
>
> Fixing these two fixes the bug.  I have to find out how to do this in an
> unobtrusive way though.

Would it suffice if `run_window_configuration_change_hook' re-selected
the selected window at the end? i.e.,

2008-10-25  Markus Triska  <markus.triska@gmx.at>

	* window.c (run_window_configuration_change_hook): Re-select the
	selected window, to get the most recently used window right.

diff --git a/src/window.c b/src/window.c
index 5931955..28e089a 100644
--- a/src/window.c
+++ b/src/window.c
@@ -3382,6 +3382,9 @@ run_window_configuration_change_hook (struct frame *f)
       }
   }
 
+  /* Make sure that the selected window remains most recently used. */
+  Fselect_window (Fselected_window (), Qnil);
+
   run_funs (global_wcch);
   unbind_to (count, Qnil);
 }

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > Would it suffice if `run_window_configuration_change_hook' re-selected
 > the selected window at the end? i.e.,

It should handle your two windows case.  I'm not sure what happens with
three or more windows.  The problem is that we want the _least_ recently
used window and the `select-window' would not fix anything messed up
down there.  In some sense this virtual list of windows ordered by their
use_time gets rotated with every call of `walk-windows'.

The real problem is the unholy combination of
run_window_configuration_change_hook and `walk-windows'.  The former
carefully selects each window _without_ recording it and the latter
annihilates the former's efforts by selecting the window _and_ recording
it.  I tried with a `save-selected-window-norecord' macro and equipped
`set-frame-selected-window' with an extra "norecord" parameter.  This
works but is quite clumsy.  Also, I'm by no means sure whether the
effect of recorded selecting a window within `walk-windows' might have
been considered a feature by other callers.

martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Stefan Monnier]

> I constructed the following self-contained case that I hope helps you:

>    (defun my-walk () (walk-windows 'identity))
>    (add-hook 'window-configuration-change-hook 'my-walk nil t)
>    (split-window)
>    (switch-to-buffer-other-window "test")

In Emacs-22, window-configuration-change-hook did not work correctly
with buffer-local hooks, but in some cases you could make it work
somewhat by using `walk-window' in your hook to try and cover all the
affected windows.

With the code I installed to properly support buffer-local hooks on
window-configuration-change-hook, the walk-window should be unnecessary,
so the patch below should not break linum and might remove the problem.

This said, walk-windows should probably use `norecord', what do
people think?


        Stefan


=== modified file 'lisp/linum.el'
--- lisp/linum.el	2008-08-31 19:46:54 +0000
+++ lisp/linum.el	2008-10-25 19:06:30 +0000
@@ -82,17 +82,22 @@
                                            'linum-update-current) nil t)
           (add-hook 'after-change-functions 'linum-after-change nil t))
         (add-hook 'window-scroll-functions 'linum-after-scroll nil t)
+        ;; FIXME: using both window-size-change-functions and
+        ;; window-configuration-change-hook seems redundant.
         (add-hook 'window-size-change-functions 'linum-after-size nil t)
         (add-hook 'change-major-mode-hook 'linum-delete-overlays nil t)
         (add-hook 'window-configuration-change-hook
-                  'linum-after-config nil t)
+                  ;; FIXME: If the buffer is shows in N windows, this
+                  ;; will be called N times rather than once.  We should use
+                  ;; something like linum-update-window instead.
+                  'linum-update-current nil t)
         (linum-update-current))
     (remove-hook 'post-command-hook 'linum-update-current t)
     (remove-hook 'post-command-hook 'linum-schedule t)
     (remove-hook 'window-size-change-functions 'linum-after-size t)
     (remove-hook 'window-scroll-functions 'linum-after-scroll t)
     (remove-hook 'after-change-functions 'linum-after-change t)
-    (remove-hook 'window-configuration-change-hook 'linum-after-config t)
+    (remove-hook 'window-configuration-change-hook 'linum-update-current t)
     (remove-hook 'change-major-mode-hook 'linum-delete-overlays t)
     (linum-delete-overlays)))
 

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Markus Triska]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> This said, walk-windows should probably use `norecord', what do
> people think?

Yes, I find it very desirable that `walk-windows' have no side-effects.

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Lennart Borgman]

On Wed, Oct 29, 2008 at 2:25 PM, Markus Triska <markus.triska@gmx.at> wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> This said, walk-windows should probably use `norecord', what do
>> people think?
>
> Yes, I find it very desirable that `walk-windows' have no side-effects.

Me too.

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

I've been looking into this and found out that in order to fix problems
in this area we need an optional "norecord" argument for `select-frame'
and `set-frame-selected-window'.  Both of these may (indirectly) call
`select-window' and thus mess with the order of recently selected
windows (and probably also the buffer list).

As a rule, the norecord argument would be set by `with-selected-window'
and `with-selected-frame' in addition to `walk-windows'.  Personally,
I'd set it in `save-selected-window' too, but this might break
applications that rely on the implicit selection provided by the current
mechanism.

The changes are trivial and not very interesting to post.  So please
tell me whether you want them installed.

martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Stefan Monnier]

> I've been looking into this and found out that in order to fix problems
> in this area we need an optional "norecord" argument for `select-frame'
> and `set-frame-selected-window'.  Both of these may (indirectly) call
> `select-window' and thus mess with the order of recently selected
> windows (and probably also the buffer list).

> As a rule, the norecord argument would be set by `with-selected-window'
> and `with-selected-frame' in addition to `walk-windows'.  Personally,
> I'd set it in `save-selected-window' too, but this might break
> applications that rely on the implicit selection provided by the current
> mechanism.

> The changes are trivial and not very interesting to post.  So please
> tell me whether you want them installed.

That sounds good, please install them.


        Stefan

bug#1237: marked as done (23.0.60; `switch-to-buffer-other-window' sometimes uses selected window)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]


Your message dated Sun, 02 Nov 2008 12:14:05 +0100
with message-id <490D8B7D.3020008@gmx.at>
and subject line Re: bug#1237: 23.0.60;	`switch-to-buffer-other-window' sometimes uses selected window
has caused the Emacs bug report #1237,
regarding 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
1237: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=1237
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 2841 bytes --]

From: Markus Triska <markus.triska@gmx.at>
To: emacs-pretest-bug@gnu.org
Subject: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window
Date: Fri, 24 Oct 2008 12:27:30 +0200 (CEST)
Message-ID: <20081024102730.972E2B43B34@mt-computer.local>


When you do:

   $ emacs -Q -f linum-mode

and then:

   C-x 2

you obtain one frame with two windows, the one on top being selected.

When you then do:

   M-: (switch-to-buffer-other-window "test") RET

the selected window is unexpectedly reused to display the new buffer.

In GNU Emacs 23.0.60.11 (i386-apple-darwin8.11.1, GTK+ Version 2.12.9)
 of 2008-10-14 on mt-computer.local
Windowing system distributor `The XFree86 Project, Inc', version 11.0.40400000
Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: nil
  default-enable-multibyte-characters: t




[-- Attachment #3: Type: message/rfc822, Size: 1727 bytes --]

From: martin rudalics <rudalics@gmx.at>
To: 1237-done@emacsbugs.donarmstrong.com
Cc: Markus Triska <markus.triska@gmx.at>
Subject: Re: bug#1237: 23.0.60;	`switch-to-buffer-other-window' sometimes uses selected window
Date: Sun, 02 Nov 2008 12:14:05 +0100
Message-ID: <490D8B7D.3020008@gmx.at>

Fixed as

2008-11-02  Martin Rudalics  <rudalics@gmx.at>

	* window.el (save-selected-window-norecord): New macro
	(walk-windows): Use save-selected-window-norecord and call
	select-window with NORECORD set.  (Bug#1237)

Thanks for the report, martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Richard M. Stallman]

    > As a rule, the norecord argument would be set by `with-selected-window'
    > and `with-selected-frame' in addition to `walk-windows'.  Personally,
    > I'd set it in `save-selected-window' too, but this might break
    > applications that rely on the implicit selection provided by the current
    > mechanism.

I think it is very important to keep `with-selected-window'
and `save-selected-window' similar.  If we make this change
in one, we had better make the change in the other too.

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > I think it is very important to keep `with-selected-window'
 > and `save-selected-window' similar.  If we make this change
 > in one, we had better make the change in the other too.

I think so too.  But the behavior was different (in the NORECORD aspect)
already before my recent change.

martin

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Richard M. Stallman]

     > I think it is very important to keep `with-selected-window'
     > and `save-selected-window' similar.  If we make this change
     > in one, we had better make the change in the other too.

    I think so too.  But the behavior was different (in the NORECORD aspect)
    already before my recent change.

Was that intentional or was it a bug?

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[Stefan Monnier]

>> I think it is very important to keep `with-selected-window'
>> and `save-selected-window' similar.  If we make this change
>> in one, we had better make the change in the other too.

>     I think so too.  But the behavior was different (in the NORECORD aspect)
>     already before my recent change.

> Was that intentional or was it a bug?

The fact that with-selected-window does not "record" it very
much intentional.  The "norecord" argument was added (to select-window)
specifically for that purpose.

I believe its absence in save-selected-window is only accidental.


        Stefan

bug#1237: 23.0.60; `switch-to-buffer-other-window' sometimes uses selected window

[martin rudalics]

 > The fact that with-selected-window does not "record" it very
 > much intentional.  The "norecord" argument was added (to select-window)
 > specifically for that purpose.
 >
 > I believe its absence in save-selected-window is only accidental.

I hopefully removed all accidental absences by now ;-)

martin