bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Jim Porter]

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

Currently, "(thing-at-point 'url)" returns nil when point is over a bug 
reference. It would be nice to return the URL here. With this, it's 
easier to write a function that copies (or browses to) the URL at point 
without coding so many special cases.

Attached is a patch plus a regression test for this.

[-- Attachment #2: 0001-Hook-bug-reference-mode-up-to-thing-at-point.patch --]
[-- Type: text/plain, Size: 4911 bytes --]

From 7abe1293ce17a43261774f6111945444f8668606 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 25 Oct 2023 15:24:28 -0700
Subject: [PATCH] Hook 'bug-reference-mode' up to 'thing-at-point'

* lisp/progmodes/bug-reference.el (bug-reference--url-at-point): New
function.
(bug-reference-mode, bug-reference-prog-mode): Factor initialization
code out to...
(bug-reference--init): ... here.

* test/lisp/progmodes/bug-reference-tests.el (test-thing-at-point):
New test.

* etc/NEWS: Announce this change.
---
 etc/NEWS                                   |  5 +++
 lisp/progmodes/bug-reference.el            | 36 ++++++++++++++--------
 test/lisp/progmodes/bug-reference-tests.el | 15 +++++++++
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index f8d4a3c3efe..8dbba232004 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -951,6 +951,11 @@ For links in 'webjump-sites' without an explicit URI scheme, it was
 previously assumed that they should be prefixed with "http://".  Such
 URIs are now prefixed with "https://" instead.
 
+---
+*** 'bug-reference-mode' now supports 'thing-at-point'.
+Now, calling 'thing-at-point' when point is on a bug reference will
+return the URL for that bug.
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index bc280284588..e55b4dc1c68 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -654,17 +654,34 @@ bug-reference--run-auto-setup
         (run-hook-with-args-until-success
          'bug-reference-auto-setup-functions)))))
 
-;;;###autoload
-(define-minor-mode bug-reference-mode
-  "Toggle hyperlinking bug references in the buffer (Bug Reference mode)."
-  :after-hook (bug-reference--run-auto-setup)
-  (if bug-reference-mode
-      (jit-lock-register #'bug-reference-fontify)
+(defun bug-reference--url-at-point ()
+  (get-char-property (point) 'bug-reference-url))
+
+(defun bug-reference--init (enable)
+  (if enable
+      (progn
+        (jit-lock-register #'bug-reference-fontify)
+        (require 'thingatpt)
+        (setq-local thing-at-point-provider-alist
+                    (append thing-at-point-provider-alist
+                            '((url . bug-reference--url-at-point)))))
     (jit-lock-unregister #'bug-reference-fontify)
+    (setq thing-at-point-provider-alist
+          (delete '((url . bug-reference--url-at-point))
+                  thing-at-point-provider-alist))
+    (when (equal thing-at-point-provider-alist
+                 (default-value 'thing-at-point-provider-alist))
+      (kill-local-variable 'thing-at-point-provider-alist))
     (save-restriction
       (widen)
       (bug-reference-unfontify (point-min) (point-max)))))
 
+;;;###autoload
+(define-minor-mode bug-reference-mode
+  "Toggle hyperlinking bug references in the buffer (Bug Reference mode)."
+  :after-hook (bug-reference--run-auto-setup)
+  (bug-reference--init bug-reference-mode))
+
 (defun bug-reference-mode-force-auto-setup ()
   "Enable `bug-reference-mode' and force auto-setup.
 Enabling `bug-reference-mode' runs its auto-setup only if
@@ -681,12 +698,7 @@ bug-reference-mode-force-auto-setup
 (define-minor-mode bug-reference-prog-mode
   "Like `bug-reference-mode', but only buttonize in comments and strings."
   :after-hook (bug-reference--run-auto-setup)
-  (if bug-reference-prog-mode
-      (jit-lock-register #'bug-reference-fontify)
-    (jit-lock-unregister #'bug-reference-fontify)
-    (save-restriction
-      (widen)
-      (bug-reference-unfontify (point-min) (point-max)))))
+  (bug-reference--init bug-reference-prog-mode))
 
 (provide 'bug-reference)
 ;;; bug-reference.el ends here
diff --git a/test/lisp/progmodes/bug-reference-tests.el b/test/lisp/progmodes/bug-reference-tests.el
index 790582aed4c..e5b207748bf 100644
--- a/test/lisp/progmodes/bug-reference-tests.el
+++ b/test/lisp/progmodes/bug-reference-tests.el
@@ -25,6 +25,7 @@
 
 (require 'bug-reference)
 (require 'ert)
+(require 'ert-x)
 
 (defun test--get-github-entry (url)
   (and (string-match
@@ -125,4 +126,18 @@ test-gitea-entry
     (test--get-gitea-entry "https://gitea.com/magit/magit/")
     "magit/magit")))
 
+(ert-deftest test-thing-at-point ()
+  "Ensure that (thing-at-point 'url) returns the bug URL."
+  (ert-with-test-buffer (:name "thingatpt")
+    (setq-local bug-reference-url-format "https://debbugs.gnu.org/%s")
+    (insert "bug#1234")
+    (bug-reference-mode)
+    (jit-lock-fontify-now (point-min) (point-max))
+    (goto-char (point-min))
+    ;; Make sure we get the URL when `bug-reference-mode' is active...
+    (should (equal (thing-at-point 'url) "https://debbugs.gnu.org/1234"))
+    (bug-reference-mode -1)
+    ;; ... and get nil when `bug-reference-mode' is inactive.
+    (should-not (thing-at-point 'url))))
+
 ;;; bug-reference-tests.el ends here
-- 
2.25.1

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Eli Zaretskii]

> Date: Wed, 25 Oct 2023 15:33:17 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Currently, "(thing-at-point 'url)" returns nil when point is over a bug 
> reference. It would be nice to return the URL here. With this, it's 
> easier to write a function that copies (or browses to) the URL at point 
> without coding so many special cases.
> 
> Attached is a patch plus a regression test for this.

Tassilo, any comments or suggestions?

Thanks.

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Tassilo Horn]

Jim Porter <jporterbugs@gmail.com> writes:

Hi Jim & Eli,

> Currently, "(thing-at-point 'url)" returns nil when point is over a
> bug reference.  It would be nice to return the URL here.

Yes, why not.

> Attached is a patch plus a regression test for this.

The patch and test look good.  One minor nit below.

> +(defun bug-reference--url-at-point ()
> +  (get-char-property (point) 'bug-reference-url))
> +
> +(defun bug-reference--init (enable)
> +  (if enable
> +      (progn
> +        (jit-lock-register #'bug-reference-fontify)
> +        (require 'thingatpt)
> +        (setq-local thing-at-point-provider-alist
> +                    (append thing-at-point-provider-alist
> +                            '((url . bug-reference--url-at-point)))))

Quoting this way would be better:

                               `((url . ,#'bug-reference--url-at-point))

>      (jit-lock-unregister #'bug-reference-fontify)
> +    (setq thing-at-point-provider-alist
> +          (delete '((url . bug-reference--url-at-point))
> +                  thing-at-point-provider-alist))
> +    (when (equal thing-at-point-provider-alist
> +                 (default-value 'thing-at-point-provider-alist))
> +      (kill-local-variable 'thing-at-point-provider-alist))

Is that conventional, nuking a buffer-local value once it's back to its
default value.  Just asking out of curiosity.

Bye,
Tassilo

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Jim Porter]

On 11/4/2023 12:24 PM, Tassilo Horn wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
>>       (jit-lock-unregister #'bug-reference-fontify)
>> +    (setq thing-at-point-provider-alist
>> +          (delete '((url . bug-reference--url-at-point))
>> +                  thing-at-point-provider-alist))
>> +    (when (equal thing-at-point-provider-alist
>> +                 (default-value 'thing-at-point-provider-alist))
>> +      (kill-local-variable 'thing-at-point-provider-alist))
> 
> Is that conventional, nuking a buffer-local value once it's back to its
> default value.  Just asking out of curiosity.

I'm not sure. The only example I saw for adding to 
'thing-at-provider-alist' was for EWW (a major mode). I thought, "What 
if someone activated and deactivated 'bug-reference-mode' and then later 
added something to the default value of 'thing-at-provider-alist'?"

I don't know if that's a case we want to support; maybe I'm just being 
overly cautious. (Or maybe there should be - or already is - some 
utility function that does this for us.) Eli, do you have any thoughts 
on this part?

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Eli Zaretskii]

> Date: Sat, 4 Nov 2023 13:07:38 -0700
> Cc: 66752@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 11/4/2023 12:24 PM, Tassilo Horn wrote:
> > Jim Porter <jporterbugs@gmail.com> writes:
> >>       (jit-lock-unregister #'bug-reference-fontify)
> >> +    (setq thing-at-point-provider-alist
> >> +          (delete '((url . bug-reference--url-at-point))
> >> +                  thing-at-point-provider-alist))
> >> +    (when (equal thing-at-point-provider-alist
> >> +                 (default-value 'thing-at-point-provider-alist))
> >> +      (kill-local-variable 'thing-at-point-provider-alist))
> > 
> > Is that conventional, nuking a buffer-local value once it's back to its
> > default value.  Just asking out of curiosity.
> 
> I'm not sure. The only example I saw for adding to 
> 'thing-at-provider-alist' was for EWW (a major mode). I thought, "What 
> if someone activated and deactivated 'bug-reference-mode' and then later 
> added something to the default value of 'thing-at-provider-alist'?"
> 
> I don't know if that's a case we want to support; maybe I'm just being 
> overly cautious. (Or maybe there should be - or already is - some 
> utility function that does this for us.) Eli, do you have any thoughts 
> on this part?

I don't see any problems with this.  But I added Stefan in case he has
an opinion.

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Jim Porter]

On 11/4/2023 10:31 PM, Eli Zaretskii wrote:
>> Date: Sat, 4 Nov 2023 13:07:38 -0700
>> Cc: 66752@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> I don't know if that's a case we want to support; maybe I'm just being
>> overly cautious. (Or maybe there should be - or already is - some
>> utility function that does this for us.) Eli, do you have any thoughts
>> on this part?
> 
> I don't see any problems with this.  But I added Stefan in case he has
> an opinion.

Another implementation option might be:

1. When activating 'bug-reference-mode', only add 
'bug-reference--url-at-point' to 'thing-at-point-provider-alist' if it's 
not already there,

2. Do nothing when deactivating 'bug-reference-mode', and

3. Inside bug-reference--url-at-point' always return nil if 
'bug-reference-mode' is inactive.

Though I suppose this would use 'cl-pushnew' for (1), which might be 
inadvisable given the recent discussion on emacs-devel. (Or we could use 
'add-to-list', but its docstring says "please do not abuse it in Elisp 
code, where you are usually better off using ‘push’ or ‘cl-pushnew’.")

I don't have a very strong opinion on this part, so I'll do whatever 
makes everyone happy here.

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Stefan Kangas]

Jim Porter <jporterbugs@gmail.com> writes:

> Though I suppose this would use 'cl-pushnew' for (1), which might be
> inadvisable given the recent discussion on emacs-devel. (Or we could use
> 'add-to-list', but its docstring says "please do not abuse it in Elisp
> code, where you are usually better off using ‘push’ or ‘cl-pushnew’.")
>
> I don't have a very strong opinion on this part, so I'll do whatever
> makes everyone happy here.

It is perfectly fine to (require 'cl-lib).

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I'm not sure. The only example I saw for adding to
>> 'thing-at-provider-alist' was for EWW (a major mode).  I thought, "What
>> if someone activated and deactivated 'bug-reference-mode' and then later
>> added something to the default value of 'thing-at-provider-alist'?"

It's a general problem we have in Emacs, indeed.  We have solutions for
it for the specific case of hooks (with `add/remove-hook` and their
`local` argument), and for variables holding functions (with
`add-function`), but not for anything else :-(

I've played with the idea of adding a new set of functions to help
major&minor modes set variables in a more declarative way, so that any
sequence of enabling/disabling or major/minor modes would still result
in "the right" value, and I even have some basic PoC code but nothing
good enough for `master`.

The basic idea would be something like:

   (changevar-add VAR FUNCTION &optional ID LOCAL PRIO)
   (changevar-remove VAR FUNCTION-OR-ID &optional LOCAL)

where `changevar-add` conceptually does something akin to

    (set VAR (funcall FUNCTION (symbol-value VAR)))

and you use `changevar-remove` to set it back to its "previous" value.

But as it stands, you're trying to solve a problem in your code which
affects pretty much every other minor mode out there, so I think it's OK
to just disregard the problem, like everyone else has done until now.
Maybe you can even get away with modifying the variable globally and
not removing the entry when the mode is disabled.


        Stefan

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Jim Porter]

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

On 11/5/2023 3:20 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> But as it stands, you're trying to solve a problem in your code which
> affects pretty much every other minor mode out there, so I think it's OK
> to just disregard the problem, like everyone else has done until now.
> Maybe you can even get away with modifying the variable globally and
> not removing the entry when the mode is disabled.

Ok, how about we just go with this then? (Removing the 
'kill-local-variable' call.) That's simple, and reasonably close to The 
Right Thing...

If no one has any objections, I'll merge this in the next few days.

[-- Attachment #2: 0001-Hook-bug-reference-mode-up-to-thing-at-point.patch --]
[-- Type: text/plain, Size: 4881 bytes --]

From 7b850af19e73b7a72d12f96587e4b703f8862ec4 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 25 Oct 2023 15:24:28 -0700
Subject: [PATCH] Hook 'bug-reference-mode' up to 'thing-at-point'

* lisp/progmodes/bug-reference.el (bug-reference--url-at-point): New
function.
(bug-reference-mode, bug-reference-prog-mode): Factor initialization
code out to...
(bug-reference--init): ... here.

* test/lisp/progmodes/bug-reference-tests.el (test-thing-at-point):
New test.

* etc/NEWS: Announce this change.
---
 etc/NEWS                                   |  5 ++++
 lisp/progmodes/bug-reference.el            | 34 ++++++++++++++--------
 test/lisp/progmodes/bug-reference-tests.el | 15 ++++++++++
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 94bcb75835b..a7f8eca3e1c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -978,6 +978,11 @@ For links in 'webjump-sites' without an explicit URI scheme, it was
 previously assumed that they should be prefixed with "http://".  Such
 URIs are now prefixed with "https://" instead.
 
+---
+*** 'bug-reference-mode' now supports 'thing-at-point'.
+Now, calling '(thing-at-point 'url)' when point is on a bug reference
+will return the URL for that bug.
+
 ** Customize
 
 +++
diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index bc280284588..a76f78ad69d 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -35,6 +35,8 @@
 
 ;;; Code:
 
+(require 'thingatpt)
+
 (defgroup bug-reference nil
   "Hyperlinking references to bug reports."
   ;; Somewhat arbitrary, by analogy with eg goto-address.
@@ -654,17 +656,30 @@ bug-reference--run-auto-setup
         (run-hook-with-args-until-success
          'bug-reference-auto-setup-functions)))))
 
-;;;###autoload
-(define-minor-mode bug-reference-mode
-  "Toggle hyperlinking bug references in the buffer (Bug Reference mode)."
-  :after-hook (bug-reference--run-auto-setup)
-  (if bug-reference-mode
-      (jit-lock-register #'bug-reference-fontify)
+(defun bug-reference--url-at-point ()
+  (get-char-property (point) 'bug-reference-url))
+
+(defun bug-reference--init (enable)
+  (if enable
+      (progn
+        (jit-lock-register #'bug-reference-fontify)
+        (setq-local thing-at-point-provider-alist
+                    (append thing-at-point-provider-alist
+                            '((url . bug-reference--url-at-point)))))
     (jit-lock-unregister #'bug-reference-fontify)
+    (setq thing-at-point-provider-alist
+          (delete '((url . bug-reference--url-at-point))
+                  thing-at-point-provider-alist))
     (save-restriction
       (widen)
       (bug-reference-unfontify (point-min) (point-max)))))
 
+;;;###autoload
+(define-minor-mode bug-reference-mode
+  "Toggle hyperlinking bug references in the buffer (Bug Reference mode)."
+  :after-hook (bug-reference--run-auto-setup)
+  (bug-reference--init bug-reference-mode))
+
 (defun bug-reference-mode-force-auto-setup ()
   "Enable `bug-reference-mode' and force auto-setup.
 Enabling `bug-reference-mode' runs its auto-setup only if
@@ -681,12 +696,7 @@ bug-reference-mode-force-auto-setup
 (define-minor-mode bug-reference-prog-mode
   "Like `bug-reference-mode', but only buttonize in comments and strings."
   :after-hook (bug-reference--run-auto-setup)
-  (if bug-reference-prog-mode
-      (jit-lock-register #'bug-reference-fontify)
-    (jit-lock-unregister #'bug-reference-fontify)
-    (save-restriction
-      (widen)
-      (bug-reference-unfontify (point-min) (point-max)))))
+  (bug-reference--init bug-reference-prog-mode))
 
 (provide 'bug-reference)
 ;;; bug-reference.el ends here
diff --git a/test/lisp/progmodes/bug-reference-tests.el b/test/lisp/progmodes/bug-reference-tests.el
index 790582aed4c..e5b207748bf 100644
--- a/test/lisp/progmodes/bug-reference-tests.el
+++ b/test/lisp/progmodes/bug-reference-tests.el
@@ -25,6 +25,7 @@
 
 (require 'bug-reference)
 (require 'ert)
+(require 'ert-x)
 
 (defun test--get-github-entry (url)
   (and (string-match
@@ -125,4 +126,18 @@ test-gitea-entry
     (test--get-gitea-entry "https://gitea.com/magit/magit/")
     "magit/magit")))
 
+(ert-deftest test-thing-at-point ()
+  "Ensure that (thing-at-point 'url) returns the bug URL."
+  (ert-with-test-buffer (:name "thingatpt")
+    (setq-local bug-reference-url-format "https://debbugs.gnu.org/%s")
+    (insert "bug#1234")
+    (bug-reference-mode)
+    (jit-lock-fontify-now (point-min) (point-max))
+    (goto-char (point-min))
+    ;; Make sure we get the URL when `bug-reference-mode' is active...
+    (should (equal (thing-at-point 'url) "https://debbugs.gnu.org/1234"))
+    (bug-reference-mode -1)
+    ;; ... and get nil when `bug-reference-mode' is inactive.
+    (should-not (thing-at-point 'url))))
+
 ;;; bug-reference-tests.el ends here
-- 
2.25.1

bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'

[Jim Porter]

On 11/5/2023 8:55 PM, Jim Porter wrote:
> If no one has any objections, I'll merge this in the next few days.

Ok, now merged as e5ba52ad72d to master. Closing this bug.