bug#68958: [PATCH] Support bookmarking Xref results buffers

[Dmitry Gutov]

On 12/02/2024 13:45, Eshel Yaron wrote:

> > With that, the simple API "patch FETCHER and DISPLAY-ACTION (probably
> > nil)" would turn into something at least twice (or 3x) as complex.
>
> Well, ISTM that one could also say that the API is as simple as it was:
> you pass the same stuff, and get the same result.  It's just that,
> optionally, you can also do a bit more (set a variable inside FETCHER)
> and get a bit more (bookmarking).

A fair amount more: to fill out an alist, and have a new generic 
function implemented.

> I agree that redundant complexity is
> better avoided, but this is the simplest compatible extension to the API
> I came up with to implement this feature.

If we're going to recommend the callers use the new capability, I'd 
rather they didn't have to be redundant every time.

> > I'm not quite convinced that being able to bookmark Xref buffers is
> > worth this cost.
>
> Fair enough, it's your call.  IMO this is a rather useful capability,
> and I don't think it's that important to keep the API maximally simple
> if it doesn't facilitate everything we want it to.  In other words, as
> long as we maintain compatibility, what's wrong with extending the API
> to support more features?

A more concise yet backward-compatible approach could also look like the 
below.

Though I'm not sure whether the fetcher should reach 
xref-show-xrefs-function intact (simpler code, but a breakage in the 
interface, which could be mended with catching 
wrong-number-of-arguments), or like in this example, both the original 
fetcher and the arguments should be passed through alist.

Otherwise, the requirements on the arguments are the same (fetcher -- 
named function, args -- printability).

Also, I'm not sure how we're supposed to guarantee that 
xref--original-buffer is live. Is that for use with desktop-mode only?

And it seems like as soon as the buffer has some new changes, the 
bookmark is likely to become invalid (the same value of point will point 
to a different identifier).

Anyway, regarding that partial patch:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 717b837a2e5..cfdb9cd72de 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1476,13 +1476,13 @@ xref--read-identifier-history
 (defvar xref--read-pattern-history nil)

 ;;;###autoload
-(defun xref-show-xrefs (fetcher display-action)
+(defun xref-show-xrefs (fetcher display-action &optional args)
   "Display some Xref values produced by FETCHER using DISPLAY-ACTION.
 The meanings of both arguments are the same as documented in
 `xref-show-xrefs-function'."
-  (xref--show-xrefs fetcher display-action))
+  (xref--show-xrefs fetcher display-action args))

-(defun xref--show-xrefs (fetcher display-action &optional 
_always-show-list)
+(defun xref--show-xrefs (fetcher display-action &optional 
_always-show-list args)
   (unless (functionp fetcher)
     ;; Old convention.
     (let ((xrefs fetcher))
@@ -1494,12 +1494,16 @@ xref--show-xrefs
                     xrefs
                   (setq xrefs 'called-already)))))))
   (let ((cb (current-buffer))
-        (pt (point)))
-    (prog1
+        (pt (point))
+        (orig-fetcher fetcher))
+    (when args (setq fetcher (lambda () (apply fetcher args))))
+      (prog1
         (funcall xref-show-xrefs-function fetcher
                  `((window . ,(selected-window))
                    (display-action . ,display-action)
-                   (auto-jump . ,xref-auto-jump-to-first-xref)))
+                   (auto-jump . ,xref-auto-jump-to-first-xref)
+                   (orig-fetcher . ,orig-fetcher)
+                   (fetcher-args . ,args)))
       (xref--push-markers cb pt))))

 (defun xref--show-defs (xrefs display-action)