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

[martin rudalics]

>> 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