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

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

Tags: patch

Hello Dmitry, All,

This patch adds support for bookmarking "*xref*" buffers and restoring
them later, even across Emacs sessions.

To make this happen, we need to propagate some more information to the
"*xref*" buffer (and any other Xref fronted).  We do this, without
breaking compatibility, by setting a new variable from inside the xrefs
fetcher function.  The frontend can examine this variable to learn all
about the source of the fetched xrefs after invoking the fetcher.
Namely, the "*xref*" buffer uses this information to create bookmarks.

WDYT?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-bookmarking-Xref-results-buffers.patch --]
[-- Type: text/patch, Size: 15350 bytes --]

From 62f76297ec240df8101ab47fa4a89fa584b7f05c Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Tue, 6 Feb 2024 20:33:53 +0100
Subject: [PATCH] Support bookmarking Xref results buffers

* lisp/progmodes/xref.el (bookmark-make-record-default)
(bookmark-make-record, bookmark-prop-get)
(bookmark-handle-bookmark, bookmark-get-rear-context-string)
(bookmark-get-front-context-string): Declare functions.
(xref-backend-context, xref-backend-restore): New generic functions.
(xref--backend, xref--identifier, xref--kind)
(xref--original-buffer, xref--original-point): New local variables.
(xref--show-common-initialize): Set them in Xref results buffer.
(xref-default-bookmark-name-format): New user option.
(xref-bookmark-make-record, xref-bookmark-jump): New functions.
(xref--xref-buffer-mode): Set 'bookmark-make-record-function'.
(xref-fetcher-alist): New variable.
(xref--show-xref-buffer, xref-show-definitions-buffer)
(xref-show-definitions-buffer-at-bottom): Use it.
(xref--read-identifier): Improve error message.
(xref-make-fetcher): Extract from...
(xref--create-fetcher): ...here.

* doc/emacs/maintaining.texi (Xref Commands): Document new feature.

* etc/NEWS: Announce new feature and Xref generic functions.
---
 doc/emacs/maintaining.texi |   4 +
 etc/NEWS                   |  12 +++
 lisp/progmodes/xref.el     | 166 +++++++++++++++++++++++++++++++++----
 3 files changed, 165 insertions(+), 17 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index d3e06fa697b..dd3fb3c2dd2 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -2466,6 +2466,10 @@ Xref Commands
 @kbd{C-n}, and @kbd{C-p} are available for moving around the buffer
 without displaying the references.
 
+You can also bookmark the @file{*xref*} buffer with @kbd{C-x r m} and
+restore it from the same state later by jumping to that bookmark with
+@kbd{C-x r b}.  @xref{Bookmarks}.
+
 @node Identifier Search
 @subsubsection Searching and Replacing with Identifiers
 @cindex search and replace in multiple source files
diff --git a/etc/NEWS b/etc/NEWS
index f980d612a57..a33a0e9855b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -437,6 +437,11 @@ This mode now emits 'wheel-up/down/right/left' events instead of
 It uses the 'mouse-wheel-up/down/left/right-event'
 variables to decide which button maps to which wheel event (if any).
 
+** Xref
+
++++
+*** You can now bookmark (and later restore) "*xref*" buffers.
+
 ** Info
 
 ---
@@ -1664,6 +1669,13 @@ styles to skip eager fontification of completion candidates, which
 improves performance.  Such a Lisp program can then use the
 'completion-lazy-hilit' function to fontify candidates just in time.
 
+** New Xref generic functions for recording and restoring context.
+Xref backends can now implement the generic function
+'xref-backend-context' to change how Xref records the context used for
+fetching cross-references when bookmarking Xref results for later use.
+In addition, the new generic function 'xref-backend-restore' lets
+backends change how Xref then restores this context.
+
 ** Functions and variables to transpose sexps
 
 +++
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 717b837a2e5..249e018eb56 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -314,6 +314,21 @@ xref-backend-identifier-completion-ignore-case
   "Return t if case is not significant in identifier completion."
   completion-ignore-case)
 
+(declare-function bookmark-make-record              "bookmark")
+(declare-function bookmark-make-record-default      "bookmark")
+(declare-function bookmark-prop-get                 "bookmark")
+(declare-function bookmark-handle-bookmark          "bookmark")
+(declare-function bookmark-get-rear-context-string  "bookmark")
+(declare-function bookmark-get-front-context-string "bookmark")
+
+(cl-defgeneric xref-backend-context (_backend _identifier _kind)
+  "Return BACKEND-specific context for finding references to IDENTIFIER."
+  (bookmark-make-record))
+
+(cl-defgeneric xref-backend-restore (_backend context)
+  "Restore BACKEND-specific CONTEXT."
+  (bookmark-handle-bookmark context))
+
 \f
 ;;; misc utilities
 (defun xref--alistify (list key)
@@ -671,6 +686,23 @@ xref--original-window
 (defvar-local xref--fetcher nil
   "The original function to call to fetch the list of xrefs.")
 
+(defvar-local xref--backend nil
+  "The backend that produced the xrefs that the current buffer is showing.")
+
+(defvar-local xref--identifier nil
+  "The identifier for which the current buffer is showing xrefs.")
+
+(defvar-local xref--kind nil
+  "The kind of xrefs the current buffer is showing.")
+
+(defvar-local xref--original-buffer nil
+  "Buffer in which the Xref command that created this buffer was called.")
+
+(defvar-local xref--original-point nil
+  "Position in which the Xref command that created this buffer was called.
+
+See also `xref--original-buffer'.")
+
 (defun xref--show-pos-in-buf (pos buf)
   "Goto and display position POS of buffer BUF in a window.
 Honor `xref--original-window-intent', run `xref-after-jump-hook'
@@ -997,6 +1029,63 @@ xref--xref-buffer-mode-map
     (define-key map (kbd "M-,") #'xref-quit-and-pop-marker-stack)
     map))
 
+(defcustom xref-default-bookmark-name-format "%i %k"
+  "Format of the default bookmark name for Xref buffer bookmarks.
+
+The default bookmark name is the value of this option (a string), with
+\"%i\" sequences substituted for the identifier that the Xref buffer is
+showing information about, \"%k\" substituted with the kind of
+information shown (\"references\", \"definitions\", etc.), and \"%b\"
+substituted for the name of the backend that produced the information."
+  :type 'string
+  :version "30.1")
+
+(defun xref-bookmark-make-record ()
+  "Return a bookmark record for bookmarking the current Xref buffer.
+
+This function is used as the value of `bookmark-make-record-function' in
+Xref buffers."
+  (unless xref--backend
+    (user-error "Cannot bookmark due to unknown Xref backend"))
+  `(,(format-spec xref-default-bookmark-name-format
+                  `((?i . ,xref--identifier)
+                    (?k . ,xref--kind)
+                    (?b . ,xref--backend)))
+    ,@(bookmark-make-record-default t)
+    (backend . ,xref--backend)
+    (context . ,(when (buffer-live-p xref--original-buffer)
+                  (with-current-buffer xref--original-buffer
+                    (save-excursion
+                      (ignore-errors (goto-char xref--original-point))
+                      (xref-backend-context xref--backend
+                                            xref--identifier
+                                            xref--kind)))))
+    (identifier . ,xref--identifier)
+    (kind . ,xref--kind)
+    (handler . xref-bookmark-jump)))
+
+(defun xref-bookmark-jump (bookmark)
+  "Jump to Xref buffer bookmark BOOKMARK."
+  (let* ((backend (bookmark-prop-get bookmark 'backend))
+         (context (bookmark-prop-get bookmark 'context))
+         (identifier (bookmark-prop-get bookmark 'identifier))
+         (kind (bookmark-prop-get bookmark 'kind))
+         (fetcher (save-current-buffer
+                    (xref-backend-restore backend context)
+                    (xref-make-fetcher backend identifier kind identifier
+                                       (current-buffer) (point))))
+         (xref-auto-jump-to-first-xref nil))
+    (set-buffer (xref--show-xref-buffer fetcher nil))
+    (let ((forward-str (bookmark-get-front-context-string bookmark))
+          (behind-str (bookmark-get-rear-context-string bookmark)))
+      (when (and forward-str (search-forward forward-str (point-max) t))
+        (goto-char (match-beginning 0)))
+      (when (and behind-str (search-backward behind-str (point-min) t))
+        (goto-char (match-end 0)))
+      nil)))
+
+(put 'xref-bookmark-jump 'bookmark-handler-type "Xref")
+
 (declare-function outline-search-text-property "outline"
                   (property &optional value bound move backward looking-at))
 
@@ -1017,7 +1106,8 @@ xref--xref-buffer-mode
               (lambda (&optional bound move backward looking-at)
                 (outline-search-text-property
                  'xref-group nil bound move backward looking-at)))
-  (setq-local outline-level (lambda () 1)))
+  (setq-local outline-level (lambda () 1))
+  (setq-local bookmark-make-record-function #'xref-bookmark-make-record))
 
 (defvar xref--transient-buffer-mode-map
   (let ((map (make-sparse-keymap)))
@@ -1235,11 +1325,29 @@ xref--ensure-default-directory
    0 nil
    (lambda () (with-current-buffer buffer (setq default-directory dd)))))
 
+(defvar xref-fetcher-alist nil
+  "Alist with information about the last used Xref fetcher function.
+
+Fetcher functions which Xref passes to `xref-show-xrefs-function' set
+this variable to an alist with the following key-value pairs:
+
+- (backend . BACKEND) where BACKEND is the Xref backend that the
+  fetcher invokes.
+- (identifier . ID) where ID is the identifier for which the fetcher
+  fetches references.
+- (kind . KIND) where KIND is the kind of references that the fetcher
+  fetches.
+- (original-buffer . BUF) where BUF is the buffer in which the Xref
+  command that created the fetcher was invoked.
+- (original-point . POS) where POS is the buffer position in which the
+  Xref command that created the fetcher was invoked.")
+
 (defun xref--show-xref-buffer (fetcher alist)
   (cl-assert (functionp fetcher))
   (let* ((xrefs
           (or
            (assoc-default 'fetched-xrefs alist)
+           (setq xref-fetcher-alist nil)
            (funcall fetcher)))
          (xref-alist (xref--analyze xrefs))
          (dd default-directory)
@@ -1247,7 +1355,7 @@ xref--show-xref-buffer
     (with-current-buffer (get-buffer-create xref-buffer-name)
       (xref--ensure-default-directory dd (current-buffer))
       (xref--xref-buffer-mode)
-      (xref--show-common-initialize xref-alist fetcher alist)
+      (xref--show-common-initialize xref-alist fetcher (append xref-fetcher-alist alist))
       (setq xref-num-matches-found (length xrefs))
       (setq mode-line-process (list xref-mode-line-matches))
       (pop-to-buffer (current-buffer))
@@ -1272,7 +1380,12 @@ xref--show-common-initialize
     (add-hook 'post-command-hook 'xref--apply-truncation nil t)
     (goto-char (point-min))
     (setq xref--original-window (assoc-default 'window alist)
-          xref--original-window-intent (assoc-default 'display-action alist))
+          xref--original-window-intent (assoc-default 'display-action alist)
+          xref--original-buffer (assoc-default 'original-buffer alist)
+          xref--original-point (assoc-default 'original-point alist)
+          xref--backend (assoc-default 'backend alist)
+          xref--identifier (assoc-default 'identifier alist)
+          xref--kind (assoc-default 'kind alist))
     (setq xref--fetcher fetcher)))
 
 (defun xref-revert-buffer ()
@@ -1310,6 +1423,7 @@ xref-show-definitions-buffer
   "Show the definitions list in a regular window.
 
 When only one definition found, jump to it right away instead."
+  (setq xref-fetcher-alist nil)
   (let ((xrefs (funcall fetcher))
         buf)
     (cond
@@ -1333,6 +1447,7 @@ xref-show-definitions-buffer-at-bottom
 When there is more than one definition, split the selected window
 and show the list in a small window at the bottom.  And use a
 local keymap that binds `RET' to `xref-quit-and-goto-xref'."
+  (setq xref-fetcher-alist nil)
   (let* ((xrefs (funcall fetcher))
          (dd default-directory)
          ;; XXX: Make percentage customizable maybe?
@@ -1353,7 +1468,7 @@ xref-show-definitions-buffer-at-bottom
       (with-current-buffer (get-buffer-create xref-buffer-name)
         (xref--ensure-default-directory dd (current-buffer))
         (xref--transient-buffer-mode)
-        (xref--show-common-initialize xref-alist fetcher alist)
+        (xref--show-common-initialize xref-alist fetcher (append xref-fetcher-alist alist))
         (pop-to-buffer (current-buffer)
                        `(display-buffer-in-direction . ((direction . below)
                                                         (window-height . ,size-fun))))
@@ -1552,7 +1667,7 @@ xref--read-identifier
                    nil nil nil
                    'xref--read-identifier-history def t)))
              (if (equal id "")
-                 (or def (user-error "There is no default identifier"))
+                 (or def (user-error "No default identifier"))
                id)))
           (t def))))
 
@@ -1569,16 +1684,23 @@ xref--find-definitions
    (xref--create-fetcher id 'definitions id)
    display-action))
 
-(defun xref--create-fetcher (input kind arg)
-  "Return an xref list fetcher function.
+(defun xref-make-fetcher (backend input kind identifier buffer point)
+  "Return fetcher function for xrefs of kind KIND for IDENTIFIER using BACKEND.
 
-It revisits the saved position and delegates the finding logic to
-the xref backend method indicated by KIND and passes ARG to it."
-  (let* ((orig-buffer (current-buffer))
-         (orig-position (point))
-         (backend (xref-find-backend))
-         (method (intern (format "xref-backend-%s" kind))))
+INPUT is the user input for the Xref operation, usually it is the same
+as IDENTIFIER, but the two may differ when KIND is `apropos'.  BUFFER
+and POINT are the buffer and specific position in which the xref
+operation was invoked.
+
+The fetcher function returns a list of xrefs, and sets
+`xref-fetcher-alist', which see."
+  (let ((method (intern (format "xref-backend-%s" kind))))
     (lambda ()
+      (setq xref-fetcher-alist (list (cons 'original-buffer buffer)
+                                     (cons 'original-point point)
+                                     (cons 'backend backend)
+                                     (cons 'identifier identifier)
+                                     (cons 'kind kind)))
       (save-excursion
         ;; Xref methods are generally allowed to depend on the text
         ;; around point, not just on their explicit arguments.
@@ -1586,14 +1708,24 @@ xref--create-fetcher
         ;; There is only so much we can do, however, to recreate that
         ;; context, given that the user is free to change the buffer
         ;; contents freely in the meantime.
-        (when (buffer-live-p orig-buffer)
-          (set-buffer orig-buffer)
-          (ignore-errors (goto-char orig-position)))
-        (let ((xrefs (funcall method backend arg)))
+        (when (buffer-live-p buffer)
+          (set-buffer buffer)
+          (ignore-errors (goto-char point)))
+        (let ((xrefs (funcall method backend identifier)))
           (unless xrefs
             (xref--not-found-error kind input))
           xrefs)))))
 
+(defun xref--create-fetcher (input kind arg)
+  "Return an xref list fetcher function.
+
+It revisits the saved position and delegates the finding logic to
+the xref backend method indicated by KIND and passes ARG to it."
+  (xref-make-fetcher (xref-find-backend)
+                     input kind arg
+                     (current-buffer)
+                     (copy-marker (point))))
+
 (defun xref--not-found-error (kind input)
   (user-error "No %s found for: %s" (symbol-name kind) input))
 
-- 
2.42.0

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

[Eli Zaretskii]

> Cc: Dmitry Gutov <dmitry@gutov.dev>
> Date: Tue, 06 Feb 2024 21:17:45 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This patch adds support for bookmarking "*xref*" buffers and restoring
> them later, even across Emacs sessions.
> 
> To make this happen, we need to propagate some more information to the
> "*xref*" buffer (and any other Xref fronted).  We do this, without
> breaking compatibility, by setting a new variable from inside the xrefs
> fetcher function.  The frontend can examine this variable to learn all
> about the source of the fetched xrefs after invoking the fetcher.
> Namely, the "*xref*" buffer uses this information to create bookmarks.

Thanks.  Frankly, I'm surprised we need such a complex changeset for
supporting such a simple extension, but I'll let Dmitry judge that.

> --- a/doc/emacs/maintaining.texi
> +++ b/doc/emacs/maintaining.texi
> @@ -2466,6 +2466,10 @@ Xref Commands
>  @kbd{C-n}, and @kbd{C-p} are available for moving around the buffer
>  without displaying the references.
>  
> +You can also bookmark the @file{*xref*} buffer with @kbd{C-x r m} and
> +restore it from the same state later by jumping to that bookmark with
> +@kbd{C-x r b}.  @xref{Bookmarks}.

Since "C-x r m" and "C-x r b" are bookmark commands, they should not
be described here; instead, its description in "Bookmarks" should
mention any special features related to the Xref buffers (not that I
see what is there to mention, but maybe I'm missing something).  If
you think this capability is worth mentioning in the "Xref Commands"
node, you should do it in passage, like

  You can bookmark and restore your place in @file{*xref*} buffers,
  see @ref{Bookmarks}.

> +** New Xref generic functions for recording and restoring context.
> +Xref backends can now implement the generic function
> +'xref-backend-context' to change how Xref records the context used for
> +fetching cross-references when bookmarking Xref results for later use.
> +In addition, the new generic function 'xref-backend-restore' lets
> +backends change how Xref then restores this context.

I'm not sure this is for NEWS.  Either expand the documentation, place
it in the ELisp manual, and just mention the function's name in NEWS,
or simply don't mention it in NEWS.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 2746 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Dmitry Gutov <dmitry@gutov.dev>
>> Date: Tue, 06 Feb 2024 21:17:45 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> This patch adds support for bookmarking "*xref*" buffers and restoring
>> them later, even across Emacs sessions.
>>
>> To make this happen, we need to propagate some more information to the
>> "*xref*" buffer (and any other Xref fronted).  We do this, without
>> breaking compatibility, by setting a new variable from inside the xrefs
>> fetcher function.  The frontend can examine this variable to learn all
>> about the source of the fetched xrefs after invoking the fetcher.
>> Namely, the "*xref*" buffer uses this information to create bookmarks.
>
> Thanks.  Frankly, I'm surprised we need such a complex changeset for
> supporting such a simple extension, but I'll let Dmitry judge that.

Thanks for taking a look.

It's a simple extension, but unless I'm missing something in the code,
the Xref infrastructure needs these extra tweaks to accommodate for it.

>> --- a/doc/emacs/maintaining.texi
>> +++ b/doc/emacs/maintaining.texi
>> @@ -2466,6 +2466,10 @@ Xref Commands
>>  @kbd{C-n}, and @kbd{C-p} are available for moving around the buffer
>>  without displaying the references.
>>
>> +You can also bookmark the @file{*xref*} buffer with @kbd{C-x r m} and
>> +restore it from the same state later by jumping to that bookmark with
>> +@kbd{C-x r b}.  @xref{Bookmarks}.
>
> Since "C-x r m" and "C-x r b" are bookmark commands, they should not
> be described here; instead, its description in "Bookmarks" should
> mention any special features related to the Xref buffers (not that I
> see what is there to mention, but maybe I'm missing something).  If
> you think this capability is worth mentioning in the "Xref Commands"
> node, you should do it in passage, like
>
>   You can bookmark and restore your place in @file{*xref*} buffers,
>   see @ref{Bookmarks}.

That makes sense.  I updated the text accordingly in the attached patch.

>> +** New Xref generic functions for recording and restoring context.
>> +Xref backends can now implement the generic function
>> +'xref-backend-context' to change how Xref records the context used for
>> +fetching cross-references when bookmarking Xref results for later use.
>> +In addition, the new generic function 'xref-backend-restore' lets
>> +backends change how Xref then restores this context.
>
> I'm not sure this is for NEWS.  Either expand the documentation, place
> it in the ELisp manual, and just mention the function's name in NEWS,
> or simply don't mention it in NEWS.

All right, I removed this bit.

Here's the updated patch (v2):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-Support-bookmarking-Xref-results-buffers.patch --]
[-- Type: text/x-patch, Size: 14574 bytes --]

From 51b748c4dd97cb60e7328b6510e8dd55da4a75ce Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Tue, 6 Feb 2024 20:33:53 +0100
Subject: [PATCH v2] Support bookmarking Xref results buffers

* lisp/progmodes/xref.el (bookmark-make-record-default)
(bookmark-make-record, bookmark-prop-get)
(bookmark-handle-bookmark, bookmark-get-rear-context-string)
(bookmark-get-front-context-string): Declare functions.
(xref-backend-context, xref-backend-restore): New generic functions.
(xref--backend, xref--identifier, xref--kind)
(xref--original-buffer, xref--original-point): New local variables.
(xref--show-common-initialize): Set them in Xref results buffer.
(xref-default-bookmark-name-format): New user option.
(xref-bookmark-make-record, xref-bookmark-jump): New functions.
(xref--xref-buffer-mode): Set 'bookmark-make-record-function'.
(xref-fetcher-alist): New variable.
(xref--show-xref-buffer, xref-show-definitions-buffer)
(xref-show-definitions-buffer-at-bottom): Use it.
(xref--read-identifier): Improve error message.
(xref-make-fetcher): Extract from...
(xref--create-fetcher): ...here.

* doc/emacs/maintaining.texi (Xref Commands): Document new feature.

* etc/NEWS: Announce it.  (Bug#68958)
---
 doc/emacs/maintaining.texi |   3 +
 etc/NEWS                   |   5 ++
 lisp/progmodes/xref.el     | 166 +++++++++++++++++++++++++++++++++----
 3 files changed, 157 insertions(+), 17 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index d3e06fa697b..4cd02851594 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -2466,6 +2466,9 @@ Xref Commands
 @kbd{C-n}, and @kbd{C-p} are available for moving around the buffer
 without displaying the references.
 
+You can bookmark and restore your place in @file{*xref*} buffers, see
+@ref{Bookmarks}.
+
 @node Identifier Search
 @subsubsection Searching and Replacing with Identifiers
 @cindex search and replace in multiple source files
diff --git a/etc/NEWS b/etc/NEWS
index f980d612a57..500433ff955 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -437,6 +437,11 @@ This mode now emits 'wheel-up/down/right/left' events instead of
 It uses the 'mouse-wheel-up/down/left/right-event'
 variables to decide which button maps to which wheel event (if any).
 
+** Xref
+
++++
+*** You can now bookmark (and later restore) "*xref*" buffers.
+
 ** Info
 
 ---
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 717b837a2e5..249e018eb56 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -314,6 +314,21 @@ xref-backend-identifier-completion-ignore-case
   "Return t if case is not significant in identifier completion."
   completion-ignore-case)
 
+(declare-function bookmark-make-record              "bookmark")
+(declare-function bookmark-make-record-default      "bookmark")
+(declare-function bookmark-prop-get                 "bookmark")
+(declare-function bookmark-handle-bookmark          "bookmark")
+(declare-function bookmark-get-rear-context-string  "bookmark")
+(declare-function bookmark-get-front-context-string "bookmark")
+
+(cl-defgeneric xref-backend-context (_backend _identifier _kind)
+  "Return BACKEND-specific context for finding references to IDENTIFIER."
+  (bookmark-make-record))
+
+(cl-defgeneric xref-backend-restore (_backend context)
+  "Restore BACKEND-specific CONTEXT."
+  (bookmark-handle-bookmark context))
+
 \f
 ;;; misc utilities
 (defun xref--alistify (list key)
@@ -671,6 +686,23 @@ xref--original-window
 (defvar-local xref--fetcher nil
   "The original function to call to fetch the list of xrefs.")
 
+(defvar-local xref--backend nil
+  "The backend that produced the xrefs that the current buffer is showing.")
+
+(defvar-local xref--identifier nil
+  "The identifier for which the current buffer is showing xrefs.")
+
+(defvar-local xref--kind nil
+  "The kind of xrefs the current buffer is showing.")
+
+(defvar-local xref--original-buffer nil
+  "Buffer in which the Xref command that created this buffer was called.")
+
+(defvar-local xref--original-point nil
+  "Position in which the Xref command that created this buffer was called.
+
+See also `xref--original-buffer'.")
+
 (defun xref--show-pos-in-buf (pos buf)
   "Goto and display position POS of buffer BUF in a window.
 Honor `xref--original-window-intent', run `xref-after-jump-hook'
@@ -997,6 +1029,63 @@ xref--xref-buffer-mode-map
     (define-key map (kbd "M-,") #'xref-quit-and-pop-marker-stack)
     map))
 
+(defcustom xref-default-bookmark-name-format "%i %k"
+  "Format of the default bookmark name for Xref buffer bookmarks.
+
+The default bookmark name is the value of this option (a string), with
+\"%i\" sequences substituted for the identifier that the Xref buffer is
+showing information about, \"%k\" substituted with the kind of
+information shown (\"references\", \"definitions\", etc.), and \"%b\"
+substituted for the name of the backend that produced the information."
+  :type 'string
+  :version "30.1")
+
+(defun xref-bookmark-make-record ()
+  "Return a bookmark record for bookmarking the current Xref buffer.
+
+This function is used as the value of `bookmark-make-record-function' in
+Xref buffers."
+  (unless xref--backend
+    (user-error "Cannot bookmark due to unknown Xref backend"))
+  `(,(format-spec xref-default-bookmark-name-format
+                  `((?i . ,xref--identifier)
+                    (?k . ,xref--kind)
+                    (?b . ,xref--backend)))
+    ,@(bookmark-make-record-default t)
+    (backend . ,xref--backend)
+    (context . ,(when (buffer-live-p xref--original-buffer)
+                  (with-current-buffer xref--original-buffer
+                    (save-excursion
+                      (ignore-errors (goto-char xref--original-point))
+                      (xref-backend-context xref--backend
+                                            xref--identifier
+                                            xref--kind)))))
+    (identifier . ,xref--identifier)
+    (kind . ,xref--kind)
+    (handler . xref-bookmark-jump)))
+
+(defun xref-bookmark-jump (bookmark)
+  "Jump to Xref buffer bookmark BOOKMARK."
+  (let* ((backend (bookmark-prop-get bookmark 'backend))
+         (context (bookmark-prop-get bookmark 'context))
+         (identifier (bookmark-prop-get bookmark 'identifier))
+         (kind (bookmark-prop-get bookmark 'kind))
+         (fetcher (save-current-buffer
+                    (xref-backend-restore backend context)
+                    (xref-make-fetcher backend identifier kind identifier
+                                       (current-buffer) (point))))
+         (xref-auto-jump-to-first-xref nil))
+    (set-buffer (xref--show-xref-buffer fetcher nil))
+    (let ((forward-str (bookmark-get-front-context-string bookmark))
+          (behind-str (bookmark-get-rear-context-string bookmark)))
+      (when (and forward-str (search-forward forward-str (point-max) t))
+        (goto-char (match-beginning 0)))
+      (when (and behind-str (search-backward behind-str (point-min) t))
+        (goto-char (match-end 0)))
+      nil)))
+
+(put 'xref-bookmark-jump 'bookmark-handler-type "Xref")
+
 (declare-function outline-search-text-property "outline"
                   (property &optional value bound move backward looking-at))
 
@@ -1017,7 +1106,8 @@ xref--xref-buffer-mode
               (lambda (&optional bound move backward looking-at)
                 (outline-search-text-property
                  'xref-group nil bound move backward looking-at)))
-  (setq-local outline-level (lambda () 1)))
+  (setq-local outline-level (lambda () 1))
+  (setq-local bookmark-make-record-function #'xref-bookmark-make-record))
 
 (defvar xref--transient-buffer-mode-map
   (let ((map (make-sparse-keymap)))
@@ -1235,11 +1325,29 @@ xref--ensure-default-directory
    0 nil
    (lambda () (with-current-buffer buffer (setq default-directory dd)))))
 
+(defvar xref-fetcher-alist nil
+  "Alist with information about the last used Xref fetcher function.
+
+Fetcher functions which Xref passes to `xref-show-xrefs-function' set
+this variable to an alist with the following key-value pairs:
+
+- (backend . BACKEND) where BACKEND is the Xref backend that the
+  fetcher invokes.
+- (identifier . ID) where ID is the identifier for which the fetcher
+  fetches references.
+- (kind . KIND) where KIND is the kind of references that the fetcher
+  fetches.
+- (original-buffer . BUF) where BUF is the buffer in which the Xref
+  command that created the fetcher was invoked.
+- (original-point . POS) where POS is the buffer position in which the
+  Xref command that created the fetcher was invoked.")
+
 (defun xref--show-xref-buffer (fetcher alist)
   (cl-assert (functionp fetcher))
   (let* ((xrefs
           (or
            (assoc-default 'fetched-xrefs alist)
+           (setq xref-fetcher-alist nil)
            (funcall fetcher)))
          (xref-alist (xref--analyze xrefs))
          (dd default-directory)
@@ -1247,7 +1355,7 @@ xref--show-xref-buffer
     (with-current-buffer (get-buffer-create xref-buffer-name)
       (xref--ensure-default-directory dd (current-buffer))
       (xref--xref-buffer-mode)
-      (xref--show-common-initialize xref-alist fetcher alist)
+      (xref--show-common-initialize xref-alist fetcher (append xref-fetcher-alist alist))
       (setq xref-num-matches-found (length xrefs))
       (setq mode-line-process (list xref-mode-line-matches))
       (pop-to-buffer (current-buffer))
@@ -1272,7 +1380,12 @@ xref--show-common-initialize
     (add-hook 'post-command-hook 'xref--apply-truncation nil t)
     (goto-char (point-min))
     (setq xref--original-window (assoc-default 'window alist)
-          xref--original-window-intent (assoc-default 'display-action alist))
+          xref--original-window-intent (assoc-default 'display-action alist)
+          xref--original-buffer (assoc-default 'original-buffer alist)
+          xref--original-point (assoc-default 'original-point alist)
+          xref--backend (assoc-default 'backend alist)
+          xref--identifier (assoc-default 'identifier alist)
+          xref--kind (assoc-default 'kind alist))
     (setq xref--fetcher fetcher)))
 
 (defun xref-revert-buffer ()
@@ -1310,6 +1423,7 @@ xref-show-definitions-buffer
   "Show the definitions list in a regular window.
 
 When only one definition found, jump to it right away instead."
+  (setq xref-fetcher-alist nil)
   (let ((xrefs (funcall fetcher))
         buf)
     (cond
@@ -1333,6 +1447,7 @@ xref-show-definitions-buffer-at-bottom
 When there is more than one definition, split the selected window
 and show the list in a small window at the bottom.  And use a
 local keymap that binds `RET' to `xref-quit-and-goto-xref'."
+  (setq xref-fetcher-alist nil)
   (let* ((xrefs (funcall fetcher))
          (dd default-directory)
          ;; XXX: Make percentage customizable maybe?
@@ -1353,7 +1468,7 @@ xref-show-definitions-buffer-at-bottom
       (with-current-buffer (get-buffer-create xref-buffer-name)
         (xref--ensure-default-directory dd (current-buffer))
         (xref--transient-buffer-mode)
-        (xref--show-common-initialize xref-alist fetcher alist)
+        (xref--show-common-initialize xref-alist fetcher (append xref-fetcher-alist alist))
         (pop-to-buffer (current-buffer)
                        `(display-buffer-in-direction . ((direction . below)
                                                         (window-height . ,size-fun))))
@@ -1552,7 +1667,7 @@ xref--read-identifier
                    nil nil nil
                    'xref--read-identifier-history def t)))
              (if (equal id "")
-                 (or def (user-error "There is no default identifier"))
+                 (or def (user-error "No default identifier"))
                id)))
           (t def))))
 
@@ -1569,16 +1684,23 @@ xref--find-definitions
    (xref--create-fetcher id 'definitions id)
    display-action))
 
-(defun xref--create-fetcher (input kind arg)
-  "Return an xref list fetcher function.
+(defun xref-make-fetcher (backend input kind identifier buffer point)
+  "Return fetcher function for xrefs of kind KIND for IDENTIFIER using BACKEND.
 
-It revisits the saved position and delegates the finding logic to
-the xref backend method indicated by KIND and passes ARG to it."
-  (let* ((orig-buffer (current-buffer))
-         (orig-position (point))
-         (backend (xref-find-backend))
-         (method (intern (format "xref-backend-%s" kind))))
+INPUT is the user input for the Xref operation, usually it is the same
+as IDENTIFIER, but the two may differ when KIND is `apropos'.  BUFFER
+and POINT are the buffer and specific position in which the xref
+operation was invoked.
+
+The fetcher function returns a list of xrefs, and sets
+`xref-fetcher-alist', which see."
+  (let ((method (intern (format "xref-backend-%s" kind))))
     (lambda ()
+      (setq xref-fetcher-alist (list (cons 'original-buffer buffer)
+                                     (cons 'original-point point)
+                                     (cons 'backend backend)
+                                     (cons 'identifier identifier)
+                                     (cons 'kind kind)))
       (save-excursion
         ;; Xref methods are generally allowed to depend on the text
         ;; around point, not just on their explicit arguments.
@@ -1586,14 +1708,24 @@ xref--create-fetcher
         ;; There is only so much we can do, however, to recreate that
         ;; context, given that the user is free to change the buffer
         ;; contents freely in the meantime.
-        (when (buffer-live-p orig-buffer)
-          (set-buffer orig-buffer)
-          (ignore-errors (goto-char orig-position)))
-        (let ((xrefs (funcall method backend arg)))
+        (when (buffer-live-p buffer)
+          (set-buffer buffer)
+          (ignore-errors (goto-char point)))
+        (let ((xrefs (funcall method backend identifier)))
           (unless xrefs
             (xref--not-found-error kind input))
           xrefs)))))
 
+(defun xref--create-fetcher (input kind arg)
+  "Return an xref list fetcher function.
+
+It revisits the saved position and delegates the finding logic to
+the xref backend method indicated by KIND and passes ARG to it."
+  (xref-make-fetcher (xref-find-backend)
+                     input kind arg
+                     (current-buffer)
+                     (copy-marker (point))))
+
 (defun xref--not-found-error (kind input)
   (user-error "No %s found for: %s" (symbol-name kind) input))
 
-- 
2.42.0

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

[Juri Linkov]

> This patch adds support for bookmarking "*xref*" buffers and restoring
> them later, even across Emacs sessions.

Shouldn't 'revert-buffer-function' in the xref buffer
be sufficient to reconstruct the buffer contents?

Usually modes set buffer-local 'revert-buffer-function'
to a lambda that reruns the top function with previous
arguments.  But it seems xref.el doesn't set it.

I once tried to use 'revert-buffer-function' to restore
xref buffers from the desktop, but abandoned the idea.
Not because xref.el doesn't set 'revert-buffer-function'.
But because it would take too much time to restore
the desktop while it will rerun all saved xref buffers.

OTOH, saving an xref bookmark makes more sense.
And probably your patch will help to implement
'revert-buffer-function' for xref as well.

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

[Dmitry Gutov]

On 07/02/2024 19:25, Juri Linkov wrote:
>> This patch adds support for bookmarking "*xref*" buffers and restoring
>> them later, even across Emacs sessions.
> 
> Shouldn't 'revert-buffer-function' in the xref buffer
> be sufficient to reconstruct the buffer contents?
> 
> Usually modes set buffer-local 'revert-buffer-function'
> to a lambda that reruns the top function with previous
> arguments.  But it seems xref.el doesn't set it.

I can't find my original cause for not using this variable here, so if 
replacing the command xref-revert-buffer with setting 
revert-buffer-function makes things easier for someone, I'm all for it.

> I once tried to use 'revert-buffer-function' to restore
> xref buffers from the desktop, but abandoned the idea.
> Not because xref.el doesn't set 'revert-buffer-function'.
> But because it would take too much time to restore
> the desktop while it will rerun all saved xref buffers.
> 
> OTOH, saving an xref bookmark makes more sense.
> And probably your patch will help to implement
> 'revert-buffer-function' for xref as well.

I don't think bookmark would save the whole buffer contents. Would it? 
Otherwise, re-running the search(es) seems inevitable.

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

[Dmitry Gutov]

On 07/02/2024 14:25, Eli Zaretskii wrote:
>> To make this happen, we need to propagate some more information to the
>> "*xref*" buffer (and any other Xref fronted).  We do this, without
>> breaking compatibility, by setting a new variable from inside the xrefs
>> fetcher function.  The frontend can examine this variable to learn all
>> about the source of the fetched xrefs after invoking the fetcher.
>> Namely, the "*xref*" buffer uses this information to create bookmarks.
> Thanks.  Frankly, I'm surprised we need such a complex changeset for
> supporting such a simple extension, but I'll let Dmitry judge that.

A lot of changes seem to stem from the desire to add detailed info into 
the bookmarks's name (including the identifier being searched, the 
search type, and the xref backend in use).

At the moment our code doesn't save all of those separately, just 
combines them in xref--fetcher.

So whether the patch has to be complex would depend on whether we really 
need to have bookmark names look exactly like proposed. Though I'd 
rewrite it a little even in that case.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 07/02/2024 14:25, Eli Zaretskii wrote:
>>> To make this happen, we need to propagate some more information to the
>>> "*xref*" buffer (and any other Xref fronted).  We do this, without
>>> breaking compatibility, by setting a new variable from inside the xrefs
>>> fetcher function.  The frontend can examine this variable to learn all
>>> about the source of the fetched xrefs after invoking the fetcher.
>>> Namely, the "*xref*" buffer uses this information to create bookmarks.
>> Thanks.  Frankly, I'm surprised we need such a complex changeset for
>> supporting such a simple extension, but I'll let Dmitry judge that.
>
> A lot of changes seem to stem from the desire to add detailed info
> into the bookmarks's name (including the identifier being searched,
> the search type, and the xref backend in use).

That's not the case.  The changes are all meant to facilitate creating
bookmarks that you can restore at a later session.  AFAICT the backend,
identifier and search type (kind) are a required for that, no?  We use
them to suggest a meaningful bookmark name, but that's just a bonus.

> At the moment our code doesn't save all of those separately, just
> combines them in xref--fetcher.
>
> So whether the patch has to be complex would depend on whether we
> really need to have bookmark names look exactly like proposed. Though
> I'd rewrite it a little even in that case.

Again, the name of the bookmark is really not the focus here.  We can't
persist the value of xref--fetcher, since it's an anonymous function, so
we get all the info needed to /recreate/ that function to the frontend.
If there's another (simpler?) way to provide this feature, please do tell.


Best,

Eshel

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

I wrote:

> Dmitry Gutov <dmitry@gutov.dev> writes:
>
>> A lot of changes seem to stem from the desire to add detailed info
>> into the bookmarks's name (including the identifier being searched,
>> the search type, and the xref backend in use).
>
> That's not the case.  The changes are all meant to facilitate creating
> bookmarks that you can restore at a later session.  AFAICT the backend,
> identifier and search type (kind) are a required for that, no?  We use
> them to suggest a meaningful bookmark name, but that's just a bonus.
>
>> At the moment our code doesn't save all of those separately, just
>> combines them in xref--fetcher.
>>
>> So whether the patch has to be complex would depend on whether we
>> really need to have bookmark names look exactly like proposed. Though
>> I'd rewrite it a little even in that case.
>
> Again, the name of the bookmark is really not the focus here.  We can't
> persist the value of xref--fetcher, since it's an anonymous function, so
> we get all the info needed to /recreate/ that function to the frontend.
> If there's another (simpler?) way to provide this feature, please do tell.

Perhaps I can explain the reasoning behind this patch a little better:

The goal is to be able to persistently save (bookmark) your state in an
*xref* results buffer, perhaps as part of a long refactoring effort for
some code base, and restore it later, perhaps in another Emacs session.

Basically we want the following to work:

1. Use M-? to get a list of references in the *xref* buffer.
2. Do something with some of them, but not all.
3. Hit C-x r m to bookmark your position.
4. Quit the *xref* buffer, and close Emacs.
5. Open Emacs again, type C-x r b and select a bookmark to get an *xref* buffer
   corresponding to the same search as before, with point on the same
   reference that you where on when you created the bookmark, if possible.

Crucially, looking at step 3, we need to have the data needed to create
such a persistent bookmark available in the *xref* buffer, long after
the execution of the command that created this buffer.  So, what data do
we need?  IIUC, to replicate the saved search we need to invoke the same
backend, with the same type of search, for the same input.  Since Xref
backends may (and do) use the position of point and other buffer context
to guide the search, we want to preserve and restore that extra context
as well.

So in this patch, we add the new xref-fetcher-alist variable that allows
the fetcher function to communicate all this information to the
frontend, so when we create the *xref* buffer we can store it in
buffer-local variables, and then use them to create a bookmark record
when the user types C-x r m.  This record includes all of the data
needed to perform the same Xref search in a new Emacs session, and in
most cases to get back to same position.  We also let the backend
override what extra context exactly gets saved and restored, and how.

Hope this makes it clearer :)

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

[Dmitry Gutov]

On 11/02/2024 08:18, Eshel Yaron wrote:

> Again, the name of the bookmark is really not the focus here.  We can't
> persist the value of xref--fetcher, since it's an anonymous function, so
> we get all the info needed to /recreate/ that function to the frontend.
> If there's another (simpler?) way to provide this feature, please do tell.

All right, that's a good point.

Could we really not persist an anonymous function, though? It can be 
printed and, I suppose, evaluated. At least in theory, whatever links it 
has to containing lexical contexts, should be possible to "detach" when 
writing the literal to disk, to be read later.

The issue with doing this at the level of xref--create-fetcher, is that 
the addition becomes specific to the Xref searches only (find 
definitions/references), and the more generic Xref UI infrastructure 
remains unsupported (such as 'M-x project-find-regexp' or whatever calls 
to xref-show-xrefs exist in third-party packages) -- so those Xref 
buffers would remain not bookmark-able, or they will each require 
specialized code like the one you proposed here.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 11/02/2024 08:18, Eshel Yaron wrote:
>
>> Again, the name of the bookmark is really not the focus here.  We can't
>> persist the value of xref--fetcher, since it's an anonymous function, so
>> we get all the info needed to /recreate/ that function to the frontend.
>> If there's another (simpler?) way to provide this feature, please do tell.
>
> All right, that's a good point.
>
> Could we really not persist an anonymous function, though? It can be
> printed and, I suppose, evaluated. At least in theory, whatever links
> it has to containing lexical contexts, should be possible to "detach"
> when writing the literal to disk, to be read later.

I'm not sure.  It certainly cant' work for arbitrary function objects
(say, if you create a function with 'make_function' in a module).
>
> The issue with doing this at the level of xref--create-fetcher, is
> that the addition becomes specific to the Xref searches only (find
> definitions/references), and the more generic Xref UI infrastructure
> remains unsupported (such as 'M-x project-find-regexp' or whatever
> calls to xref-show-xrefs exist in third-party packages) -- so those
> Xref buffers would remain not bookmark-able, or they will each require
> specialized code like the one you proposed here.

Yes, but such callers of xref-show-xrefs can implement the same API to
have the corresponding *xref* buffer bookmark-able.  Namely, arrange for
the fetcher function to populate xref-fetcher-alist with relevant data.
Indeed, I plan to work on doing that for project-find-regexp next.  If
we had a solution that doesn't require any work from third party to
benefit from this new feature, that'd be even better, of course.  But
since the current API to Xref frontends accepts any fetcher function,
I'm not sure that's possible...

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

[Juri Linkov]

>> I once tried to use 'revert-buffer-function' to restore
>> xref buffers from the desktop, but abandoned the idea.
>> Not because xref.el doesn't set 'revert-buffer-function'.
>> But because it would take too much time to restore
>> the desktop while it will rerun all saved xref buffers.
>> OTOH, saving an xref bookmark makes more sense.
>> And probably your patch will help to implement
>> 'revert-buffer-function' for xref as well.
>
> I don't think bookmark would save the whole buffer contents. Would it?
> Otherwise, re-running the search(es) seems inevitable.

Indeed, saving the buffer contents of transient buffers
either to the bookmark or to the desktop makes no sense.
So re-running the command is inevitable.

But then I can't imagine how would it be possible
to move point to its saved location when initially
a restored buffer is empty before re-running the command.

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

[Dmitry Gutov]

On 11/02/2024 19:37, Juri Linkov wrote:
>>> I once tried to use 'revert-buffer-function' to restore
>>> xref buffers from the desktop, but abandoned the idea.
>>> Not because xref.el doesn't set 'revert-buffer-function'.
>>> But because it would take too much time to restore
>>> the desktop while it will rerun all saved xref buffers.
>>> OTOH, saving an xref bookmark makes more sense.
>>> And probably your patch will help to implement
>>> 'revert-buffer-function' for xref as well.
>> I don't think bookmark would save the whole buffer contents. Would it?
>> Otherwise, re-running the search(es) seems inevitable.
> Indeed, saving the buffer contents of transient buffers
> either to the bookmark or to the desktop makes no sense.
> So re-running the command is inevitable.
> 
> But then I can't imagine how would it be possible
> to move point to its saved location when initially
> a restored buffer is empty before re-running the command.

As long as the search is performed synchronously, first you "revert" the 
buffer and then move point. Right?

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

[Dmitry Gutov]

On 11/02/2024 19:21, Eshel Yaron wrote:
> Dmitry Gutov <dmitry@gutov.dev> writes:
> 
>> On 11/02/2024 08:18, Eshel Yaron wrote:
>>
>>> Again, the name of the bookmark is really not the focus here.  We can't
>>> persist the value of xref--fetcher, since it's an anonymous function, so
>>> we get all the info needed to /recreate/ that function to the frontend.
>>> If there's another (simpler?) way to provide this feature, please do tell.
>>
>> All right, that's a good point.
>>
>> Could we really not persist an anonymous function, though? It can be
>> printed and, I suppose, evaluated. At least in theory, whatever links
>> it has to containing lexical contexts, should be possible to "detach"
>> when writing the literal to disk, to be read later.
> 
> I'm not sure.  It certainly cant' work for arbitrary function objects
> (say, if you create a function with 'make_function' in a module).

We could detect some such functions (e.g. when the value is a subr, and 
thus not readable), but we would still support a large varied class of 
functions this way.

Also, we'd probably want to limit the size of the printed representation 
(FETCHER created by project-find-regexp currently closes over the full 
list of files, that's too much to save; it will probably be a good idea 
to rewrite it to fetch the list of files anew).

>> The issue with doing this at the level of xref--create-fetcher, is
>> that the addition becomes specific to the Xref searches only (find
>> definitions/references), and the more generic Xref UI infrastructure
>> remains unsupported (such as 'M-x project-find-regexp' or whatever
>> calls to xref-show-xrefs exist in third-party packages) -- so those
>> Xref buffers would remain not bookmark-able, or they will each require
>> specialized code like the one you proposed here.
> 
> Yes, but such callers of xref-show-xrefs can implement the same API to
> have the corresponding *xref* buffer bookmark-able.  Namely, arrange for
> the fetcher function to populate xref-fetcher-alist with relevant data.

With that, the simple API "patch FETCHER and DISPLAY-ACTION (probably 
nil)" would turn into something at least twice (or 3x) as complex.

I'm not quite convinced that being able to bookmark Xref buffers is 
worth this cost.

Also note that with such addition the clients would basically pass the 
same information twice: they would both create the fetcher, *and* still 
have to produce the data with which this fetcher is created, and the 
logic to work on it.

This information duplication could be avoided perhaps if we split the 
fetcher into a function symbol and arguments (a new optional argument to 
xref-show-xrefs and xref--show-defs, I suppose). When the caller is able 
to restructure the code to pass a named function as the first arg, the 
result should be print-able. But then they'd have to be careful to keep 
the arguments "simple" too, like not referencing the buffer object 
itself (just its name), etc. That's a fair amount of implicit 
requirements...

> Indeed, I plan to work on doing that for project-find-regexp next.

The approach with xref-backend-restore wouldn't work for it because 
there is no backend to work with.

> If
> we had a solution that doesn't require any work from third party to
> benefit from this new feature, that'd be even better, of course.  But
> since the current API to Xref frontends accepts any fetcher function,
> I'm not sure that's possible...

Perhaps our interpreter wizards could chime in regarding the 
roundtrip-ability of anonymous functions.

Whatever is required to make this work, would likely still require less 
collective effort than redoing the Xref APIs.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Dmitry,

Dmitry Gutov <dmitry@gutov.dev> writes:

>>> The issue with doing this at the level of xref--create-fetcher, is
>>> that the addition becomes specific to the Xref searches only (find
>>> definitions/references), and the more generic Xref UI infrastructure
>>> remains unsupported (such as 'M-x project-find-regexp' or whatever
>>> calls to xref-show-xrefs exist in third-party packages) -- so those
>>> Xref buffers would remain not bookmark-able, or they will each require
>>> specialized code like the one you proposed here.
>> Yes, but such callers of xref-show-xrefs can implement the same API
>> to
>> have the corresponding *xref* buffer bookmark-able.  Namely, arrange for
>> the fetcher function to populate xref-fetcher-alist with relevant data.
>
> 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).  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.

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

> Also note that with such addition the clients would basically pass the
> same information twice: they would both create the fetcher, *and*
> still have to produce the data with which this fetcher is created, and
> the logic to work on it.

Yes.  We could drop the pre-prepared fetcher function and keep only the
"ingredients", but that'd break compatibility with existing frontends,
so we're stuck with some duplication here AFAICT.

> This information duplication could be avoided perhaps if we split the
> fetcher into a function symbol and arguments (a new optional argument
> to xref-show-xrefs and xref--show-defs, I suppose). When the caller is
> able to restructure the code to pass a named function as the first
> arg, the result should be print-able. But then they'd have to be
> careful to keep the arguments "simple" too, like not referencing the
> buffer object itself (just its name), etc. That's a fair amount of
> implicit requirements...
>
>> Indeed, I plan to work on doing that for project-find-regexp next.
>
> The approach with xref-backend-restore wouldn't work for it because
> there is no backend to work with.

My thinking was that we can add an appropriate backend, and perhaps
adapt project-find-regexp to use the new xref-make-fetcher instead of
concocting the fetcher function itself.  But I didn't try it yet so
maybe I'm missing some involved challenges.


Best,

Eshel

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

[Juri Linkov]

>>>> I once tried to use 'revert-buffer-function' to restore
>>>> xref buffers from the desktop, but abandoned the idea.
>>>> Not because xref.el doesn't set 'revert-buffer-function'.
>>>> But because it would take too much time to restore
>>>> the desktop while it will rerun all saved xref buffers.
>>>> OTOH, saving an xref bookmark makes more sense.
>>>> And probably your patch will help to implement
>>>> 'revert-buffer-function' for xref as well.
>>> I don't think bookmark would save the whole buffer contents. Would it?
>>> Otherwise, re-running the search(es) seems inevitable.
>> Indeed, saving the buffer contents of transient buffers
>> either to the bookmark or to the desktop makes no sense.
>> So re-running the command is inevitable.
>> But then I can't imagine how would it be possible
>> to move point to its saved location when initially
>> a restored buffer is empty before re-running the command.
>
> As long as the search is performed synchronously, first you "revert" the
> buffer and then move point. Right?

Sorry, I forgot that xref is not asynchronous as rgrep.
But this is not a problem: most of the time xref delay
is negligible.  Still would not want to run it while
restoring the desktop.  But from a bookmark is fine.

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

[Dmitry Gutov]

On 12/02/2024 20:31, Juri Linkov wrote:
>>>>> I once tried to use 'revert-buffer-function' to restore
>>>>> xref buffers from the desktop, but abandoned the idea.
>>>>> Not because xref.el doesn't set 'revert-buffer-function'.
>>>>> But because it would take too much time to restore
>>>>> the desktop while it will rerun all saved xref buffers.
>>>>> OTOH, saving an xref bookmark makes more sense.
>>>>> And probably your patch will help to implement
>>>>> 'revert-buffer-function' for xref as well.
>>>> I don't think bookmark would save the whole buffer contents. Would it?
>>>> Otherwise, re-running the search(es) seems inevitable.
>>> Indeed, saving the buffer contents of transient buffers
>>> either to the bookmark or to the desktop makes no sense.
>>> So re-running the command is inevitable.
>>> But then I can't imagine how would it be possible
>>> to move point to its saved location when initially
>>> a restored buffer is empty before re-running the command.
>> As long as the search is performed synchronously, first you "revert" the
>> buffer and then move point. Right?
> Sorry, I forgot that xref is not asynchronous as rgrep.
> But this is not a problem: most of the time xref delay
> is negligible.  Still would not want to run it while
> restoring the desktop.  But from a bookmark is fine.

Desktop could use some deferring strategy - e.g. delaying the search 
until the buffer is shown.

Anyway, that's secondary to being able to restore such a buffer at all.

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)

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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?

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

[Juri Linkov]

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

Desktop-mode is from the parallel discussion that I started to consider
other beneficial outcomes of applying your patch.  It will also allow
to replace xref-revert-buffer with revert-buffer-function: currently
xref-revert-buffer uses xref--fetcher, but with revert-buffer-function
could use xref-fetcher-alist.

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

[Juri Linkov]

> Also, we'd probably want to limit the size of the printed representation
> (FETCHER created by project-find-regexp currently closes over the full list
> of files, that's too much to save; it will probably be a good idea to
> rewrite it to fetch the list of files anew).

Indeed, I confirm the problem: reverting an xref buffer with 'g'
can't find matches in a new file added after the first run.

Doesn't this hint that both a bookmark and a revert function
should store more high-level arguments?  For example, for
'project-find-regexp' it should be sufficient to store
its argument 'regexp' together with 'default-directory'
to reconstruct the previous xref buffer contents.
Then it could store these arguments in a buffer-local variable
that could be used to create a bookmark.

PS: With C-u 'project-find-regexp' reads more arguments
in the body, but such reading should be moved to the
interactive form anyway like 'rgrep' does.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>> Also, we'd probably want to limit the size of the printed representation
>> (FETCHER created by project-find-regexp currently closes over the full list
>> of files, that's too much to save; it will probably be a good idea to
>> rewrite it to fetch the list of files anew).
>
> Indeed, I confirm the problem: reverting an xref buffer with 'g'
> can't find matches in a new file added after the first run.
>
> Doesn't this hint that both a bookmark and a revert function
> should store more high-level arguments?  For example, for
> 'project-find-regexp' it should be sufficient to store
> its argument 'regexp' together with 'default-directory'
> to reconstruct the previous xref buffer contents.
> Then it could store these arguments in a buffer-local variable
> that could be used to create a bookmark.

Yes, that's almost exactly what I did in my bookmarking implementation.
Only difference is I kept the whole project object instead of just
'default-directory', since different project types can use different
information.  I'm attaching below the patch I applied in my local
branch, as a reference.

> PS: With C-u 'project-find-regexp' reads more arguments
> in the body, but such reading should be moved to the
> interactive form anyway like 'rgrep' does.

Agreed.  This patch does that too:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Support-bookmarking-project-find-regexp-results-buff.patch --]
[-- Type: text/x-patch, Size: 7648 bytes --]

From 5de2bfd3e2d672d0f955b916a200edc16dca6b07 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Mon, 12 Feb 2024 18:38:36 +0100
Subject: [PATCH] Support bookmarking 'project-find-regexp' results buffer

* lisp/progmodes/project.el (xref-backend-apropos)
(xref-backend-context, xref-backend-restore): New methods.
(project-find-regexp, project-or-external-find-regexp): Use
'xref-make-fetcher' instead of a bespoke fetcher function to
facilitate bookmarking the results buffer.

* lisp/progmodes/xref.el (xref-bookmark-make-record): Use
strings for 'format-spec' specifications.
(xref-bookmark-jump): Autoload it.
(xref-show-xrefs): Make DISPLAY-ACTION argument optional.
(xref-make-fetcher): Autoload, and make BUFFER and POINT
arguments optional, default to the current buffer and point.
---
 lisp/progmodes/project.el | 69 ++++++++++++++++++++-------------------
 lisp/progmodes/xref.el    | 14 +++++---
 2 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 916a031ec60..6b0b4e86851 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -947,32 +947,44 @@ project-other-tab-command
 (declare-function grep-read-files "grep")
 (declare-function xref--find-ignores-arguments "xref")
 
+(cl-defmethod xref-backend-context ((_backend (head project)) _id _kind))
+(cl-defmethod xref-backend-restore ((_backend (head project)) _context))
+(cl-defmethod xref-backend-apropos ((backend (head project)) pattern)
+  (project--find-regexp-in-files pattern (project-files (cdr backend))))
+
+(cl-defmethod xref-backend-context ((_backend (head project-dir)) _id _kind))
+(cl-defmethod xref-backend-restore ((_backend (head project-dir)) _context))
+(cl-defmethod xref-backend-apropos ((backend (head project-dir)) pattern)
+  (project--find-regexp-in-files
+   pattern (project--files-in-directory (nth 1 backend) nil (nth 2 backend))))
+
+(cl-defmethod xref-backend-context ((_backend (head project-ext)) _id _kind))
+(cl-defmethod xref-backend-restore ((_backend (head project-ext)) _context))
+(cl-defmethod xref-backend-apropos ((backend (head project-ext)) pattern)
+  (let ((pr (cdr backend)))
+    (project--find-regexp-in-files
+     pattern (project-files pr (cons (project-root pr)
+                                     (project-external-roots pr))))))
+
 ;;;###autoload
-(defun project-find-regexp (regexp)
+(defun project-find-regexp (regexp &optional dir pattern)
   "Find all matches for REGEXP in the current project's roots.
-With \\[universal-argument] prefix, you can specify the directory
-to search in, and the file name pattern to search for.  The
+With \\[universal-argument] prefix, you can specify the DIR
+to search in, and the file name PATTERN to search for.  The
 pattern may use abbreviations defined in `grep-files-aliases',
 e.g. entering `ch' is equivalent to `*.[ch]'.  As whitespace
 triggers completion when entering a pattern, including it
 requires quoting, e.g. `\\[quoted-insert]<space>'."
-  (interactive (list (project--read-regexp)))
-  (require 'xref)
-  (require 'grep)
-  (let* ((caller-dir default-directory)
-         (pr (project-current t))
-         (default-directory (project-root pr))
-         (files
-          (if (not current-prefix-arg)
-              (project-files pr)
-            (let ((dir (read-directory-name "Base directory: "
-                                            caller-dir nil t)))
-              (project--files-in-directory dir
-                                           nil
-                                           (grep-read-files regexp))))))
-    (xref-show-xrefs
-     (apply-partially #'project--find-regexp-in-files regexp files)
-     nil)))
+  (interactive (let* ((regexp (project--read-regexp))
+                      (dir-pat (when current-prefix-arg
+                                 (cons (read-directory-name "Base directory: ")
+                                       (grep-read-files regexp)))))
+                 (list regexp (car dir-pat) (cdr dir-pat))))
+  (xref-show-xrefs (xref-make-fetcher
+                    (if dir
+                        (list 'project-dir dir pattern)
+                      (cons 'project (project-current t)))
+                    regexp 'apropos regexp)))
 
 (defun project--dir-ignores (project dir)
   (let ((root (project-root project)))
@@ -987,20 +999,11 @@ project--dir-ignores
 
 ;;;###autoload
 (defun project-or-external-find-regexp (regexp)
-  "Find all matches for REGEXP in the project roots or external roots.
-With \\[universal-argument] prefix, you can specify the file name
-pattern to search for."
+  "Find all matches for REGEXP in the project roots or external roots."
   (interactive (list (project--read-regexp)))
-  (require 'xref)
-  (let* ((pr (project-current t))
-         (default-directory (project-root pr))
-         (files
-          (project-files pr (cons
-                             (project-root pr)
-                             (project-external-roots pr)))))
-    (xref-show-xrefs
-     (apply-partially #'project--find-regexp-in-files regexp files)
-     nil)))
+  (xref-show-xrefs (xref-make-fetcher
+                    (cons 'project-ext (project-current t))
+                    regexp 'apropos regexp)))
 
 (defun project--find-regexp-in-files (regexp files)
   (unless files
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 6841f93a9c2..2742cc56ea1 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1049,8 +1049,8 @@ xref-bookmark-make-record
     (user-error "Cannot bookmark due to unknown Xref backend"))
   `(,(format-spec xref-default-bookmark-name-format
                   `((?i . ,xref--identifier)
-                    (?k . ,xref--kind)
-                    (?b . ,xref--backend)))
+                    (?k . ,(symbol-name xref--kind))
+                    (?b . ,(prin1-to-string xref--backend))))
     ,@(bookmark-make-record-default t)
     (backend . ,xref--backend)
     (context . ,(when (buffer-live-p xref--original-buffer)
@@ -1064,6 +1064,7 @@ xref-bookmark-make-record
     (kind . ,xref--kind)
     (handler . xref-bookmark-jump)))
 
+;;;###autoload
 (defun xref-bookmark-jump (bookmark)
   "Jump to Xref buffer bookmark BOOKMARK."
   (let* ((backend (bookmark-prop-get bookmark 'backend))
@@ -1587,7 +1588,7 @@ xref--read-identifier-history
 (defvar xref--read-pattern-history nil)
 
 ;;;###autoload
-(defun xref-show-xrefs (fetcher display-action)
+(defun xref-show-xrefs (fetcher &optional display-action)
   "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'."
@@ -1680,7 +1681,8 @@ xref--find-definitions
    (xref--create-fetcher id 'definitions id)
    display-action))
 
-(defun xref-make-fetcher (backend input kind identifier buffer point)
+;;;###autoload
+(defun xref-make-fetcher (backend input kind identifier &optional buffer point)
   "Return fetcher function for xrefs of kind KIND for IDENTIFIER using BACKEND.
 
 INPUT is the user input for the Xref operation, usually it is the same
@@ -1690,7 +1692,9 @@ xref-make-fetcher
 
 The fetcher function returns a list of xrefs, and sets
 `xref-fetcher-alist', which see."
-  (let ((method (intern (format "xref-backend-%s" kind))))
+  (let ((method (intern (format "xref-backend-%s" kind)))
+        (buffer (or buffer (current-buffer)))
+        (point (or point (point))))
     (lambda ()
       (setq xref-fetcher-alist (list (cons 'original-buffer buffer)
                                      (cons 'original-point point)
-- 
2.42.0

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.

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

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 13/02/2024 09:10, Eshel Yaron wrote:
>> Dmitry Gutov <dmitry@gutov.dev> writes:
>>
>>> 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.

I agree, that's a good heuristic.

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

I think we should have an extensible interface that covers the Xref
commands by default, and allows other callers of `xref-show-xrefs` to
override the default to suite their needs.

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

Yes, you're right.  By default, in my proposal, the return value of this
method is itself a bookmark record (pointing to the position where you
initiate the search), so we rely on the major mode of the original
buffer to define a reasonable `bookmark-make-record-function`.  If a
backend overrides the default method, it also needs to take into account
these limitations, indeed.

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

It doesn't work out of the box for external callers, but it isn't
strictly restricted to Xref commands either: it works for any caller of
`xref-show-xrefs` that defines a (possibly trivial) Xref backend, and
passes a fetcher function that sets `xref-fetcher-alist`.
`xref-make-fetcher` is supposed to make it easier to create the such a
fetcher function.

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

Right.  For Eglot, we cannot restore a bookmark with nil context, and we
also need to make sure we're connected to the server.  Adding something
like the following in eglot.el seems to do the trick:

--8<---------------cut here---------------start------------->8---
(cl-defmethod xref-backend-restore ((_backend (eql eglot)) context)
  (unless context
    (error "No context available for restoring Xref search"))
  (bookmark-handle-bookmark context)
  (unless eglot--managed-mode
    (apply #'eglot--connect (eglot--guess-contact))))
--8<---------------cut here---------------end--------------->8---