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

[Eshel Yaron]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 12/02/2024 13:45, Eshel Yaron wrote:
>
>> 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.

Often callers can use xref-make-fetcher to make the fetcher function,
and that takes care of the redundant work for them.  That's was I did
for project-find-regexp and friends in my working branch, works well :)

[ BTW, while at it, I noticed that the docstring for
  project-or-external-find-regexp mentions a prefix argument, but the
  function doesn't actually handle one. ]

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

That might work, although it seems rather difficult to explain such
requirements, and it's difficult for callers to ensure or even check
whether they're kept (how do you know if your argument is too big
without printing it in advance?)

Furthermore, IIUC, what you get is an opaque function and argument list,
and the frontend cannot reason about these, it can only apply the
function to these arguments to get a list of xrefs.  In contrast,
xref-fetcher-alist provides clear (documented) semantics.  We use it for
bookmarking first and foremost, but the frontend can legitimately use it
for other stuff too, like showing some info in the mode line.

> Also, I'm not sure how we're supposed to guarantee that
> xref--original-buffer is live.

In my patch, we don't guarantee that (see xref-bookmark-make-record).
And that's fine, it's a best effort to give the backend all the context
it might need.  If there's no original buffer, we just don't save and
restore that bit of context.  The backend can handle a nil CONTEXT
argument in xref-backend-restore however it sees fit.  By default, it
does nothing.

> Is that for use with desktop-mode only?

What do you mean?  To be clear, this is unrelated to desktop-mode, or at
least I didn't design/implement any of this with desktop-mode in mind.

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

We don't keep the value of point as such, we use the standard bookmark
facilities to save some context around point so we can relocate to the
right place if something changes.  If we can't find that context when
restoring the bookmark, point is just left at the beginning of the
*xref* buffer.  That's also fine.  Does that make sense?