Subject: bug#35702: xref revert-buffer

From: Eli Zaretskii <[email-masked]>
Date: Fri, 24 May 2019 17:10:00 +0300

> Cc: [email-masked], [email-masked]
> From: Dmitry Gutov <[email-masked]>
> 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>