bug#66655: 29.1; Clicking buttons sometimes doesn't work

[Stefan Monnier]

>> > But then we are back at the problem which the buffer-position check
>> > tries to address:
>> >
>> >                        /* 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
>> >                           a click.  */
>> >
>> > IOW, just testing the screen coordinates is not enough.
>> 
>> In my "in short is approximately" I used `mouse_has_moved` but that
>> was an oversimplification: in the new code `mouse_has_moved` doesn't
>> revert to "false" when the mouse returns to the original position,
>> contrary to what happen in the current code.
>
> How exactly does that happen?

Because as soon as `note_mouse_highlight` receives information that the
mouse is outside of the fuzz, the var is set to `false` [ And it's only
set to true when we get the mouse-down event ].

>> The comment above talks about buffer positions (i.e. the Fcar+Fcdr
>> part of the positions), whereas this `EQ` tests the windows, and the
>> only relevant comment I see is
>> 
>>     /* Different window */
>> 
>> which reminds the reader that it's comparing windows but doesn't say why.
>> Did I miss something?
>
> Yes: we can be at the same buffer position, but a different window.

Right, but what's the problem with that?

IIUC the current code can generate a drag between windows if the mouse
has moved to a new screen position, but we never generate a drag between
windows if the mouse ends at the same screen position as it started.

I see now that this was done (in commit
6e2d3bce087d30a535b1f01715d7820576ffe390) to handle the case where
a mouse click causes some window-shuffle, so the up event ends up
pointing into another window.  IOW, a similar problem to the
display-line-number one you just fixed.

I think my code is immune to this problem since with it we only ever
generate a drag event if the mouse actually moved.  Which leads to
a further simplification of the code, see patch below.


        Stefan

diff --git a/src/keyboard.c b/src/keyboard.c
index dc2f78a7c26..f9777eee120 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5530,10 +5530,9 @@ #define ISO_FUNCTION_KEY_OFFSET 0xfe00
 /* A cons recording the original frame-relative x and y coordinates of
    the down mouse event.  */
 static Lisp_Object frame_relative_event_pos;
-
-/* The line-number display width, in columns, at the time of most
-   recent down mouse event.  */
-static int down_mouse_line_number_width;
+/* False iff the mouse has stayed within `double_click_fuzz` of
+   `frame_relative_event_pos`.  */
+static bool mouse_has_moved;
 
 /* Information about the most recent up-going button event:  Which
    button, what location, and what time.  */
@@ -5931,55 +5930,19 @@ coords_in_tab_bar_window (struct frame *f, int x, int y)
 
 #endif /* HAVE_WINDOW_SYSTEM */
 
-static void
-save_line_number_display_width (struct input_event *event)
-{
-  struct window *w;
-  int pixel_width;
-
-  if (WINDOWP (event->frame_or_window))
-    w = XWINDOW (event->frame_or_window);
-  else if (FRAMEP (event->frame_or_window))
-    w = XWINDOW (XFRAME (event->frame_or_window)->selected_window);
-  else
-    w = XWINDOW (selected_window);
-  line_number_display_width (w, &down_mouse_line_number_width, &pixel_width);
-}
-
-/* Return non-zero if the change of position from START_POS to END_POS
-   is likely to be the effect of horizontal scrolling due to a change
-   in line-number width produced by redisplay between two mouse
-   events, like mouse-down followed by mouse-up, at those positions.
-   This is used to decide whether to converts mouse-down followed by
-   mouse-up event into a mouse-drag event.  */
-static bool
-line_number_mode_hscroll (Lisp_Object start_pos, Lisp_Object end_pos)
+/*  */
+void
+check_mouse_click_fuzz (EMACS_INT x, EMACS_INT y)
 {
-  if (!EQ (Fcar (start_pos), Fcar (end_pos)) /* different window */
-      || list_length (start_pos) < 7        /* no COL/ROW info */
-      || list_length (end_pos) < 7)
-    return false;
+  EMACS_INT xdiff = x - XFIXNUM (XCAR (frame_relative_event_pos));
+  EMACS_INT ydiff = y - XFIXNUM (XCDR (frame_relative_event_pos));
 
-  Lisp_Object start_col_row = Fnth (make_fixnum (6), start_pos);
-  Lisp_Object end_col_row = Fnth (make_fixnum (6), end_pos);
-  Lisp_Object window = Fcar (end_pos);
-  int col_width, pixel_width;
-  Lisp_Object start_col, end_col;
-  struct window *w;
-  if (!WINDOW_VALID_P (window))
-    {
-      if (WINDOW_LIVE_P (window))
-       window = XFRAME (window)->selected_window;
-      else
-       window = selected_window;
-    }
-  w = XWINDOW (window);
-  line_number_display_width (w, &col_width, &pixel_width);
-  start_col = Fcar (start_col_row);
-  end_col = Fcar (end_col_row);
-  return EQ (start_col, end_col)
-        && down_mouse_line_number_width >= 0
-        && col_width != down_mouse_line_number_width;
+  if (0 < double_click_fuzz
+      && - double_click_fuzz < xdiff
+      && xdiff < double_click_fuzz
+      && - double_click_fuzz < ydiff
+      && ydiff < double_click_fuzz)
+    mouse_has_moved = true;
 }
 
 /* Given a struct input_event, build the lisp event which represents
@@ -6383,9 +6346,8 @@ make_lispy_event (struct input_event *event)
            button_down_time = event->timestamp;
            *start_pos_ptr = Fcopy_alist (position);
            frame_relative_event_pos = Fcons (event->x, event->y);
+           mouse_has_moved = false;
            ignore_mouse_drag_p = false;
-           /* Squirrel away the line-number width, if any.  */
-           save_line_number_display_width (event);
          }
 
        /* Now we're releasing a button - check the coordinates to
@@ -6407,78 +6369,16 @@ make_lispy_event (struct input_event *event)
              ignore_mouse_drag_p = false;
            else
              {
-               intmax_t xdiff = double_click_fuzz, ydiff = double_click_fuzz;
-
-               xdiff = XFIXNUM (event->x)
-                 - XFIXNUM (XCAR (frame_relative_event_pos));
-               ydiff = XFIXNUM (event->y)
-                 - XFIXNUM (XCDR (frame_relative_event_pos));
-
-               if (! (0 < double_click_fuzz
-                      && - double_click_fuzz < xdiff
-                      && xdiff < double_click_fuzz
-                      && - double_click_fuzz < ydiff
-                      && ydiff < double_click_fuzz
-                      /* 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
-                         a click.  */
-                      /* FIXME: OTOH if the buffer position has changed
-                         because of a timer or process filter rather than
-                         because of mouse movement, it should be considered as
-                         a click.  But mouse-drag-region completely ignores
-                         this case and it hasn't caused any real problem, so
-                         it's probably OK to ignore it as well.  */
-                      && (EQ (Fcar (Fcdr (start_pos)),
-                              Fcar (Fcdr (position))) /* Same buffer pos */
-                          /* Redisplay hscrolled text between down- and
-                              up-events due to display-line-numbers-mode.  */
-                          || line_number_mode_hscroll (start_pos, position)
-                          || !EQ (Fcar (start_pos),
-                                  Fcar (position))))) /* Different window */
-
+               /* Check if this position is within the fuzz.  */
+               check_mouse_click_fuzz (XFIXNUM (event->x), XFIXNUM (event->y));
+               if (mouse_has_moved
+                   && (!EQ (Fcar (Fcdr (start_pos)),
+                            Fcar (Fcdr (position)))) /* Different buffer pos */
+                   && EQ (Fcar (start_pos), Fcar (position))) /* Same window */
                  {
                    /* Mouse has moved enough.  */
                    button_down_time = 0;
                    click_or_drag_modifier = drag_modifier;
-                   /* Reset the value for future clicks.  */
-                   down_mouse_line_number_width = -1;
-                 }
-               else if (((!EQ (Fcar (start_pos), Fcar (position)))
-                         || (!EQ (Fcar (Fcdr (start_pos)),
-                                  Fcar (Fcdr (position)))))
-                        /* Was the down event in a window body? */
-                        && FIXNUMP (Fcar (Fcdr (start_pos)))
-                        && WINDOW_LIVE_P (Fcar (start_pos))
-                        && !NILP (Ffboundp (Qwindow_edges)))
-                 /* If the window (etc.) at the mouse position has
-                    changed between the down event and the up event,
-                    we assume there's been a redisplay between the
-                    two events, and we pretend the mouse is still in
-                    the old window to prevent a spurious drag event
-                    being generated.  */
-                 {
-                   Lisp_Object edges
-                     = call4 (Qwindow_edges, Fcar (start_pos), Qt, Qnil, Qt);
-                   int new_x = XFIXNUM (Fcar (frame_relative_event_pos));
-                   int new_y = XFIXNUM (Fcdr (frame_relative_event_pos));
-
-                   /* If the up-event is outside the down-event's
-                      window, use coordinates that are within it.  */
-                   if (new_x < XFIXNUM (Fcar (edges)))
-                     new_x = XFIXNUM (Fcar (edges));
-                   else if (new_x >= XFIXNUM (Fcar (Fcdr (Fcdr (edges)))))
-                     new_x = XFIXNUM (Fcar (Fcdr (Fcdr (edges)))) - 1;
-                   if (new_y < XFIXNUM (Fcar (Fcdr (edges))))
-                     new_y = XFIXNUM (Fcar (Fcdr (edges)));
-                   else if (new_y
-                            >= XFIXNUM (Fcar (Fcdr (Fcdr (Fcdr (edges))))))
-                     new_y = XFIXNUM (Fcar (Fcdr (Fcdr (Fcdr (edges))))) - 1;
-
-                   position = make_lispy_position
-                     (XFRAME (event->frame_or_window),
-                      make_fixnum (new_x), make_fixnum (new_y),
-                      event->timestamp);
                  }
              }
 
@@ -7084,6 +6984,7 @@ make_lispy_movement (struct frame *frame, Lisp_Object 
bar_window, enum scroll_ba
     {
       Lisp_Object position;
       position = make_lispy_position (frame, x, y, t);
+      mouse_has_moved = true;
       return list2 (Qmouse_movement, position);
     }
 }