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

[Noam Postavsky <npostavs@users.sourceforge.net>]

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

>> Hmm, that is actually less effect than I expected.  I recall now that
>> some non-relevant MapNotify events get sent in this case [1].  These
>> make x_wait_for_event (f, MapNotify) return earlier than the previous
>> busy wait.

I should check my assumptions more carefully, the problem was actually
that I didn't set the timeout correctly; the whole seconds part was
being dropped.  Fixed with the following (full patches are attached as
well).

-  tmo = make_timespec (0, (long int) (timeout * 1000 * 1000 * 1000));
+  time_t timeout_seconds = (time_t) timeout;
+  tmo = make_timespec
+    (timeout_seconds, (long int) ((timeout - timeout_seconds)
+                                  * 1000 * 1000 * 1000));

And here are the resulting timings (the last is less than 100 because I
got bored of waiting).

    (let ((x-wait-for-event-timeout nil))
      (benchmark 1 '(make-frame-command)))"Elapsed time: 0.117144s (0.042904s in 1 GCs)"
    (let ((x-wait-for-event-timeout 0.1)) ; default
      (benchmark 1 '(make-frame-command)))"Elapsed time: 0.210824s (0.043483s in 1 GCs)"
    (let ((x-wait-for-event-timeout 100.0))
      (benchmark 1 '(make-frame-command)))"Elapsed time: 38.288529s (0.043459s in 1 GCs)"

martin rudalics <rudalics@gmx.at> writes:

> Shouldn't it work to wait only for VisibilityNotify events for the given
> frame?

Thanks for the nudge.  Waiting for VisibilityNotify works too (after
fixing the timeout bug), by the way, but it's MapNotify which has the
Lisp visibile effect.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2256 bytes --]

From a700d113e4f4b63c677bbc9ea26eaefa3dd38c08 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 1 Sep 2017 09:38:55 -0400
Subject: [PATCH v4 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.
---
 etc/NEWS      |  4 ++++
 lisp/frame.el | 16 ++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1b5ae658f6..4bf6701e20 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1822,6 +1822,10 @@ For details see the section "Mouse Window Auto-selection" in the Elisp
 manual.
 
 ---
+*** 'select-frame-by-name' now may return a frame on another display
+if it does not find a suitable one on the current display.
+
+---
 ** 'tcl-auto-fill-mode' is now declared obsolete.  Its functionality
 can be replicated simply by setting 'comment-auto-fill-only-comments'.
 
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))))
 
 \f
 ;;;; Background mode.
-- 
2.11.0


[-- Attachment #3: patch --]
[-- Type: text/plain, Size: 4678 bytes --]

From 89503e0ca9eb7587c54ad5a1beca9da66447c3e4 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 30 Aug 2017 23:12:22 -0400
Subject: [PATCH v4 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 | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4bf6701e20..50daa62448 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).
+
 \f
 * Changes in Specialized Modes and Packages in Emacs 26.1
 
diff --git a/src/xterm.c b/src/xterm.c
index 0b321909c8..90275763cb 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -11029,17 +11029,22 @@ 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);
+  time_t timeout_seconds = (time_t) timeout;
+  tmo = make_timespec
+    (timeout_seconds, (long int) ((timeout - timeout_seconds)
+                                  * 1000 * 1000 * 1000));
   tmo_at = timespec_add (current_timespec (), tmo);
 
   while (f->wait_event_type)
@@ -11365,8 +11370,13 @@ xembed_send_message (struct frame *f, Time t, enum xembed_message msg,
 \f
 /* 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_wait_for_event_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 +11447,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 +11501,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 +13300,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


[-- Attachment #4: patch --]
[-- Type: text/plain, Size: 3633 bytes --]

From 474925d51bbb0ce1ace5e1062cd0bbe4e9f3cfc2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 25 Sep 2017 21:58:55 -0400
Subject: [PATCH v4 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 d7ec40118f..0a44a8fb22 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 ();
 
@@ -6614,7 +6607,8 @@ w32_frame_raise_lower (struct frame *f, bool raise_flag)
 \f
 /* 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,
@@ -6673,12 +6667,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 ();
@@ -6688,8 +6686,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?  */
@@ -7321,6 +7319,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