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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66752 in the body.
You can then email your comments to 66752 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Wed, 25 Oct 2023 15:33:17 -0700

[Message part 1 (text/plain, inline)]

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.

[0001-Hook-bug-reference-mode-up-to-thing-at-point.patch (text/plain, attachment)]

Message #8 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>, Tassilo Horn <tsdh <at> gnu.org>
Cc: 66752 <at> debbugs.gnu.org
Subject: Re: bug#66752: 30.0.50;
 [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'
Date: Sat, 04 Nov 2023 10:12:58 +0200

> Date: Wed, 25 Oct 2023 15:33:17 -0700
> From: Jim Porter <jporterbugs <at> 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.

Message #11 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <tsdh <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 66752 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sat, 04 Nov 2023 20:24:58 +0100

Jim Porter <jporterbugs <at> 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

Message #14 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: 66752 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sat, 4 Nov 2023 13:07:38 -0700

On 11/4/2023 12:24 PM, Tassilo Horn wrote:
> Jim Porter <jporterbugs <at> 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?

Message #17 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 66752 <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sun, 05 Nov 2023 07:31:30 +0200

> Date: Sat, 4 Nov 2023 13:07:38 -0700
> Cc: 66752 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 11/4/2023 12:24 PM, Tassilo Horn wrote:
> > Jim Porter <jporterbugs <at> 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.

Message #20 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 66752 <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sat, 4 Nov 2023 23:21:59 -0700

On 11/4/2023 10:31 PM, Eli Zaretskii wrote:
>> Date: Sat, 4 Nov 2023 13:07:38 -0700
>> Cc: 66752 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>> From: Jim Porter <jporterbugs <at> 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.

Message #23 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jim Porter <jporterbugs <at> gmail.com>, Eli Zaretskii <eliz <at> gnu.org>, 
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 66752 <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50;
 [PATCH] Add support for 'thing-at-point' to 'bug-reference-mode'
Date: Sun, 5 Nov 2023 06:19:06 -0800

Jim Porter <jporterbugs <at> 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).

Message #26 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 66752 <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sun, 05 Nov 2023 18:20:29 -0500

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

Message #29 received at 66752 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 66752 <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sun, 5 Nov 2023 20:55:19 -0800

[Message part 1 (text/plain, inline)]

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.

[0001-Hook-bug-reference-mode-up-to-thing-at-point.patch (text/plain, attachment)]

Message #34 received at 66752-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 66752-done <at> debbugs.gnu.org, tsdh <at> gnu.org
Subject: Re: bug#66752: 30.0.50; [PATCH] Add support for 'thing-at-point' to
 'bug-reference-mode'
Date: Sat, 11 Nov 2023 21:41:18 -0800

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.