Subject: Re: bug#15133: 24.3.50; REGRESSION: `after-make-frame-functions' now run with wrong frame selected

From: martin rudalics <[email-masked]>
Date: Tue, 20 Aug 2013 09:05:39 +0200

>> Did `display-buffer' work correctly?
 >
 > Not sure what you mean.  I haven't seen a problem until this build.
 > Hence the report of this being a regression.

So you didn't check?

 >> The problem is that the new frame doesn't yet show the buffer you want
 >> to display when `after-make-frame-functions' is called.
 >
 > I see.  So you are saying that the new frame object is passed to the hook
 > functions, but that new frame has not yet been displayed.  If so, that is
 > presumably the cause of the regression.

No.  I am saying that at the time `after-make-frame-functions' is
called, the new frame does not yet show the buffer you want to display
in it via plain `display-buffer'.

 > What's the point of passing the newly created frame object to hook functions
 > intended to act on it, if that frame has not yet been displayed so they can
 > do so?
 >
 > Perhaps you are allowing for hook functions that do not assume the frame is
 > displayed.  Is that the point of this change?  Should I change the hook
 > function here to, say, (lambda (fr) (raise-frame fr) (fit-frame fr))?  Or
 > perhaps `make-frame-visible' instead of `raise-frame'?
 >
 > I just tried those, and they does not work either.  If I want to apply a
 > function such as `fit-frame' to the new frame, and it is not yet displayed,
 > what do I need to call in the hook function to display it first?

You want to apply `fit-frame' to the buffer you eventually want to
display on the new frame.  Right?

 > The doc string of `make-frame' suggests, BTW, that it should both (a) make
 > a frame object and (b) display it, as I have always thought it did do and
 > should do.  It says: "Return a newly created frame displaying the current
 > buffer."

The _current_ buffer.

 > You will perhaps say that the PARAMETERS argument could include `invisible'
 > or `iconified' or some such that has the effect of creating a frame that
 > is not visible (displayed).  If that is the idea behind this change then
 > I might not have anything against it, provided the frame is in fact
 > displayed when PARAMETERS does not do something to make it invisible etc.
 >
 > IOW, in the use cases I have, an ordinary frame is created displayed, and
 > after that happens I want to fit the frame.  `after-make-frame-functions'
 > has always been the right place to do that, in the past.  Seems like this
 > is being redefined now.
 >
 > Please advise.  This should be simple.  And it's still not clear to me
 > why it should not be as it was before (i.e., since forever).

Because the "since forever" behavior is inconsistent.  Consider the
following forms:

(defun mess (frame)
   (message "selected: %s ... frame: %s ... buffer: %s"
	   (selected-frame) frame
	   (window-buffer (frame-root-window frame))))

(let ((pop-up-frames t))
   (add-hook 'after-make-frame-functions 'mess)
   (display-buffer "*Messages*")
   (remove-hook 'after-make-frame-functions 'mess))

(let ((pop-up-frames t))
   (add-hook 'after-make-frame-functions 'mess)
   (pop-to-buffer "*Messages*")
   (remove-hook 'after-make-frame-functions 'mess))

With emacs -Q evaluate the first and and then try the other two with
some older version and the current trunk.  You should notice that the
FRAME argument of `mess' is always different from the selected frame.
But the buffer FRAME displays is different.  With the older versions the
buffer is *scratch* for plain `display-buffer' and *Messages* for
`pop-to-buffer'.  With current trunk it is *scratch* for both.

So I suppose that since forever your `fit-frame' function works on the
"wrong" buffer when you call plain `display-buffer' and the buffer you
want to display is not current at that time.

What I propose is to use the following as substitute for the old
`display-buffer-pop-up-frame':

(defun display-buffer-pop-up-frame (buffer alist)
   "Display BUFFER in a new frame.
This works by calling `pop-up-frame-function'.  If successful,
return the window used; otherwise return nil.

If ALIST has a non-nil `inhibit-switch-frame' entry, avoid
raising the new frame.

If ALIST has a non-nil `pop-up-frame-parameters' entry, the
corresponding value is an alist of frame parameters to give the
new frame."
   (let* ((params (cdr (assq 'pop-up-frame-parameters alist)))
	 (pop-up-frame-alist (append params pop-up-frame-alist))
	 (fun pop-up-frame-function)
	 frame window)
     (when (and fun
	       (with-current-buffer buffer
		 (setq frame (funcall fun)))
	       (setq window (frame-selected-window frame)))
       (prog1 (window--display-buffer
	      buffer window 'frame alist display-buffer-mark-dedicated)
	(unless (cdr (assq 'inhibit-switch-frame alist))
	  (window--maybe-raise-frame frame))))))

With this the buffer printed by `mess' should be *Messages* for both,
the plain `display-buffer' case and `pop-to-buffer'.

martin