bug#35702: xref revert-buffer

[Dmitry Gutov <dgutov@yandex.ru>]

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