bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Drew Adams]

emacs -Q

In *scratch*, evaluate this:

(defgroup foo nil
  "..." :prefix "foo-" :group 'editing
  :link '(url-link "http://www.emacswiki.org/"))

(defcustom foobar t "..." :type 'boolean :group 'foo)

M-x set-variable mouse-1-click-follows-link nil
M-x customize-option foobar

Click the link `http://www.emacswiki.org/' using `mouse-1'.  The link is
followed - it should not be followed.

Note that `mouse-on-link-p' returns `t' for positions on this link, and
such positions have face `custom-link', property `follow-link' with
value `mouse-face', and property `mouse-face' with a face value, all of
which show further that the behavior violates the mandate of
`mouse-1-click-follows-link'.

Quite annoying.  Emacs should not overrule user settings like this.
This is not a regression: it has been broken from the outset (broken in
all Emacs versions that support `:link').

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-10-19 on LEG570
Bzr revision: 114715 rgm@gnu.org-20131019023520-s8mwtib7xcx9e05w
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking 'CFLAGS=-O0 -g3' CPPFLAGS=-DGLYPH_DEBUG=1'

bug#52: bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> (defgroup foo nil
>   "..." :prefix "foo-" :group 'editing
>   :link '(url-link "http://www.emacswiki.org/"))
>
> (defcustom foobar t "..." :type 'boolean :group 'foo)
>
> M-x set-variable mouse-1-click-follows-link nil
> M-x customize-option foobar
>
> Click the link `http://www.emacswiki.org/' using `mouse-1'.  The link is
> followed - it should not be followed.
>
> Note that `mouse-on-link-p' returns `t' for positions on this link, and
> such positions have face `custom-link', property `follow-link' with
> value `mouse-face', and property `mouse-face' with a face value, all of
> which show further that the behavior violates the mandate of
> `mouse-1-click-follows-link'.

I've had another peek at this, but the main problem is that I don't
quite understand why the Widget code is so...  complicated.  Why does it
do all the stuff below?  I mean, no other modes that react to mouse
clicks need to ... do all that...

(defun widget-button-click (event)
  "Invoke the button that the mouse is pointing at."
  (interactive "e")
  (if (widget-event-point event)
      (let* ((oevent event)
	     (mouse-1 (memq (event-basic-type event) '(mouse-1 down-mouse-1)))
	     (pos (widget-event-point event))
	     (start (event-start event))
	     (button (get-char-property
		      pos 'button (and (windowp (posn-window start))
				       (window-buffer (posn-window start)))))
	     newpoint)
	(when (or (null button)
		  (catch 'button-press-cancelled
	      ;; Mouse click on a widget button.  Do the following
	      ;; in a save-excursion so that the click on the button
	      ;; doesn't change point.
	      (save-selected-window
		(select-window (posn-window (event-start event)))
		(save-excursion
		  (goto-char (posn-point (event-start event)))
		  (let* ((overlay (widget-get button :button-overlay))
			 (pressed-face (or (widget-get button :pressed-face)
					   widget-button-pressed-face))
			 (face (overlay-get overlay 'face))
			 (mouse-face (overlay-get overlay 'mouse-face)))


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

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Drew Adams]

> I've had another peek at this, 

Thanks for looking into it.

> but the main problem is that I don't quite understand why the
> Widget code is so...  complicated.

I sympathize, and I agree that is difficult to fathom.
Can't help with the understanding, however.

> Why does it do all the stuff below?  I mean, no other modes
> that react to mouse clicks need to ... do all that...

Dunno.  But just because the code can be difficult to follow
is not a reason to assume that it does unnecessary things, in
general.  The widget code that I've been able to follow does
DTRT, generally (AFAICT).

I doubt that anyone left understands the widget code well.
Maybe you can find Per Abrahamsen and get him to take a look?
The URLs for him listed here are dead, but maybe looking here
would be a start: https://www.emacswiki.org/emacs/PerAbrahamsen.

bug#52: bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> Dunno.  But just because the code can be difficult to follow
> is not a reason to assume that it does unnecessary things, in
> general.  The widget code that I've been able to follow does
> DTRT, generally (AFAICT).

It does, but the logic is very hard to follow.  Custom/widget works by
inserting text into the buffer, and then "converting" it to a widget, or
by creating a widget and then inserting it.  And it does it all with
overlays, and seems like it's created its own event handling distinct
from the normal Emacs event handler, sort of.

There are, of course, historical reasons for this.  Per wrote Widget in
the mid 90s when many of these issues hadn't been resolved, and it
worked across many Emacs versions.

I kinda think it might be time to consider doing a rewrite from scratch
using modern Emacs features, and then things like this bug report would
start working automatically.

How big a task would this be?  I mean, of course we'd keep compatibility
(the defcustom language is fine), but just rewrite the UI `M-x
customize' bits...  Hm...

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

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Drew Adams]

> I kinda think it might be time to consider doing a rewrite from scratch
> using modern Emacs features, and then things like this bug report would
> start working automatically.
> 
> How big a task would this be?  I mean, of course we'd keep compatibility
> (the defcustom language is fine), but just rewrite the UI `M-x
> customize' bits...  Hm...

I can't speak to this, sorry.  I'll just mention that I think:
(1) it could be a large undertaking and (2) there is existing
code out there that depends on or enhances cus*.el and wid*.el
code.

Personally, I would prefer that we live with it, and try to
improve it incrementally, rather than opting for a rewrite or
replacement.  I'd like to see a resident Emacs expert on wid*
and cus*, rather than someone who doesn't master it just trying
for a replacement.

(Yes, I'm conservative wrt this stuff.  Emacs is one of the
few things I'm conservative about. ;-))

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 2292 bytes --]

I took a look at this bug and it turned out to be really tricky.  So I did
all the research I could, to try to find a good fix.

Because the widget-button-click code was written before the addition of
mouse-1-click-follows-link, there is some work to be done to adapt it.  Even
though there were some attempts [1], they weren't enough.

The attached patch tries to fix this bug, while trying to not change too
much
of the code of the widget library.  It introduces a keymap for the link
widgets,
and only binds mouse-2 events.  Since widget-button-click isn't changed, it
has to bind down-mouse-2 too.  In this fix, I also have been careful to not
reintroduce bugs that have come up by changing the keybindings in the
widget-keymap, such as here:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23571

I investigated the emacs sources, and have noticed that this patch could
cause
problems with three pieces of code:

1) recentf.el: Because of Bug#22434, puts a nil :follow-link property to the
link widget.  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=22434
The patch would make that mouse-1 isn't translated to mouse-2, but since
this
was a workaround, changing this code would be OK, I think.  Without
overriding :follow-link, I have checked that recentf.el works like it
should,
with this patch applied.

2) epa.el: Typing M-x epa-encrypt-file brings up a *Keys* buffer.  This
buffer
has clickable text: [Cancel] and [OK].  The epa code create this as a link
widget, and when mouse-1-click-follows-link is nil, this text becomes not
responsive with mouse-1.  I'd say this are more buttons than links,
so a fix for it (if it is considered it needs fixing) would be to turn them
into
push-button widgets.

3) mh-mime.el: I have never used MH-E.  But there is some code in mh-mime.el
that creates a link widget, with an action mh-widget-press-button, and it
looks like it has a keymap of its own.  I don't know if the behavior could
change here.

I haven't found any other code in the Emacs sources that could have
problems.
And fixing this would make code that depends on the link widgets to behave
as
expected, when mouse-1-click-follows-link is customized to a nil value.  So
I
think it would be good to fix this.

[1] https://lists.gnu.org/archive/html/emacs-devel/2008-03/msg01312.html

[-- Attachment #1.2: Type: text/html, Size: 2734 bytes --]

[-- Attachment #2: 0001-Make-link-widgets-obey-mouse-1-click-follows-link.patch --]
[-- Type: text/x-patch, Size: 1906 bytes --]

From d40f8a1fa30fcc67444b6afffd8149d9e9b9af1d Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Tue, 20 Aug 2019 20:32:41 -0300
Subject: [PATCH] Make link widgets obey mouse-1-click-follows-link

* lisp/wid-edit.el (widget-link-keymap): New variable, a keymap to use
inside a link widget.
('link widget): Restore the :follow-link property and add widget-link-keymap
as the :keymap property.  (Bug#15682)
---
 lisp/wid-edit.el | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index dd03a24..1ddc461 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -1790,17 +1790,22 @@ widget-link-suffix
   :type 'string
   :group 'widget-button)
 
+(defvar widget-link-keymap
+  (let ((map (copy-keymap widget-keymap)))
+    ;; Only bind mouse-2, since mouse-1 will be translated accordingly to
+    ;; the customization of `mouse-1-click-follows-link'.
+    (define-key map [down-mouse-1] (lookup-key widget-global-map [down-mouse-1]))
+    (define-key map [down-mouse-2] 'widget-button-click)
+    (define-key map [mouse-2] 'widget-button-click)
+    map)
+  "Keymap used inside a link widget.")
+
 (define-widget 'link 'item
   "An embedded link."
   :button-prefix 'widget-link-prefix
   :button-suffix 'widget-link-suffix
-  ;; The `follow-link' property should only be used in those contexts where the
-  ;; mouse-1 event normally doesn't follow the link, yet the `link' widget
-  ;; seems to almost always be used in contexts where (down-)mouse-1 is bound
-  ;; to `widget-button-click' and hence the "mouse-1 to mouse-2" remapping is
-  ;; not necessary (and can even be harmful).  So let's not add a :follow-link
-  ;; by default.  See (bug#22434).
-  ;; :follow-link 'mouse-face
+  :follow-link 'mouse-face
+  :keymap widget-link-keymap
   :help-echo "Follow the link."
   :format "%[%t%]")
 
-- 
2.7.4

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> The attached patch tries to fix this bug, while trying to not change
> too much of the code of the widget library.  It introduces a keymap
> for the link widgets, and only binds mouse-2 events.  Since
> widget-button-click isn't changed, it has to bind down-mouse-2 too.
> In this fix, I also have been careful to not reintroduce bugs that
> have come up by changing the keybindings in the widget-keymap, such as
> here: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23571

I think this makes sense.

> I investigated the emacs sources, and have noticed that this patch could cause
> problems with three pieces of code:

Would it be possible for you to come up with patches for fixing these
three pieces, too?  And then we could apply all four patches at the same
time and not break anything.

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

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Mauro Aranda]

[-- Attachment #1.1: Type: text/plain, Size: 673 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> I investigated the emacs sources, and have noticed that this patch could
cause
>> problems with three pieces of code:
>
> Would it be possible for you to come up with patches for fixing these
> three pieces, too?  And then we could apply all four patches at the same
> time and not break anything.

Sure.  I attach 3 patches.  The same one I attached in my previous
message, one patch for recentf.el and one patch for epa.el.

I tried MH-E, to test the mh-mime.el code, and doesn't seem to misbehave
with the change in wid-edit.el, so I didn't touch it.

Best regards,
Mauro.

[-- Attachment #1.2: Type: text/html, Size: 882 bytes --]

[-- Attachment #2: 0001-Make-link-widgets-obey-mouse-1-click-follows-link.patch --]
[-- Type: text/x-patch, Size: 1906 bytes --]

From 583cde65b0377e56c7186354571cce226e827cc5 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Tue, 20 Aug 2019 20:32:41 -0300
Subject: [PATCH] Make link widgets obey mouse-1-click-follows-link

* lisp/wid-edit.el (widget-link-keymap): New variable, a keymap to use
inside a link widget.
('link widget): Restore the :follow-link property and add
widget-link-keymap as the :keymap property.  (Bug#15682)
---
 lisp/wid-edit.el | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index dd03a24..1ddc461 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -1790,17 +1790,22 @@ widget-link-suffix
   :type 'string
   :group 'widget-button)
 
+(defvar widget-link-keymap
+  (let ((map (copy-keymap widget-keymap)))
+    ;; Only bind mouse-2, since mouse-1 will be translated accordingly to
+    ;; the customization of `mouse-1-click-follows-link'.
+    (define-key map [down-mouse-1] (lookup-key widget-global-map [down-mouse-1]))
+    (define-key map [down-mouse-2] 'widget-button-click)
+    (define-key map [mouse-2] 'widget-button-click)
+    map)
+  "Keymap used inside a link widget.")
+
 (define-widget 'link 'item
   "An embedded link."
   :button-prefix 'widget-link-prefix
   :button-suffix 'widget-link-suffix
-  ;; The `follow-link' property should only be used in those contexts where the
-  ;; mouse-1 event normally doesn't follow the link, yet the `link' widget
-  ;; seems to almost always be used in contexts where (down-)mouse-1 is bound
-  ;; to `widget-button-click' and hence the "mouse-1 to mouse-2" remapping is
-  ;; not necessary (and can even be harmful).  So let's not add a :follow-link
-  ;; by default.  See (bug#22434).
-  ;; :follow-link 'mouse-face
+  :follow-link 'mouse-face
+  :keymap widget-link-keymap
   :help-echo "Follow the link."
   :format "%[%t%]")
 
-- 
2.7.4


[-- Attachment #3: 0001-Adapt-recentf.el-to-the-change-in-the-Widget-Library.patch --]
[-- Type: text/x-patch, Size: 962 bytes --]

From 98a4f24b0bc85c5bc3f749a13b51b8de35720c2f Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 25 Aug 2019 11:37:16 -0300
Subject: [PATCH] Adapt recentf.el to the change in the Widget Library

* lisp/recentf.el (recentf-open-files-item): Stop overriding
:follow-link property of the link widgets, since now it should work as
expected.
---
 lisp/recentf.el | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lisp/recentf.el b/lisp/recentf.el
index 4112b44..2720286 100644
--- a/lisp/recentf.el
+++ b/lisp/recentf.el
@@ -1184,9 +1184,6 @@ recentf-open-files-item
            :format "%[%t\n%]"
            :help-echo ,(concat "Open " (cdr menu-element))
            :action recentf-open-files-action
-           ;; Override the (problematic) follow-link property of the
-           ;; `link' widget (bug#22434).
-           :follow-link nil
            ,(cdr menu-element))))
 
 (defun recentf-open-files-items (files)
-- 
2.7.4


[-- Attachment #4: 0001-Create-push-button-widgets-instead-of-links-in-epa-K.patch --]
[-- Type: text/x-patch, Size: 1102 bytes --]

From 4cc06f7ec6e029ac5cc31d1dc44e5f950957441e Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 25 Aug 2019 11:33:54 -0300
Subject: [PATCH] Create push-button widgets instead of links in epa *Keys*
 buffer

* lisp/epa.el (epa--select-keys): OK and Cancel should be buttons.
---
 lisp/epa.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/epa.el b/lisp/epa.el
index 9e6edf4..b55a55f 100644
--- a/lisp/epa.el
+++ b/lisp/epa.el
@@ -440,12 +440,12 @@ epa--select-keys
 	      (substitute-command-keys "\
 - `\\[epa-mark-key]' to mark a key on the line
 - `\\[epa-unmark-key]' to unmark a key on the line\n"))
-      (widget-create 'link
+      (widget-create 'push-button
 		     :notify (lambda (&rest _ignore) (abort-recursive-edit))
 		     :help-echo
 		     "Click here or \\[abort-recursive-edit] to cancel"
 		     "Cancel")
-      (widget-create 'link
+      (widget-create 'push-button
 		     :notify (lambda (&rest _ignore) (exit-recursive-edit))
 		     :help-echo
 		     "Click here or \\[exit-recursive-edit] to finish"
-- 
2.7.4

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Drew Adams]

What happened to the bug # in the Subject line?
Please restore it.

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Drew Adams]

> What happened to the bug # in the Subject line?
> Please restore it.

Hm. I see it got restored by my reply.
Why was it missing from Mauro's and Lars's
mails?  E.g.:

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Sent: Saturday, August 24, 2019 10:47 PM
> To: Mauro Aranda <maurooaranda@gmail.com>
> Cc: 15682@debbugs.gnu.org; Drew Adams <drew.adams@oracle.com>
> Subject: Re: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-
> click-follows-link'

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Eli Zaretskii]

> Date: Sun, 25 Aug 2019 09:22:54 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 15682@debbugs.gnu.org
> 
> > What happened to the bug # in the Subject line?
> > Please restore it.
> 
> Hm. I see it got restored by my reply.
> Why was it missing from Mauro's and Lars's
> mails?  E.g.:
> 
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Sent: Saturday, August 24, 2019 10:47 PM
> > To: Mauro Aranda <maurooaranda@gmail.com>
> > Cc: 15682@debbugs.gnu.org; Drew Adams <drew.adams@oracle.com>
> > Subject: Re: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-
> > click-follows-link'

You need to get a better email client.

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Noam Postavsky]

Drew Adams <drew.adams@oracle.com> writes:

>> What happened to the bug # in the Subject line?
>> Please restore it.
>
> Hm. I see it got restored by my reply.
> Why was it missing from Mauro's and Lars's
> mails?  E.g.:

I guess Mauro just copied the subject from
https://debbugs.gnu.org/15682#5, which doesn't include the bug number
(because it shows mail as it was originally sent, before the bug number
was assigned).

When someone sends a message without the bug number to a
nnnnn@debbugs.gnu.org address, the bug list mail server adds the bug
number back; but if you're on Cc, then you'll get the message directly
from the sender (i.e., not from the server, and so you won't see the
number added back in.  I think you would *also* get the message from the
server with the bug number, but depending on your mail setup, it might
get hidden/deleted as a duplicate).

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Mauro Aranda]

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

Noam Postavsky <npostavs@gmail.com> writes:

> Drew Adams <drew.adams@oracle.com> writes:
>
>>> What happened to the bug # in the Subject line?
>>> Please restore it.
>>
>> Hm. I see it got restored by my reply.
>> Why was it missing from Mauro's and Lars's
>> mails?  E.g.:
>
> I guess Mauro just copied the subject from
> https://debbugs.gnu.org/15682#5, which doesn't include the bug number
> (because it shows mail as it was originally sent, before the bug number
> was assigned).
>

That might be what happened.

Sorry for the inconvenience.

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

bug#15682: 24.3.50; `:link' in `defgroup' does not respect `mouse-1-click-follows-link'

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Sure.  I attach 3 patches.  The same one I attached in my previous
> message, one patch for recentf.el and one patch for epa.el.
>
> I tried MH-E, to test the mh-mime.el code, and doesn't seem to misbehave
> with the change in wid-edit.el, so I didn't touch it.

Thanks; I've now applied all three patches to the Emacs trunk.

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