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

[Lars Ingebrigtsen]

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.
+
 
 
 ** 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