bug#43379: [PATCH] Double-click events can occur without preceding single-click events

[Lars Ingebrigtsen <larsi@gnus.org>]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've just read this thread and skimmed the patches, and they seem to
> make sense to me.  However, when I tried to apply them to test, they
> don't apply cleanly any more (since this was almost a year ago,
> unfortunately).
>
> Do you have updated versions of the patch sets?  If so, I can test and
> get them pushed to Emacs 28.

I've respun the patches below -- some of the code has changed
considerably, so it may not be 100% correct any more.

However, I can't reproduce the original bug report (in either emacs-28
or the current trunk).  Here's the test case:

(let ((double-click-time 500))
  (require 'cl-lib)
  (pop-to-buffer "*Events*")
  (cl-loop
   (let ((event (read-event)))
     (with-current-buffer "*Events*"
       (insert (replace-regexp-in-string
		" .*" ""
		(prin1-to-string event))
	       "\n")
       (goto-char (point-min))))))

I can't get a single instance of a double-mouse-x without a mouse-x
happening before, even with long double-click-times.

Can anybody else reproduce this problem?

diff --git a/doc/emacs/custom.texi b/doc/emacs/custom.texi
index d9d6a68005..92044309ea 100644
--- a/doc/emacs/custom.texi
+++ b/doc/emacs/custom.texi
@@ -2186,8 +2186,12 @@ Mouse Buttons
 
   The symbols for mouse events also indicate the status of the modifier
 keys, with the usual prefixes @samp{C-}, @samp{M-}, @samp{H-},
-@samp{s-}, @samp{A-}, and @samp{S-}.  These always precede @samp{double-}
-or @samp{triple-}, which always precede @samp{drag-} or @samp{down-}.
+@samp{s-}, @samp{A-}, and @samp{S-}.  These always precede
+@samp{double-} or @samp{triple-}, which always precede @samp{drag-} or
+@samp{down-}.  (Two clicks with different modifier keys can never
+produce a double event, no matter how close together the clicks are.
+Otherwise, Emacs could get a @code{double-mouse-1} event without getting
+a @code{mouse-1} event first.)
 
   A frame includes areas that don't show text from the buffer, such as
 the mode line and the scroll bar.  You can tell whether a mouse button
diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index 6ed46fa6a2..d1471e9221 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -1717,9 +1717,12 @@ Repeat Events
 @cindex triple-click events
 @cindex mouse events, repeated
 
-If you press the same mouse button more than once in quick succession
-without moving the mouse, Emacs generates special @dfn{repeat} mouse
-events for the second and subsequent presses.
+Emacs generates special @dfn{repeat} mouse events to represent actions
+like double and triple clicks.  If you press a button twice in quick
+succession without moving the mouse in between, Emacs will designate the
+second event as a repeat.  (However, if you hold down any modifier keys
+during one press and not the other, Emacs will treat the two events as
+unconnected, and the second event will not be a repeat.)
 
 The most common repeat events are @dfn{double-click} events.  Emacs
 generates a double-click event when you click a button twice; the event
@@ -1793,7 +1796,10 @@ Repeat Events
 clicks to make a double-click.
 
 This variable is also the threshold for motion of the mouse to count
-as a drag.
+as a drag.  (But if the mouse moves from one window to another while
+the button is held down, or from one screen position to another, it
+always counts as a drag, no matter the value of
+@code{double-click-fuzz}.)
 @end defopt
 
 @defopt double-click-time
diff --git a/etc/NEWS b/etc/NEWS
index 0057fbdcbf..234059a781 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -138,6 +138,16 @@ LRI).  The new command 'highlight-confusing-reorderings' finds and
 highlights segments of buffer text whose reordering for display is
 suspicious and could be malicious.
 
++++
+** Repeat events are now produced only when the modifier keys are the same.
+Before, when the user pressed the same mouse button repeatedly within
+the bounds specified by 'double-click-fuzz' and 'double-click-time',
+it always produced a 'double-' or 'triple-' event, even if the user
+was holding down modifier keys on one click and not another.  This
+meant that it was possible for Emacs to read a double-click event
+without reading the same kind of single-click event first.  Emacs now
+looks at modifier keys to determine if a mouse event is a repeat.
+
 \f
 
 ** Emacs server and client changes.
diff --git a/src/keyboard.c b/src/keyboard.c
index de9805df32..c30b980ba2 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -351,6 +351,7 @@ #define READABLE_EVENTS_IGNORE_SQUEEZABLES	(1 << 2)
 static Lisp_Object read_char_x_menu_prompt (Lisp_Object,
                                             Lisp_Object, bool *);
 static Lisp_Object read_char_minibuf_menu_prompt (int, Lisp_Object);
+static intmax_t mouse_repeat_count (const struct input_event *);
 static Lisp_Object make_lispy_event (struct input_event *);
 static Lisp_Object make_lispy_movement (struct frame *, Lisp_Object,
                                         enum scroll_bar_part,
@@ -5062,18 +5063,6 @@ #define ISO_FUNCTION_KEY_OFFSET 0xfe00
    the down mouse event.  */
 static Lisp_Object frame_relative_event_pos;
 
-/* Information about the most recent up-going button event:  Which
-   button, what location, and what time.  */
-
-static int last_mouse_button;
-static int last_mouse_x;
-static int last_mouse_y;
-static Time button_down_time;
-
-/* The number of clicks in this multiple-click.  */
-
-static int double_click_count;
-
 /* X and Y are frame-relative coordinates for a click or wheel event.
    Return a Lisp-style event list.  */
 
@@ -5388,6 +5377,139 @@ make_scroll_bar_position (struct input_event *ev, Lisp_Object type)
 		builtin_lisp_symbol (scroll_bar_parts[ev->part]));
 }
 
+/* Whether the next click (or wheel event) should be treated as a
+   single no matter what, even if a matching prior event would make it
+   a repeat.  This gets flipped on when a keystroke or drag event
+   comes in: double clicks can't extend across one of those.  */
+
+static bool interrupt_next_repeat_click;
+
+/* If EVENT is a repeat, return the number of consecutive mouse events
+   so far.  Return 1 otherwise.  In the background, keep a record of
+   where we are in the current sequence of repeats.
+
+   This should only receive a pointer to an event of an applicable
+   kind -- namely, a button-down, button-up, or wheel event.  */
+
+static intmax_t
+mouse_repeat_count (const struct input_event *event)
+{
+  /* Keep persistent information about any chain of repeat events we
+     might be in the middle of.  */
+
+  static ENUM_BF (event_kind) kind;
+  static unsigned code;
+  static unsigned keys;
+  static unsigned wheel_dir;
+
+  static Time timeout_start;
+  static EMACS_INT x, y;
+
+  static intmax_t count;
+
+  /* Analyze the new event that came in.  */
+
+  unsigned new_keys = event->modifiers & CHAR_MODIFIER_MASK;
+  unsigned new_wheel_dir;
+
+  Time interval;
+  EMACS_INT new_x, new_y, offset;
+
+  bool is_wheel = (event->kind == WHEEL_EVENT
+                   || event->kind == HORIZ_WHEEL_EVENT);
+  bool is_button = !is_wheel;
+  bool is_button_up = (is_button && (event->modifiers & up_modifier));
+
+  if (is_wheel)
+    new_wheel_dir = (event->modifiers & (down_modifier | up_modifier));
+  else
+    new_wheel_dir = 0;
+
+  /* We need to update timestamp and position after both repeat and
+     non-repeat events.  Make the relevant comparisons in advance,
+     then do the update immediately.  */
+
+  interval = event->timestamp - timeout_start;
+  if (!is_button_up)
+    timeout_start = event->timestamp;
+
+  new_x = XFIXNUM (event->x);
+  new_y = XFIXNUM (event->y);
+  offset = max (eabs (new_x - x), eabs (new_y - y));
+  x = new_x;
+  y = new_y;
+
+  /* Is this a repeat?  Start checking for conditions that imply
+     otherwise.  */
+
+  if (interrupt_next_repeat_click && !is_button_up)
+    {
+      interrupt_next_repeat_click = false;
+      goto not_a_repeat;
+    }
+
+  if (event->kind != kind || event->code != code || new_keys != keys
+      || new_wheel_dir != wheel_dir)
+    goto not_a_repeat;
+
+  if (is_button_up)
+    /* That's it for button-up events.  At this point, we declare that
+       it's a repeat that should inherit the count of the preceding
+       button-down.  */
+    return count;
+
+  if (FIXNATP (Vdouble_click_time))
+    {
+      if (interval >= XFIXNAT (Vdouble_click_time))
+        goto not_a_repeat;
+    }
+  else if (!EQ (Vdouble_click_time, Qt))
+    goto not_a_repeat;
+
+  {
+    /* On window-system frames, use the value of 'double-click-fuzz'
+       as is.  On other frames, interpret it as a multiple of 1/8
+       characters.  */
+    struct frame *f;
+    intmax_t fuzz;
+
+    if (WINDOWP (event->frame_or_window))
+      f = XFRAME (XWINDOW (event->frame_or_window)->frame);
+    else if (FRAMEP (event->frame_or_window))
+      f = XFRAME (event->frame_or_window);
+    else
+      emacs_abort ();
+
+    if (FRAME_WINDOW_P (f))
+      fuzz = double_click_fuzz;
+    else
+      fuzz = double_click_fuzz / 8;
+
+    if (offset > fuzz)
+      goto not_a_repeat;
+  }
+
+  /* We've ruled out everything that could disqualify it as a repeat,
+     so treat it as one.  */
+  count++;
+  return count;
+
+ not_a_repeat:
+  /* Base a new repeat chain off of this event.  */
+  count = 1;
+  kind = event->kind;
+  code = event->code;
+  keys = new_keys;
+  wheel_dir = new_wheel_dir;
+
+  if (is_button_up)
+    /* A button-up event should cut off a chain when it doesn't match
+       the last event, but it shouldn't start its own chain.  */
+    interrupt_next_repeat_click = true;
+
+  return count;
+}
+
 /* Given a struct input_event, build the lisp event which represents
    it.  If EVENT is 0, build a mouse movement event from the mouse
    movement buffer, which should have a movement event in it.
@@ -5512,7 +5634,7 @@ make_lispy_event (struct input_event *event)
 	if ((event->code) == 040
 	    && event->modifiers & shift_modifier)
 	  c |= shift_modifier;
-	button_down_time = 0;
+	interrupt_next_repeat_click = true;
 	XSETFASTINT (lispy_c, c);
 	return lispy_c;
       }
@@ -5531,7 +5653,7 @@ make_lispy_event (struct input_event *event)
       /* A function key.  The symbol may need to have modifier prefixes
 	 tacked onto it.  */
     case NON_ASCII_KEYSTROKE_EVENT:
-      button_down_time = 0;
+      interrupt_next_repeat_click = true;
 
       for (i = 0; i < ARRAYELTS (lispy_accent_codes); i++)
 	if (event->code == lispy_accent_codes[i])
@@ -5617,10 +5739,10 @@ make_lispy_event (struct input_event *event)
 #endif
       {
 	int button = event->code;
-	bool is_double;
 	Lisp_Object position;
 	Lisp_Object *start_pos_ptr;
 	Lisp_Object start_pos;
+	int repeat_count;
 
 	position = Qnil;
 
@@ -5709,57 +5831,20 @@ make_lispy_event (struct input_event *event)
 	    mouse_syms = larger_vector (mouse_syms, incr, -1);
 	  }
 
+	repeat_count = mouse_repeat_count (event);
+	if (repeat_count == 2)
+	  event->modifiers |= double_modifier;
+	else if (repeat_count >= 3)
+	  event->modifiers |= triple_modifier;
+
 	start_pos_ptr = aref_addr (button_down_location, button);
 	start_pos = *start_pos_ptr;
 	*start_pos_ptr = Qnil;
 
-	{
-	  /* On window-system frames, use the value of
-	     double-click-fuzz as is.  On other frames, interpret it
-	     as a multiple of 1/8 characters.  */
-	  struct frame *f;
-	  intmax_t fuzz;
-
-	  if (WINDOWP (event->frame_or_window))
-	    f = XFRAME (XWINDOW (event->frame_or_window)->frame);
-	  else if (FRAMEP (event->frame_or_window))
-	    f = XFRAME (event->frame_or_window);
-	  else
-	    emacs_abort ();
-
-	  if (FRAME_WINDOW_P (f))
-	    fuzz = double_click_fuzz;
-	  else
-	    fuzz = double_click_fuzz / 8;
-
-	  is_double = (button == last_mouse_button
-		       && (eabs (XFIXNUM (event->x) - last_mouse_x) <= fuzz)
-		       && (eabs (XFIXNUM (event->y) - last_mouse_y) <= fuzz)
-		       && button_down_time != 0
-		       && (EQ (Vdouble_click_time, Qt)
-			   || (FIXNATP (Vdouble_click_time)
-			       && (event->timestamp - button_down_time
-				   < XFIXNAT (Vdouble_click_time)))));
-	}
-
-	last_mouse_button = button;
-	last_mouse_x = XFIXNUM (event->x);
-	last_mouse_y = XFIXNUM (event->y);
-
 	/* If this is a button press, squirrel away the location, so
            we can decide later whether it was a click or a drag.  */
 	if (event->modifiers & down_modifier)
 	  {
-	    if (is_double)
-	      {
-		double_click_count++;
-		event->modifiers |= ((double_click_count > 2)
-				     ? triple_modifier
-				     : double_modifier);
-	      }
-	    else
-	      double_click_count = 1;
-	    button_down_time = event->timestamp;
 	    *start_pos_ptr = Fcopy_alist (position);
 	    frame_relative_event_pos = Fcons (event->x, event->y);
 	    ignore_mouse_drag_p = false;
@@ -5796,6 +5881,8 @@ make_lispy_event (struct input_event *event)
 		       && xdiff < double_click_fuzz
 		       && - double_click_fuzz < ydiff
 		       && ydiff < double_click_fuzz
+		       /* If we jumped windows, it has to be a drag.  */
+		       && EQ (POSN_WINDOW (start_pos), POSN_WINDOW (position))
 		       /* Maybe the mouse has moved a lot, caused scrolling, and
 			  eventually ended up at the same screen position (but
 			  not buffer position) in which case it is a drag, not
@@ -5812,7 +5899,7 @@ make_lispy_event (struct input_event *event)
 				   Fcar (position))))) /* Different window */
 		  {
 		    /* Mouse has moved enough.  */
-		    button_down_time = 0;
+		    interrupt_next_repeat_click = true;
 		    click_or_drag_modifier = drag_modifier;
 		  }
 		else if (((!EQ (Fcar (start_pos), Fcar (position)))
@@ -5853,14 +5940,8 @@ make_lispy_event (struct input_event *event)
 		  }
 	      }
 
-	    /* Don't check is_double; treat this as multiple if the
-	       down-event was multiple.  */
-	    event->modifiers
-	      = ((event->modifiers & ~up_modifier)
-		 | click_or_drag_modifier
-		 | (double_click_count < 2 ? 0
-		    : double_click_count == 2 ? double_modifier
-		    : triple_modifier));
+	    event->modifiers = ((event->modifiers & ~up_modifier)
+				| click_or_drag_modifier);
 	  }
 	else
 	  /* Every mouse event should either have the down_modifier or
@@ -5880,7 +5961,7 @@ make_lispy_event (struct input_event *event)
 	  if (event->modifiers & drag_modifier)
 	    return list3 (head, start_pos, position);
 	  else if (event->modifiers & (double_modifier | triple_modifier))
-	    return list3 (head, position, make_fixnum (double_click_count));
+	    return list3 (head, position, make_fixnum (repeat_count));
 	  else
 	    return list2 (head, position);
 	}
@@ -5891,6 +5972,7 @@ make_lispy_event (struct input_event *event)
       {
 	Lisp_Object position;
 	Lisp_Object head;
+	int repeat_count;
 
 	/* Build the position as appropriate for this mouse click.  */
 	struct frame *f = XFRAME (event->frame_or_window);
@@ -5903,38 +5985,15 @@ make_lispy_event (struct input_event *event)
 	position = make_lispy_position (f, event->x, event->y,
 					event->timestamp);
 
-	/* Set double or triple modifiers to indicate the wheel speed.  */
 	{
-	  /* On window-system frames, use the value of
-	     double-click-fuzz as is.  On other frames, interpret it
-	     as a multiple of 1/8 characters.  */
-	  struct frame *fr;
-	  intmax_t fuzz;
 	  int symbol_num;
-	  bool is_double;
-
-	  if (WINDOWP (event->frame_or_window))
-	    fr = XFRAME (XWINDOW (event->frame_or_window)->frame);
-	  else if (FRAMEP (event->frame_or_window))
-	    fr = XFRAME (event->frame_or_window);
-	  else
-	    emacs_abort ();
-
-	  fuzz = FRAME_WINDOW_P (fr)
-	    ? double_click_fuzz : double_click_fuzz / 8;
 
 	  if (event->modifiers & up_modifier)
-	    {
 	      /* Emit a wheel-up event.  */
-	      event->modifiers &= ~up_modifier;
-	      symbol_num = 0;
-	    }
+	    symbol_num = 0;
 	  else if (event->modifiers & down_modifier)
-	    {
 	      /* Emit a wheel-down event.  */
-	      event->modifiers &= ~down_modifier;
-	      symbol_num = 1;
-	    }
+	    symbol_num = 1;
 	  else
 	    /* Every wheel event should either have the down_modifier or
 	       the up_modifier set.  */
@@ -5943,32 +6002,20 @@ make_lispy_event (struct input_event *event)
           if (event->kind == HORIZ_WHEEL_EVENT)
             symbol_num += 2;
 
-	  is_double = (last_mouse_button == - (1 + symbol_num)
-		       && (eabs (XFIXNUM (event->x) - last_mouse_x) <= fuzz)
-		       && (eabs (XFIXNUM (event->y) - last_mouse_y) <= fuzz)
-		       && button_down_time != 0
-		       && (EQ (Vdouble_click_time, Qt)
-			   || (FIXNATP (Vdouble_click_time)
-			       && (event->timestamp - button_down_time
-				   < XFIXNAT (Vdouble_click_time)))));
-	  if (is_double)
-	    {
-	      double_click_count++;
-	      event->modifiers |= ((double_click_count > 2)
-				   ? triple_modifier
-				   : double_modifier);
-	    }
+	  /* Set double or triple modifiers to indicate the wheel
+	     speed.  */
+	  repeat_count = mouse_repeat_count (event);
+	  if (repeat_count == 2)
+	    event->modifiers |= double_modifier;
+	  else if (repeat_count >= 3)
+	    event->modifiers |= triple_modifier;
 	  else
-	    {
-	      double_click_count = 1;
-	      event->modifiers |= click_modifier;
-	    }
+	    /* Non-repeat wheel events are tagged as clicks.  */
+	    event->modifiers |= click_modifier;
 
-	  button_down_time = event->timestamp;
-	  /* Use a negative value to distinguish wheel from mouse button.  */
-	  last_mouse_button = - (1 + symbol_num);
-	  last_mouse_x = XFIXNUM (event->x);
-	  last_mouse_y = XFIXNUM (event->y);
+	  /* The Lisp side expects to see direction information in
+	     'symbol_num', but not in the modifier bits.  */
+	  event->modifiers &= ~(down_modifier | up_modifier);
 
 	  /* Get the symbol we should use for the wheel event.  */
 	  head = modify_event_symbol (symbol_num,
@@ -5981,10 +6028,10 @@ make_lispy_event (struct input_event *event)
 	}
 
         if (NUMBERP (event->arg))
-          return list4 (head, position, make_fixnum (double_click_count),
+          return list4 (head, position, make_fixnum (repeat_count),
                         event->arg);
 	else if (event->modifiers & (double_modifier | triple_modifier))
-	  return list3 (head, position, make_fixnum (double_click_count));
+	  return list3 (head, position, make_fixnum (repeat_count));
 	else
 	  return list2 (head, position);
       }
diff --git a/src/nsterm.m b/src/nsterm.m
index 1f17a30272..b11fcb5ed5 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -291,6 +291,7 @@ - (NSColor *)colorUsingDefaultColorSpace
 static NSAutoreleasePool *outerpool;
 static struct input_event *emacs_event = NULL;
 static struct input_event *q_event_ptr = NULL;
+static int last_real_wheel_modifier_keys = 0;
 static int n_emacs_events_pending = 0;
 static NSMutableArray *ns_pending_files, *ns_pending_service_names,
   *ns_pending_service_args;
@@ -6559,8 +6560,13 @@ - (void)mouseDown: (NSEvent *)theEvent
           int lines = 0;
           int scrollUp = NO;
 
-          /* FIXME: At the top or bottom of the buffer we should
-           * ignore momentum-phase events.  */
+          /* FIXME: At the top or bottom of the buffer, we should
+           * ignore momentum-phase events that are bound to scrolling
+           * the buffer down or up respectively.  But since this code
+           * doesn't know about bindings and the keymap code doesn't
+           * know about wheel momentum, that doesn't seem to be
+           * possible yet.  */
+
           if (! ns_use_mwheel_momentum
               && [theEvent momentumPhase] != NSEventPhaseNone)
             return;
@@ -6645,6 +6651,21 @@ - (void)mouseDown: (NSEvent *)theEvent
           emacs_event->code = 0;
           emacs_event->modifiers = EV_MODIFIERS (theEvent) |
             (scrollUp ? up_modifier : down_modifier);
+
+          /* If this is a momentum-phase event, ignore the modifier
+           * keys it arrived with.  Inherit the modifier keys from the
+           * last non-momentum event in the sequence.  The user may be
+           * pressing or releasing modifier keys, intending to use
+           * them in the next event, while the wheel events are still
+           * firing.  */
+          if ([theEvent momentumPhase] != NSEventPhaseNone)
+            {
+              emacs_event->modifiers &= ~CHAR_MODIFIER_MASK;
+              emacs_event->modifiers |= last_real_wheel_modifier_keys;
+            }
+          else
+              last_real_wheel_modifier_keys =
+                emacs_event->modifiers & CHAR_MODIFIER_MASK;
 #if MAC_OS_X_VERSION_MIN_REQUIRED < 1070
         }
       else


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no