Re: master 4b98a79a50: Improve X event timestamp tracking

[Daniel Colascione]

On Sun, 2022-08-07 at 13:53 +0800, Po Lu wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm as far from understanding the fine technical details of X
> > interaction as it gets, but from the POV of an interested bystander,
> > it sounds like the practical difference between you two is whether in
> > the following snippet:
> > 
> >   --- 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)))))
> > 
> > we should call a special function that Daniel suggests to add, or
> > introduce a new optional argument to select-frame-set-input-focus to
> > force raising the frame, is that right?
> 
> Right, but after digging some more I would rather just make
> x-focus-frame activate the frame by default, as long as noactivate is
> non-nil.  As documented.

And I'd rather not explicitly bypass window manager policy. Let's not
get into an arms race.

By the way: if we're keen on making things work as documented,
shouldn't we add a bunch of workarounds for x-raise-frame, which
basically does nothing right now?

> 
> > You also say that the issue is more general than just that of raising
> > a frame when it gets focus, but do we actually know about other
> > situations where that could be an issue?  If we do, then indeed a more
> > general approach is more beneficial, IMO.
> 
> I can't think of any more general issue here.
> 
> Anyway, this patch makes x-focus-frame work as documented.  Daniel,
> please see if it works for you (on and outside GTK) with the original
> recipe in the bug report:

Don't we still have the problem of XSetInputFocus succeeding while
the EWMH activation fails, leaving the whole window stack in a state
confusing for both the human and WM components of the system?

> 
> diff --git a/src/xfns.c b/src/xfns.c
> index 672097c0d8..267e2a824c 100644
> --- a/src/xfns.c
> +++ b/src/xfns.c
> @@ -2578,6 +2578,7 @@ append_wm_protocols (struct x_display_info *dpyinfo,
>  #if !defined HAVE_GTK3 && defined HAVE_XSYNC
>    bool found_wm_sync_request = false;
>  #endif
> +  bool found_wm_take_focus = false;
>    unsigned long bytes_after;
>  
>    block_input ();
> @@ -2601,6 +2602,9 @@ append_wm_protocols (struct x_display_info *dpyinfo,
>                  == dpyinfo->Xatom_net_wm_sync_request)
>           found_wm_sync_request = true;
>  #endif
> +       else if (existing_protocols[nitems]
> +                == dpyinfo->Xatom_wm_take_focus)
> +         found_wm_take_focus = true;
>       }
>      }
>  
> @@ -2609,6 +2613,8 @@ append_wm_protocols (struct x_display_info *dpyinfo,
>  
>    if (!found_wm_ping)
>      protos[num_protos++] = dpyinfo->Xatom_net_wm_ping;
> +  if (!found_wm_take_focus)
> +    protos[num_protos++] = dpyinfo->Xatom_wm_take_focus;
>  #if !defined HAVE_GTK3 && defined HAVE_XSYNC
>    if (!found_wm_sync_request && dpyinfo->xsync_supported_p)
>      protos[num_protos++] = dpyinfo->Xatom_net_wm_sync_request;
> diff --git a/src/xterm.c b/src/xterm.c
> index 97985c8d9e..b5487f7e9d 100644
> --- a/src/xterm.c
> +++ b/src/xterm.c
> @@ -17295,6 +17295,10 @@ handle_one_xevent (struct x_display_info *dpyinfo,
>                    }
>                  /* Not certain about handling scroll bars here */
>  #endif
> +             /* Set the last focus time to the time provided inside
> +                the _WM_TAKE_FOCUS message.  */
> +
> +             dpyinfo->last_focus_time = event->xclient.data.l[1];
>               goto done;
>                }
>  
> @@ -25883,6 +25887,44 @@ xembed_request_focus (struct frame *f)
>                        XEMBED_REQUEST_FOCUS, 0, 0, 0);
>  }
>  
> +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;
> +}
> +
> +/* Get the server time.  The X server is guaranteed to deliver the
> +   PropertyNotify event, so there is no reason to use x_if_event.  */

Even if the window disappears while we're waiting?

> +
> +static Time
> +x_get_server_time (struct frame *f)
> +{
> +  Atom property_atom;
> +  XEvent property_dummy;
> +  struct x_display_info *dpyinfo;
> +
> +  dpyinfo = FRAME_DISPLAY_INFO (f);
> +  property_atom = dpyinfo->Xatom_EMACS_SERVER_TIME_PROP;
> +
> +  XChangeProperty (dpyinfo->display, FRAME_OUTER_WINDOW (f),
> +                property_atom, XA_ATOM, 32,
> +                PropModeReplace, (unsigned char *) &property_atom, 1);
> +
> +  XIfEvent (dpyinfo->display, &property_dummy, server_timestamp_predicate,
> +         (XPointer) &(XID[]) {FRAME_OUTER_WINDOW (f), property_atom});
> +
> +  return property_dummy.xproperty.time;
> +}
> +
>  /* Activate frame with Extended Window Manager Hints */
>  
>  static void
> @@ -25890,11 +25932,11 @@ x_ewmh_activate_frame (struct frame *f)
>  {
>    XEvent msg;
>    struct x_display_info *dpyinfo;
> +  Time time;
>  
>    dpyinfo = FRAME_DISPLAY_INFO (f);
>  
> -  if (FRAME_VISIBLE_P (f)
> -      && x_wm_supports (f, dpyinfo->Xatom_net_active_window))
> +  if (FRAME_VISIBLE_P (f))
>      {
>        /* See the documentation at
>        https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html
> @@ -25904,13 +25946,52 @@ x_ewmh_activate_frame (struct frame *f)
>        msg.xclient.message_type = dpyinfo->Xatom_net_active_window;
>        msg.xclient.format = 32;
>        msg.xclient.data.l[0] = 1;
> -      msg.xclient.data.l[1] = dpyinfo->last_user_time;
> +      msg.xclient.data.l[1] = (dpyinfo->last_user_time > 
> dpyinfo->last_focus_time
> +                            ? dpyinfo->last_user_time
> +                            : dpyinfo->last_focus_time);
>        msg.xclient.data.l[2] = (!dpyinfo->x_focus_frame
>                              ? None
>                              : FRAME_OUTER_WINDOW (dpyinfo->x_focus_frame));
>        msg.xclient.data.l[3] = 0;
>        msg.xclient.data.l[4] = 0;
>  
> +      /* No frame is currently focused on that display, so apply any
> +      bypass for focus stealing prevention that the user has
> +      specified.  */
> +      if (!dpyinfo->x_focus_frame)
> +     {
> +       if (EQ (Vx_allow_focus_stealing, Qimitate_pager))
> +         msg.xclient.data.l[0] = 2;
> +       else if (EQ (Vx_allow_focus_stealing, Qnewer_time))
> +         {
> +           block_input ();
> +           time = x_get_server_time (f);
> +#ifdef USE_GTK
> +           x_set_gtk_user_time (f, time);
> +#endif
> +           /* Temporarily override dpyinfo->x_focus_frame so the
> +              user time property is set on the right window.  */
> +           dpyinfo->x_focus_frame = f;
> +           x_display_set_last_user_time (dpyinfo, time, true);
> +           dpyinfo->x_focus_frame = NULL;
> +           unblock_input ();
> +
> +           msg.xclient.data.l[1] = time;
> +         }
> +       else if (EQ (Vx_allow_focus_stealing, Qraise_and_focus))
> +         {
> +           time = x_get_server_time (f);
> +
> +           x_ignore_errors_for_next_request (dpyinfo);
> +           XSetInputFocus (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f),
> +                           RevertToParent, time);
> +           XRaiseWindow (FRAME_X_DISPLAY (f), FRAME_OUTER_WINDOW (f));
> +           x_stop_ignoring_errors (dpyinfo);
> +
> +           return;
> +         }
> +     }
> +
>        XSendEvent (dpyinfo->display, dpyinfo->root_window,
>                 False, (SubstructureRedirectMask
>                         | SubstructureNotifyMask), &msg);
> @@ -25954,14 +26035,19 @@ x_focus_frame (struct frame *f, bool noactivate)
>      xembed_request_focus (f);
>    else
>      {
> -      /* 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)
> +      if (!noactivate && NILP (Vx_no_window_manager)
> +       && x_wm_supports (f, dpyinfo->Xatom_net_active_window))
> +     /* Calling XSetInputFocus manually can be skipped, since
> +        activating a frame means to make it the input focus.  */
>       x_ewmh_activate_frame (f);
> +      else
> +     {
> +       /* 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);
> +     }
>      }
>  }
>  
> @@ -29068,6 +29154,9 @@ syms_of_xterm (void)
>    Fput (Qsuper, Qmodifier_value, make_fixnum (super_modifier));
>    DEFSYM (QXdndSelection, "XdndSelection");
>    DEFSYM (Qx_selection_alias_alist, "x-selection-alias-alist");
> +  DEFSYM (Qimitate_pager, "imitate-pager");
> +  DEFSYM (Qnewer_time, "newer-time");
> +  DEFSYM (Qraise_and_focus, "raise-and-focus");
>  
>    DEFVAR_LISP ("x-ctrl-keysym", Vx_ctrl_keysym,
>      doc: /* Which keys Emacs uses for the ctrl modifier.
> @@ -29294,4 +29383,23 @@ syms_of_xterm (void)
>  In addition, when this variable is a list, only preserve the
>  selections whose names are contained within.  */);
>    Vx_auto_preserve_selections = list2 (QCLIPBOARD, QPRIMARY);
> +
> +  DEFVAR_LISP ("x-allow-focus-stealing", Vx_allow_focus_stealing,
> +    doc: /* How to bypass window manager focus stealing prevention.


It's hard to imagine any user sitting down and tweaking this
variable. This seems like one of those preferences added not to
account for differences in taste, but to punt a hard technical choice
to users not prepared to make it.

> +
> +Some window managers prevent `x-focus-frame' from activating the given
> +frame when Emacs is in the background.  This variable specifies the
> +strategy used to activate frames when that is the case, and has
> +several valid values (any other value means to not bypass window
> +manager focus stealing prevention):


Again, I disagree philosophically with the thrust of this patch.
Focus stealing prevention isn't some nuisance to work around. If
Emacs properly manages user-interaction timestamps, there's no need
to work around it. We don't need to pretend we're a panel or
something if we just give the window manager the information it needs
to make the right decision.