bug#35702: xref revert-buffer

[Dmitry Gutov]

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.

xref-find-definitions-revertable.diff
Description: Text Data