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

[Dmitry Gutov]

On 13/02/2024 09:10, Eshel Yaron wrote:
> 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. ]

Thank you for noting, now fixed.

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

You can usually track that on the level of user input. A good rule of 
thumb would be not to pass a generated list of files. And if some user's 
interactive input string is veeeeeery long, well, whatever disk space is 
wasted as a result is their own doing.

What's the alternative, though? Writing a separate bookmark storage 
function for every sort of search? For project, lsp-mode/eglot (they 
both have additional commands doing extra searches), etc?

And the return value of xref-backend-context (from your proposal) must 
likewise be print-able and compact enough, right?

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

Which will only work for Xref's own commands but not for any external 
callers of xref-show-xrefs. Right?

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

Okay, I see that. Basically, you bookmark the "original point" and then 
restore it from xref-backend-restore. So this would work, most of the time.

> The backend can handle a nil CONTEXT
> argument in xref-backend-restore however it sees fit.  By default, it
> does nothing.

I don't any LSP backend could handle nil, though. It would need 
additional data, like the origin file name, the value of point, etc.

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

I meant that if you require the original buffer to be available when the 
bookmark is loaded, the easiest way to satisfy that is for desktop-mode 
to be used. But I see you solved that in a different way.

> > 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?

I meant the position of point in the original buffer, not in the Xref 
buffer, which is required for the Xref searches to work in LSP backends.

I suppose the same bookmark mechanism would be used, too, though.