bug#25521: 26.0.50; After (make-frame '((name . "foo"))) (select-frame-by-name "foo") doesn't see the frame

[Noam Postavsky]

martin rudalics <rudalics@gmx.at> writes:

>> Ah, so we could just do something like this? (untested)
>
> With a slightly amended doc-string, yes.  If someone insists on having
> two frames with the same name on different displays and relying on the
> old behavior to choose the one on the same display, we could add such a
> check to give preference to a frame with the given name on the same
> display.  I doubt that someone relies on this function to throw an error
> when a frame with the given name exists only on another display.

Right, here's an update:

>From 846c5603a02544ef9b25f395c253b3621aea4984 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 1 Sep 2017 09:38:55 -0400
Subject: [PATCH 1/3] Let select-frame-by-name choose any frame when called
 from lisp (Bug#25521)

* lisp/frame.el (select-frame-by-name): Choose from the whole list of
frames in the non-interactive part, if not found on the current
display.
---
 lisp/frame.el | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lisp/frame.el b/lisp/frame.el
index 76c1842455..a710360cdb 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -892,7 +892,8 @@ make-frame-names-alist
 
 (defvar frame-name-history nil)
 (defun select-frame-by-name (name)
-  "Select the frame on the current terminal whose name is NAME and raise it.
+  "Select the frame whose name is NAME and raise it.
+Frames on the current terminal are checked first.
 If there is no frame by that name, signal an error."
   (interactive
    (let* ((frame-names-alist (make-frame-names-alist))
@@ -903,11 +904,14 @@ select-frame-by-name
      (if (= (length input) 0)
         (list default)
        (list input))))
-  (let* ((frame-names-alist (make-frame-names-alist))
-        (frame (cdr (assoc name frame-names-alist))))
-    (if frame
-       (select-frame-set-input-focus frame)
-      (error "There is no frame named `%s'" name))))
+  (select-frame-set-input-focus
+   ;; Prefer frames on the current display.
+   (or (cdr (assoc name (make-frame-names-alist)))
+       (catch 'done
+         (dolist (frame (frame-list))
+           (when (equal (frame-parameter frame 'name) name)
+             (throw 'done frame))))
+       (error "There is no frame named `%s'" name))))
 
 
 ;;;; Background mode.
-- 
2.11.0

> Note that I'm not opposed to a timeout solution.  As far as w32term.c is
> concerned, I think your solution is superior to the present one.  But
> IMHO we should try to orchestrate all these efforts using a timer to
> synchronize Emacs with the window manager or window system in a uniform
> way so that the user knows where and how her time is spent.
>
> For example, if we decided that x_wait_for_event is a good idea, then we
> should implement it on Windows too and use it uniformly when waiting for
> a move frame event or the visibility confirmation.

Huh.  I was not aware of those other functions.  Here's a patch which
makes the timeout in x_wait_for_event configurable, and uses it in
x_make_frame_visible.

>From 9c6730723507505f2c9f46148addd56901cd0bcc Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 30 Aug 2017 23:12:22 -0400
Subject: [PATCH 2/3] Bring back the busy wait after x_make_frame_visible
 (Bug#25521)

But wait specfically for a MapNotify event, and only for a
configurable amount of time.
* src/xterm.c (syms_of_xterm) [x-wait-for-event-timeout]: New
variable.
(x_wait_for_event): Use it instead of hardcoding the wait to 0.1s.
(x_make_frame_visible): Call x_wait_for_event at the end.
* etc/NEWS: Announce x_wait_for_event.
---
 etc/NEWS    |  5 +++++
 src/xterm.c | 37 +++++++++++++++++++++++++++++++------
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1b5ae658f6..adcc879cb2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -600,6 +600,11 @@ The two new variables, 'bidi-paragraph-start-re' and
 'bidi-paragraph-separate-re', allow customization of what exactly are
 paragraphs, for the purposes of bidirectional display.
 
+---
+** New variable 'x-wait-for-event-timeout'.
+This controls how long Emacs will wait for updates to the graphical
+state to take effect (making a frame visible, for example).
+
 
 * Changes in Specialized Modes and Packages in Emacs 26.1
 
diff --git a/src/xterm.c b/src/xterm.c
index 0b321909c8..50d023f4d9 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -11029,17 +11029,19 @@ x_sync_with_move (struct frame *f, int left, int top, 
bool fuzzy)
 void
 x_wait_for_event (struct frame *f, int eventtype)
 {
-  int level = interrupt_input_blocked;
+  if (!FLOATP (Vx_wait_for_event_timeout))
+    return;
 
+  int level = interrupt_input_blocked;
   fd_set fds;
   struct timespec tmo, tmo_at, time_now;
   int fd = ConnectionNumber (FRAME_X_DISPLAY (f));
 
   f->wait_event_type = eventtype;
 
-  /* Set timeout to 0.1 second.  Hopefully not noticeable.
-     Maybe it should be configurable.  */
-  tmo = make_timespec (0, 100 * 1000 * 1000);
+  /* Default timeout is 0.1 second.  Hopefully not noticeable.  */
+  double timeout = XFLOAT_DATA (Vx_wait_for_event_timeout);
+  tmo = make_timespec (0, (long int) (timeout * 1000 * 1000 * 1000));
   tmo_at = timespec_add (current_timespec (), tmo);
 
   while (f->wait_event_type)
@@ -11365,8 +11367,13 @@ xembed_send_message (struct frame *f, Time t, enum 
xembed_message msg,
 
 /* Change of visibility.  */
 
-/* This function sends the request to make the frame visible, but may
-   return before it the frame's visibility is changed.  */
+/* This tries to wait until the frame is really visible, depending on
+   the value of Vx_visible_frame_timeout.
+   However, if the window manager asks the user where to position
+   the frame, this will return before the user finishes doing that.
+   The frame will not actually be visible at that time,
+   but it will become visible later when the window manager
+   finishes with it.  */
 
 void
 x_make_frame_visible (struct frame *f)
@@ -11437,11 +11444,14 @@ x_make_frame_visible (struct frame *f)
      before we do anything else.  We do this loop with input not blocked
      so that incoming events are handled.  */
   {
+    Lisp_Object frame;
     /* This must be before UNBLOCK_INPUT
        since events that arrive in response to the actions above
        will set it when they are handled.  */
     bool previously_visible = f->output_data.x->has_been_visible;
 
+    XSETFRAME (frame, f);
+
     int original_left = f->left_pos;
     int original_top = f->top_pos;
 
@@ -11488,6 +11498,10 @@ x_make_frame_visible (struct frame *f)
 
        unblock_input ();
       }
+
+    /* Try to wait for a MapNotify event (that is what tells us when a
+       frame becomes visible).  */
+    x_wait_for_event (f, MapNotify);
   }
 }
 
@@ -13283,6 +13297,17 @@ syms_of_xterm (void)
 keysyms.  The default is nil, which is the same as `super'.  */);
   Vx_super_keysym = Qnil;
 
+  DEFVAR_LISP ("x-wait-for-event-timeout", Vx_wait_for_event_timeout,
+    doc: /* How long to wait for X events.
+
+Emacs will wait up to this many seconds to receive X events after
+making changes which affect the state of the graphical interface.
+Under some window managers this can take an indefinite amount of time,
+so it is important to limit the wait.
+
+If set to a non-float value, there will be no wait at all.  */);
+  Vx_wait_for_event_timeout = make_float (0.1);
+
   DEFVAR_LISP ("x-keysym-table", Vx_keysym_table,
     doc: /* Hash table of character codes indexed by X keysym codes.  */);
   Vx_keysym_table = make_hash_table (hashtest_eql, 900,
-- 
2.11.0

However, based on this comment in w32_read_socket, I think we can't use
the x_wait_for_event approach for w32.

  /* Check which frames are still visible, if we have enqueued any user
     events or been notified of events that may affect visibility.  We
     do this here because there doesn't seem to be any direct
     notification from Windows that the visibility of a window has
     changed (at least, not in all cases).  */

The final patch uses the same timeout variable, not sure if that's much
of an improvement.

>From e0f8d29834afe72b64a3b6f87490cd4e0e571490 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 25 Sep 2017 21:58:55 -0400
Subject: [PATCH 3/3] Wait for frame visibility with timeout in w32term too

* src/w32term.c (syms_of_w32term) [x-wait-for-event-timeout]: New
variable.
(x_make_frame_visible): Wait for frame to become visible according to
its value.
(input_signal_count): Remove.
---
 src/w32term.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/w32term.c b/src/w32term.c
index a7a510b9ec..59d1b950ae 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -163,10 +163,6 @@ BOOL (WINAPI *pfnSetLayeredWindowAttributes) (HWND, 
COLORREF, BYTE, DWORD);
 /* Keyboard code page - may be changed by language-change events.  */
 int w32_keyboard_codepage;
 
-/* Incremented by w32_read_socket whenever it really tries to read
-   events.  */
-static int volatile input_signal_count;
-
 #ifdef CYGWIN
 int w32_message_fd = -1;
 #endif /* CYGWIN */
@@ -4658,9 +4654,6 @@ w32_read_socket (struct terminal *terminal,
 
   block_input ();
 
-  /* So people can tell when we have read the available input.  */
-  input_signal_count++;
-
   /* Process any incoming thread messages.  */
   drain_message_queue ();
 
@@ -6612,7 +6605,8 @@ w32_frame_raise_lower (struct frame *f, bool raise_flag)
 
 /* Change of visibility.  */
 
-/* This tries to wait until the frame is really visible.
+/* This tries to wait until the frame is really visible, depending on
+   the value of Vx_visible_frame_timeout.
    However, if the window manager asks the user where to position
    the frame, this will return before the user finishes doing that.
    The frame will not actually be visible at that time,
@@ -6671,12 +6665,16 @@ x_make_frame_visible (struct frame *f)
                      : SW_SHOWNORMAL);
     }
 
+  if (!FLOATP (Vx_wait_for_event_timeout))
+      return;
+
   /* Synchronize to ensure Emacs knows the frame is visible
      before we do anything else.  We do this loop with input not blocked
      so that incoming events are handled.  */
   {
     Lisp_Object frame;
-    int count;
+    double timeout = XFLOAT_DATA (Vx_wait_for_event_timeout);
+    double start_time = XFLOAT_DATA (Ffloat_time (Qnil));
 
     /* This must come after we set COUNT.  */
     unblock_input ();
@@ -6686,8 +6684,8 @@ x_make_frame_visible (struct frame *f)
     /* Wait until the frame is visible.  Process X events until a
        MapNotify event has been seen, or until we think we won't get a
        MapNotify at all..  */
-    for (count = input_signal_count + 10;
-        input_signal_count < count && !FRAME_VISIBLE_P (f);)
+    while (timeout > (XFLOAT_DATA (Ffloat_time (Qnil)) - start_time) &&
+           !FRAME_VISIBLE_P (f))
       {
        /* Force processing of queued events.  */
         /* TODO: x_sync equivalent?  */
@@ -7319,6 +7317,17 @@ syms_of_w32term (void)
   DEFSYM (Qrenamed_from, "renamed-from");
   DEFSYM (Qrenamed_to, "renamed-to");
 
+  DEFVAR_LISP ("x-wait-for-event-timeout", Vx_wait_for_event_timeout,
+    doc: /* How long to wait for X events.
+
+Emacs will wait up to this many seconds to receive X events after
+making changes which affect the state of the graphical interface.
+Under some window managers this can take an indefinite amount of time,
+so it is important to limit the wait.
+
+If set to a non-float value, there will be no wait at all.  */);
+  Vx_wait_for_event_timeout = make_float (0.1);
+
   DEFVAR_INT ("w32-num-mouse-buttons",
              w32_num_mouse_buttons,
              doc: /* Number of physical mouse buttons.  */);
-- 
2.11.0