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: 2086 bytes --]

>> > 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

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

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