bug#57012: Activating versus raising frames

[Daniel Colascione <dancol@dancol.org>]

On Sat, 2022-08-06 at 09:44 +0800, Po Lu wrote:
> Daniel Colascione <dancol@dancol.org> writes:
> 
> > I noticed that raising an Emacs frame (latest master) with
> > emacsclient
> > does nothing under current Cinnamon. Emacs gains focus, but the
> > focused window isn't raised.
> > 
> > I can reproduce the problem with xdotool's windowraise command,
> > which
> > similarly did nothing. xdotool windowactivate works though, so
> > I'm
> > guessing the EWMH activate code in xterm.c would similarly do the
> > trick. Emacs used to use EWMH activate on frame raise but stopped
> > in
> > 2007 to work around deadlocks in a version of metacity in use at
> > the
> > time. Can we once again activate Emacs frames on raise?
> 
> Wouldn't it make more sense for emacsclient to focus the frame,
> which
> does activate it by default?
> 

Hoo boy. I spent a bit of time digging into the event code. The root
cause of the inability of emacsclient to raise the frame is that
we've been getting X11 event timestamps wrong for some time. In
particular, 1) in GTK builds, we're not updating the X timestamps for
keyboard and mouse input events, and 2) we're not updating the X
timestamp when we get an emacsclient request.  Because of #2, when
we call x-focus-window in select-frame-set-input-focus, the timestamp
we send along with the _NET_ACTIVE_WINDOW request is stale, causing
some window managers (e.g. cinnamon and kwin) to just ignore the
_NET_ACTIVE_WINDOW. But because we use _NET_ACTIVE_WINDOW *and*
XSetInputFocus and the latter works, the overall effect is that the
call to select-frame-set-input-focus in server.el focuses the Emacs
window, but doesn't raise it.

The following patch should fix both problems:

diff --git a/lisp/server.el b/lisp/server.el
index a06f2f952f..cd3a8f80f0 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1721,7 +1721,9 @@ server-switch-buffer
 	      ;; a minibuffer/dedicated-window (if there's no
other).
 	      (error (pop-to-buffer next-buffer)))))))
     (when server-raise-frame
-      (select-frame-set-input-focus (window-frame)))))
+      (let ((frame (window-frame)))
+        (frame-note-oob-interaction frame)
+        (select-frame-set-input-focus frame)))))
 
 (defvar server-stop-automatically nil
   "Internal status variable for `server-stop-automatically'.")
diff --git a/src/frame.c b/src/frame.c
index 25d71e0769..084df8ef21 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -5942,6 +5942,25 @@ DEFUN ("frame--set-was-invisible",
Fframe__set_was_invisible,
 
   return f->was_invisible ? Qt : Qnil;
 }
+
+DEFUN ("frame-note-oob-interaction",
+       Fframe_note_oob_interaction,
+       Sframe_note_oob_interaction, 0, 1, 0,
+       doc: /* Note that the user has interacted with a frame.
+This function is useful when the user interacts with Emacs out-of-
band
+(e.g., via the server) and we want to pretend for purposes of Emacs
+interacting with the window system that the last interaction time
was
+the time of that out-of-band interaction, not the time of the last
+window system input event delivered to that frame.  */)
+  (Lisp_Object frame)
+{
+  struct frame *f = decode_any_frame (frame);
+  if (FRAME_LIVE_P (f) &&
+      FRAME_TERMINAL (f)->note_oob_interaction_hook)
+    FRAME_TERMINAL (f)->note_oob_interaction_hook (f);
+  return Qnil;
+}
+
 \f
/*********************************************************************
**
 			Multimonitor data
@@ -6626,6 +6645,7 @@ focus (where a frame immediately loses focus
when it's left by the mouse
   defsubr (&Sframe_window_state_change);
   defsubr (&Sset_frame_window_state_change);
   defsubr (&Sframe_scale_factor);
+  defsubr (&Sframe_note_oob_interaction);
 
 #ifdef HAVE_WINDOW_SYSTEM
   defsubr (&Sx_get_resource);
diff --git a/src/gtkutil.c b/src/gtkutil.c
index a6bba096a4..b2af5ff5c2 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -6658,6 +6658,17 @@ xg_filter_key (struct frame *frame, XEvent
*xkey)
 }
 #endif
 
+#ifndef HAVE_PGTK
+void
+xg_set_user_timestamp (struct frame *frame, guint32 time)
+{
+  GtkWidget *widget = FRAME_GTK_OUTER_WIDGET (frame);
+  GdkWindow *window = gtk_widget_get_window (widget);
+  eassert (window);
+  gdk_x11_window_set_user_time (window, time);
+}
+#endif
+
 #if GTK_CHECK_VERSION (3, 10, 0)
 static void
 xg_widget_style_updated (GtkWidget *widget, gpointer user_data)
diff --git a/src/gtkutil.h b/src/gtkutil.h
index 190d662831..bca7ea8176 100644
--- a/src/gtkutil.h
+++ b/src/gtkutil.h
@@ -224,6 +224,10 @@ #define XG_ITEM_DATA "emacs_menuitem"
 extern bool xg_filter_key (struct frame *frame, XEvent *xkey);
 #endif
 
+#ifndef HAVE_PGTK
+extern void xg_set_user_timestamp (struct frame *frame, guint32
time);
+#endif
+
 /* Mark all callback data that are Lisp_Objects during GC.  */
 extern void xg_mark_data (void);
 
diff --git a/src/termhooks.h b/src/termhooks.h
index c5f1e286e9..2c204afeca 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -860,6 +860,13 @@ #define EVENT_INIT(event) (memset (&(event), 0,
sizeof (struct input_event)), \
      will be considered as grabbed.  */
   bool (*any_grab_hook) (Display_Info *);
 #endif
+
+  /* Called to note that the user has interacted with a window
system
+     frame outside the window system and that we should update the
+     window system's notion of the user's last interaction time with
+     that frame.  */
+  void (*note_oob_interaction_hook) (struct frame *);
+
 } GCALIGNED_STRUCT;
 
 INLINE bool
diff --git a/src/xterm.c b/src/xterm.c
index 4bbcfb0e59..4d42f30e20 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -6581,12 +6581,6 @@ x_set_frame_alpha (struct frame *f)
   x_stop_ignoring_errors (dpyinfo);
 }
 
-
/*********************************************************************
**
-		    Starting and ending an update
-
*********************************************************************
**/
-
-#if defined HAVE_XSYNC && !defined USE_GTK
-
 /* Wait for an event matching PREDICATE to show up in the event
    queue, or TIMEOUT to elapse.
 
@@ -6640,6 +6634,12 @@ x_if_event (Display *dpy, XEvent
*event_return,
     }
 }
 
+/*******************************************************************
****
+		    Starting and ending an update
+
*********************************************************************
**/
+
+#if defined HAVE_XSYNC && !defined USE_GTK
+
 /* Return the monotonic time corresponding to the high-resolution
    server timestamp TIMESTAMP.  Return 0 if the necessary
information
    is not available.  */
@@ -7521,26 +7521,25 @@ x_draw_fringe_bitmap (struct window *w,
struct glyph_row *row,
    user time.  We don't sanitize timestamps from events sent by the
X
    server itself because some Lisp might have set the user time to a
    ridiculously large value, and this way a more reasonable
timestamp
-   can be obtained upon the next event.  */
+   can be obtained upon the next event.  If EXPLICIT_FRAME is NULL,
+   update the focused frame's timestamp; otherwise, update
+   EXPLICIT_FRAME's. */
 
 static void
-x_display_set_last_user_time (struct x_display_info *dpyinfo, Time
time,
-			      bool send_event)
+x_display_set_last_user_time_1 (struct x_display_info *dpyinfo, Time
time,
+				bool send_event,
+				struct frame *explicit_frame)
 {
-#ifndef USE_GTK
-  struct frame *focus_frame;
+  struct frame *frame;
   Time old_time;
-#if defined HAVE_XSYNC
+#if defined HAVE_XSYNC && !defined USE_GTK
   uint64_t monotonic_time;
 #endif
 
-  focus_frame = dpyinfo->x_focus_frame;
+  frame = explicit_frame ? explicit_frame : dpyinfo->x_focus_frame;
   old_time = dpyinfo->last_user_time;
-#endif
 
-#ifdef ENABLE_CHECKING
   eassert (time <= X_ULONG_MAX);
-#endif
 
   if (!send_event || time > dpyinfo->last_user_time)
     dpyinfo->last_user_time = time;
@@ -7567,23 +7566,35 @@ x_display_set_last_user_time (struct
x_display_info *dpyinfo, Time time,
     }
 #endif
 
-#ifndef USE_GTK
-  /* Don't waste bandwidth if the time hasn't actually changed.  */
-  if (focus_frame && old_time != dpyinfo->last_user_time)
+  /* Don't waste bandwidth if the time hasn't actually changed.
+     Update anyway if we're updating the timestamp for a non-focused
+     frame, since the event loop might not have gotten around to
+     updating that frame's timestamp.  */
+  if (frame && (explicit_frame || old_time != dpyinfo-
>last_user_time))
     {
       time = dpyinfo->last_user_time;
 
-      while (FRAME_PARENT_FRAME (focus_frame))
-	focus_frame = FRAME_PARENT_FRAME (focus_frame);
+      while (FRAME_PARENT_FRAME (frame))
+	frame = FRAME_PARENT_FRAME (frame);
 
-      if (FRAME_X_OUTPUT (focus_frame)->user_time_window != None)
+#if defined USE_GTK
+      xg_set_user_timestamp (frame, time);
+#else
+      if (FRAME_X_OUTPUT (frame)->user_time_window != None)
 	XChangeProperty (dpyinfo->display,
-			 FRAME_X_OUTPUT (focus_frame)-
>user_time_window,
+			 FRAME_X_OUTPUT (frame)->user_time_window,
 			 dpyinfo->Xatom_net_wm_user_time,
 			 XA_CARDINAL, 32, PropModeReplace,
 			 (unsigned char *) &time, 1);
-    }
 #endif
+    }
+}
+
+static void
+x_display_set_last_user_time (struct x_display_info *dpyinfo, Time
time,
+			      bool send_event)
+{
+  x_display_set_last_user_time_1 (dpyinfo, time, send_event, NULL);
 }
 
 /* Not needed on GTK because GTK handles reporting the user time
@@ -25859,9 +25870,11 @@ xembed_request_focus (struct frame *f)
 			 XEMBED_REQUEST_FOCUS, 0, 0, 0);
 }
 
-/* Activate frame with Extended Window Manager Hints */
+/* Activate frame with Extended Window Manager Hints
 
-static void
+Return whether we were successful in doing so.  */
+
+static bool
 x_ewmh_activate_frame (struct frame *f)
 {
   XEvent msg;
@@ -25869,8 +25882,7 @@ x_ewmh_activate_frame (struct frame *f)
 
   dpyinfo = FRAME_DISPLAY_INFO (f);
 
-  if (FRAME_VISIBLE_P (f)
-      && x_wm_supports (f, dpyinfo->Xatom_net_active_window))
+  if (x_wm_supports (f, dpyinfo->Xatom_net_active_window))
     {
       /* See the documentation at
 	
https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html
@@ -25890,7 +25902,9 @@ x_ewmh_activate_frame (struct frame *f)
       XSendEvent (dpyinfo->display, dpyinfo->root_window,
 		  False, (SubstructureRedirectMask
 			  | SubstructureNotifyMask), &msg);
+      return true;
     }
+  return false;
 }
 
 static Lisp_Object
@@ -25928,16 +25942,14 @@ x_focus_frame (struct frame *f, bool
noactivate)
        events.  See XEmbed Protocol Specification at
        https://freedesktop.org/wiki/Specifications/xembed-spec/  */
     xembed_request_focus (f);
-  else
+  else if (noactivate ||
+	   (!FRAME_PARENT_FRAME (f) && !x_ewmh_activate_frame (f)))
     {
       /* Ignore any BadMatch error this request might result in.  */
       x_ignore_errors_for_next_request (dpyinfo);
       XSetInputFocus (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
 		      RevertToParent, CurrentTime);
       x_stop_ignoring_errors (dpyinfo);
-
-      if (!noactivate)
-	x_ewmh_activate_frame (f);
     }
 }
 
@@ -28552,6 +28564,54 @@ x_have_any_grab (struct x_display_info
*dpyinfo)
 }
 #endif
 
+static Bool
+server_timestamp_predicate (Display *display,
+			    XEvent *xevent,
+			    XPointer arg)
+{
+  XID *args = (XID *) arg;
+
+  if (xevent->type == PropertyNotify
+      && xevent->xproperty.window == args[0]
+      && xevent->xproperty.atom == args[1])
+    return True;
+
+  return False;
+}
+
+static bool
+x_get_server_time (struct frame* f, Time* time)
+{
+  struct x_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
+  Atom property_atom = dpyinfo->Xatom_EMACS_SERVER_TIME_PROP;
+  XEvent event;
+
+  XChangeProperty (dpyinfo->display, FRAME_OUTER_WINDOW (f),
+		   property_atom, XA_ATOM, 32,
+		   PropModeReplace, (unsigned char *)
&property_atom, 1);
+
+  if (x_if_event (dpyinfo->display, &event,
server_timestamp_predicate,
+		  (XPointer) &(XID[]) {FRAME_OUTER_WINDOW (f),
property_atom},
+		  dtotimespec (XFLOAT_DATA
(Vx_wait_for_event_timeout))))
+    return false;
+  *time = event.xproperty.time;
+  return true;
+}
+
+static void
+x_note_oob_interaction (struct frame *f)
+{
+  while (FRAME_PARENT_FRAME (f))
+    f = FRAME_PARENT_FRAME (f);
+  if (FRAME_LIVE_P (f)) {
+    Time server_time;
+    if (!x_get_server_time (f, &server_time))
+      error ("Timed out waiting for server timestamp");
+    x_display_set_last_user_time_1 (
+      FRAME_DISPLAY_INFO (f), server_time, false, f);
+  }
+}
+
 /* Create a struct terminal, initialize it with the X11 specific
    functions and make DISPLAY->TERMINAL point to it.  */
 
@@ -28622,6 +28682,7 @@ x_create_terminal (struct x_display_info
*dpyinfo)
 #ifdef HAVE_XINPUT2
   terminal->any_grab_hook = x_have_any_grab;
 #endif
+  terminal->note_oob_interaction_hook = x_note_oob_interaction;
   /* Other hooks are NULL by default.  */
 
   return terminal;