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

[Stefan Monnier]

Eli Zaretskii [2023-10-23 21:30:38] wrote:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
>> Date: Mon, 23 Oct 2023 12:38:35 -0400
>> 
>> > Stefan, I'd appreciate your review of the change, as this is a tricky
>> > code, where we already had quite a few changes to avoid interpreting
>> > an up-event as a drag event.
>> 
>> The change looks OK.  But yeah, it does feel like adding yet an
>> other hack.  The whole:
>> 
>>                     /* 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 */
>> 
>> is unsatisfactory.  But it's not clear what is the right way to look at
>> the problem.  As the comment says, we generally want "down+scroll+up" to
>> be treated as a drag, but not in the current case.  I think the
>> difference relies on what caused the scroll: if the scroll was the
>> result of a deliberate act by the user (they moved the mouse after
>> `down` to cause a scroll), then it's a drag and else it's not?
>
> Yes, that's the logic here.  Technically, it happens because clicking
> the mouse emits 2 events: down-mouse-1 followed by another one caused
> by releasing the mouse button, and the first event could cause
> redisplay (as happens in this case) because it generally moves point
> to the location of the click.

How 'bout something like the 100% untested patch below?

Basically, generate a drag only if all 3 are satisfied:
- the mouse has moved (and maybe come back).
- the buffer position has changed.
- the window has not changed (not sure why we have that, but IIUC
  that's a constraint we have on drag events).
And to check the first we simply "mess up" the remembered start position
whenever we emit a mouse-movement.


        Stefan

diff --git a/src/keyboard.c b/src/keyboard.c
index dc2f78a7c26..43d5b085857 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -5530,10 +5530,7 @@ #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;
+static bool mouse_has_moved;
 
 /* Information about the most recent up-going button event:  Which
    button, what location, and what time.  */
@@ -5931,57 +5928,6 @@ 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)
-{
-  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;
-
-  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;
-}
-
 /* 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.
@@ -6383,9 +6329,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
@@ -6415,34 +6360,18 @@ make_lispy_event (struct input_event *event)
                  - XFIXNUM (XCDR (frame_relative_event_pos));
 
                if (! (0 < double_click_fuzz
+                      && !mouse_has_moved
                       && - 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 */
-
+                      && ydiff < double_click_fuzz)
+                    && (!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)),
@@ -7084,6 +7013,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);
     }
 }