Package: emacs;
Reported by: tomasralph2000 <at> gmail.com
Date: Fri, 20 Oct 2023 20:29:01 UTC
Severity: normal
Tags: fixed
Found in version 29.1
View this message in rfc822 format
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: tomasralph2000 <at> gmail.com, 66655 <at> debbugs.gnu.org Subject: bug#66655: 29.1; Clicking buttons sometimes doesn't work Date: Wed, 25 Oct 2023 17:22:41 -0400
[Message part 1 (text/plain, inline)]
>> > 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
[drag.patch (text/x-diff, inline)]
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);
}
}
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.