bug#23419: 25.0.93; double-click-time

bug#23419: 25.0.93; double-click-time

[martin rudalics]

The documentation of ‘double-click-time’ is as:

   Maximum time between mouse clicks to make a double-click.
   Measured in milliseconds.  The value nil means disable double-click
   recognition; t means double-clicks have no time limit and are detected
   by position only.

However, most uses of ‘double-click-time’ in the lisp directory silently
assume that the value is a number which typically fails in ‘sit-for’s
like, for example, in help.el's

   (not (sit-for (/ double-click-time 1000.0) t))

when ‘double-click-time’ is either t or nil.

Could someone out there who understands the semantics of these
‘sit-for’s please fix them.  I can eventually do that myself but these
semantics are yet pretty unclear to me.

TIA, martin

bug#23419: 25.0.93; double-click-time

[Stefan Kangas]

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

tags 23419 + patch
thanks

martin rudalics <rudalics@gmx.at> writes:

> The documentation of ‘double-click-time’ is as:
>
>   Maximum time between mouse clicks to make a double-click.
>   Measured in milliseconds.  The value nil means disable double-click
>   recognition; t means double-clicks have no time limit and are detected
>   by position only.
>
> However, most uses of ‘double-click-time’ in the lisp directory silently
> assume that the value is a number which typically fails in ‘sit-for’s
> like, for example, in help.el's
>
>   (not (sit-for (/ double-click-time 1000.0) t))
>
> when ‘double-click-time’ is either t or nil.
>
> Could someone out there who understands the semantics of these
> ‘sit-for’s please fix them.  I can eventually do that myself but these
> semantics are yet pretty unclear to me.

I can confirm this.  To reproduce the issue in help.el, use:

0. emacs -Q
1. (setq double-click-time nil)
2. C-h k mouse-1

In the attached patch I add a new function `mouse-double-click-time'
which always returns a number.  This seems better than adding
specialized logic everywhere to handle the various cases.

Any comments?

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Fix-handling-double-click-time-nil-or-t-Bug-23419.patch --]
[-- Type: text/x-diff, Size: 6050 bytes --]

From 81d8fcd69fd04fed6fbf0e0e88b8df1bf3f4f603 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sat, 15 Aug 2020 05:41:16 +0200
Subject: [PATCH] Fix handling double-click-time nil or t (Bug#23419)

* lisp/mouse.el (mouse-double-click-time): New function to always
return a number for `double-click-time'.
* lisp/emulation/viper-mous.el (viper-multiclick-timeout):
* lisp/foldout.el (foldout-mouse-swallow-events):
* lisp/help.el (help--read-key-sequence):
* lisp/org/org-mouse.el (org-mouse-show-context-menu): Use
'mouse-double-click-time' instead of 'double-click-time'.
* src/keyboard.c (syms_of_keyboard): Mention
'mouse-double-click-time' in doc string of 'double-click-time'.
* test/lisp/mouse-tests.el (mouse-test-mouse-double-click-time):
New test.
---
 lisp/emulation/viper-mous.el |  4 ++--
 lisp/foldout.el              |  2 +-
 lisp/help.el                 |  2 +-
 lisp/mouse.el                | 11 +++++++++++
 lisp/org/org-mouse.el        |  2 +-
 src/keyboard.c               |  5 ++++-
 test/lisp/mouse-tests.el     | 14 ++++++++++++++
 7 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/lisp/emulation/viper-mous.el b/lisp/emulation/viper-mous.el
index 6ecfec548c..02f4ff4597 100644
--- a/lisp/emulation/viper-mous.el
+++ b/lisp/emulation/viper-mous.el
@@ -66,9 +66,9 @@ viper-surrounding-word-function
 ;; time interval in millisecond within which successive clicks are
 ;; considered related
 (defcustom viper-multiclick-timeout (if (viper-window-display-p)
-                                        double-click-time
+                                        (mouse-double-click-time)
 				    500)
-  "Time interval in millisecond within which successive mouse clicks are
+  "Time interval in milliseconds within which successive mouse clicks are
 considered related."
   :type 'integer
   :group 'viper-mouse)
diff --git a/lisp/foldout.el b/lisp/foldout.el
index 0d7a7a88a6..4a3a02b22d 100644
--- a/lisp/foldout.el
+++ b/lisp/foldout.el
@@ -486,7 +486,7 @@ foldout-mouse-swallow-events
   "Swallow intervening mouse events so we only get the final click-count.
 Signal an error if the final event isn't the same type as the first one."
   (let ((initial-event-type (event-basic-type event)))
-    (while (null (sit-for (/ double-click-time 1000.0) 'nodisplay))
+    (while (null (sit-for (/ (mouse-double-click-time) 1000.0) 'nodisplay))
       (setq event (read-event)))
     (or (eq initial-event-type (event-basic-type event))
 	(error "")))
diff --git a/lisp/help.el b/lisp/help.el
index b7d867eb70..6760c04019 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -766,7 +766,7 @@ help--read-key-sequence
                   (memq 'down last-modifiers)
                   ;; After a click, see if a double click is on the way.
                   (and (memq 'click last-modifiers)
-                       (not (sit-for (/ double-click-time 1000.0) t))))
+                       (not (sit-for (/ (mouse-double-click-time) 1000.0) t))))
             (let* ((seq (read-key-sequence "\
 Describe the following key, mouse click, or menu item: "
                                            nil nil 'can-return-switch-frame))
diff --git a/lisp/mouse.el b/lisp/mouse.el
index a06ca2a56c..0a62211dd7 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -154,6 +154,17 @@ key-translation-map
 (define-key key-translation-map [double-mouse-1]
   #'mouse--click-1-maybe-follows-link)
 
+(defun mouse-double-click-time ()
+  "Return a number for `double-click-time'.
+In contrast to using the `double-click-time' variable directly,
+which could be set to nil or t, this function is guaranteed to
+always return a positive integer or zero."
+  (let ((ct double-click-time))
+   (cond ((eq ct t) 10000) ; arbitrary number useful for sit-for
+         ((eq ct nil) 0)
+         ((and (numberp ct) (> ct 0)) ct)
+         (t 0))))
+
 \f
 ;; Provide a mode-specific menu on a mouse button.
 
diff --git a/lisp/org/org-mouse.el b/lisp/org/org-mouse.el
index 02798874d2..0cc8c82fdd 100644
--- a/lisp/org/org-mouse.el
+++ b/lisp/org/org-mouse.el
@@ -210,7 +210,7 @@ org-mouse-show-context-menu
   (interactive "@e \nP")
   (if (and (= (event-click-count event) 1)
 	   (or (not mark-active)
-	       (sit-for (/ double-click-time 1000.0))))
+               (sit-for (/ (mouse-double-click-time) 1000.0))))
       (progn
 	(select-window (posn-window (event-start event)))
 	(when (not (org-mouse-mark-active))
diff --git a/src/keyboard.c b/src/keyboard.c
index 5fa58abce1..1e957ee91e 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -11887,7 +11887,10 @@ syms_of_keyboard (void)
 	       doc: /* Maximum time between mouse clicks to make a double-click.
 Measured in milliseconds.  The value nil means disable double-click
 recognition; t means double-clicks have no time limit and are detected
-by position only.  */);
+by position only.
+
+In Lisp, you might want to use `mouse-double-click-time' instead of
+reading the value of this variable directly.  */);
   Vdouble_click_time = make_fixnum (500);
 
   DEFVAR_INT ("double-click-fuzz", double_click_fuzz,
diff --git a/test/lisp/mouse-tests.el b/test/lisp/mouse-tests.el
index d520da7af5..d7f84ba04d 100644
--- a/test/lisp/mouse-tests.el
+++ b/test/lisp/mouse-tests.el
@@ -25,6 +25,20 @@
 
 ;;; Code:
 
+(ert-deftest mouse-test-mouse-double-click-time ()
+  (let ((double-click-time 500))
+    (should (= (mouse-double-click-time) 500)))
+  (let ((double-click-time 0))
+    (should (= (mouse-double-click-time) 0)))
+  (let ((double-click-time -500))
+    (should (= (mouse-double-click-time) 0)))
+  (let ((double-click-time nil))
+    (should (= (mouse-double-click-time) 0)))
+  (let ((double-click-time t))
+    (should (numberp (mouse-double-click-time))))
+  (let ((double-click-time '(invalid)))
+    (should (= (mouse-double-click-time) 0))))
+
 (ert-deftest bug23288-use-return-value ()
   "If `mouse-on-link-p' returns a string, its first character is used."
   (cl-letf ((unread-command-events '((down-mouse-1 nil 1) (mouse-1 nil 1)))
-- 
2.28.0

bug#23419: 25.0.93; double-click-time

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 14 Aug 2020 20:49:18 -0700
> Cc: 23419@debbugs.gnu.org
> 
> In the attached patch I add a new function `mouse-double-click-time'
> which always returns a number.  This seems better than adding
> specialized logic everywhere to handle the various cases.

I don't see how this resolves the issue pointed out by Martin.  How do
we know that the problematic callers will DTRT with the arbitrary
values your function returns for t and nil?  Using a very large number
for t may be a good approximation (although I'm not even sure in that
case), but using zero for nil?

I'm afraid there's no way around auditing each caller and determining
what those non-numerical values mean in each case.  For example, I'd
actually suggest to avoid the call to sit-for in the nil case, because
a double-click will never be "on the way" in that case.

Thanks.

bug#23419: 25.0.93; double-click-time

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm afraid there's no way around auditing each caller and determining
> what those non-numerical values mean in each case.  For example, I'd
> actually suggest to avoid the call to sit-for in the nil case, because
> a double-click will never be "on the way" in that case.

I've gone through all the usage sites, and I think Stefan's patch is
correct -- they're using double-click-time as a sensible default for
what they're doing, but they're not really interested in the nil logic
(i.e., they're not supposed to be disabled in that case).

So I've now pushed Stefan's patch to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no