bug#35702: xref revert-buffer

bug#35702: xref revert-buffer

[Juri Linkov]

I tried to use project-find-regexp more often than rgrep,
but xref misses an important feature: typing 'g' in the
*xref* buffer fails with this error:

Debugger entered--Lisp error: (error "Buffer does not seem to be associated with any file")
  signal(error ("Buffer does not seem to be associated with any file"))
  error("Buffer does not seem to be associated with any file")
  revert-buffer--default(t nil)
  revert-buffer(t)
  funcall-interactively(revert-buffer t)
  call-interactively(revert-buffer nil nil)
  command-execute(revert-buffer)

bug#35702: xref revert-buffer

[Dmitry Gutov]

Version: 27.1

On 12.05.2019 22:45, Juri Linkov wrote:
> I tried to use project-find-regexp more often than rgrep,
> but xref misses an important feature: typing 'g' in the
> *xref* buffer

Should work now, please see the commit 62349fe82a.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 04:51:51 +0300
> 
> Version: 27.1
> 
> On 12.05.2019 22:45, Juri Linkov wrote:
> > I tried to use project-find-regexp more often than rgrep,
> > but xref misses an important feature: typing 'g' in the
> > *xref* buffer
> 
> Should work now, please see the commit 62349fe82a.

Thanks, but that changeset has a few problems:

  . the new command xref--revert-xref-buffer uses an internal name,
    and has no doc string
  . neither NEWS nor the user manual document the 'g' key in XREF
    buffers
  . it looks like this new command is not useful after M-., because I
    get an error message when I try using it (perhaps this is because
    I didn't understand its use case due to lack of docs)

Let me know if I can help in fixing any of the above.  (I tried to
figure out what this command does and how, but quickly got lost in a
chain of indirections via undocumented internal functions and
variables, sorry.)

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 24.05.2019 11:36, Eli Zaretskii wrote:

> Thanks, but that changeset has a few problems:
> 
>    . the new command xref--revert-xref-buffer uses an internal name,

Is that a problem by itself? We have other bindings that use internal 
command names as well.

>      and has no doc string

How about something like:

   Refresh the search results by repeating the search.

>    . neither NEWS nor the user manual document the 'g' key in XREF
>      buffers

I can add the NEWS entry.

>    . it looks like this new command is not useful after M-., because I
>      get an error message when I try using it (perhaps this is because
>      I didn't understand its use case due to lack of docs)

It has been a deliberate choice to simplify the implementation. IME, you 
don't ever want to refresh the list of definitions. But for other search 
results (references, apropos, project-find-regexp, dired-do-find-regexp) 
it's a lot more common.

Commit 49a363c875 also brings in another difference between the 
behaviors of xref-find-definitions and xref-find-references: the latter 
now shows the xref buffer even when there is just one hit.

> Let me know if I can help in fixing any of the above.  (I tried to
> figure out what this command does and how, but quickly got lost in a
> chain of indirections via undocumented internal functions and
> variables, sorry.)

Do you have a better idea now? Please let me know if you have any 
further questions.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 13:09:50 +0300
> 
> On 24.05.2019 11:36, Eli Zaretskii wrote:
> 
> > Thanks, but that changeset has a few problems:
> > 
> >    . the new command xref--revert-xref-buffer uses an internal name,
> 
> Is that a problem by itself? We have other bindings that use internal 
> command names as well.

That's a problem, yes.  Commands shouldn't be internal functions, by
their very definition.

> >      and has no doc string
> 
> How about something like:
> 
>    Refresh the search results by repeating the search.

Given that it doesn't, at least after M-., this sounds like not all
the truth.  Can it be more detailed?

> >    . neither NEWS nor the user manual document the 'g' key in XREF
> >      buffers
> 
> I can add the NEWS entry.

Please do, and thanks.

> >    . it looks like this new command is not useful after M-., because I
> >      get an error message when I try using it (perhaps this is because
> >      I didn't understand its use case due to lack of docs)
> 
> It has been a deliberate choice to simplify the implementation. IME, you 
> don't ever want to refresh the list of definitions.

Well, one situation where I'd like to refresh is when the TAGS file
was updated.  It could mean that more identifiers matching the search
string are now known.

> But for other search results (references, apropos,
> project-find-regexp, dired-do-find-regexp) it's a lot more common.

At the very least, this should be reflected in the doc string.

> Commit 49a363c875 also brings in another difference between the 
> behaviors of xref-find-definitions and xref-find-references: the latter 
> now shows the xref buffer even when there is just one hit.

This should arguable be in NEWS.

> > Let me know if I can help in fixing any of the above.  (I tried to
> > figure out what this command does and how, but quickly got lost in a
> > chain of indirections via undocumented internal functions and
> > variables, sorry.)
> 
> Do you have a better idea now?

Only slightly so.  The code still doesn't speak to me, but I guess
there isn't much that can be done about that.

Thanks.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 24.05.2019 15:25, Eli Zaretskii wrote:

>>> Thanks, but that changeset has a few problems:
>>>
>>>     . the new command xref--revert-xref-buffer uses an internal name,
>>
>> Is that a problem by itself? We have other bindings that use internal
>> command names as well.
> 
> That's a problem, yes.  Commands shouldn't be internal functions, by
> their very definition.

I've been kind of using it as a means of denoting "we're free to change 
it later", which is, of course, not great for a user, but I don't feel 
like we're settled on a final UI.

If we have a rule about private commands, though, let's fix them. But 
I'd prefer to do it a bit later, in a separate discussion.

>>>       and has no doc string
>>
>> How about something like:
>>
>>     Refresh the search results by repeating the search.
> 
> Given that it doesn't, at least after M-., this sounds like not all
> the truth.  Can it be more detailed?

The truth is, most xref datasets support refreshing, but some don't. At 
least xref-find-definitions doesn't.

I'm not sure we should document that in the command's docstring, though, 
because I think we'll end up with a more different UI for 
xref-find-definitions results, with a different major mode and a keymap 
where 'g' is simply not bound.

>>>     . neither NEWS nor the user manual document the 'g' key in XREF
>>>       buffers
>>
>> I can add the NEWS entry.
> 
> Please do, and thanks.

OK. Does it have to mention the command name?

Here's what I have:

** Xref

*** Refreshing the search results
When an Xref buffer shows search results (e.g. from
xref-find-references or project-find-regexp) you can type 'g' to
repeat the search and refresh the results.

>>>     . it looks like this new command is not useful after M-., because I
>>>       get an error message when I try using it (perhaps this is because
>>>       I didn't understand its use case due to lack of docs)
>>
>> It has been a deliberate choice to simplify the implementation. IME, you
>> don't ever want to refresh the list of definitions.
> 
> Well, one situation where I'd like to refresh is when the TAGS file
> was updated.  It could mean that more identifiers matching the search
> string are now known.

Right, but how often would the event of updading the TAGS file with 
coincide with you wanting to refresh the xref-find-definitions results?

Wouldn't you just do the search again with 'M-.'? This command has the 
easiest keybinding.

>> But for other search results (references, apropos,
>> project-find-regexp, dired-do-find-regexp) it's a lot more common.
> 
> At the very least, this should be reflected in the doc string.

Given that we're dealing with a certain level of abstration, and the 
list of commands using Xref is not limited, how would you phrase it?

>> Commit 49a363c875 also brings in another difference between the
>> behaviors of xref-find-definitions and xref-find-references: the latter
>> now shows the xref buffer even when there is just one hit.
> 
> This should arguable be in NEWS.

How about:

*** New variable 'xref-show-definitions-function'
It encapsulates the logic pertinent to showing the result of
xref-find-definitions. The user can change it to customize its
behavior and the display of results.

*** Search results show the buffer even for one hit
The search-type Xref commands (e.g. xref-find-references or
project-find-regexp) now show the results buffer even when there is
only one hit. This can be altered by changing
xref-show-xrefs-function.

You can probably see a certain level of duplication there, though.

>> Do you have a better idea now?
> 
> Only slightly so.  The code still doesn't speak to me, but I guess
> there isn't much that can be done about that.

This idea is pretty simple: instead of passing a list of search results 
to xref--show-xrefs, we pass an anonymous function that can be called to 
do the search, as well as repeat it, and returns the said list.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 15:57:03 +0300
> 
> >>>     . the new command xref--revert-xref-buffer uses an internal name,
> >>
> >> Is that a problem by itself? We have other bindings that use internal
> >> command names as well.
> > 
> > That's a problem, yes.  Commands shouldn't be internal functions, by
> > their very definition.
> 
> I've been kind of using it as a means of denoting "we're free to change 
> it later", which is, of course, not great for a user, but I don't feel 
> like we're settled on a final UI.
> 
> If we have a rule about private commands, though, let's fix them. But 
> I'd prefer to do it a bit later, in a separate discussion.

A command can legitimately be invoked via its name, so once it's
introduced, you cannot change its name, or change the API in
backward-incompatible way.  If you'd prefer to reserve future changes,
make the command call an internal function which does most of the job.

And no, I don't think we can defer this to later.  Commands must not
be internal.

> >>     Refresh the search results by repeating the search.
> > 
> > Given that it doesn't, at least after M-., this sounds like not all
> > the truth.  Can it be more detailed?
> 
> The truth is, most xref datasets support refreshing, but some don't. At 
> least xref-find-definitions doesn't.
> 
> I'm not sure we should document that in the command's docstring, though, 
> because I think we'll end up with a more different UI for 
> xref-find-definitions results, with a different major mode and a keymap 
> where 'g' is simply not bound.

I'm not a great fan of obfuscating the docs, except for reasons of
hiding internal implementation details.  Why is it a problem to say
that XREF buffers created by xref-find-definitions currently don't
support 'g'?  For that matter, why shouldn't the error message be
explicit about that, instead of saying something technically accurate
but quite obscure from user's POV?

> >>>     . neither NEWS nor the user manual document the 'g' key in XREF
> >>>       buffers
> >>
> >> I can add the NEWS entry.
> > 
> > Please do, and thanks.
> 
> OK. Does it have to mention the command name?

I think so, yes.  People may wish to bind it to a different key.

> Here's what I have:
> 
> ** Xref
> 
> *** Refreshing the search results
> When an Xref buffer shows search results (e.g. from
> xref-find-references or project-find-regexp) you can type 'g' to
> repeat the search and refresh the results.

This should say explicitly that only some Xref functions can support
refreshing the results.  "E.g." can be interpreted as just an example,
not that some other examples don't support this.

> > Well, one situation where I'd like to refresh is when the TAGS file
> > was updated.  It could mean that more identifiers matching the search
> > string are now known.
> 
> Right, but how often would the event of updading the TAGS file with 
> coincide with you wanting to refresh the xref-find-definitions results?

When the original M-. doesn't show what I expected, for example.

> Wouldn't you just do the search again with 'M-.'?

Yes, but that argument is true for any 'revert' action as well, isn't
it?  And we still have revert actions.

> >> But for other search results (references, apropos,
> >> project-find-regexp, dired-do-find-regexp) it's a lot more common.
> > 
> > At the very least, this should be reflected in the doc string.
> 
> Given that we're dealing with a certain level of abstration, and the 
> list of commands using Xref is not limited, how would you phrase it?

I'm not sure I understand the difficulty.  Regardless of the level of
abstraction, 'g' invokes a specific command, and I see no problems
having the doc string of that command mention other specific commands.
What am I missing?

> >> Commit 49a363c875 also brings in another difference between the
> >> behaviors of xref-find-definitions and xref-find-references: the latter
> >> now shows the xref buffer even when there is just one hit.
> > 
> > This should arguable be in NEWS.
> 
> How about:
> 
> *** New variable 'xref-show-definitions-function'
> It encapsulates the logic pertinent to showing the result of
> xref-find-definitions. The user can change it to customize its
> behavior and the display of results.
> 
> *** Search results show the buffer even for one hit
> The search-type Xref commands (e.g. xref-find-references or
> project-find-regexp) now show the results buffer even when there is
> only one hit. This can be altered by changing
> xref-show-xrefs-function.

OK, but (a) the heading sentence should end in a period; (b) please
use 2 spaces between sentences.

> You can probably see a certain level of duplication there, though.

I don't.  I see one entry referencing another, that's all.  We
shouldn't expect the readers to make complicated logical deliberations
to understand that the first entry hints on what the second entry
spells out explicitly.  NEWS is not a riddle, it's a means for quickly
reviewing the important changes in a new Emacs version.

> >> Do you have a better idea now?
> > 
> > Only slightly so.  The code still doesn't speak to me, but I guess
> > there isn't much that can be done about that.
> 
> This idea is pretty simple: instead of passing a list of search results 
> to xref--show-xrefs, we pass an anonymous function that can be called to 
> do the search, as well as repeat it, and returns the said list.

<rant>
The idea is simple and clear, but trying to understand what it does in
any particular invocation isn't.  I tried to figure out what happens
after M-., which required me to understand when is the xref--fetcher
variable set and to what values.  There's no easy answer to that
question, neither in comments nor in the doc strings.  The value is
set from a function passed as an argument to a couple of other
internal functions, so now I need to go deeper into the rabbit hole
and understand when and how these two functions are called, and with
what function as that argument.  Etc. etc. -- it feels like following
a deeply OO C++ code, where basically the only way to figure out how
the code works is to step through the code with a debugger.  Except
that Edebug doesn't support generics well enough to do that, so I'm
screwed even if I have the time and energy to go down that hole.

It used to be possible to understand what happens in a Lisp program by
just reading the code, but it no longer is, in many cases.

Useful comments can go a long way towards making such investigations
much easier and more efficient, but I guess I will hear
counter-arguments about the need to keep comments up-to-date with the
code...
</rant>

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 24.05.2019 17:10, Eli Zaretskii wrote:
> And no, I don't think we can defer this to later.  Commands must not
> be internal.

A later change can "fix" several commands at once.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 17:26:20 +0300
> 
> On 24.05.2019 17:10, Eli Zaretskii wrote:
> > And no, I don't think we can defer this to later.  Commands must not
> > be internal.
> 
> A later change can "fix" several commands at once.

Sigh.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 24.05.2019 18:02, Eli Zaretskii wrote:

>> A later change can "fix" several commands at once.
> 
> Sigh.

Okay then. Fixed the new one now. NEWS entries added as well.

bug#35702: xref revert-buffer

[Dmitry Gutov]

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

On 24.05.2019 17:10, Eli Zaretskii wrote:

>> I'm not sure we should document that in the command's docstring, though,
>> because I think we'll end up with a more different UI for
>> xref-find-definitions results, with a different major mode and a keymap
>> where 'g' is simply not bound.
> 
> I'm not a great fan of obfuscating the docs, except for reasons of
> hiding internal implementation details.

First: my point is, it's possible that it *will* always work wherever 
you are able to invoke it with 'g'. The Xref buffers where it wouldn't 
work wouldn't bind 'g'.

Second: we can "obfuscate" the docs for things we're not sure about. 
Think forward: if we expose the Xref UI as a library for other packages 
in the future (and we probably will), should we allow some of them to 
opt out of revert-ability (for simplicity, let's say), or not?

And if we do, we won't really be able to enumerate all the commands that 
opted out in xref--revert-xref-buffer's docstring.

> Why is it a problem to say
> that XREF buffers created by xref-find-definitions currently don't
> support 'g'?

Because it feels ad-hoc and kinda weird. The intention is to support 
reverting in "search results" buffers, not intentionally refuse support 
it in xref-find-definitions.

But we can do that.

> For that matter, why shouldn't the error message be
> explicit about that, instead of saying something technically accurate
> but quite obscure from user's POV?

How would you phrase it?

>> *** Refreshing the search results
>> When an Xref buffer shows search results (e.g. from
>> xref-find-references or project-find-regexp) you can type 'g' to
>> repeat the search and refresh the results.
> 
> This should say explicitly that only some Xref functions can support
> refreshing the results.  "E.g." can be interpreted as just an example,
> not that some other examples don't support this.

The intent was to introduce a kind of classification. That is, to call 
all commands that do a "wide" search as "commands that show search results".

xref-find-definitions, meanwhile, might be considered a "jump with 
completions" command.

>> Right, but how often would the event of updading the TAGS file with
>> coincide with you wanting to refresh the xref-find-definitions results?
> 
> When the original M-. doesn't show what I expected, for example.

Hmm. So it's something you would really find useful?

>> Wouldn't you just do the search again with 'M-.'?
> 
> Yes, but that argument is true for any 'revert' action as well, isn't
> it?  And we still have revert actions.

Not exactly: first, M-. is easier to invoke and re-invoke than 
project-find-regexp, and you are likely to edit the regexp before searching.

Second, I quite frequently do something like this:

- Do a search for a string with project-find-regexp,
- Do *something* with the results, e.g. rename them all,
- Repeat the search, to make sure I have dealt with all occurrences.

So 'g' helps considerably there. And I don't have any similar scenarios 
for xref-find-definitions.

To be clear, though, we *can* add support for reverting to 
xref-find-definitions as well, if you want. Just at the cost of a 
certain increase of complexity, see if you like the patch. 
xref-find-defitions-revertable.diff is attached.

> OK, but (a) the heading sentence should end in a period; (b) please
> use 2 spaces between sentences.

OK.

>> You can probably see a certain level of duplication there, though.
> 
> I don't.  I see one entry referencing another, that's all.

Just to be clear: I'm referring to two of the three entries I've showed 
in the previous email mentioning "search-type Xref commands".

>> This idea is pretty simple: instead of passing a list of search results
>> to xref--show-xrefs, we pass an anonymous function that can be called to
>> do the search, as well as repeat it, and returns the said list.
> 
> <rant>
> The idea is simple and clear, but trying to understand what it does in
> any particular invocation isn't.  I tried to figure out what happens
> after M-., which required me to understand when is the xref--fetcher
> variable set and to what values.  There's no easy answer to that
> question, neither in comments nor in the doc strings.

Would a docstring saying "Function that returns a list of xrefs 
containing the search results" change things?

> The value is
> set from a function passed as an argument to a couple of other
> internal functions, so now I need to go deeper into the rabbit hole
> and understand when and how these two functions are called, and with
> what function as that argument.

Where the fetcher is created is not to hard to find, I think (only a few 
local variables with that name).

> Etc. etc. -- it feels like following
> a deeply OO C++ code, where basically the only way to figure out how
> the code works is to step through the code with a debugger.

It's actually more "functional programming" than OOP, and not the worst 
example of it (very little function composition, for instance).

I can feel your pain (there's a lot of the code I have difficulty 
following as well), but I'm not sure where I could add comments where 
they wouldn't be self-evident. A couple of docstrings wouldn't hurt, though.

> Except
> that Edebug doesn't support generics well enough to do that, so I'm
> screwed even if I have the time and energy to go down that hole.

Well, at least in this case there are no generic calls between 
xref-find-references and xref--show-xrefs and the fetcher creation.

> It used to be possible to understand what happens in a Lisp program by
> just reading the code, but it no longer is, in many cases.

Err, my experience of reading old Lisp code in various packages doesn't 
really agree.

> Useful comments can go a long way towards making such investigations
> much easier and more efficient, but I guess I will hear
> counter-arguments about the need to keep comments up-to-date with the
> code...

It's not like I'm against those, but it might take a different person to 
identify the places that need to be commented.

[-- Attachment #2: xref-find-definitions-revertable.diff --]
[-- Type: text/x-patch, Size: 3228 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6a4906a232..0fd59dc330 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -782,7 +782,10 @@ xref--analyze
                   #'equal))
 
 (defun xref--show-xref-buffer (fetcher alist)
-  (let* ((xrefs (if (functionp fetcher) (funcall fetcher) fetcher))
+  (let* ((xrefs
+          (or
+           (assoc-default 'xrefs alist)
+           (funcall fetcher)))
          (xref-alist (xref--analyze xrefs)))
     (with-current-buffer (get-buffer-create xref-buffer-name)
       (setq buffer-undo-list nil)
@@ -817,13 +820,16 @@ xref--revert-xref-buffer
            'face 'error))))
       (goto-char (point-min)))))
 
-(defun xref--show-defs-buffer (xrefs alist)
-  (cond
-   ((not (cdr xrefs))
-    (xref--pop-to-location (car xrefs)
-                           (assoc-default 'display-action alist)))
-   (t
-    (xref--show-xref-buffer xrefs alist))))
+(defun xref--show-defs-buffer (fetcher alist)
+  (let ((xrefs (funcall fetcher)))
+    (cond
+     ((not (cdr xrefs))
+      (xref--pop-to-location (car xrefs)
+                             (assoc-default 'display-action alist)))
+     (t
+      (xref--show-xref-buffer fetcher
+                              (cons (cons 'xrefs xrefs)
+                                    alist))))))
 
 \f
 (defvar xref-show-xrefs-function 'xref--show-xref-buffer
@@ -885,28 +891,29 @@ xref--read-identifier
 ;;; Commands
 
 (defun xref--find-xrefs (input kind arg display-action)
+  (xref--show-xrefs
+   (xref--create-fetcher input kind arg)
+   display-action))
+
+(defun xref--find-definitions (id display-action)
+  (xref--show-defs
+   (xref--create-fetcher id 'definitions id)
+   display-action))
+
+(defun xref--create-fetcher (input kind arg)
   (let* ((orig-buffer (current-buffer))
          (orig-position (point))
          (backend (xref-find-backend))
-         (method (intern (format "xref-backend-%s" kind)))
-         (fetcher (lambda ()
-                    (save-excursion
-                      (when (buffer-live-p orig-buffer)
-                        (set-buffer orig-buffer)
-                        (ignore-errors (goto-char orig-position)))
-                      (let ((xrefs (funcall method backend arg)))
-                        (unless xrefs
-                          (xref--not-found-error kind input))
-                        xrefs)))))
-    (xref--show-xrefs fetcher display-action)))
-
-(defun xref--find-definitions (id display-action)
-  (let ((xrefs (funcall #'xref-backend-definitions
-                        (xref-find-backend)
-                        id)))
-    (unless xrefs
-      (xref--not-found-error 'definitions id))
-    (xref--show-defs xrefs display-action)))
+         (method (intern (format "xref-backend-%s" kind))))
+    (lambda ()
+      (save-excursion
+        (when (buffer-live-p orig-buffer)
+          (set-buffer orig-buffer)
+          (ignore-errors (goto-char orig-position)))
+        (let ((xrefs (funcall method backend arg)))
+          (unless xrefs
+            (xref--not-found-error kind input))
+          xrefs)))))
 
 (defun xref--not-found-error (kind input)
   (user-error "No %s found for: %s" (symbol-name kind) input))

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 18:15:57 +0300
> 
> >> I'm not sure we should document that in the command's docstring, though,
> >> because I think we'll end up with a more different UI for
> >> xref-find-definitions results, with a different major mode and a keymap
> >> where 'g' is simply not bound.
> > 
> > I'm not a great fan of obfuscating the docs, except for reasons of
> > hiding internal implementation details.
> 
> First: my point is, it's possible that it *will* always work wherever 
> you are able to invoke it with 'g'. The Xref buffers where it wouldn't 
> work wouldn't bind 'g'.

Sorry, I don't understand what you are saying here.  If some XREF
buffers won't have the 'g' bound to a command, why do we have it bound
now, and why do we have an error message when the command cannot be
used?

> Second: we can "obfuscate" the docs for things we're not sure about. 

Doc strings aren't sacred: if we change the functionality, we can
modify the doc string accordingly.  We do that all the time.  So
there's no need to avoid documenting something just because we might
change it later.

> Think forward: if we expose the Xref UI as a library for other packages 
> in the future (and we probably will), should we allow some of them to 
> opt out of revert-ability (for simplicity, let's say), or not?

In general, I consider such changes a bad idea: XREF is a mode, and a
mode should be consistent across all its users.

> And if we do, we won't really be able to enumerate all the commands that 
> opted out in xref--revert-xref-buffer's docstring.

We don't need to enumerate all of them, we only need to enumerate the
ones that are provided with Emacs and are known not to support 'g'.
xref-find-definitions is a very frequently used command, so saying
that it doesn't support 'g' is IMO important enough.

> > Why is it a problem to say that XREF buffers created by
> > xref-find-definitions currently don't support 'g'?
> 
> Because it feels ad-hoc and kinda weird.

Are you going to add support for it any time soon?  If so, no need to
document the missing feature just yet.  But if there's a real chance
this will remain unsupported, we should document that now, lest we
forget.

> > For that matter, why shouldn't the error message be
> > explicit about that, instead of saying something technically accurate
> > but quite obscure from user's POV?
> 
> How would you phrase it?

  XREF buffers created by xref-find-definitions cannot be reverted.

> >> *** Refreshing the search results
> >> When an Xref buffer shows search results (e.g. from
> >> xref-find-references or project-find-regexp) you can type 'g' to
> >> repeat the search and refresh the results.
> > 
> > This should say explicitly that only some Xref functions can support
> > refreshing the results.  "E.g." can be interpreted as just an example,
> > not that some other examples don't support this.
> 
> The intent was to introduce a kind of classification. That is, to call 
> all commands that do a "wide" search as "commands that show search results".
> 
> xref-find-definitions, meanwhile, might be considered a "jump with 
> completions" command.

The classification should be described in more detail before it's
used.  Just naming it is not enough.  If you add such a description, I
probably won't object (but please watch out for the danger of too long
descriptions which don't belong in NEWS).

> >> Right, but how often would the event of updading the TAGS file with
> >> coincide with you wanting to refresh the xref-find-definitions results?
> > 
> > When the original M-. doesn't show what I expected, for example.
> 
> Hmm. So it's something you would really find useful?

Yes.

> >> Wouldn't you just do the search again with 'M-.'?
> > 
> > Yes, but that argument is true for any 'revert' action as well, isn't
> > it?  And we still have revert actions.
> 
> Not exactly: first, M-. is easier to invoke and re-invoke than 
> project-find-regexp, and you are likely to edit the regexp before searching.

I don't know about "easier", but the use case I had in mind was with
original invocation via "C-u M-.".

> - Do a search for a string with project-find-regexp,
> - Do *something* with the results, e.g. rename them all,
> - Repeat the search, to make sure I have dealt with all occurrences.
> 
> So 'g' helps considerably there. And I don't have any similar scenarios 
> for xref-find-definitions.

I don't think this is a reason good enough not to support 'g'.
Consistency is more important than the importance of use cases.
Moreover, you may not see important use cases now, but someone else
might.  We should only refrain from supporting a command when we are
100% sure it can never be useful.

> To be clear, though, we *can* add support for reverting to 
> xref-find-definitions as well, if you want. Just at the cost of a 
> certain increase of complexity, see if you like the patch. 
> xref-find-defitions-revertable.diff is attached.

Doesn't look to me like it adds any significant complexity.

> >> You can probably see a certain level of duplication there, though.
> > 
> > I don't.  I see one entry referencing another, that's all.
> 
> Just to be clear: I'm referring to two of the three entries I've showed 
> in the previous email mentioning "search-type Xref commands".

Why is that "duplication"?  using the same terminology is a Good
Thing, as it allows the reader easier understanding what is being
discussed.

> > The idea is simple and clear, but trying to understand what it does in
> > any particular invocation isn't.  I tried to figure out what happens
> > after M-., which required me to understand when is the xref--fetcher
> > variable set and to what values.  There's no easy answer to that
> > question, neither in comments nor in the doc strings.
> 
> Would a docstring saying "Function that returns a list of xrefs 
> containing the search results" change things?

I meant a comment that would explain how things worked and in what
scenarios.

> > The value is
> > set from a function passed as an argument to a couple of other
> > internal functions, so now I need to go deeper into the rabbit hole
> > and understand when and how these two functions are called, and with
> > what function as that argument.
> 
> Where the fetcher is created is not to hard to find, I think (only a few 
> local variables with that name).

Finding the places where it is defined is easy enough; understanding
the semantics of that isn't.  The code is obfuscated by using generic
names like "method", and has no comments explaining what is going on
in plain English.

> > Etc. etc. -- it feels like following
> > a deeply OO C++ code, where basically the only way to figure out how
> > the code works is to step through the code with a debugger.
> 
> It's actually more "functional programming" than OOP, and not the worst 
> example of it (very little function composition, for instance).

Thank God for small favors!

> > It used to be possible to understand what happens in a Lisp program by
> > just reading the code, but it no longer is, in many cases.
> 
> Err, my experience of reading old Lisp code in various packages doesn't 
> really agree.

I guess we are talking about different parts of Emacs.

> It's not like I'm against those, but it might take a different person to 
> identify the places that need to be commented.

That different person will not know enough about the code to add
useful comments.  Not unless they invest the effort to study and
understand it.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 24.05.2019 22:35, Eli Zaretskii wrote:

>> First: my point is, it's possible that it *will* always work wherever
>> you are able to invoke it with 'g'. The Xref buffers where it wouldn't
>> work wouldn't bind 'g'.
> 
> Sorry, I don't understand what you are saying here.  If some XREF
> buffers won't have the 'g' bound to a command, why do we have it bound
> now, and why do we have an error message when the command cannot be
> used?

Because nobody has written a better alternative UI for 
xref-find-definitions specifically yet. But when someone(tm) does, it'll 
likely have a different keymap.

Anyway, never mind that for now.

>> Think forward: if we expose the Xref UI as a library for other packages
>> in the future (and we probably will), should we allow some of them to
>> opt out of revert-ability (for simplicity, let's say), or not?
> 
> In general, I consider such changes a bad idea: XREF is a mode, and a
> mode should be consistent across all its users.

Fair enough.

>>> Why is it a problem to say that XREF buffers created by
>>> xref-find-definitions currently don't support 'g'?
>>
>> Because it feels ad-hoc and kinda weird.
> 
> Are you going to add support for it any time soon?

Apparently I am!

>> Hmm. So it's something you would really find useful?
> 
> Yes.

OK.

>> To be clear, though, we *can* add support for reverting to
>> xref-find-definitions as well, if you want. Just at the cost of a
>> certain increase of complexity, see if you like the patch.
>> xref-find-defitions-revertable.diff is attached.
> 
> Doesn't look to me like it adds any significant complexity.

OK, I'll install it, if only to make the documentation simpler. That's 
something I hadn't really considered in advance.

>> Just to be clear: I'm referring to two of the three entries I've showed
>> in the previous email mentioning "search-type Xref commands".
> 
> Why is that "duplication"?  using the same terminology is a Good
> Thing, as it allows the reader easier understanding what is being
> discussed.

I was thinking it would be better to coin a common term that separates 
"other" Xref commands from xref-find-definitions, so we don't have to 
enumerate them later.

This distinction is also important, for instance, to make the purposes 
of xref-show-xrefs-function and xref-show-definitions-function clear in 
their docstrings.

>> Would a docstring saying "Function that returns a list of xrefs
>> containing the search results" change things?
> 
> I meant a comment that would explain how things worked and in what
> scenarios.

Would you be surprised to hear that I don't even know where to begin?

When doing something for xref.el or project.el, lately I spend quite a 
bit of time thinking how to make the concepts more transparent, and very 
little time implementing them. So I currently feel that the ideas are 
simple (meaning, there are no behaviors that require particular extra 
commentary), and the implementations are maybe too simplistic.

There are much more difficult things in this package, e.g. window 
management.

>> Where the fetcher is created is not to hard to find, I think (only a few
>> local variables with that name).
> 
> Finding the places where it is defined is easy enough; understanding
> the semantics of that isn't.  The code is obfuscated by using generic
> names like "method", and has no comments explaining what is going on
> in plain English.

method uses the cl-generic infrastructure (which might be 
under-documented, though happily though no fault of my own), but you 
don't need to understand it to see what's going on with fetcher.

(xref-backend-definitions backend id) returns a list of definitions. 
That's all what's necessary to know here.

>> It's not like I'm against those, but it might take a different person to
>> identify the places that need to be commented.
> 
> That different person will not know enough about the code to add
> useful comments.  Not unless they invest the effort to study and
> understand it.

They could ask specific questions. It's harder to answer a question like 
"explain all this stuff to me".

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 24 May 2019 23:51:58 +0300
> 
> >> Just to be clear: I'm referring to two of the three entries I've showed
> >> in the previous email mentioning "search-type Xref commands".
> > 
> > Why is that "duplication"?  using the same terminology is a Good
> > Thing, as it allows the reader easier understanding what is being
> > discussed.
> 
> I was thinking it would be better to coin a common term that separates 
> "other" Xref commands from xref-find-definitions, so we don't have to 
> enumerate them later.
> 
> This distinction is also important, for instance, to make the purposes 
> of xref-show-xrefs-function and xref-show-definitions-function clear in 
> their docstrings.

I'm fine with coming up with some classification of these commands.
But I think the classification should be in the manual, not in NEWS.

> >> Would a docstring saying "Function that returns a list of xrefs
> >> containing the search results" change things?
> > 
> > I meant a comment that would explain how things worked and in what
> > scenarios.
> 
> Would you be surprised to hear that I don't even know where to begin?

Yes.

> When doing something for xref.el or project.el, lately I spend quite a 
> bit of time thinking how to make the concepts more transparent, and very 
> little time implementing them. So I currently feel that the ideas are 
> simple (meaning, there are no behaviors that require particular extra 
> commentary), and the implementations are maybe too simplistic.
> 
> There are much more difficult things in this package, e.g. window 
> management.

I didn't mean to imply that the stuff we were discussing is the only
one that is painful to decipher.

Thanks.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 25.05.2019 10:39, Eli Zaretskii wrote:
> I'm fine with coming up with some classification of these commands.
> But I think the classification should be in the manual, not in NEWS.

If you have any suggestions for better docstrings for there variables, 
I'd like to know.

>> Would you be surprised to hear that I don't even know where to begin?
> Yes.

Well, it's true. I don't know where the comments will work better for 
the recent additions than just reading the code.

Do you have any particular questions?

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 25 May 2019 18:47:57 +0300
> 
> Do you have any particular questions?

Thanks, but the purpose of my rant was not to follow up with
questions.  If I cannot convince you to clarify the code by adding
more comments, then I guess I failed, and let's drop this.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 25.05.2019 19:06, Eli Zaretskii wrote:
> Thanks, but the purpose of my rant was not to follow up with
> questions.  If I cannot convince you to clarify the code by adding
> more comments, then I guess I failed, and let's drop this.

I was meaning to put at least some of the answers into comments.

But OK.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 25 May 2019 19:14:39 +0300
> 
> On 25.05.2019 19:06, Eli Zaretskii wrote:
> > Thanks, but the purpose of my rant was not to follow up with
> > questions.  If I cannot convince you to clarify the code by adding
> > more comments, then I guess I failed, and let's drop this.
> 
> I was meaning to put at least some of the answers into comments.

Then I already asked those questions.  The most important one is what
values can the fetcher variable have, and in what situations (i.e.,
triggered by what command(s)) will we see each value.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 25.05.2019 19:49, Eli Zaretskii wrote:

> Then I already asked those questions.  The most important one is what
> values can the fetcher variable have, and in what situations (i.e.,
> triggered by what command(s)) will we see each value.

It's akin to asking what values could revert-buffer-function have: 
different ones. Every command that uses the xref UI is responsible for 
creating such a function (though most of current ones have a common 
implementation only parameterized by KIND, see xref--create-fetcher).

I have added some more docstrings and a comment, though. Hope they clear 
up a few things.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 26 May 2019 00:33:33 +0300
> 
> On 25.05.2019 19:49, Eli Zaretskii wrote:
> 
> > Then I already asked those questions.  The most important one is what
> > values can the fetcher variable have, and in what situations (i.e.,
> > triggered by what command(s)) will we see each value.
> 
> It's akin to asking what values could revert-buffer-function have: 
> different ones.

Although in theory there could indeed be an infinite number of values,
in practice the current actual set is very small, and can be easily
described.  If you want to avoid the (IMO imaginary) danger of
implying there could be no other values, you can say explicitly that
other values are possible.

IOW, useful documentation should be general, but not too general.  If
being general means refraining saying something that could potentially
help the reader understand the software and use it, then you are
probably on the wrong side of "too general".

> I have added some more docstrings and a comment, though. Hope they clear 
> up a few things.

Thanks.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 26.05.2019 19:44, Eli Zaretskii wrote:

>> It's akin to asking what values could revert-buffer-function have:
>> different ones.
> 
> Although in theory there could indeed be an infinite number of values,
> in practice the current actual set is very small, and can be easily
> described.

And yet, it's not hugely important to the code that's calling it. Or for 
understanding of said code (call the function, show the returned items; 
call it again if the user wants to refresh the list). All that as long 
as the function adheres to its protocol. It's like 'cons' in that 
regard. Or 'seq-map.'

So previously, we passed a list of xrefs to xref--show-xrefs. Now we 
pass a function that returns said list instead. It's a fairly trivial 
change by itself, so if the previous state of affairs was okay, the new 
one shouldn't require a lot of new documentation.

> If you want to avoid the (IMO imaginary) danger of
> implying there could be no other values, you can say explicitly that
> other values are possible.

That depends on the level of detail you would like. The minimal level 
description is in the docstring, and it should be fine for understanding 
any code that uses FETCHER.

There is some intermediate description in xref--create-fetcher's 
docstring, though I don't know how much it helped.

But if you want a description that goes to the lower level and describes 
*everything*, like how it uses obarray for Elisp and usually scans the 
contents of TAGS otherwise... I don't think it's helpful for 
understanding of the xref--create-fetcher variable, or the corresponding 
function arguments.

It's simply not the appropriate place for it (not sure if an "overview" 
section would be better, but the manual seems like the best place; we 
could add some extra commentary in elisp-mode.el or etags.el if the 
existing ones seem not enough).

> IOW, useful documentation should be general, but not too general.  If
> being general means refraining saying something that could potentially
> help the reader understand the software and use it, then you are
> probably on the wrong side of "too general".

On the other hand, I wouldn't want to bog the description of a fairly 
clean abstraction (if I do say that myself) with incidental details. Or 
duplicate information that's already available elsewhere.

The general way we describe our code could, of course, be improved, but 
I don't subscribe to the idea that we can have a general overview of the 
system no matter where we start reading the code.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 27 May 2019 17:54:21 +0300
> 
> On 26.05.2019 19:44, Eli Zaretskii wrote:
> 
> >> It's akin to asking what values could revert-buffer-function have:
> >> different ones.
> > 
> > Although in theory there could indeed be an infinite number of values,
> > in practice the current actual set is very small, and can be easily
> > described.
> 
> And yet, it's not hugely important to the code that's calling it.

It was important to me.  You prodded me to ask questions and tell you
what made the code reading difficult for me, remember?  Now you are
trying to convince me that it isn't a difficulty, or that I shouldn't
be asking for that?

> So previously, we passed a list of xrefs to xref--show-xrefs. Now we 
> pass a function that returns said list instead. It's a fairly trivial 
> change by itself, so if the previous state of affairs was okay, the new 
> one shouldn't require a lot of new documentation.

You assume that the previous state was okay, which is not a given.
Moreover, you assume that the reader remembers there was a list
before, and can therefore "easily" translate this knowledge to the new
code, instantly understanding that the function now returns the list
that was previously passed as argument.  That's a lot of assumptions.

> > If you want to avoid the (IMO imaginary) danger of
> > implying there could be no other values, you can say explicitly that
> > other values are possible.
> 
> That depends on the level of detail you would like. The minimal level 
> description is in the docstring, and it should be fine for understanding 
> any code that uses FETCHER.

I hope you trust me to have read every bit of comment and doc string I
could possibly find before complaining.

> The general way we describe our code could, of course, be improved, but 
> I don't subscribe to the idea that we can have a general overview of the 
> system no matter where we start reading the code.

See, I was sure I will get a response like that, which was why I
marked what I wrote as <rant>.  If you don't intend to humor requests
for more documentation of the code's workings, then why do you respond
to such rants?  It's a waste of time for both of us, and the result is
known in advance.

I guess I should simply shut up about this next time.  Sorry I wasn't
wiser this time.

bug#35702: xref revert-buffer

[Dmitry Gutov]

On 27.05.2019 19:31, Eli Zaretskii wrote:

>>> Although in theory there could indeed be an infinite number of values,
>>> in practice the current actual set is very small, and can be easily
>>> described.
>>
>> And yet, it's not hugely important to the code that's calling it.
> 
> It was important to me.  You prodded me to ask questions and tell you
> what made the code reading difficult for me, remember?  Now you are
> trying to convince me that it isn't a difficulty, or that I shouldn't
> be asking for that?

I'm sorry to disappoint. I'm sure you understand that there are question 
I can answer but I'd have hard time converting into code comments.

Here are the kinds of questions I was hoping for:

* What does this function/variable do, or what is it for?

These can translate into [improvements for] docstrings or into top 
Commentary.

* Given the stated purpose of <function> why does it implement it *this 
particular way*?

These can translate into inline comments.

As such, I can answer your question (probably already did), but since 
IMO they are not about xref--fetcher or xref--show-defs, I can't put the 
answers into nearby comments.

So instead I ended up thinking of a few questions along these lines and 
asking them myself, hence the resulting commit that added some more 
documentation.

>> So previously, we passed a list of xrefs to xref--show-xrefs. Now we
>> pass a function that returns said list instead. It's a fairly trivial
>> change by itself, so if the previous state of affairs was okay, the new
>> one shouldn't require a lot of new documentation.
> 
> You assume that the previous state was okay, which is not a given.
> Moreover, you assume that the reader remembers there was a list
> before, and can therefore "easily" translate this knowledge to the new
> code, instantly understanding that the function now returns the list
> that was previously passed as argument.  That's a lot of assumptions.

Hopefully the new docstrings will make understanding all that easier anyway.

>> That depends on the level of detail you would like. The minimal level
>> description is in the docstring, and it should be fine for understanding
>> any code that uses FETCHER.
> 
> I hope you trust me to have read every bit of comment and doc string I
> could possibly find before complaining.

Well, there was some extra documentation added in the meantime, and I'm 
not sure how you feel about it.

>> The general way we describe our code could, of course, be improved, but
>> I don't subscribe to the idea that we can have a general overview of the
>> system no matter where we start reading the code.
> 
> See, I was sure I will get a response like that, which was why I
> marked what I wrote as <rant>.  If you don't intend to humor requests
> for more documentation of the code's workings, then why do you respond
> to such rants?
I did suggest some other places where new commentary can go.

Maybe you could propose something else as well, now that I've tried to 
explain why your previous suggestion is hard for me to do. If I 
interpreted it correctly, at least.

bug#35702: xref revert-buffer

[Eli Zaretskii]

> Cc: 35702@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 28 May 2019 17:10:03 +0300
> 
> > It was important to me.  You prodded me to ask questions and tell you
> > what made the code reading difficult for me, remember?  Now you are
> > trying to convince me that it isn't a difficulty, or that I shouldn't
> > be asking for that?
> 
> I'm sorry to disappoint.

Relax, I wasn't disappointed.