bug#37826: Very annoying autoraise client/server behavior with -t option

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

In client/server mode, if I have a focused client open in a X frame
and then focus a terminal and open a file in that tty in a second
emacs client, the X client is automatically raised and focused (more
precisely, I just get an "emacs is ready" notification because of
gnome/mutter focus stealing prevention). I can go back and forth
between the two clients producing the same effect as many times as
desired.

I understand why this is happening. In server-visit-files:

(let* ((minibuffer-auto-raise (or server-raise-frame
  minibuffer-auto-raise))
...
  (set-buffer (find-file-noselect filen))
...

The problem is that if, for example, I open a bash shell in the tty
buffer, I get a message like "Indentation setup for shell type bash"
in the X minibuffer, so the X frame is autoraised. There are a couple
of issues mixed here, I believe:

1. Maybe the scope of minibuffer-auto-raise = t should be restricted
to the revert/write operations so that focus is not stolen because of
any "accidental" little message.

2. The "Indentation setup for shell type bash" message clearly belongs
to the tty frame but somehow is showing in the other frame. This seems
harder to fix but I'm going to investigate it.

Best regards
--
Carlos

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

I think I need some feedback in order to advance. I would like to do
something like:

```
frame = create-frame-func()
with-selected-frame frame:
    buffers = server-visit-files(files)
```

so that visiting messages show in the right frame. But this introduces
circularity, since frame is currently obtained as:

```
buffers = server-visit-files(files)
with-current-buffer car(buffers):
     frame = create-frame-func()
```

with the following rationale:

   ;; Set current buffer so that newly created tty frames
   ;; show the correct buffer initially.

So, short of deeper modifications, I could just let the messages show
in the wrong minibuffer and restrict the scope of auto-rise-minibuffer
to buffer reverting/writing questions:

```
            (cond ((file-exists-p filen)
                   (when (not (verify-visited-file-modtime obuf))
                     (revert-buffer t nil)))
                  (t
                   (when (y-or-n-p
                          (concat "File no longer exists: " filen
                                  ", write buffer to file? "))
                     (write-file filen))))
```

Nevertheless, not only the messages will show in the wrong minibuffer
but the questions will also be asked in the wrong place, although
these questions should happen exceptionally.

I believe one way or another there is some price to pay here: do we
prefer for the frame to be created with the right buffer from the very
beginning, at the expense of ill-placed messages and questions? Or do
we prefer to show this stuff in the new frame minibuffer, while
postponing the setting of its initial buffer until all files are
visited, producing a frequent and unpleasant switching visual artifact
(which AFAICR has been always the case, has this code changed recently
in order to address that?) ?

Of course, the first option is easier to implement since it's closer
to what there already exists. Also, insofar we avoid the autoraise
issue, the problem should be less noticeable than the "initial switch"
effect.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 19 Oct 2019 18:39:31 -0300
> 
> I believe one way or another there is some price to pay here: do we
> prefer for the frame to be created with the right buffer from the very
> beginning, at the expense of ill-placed messages and questions? Or do
> we prefer to show this stuff in the new frame minibuffer, while
> postponing the setting of its initial buffer until all files are
> visited, producing a frequent and unpleasant switching visual artifact
> (which AFAICR has been always the case, has this code changed recently
> in order to address that?) ?

We prefer to avoid both unpleasant effects.  But if there's no way to
solve both, I think we prefer the former, because visiting a file in a
way that causes an echo-area message is relatively rare.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

> We prefer to avoid both unpleasant effects.  But if there's no way to
> solve both, I think we prefer the former, because visiting a file in a
> way that causes an echo-area message is relatively rare.

Here is a patch that narrows the scope of minibuffer-auto-raise to
write/revert actions that require user intervention.

Short of a more selective auto-raise mechanism, I believe this is
preferable to raising and focusing a frame when creating a new one
just because of random uninteresting messages.

Moreover, if the user has explicitly set minibuffer-auto-raise, the
scope won't be narrowed at all. So I think it's safe to reduce its
scope a bit when the user has not even signalled interest by toggling
minibuffer-auto-raise to t.

[-- Attachment #2: 0001-Avoid-autoraising-frame-when-tty-client-is-run-Bug-3.patch --]
[-- Type: text/x-patch, Size: 2204 bytes --]

From 0a80b3c71ac52829bc89036f070f1fb5632ea889 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Mon, 21 Oct 2019 05:12:48 -0300
Subject: [PATCH] Avoid autoraising frame when tty client is run (Bug#37826)

* lisp/server.el (server-visit-files): Narrow scope of minibuffer-auto-rise.
---
 lisp/server.el | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 45fa55ad6b..46c76895e9 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1397,9 +1397,7 @@ server-visit-files
 	;; If there is an existing buffer modified or the file is
 	;; modified, revert it.  If there is an existing buffer with
 	;; deleted file, offer to write it.
-	(let* ((minibuffer-auto-raise (or server-raise-frame
-					  minibuffer-auto-raise))
-	       (filen (car file))
+	(let* ((filen (car file))
 	       (obuf (get-file-buffer filen)))
 	  (add-to-history 'file-name-history filen)
 	  (if (null obuf)
@@ -1410,14 +1408,16 @@ server-visit-files
 	    ;; separately for each file, in sync with post-command hooks,
 	    ;; with the new buffer current:
 	    (run-hooks 'pre-command-hook)
-            (cond ((file-exists-p filen)
-                   (when (not (verify-visited-file-modtime obuf))
-                     (revert-buffer t nil)))
-                  (t
-                   (when (y-or-n-p
-                          (concat "File no longer exists: " filen
-                                  ", write buffer to file? "))
-                     (write-file filen))))
+	    (let ((minibuffer-auto-raise (or server-raise-frame
+					     minibuffer-auto-raise)))
+              (cond ((file-exists-p filen)
+                     (when (not (verify-visited-file-modtime obuf))
+                       (revert-buffer t nil)))
+                    (t
+                     (when (y-or-n-p
+                            (concat "File no longer exists: " filen
+                                    ", write buffer to file? "))
+                       (write-file filen)))))
             (unless server-buffer-clients
               (setq server-existing-buffer t)))
           (server-goto-line-column (cdr file))
-- 
2.20.1

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 05:58:13 -0300
> Cc: 37826@debbugs.gnu.org
> 
> Here is a patch that narrows the scope of minibuffer-auto-raise to
> write/revert actions that require user intervention.

I don't think I agree with such narrowing of the scope, certainly not
as an unconditional default behavior.  There are definitely situations
where an informational message in the echo-area needs to attract the
user's attention.

I also think your proposal would have an un-proportionally large
effect, whereas the original problem is quite minor.  Is it possible
to have a solution that only affects this particular use case?  E.g.,
can we delay the message until the new frame is created?

Thanks.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

>
> I don't think I agree with such narrowing of the scope, certainly not
> as an unconditional default behavior.


Ok, I was expecting that anyway :)

 Is it possible
> to have a solution that only affects this particular use case?
>

> can we delay the message until the new frame is created?

>
The message "Indentation setup for shell type bash"  is like, I don't know,
debug level. If modules are that verbose, for me this means a case by case
approach has no hope of success. And in this particular case I would just
have suppressed the message and not merely delayed it, but then we would be
having a new discussion, and another one, and another one... And how to
know when to delay/ignore and when to show if there is no priority hint?
Again, I would have to proceed on a case by case basis, matching message
patterns or something like that.

Maybe you're aware of this and could spare me some time (and I don't have
access to emacs right now): why the buffer "flashing" is a non-issue for
standalone emacs? I mean, it also has to show something at the very
beginning, while still loading the initial buffer.

Best regards
--
Carlos

[-- Attachment #2: Type: text/html, Size: 1981 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 10:37:34 -0300
> Cc: 37826@debbugs.gnu.org
> 
> > can we delay the message until the new frame is created?
> 
> The message "Indentation setup for shell type bash"  is like, I don't know, debug level. If modules are that
> verbose, for me this means a case by case approach has no hope of success.

Maybe there are very few of commands that issue these messages, in
which case a case by case approach is not that bad.

We have infrastructure for delaying messages, see "Delayed Warnings"
in the ELisp manual.  Using something like this could solve this issue
without inordinately affecting more important functionality.

> And how to know when to delay/ignore and when to show if there is no
> priority hint?

I think this is pretty clear: we want to delay when a shell script is
visited via the client.

> Maybe you're aware of this and could spare me some time (and I don't have access to emacs right now): why
> the buffer "flashing" is a non-issue for standalone emacs? I mean, it also has to show something at the very
> beginning, while still loading the initial buffer.

In that case, we visit the file in an existing frame.  By contrast,
the client needs to create a frame, and if it does that before
visiting the file, it will momentarily show some other buffer in that
frame.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

>
> > And how to know when to delay/ignore and when to show if there is no
> > priority hint?
>
> I think this is pretty clear: we want to delay when a shell script is
> visited via the client.

I didn't mean what common sense would tell us in each case, but how to
codify that in rules that don't quickly become a long list of patterns
always missing some case (and then there are non-builtin packages...).

> In that case, we visit the file in an existing frame.  By contrast,
> the client needs to create a frame, and if it does that before
> visiting the file, it will momentarily show some other buffer in that
> frame.
> [...]
> No, because it will flash an empty buffer, something that Emacs
doesn't do.

I don't get this, in both cases we have a frame that is created and a
file that is visited. The client has the extra possibility of
pre-loading the buffer without selecting it, which you have exploited.
But standalone emacs has to show something before. I'm checking that
right now and it indeed shows a blank screen (plenty of "flashes"
while resizing, hiding bars, etc).

Isn't "blank -> desired buffer" much better than "random visited
buffer -> desired buffer" in terms of flashing? Could we give that a
try?

I certainly don't see the current focus stealing behavior as cosmetic
or minor, although I do understand that my current proposal is
dangerous.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

Also I'm seeing that advicing `message` doesn't affect yes/no
questions, seems to follow a completely different code path. My main
concern regarding delaying messages were related to these kind of
interactions with the user. If that's not a concern, maybe all
non-interactive messages could be easily delayed, after all.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

Well... not true of y/n questions...

(let* ((messages nil)
       (delay (lambda (fun &rest args)
                (setq messages (cons args messages)))))
  (unwind-protect
      (progn
        (advice-add #'message :around delay)
        (message "Raise frame!")
        (yes-or-no-p "Are you a frame?")
        (message "I said raise frame!")
        (y-or-n-p "Are you listening?")
        (message "Weird... doesn't seem to work"))
    (advice-remove #'message delay)
    messages))

=>

(("Weird... doesn't seem to work")
 ("%s%c" "Are you listening? (y or n) " 121) ; <----- :(
 (nil)  ; <---- ?????
 ("I said raise frame!")
 ("Raise frame!"))

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 11:52:57 -0300
> Cc: 37826@debbugs.gnu.org
> 
> Also I'm seeing that advicing `message` doesn't affect yes/no
> questions, seems to follow a completely different code path. My main
> concern regarding delaying messages were related to these kind of
> interactions with the user. If that's not a concern, maybe all
> non-interactive messages could be easily delayed, after all.

AFAIK, questions like that are asked after creating a new frame, so if
you know otherwise, please tell the details.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 11:18:04 -0300
> Cc: 37826@debbugs.gnu.org
> 
> >
> > > And how to know when to delay/ignore and when to show if there is no
> > > priority hint?
> >
> > I think this is pretty clear: we want to delay when a shell script is
> > visited via the client.
> 
> I didn't mean what common sense would tell us in each case, but how to
> codify that in rules that don't quickly become a long list of patterns
> always missing some case (and then there are non-builtin packages...).

Maybe I don't understand what you are asking.  The important part of
my answer is "visited via the client", so I'm not sure what is hard in
that rule.

> > In that case, we visit the file in an existing frame.  By contrast,
> > the client needs to create a frame, and if it does that before
> > visiting the file, it will momentarily show some other buffer in that
> > frame.
> > [...]
> > No, because it will flash an empty buffer, something that Emacs
> doesn't do.
> 
> I don't get this, in both cases we have a frame that is created and a
> file that is visited. The client has the extra possibility of
> pre-loading the buffer without selecting it, which you have exploited.
> But standalone emacs has to show something before.

No, it shows the visited file immediately.

> I'm checking that right now and it indeed shows a blank screen
> (plenty of "flashes" while resizing, hiding bars, etc).

That "empty screen" is really empty, i.e. it's a window created by the
windowing system, and Emacs didn't yet display anything there.  Your
suggestion would show a buffer, with its mode line and other
decorations.  That's what the change you are talking about wanted to
avoid.

> Isn't "blank -> desired buffer" much better than "random visited
> buffer -> desired buffer" in terms of flashing? Could we give that a
> try?

I'd like to try cleaner solutions first, okay?

> I certainly don't see the current focus stealing behavior as cosmetic
> or minor

But it happens only in a small number of major modes, right?  Until
now, you identified just one: shell-script mode.  Are there others?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> > > > > Is it possible to have a solution that only affects this particular use case?

> > > I think this is pretty clear: we want to delay when a shell script is
> > > visited via the client.

> Maybe I don't understand what you are asking.  The important part of
> my answer is "visited via the client", so I'm not sure what is hard in
> that rule.

Ok, ok, it's just that you said "a shell script" and previously had
suggested a case by case approach, so I got the wrong message of
having a special rule for shell scripts (and then other special
rules).

The only concern I have regarding a general "delay messages when
visited via the client" is that I still don't know how to solve the
interactive messages (questions) issue that I described in my last
message.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> The only concern I have regarding a general "delay messages when
> visited via the client" is that I still don't know how to solve the
> interactive messages (questions) issue that I described in my last
> message.

>>  ("%s%c" "Are you listening? (y or n) " 121) ; <----- :(

I got this wrong, this is not the prompt for the question itself but a
message with the actual answer that is reported afterwards! So we're
fine with both yes-or-no-p and y-or-n-p. Are you aware of any other
interactive case that might be problematic? Are all prompt-like
interactions going ultimately through the same code path?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 13:33:36 -0300
> Cc: 37826@debbugs.gnu.org
> 
> Are you aware of any other interactive case that might be
> problematic?

No.

> Are all prompt-like interactions going ultimately through the same
> code path?

They should go through y-or-n-p and its ilk, yes.  But again, I don't
think we should have these in the use cases we are discussing.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 13:21:02 -0300
> Cc: 37826@debbugs.gnu.org
> 
> The only concern I have regarding a general "delay messages when
> visited via the client" is that I still don't know how to solve the
> interactive messages (questions) issue that I described in my last
> message.

We shouldn't have those.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Juanma Barranquero]

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

On Mon, Oct 21, 2019 at 3:58 PM Eli Zaretskii <eliz@gnu.org> wrote:

> We have infrastructure for delaying messages, see "Delayed Warnings"
> in the ELisp manual.

Which, interestingly enough, does not document `delay-warning', the delayed
counterpart of `display-waning'.

[-- Attachment #2: Type: text/html, Size: 415 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 21 Oct 2019 17:56:29 +0200
> Cc: Carlos Pita <carlosjosepita@gmail.com>, 37826@debbugs.gnu.org
> 
> > We have infrastructure for delaying messages, see "Delayed Warnings"
> > in the ELisp manual.
> 
> Which, interestingly enough, does not document `delay-warning', the delayed counterpart of `display-waning'.

Patches are welcome.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Juanma Barranquero]

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

On Mon, Oct 21, 2019 at 6:26 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> Patches are welcome.

Why did I suspect that was coming?


From afc1bfb78e89837dbd2320231d2e13db99db0a1d Mon Sep 17 00:00:00 2001
From: Juanma Barranquero <lekktu@gmail.com>
Date: Mon, 21 Oct 2019 18:59:14 +0200
Subject: [PATCH] * doc/lispref/display.texi: Document `delay-warning'

---
 doc/lispref/display.texi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 82d9f1db61..7ea582d012 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -870,6 +870,12 @@ Delayed Warnings
 @code{delayed-warnings-list} to @code{nil}.
 @end defvar

+@defun delay-warning type message &optional level buffer-name
+This function is the delayed counterpart to @code{display-warning},
+and it is called with the same arguments.  The warning message is
+queued into @code{delayed-warnings-list}.
+@end defun
+
 @node Invisible Text
 @section Invisible Text

-- 
2.23.0.windows.1

[-- Attachment #2: Type: text/html, Size: 1419 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 21 Oct 2019 19:02:42 +0200
> Cc: carlosjosepita@gmail.com, 37826@debbugs.gnu.org
> 
> diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
> index 82d9f1db61..7ea582d012 100644
> --- a/doc/lispref/display.texi
> +++ b/doc/lispref/display.texi
> @@ -870,6 +870,12 @@ Delayed Warnings
>  @code{delayed-warnings-list} to @code{nil}.
>  @end defvar
>  
> +@defun delay-warning type message &optional level buffer-name
> +This function is the delayed counterpart to @code{display-warning},
> +and it is called with the same arguments.  The warning message is
> +queued into @code{delayed-warnings-list}.
> +@end defun
> +
>  @node Invisible Text
>  @section Invisible Text

Thanks.  I think this should go before the description of
delayed-warnings-list, and the text at the beginning of the section
should be modifies to mention this function as another alternative to
queue warnings for delayed display.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

Here is some preliminary work. The patch suppresses messages from
server-visit-files.

Some remarks:

1. Perhaps I'd have preferred to advice only around
find-file-noselect, but this would probably require using a global
variable to pass delayed messages from server-visit-files to
server-execute. Thus I wrapped the entire server-visit-files call and
keep the messages local to server-execute.

2. Also I'd have liked that the mock message function returned the
formatted message (since it's part of the interface) but there are
some corner cases (nil, non-string, empty first parameter) that force
me to replicate much of the actual implementation (Fmessage), which I
dislike.

3. I'm dumping the delayed messages in an environment with `(or frame
(selected-frame))` selected frame. The last `((not (null buffers))`
guard in the case statement that comes after my change suggests that
indeed `frame` might be nil, so I'm being careful and fallbacking to
the selected frame in that case. Now, there might be no frame at all,
but then I fail to see what else can be done.

I'm rather open to change any of the above points.


diff --git a/lisp/server.el b/lisp/server.el
index 45fa55ad6b..5943a9ddca 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1304,7 +1304,14 @@ server-execute
   ;; including code that needs to wait.
   (with-local-quit
     (condition-case err
-        (let* ((buffers (server-visit-files files proc nowait))
+        (let* (;; Delay messages to avoid auto raising frame (Bug#37826).
+               (messages nil)
+               (delay (lambda (fun &rest args) (push args messages)))
+               (buffers (unwind-protect
+                            (progn
+                              (advice-add #'message :around delay)
+                              (server-visit-files files proc nowait))
+                          (advice-remove #'message delay)))
                ;; If we were told only to open a new client, obey
                ;; `initial-buffer-choice' if it specifies a file
                ;; or a function.
@@ -1325,6 +1332,10 @@ server-execute
                           ;; Switch to initial buffer in case the
frame was reused.
                           (when initial-buffer
                             (switch-to-buffer initial-buffer 'norecord))))))
+          ;; Show all delayed messages in the new frame (if any).
+          (with-selected-frame (or frame (selected-frame))
+            (dolist (args (nreverse messages))
+              (apply #'message args)))

           (mapc #'funcall (nreverse commands))

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

> 1. Perhaps I'd have preferred to advice only around
> find-file-noselect, but this would probably require using a global
> variable to pass delayed messages from server-visit-files to
> server-execute. Thus I wrapped the entire server-visit-files call and
> keep the messages local to server-execute.

Regarding this point I've changed my mind after more carefully
inspecting server-visit-files. There are many hooks involved that
might run arbitrary code and therefore show messages. We're pretty
confident that any code requiring interaction will work anyway and all
delayed messages are going to be shown afterwards, so even
disregarding the global variable argument above, I still prefer the
version wrapping the entire call to server-visit-files to a more
selective one.

> 2. Also I'd have liked that the mock message function returned the
> formatted message (since it's part of the interface) but there are
> some corner cases (nil, non-string, empty first parameter) that force
> me to replicate much of the actual implementation (Fmessage), which I
> dislike.

I've addressed this point in the less sketchy patch I'm attaching, it
was simpler than I'd though, I believe I'm covering all input cases.

If you prefer to do minor reviews online, this is the commit in my
fork of your mirror
https://github.com/memeplex/emacs/commit/9d53e50848d9d8be758a21d0b5e078f82af25754.
I could create a PR also, just for the sake of reviewing, but it seems
like individual commits are also commentable (until my next forced
push deletes all comments, at least :).

Btw, here is another noisy case: when visiting a new file you get a
"(New file)" message.

[-- Attachment #2: 0001-Avoid-auto-raising-minibufer-before-new-frame-is-cre.patch --]
[-- Type: text/x-patch, Size: 1998 bytes --]

From 9d53e50848d9d8be758a21d0b5e078f82af25754 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Mon, 21 Oct 2019 17:04:38 -0300
Subject: [PATCH] Avoid auto-raising minibufer before new frame is created
 (Bug#37826)

* lisp/server.el (server-execute): Advice `message' to delay output
until the new frame is created.
---
 lisp/server.el | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/lisp/server.el b/lisp/server.el
index 45fa55ad6b..6fab0ef747 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1304,7 +1304,16 @@ server-execute
   ;; including code that needs to wait.
   (with-local-quit
     (condition-case err
-        (let* ((buffers (server-visit-files files proc nowait))
+        (let* (;; Delay messages to avoid auto raising frame (Bug#37826).
+               (messages nil)
+               (delay (lambda (_ fmt &rest args)
+                        (let ((msg (and fmt (apply #'format-message fmt args))))
+                          (car (push msg messages)))))
+               (buffers (unwind-protect
+                            (progn
+                              (advice-add #'message :around delay)
+                              (server-visit-files files proc nowait))
+                          (advice-remove #'message delay)))
                ;; If we were told only to open a new client, obey
                ;; `initial-buffer-choice' if it specifies a file
                ;; or a function.
@@ -1325,6 +1334,10 @@ server-execute
                           ;; Switch to initial buffer in case the frame was reused.
                           (when initial-buffer
                             (switch-to-buffer initial-buffer 'norecord))))))
+          ;; Show all delayed messages in the new frame (if any).
+          (with-selected-frame (or frame (selected-frame))
+            (dolist (msg (nreverse messages))
+              (message msg)))
 
           (mapc #'funcall (nreverse commands))
 
-- 
2.20.1

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 17:14:32 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> I've addressed this point in the less sketchy patch I'm attaching, it
> was simpler than I'd though, I believe I'm covering all input cases.

Thanks.

We generally prefer not to use advice-add etc. in our own code.  What
I had in mind was to modify the places where such messages originate,
and make them use delay-message under the right circumstances.  If
there's some mechanism to do this without changing each place, I'm
okay with that, but using advice is not one of them.

> Btw, here is another noisy case: when visiting a new file you get a
> "(New file)" message.

FWIW, I cannot reproduce this on my system.  In fact, I cannot even
reproduce your original reported issue with visiting a
shell-scrip-mode file: I get a new frame with the message, and no old
frame is raised.  Are you using something other than "emacsclient -c"?
If not, I guess this is specific to your window-manager?  Or what am I
missing?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

>
>
>
> We generally prefer not to use advice-add etc. in our own code.  What
> I had in mind was to modify the places where such messages originate,
> and make them use delay-message under the right circumstances.  If
> there's some mechanism to do this without changing each place, I'm
> okay with that, but using advice is not one of them.
>

Besides monkey-patching the message function in an even dirtier way, I
don't think that would be possible. I don't have the time nor the energy to
review every possible module that is messaging during startup. I could add
an option for message to redirect its messages to some other place, but I
don't see any advantage in doing so.

>
FWIW, I cannot reproduce this on my system.  In fact, I cannot even
> reproduce your original reported issue with visiting a
> shell-scrip-mode file: I get a new frame with the message, and no old
> frame is raised.  Are you using something other than "emacsclient -c"?
>

emacsclient -c

Then opening my .bashrc with

emacsclient -t

Raises the first frame.

The daemon was launched with -Q.

[-- Attachment #2: Type: text/html, Size: 1954 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 26 Oct 2019 12:53:30 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> emacsclient -c
> 
> Then opening my .bashrc with
> 
> emacsclient -t
> 
> Raises the first frame.
> 
> The daemon was launched with -Q.

So if you customize server-raise-frame to nil, the problem goes away
in this scenario?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

Only if minibuffer-auto-rise is also nil:

(let* ((minibuffer-auto-raise (or server-raise-frame minibuffer-auto-raise))

Anyway, I want the frame to be raised when it's switched to, just not
another frame and specially not because of barely relevant messages.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 26 Oct 2019 17:27:02 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> Only if minibuffer-auto-rise is also nil:
> 
> (let* ((minibuffer-auto-raise (or server-raise-frame minibuffer-auto-raise))

Yes, but minibuffer-auto-raise is nil by default.

> Anyway, I want the frame to be raised when it's switched to, just not
> another frame and specially not because of barely relevant messages.

I understand.

So here's an idea.  Does the following change work for you in your
original scenario?

diff --git a/lisp/server.el b/lisp/server.el
index 45fa55a..b23fdc4 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1405,7 +1405,9 @@ server-visit-files
 	  (if (null obuf)
 	      (progn
 		(run-hooks 'pre-command-hook)
-		(set-buffer (find-file-noselect filen)))
+		(set-buffer
+                 (let (minibuffer-auto-raise)
+                   (find-file-noselect filen))))
             (set-buffer obuf)
 	    ;; separately for each file, in sync with post-command hooks,
 	    ;; with the new buffer current:

If this works, then can you test this in several situations different
from yours, including:

  . invoking emacsclient without -t when there's a GUI client frame
    that is iconified
  . an existing client frame is visible (not iconified)
  . visiting a file that is already visited

and maybe others, and see if the message displayed by
shell-script-mode appears as expected, and causes the right frame to
be raised (if needed)?  (It might be that in some of these situations
we need to avoid binding minibuffer-auto-raise to nil, in which case
server-visit-files will need some additional logic or an additional
argument to discern between them.)

Thanks.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> be raised (if needed)?  (It might be that in some of these situations
> we need to avoid binding minibuffer-auto-raise to nil, in which case
> server-visit-files will need some additional logic or an additional
> argument to discern between them.)
>

Hi Eli, the frame is not raised for a -t shell script nor for any
other scenario you listed (considering a very limited set of examples,
at least). This change is similar to my first patch except that mine
also covered the handful of hooks surrounding that part. But even if
yours is more selective it still could be letting an error pass
silently, which was the reason to "record" the output messages and
showing them in the right frame. Thank you.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 27 Oct 2019 02:45:31 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> Hi Eli, the frame is not raised for a -t shell script nor for any
> other scenario you listed (considering a very limited set of examples,
> at least). This change is similar to my first patch except that mine
> also covered the handful of hooks surrounding that part.

Most importantly, it affected the question we ask there if the file is
gone.

> But even if yours is more selective it still could be letting an
> error pass silently

You mean, errors inside find-file-noselect?  Maybe I'm missing
something, but I'd expect such an error to be displayed as usual.  Can
you simulate an error there and see what happens?

Also, an important use case is when the file visited by the client
with -t has an auto-save file.  Emacs should display a message in that
case; can you see if it gets displayed, so the user could see it?
(It's possible that the message is overwritten by "When done with a
buffer, type ..." which the server always displays, but it's important
that the user could see the message about auto-save file for a second
or so.)

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

>
> ). This change is similar to my first patch except that mine
> > also covered the handful of hooks surrounding that part.
>
> Most importantly, it affected the question we ask there if the file is
> gone.
>

I don't think so, my first patch set the

(let* ((minibuffer-auto-raise (or server-raise-frame minibuffer-auto-raise))

environment specifically for those questions, instead of also affecting
everything else. Now you keep the "raisy" environment for everything else,
but disable it specifically for find-file-noselect. My approach was more
risky in the sense that it could have masked more situations requiring
intervention than yours, but the risk is still there, it's just a different
type I vs type II error trade-off.


> You mean, errors inside find-file-noselect?  Maybe I'm missing
> something, but I'd expect such an error to be displayed as usual.  Can
> you simulate an error there and see what happens?
>

I probably couldn't find a situation requiring intervention for the random
examples I would pick now, I admit it would be a rare case, but there are
tons of minor and major modes out there and some of them might ask some
questions at startup. It comes to my mind that pdf-tools sometimes wants to
recompile it's C part after an upgrade, but AFAICR this is done when the
server starts and not when the file is visited, so the question is lost in
the stdout of the daemon, which is an instance of a closely related issue.

>
>

[-- Attachment #2: Type: text/html, Size: 2238 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 27 Oct 2019 12:04:37 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
>  You mean, errors inside find-file-noselect?  Maybe I'm missing
>  something, but I'd expect such an error to be displayed as usual.  Can
>  you simulate an error there and see what happens?
> 
> I probably couldn't find a situation requiring intervention for the random examples I would pick now, I admit it
> would be a rare case, but there are tons of minor and major modes out there and some of them might ask
> some questions at startup. It comes to my mind that pdf-tools sometimes wants to recompile it's C part after
> an upgrade, but AFAICR this is done when the server starts and not when the file is visited, so the question is
> lost in the stdout of the daemon, which is an instance of a closely related issue.

If some mode asks a question, we have a problem already.

I wasn't talking about questions, I was talking about signaling an
error.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> I wasn't talking about questions, I was talking about signaling an
> error.

Ok,  by "requiring intervention" I was referring to both situations,
errors and questions. Regarding errors, nothing special happens, I've
added an error signal at the beginning of sh-mode, unsurprisingly the
error is just buried in the messages buffer and only shown in the
minibuffer of a frame that might be well hidden behind other app
windows and is never risen because of the error.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 27 Oct 2019 13:31:11 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> > I wasn't talking about questions, I was talking about signaling an
> > error.
> 
> Ok,  by "requiring intervention" I was referring to both situations,
> errors and questions. Regarding errors, nothing special happens, I've
> added an error signal at the beginning of sh-mode, unsurprisingly the
> error is just buried in the messages buffer and only shown in the
> minibuffer of a frame that might be well hidden behind other app
> windows and is never risen because of the error.

I'm not sure I follow: if sh-mode signals an error, there should be no
other messages after that, because an error throws to top-level.  What
am I missing?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> I'm not sure I follow: if sh-mode signals an error, there should be no
> other messages after that, because an error throws to top-level.  What
> am I missing?

When done with a buffer, type C-x #  <------ this

Anyway, even if the error were the last message, it would still be
printed in the minibuffer of some potentially invisible frame,
wouldn't it?

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 27 Oct 2019 14:02:27 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> > I'm not sure I follow: if sh-mode signals an error, there should be no
> > other messages after that, because an error throws to top-level.  What
> > am I missing?
> 
> When done with a buffer, type C-x #  <------ this

OK.

> Anyway, even if the error were the last message, it would still be
> printed in the minibuffer of some potentially invisible frame,
> wouldn't it?

Does it mean that my suggestion is unworkable?  I expected the frame
with the error message to be auto-raised, but you seem to say that it
isn't?

We want a solution that avoids raising the wrong frame for some
unimportant messages, but still does raise some frame for displaying
important messages, such as errors.  If that doesn't happen in some
scenario, then we cannot use this idea.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

> We want a solution that avoids raising the wrong frame for some
> unimportant messages, but still does raise some frame for displaying
> important messages, such as errors.  If that doesn't happen in some
> scenario, then we cannot use this idea.

Temporarily redirecting the output of message was a solution that
fitted that description:

* Questions are still asked.
* Errors still raise some frame with a message.

And then, at the end, you get a dump of all temporarily silenced
messages in the right frame.

You dislike the instrumenting/monkey-patching approach, be it by
advicing or by directly accessing the function slot of the symbol. I'm
not sure why, message is a function with a simple and well defined
interface that I'm honouring except for its side effect, and
everything is carefully done inside an unwind-protect clause. But, in
any case, we could add some global flag or something that message
could check in order to change its output destination. That wouldn't
be an instrumentation. But I find that considerably more complex and
it affects the overall behavior of message instead of just mocking it
for a while.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 27 Oct 2019 14:19:51 -0300
> Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> 
> You dislike the instrumenting/monkey-patching approach, be it by
> advicing or by directly accessing the function slot of the symbol. I'm
> not sure why

It's in the ELisp manual: Emacs's own code should avoid advices.  The
reason is it complicates code reading and debugging, and it also looks
like we are not familiar with our own code, so we piggy-back it
instead of changing it.

> we could add some global flag or something that message could check
> in order to change its output destination. That wouldn't be an
> instrumentation.

Some infrastructure that would allow doing that would be nice.  Maybe
we should have delayed-message, like we have delayed-warning.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> Date: Sun, 27 Oct 2019 19:50:30 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: lekktu@gmail.com, 37826@debbugs.gnu.org
> 
> > From: Carlos Pita <carlosjosepita@gmail.com>
> > Date: Sun, 27 Oct 2019 14:19:51 -0300
> > Cc: Juanma Barranquero <lekktu@gmail.com>, 37826@debbugs.gnu.org
> > 
> > You dislike the instrumenting/monkey-patching approach, be it by
> > advicing or by directly accessing the function slot of the symbol. I'm
> > not sure why
> 
> It's in the ELisp manual: Emacs's own code should avoid advices.  The
> reason is it complicates code reading and debugging, and it also looks
> like we are not familiar with our own code, so we piggy-back it
> instead of changing it.
> 
> > we could add some global flag or something that message could check
> > in order to change its output destination. That wouldn't be an
> > instrumentation.
> 
> Some infrastructure that would allow doing that would be nice.  Maybe
> we should have delayed-message, like we have delayed-warning.

I hope the problem is solved by reverting the changes which caused it.
Please try the latest master.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > we could add some global flag or something that message could check
>> > in order to change its output destination. That wouldn't be an
>> > instrumentation.
>> 
>> Some infrastructure that would allow doing that would be nice.  Maybe
>> we should have delayed-message, like we have delayed-warning.
>
> I hope the problem is solved by reverting the changes which caused it.
> Please try the latest master.

This was in November last year.

I have not read the complete thread (it is very long), so I'm not sure
whether this fixes the original problem.  Was there anything further to
be done here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: carlosjosepita@gmail.com,  lekktu@gmail.com,  37826@debbugs.gnu.org
> Date: Sun, 09 Aug 2020 18:12:20 +0200
> 
> > I hope the problem is solved by reverting the changes which caused it.
> > Please try the latest master.
> 
> This was in November last year.
> 
> I have not read the complete thread (it is very long), so I'm not sure
> whether this fixes the original problem.  Was there anything further to
> be done here?

I think the problem was solved by reverting the offending commit.  So
I believe we can close this bug.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think the problem was solved by reverting the offending commit.  So
> I believe we can close this bug.

OK; done.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#37826: Very annoying autoraise client/server behavior with -t option

[Juanma Barranquero]

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

On Mon, Oct 21, 2019 at 7:14 PM Eli Zaretskii <eliz@gnu.org> wrote:

> I think this should go before the description of
> delayed-warnings-list, and the text at the beginning of the section
> should be modifies to mention this function as another alternative to
> queue warnings for delayed display.

I did so, just moving up the description of delay-warning, and changing the
beginning of the section to suggest using it, instead of directly
`delayed-warnings-list'. That way, the low level details follow the
function, for those interested in them.


From c00a590b8847a444e198b7f69b0faff69262b7ab Mon Sep 17 00:00:00 2001
From: Juanma Barranquero <lekktu@gmail.com>
Date: Tue, 22 Oct 2019 16:36:17 +0200
Subject: [PATCH] * doc/lispref/display.texi: Document `delay-warning'

---
 doc/lispref/display.texi | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 82d9f1db61..8ba0327fbb 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -828,7 +828,13 @@ Delayed Warnings

 Sometimes, you may wish to avoid showing a warning while a command is
 running, and only show it only after the end of the command.  You can
-use the variable @code{delayed-warnings-list} for this.
+use the function @code{delay-warning} for this.
+
+@defun delay-warning type message &optional level buffer-name
+This function is the delayed counterpart to @code{display-warning}
+(@pxref{Warning Basics}), and it is called with the same arguments.
+The warning message is queued into @code{delayed-warnings-list}.
+@end defun

 @defvar delayed-warnings-list
 The value of this variable is a list of warnings to be displayed after
@@ -840,8 +846,8 @@ Delayed Warnings

 @noindent
 with the same form, and the same meanings, as the argument list of
-@code{display-warning} (@pxref{Warning Basics}).  Immediately after
-running @code{post-command-hook} (@pxref{Command Overview}), the Emacs
+@code{display-warning}.  Immediately after running
+@code{post-command-hook} (@pxref{Command Overview}), the Emacs
 command loop displays all the warnings specified by this variable,
 then resets it to @code{nil}.
 @end defvar
-- 
2.23.0.windows.1

[-- Attachment #2: Type: text/html, Size: 2605 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Carlos Pita]

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

>
>
> Maybe you're aware of this and could spare me some time (and I don't have
> access to emacs right now): why the buffer "flashing" is a non-issue for
> standalone emacs? I mean, it also has to show something at the very
> beginning, while still loading the initial buffer.
>

Would reverting your create-after-visiting fix and merely creating the new
frame with an initial empty tmp buffer would fix the "flash", at least to
the level of standalone emacs?

[-- Attachment #2: Type: text/html, Size: 982 bytes --]

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 21 Oct 2019 10:55:11 -0300
> Cc: 37826@debbugs.gnu.org
> 
> Would reverting your create-after-visiting fix and merely creating the new frame with an initial empty tmp
> buffer would fix the "flash", at least to the level of standalone emacs?

No, because it will flash an empty buffer, something that Emacs
doesn't do.  When you visit a file in Emacs, you see the current
buffer, and right afterwards the buffer visiting the new file; there's
no empty buffer flashing in-between.

bug#37826: Very annoying autoraise client/server behavior with -t option

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 19 Oct 2019 17:46:26 -0300
> 
> (let* ((minibuffer-auto-raise (or server-raise-frame
>   minibuffer-auto-raise))
> ...
>   (set-buffer (find-file-noselect filen))
> ...
> 
> The problem is that if, for example, I open a bash shell in the tty
> buffer, I get a message like "Indentation setup for shell type bash"
> in the X minibuffer, so the X frame is autoraised.  There are a couple
> of issues mixed here, I believe:
> 
> 1. Maybe the scope of minibuffer-auto-raise = t should be restricted
> to the revert/write operations so that focus is not stolen because of
> any "accidental" little message.
> 
> 2. The "Indentation setup for shell type bash" message clearly belongs
> to the tty frame but somehow is showing in the other frame. This seems
> harder to fix but I'm going to investigate it.

Emacs always shows echo-area messages in the selected frame.  What you
see is the result of visiting the new file in the existing frame, and
only after that creating the new frame.  This order is the result of
fixing bug#24218, which solved another unpleasant and annoying aspect
of visiting a file in a client frame.  See commit 49fc040.

The question is: can we somehow prevent your annoyance without
re-introducing that other one.

Thanks.