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

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

[tomasralph2000]

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

If using =display-line-numbers-mode= with the =visual= style set for it,
sometimes clicking buttons does not actually click it. It instead shifts
the buffer slightly, either to the left or to the right, and a mark is
set (by clicking I refer with the mouse, specifically). You will need to
press again in order to actually click the button.

This is inconsistent, there's no guarantee that it will happen, it
sometimes does, but it is often enough for you to be able to notice within
just a couple of seconds of clicking through things.

You can launch =emacs -Q= and run these two lines:

(setq display-line-numbers-type 'visual)
(global-display-line-numbers-mode)

Then just click through buttons. A good test is to open the emacs manual
=C-h R emacs= since it has a big index.

The bug does not happen with =relative= style line numbers.

In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38,
cairo version 1.17.8)
System Description: Arch Linux

Configured using:
 'configure --with-pgtk --with-native-compilation=aot --sysconfdir=/etc
 --prefix=/usr --libexecdir=/usr/lib --with-tree-sitter
 --localstatedir=/var --with-cairo --disable-build-details
 --with-harfbuzz --with-libsystemd --with-modules 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2
 -Wformat -Werror=format-security -fstack-clash-protection
 -fcf-protection -g
 -ffile-prefix-map=/build/emacs/src=/usr/src/debug/emacs -flto=auto'
 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -flto=auto'
 'CXXFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions
 -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security
 -fstack-clash-protection -fcf-protection -Wp,-D_GLIBCXX_ASSERTIONS -g
 -ffile-prefix-map=/build/emacs/src=/usr/src/debug/emacs -flto=auto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
 value of $LANG: en_US.UTF-8
 locale-coding-system: utf-8-unix

Major mode: Dashboard

Minor modes in effect:
 global-undo-tree-mode: t
 undo-tree-mode: t
 marginalia-mode: t
 which-key-mode: t
 global-tree-sitter-mode: t
 recentf-mode: t
 treemacs-filewatch-mode: t
 treemacs-follow-mode: t
 treemacs-git-mode: t
 global-git-commit-mode: t
 magit-auto-revert-mode: t
 shell-dirtrack-mode: t
 override-global-mode: t
 vertico-mouse-mode: t
 vertico-mode: t
 global-company-mode: t
 company-mode: t
 pixel-scroll-precision-mode: t
 xterm-mouse-mode: t
 global-auto-revert-mode: t
 electric-pair-mode: t
 delete-selection-mode: t
 tooltip-mode: t
 global-eldoc-mode: t
 show-paren-mode: t
 electric-indent-mode: t
 mouse-wheel-mode: t
 file-name-shadow-mode: t
 context-menu-mode: t
 global-font-lock-mode: t
 font-lock-mode: t
 blink-cursor-mode: t
 buffer-read-only: t
 line-number-mode: t
 transient-mark-mode: t
 auto-composition-mode: t
 auto-encryption-mode: t
 auto-compression-mode: t

Load-path shadows:
/home/tralph3/.local/share/emacs/elpa/transient-20230919.2146/transient hides /usr/share/emacs/29.1/lisp/transient
/home/tralph3/.local/share/emacs/elpa/seq-2.24/seq hides /usr/share/emacs/29.1/lisp/emacs-lisp/seq

Features:
(shadow sort mail-extr emacsbug mule-util display-line-numbers time
org-tempo tempo eglot external-completion array jsonrpc ert ewoc debug
backtrace flymake-proc flymake vterm tramp tramp-loaddefs trampver
tramp-integration files-x tramp-compat parse-time iso8601 face-remap
term disp-table ehelp vterm-module term/xterm xterm embark-consult
consult treemacs-bookmarks treemacs-tags magit-bookmark bookmark
embark-org embark multiple-cursors mc-separate-operations
rectangular-region-mode mc-mark-pop mc-edit-lines
mc-hide-unmatched-lines-mode mc-mark-more mc-cycle-cursors
multiple-cursors-core advice rect undo-tree queue org-fragtog
org-superstar org-element org-persist xdg org-id org-refile avl-tree org
ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro org-src ob-comint
org-pcomplete org-list org-footnote org-faces org-entities noutline
outline ob-emacs-lisp ob-core ob-eval org-cycle org-table ol org-fold
org-fold-core org-keys oc org-loaddefs find-func cal-menu calendar
cal-loaddefs org-version org-compat org-macs marginalia which-key
tree-sitter-langs tree-sitter-langs-build tar-mode arc-mode archive-mode
pp tree-sitter-hl tree-sitter tree-sitter-load tree-sitter-cli tsc
tsc-dyn tsc-dyn-get dired-aux tsc-obsolete dashboard dashboard-widgets
recentf tree-widget wid-edit ffap treemacs treemacs-header-line
treemacs-compatibility treemacs-mode treemacs-interface
treemacs-persistence treemacs-filewatch-mode treemacs-follow-mode
treemacs-rendering treemacs-annotations treemacs-async
treemacs-workspaces treemacs-dom treemacs-visuals
treemacs-fringe-indicator pulse color treemacs-faces treemacs-icons
treemacs-scope treemacs-themes treemacs-core-utils pfuture inline
hl-line ht treemacs-logging treemacs-customization treemacs-macros s
orderless magit-submodule magit-blame magit-stash magit-reflog
magit-bisect magit-push magit-pull magit-fetch magit-clone magit-remote
magit-commit magit-sequence magit-notes magit-worktree magit-tag
magit-merge magit-branch magit-reset magit-files magit-refs magit-status
magit magit-repos magit-apply magit-wip magit-log which-func imenu
magit-diff smerge-mode diff diff-mode git-commit log-edit message
sendmail yank-media puny dired dired-loaddefs rfc822 mml mml-sec epa
derived epg rfc6068 epg-config gnus-util time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr mailabbrev mail-utils gmm-utils mailheader pcvs-util add-log
magit-core magit-autorevert magit-margin magit-transient magit-process
with-editor shell pcomplete server magit-mode transient magit-git
magit-base magit-section format-spec cursor-sensor crm dash
use-package-bind-key bind-key easy-mmode vertico-mouse vertico compat
yaml-mode lua-mode edmacro kmacro dart-mode flutter flutter-l10n
flutter-project rust-utils thingatpt rust-mode rust-rustfmt rust-playpen
rust-compile compile text-property-search comint ansi-osc ansi-color
rust-cargo company-oddmuse company-keywords company-etags etags fileloop
generator xref project company-gtags company-dabbrev-code
company-dabbrev company-files company-clang company-capf company-cmake
company-semantic company-template company-bbdb company all-the-icons
all-the-icons-faces data-material data-weathericons data-octicons
data-fileicons data-faicons data-alltheicons use-package-ensure
use-package-core system-theme system-theme-theme pixel-scroll cua-base
ring xt-mouse autorevert filenotify elec-pair delsel comp comp-cstr
warnings icons rx cl-extra help-mode all-the-icons-autoloads
company-autoloads corfu-autoloads dart-mode-autoloads
dashboard-autoloads dirvish-autoloads embark-consult-autoloads
consult-autoloads embark-autoloads flutter-autoloads kind-icon-autoloads
lua-mode-autoloads magit-autoloads pcase git-commit-autoloads
marginalia-autoloads multiple-cursors-autoloads orderless-autoloads
org-fragtog-autoloads org-roam-ui-autoloads org-roam-autoloads
magit-section-autoloads emacsql-autoloads org-superstar-autoloads
rust-mode-autoloads simple-httpd-autoloads svg-lib-autoloads
transient-autoloads tree-sitter-langs-autoloads tree-sitter-autoloads
treemacs-autoloads cfrs-autoloads posframe-autoloads ht-autoloads
hydra-autoloads lv-autoloads pfuture-autoloads ace-window-autoloads
avy-autoloads s-autoloads dash-autoloads tsc-autoloads
undo-tree-autoloads queue-autoloads vertico-autoloads vterm-autoloads
websocket-autoloads which-key-autoloads with-editor-autoloads info
compat-autoloads seq-autoloads yaml-mode-autoloads package browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win
term/common-win pgtk-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo gtk pgtk lcms2 multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 446436 331408)
 (symbols 48 33070 158)
 (strings 32 125268 59058)
 (string-bytes 1 4550538)
 (vectors 16 73483)
 (vector-slots 8 1709579 787318)
 (floats 8 1069 2511)
 (intervals 56 1048 450)
 (buffers 984 14))

[-- Attachment #2: Type: text/html, Size: 10298 bytes --]

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

[Stefan Kangas]

tomasralph2000@gmail.com writes:

> If using =display-line-numbers-mode= with the =visual= style set for it,
> sometimes clicking buttons does not actually click it. It instead shifts
> the buffer slightly, either to the left or to the right, and a mark is
> set (by clicking I refer with the mouse, specifically). You will need to
> press again in order to actually click the button.
>
> This is inconsistent, there's no guarantee that it will happen, it
> sometimes does, but it is often enough for you to be able to notice within
> just a couple of seconds of clicking through things.
>
> You can launch =emacs -Q= and run these two lines:
>
> (setq display-line-numbers-type 'visual)
> (global-display-line-numbers-mode)
>
> Then just click through buttons. A good test is to open the emacs manual
> =C-h R emacs= since it has a big index.
>
> The bug does not happen with =relative= style line numbers.

I can reproduce this on master, but it did take like 1-2 minutes of
clicking around to trigger it.

I might be making this up, but I feel like I've seen this before --
without `display-line-numbers-mode'.  Perhaps there's some rare bug that
is just more frequently triggered when that mode is on?  But I'm not
really sure about that, it's quite possible that I'm wrong.


In GNU Emacs 30.0.50 (build 1, x86_64-apple-darwin21.6.0, NS
 appkit-2113.60 Version 12.7 (Build 21G816)) of 2023-10-18 built on
 Newton.local
Repository revision: 50514298cf98fcd0dfb0d59a7e03b1ebb0d03490
Repository branch: scratch/no-purespace
Windowing system distributor 'Apple', version 10.3.2113
System Description:  macOS 12.7

Configured using:
 'configure 'LDFLAGS=-L/usr/local/opt/llvm/lib
 -L/usr/local/opt/libffi/lib' 'CPPFLAGS=-I/usr/local/opt/llvm/include
 -I/usr/local/opt/libffi/include''

Configured features:
ACL GIF GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY KQUEUE NS
PDUMPER PNG SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP
XIM ZLIB

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

[Eli Zaretskii]

> Date: Fri, 20 Oct 2023 20:27:19 +0000
> From: tomasralph2000@gmail.com
> 
> If using =display-line-numbers-mode= with the =visual= style set for it,
> sometimes clicking buttons does not actually click it. It instead shifts
> the buffer slightly, either to the left or to the right, and a mark is
> set (by clicking I refer with the mouse, specifically). You will need to
> press again in order to actually click the button.
> 
> This is inconsistent, there's no guarantee that it will happen, it
> sometimes does, but it is often enough for you to be able to notice within
> just a couple of seconds of clicking through things.
> 
> You can launch =emacs -Q= and run these two lines:
> 
> (setq display-line-numbers-type 'visual)
> (global-display-line-numbers-mode)
> 
> Then just click through buttons. A good test is to open the emacs manual
> =C-h R emacs= since it has a big index.

Thanks, should be fixed now on the master branch.

I only tested this in Info buffers, so any testing with other kinds of
buttons/links will be welcome.

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.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

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

I can't see the change on master.  Did you forget to push perhaps?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 21 Oct 2023 04:23:30 -0700
> Cc: 66655@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 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.
> 
> I can't see the change on master.  Did you forget to push perhaps?

Oops.

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

[Stefan Kangas]

tags 66655 fixed
thanks

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, should be fixed now on the master branch.

I'm adding the fixed tag.

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]

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


        Stefan

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

[Eli Zaretskii]

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

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]

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

So maybe, in order to upgrade an `up` to a `drag` we should check that:
- the end (buffer) position is different.
- and the mouse has actually moved.
Currently we detect a mouse-has-moved when the start and end (screen)
position of the mouse are different, but maybe we should additionally
set a flag when the mouse is moved, so that it's still considered as
"moved" if the mouse returns to its initial (screen) position?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Mon, 23 Oct 2023 18:36:11 -0400
> 
> > 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.
> 
> So maybe, in order to upgrade an `up` to a `drag` we should check that:
> - the end (buffer) position is different.
> - and the mouse has actually moved.

How do you do the latter in a way that still supports arbitrary values
in double-click-fuzz?

> Currently we detect a mouse-has-moved when the start and end (screen)
> position of the mouse are different, but maybe we should additionally
> set a flag when the mouse is moved, so that it's still considered as
> "moved" if the mouse returns to its initial (screen) position?

What will that do if the mouse was moved, then returned to the
original position before the button is released?

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]

> What will that do if the mouse was moved, then returned to the
> original position before the button is released?

In the sample patch I sent it depends if the original mouse/screen position
still shows the same buffer position.  If it does, then the `up` will
stay as an `up`, otherwise it will turn into a `drag`.
[ At least, that's what I think my code does.  ]


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Tue, 24 Oct 2023 09:44:20 -0400
> 
> > What will that do if the mouse was moved, then returned to the
> > original position before the button is released?
> 
> In the sample patch I sent it depends if the original mouse/screen position
> still shows the same buffer position.  If it does, then the `up` will
> stay as an `up`, otherwise it will turn into a `drag`.
> [ At least, that's what I think my code does.  ]

OK, but I think the issue with double-click-fuzz still remains to be
solved with this approach.

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]

>> > What will that do if the mouse was moved, then returned to the
>> > original position before the button is released?
>> 
>> In the sample patch I sent it depends if the original mouse/screen position
>> still shows the same buffer position.  If it does, then the `up` will
>> stay as an `up`, otherwise it will turn into a `drag`.
>> [ At least, that's what I think my code does.  ]
>
> OK, but I think the issue with double-click-fuzz still remains to be
> solved with this approach.

Not sure what you men by that (probably because I don't know enough of
what we do with double-click-fuzz, i.e. where we do obey it and where we
don't).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Tue, 24 Oct 2023 09:57:26 -0400
> 
> >> > What will that do if the mouse was moved, then returned to the
> >> > original position before the button is released?
> >> 
> >> In the sample patch I sent it depends if the original mouse/screen position
> >> still shows the same buffer position.  If it does, then the `up` will
> >> stay as an `up`, otherwise it will turn into a `drag`.
> >> [ At least, that's what I think my code does.  ]
> >
> > OK, but I think the issue with double-click-fuzz still remains to be
> > solved with this approach.
> 
> Not sure what you men by that (probably because I don't know enough of
> what we do with double-click-fuzz, i.e. where we do obey it and where we
> don't).

It is used to decide whether to emit a drag event or a click event,
just search for it in keyboard.c and you will find the code.

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]

>> > OK, but I think the issue with double-click-fuzz still remains to be
>> > solved with this approach.
>> 
>> Not sure what you men by that (probably because I don't know enough of
>> what we do with double-click-fuzz, i.e. where we do obey it and where we
>> don't).
>
> It is used to decide whether to emit a drag event or a click event,
> just search for it in keyboard.c and you will find the code.

I still don't see what you mean by "the issue with double-click-fuzz
still remains to be solved with this approach".

AFAICT my patch still obeys `double-click-fuzz` at the same place using
the same code.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Tue, 24 Oct 2023 10:29:30 -0400
> 
> >> > OK, but I think the issue with double-click-fuzz still remains to be
> >> > solved with this approach.
> >> 
> >> Not sure what you men by that (probably because I don't know enough of
> >> what we do with double-click-fuzz, i.e. where we do obey it and where we
> >> don't).
> >
> > It is used to decide whether to emit a drag event or a click event,
> > just search for it in keyboard.c and you will find the code.
> 
> I still don't see what you mean by "the issue with double-click-fuzz
> still remains to be solved with this approach".
> 
> AFAICT my patch still obeys `double-click-fuzz` at the same place using
> the same code.

You suggested to check whether the mouse coordinates changed between
the down- and up-event.  My point is that we already check the screen
position of the mouse between these two events, and allow them to
change if the change is small enough.  So I'm not sure I understand
what is new in your idea, if you want to keep the double-click-fuzz
test.

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]

> change if the change is small enough.  So I'm not sure I understand
> what is new in your idea, if you want to keep the double-click-fuzz
> test.

What's new is the `mouse_has_moved` boolean which remembers if the mouse
has been moved some time between the down and the up, contrary to the
current code which only looks at the relative position of the down and
the up.

The patch looks big mostly because it reverts your patch.


        Setfan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Tue, 24 Oct 2023 10:50:03 -0400
> 
> > change if the change is small enough.  So I'm not sure I understand
> > what is new in your idea, if you want to keep the double-click-fuzz
> > test.
> 
> What's new is the `mouse_has_moved` boolean which remembers if the mouse
> has been moved some time between the down and the up, contrary to the
> current code which only looks at the relative position of the down and
> the up.

With the current code, if I move the mouse between the events, but the
coordinates differ by less than double-click-fuzz, we will not
generate a drag event.  If the mouse-moved flag overrides that, we
will generate a drag event where previously we didn't, isn't that so?

> The patch looks big mostly because it reverts your patch.

I know, and that doesn't bother me per se.

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]

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

> With the current code, if I move the mouse between the events, but the
> coordinates differ by less than double-click-fuzz, we will not
> generate a drag event.  If the mouse-moved flag overrides that, we
> will generate a drag event where previously we didn't, isn't that so?

Good point.

The patch below should fix this problem (as well as the problem that we
only considered the mouse to move when we emitted a `mouse-movement`
event).


        Stefan

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

diff --git a/src/indent.c b/src/indent.c
index 7d34d3638d9..ca8d59ff8f0 100644
--- a/src/indent.c
+++ b/src/indent.c
@@ -2031,7 +2031,7 @@ vmotion (ptrdiff_t from, ptrdiff_t from_byte,
 }
 
 /* Return the width taken by line-number display in window W.  */
-void
+static void
 line_number_display_width (struct window *w, int *width, int *pixel_width)
 {
   if (NILP (Vdisplay_line_numbers))
diff --git a/src/keyboard.c b/src/keyboard.c
index dc2f78a7c26..56672b7d453 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;
+/* True 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,42 +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)),
@@ -7084,6 +7020,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);
     }
 }
diff --git a/src/keyboard.h b/src/keyboard.h
index 9f6e65f9a09..29591d1c39f 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -507,6 +507,7 @@ kbd_buffer_store_event_hold (struct input_event *event,
                             Lisp_Object);
 extern void gen_help_event (Lisp_Object, Lisp_Object, Lisp_Object,
                             Lisp_Object, ptrdiff_t);
+extern void check_mouse_click_fuzz (EMACS_INT x, EMACS_INT y);
 extern void kbd_buffer_store_help_event (Lisp_Object, Lisp_Object);
 extern Lisp_Object menu_item_eval_property (Lisp_Object);
 extern bool kbd_buffer_events_waiting (void);
diff --git a/src/lisp.h b/src/lisp.h
index df6cf1df544..39aa51531fe 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4883,7 +4883,6 @@ fast_c_string_match_ignore_case (Lisp_Object regexp,
 
 /* Defined in indent.c.  */
 extern ptrdiff_t current_column (void);
-extern void line_number_display_width (struct window *, int *, int *);
 extern void invalidate_current_column (void);
 extern bool indented_beyond_p (ptrdiff_t, ptrdiff_t, EMACS_INT);
 extern void syms_of_indent (void);
diff --git a/src/xdisp.c b/src/xdisp.c
index b9009df5df9..9eff1f6dd91 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -35414,6 +35414,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
   Lisp_Object pointer = Qnil;  /* Takes precedence over cursor!  */
   struct buffer *b;
 
+  check_mouse_click_fuzz (x, y);
+
   /* When a menu is active, don't highlight because this looks odd.  */
 #if defined (HAVE_X_WINDOWS) || defined (HAVE_NS) || defined (MSDOS) \
   || defined (HAVE_ANDROID)

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Tue, 24 Oct 2023 18:00:14 -0400
> 
> > With the current code, if I move the mouse between the events, but the
> > coordinates differ by less than double-click-fuzz, we will not
> > generate a drag event.  If the mouse-moved flag overrides that, we
> > will generate a drag event where previously we didn't, isn't that so?
> 
> Good point.
> 
> The patch below should fix this problem (as well as the problem that we
> only considered the mouse to move when we emitted a `mouse-movement`
> event).

This seems to assume that double-click-fuzz cannot be large enough to
allow the events be on different buffer positions?  Isn't this change
in behavior?

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]

>> The patch below should fix this problem (as well as the problem that we
>> only considered the mouse to move when we emitted a `mouse-movement`
>> event).
> This seems to assume that double-click-fuzz cannot be large enough to
> allow the events be on different buffer positions?

Where do you see that?

> Isn't this change in behavior?

I don't know.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Wed, 25 Oct 2023 11:13:51 -0400
> 
> >> The patch below should fix this problem (as well as the problem that we
> >> only considered the mouse to move when we emitted a `mouse-movement`
> >> event).
> > This seems to assume that double-click-fuzz cannot be large enough to
> > allow the events be on different buffer positions?
> 
> Where do you see that?

In the test for the equal buffer position.

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]

>> Where do you see that?
> In the test for the equal buffer position.

The old code did:

                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 */
                           || !EQ (Fcar (start_pos),
                                   Fcar (position))))) /* Different window */

which in short is approximately:

                if (! (!mouse_has_moved
                       && (EQ (Fcar (Fcdr (start_pos)),
                               Fcar (Fcdr (position)))
                           || !EQ (Fcar (start_pos),
                                   Fcar (position)))))

If we apply deMorgan we get:

                if (mouse_has_moved
                    || !(EQ (Fcar (Fcdr (start_pos)),
                             Fcar (Fcdr (position)))
                         || !EQ (Fcar (start_pos),
                                 Fcar (position)))))

apply deMorgan again we get:

                if (mouse_has_moved
                    || (!EQ (Fcar (Fcdr (start_pos)),
                             Fcar (Fcdr (position)))
                         && EQ (Fcar (start_pos),
                                Fcar (position)))))

I changed it to:

		if (mouse_has_moved
		    && (!EQ (Fcar (Fcdr (start_pos)),
			     Fcar (Fcdr (position))))
		    && EQ (Fcar (start_pos), Fcar (position)))

The main purpose of the change is to address the "FIXME" in the comment:
if the mouse hasn't moved, I don't think we should generate a `drag`
even if the buffer content under the mouse has changed.

So, IIUC, what you were saying is that with the new code, a small
movement that goes from one buffer position to another both of which are
within the fuzz will be considered as a click whereas with the current
code it will be a `drag`.  Maybe that's worse than the "FIXME" issue it
tries to address?

The other part of the change is the handling of `EQ (Fcar (start_pos),
Fcar (position))` and I must admit I don't know what to do with it, so
this part of the change is largely arbitrary: I don't know why we
currently check this condition nor why we only check it when mouse has
not moved.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Wed, 25 Oct 2023 12:36:00 -0400
> 
> 		if (mouse_has_moved
> 		    && (!EQ (Fcar (Fcdr (start_pos)),
> 			     Fcar (Fcdr (position))))
> 		    && EQ (Fcar (start_pos), Fcar (position)))
> 
> The main purpose of the change is to address the "FIXME" in the comment:
> if the mouse hasn't moved, I don't think we should generate a `drag`
> even if the buffer content under the mouse has changed.

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.

> So, IIUC, what you were saying is that with the new code, a small
> movement that goes from one buffer position to another both of which are
> within the fuzz will be considered as a click whereas with the current
> code it will be a `drag`.  Maybe that's worse than the "FIXME" issue it
> tries to address?
> 
> The other part of the change is the handling of `EQ (Fcar (start_pos),
> Fcar (position))` and I must admit I don't know what to do with it, so
> this part of the change is largely arbitrary: I don't know why we
> currently check this condition nor why we only check it when mouse has
> not moved.

I think the comment above explains that, or at least tries to.

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]

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

So, no we shouldn't suffer from this problem.

>> The other part of the change is the handling of `EQ (Fcar (start_pos),
>> Fcar (position))` and I must admit I don't know what to do with it, so
>> this part of the change is largely arbitrary: I don't know why we
>> currently check this condition nor why we only check it when mouse has
>> not moved.
>
> I think the comment above explains that, or at least tries to.

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?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Wed, 25 Oct 2023 13:27:31 -0400
> 
> > 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?

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

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]

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: tomasralph2000@gmail.com,  66655@debbugs.gnu.org
> Date: Wed, 25 Oct 2023 17:22:41 -0400
> 
> >> 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 ].

I don't think I understand this explanation.  (Did you mean "true"
instead of "false"?)

You started by saying that your code detects mouse movement, and I
asked how does it detect it.  AFAIR, Emacs doesn't receive
mouse-motion events unless we turn on track-mouse, which we usually
avoid doing.  So this code was originally detecting mouse movement by
comparing the screen coordinates of the down- and up-events.  I'm
asking how is your code different?

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

You could have redisplay change the windows under your feet, such that
the screen coordinates are the same, the buffer position is the same,
but the window is not the same.  That test wants to emit a drag event
in such a case.  That is my understanding of the test.

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

Exactly.

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

I still don't understand the implication of the mouse_has_moved part.

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]

>> 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 ].
>
> I don't think I understand this explanation.  (Did you mean "true"
> instead of "false"?)

Yes, I reversed them (and I made a similar mistake in one of the
comments in the patch.  I'm having some kind of "polarity issue",
apparently, sorry 'bout that).

> You started by saying that your code detects mouse movement, and I
> asked how does it detect it.  AFAIR, Emacs doesn't receive
> mouse-motion events unless we turn on track-mouse, which we usually
> avoid doing.

It does receive them (it uses them for `mouse-face` highlighting).
`track-mouse` only controls whether those C-level events get turned
into ELisp-level events.

>> > Yes: we can be at the same buffer position, but a different window.
>> Right, but what's the problem with that?
>
> You could have redisplay change the windows under your feet, such that
> the screen coordinates are the same, the buffer position is the same,
> but the window is not the same.  That test wants to emit a drag event
> in such a case.  That is my understanding of the test.

No, the code does:

                if (mouse_has_moved
                    || (!EQ (Fcar (Fcdr (start_pos)),
                             Fcar (Fcdr (position)))
                         && EQ (Fcar (start_pos),
                                Fcar (position)))))

so the last `EQ` *prevents* emitting a drag event when the window has
changed (but only when the mouse's start and end screen position are the
same).

>> 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.
> I still don't understand the implication of the mouse_has_moved part.

The implication, AFAICT is that with my current code you can't easily
drag over a distance less than fuzz:

     down-mouse-1
     ...move a tiny bit to the next char-cell...
     up-mouse-1

will turn into a click.

But indeed, that's what the fuzz is about because the above "move" could
have been accidental the mere result of pushing the mouse button (what
I'd call "click with noise").

Not sure how to distinguish the above drag from a "click with noise".
Maybe we should take *time* into account?


        Stefan

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]

> OK, but I think the issue with double-click-fuzz still remains to be
> solved with this approach.

I suspect another problem with my patch is that it only takes effect when
we generate `mouse-movement` events, i.e. inside `track-mouse`.


        Stefan

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]

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