bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Drew Adams]

In bookmark-jump-other-window and bookmark-bmenu-other-window we call
switch-to-buffer-other-window. We should use pop-to-buffer, instead.
 
With non-nil pop-up-frames, switch-to-buffer-other-window creates a
new frame each time, even if the destination buffer is already showing
in some frame. pop-to-buffer DTRT: it reuses the existing frame.
 

In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[martin rudalics]

 > In bookmark-jump-other-window and bookmark-bmenu-other-window we call
 > switch-to-buffer-other-window. We should use pop-to-buffer, instead.
 >
 > With non-nil pop-up-frames, switch-to-buffer-other-window creates a
 > new frame each time, even if the destination buffer is already showing
 > in some frame. pop-to-buffer DTRT: it reuses the existing frame.

I'm not sure what the problem is here.  `switch-to-buffer-other-window'
has a clear purpose - do _not reuse the selected window_ (which is the
bookmarks window, IIUC).  OTOH `display-buffer-reuse-frames' non-nil
should assure that another frame is reused.

martin

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Drew Adams]

>  > In bookmark-jump-other-window and bookmark-bmenu-other-window we call
>  > switch-to-buffer-other-window. We should use pop-to-buffer, instead.
>  >
>  > With non-nil pop-up-frames, switch-to-buffer-other-window creates a
>  > new frame each time, even if the destination buffer is 
>  > already showing in some frame. pop-to-buffer DTRT: it reuses
>  > the existing frame.
> 
> I'm not sure what the problem is here.  
> `switch-to-buffer-other-window'
> has a clear purpose - do _not reuse the selected window_ (which is the
> bookmarks window, IIUC).  OTOH `display-buffer-reuse-frames' non-nil
> should assure that another frame is reused.

Users should not have to customize a global variable, to prevent a new frame
from being used in particular places like this.

As Stefan says repeatedly (paraphrasing), switch-to-buffer-other-window is
almost always the wrong thing to do, and should be replaced in most places by
pop-to-buffer.

Use of switch-to-buffer-other-window is a bug in general, typically made by
someone who doesn't use non-nil pop-up-frames.

In this particular context, there is no reason to use
switch-to-buffer-other-frame.

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[martin rudalics]

 >> I'm not sure what the problem is here.
 >> `switch-to-buffer-other-window'
 >> has a clear purpose - do _not reuse the selected window_ (which is the
 >> bookmarks window, IIUC).  OTOH `display-buffer-reuse-frames' non-nil
 >> should assure that another frame is reused.
 >
 > Users should not have to customize a global variable, to prevent a new frame
 > from being used in particular places like this.

I thought you wanted to avoid popping up a new frame.  At least in your
first mail you said "pop-to-buffer DTRT: it reuses the existing frame".

 > As Stefan says repeatedly (paraphrasing), switch-to-buffer-other-window is
 > almost always the wrong thing to do, and should be replaced in most places by
 > pop-to-buffer.
 >
 > Use of switch-to-buffer-other-window is a bug in general, typically made by
 > someone who doesn't use non-nil pop-up-frames.
 >
 > In this particular context, there is no reason to use
 > switch-to-buffer-other-frame.

If you have `display-buffer-reuse-frames' set to nil, `pop-to-buffer'
will not reuse another frame displaying that buffer either.  Please tell
which specific detail of `switch-to-buffer-other-window' you dislike in
the present use case.  Note: It can't be the `other-window' argument,
because in that case we'd have to change the names of the respective
bookmark functions.

martin

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Drew Adams]

>  >> I'm not sure what the problem is here.
>  >> `switch-to-buffer-other-window'
>  >> has a clear purpose - do _not reuse the selected window_ 
>  >> (which is the bookmarks window, IIUC).  OTOH 
>  >> `display-buffer-reuse-frames' non-nil
>  >> should assure that another frame is reused.
>  >
>  > Users should not have to customize a global variable, to 
>  > prevent a new frame from being used in particular places like this.
> 
> I thought you wanted to avoid popping up a new frame.  At 
> least in your first mail you said "pop-to-buffer DTRT:
> it reuses the existing frame".

I do. With pop-up-frames = t, and with a frame alteady showing buffer foo,
(switch-to-buffer-other-window 'foo) opens a *new*, additional frame to show
foo. That's the bug. pop-to-buffer instead simply navigates to the existing
frame, selecting buffer foo there.

>  > As Stefan says repeatedly (paraphrasing), 
>  > switch-to-buffer-other-window is almost always the wrong
>  > thing to do, and should be replaced in most places by
>  > pop-to-buffer.
>  >
>  > Use of switch-to-buffer-other-window is a bug in general, 
>  > typically made by someone who doesn't use non-nil pop-up-frames.
>  >
>  > In this particular context, there is no reason to use
>  > switch-to-buffer-other-frame.
> 
> If you have `display-buffer-reuse-frames' set to nil, `pop-to-buffer'
> will not reuse another frame displaying that buffer either.

Right. This is for the case when it is set to non-nil. For the nil case, I
imagine that there is not much difference between pop-to-buffer and
switch-to-buffer-other-window (but I don't know, as I've always set it to t).

(In fact, I set both display-buffer-reuse-frames and the obsolete (?)
display-buffer-reuse-frame to t, just in case. ;-))

I expect that most people who use non-nil pop-up-frames set
display-buffer-reuse-frames to t (but I don't know that for a fact).

> Please tell which specific detail of `switch-to-buffer-other-window'
> you dislike in the present use case.

Opening a new frame for the buffer, when there is already an existing frame
showing it. In the present use case (and in most use cases), that is uncalled
for.

Context: non-nil pop-up-frames, non-nil display-buffer-reuse-frames.
pop-to-buffer DTRT; switch-to-buffer-other-window does not.

(No, we don't want to change the behavior of switch-to-buffer-other-window; that
command has its uses. It's just that most or many of the existing calls of
switch-to-buffer-other-window should really be calls of pop-to-buffer.)

> Note: It can't be the `other-window' argument,
> because in that case we'd have to change the names of the respective
> bookmark functions.

Sorry, I don't what you're saying, there.

It's pretty simple, really: When I want to go to a bookmark in another
window/frame (which is most of the time I want to go to a bookmark), I don't
want to create a new frame for that destination buffer - I just want to go to
the frame that's already showing it. Imagine hitting the key to go back to that
bookmark several times, off and on over a period of time. You would end up with
lots of frames showing that same buffer. Silly.

Thx.

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[martin rudalics]

 > I do. With pop-up-frames = t, and with a frame alteady showing buffer foo,
 > (switch-to-buffer-other-window 'foo) opens a *new*, additional frame to show
 > foo. That's the bug. pop-to-buffer instead simply navigates to the existing
 > frame, selecting buffer foo there.

The body of `switch-to-buffer-other-window' is deceptively simple

   (let ((pop-up-windows t)
	same-window-buffer-names same-window-regexps)
     (pop-to-buffer buffer-or-name t norecord)))

so let's look into this:

- `pop-up-windows' t means it can pop-up a new window in the selected
   frame.  I suppose we don't care about this.

- `same-window-buffer-names' and `same-window-regexps' make sure the
   selected window is not chosen.  So we don't care about these either.

- The `buffer-or-name' and `norecord' arguments for `pop-to-buffer' are
   the same as for `pop-to-buffer'.  We don't care about them.

- The `other-window' argument set to t is the only one that would differ
   with respect to a plain `pop-to-buffer'.  But we need it in order to
   not reuse the selected window, just as the names of the bookmark
   functions indicate.  So we won't care about these either.

All that's left are variables like `display-buffer-reuse-frames' which
are handled the same way by `display-buffer'.

 >> If you have `display-buffer-reuse-frames' set to nil, `pop-to-buffer'
 >> will not reuse another frame displaying that buffer either.
 >
 > Right. This is for the case when it is set to non-nil. For the nil case, I
 > imagine that there is not much difference between pop-to-buffer and
 > switch-to-buffer-other-window (but I don't know, as I've always set it to t).

So show me where there's a difference for the `t' case.

 > (In fact, I set both display-buffer-reuse-frames and the obsolete (?)
 > display-buffer-reuse-frame to t, just in case. ;-))
 >
 > I expect that most people who use non-nil pop-up-frames set
 > display-buffer-reuse-frames to t (but I don't know that for a fact).

Then why does this not work for `switch-to-buffer-other-window'?

 >> Please tell which specific detail of `switch-to-buffer-other-window'
 >> you dislike in the present use case.
 >
 > Opening a new frame for the buffer, when there is already an existing frame
 > showing it. In the present use case (and in most use cases), that is uncalled
 > for.
 >
 > Context: non-nil pop-up-frames, non-nil display-buffer-reuse-frames.
 > pop-to-buffer DTRT; switch-to-buffer-other-window does not.

It does so here.

 > (No, we don't want to change the behavior of switch-to-buffer-other-window; that
 > command has its uses. It's just that most or many of the existing calls of
 > switch-to-buffer-other-window should really be calls of pop-to-buffer.)
 >
 >> Note: It can't be the `other-window' argument,
 >> because in that case we'd have to change the names of the respective
 >> bookmark functions.
 >
 > Sorry, I don't what you're saying, there.
 >
 > It's pretty simple, really: When I want to go to a bookmark in another
 > window/frame (which is most of the time I want to go to a bookmark), I don't
 > want to create a new frame for that destination buffer - I just want to go to
 > the frame that's already showing it. Imagine hitting the key to go back to that
 > bookmark several times, off and on over a period of time. You would end up with
 > lots of frames showing that same buffer. Silly.

I suppose you redefined some of the involved functions in a non-standard
manner.  Please have a look.  Otherwise, we need someone else to confirm
the behavior you report.  I can't reproduce it.

martin

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Drew Adams]

> I can't reproduce it.

I can't reproduce it now either. ;-)

I don't have the time now to try to get back to where/how it happened. Perhaps
something changed in this regard between Emacs 22 and 23? Dunno.

Sorry for the noise. If I get some more info about this, I'll file a new bug.
Thx.

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Stefan Monnier]

> Users should not have to customize a global variable, to prevent a new frame
> from being used in particular places like this.

I believe you are a bit confused about what's happening.  Sticking to
the fact, your bug report complains that bookmark created a new frame
when it could have reused another one, right?

AFAICT, switch-to-buffer-other-window and pop-to-buffer should behave
identically in this respect, to the extend that you pass a non-nil
`other-window' argument to pop-to-buffer.

So most likely the problem is in pop-to-buffer (which is used by
switch-to-buffer-other-window).

> As Stefan says repeatedly (paraphrasing), switch-to-buffer-other-window is
> almost always the wrong thing to do, and should be replaced in most places by
> pop-to-buffer.

Actually, no: switch-to-buffer-other-window is just a wrapper around
pop-to-buffer, and doesn't bother me at all.
It's switch-to-buffer that's a problem when called from Lisp.


        Stefan

bug#4293: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el

[Drew Adams]

> Actually, no: switch-to-buffer-other-window is just a wrapper around
> pop-to-buffer, and doesn't bother me at all.
> It's switch-to-buffer that's a problem when called from Lisp.

My bad. And I cannot reproduce the problem I saw now.

bug#4293: marked as done (23.1; use pop-to-buffer, not switch...other-window, in bookmark.el)

[Emacs bug Tracking System]

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

Your message dated Sun, 04 Oct 2009 22:01:43 -0400
with message-id <87skdymwe0.fsf@red-bean.com>
and subject line Re: use pop-to-buffer, not switch...other-window, in bookmark.el
has caused the Emacs bug report #4293,
regarding 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el
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 owner@emacsbugs.donarmstrong.com
immediately.)


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

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

From: "Drew Adams" <drew.adams@oracle.com>
To: <bug-gnu-emacs@gnu.org>
Subject: 23.1; use pop-to-buffer, not switch...other-window, in bookmark.el
Date: Sun, 30 Aug 2009 07:52:34 -0700
Message-ID: <E7A6103CCA824B978818327053AA6BEB@us.oracle.com>

In bookmark-jump-other-window and bookmark-bmenu-other-window we call
switch-to-buffer-other-window. We should use pop-to-buffer, instead.
 
With non-nil pop-up-frames, switch-to-buffer-other-window creates a
new frame each time, even if the destination buffer is already showing
in some frame. pop-to-buffer DTRT: it reuses the existing frame.
 

In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'





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

From: Karl Fogel <kfogel@red-bean.com>
To: 4293-close@emacsbugs.donarmstrong.com
Subject: Re: use pop-to-buffer, not switch...other-window, in bookmark.el
Date: Sun, 04 Oct 2009 22:01:43 -0400
Message-ID: <87skdymwe0.fsf@red-bean.com>

tags unreproducible
--

Closing this as "unreproducible".  Or anyway, attempting to close as
unreproducible -- our bug tracker is too unusuable for me to be
confident of the effects of the commands I'm trying to issue.

FWIW, here's a response I typed up before reading the full bug
correspondence.  It's not relevant anymore, but if the bug ever
reappears, it might become relevant to testers:

  This would change the user-visible behavior.  For example, assume
  you're in a frame with one window, displaying buffer X.  In the
  current code, doing 'M-x bookmark-jump-other-window' and entering
  bookmark "foo" at the prompt will open a new window displaying buffer
  Y (assuming 'foo' points to a location in Y).  In other words, the
  frame will be divided into two windows.

  But if we replace "switch-to-buffer-other-window" with "pop-to-buffer"
  in `bookmark-jump-other-window', then the new behavior will be to
  *replace* the current whole-frame window displaying X with a new
  whole-frame window displaying Y.  That is not the desired behavior.