Subject: Re: bug#68796: xterm.c: Convert mouse-4/5/6/7 to wheel-up/down/left/right

From: Stefan Monnier <[email-masked]>
Date: Mon, 29 Jan 2024 22:32:29 -0500

>> +  /* Convert pre-XInput2 wheel events represented as mouse-clicks.  */
>> +#ifdef HAVE_XINPUT2
>> +  if (!dpyinfo->supports_xi2)
>> +#endif
>
> dpyinfo->supports_xi2 is no guarantee that exclusively input extension
> events will be received, but only indicates that the X display supports
> the input extension.  There are several scenarios where it may be true
> while core events are still delivered to windows, such as a combination
> of GTK 3 and GDK_CORE_DEVICE_EVENTS=1's being enabled.

Hmm... so IIUC you're saying that even when `dpyinfo->supports_xi2` is
true, we may receive `mouse-4/5/6/7` events for wheel motions?
Yuck!

> The correct test of event type is whether x_construct_mouse_click is
> being called from the XI_ButtonPress or XI_ButtonRelease label in
> handle_one_xevent.

OK, that sounds fixable.  See additional patch below.

> Incidentally, servers with the input extension always assign buttons 4
> through 7 to wheel movement.

Interesting.

>> +      Lisp_Object base = base_mouse_symbol (result->code);
>> +      int wheel
>> +       /* BEWARE: `mouse-wheel-UP-event' corresponds to
>> +          `wheel-DOWN' events and vice versa!!  */
>> +        = BASE_EQ   (base, find_symbol_value (Qmouse_wheel_up_event))    ? 0
>> +          : BASE_EQ (base, find_symbol_value (Qmouse_wheel_down_event))  ? 1
>> +          : BASE_EQ (base, find_symbol_value (Qmouse_wheel_left_event))  ? 2
>> +          : BASE_EQ (base, find_symbol_value (Qmouse_wheel_right_event)) ? 3
>> +          : -1;
>> +      if (wheel >= 0)
>> +        {
>> +          result->kind = (event->type != ButtonRelease ? NO_EVENT
>> +		          : wheel & 2 ? HORIZ_WHEEL_EVENT : WHEEL_EVENT);
>> +          result->code = 0;         /* Not used.  */
>> +          result->modifiers &= ~(up_modifier || down_modifier);
>> +          result->modifiers |= wheel & 1 ? up_modifier : down_modifier;
>> +        }
>> +    }
>
> Isn't this insufficient to completely supersede mouse events by wheel
> events, considering that such events will still be delivered as mouse
> button events to tab bars?

Hmm... indeed there was a crash here, which I just fixed in the
patch below.  But with that fix, I do see `wheel-up/down` events on the
tab-tab as well.  Am I missing something?

> Lastly, plenty of old code that binds just the button events will be
> broken,

Indeed.  But that should be a disappearing breed because such bindings
never worked under non-X11 GUIs and now they don't work either under X11
builds that use XInput2, which I expect is the vast majority of Emacs≥29
builds in GNU/Linux distributions.

> and the code that keeps track of button-down and up events in
> read_key_sequence and friends will cease applying to X wheel events.

Hmm... the only code I can think of in `read_key_sequence` which cares
about button up/down is the code that drops the button down events and
the one that turns pairs into drag events.  Is that ever useful
currently with the `mouse-4/5/6/7` events?  No one has lamented this
under non-X11 builds so far nor under XInput2, so it seems unlikely to
be a problem.

> This area has given me a lot of grief over the years (as it has given
> you), so I'm reluctant to see any change there, as no matter how well
> the argument in favor of its safety is posed, some unforeseen problem
> invariably emerges afterwards.

There's no doubt that event representation is a delicate/nasty part of
Emacs code, but I think it's really a trade-off between possible
short-term pain and the longer term simplification of unifying all wheel
handling under `wheel-*` events.
Also, I think we *will* have go through that pain sooner or later.

        Stefan
diff --git a/src/keyboard.c b/src/keyboard.c
index 7c1822b3423..a881c3d6117 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -6628,8 +6628,13 @@ make_lispy_event (struct input_event *event)
 
 	if (CONSP (event->arg))
 	  return list5 (head, position, make_fixnum (double_click_count),
-			XCAR (event->arg), Fcons (XCAR (XCDR (event->arg)),
-						  XCAR (XCDR (XCDR (event->arg)))));
+			XCAR (event->arg),
+			/* FIXME: I don't know what I'm doing here.  */
+			(CONSP (XCDR (event->arg))
+			 && CONSP (XCDR (XCDR (event->arg))))
+			? Fcons (XCAR (XCDR (event->arg)),
+			         XCAR (XCDR (XCDR (event->arg))))
+			: Qnil);
         else if (NUMBERP (event->arg))
           return list4 (head, position, make_fixnum (double_click_count),
                         event->arg);
diff --git a/src/xterm.c b/src/xterm.c
index fcbe7a1ec4f..237b705b555 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -14548,7 +14548,7 @@ x_query_pointer (Display *dpy, Window w, Window *root_return,
 static Lisp_Object
 x_construct_mouse_click (struct input_event *result,
                          const XButtonEvent *event,
-                         struct frame *f)
+                         struct frame *f, bool xi2)
 {
   int x = event->x;
   int y = event->y;
@@ -14563,9 +14563,7 @@ x_construct_mouse_click (struct input_event *result,
 			  : down_modifier));
 
   /* Convert pre-XInput2 wheel events represented as mouse-clicks.  */
-#ifdef HAVE_XINPUT2
-  if (!dpyinfo->supports_xi2)
-#endif
+  if (!xi2)
     {
       Lisp_Object base = base_mouse_symbol (result->code);
       int wheel
@@ -21895,13 +21893,14 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 			&& event->xbutton.time > ignore_next_mouse_click_timeout)
 		      {
 			ignore_next_mouse_click_timeout = 0;
-			x_construct_mouse_click (&inev.ie, &event->xbutton, f);
+			x_construct_mouse_click (&inev.ie, &event->xbutton,
+			                         f, false);
 		      }
 		    if (event->type == ButtonRelease)
 		      ignore_next_mouse_click_timeout = 0;
 		  }
 		else
-		  x_construct_mouse_click (&inev.ie, &event->xbutton, f);
+		  x_construct_mouse_click (&inev.ie, &event->xbutton, f, false);
 
 		*finish = X_EVENT_DROP;
 		goto OTHER;
@@ -21971,13 +21970,15 @@ handle_one_xevent (struct x_display_info *dpyinfo,
                           && event->xbutton.time > ignore_next_mouse_click_timeout)
                         {
                           ignore_next_mouse_click_timeout = 0;
-                          x_construct_mouse_click (&inev.ie, &event->xbutton, f);
+                          x_construct_mouse_click (&inev.ie, &event->xbutton,
+                                                   f, false);
                         }
                       if (event->type == ButtonRelease)
                         ignore_next_mouse_click_timeout = 0;
                     }
                   else
-                    x_construct_mouse_click (&inev.ie, &event->xbutton, f);
+                    x_construct_mouse_click (&inev.ie, &event->xbutton,
+                                             f, false);
 
 		  if (!NILP (tab_bar_arg))
 		    inev.ie.arg = tab_bar_arg;
@@ -23754,13 +23755,13 @@ handle_one_xevent (struct x_display_info *dpyinfo,
 				&& xev->time > ignore_next_mouse_click_timeout)
 			      {
 				ignore_next_mouse_click_timeout = 0;
-				x_construct_mouse_click (&inev.ie, &bv, f);
+				x_construct_mouse_click (&inev.ie, &bv, f, true);
 			      }
 			    if (xev->evtype == XI_ButtonRelease)
 			      ignore_next_mouse_click_timeout = 0;
 			  }
 			else
-			  x_construct_mouse_click (&inev.ie, &bv, f);
+			  x_construct_mouse_click (&inev.ie, &bv, f, true);
 
 			if (!NILP (tab_bar_arg))
 			  inev.ie.arg = tab_bar_arg;