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

[Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: drag.patch --]
[-- Type: text/x-diff, Size: 5299 bytes --]

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);
     }
 }