Re: master b02c9bc: Improve documentation of new Xref options

Re: master b02c9bc: Improve documentation of new Xref options

[Dmitry Gutov]

Thanks for the updates, some comments:

On 07.09.2021 16:04, Eli Zaretskii wrote:

>   @findex project-search
> -  @kbd{M-x project-search} is an interactive variant of
> +  @kbd{M-x project-search} is an incremental variant of

Is it really incremental? Maybe call it "iterative".

Compared to isearch (which is "incremental search"), we are not allowed 
to change the search string on-the-fly, for example.

> -@c Sadly, the new-and-improved Xref feature doesn't provide anything
> -@c close to the described below features of the now-obsoleted
> -@c tags-apropos.  I'm leaving this here to encourage enhancements to
> -@c xref.el.
> +@c Sadly, the new-and-improved Xref feature doesn't provide some
> +@c of the features of the now-obsoleted tags-apropos.  I'm leaving
> +@c this here to encourage enhancements to xref.el.

Is that about the display of tag file names in the apropos output buffer?

TBH I'm only reading this paragraph now. A bug report with a fuller 
explanation might help (no promises, though, sorry).

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2309,13 +2309,18 @@ before navigating to the selected location.
>   +++
>   *** New user options to automatically show the first Xref match.
>   The new user option 'xref-auto-jump-to-first-definition' controls the
> -behavior of 'xref-find-definitions' and related commands: if it's t or
> -'show', the first match is automatically displayed; if it's 'move',
> -point in the "*xref*" buffer is automatically moved to the first match
> -without displaying it.
> -The new user option 'xref-auto-jump-to-first-xref' changes the behavior of
> -all Xref commands in the same way as 'xref-auto-jump-to-first-definition'
> -affects the "find-definitions" commands.
> +behavior of 'xref-find-definitions' and related commands, like

Maybe "similar" rather than related? The point is that those commands 
use the same UI (to show, sometimes, very different information), rather 
than that are united by subject matter.

> +'xref-find-definitions-other-windo': if it's t or 'show', the first

Typo: windo -> window

> +match is automatically displayed; if it's 'move', point in the
> +"*xref*" buffer is automatically moved to the first match without
> +displaying it.
> +The new user option 'xref-auto-jump-to-first-xref' changes the
> +behavior of Xref commands such as 'xref-find-references',
> +'xref-find-apropos', and 'project-find-regexp', which are expected to
> +display many match that the user would like to
> +visit. 'xref-auto-jump-to-first-xref' changes their behavior muuch in

Typo: muuch

> +the same way as 'xref-auto-jump-to-first-definition' affects the
> +"find-definitions" commands.
>   
>   *** New user options 'xref-search-program' and 'xref-search-program-alist'.
>   So far 'grep' and 'ripgrep' are supported.  'ripgrep' seems to offer better
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index d022baaf..f6d88e1 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -420,7 +420,9 @@ elements is negated: these commands will NOT prompt."
>     "If t, `xref-find-definitions' always jumps to the first result.
>   `show' means to show the first result's location, but keep the
>   focus on the Xref buffer's window.
> -`move' means to only move point to the first result."
> +`move' means to only move point to the first result.
> +This variable also affects commands similar to `xref-find-definitions',
> +such as `xref-find-definitions-other-window'."

I would call them "variations", rather than separate commands, probably.

This is in contrast to xref-find-references and xref-find-apropos 
actually being similar while being different commands.

>     :type '(choice (const :tag "Jump" t)
>                    (const :tag "Show" show)
>                    (const :tag "Move point only" move)
> @@ -429,13 +431,16 @@ focus on the Xref buffer's window.
>     :package-version '(xref . "1.2.0"))
>   
>   (defcustom xref-auto-jump-to-first-xref nil
> -  "If t, xref commands always jump to the first result.
> +  "If t, `xref-find-references' always jumps to the first result.
>   `show' means to show the first result's location, but keep the
>   focus on the Xref buffer's window.
>   `move' means to only move point to the first result.
> +This variable also affects commands similar to `xref-find-definitions',

similar to `xref-find-references'

Here you call them "similar", like I suggest above. That seems like a 
good choice.

Thanks.

Re: master b02c9bc: Improve documentation of new Xref options

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 7 Sep 2021 18:21:24 +0300
> 
> >   @findex project-search
> > -  @kbd{M-x project-search} is an interactive variant of
> > +  @kbd{M-x project-search} is an incremental variant of
> 
> Is it really incremental? Maybe call it "iterative".

Is "sequential" better?  "Iterative" sounds too "mathematical".

> Compared to isearch (which is "incremental search"), we are not allowed 
> to change the search string on-the-fly, for example.

Well, I meant incremental in the sense that we do it one match at a
time.

> > -@c Sadly, the new-and-improved Xref feature doesn't provide anything
> > -@c close to the described below features of the now-obsoleted
> > -@c tags-apropos.  I'm leaving this here to encourage enhancements to
> > -@c xref.el.
> > +@c Sadly, the new-and-improved Xref feature doesn't provide some
> > +@c of the features of the now-obsoleted tags-apropos.  I'm leaving
> > +@c this here to encourage enhancements to xref.el.
> 
> Is that about the display of tag file names in the apropos output buffer?

No, it's about the features listed after the shown hunk.  I just made
its language less extreme, because xref-find-apropos does exist.

> >   The new user option 'xref-auto-jump-to-first-definition' controls the
> > -behavior of 'xref-find-definitions' and related commands: if it's t or
> > -'show', the first match is automatically displayed; if it's 'move',
> > -point in the "*xref*" buffer is automatically moved to the first match
> > -without displaying it.
> > -The new user option 'xref-auto-jump-to-first-xref' changes the behavior of
> > -all Xref commands in the same way as 'xref-auto-jump-to-first-definition'
> > -affects the "find-definitions" commands.
> > +behavior of 'xref-find-definitions' and related commands, like
> 
> Maybe "similar" rather than related? The point is that those commands 
> use the same UI (to show, sometimes, very different information), rather 
> than that are united by subject matter.

I went with "variants", okay?

Re: master b02c9bc: Improve documentation of new Xref options

[Dmitry Gutov]

On 07.09.2021 18:45, Eli Zaretskii wrote:
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 7 Sep 2021 18:21:24 +0300
>>
>>>    @findex project-search
>>> -  @kbd{M-x project-search} is an interactive variant of
>>> +  @kbd{M-x project-search} is an incremental variant of
>>
>> Is it really incremental? Maybe call it "iterative".
> 
> Is "sequential" better?  "Iterative" sounds too "mathematical".

Yes, I think it's a fine choice.

>>> -@c Sadly, the new-and-improved Xref feature doesn't provide anything
>>> -@c close to the described below features of the now-obsoleted
>>> -@c tags-apropos.  I'm leaving this here to encourage enhancements to
>>> -@c xref.el.
>>> +@c Sadly, the new-and-improved Xref feature doesn't provide some
>>> +@c of the features of the now-obsoleted tags-apropos.  I'm leaving
>>> +@c this here to encourage enhancements to xref.el.
>>
>> Is that about the display of tag file names in the apropos output buffer?
> 
> No, it's about the features listed after the shown hunk.  I just made
> its language less extreme, because xref-find-apropos does exist.

tags-apropos-additional-actions, then? That seems easier to support.

>>>    The new user option 'xref-auto-jump-to-first-definition' controls the
>>> -behavior of 'xref-find-definitions' and related commands: if it's t or
>>> -'show', the first match is automatically displayed; if it's 'move',
>>> -point in the "*xref*" buffer is automatically moved to the first match
>>> -without displaying it.
>>> -The new user option 'xref-auto-jump-to-first-xref' changes the behavior of
>>> -all Xref commands in the same way as 'xref-auto-jump-to-first-definition'
>>> -affects the "find-definitions" commands.
>>> +behavior of 'xref-find-definitions' and related commands, like
>>
>> Maybe "similar" rather than related? The point is that those commands
>> use the same UI (to show, sometimes, very different information), rather
>> than that are united by subject matter.
> 
> I went with "variants", okay?

Thanks.

Re: master b02c9bc: Improve documentation of new Xref options

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 7 Sep 2021 19:06:29 +0300
> 
> >> Is it really incremental? Maybe call it "iterative".
> > 
> > Is "sequential" better?  "Iterative" sounds too "mathematical".
> 
> Yes, I think it's a fine choice.

Done.

> > No, it's about the features listed after the shown hunk.  I just made
> > its language less extreme, because xref-find-apropos does exist.
> 
> tags-apropos-additional-actions, then? That seems easier to support.

Yes, pretty much.

Re: master b02c9bc: Improve documentation of new Xref options

[Dmitry Gutov]

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

On 07.09.2021 19:23, Eli Zaretskii wrote:
>>> No, it's about the features listed after the shown hunk.  I just made
>>> its language less extreme, because xref-find-apropos does exist.
>> tags-apropos-additional-actions, then? That seems easier to support.
> Yes, pretty much.

Here's a quick implementation.

But the feature doesn't seem to be particularly popular/polished: the 
example value for Elisp, for instance, sets up search across all symbols 
(obarray), but subsequent navigation only works for commands, and only 
ones documented in the manual.

[-- Attachment #2: xref-emacs-additional-apropos.diff --]
[-- Type: text/x-patch, Size: 2797 bytes --]

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index a1f806ae8c..7efa88546d 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -2096,7 +2096,10 @@ xref-backend-definitions
     definitions))
 
 (cl-defmethod xref-backend-apropos ((_backend (eql 'etags)) pattern)
-  (etags--xref-find-definitions (xref-apropos-regexp pattern) t))
+  (let ((regexp (xref-apropos-regexp pattern)))
+    (nconc
+     (etags--xref-find-definitions regexp t)
+     (etags--xref-apropos-additional regexp))))
 
 (defun etags--xref-find-definitions (pattern &optional regexp?)
   ;; This emulates the behavior of `find-tag-in-order' but instead of
@@ -2131,6 +2134,32 @@ etags--xref-find-definitions
                       (puthash mark-key t marks))))))))))
     (nreverse xrefs)))
 
+(defun etags--xref-apropos-additional (regexp)
+  (cl-mapcan
+   (lambda (oba)
+     (pcase-let* ((`(,group ,goto-fun ,symbs) oba)
+                  (res nil)
+                  (add-xref (lambda (sym)
+                              (let ((sn (symbol-name sym)))
+                                (when (string-match-p regexp sn)
+                                  (push
+                                   (xref-make
+                                    sn
+                                    (xref-make-etags-apropos-location
+                                     sym goto-fun group))
+                                   res))))))
+       (when (symbolp symbs)
+         (if (boundp symbs)
+             (setq symbs (symbol-value symbs))
+           (warn "symbol `%s' has no value" symbs)
+           (setq symbs nil))
+         (if (vectorp symbs)
+             (mapatoms add-xref symbs)
+           (dolist (sy symbs)
+             (funcall add-xref (car sy))))
+         (nreverse res))))
+   tags-apropos-additional-actions))
+
 (defclass xref-etags-location (xref-location)
   ((tag-info :type list   :initarg :tag-info)
    (file     :type string :initarg :file
@@ -2155,6 +2184,25 @@ xref-location-line
   (with-slots (tag-info) l
     (nth 1 tag-info)))
 
+(defclass xref-etags-apropos-location (xref-location)
+  ((symbol :type symbol :initarg :symbol)
+   (goto-fun :type function :initarg :goto-fun)
+   (group :type string :initarg :group
+          :reader xref-location-group))
+  :documentation "Location of an additional apropos etags symbol.")
+
+(defun xref-make-etags-apropos-location (symbol goto-fun group)
+  (make-instance 'xref-etags-apropos-location
+                 :symbol symbol
+                 :goto-fun goto-fun
+                 :group group))
+
+(cl-defmethod xref-location-marker ((l xref-etags-apropos-location))
+  (save-window-excursion
+    (with-slots (goto-fun symbol) l
+      (funcall goto-fun symbol)
+      (point-marker))))
+
 \f
 (provide 'etags)
 

Re: master b02c9bc: Improve documentation of new Xref options

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 8 Sep 2021 03:24:12 +0300
> 
> On 07.09.2021 19:23, Eli Zaretskii wrote:
> >>> No, it's about the features listed after the shown hunk.  I just made
> >>> its language less extreme, because xref-find-apropos does exist.
> >> tags-apropos-additional-actions, then? That seems easier to support.
> > Yes, pretty much.
> 
> Here's a quick implementation.

Thanks.

> But the feature doesn't seem to be particularly popular/polished: the 
> example value for Elisp, for instance, sets up search across all symbols 
> (obarray), but subsequent navigation only works for commands, and only 
> ones documented in the manual.

I don't think we should be too bothered about that: once we have
replacements for all of those etags features, we could declare the
originals obsolete and point to replacements, something we cannot do
when the replacements are missing.  So I think we should install this.

>  (cl-defmethod xref-backend-apropos ((_backend (eql 'etags)) pattern)
> -  (etags--xref-find-definitions (xref-apropos-regexp pattern) t))
> +  (let ((regexp (xref-apropos-regexp pattern)))
> +    (nconc
> +     (etags--xref-find-definitions regexp t)
> +     (etags--xref-apropos-additional regexp))))

I'm not sure I understand why is this specific to the etags backend.
The spec seems to be more general, and xref-find-apropos is not
specific to etags, right?

We'd also need a defcustom, similar to tags-apropos-additional-actions.

Re: master b02c9bc: Improve documentation of new Xref options

[Dmitry Gutov]

On 08.09.2021 09:18, Eli Zaretskii wrote:

>> But the feature doesn't seem to be particularly popular/polished: the
>> example value for Elisp, for instance, sets up search across all symbols
>> (obarray), but subsequent navigation only works for commands, and only
>> ones documented in the manual.
> 
> I don't think we should be too bothered about that: once we have
> replacements for all of those etags features, we could declare the
> originals obsolete and point to replacements, something we cannot do
> when the replacements are missing.  So I think we should install this.

Fair enough.

I'd still like to see someone working on a better suggested default 
config for it, but it's not a blocker.

>>   (cl-defmethod xref-backend-apropos ((_backend (eql 'etags)) pattern)
>> -  (etags--xref-find-definitions (xref-apropos-regexp pattern) t))
>> +  (let ((regexp (xref-apropos-regexp pattern)))
>> +    (nconc
>> +     (etags--xref-find-definitions regexp t)
>> +     (etags--xref-apropos-additional regexp))))
> 
> I'm not sure I understand why is this specific to the etags backend.
> The spec seems to be more general, and xref-find-apropos is not
> specific to etags, right?

If as you say above it is for feature parity with 'M-x tags-apropos', 
then the previously sent patch should be enough.

We are honoring the etags-specific variable 
(tags-apropos-additional-actions), so the result should be specific to 
the etags backend.

> We'd also need a defcustom, similar to tags-apropos-additional-actions.

Could we extend it to be the feature of 'M-x xref-find-apropos' in 
general rather than the etags backend? It's possible, if we see specific 
user demand for it.

Here are considerations why I chose the other route:

- As I said, the existing etags feature is not very polished. To bring 
it into the Xref API proper, we'd have to redesign it somehow, and since 
the feature doesn't seem to scratch any of my personal itches, I need 
feature requests to narrow down the design space.

- It has been somewhat of a rule that backend methods don't change 
behavior based on some global user option, both in project.el and xref. 
Rather, each backend is more free to do its own thing and have 
backend-specific options for configuring. We can break this rule, of 
course, when presented with good reasons for it.

- It's hard for me to say whether third-party backends will want this 
behavior added to their backends unconditionally. But even when we're 
talking about the elisp backend... do we? Want it there?

- We don't need to make this choice now. It will be easy to install the 
already provided patch and then possibly extend the feature to all 
backends at some later point.

Re: master b02c9bc: Improve documentation of new Xref options

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 9 Sep 2021 04:22:48 +0300
> 
> >>   (cl-defmethod xref-backend-apropos ((_backend (eql 'etags)) pattern)
> >> -  (etags--xref-find-definitions (xref-apropos-regexp pattern) t))
> >> +  (let ((regexp (xref-apropos-regexp pattern)))
> >> +    (nconc
> >> +     (etags--xref-find-definitions regexp t)
> >> +     (etags--xref-apropos-additional regexp))))
> > 
> > I'm not sure I understand why is this specific to the etags backend.
> > The spec seems to be more general, and xref-find-apropos is not
> > specific to etags, right?
> 
> If as you say above it is for feature parity with 'M-x tags-apropos', 
> then the previously sent patch should be enough.

If you think it's enough, please go ahead and install it.

Thanks.

Re: master b02c9bc: Improve documentation of new Xref options

[Dmitry Gutov]

On 09.09.2021 09:26, Eli Zaretskii wrote:
>> If as you say above it is for feature parity with 'M-x tags-apropos',
>> then the previously sent patch should be enough.
> If you think it's enough, please go ahead and install it.

And done!

Re: master b02c9bc: Improve documentation of new Xref options

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 10 Sep 2021 03:18:58 +0300
> Cc: emacs-devel@gnu.org
> 
> On 09.09.2021 09:26, Eli Zaretskii wrote:
> >> If as you say above it is for feature parity with 'M-x tags-apropos',
> >> then the previously sent patch should be enough.
> > If you think it's enough, please go ahead and install it.
> 
> And done!

Thanks.