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

[Dmitry Gutov <dmitry@gutov.dev>]

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.