bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[yyoncho]

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

ATM there xref-pop-marker-stack which is equivalent to going backwards
but there is no operation for going forward.

[-- Attachment #2: Type: text/html, Size: 144 bytes --]

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

Hi Ivan,

On 29.12.2019 21:11, yyoncho wrote:
> ATM there xref-pop-marker-stack which is equivalent to going backwards
> but there is no operation for going forward.

Do you want to try writing the patch?

Having written one third-party package for this purpose, and now using 
another (history), I personally have a whole set of requirements for 
this feature to be able to use it (e.g. making the rings window-local 
and having a visualization when moving back and forth).

I'm not sure how important those are for an average user, though. So... 
would you like to submit something you would be happy with personally?

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

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

As usual my message was lost. Here's the patch at least.


[-- Attachment #2: 0001-Add-xref-forward-history-bug-38797.patch --]
[-- Type: application/octet-stream, Size: 9896 bytes --]

From 9191300d574a1a7d4adc3eb86f0381a6e836c887 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 19 Oct 2021 13:26:19 +0200
Subject: [PATCH] Add xref forward history (bug#38797)

Make it possible to go forward as well as back in the Xref history.
The new `xref-forward-history` is bound to `C-M-,`.

* lisp/progmodes/etags.el (find-tag-marker-ring-length): Update.
(tags-location-ring-length): New.
(find-tag-marker-ring): Remove.
(tags-location-ring, tags-reset-tags-tables):
Use `tags-location-ring-length` instead of `xref-marker-ring-length`.
* lisp/progmodes/xref.el (xref-marker-ring-length, xref-marker-ring)
(xref-set-marker-ring-length):
Make obsolete.
(xref--history, xref-forward-history): New.
(xref-push-marker-stack, xref-pop-marker-stack)
(xref-clear-marker-stack, xref-marker-stack-empty-p):
Use `xref--history`.
* doc/emacs/maintaining.texi (Looking Up Identifiers): Document.
* etc/NEWS: Announce.
---
 doc/emacs/maintaining.texi | 13 +++++--
 etc/NEWS                   |  5 +++
 lisp/progmodes/etags.el    | 11 +++---
 lisp/progmodes/xref.el     | 78 ++++++++++++++++++++++----------------
 4 files changed, 66 insertions(+), 41 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index 5b66031b8a..46b4d8181f 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -2140,6 +2140,9 @@ Looking Up Identifiers
 @item M-,
 Go back to where you previously invoked @kbd{M-.} and friends
 (@code{xref-pop-marker-stack}).
+@item C-M-,
+Go forward to where you previously invoked @kbd{M-,}
+(@code{xref-forward-history}).
 @item M-x xref-etags-mode
 Switch @code{xref} to use the @code{etags} backend.
 @end table
@@ -2205,14 +2208,16 @@ Looking Up Identifiers
 
 @kindex M-,
 @findex xref-pop-marker-stack
-@vindex xref-marker-ring-length
   To go back to places @emph{from where} you've displayed the definition,
 use @kbd{M-,} (@code{xref-pop-marker-stack}).  It jumps back to the
 point of the last invocation of @kbd{M-.}.  Thus you can find and
 examine the definition of something with @kbd{M-.} and then return to
-where you were with @kbd{M-,}.  @kbd{M-,} allows you to retrace your
-steps to a depth determined by the variable
-@code{xref-marker-ring-length}, which defaults to 16.
+where you were with @kbd{M-,}.
+
+@kindex C-M-,
+@findex xref-forward-history
+  Go forward to a place from where you previously went back using @kbd{M-,}.
+This is useful if you find that you went back too far.
 
 @findex xref-etags-mode
   Some major modes install @code{xref} support facilities that might
diff --git a/etc/NEWS b/etc/NEWS
index 9f0a4ac4ce..21f9a0f75f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -153,6 +153,11 @@ it with new 'term-{faint,italic,slow-blink,fast-blink}' faces.
 *** 'project-find-file' and 'project-or-external-find-file' now accept
 a prefix argument which is interpreted to mean "include all files".
 
++++
+*** New command 'xref-forward-history'.
+It is bound to 'C-M-,' and jumps to the location where 'xref-pop-marker-stack'
+was invoked previously.
+
 ** File notifications
 
 +++
diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index d2ce813daa..518c4ec81b 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -145,7 +145,9 @@ find-tag-default-function
   :type '(choice (const nil) function))
 
 (define-obsolete-variable-alias 'find-tag-marker-ring-length
-  'xref-marker-ring-length "25.1")
+  'tags-location-ring-length "29.1")
+
+(defvar tags-location-ring-length 16)
 
 (defcustom tags-tag-face 'default
   "Face for tags in the output of `tags-apropos'."
@@ -180,7 +182,6 @@ tags-apropos-additional-actions
 		       (sexp :tag "Tags to search")))
   :version "21.1")
 
-(defvaralias 'find-tag-marker-ring 'xref--marker-ring)
 (make-obsolete-variable
  'find-tag-marker-ring
  "use `xref-push-marker-stack' or `xref-pop-marker-stack' instead."
@@ -191,7 +192,7 @@ default-tags-table-function
 This function receives no arguments and should return the default
 tags table file to use for the current buffer.")
 
-(defvar tags-location-ring (make-ring xref-marker-ring-length)
+(defvar tags-location-ring (make-ring tags-location-ring-length)
   "Ring of markers which are locations visited by \\[find-tag].
 Pop back to the last location with \\[negative-argument] \\[find-tag].")
 \f
@@ -731,13 +732,13 @@ tags-reset-tags-tables
   (interactive)
   ;; Clear out the markers we are throwing away.
   (let ((i 0))
-    (while (< i xref-marker-ring-length)
+    (while (< i tags-location-ring-length)
       (if (aref (cddr tags-location-ring) i)
 	  (set-marker (aref (cddr tags-location-ring) i) nil))
       (setq i (1+ i))))
   (xref-clear-marker-stack)
   (setq tags-file-name nil
-	tags-location-ring (make-ring xref-marker-ring-length)
+	tags-location-ring (make-ring tags-location-ring-length)
 	tags-table-list nil
 	tags-table-computed-list nil
 	tags-table-computed-list-for nil
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 52a4e0c543..502cab022c 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -334,15 +334,7 @@ xref--search-property
           (t (goto-char start) nil))))
 
 \f
-;;; Marker stack  (M-. pushes, M-, pops)
-
-(defcustom xref-marker-ring-length 16
-  "Length of the xref marker ring.
-If this variable is not set through Customize, you must call
-`xref-set-marker-ring-length' for changes to take effect."
-  :type 'integer
-  :initialize #'custom-initialize-default
-  :set #'xref-set-marker-ring-length)
+(make-obsolete-variable 'xref-marker-ring-length nil "29.1")
 
 (defcustom xref-prompt-for-identifier '(not xref-find-definitions
                                             xref-find-definitions-other-window
@@ -413,29 +405,47 @@ xref-auto-jump-to-first-xref
   :version "28.1"
   :package-version '(xref . "1.2.0"))
 
-(defvar xref--marker-ring (make-ring xref-marker-ring-length)
-  "Ring of markers to implement the marker stack.")
+(defvar xref--history (cons nil nil)
+  "(HISTORY-STACK . FUTURE-STACK) of markers to visited Xref locations.")
+
+(make-obsolete-variable 'xref-marker-ring nil "29.1")
 
-(defun xref-set-marker-ring-length (var val)
-  "Set `xref-marker-ring-length'.
-VAR is the symbol `xref-marker-ring-length' and VAL is the new
-value."
-  (set-default var val)
-  (if (ring-p xref--marker-ring)
-      (ring-resize xref--marker-ring val)))
+(defun xref-set-marker-ring-length (_var _val)
+  (declare (obsolete nil "29.1"))
+  nil)
 
 (defun xref-push-marker-stack (&optional m)
-  "Add point M (defaults to `point-marker') to the marker stack."
-  (ring-insert xref--marker-ring (or m (point-marker))))
+  "Add point M (defaults to `point-marker') to the marker stack.
+The future stack is erased."
+  (push (or m (point-marker)) (car xref--history))
+  (dolist (mk (cdr xref--history))
+    (set-marker mk nil nil))
+  (setcdr xref--history nil))
 
 ;;;###autoload
 (defun xref-pop-marker-stack ()
-  "Pop back to where \\[xref-find-definitions] was last invoked."
+  "Go back to the previous position in xref history.
+To undo, use \\[xref-forward-history]."
+  (interactive)
+  (if (null (car xref--history))
+      (user-error "At start of xref history")
+    (let ((marker (pop (car xref--history))))
+      (push (point-marker) (cdr xref--history))
+      (switch-to-buffer (or (marker-buffer marker)
+                            (user-error "The marked buffer has been deleted")))
+      (goto-char (marker-position marker))
+      (set-marker marker nil nil)
+      (run-hooks 'xref-after-return-hook))))
+;; FIXME: rename to `xref-back-history'.
+
+;;;###autoload
+(defun xref-forward-history ()
+  "Got to the point where a previous \\[xref-pop-marker-stack] was invoked."
   (interactive)
-  (let ((ring xref--marker-ring))
-    (when (ring-empty-p ring)
-      (user-error "Marker stack is empty"))
-    (let ((marker (ring-remove ring 0)))
+  (if (null (cdr xref--history))
+      (user-error "At end of xref history")
+    (let ((marker (pop (cdr xref--history))))
+      (push (point-marker) (car xref--history))
       (switch-to-buffer (or (marker-buffer marker)
                             (user-error "The marked buffer has been deleted")))
       (goto-char (marker-position marker))
@@ -458,16 +468,19 @@ xref-pulse-momentarily
 
 ;; etags.el needs this
 (defun xref-clear-marker-stack ()
-  "Discard all markers from the marker stack."
-  (let ((ring xref--marker-ring))
-    (while (not (ring-empty-p ring))
-      (let ((marker (ring-remove ring)))
-        (set-marker marker nil nil)))))
+  "Discard all markers from the xref history."
+  (dolist (l (list (car xref--history) (cdr xref--history)))
+    (dolist (m l)
+      (set-marker m nil nil)))
+  (setq xref--history (cons nil nil))
+  nil)
 
 ;;;###autoload
 (defun xref-marker-stack-empty-p ()
-  "Return t if the marker stack is empty; nil otherwise."
-  (ring-empty-p xref--marker-ring))
+  "Whether the xref back-history is empty."
+  (null (car xref--history)))
+;; FIXME: rename this to `xref--back-history-empty-p' and add the
+;; corresponding forward history function.
 
 \f
 
@@ -1487,6 +1500,7 @@ xref-apropos-regexp
 ;;;###autoload (define-key esc-map "," #'xref-pop-marker-stack)
 ;;;###autoload (define-key esc-map "?" #'xref-find-references)
 ;;;###autoload (define-key esc-map [?\C-.] #'xref-find-apropos)
+;;;###autoload (define-key esc-map [?\C-,] #'xref-forward-history)
 ;;;###autoload (define-key ctl-x-4-map "." #'xref-find-definitions-other-window)
 ;;;###autoload (define-key ctl-x-5-map "." #'xref-find-definitions-other-frame)
 
-- 
2.21.1 (Apple Git-122.3)

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 20.10.2021 22:12, Mattias Engdegård wrote:
> As usual my message was lost. Here's the patch at least.

Thanks, Mattias.

Some comments on the latest version of the patch:

- The removal of

   (defvaralias 'find-tag-marker-ring 'xref--marker-ring)

and the subsequent initialization of the aliased variable to a new ring 
will likely break any real code code which is still depending on that 
variable. Because now it is nil by default, and not a ring. Not sure 
what's the best solution, perhaps it's just time to remove it.

- The call

   (make-obsolete-variable 'xref-marker-ring-length nil "29.1")

is similarly pointless because the variable definition has been deleted. 
Declaring it obsolete does not help the fact that it is unbound now.

It could be made into an alias for the newly introduced 
tags-location-ring-length, I suppose.

Or we hold off on removing it, declare it obsolete (with some 
explanation), but keep using its alias in etags.el. That would mean 
dropping all changes in the etags.el part of the patch. But probably 
adding some 'with-suppressed-warinings' wrappers in there.

- Regarding the names of the commands, some alternatives:

* xref-rewind-history? It's a play on "fast forward". A bit cryptic, 
perhaps.
* xref-go-back and xref-go-forward? xref--history is fine the name of 
the variable, but the purpose of the commands seems obvious enough even 
without "history" in them.

No pressure. Just in case you like any of these better.

- The slots in the docstring of xref--history (should we call it 
xref--location-history?) could be called (BACK-STACK . FORWARD-STACK).

- xref-set-marker-ring-length's fate is changed accordingly to what we 
choose to do with xref-marker-ring-length.

- The FIXME about proposed xref--back-history-empty-p doesn't make much 
sense if the introduced function name is going to be private (we don't 
need private functions that aren't used in the package itself). 
xref-back-history-empty-p and xref-forward-history-empty-p could work. 
But we can keep xref-marker-stack-empty-p around, pending feature 
requests. Maybe add a boolean optional FORWARD argument. Or rename it to 
xref-history-empty-p, for instance (with the optional FORWARD argument).

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

> - Regarding the names of the commands, some alternatives:
>
> * xref-rewind-history? It's a play on "fast forward". A bit cryptic,
>   perhaps.
> * xref-go-back and xref-go-forward? xref--history is fine the name of the
>   variable, but the purpose of the commands seems obvious enough even
>   without "history" in them.

Two different naming conventions are in use already:

  Info-history-back
  Info-history-forward

  help-go-back
  help-go-forward

Choosing any of those would be fine.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 24.10.2021 10:54, Juri Linkov wrote:
>> - Regarding the names of the commands, some alternatives:
>>
>> * xref-rewind-history? It's a play on "fast forward". A bit cryptic,
>>    perhaps.
>> * xref-go-back and xref-go-forward? xref--history is fine the name of the
>>    variable, but the purpose of the commands seems obvious enough even
>>    without "history" in them.
> Two different naming conventions are in use already:
> 
>    Info-history-back
>    Info-history-forward
> 
>    help-go-back
>    help-go-forward
> 
> Choosing any of those would be fine.

That reminds me of a potential feature: saving the history of searches 
and going back/forward between them, in the manner of help-go-back and 
help-go-forward (maybe even with same keybindings).

 From that perspective, we could choose the names in advance. Either

   xref-go-back/forward and xref-history-back/forward

or, I suppose

   xref-back-history/forward-history and xref-searches-back/forward

...something like that.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

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

24 okt. 2021 kl. 03.40 skrev Dmitry Gutov <dgutov@yandex.ru>:

> - The removal of
> 
>  (defvaralias 'find-tag-marker-ring 'xref--marker-ring)
> 
> and the subsequent initialization of the aliased variable to a new ring will likely break any real code code which is still depending on that variable. Because now it is nil by default, and not a ring. Not sure what's the best solution, perhaps it's just time to remove it.

Right; I've resurrected find-tag-marker-ring as a zombie variable. Code manipulating it will have no effect but at least won't crash, which is probably a good trade-off.

>  (make-obsolete-variable 'xref-marker-ring-length nil "29.1")
> 
> is similarly pointless because the variable definition has been deleted. Declaring it obsolete does not help the fact that it is unbound now.

Correct -- resurrected as a zombie variable here too.

> * xref-go-back and xref-go-forward?

Good suggestions and now used. (I've still not added xref-go-back as an alias but that should probably be done.)

> - The slots in the docstring of xref--history (should we call it xref--location-history?) could be called (BACK-STACK . FORWARD-STACK).

Yes, done.

> - xref-set-marker-ring-length's fate is changed accordingly to what we choose to do with xref-marker-ring-length.

We could make it set the pointless xref-marker-ring-length but I didn't bother. I doubt anyone is using that function.

> - The FIXME about proposed xref--back-history-empty-p doesn't make much sense if the introduced function name is going to be private

Sorry, that wasn't the intention. I've added xref-back-history-empty-p for use in the new menu entry.
(I'll let someone else add it to the context menu because that seemed much more complicated.)

Revised patch attached.


[-- Attachment #2: 0001-Add-xref-forward-history-bug-38797.patch --]
[-- Type: application/octet-stream, Size: 11099 bytes --]

From 6984b9ed0880a31e70d297f74799e47ee2903d97 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 19 Oct 2021 13:26:19 +0200
Subject: [PATCH] Add xref forward history (bug#38797)

Make it possible to go forward as well as back in the Xref history.
The new `xref-go-forward` is bound to `C-M-,`.

* lisp/progmodes/etags.el (find-tag-marker-ring-length): Update.
(tags-location-ring-length): New.
(find-tag-marker-ring): Keep as dummy.
(tags-location-ring, tags-reset-tags-tables):
Use `tags-location-ring-length` instead of `xref-marker-ring-length`.
* lisp/progmodes/xref.el (xref-marker-ring-length, xref-marker-ring)
(xref-set-marker-ring-length):
Make obsolete.
(xref--history, xref-go-forward, xref-forward-history-empty-p): New.
(xref-push-marker-stack, xref-pop-marker-stack)
(xref-clear-marker-stack, xref-marker-stack-empty-p):
Use `xref--history`.
* lisp/menu-bar.el (menu-bar-goto-menu): Add Forward entry.
* doc/emacs/maintaining.texi (Looking Up Identifiers): Document.
* etc/NEWS: Announce.
---
 doc/emacs/maintaining.texi | 13 ++++--
 etc/NEWS                   |  5 +++
 lisp/menu-bar.el           |  6 +++
 lisp/progmodes/etags.el    | 13 +++---
 lisp/progmodes/xref.el     | 83 +++++++++++++++++++++++---------------
 5 files changed, 79 insertions(+), 41 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index 5b66031b8a..e28ec5fb43 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -2140,6 +2140,9 @@ Looking Up Identifiers
 @item M-,
 Go back to where you previously invoked @kbd{M-.} and friends
 (@code{xref-pop-marker-stack}).
+@item C-M-,
+Go forward to where you previously invoked @kbd{M-,}
+(@code{xref-go-forward}).
 @item M-x xref-etags-mode
 Switch @code{xref} to use the @code{etags} backend.
 @end table
@@ -2205,14 +2208,16 @@ Looking Up Identifiers
 
 @kindex M-,
 @findex xref-pop-marker-stack
-@vindex xref-marker-ring-length
   To go back to places @emph{from where} you've displayed the definition,
 use @kbd{M-,} (@code{xref-pop-marker-stack}).  It jumps back to the
 point of the last invocation of @kbd{M-.}.  Thus you can find and
 examine the definition of something with @kbd{M-.} and then return to
-where you were with @kbd{M-,}.  @kbd{M-,} allows you to retrace your
-steps to a depth determined by the variable
-@code{xref-marker-ring-length}, which defaults to 16.
+where you were with @kbd{M-,}.
+
+@kindex C-M-,
+@findex xref-go-forward
+  Go forward to a place from where you previously went back using @kbd{M-,}.
+This is useful if you find that you went back too far.
 
 @findex xref-etags-mode
   Some major modes install @code{xref} support facilities that might
diff --git a/etc/NEWS b/etc/NEWS
index 294181635e..e0c2aead94 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -176,6 +176,11 @@ it with new 'term-{faint,italic,slow-blink,fast-blink}' faces.
 *** 'project-find-file' and 'project-or-external-find-file' now accept
 a prefix argument which is interpreted to mean "include all files".
 
++++
+*** New command 'xref-go-forward'.
+It is bound to 'C-M-,' and jumps to the location where 'xref-pop-marker-stack'
+was invoked previously.
+
 ** File notifications
 
 +++
diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index fafc99eb95..d1bb8d9677 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -419,6 +419,12 @@ menu-bar-goto-menu
                                 (not (xref-marker-stack-empty-p)))
                   :help "Back to the position of the last search"))
 
+    (bindings--define-key menu [xref-forward]
+      '(menu-item "Forward" xref-go-forward
+                  :visible (and (featurep 'xref)
+                                (not (xref-forward-history-empty-p)))
+                  :help "Forward to the position gone Back from"))
+
     (bindings--define-key menu [xref-apropos]
       '(menu-item "Find Apropos..." xref-find-apropos
                   :help "Find function/variables whose names match regexp"))
diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index d2ce813daa..318121fbb5 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -145,7 +145,9 @@ find-tag-default-function
   :type '(choice (const nil) function))
 
 (define-obsolete-variable-alias 'find-tag-marker-ring-length
-  'xref-marker-ring-length "25.1")
+  'tags-location-ring-length "25.1")
+
+(defvar tags-location-ring-length 16)
 
 (defcustom tags-tag-face 'default
   "Face for tags in the output of `tags-apropos'."
@@ -180,7 +182,8 @@ tags-apropos-additional-actions
 		       (sexp :tag "Tags to search")))
   :version "21.1")
 
-(defvaralias 'find-tag-marker-ring 'xref--marker-ring)
+;; Obsolete variable kept for compatibility. We don't use it in any way.
+(defvar find-tag-marker-ring (make-ring 16))
 (make-obsolete-variable
  'find-tag-marker-ring
  "use `xref-push-marker-stack' or `xref-pop-marker-stack' instead."
@@ -191,7 +194,7 @@ default-tags-table-function
 This function receives no arguments and should return the default
 tags table file to use for the current buffer.")
 
-(defvar tags-location-ring (make-ring xref-marker-ring-length)
+(defvar tags-location-ring (make-ring tags-location-ring-length)
   "Ring of markers which are locations visited by \\[find-tag].
 Pop back to the last location with \\[negative-argument] \\[find-tag].")
 \f
@@ -731,13 +734,13 @@ tags-reset-tags-tables
   (interactive)
   ;; Clear out the markers we are throwing away.
   (let ((i 0))
-    (while (< i xref-marker-ring-length)
+    (while (< i tags-location-ring-length)
       (if (aref (cddr tags-location-ring) i)
 	  (set-marker (aref (cddr tags-location-ring) i) nil))
       (setq i (1+ i))))
   (xref-clear-marker-stack)
   (setq tags-file-name nil
-	tags-location-ring (make-ring xref-marker-ring-length)
+	tags-location-ring (make-ring tags-location-ring-length)
 	tags-table-list nil
 	tags-table-computed-list nil
 	tags-table-computed-list-for nil
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index a198ae349e..66ac89e72f 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -341,15 +341,9 @@ xref--search-property
           (t (goto-char start) nil))))
 
 \f
-;;; Marker stack  (M-. pushes, M-, pops)
-
-(defcustom xref-marker-ring-length 16
-  "Length of the xref marker ring.
-If this variable is not set through Customize, you must call
-`xref-set-marker-ring-length' for changes to take effect."
-  :type 'integer
-  :initialize #'custom-initialize-default
-  :set #'xref-set-marker-ring-length)
+;; Dummy variable retained for compatibility.
+(defvar xref-marker-ring-length 16)
+(make-obsolete-variable 'xref-marker-ring-length nil "29.1")
 
 (defcustom xref-prompt-for-identifier '(not xref-find-definitions
                                             xref-find-definitions-other-window
@@ -420,29 +414,47 @@ xref-auto-jump-to-first-xref
   :version "28.1"
   :package-version '(xref . "1.2.0"))
 
-(defvar xref--marker-ring (make-ring xref-marker-ring-length)
-  "Ring of markers to implement the marker stack.")
+(defvar xref--history (cons nil nil)
+  "(BACKWARD-STACK . FORWARD-STACK) of markers to visited Xref locations.")
+
+(make-obsolete-variable 'xref-marker-ring nil "29.1")
 
-(defun xref-set-marker-ring-length (var val)
-  "Set `xref-marker-ring-length'.
-VAR is the symbol `xref-marker-ring-length' and VAL is the new
-value."
-  (set-default var val)
-  (if (ring-p xref--marker-ring)
-      (ring-resize xref--marker-ring val)))
+(defun xref-set-marker-ring-length (_var _val)
+  (declare (obsolete nil "29.1"))
+  nil)
 
 (defun xref-push-marker-stack (&optional m)
-  "Add point M (defaults to `point-marker') to the marker stack."
-  (ring-insert xref--marker-ring (or m (point-marker))))
+  "Add point M (defaults to `point-marker') to the marker stack.
+The future stack is erased."
+  (push (or m (point-marker)) (car xref--history))
+  (dolist (mk (cdr xref--history))
+    (set-marker mk nil nil))
+  (setcdr xref--history nil))
 
 ;;;###autoload
 (defun xref-pop-marker-stack ()
-  "Pop back to where \\[xref-find-definitions] was last invoked."
+  "Go back to the previous position in xref history.
+To undo, use \\[xref-go-forward]."
   (interactive)
-  (let ((ring xref--marker-ring))
-    (when (ring-empty-p ring)
-      (user-error "Marker stack is empty"))
-    (let ((marker (ring-remove ring 0)))
+  (if (null (car xref--history))
+      (user-error "At start of xref history")
+    (let ((marker (pop (car xref--history))))
+      (push (point-marker) (cdr xref--history))
+      (switch-to-buffer (or (marker-buffer marker)
+                            (user-error "The marked buffer has been deleted")))
+      (goto-char (marker-position marker))
+      (set-marker marker nil nil)
+      (run-hooks 'xref-after-return-hook))))
+;; FIXME: rename to `xref-go-back'.
+
+;;;###autoload
+(defun xref-go-forward ()
+  "Got to the point where a previous \\[xref-pop-marker-stack] was invoked."
+  (interactive)
+  (if (null (cdr xref--history))
+      (user-error "At end of xref history")
+    (let ((marker (pop (cdr xref--history))))
+      (push (point-marker) (car xref--history))
       (switch-to-buffer (or (marker-buffer marker)
                             (user-error "The marked buffer has been deleted")))
       (goto-char (marker-position marker))
@@ -465,17 +477,23 @@ xref-pulse-momentarily
 
 ;; etags.el needs this
 (defun xref-clear-marker-stack ()
-  "Discard all markers from the marker stack."
-  (let ((ring xref--marker-ring))
-    (while (not (ring-empty-p ring))
-      (let ((marker (ring-remove ring)))
-        (set-marker marker nil nil)))))
+  "Discard all markers from the xref history."
+  (dolist (l (list (car xref--history) (cdr xref--history)))
+    (dolist (m l)
+      (set-marker m nil nil)))
+  (setq xref--history (cons nil nil))
+  nil)
 
 ;;;###autoload
 (defun xref-marker-stack-empty-p ()
-  "Return t if the marker stack is empty; nil otherwise."
-  (ring-empty-p xref--marker-ring))
+  "Whether the xref back-history is empty."
+  (null (car xref--history)))
+;; FIXME: rename this to `xref-back-history-empty-p'.
 
+;;;###autoload
+(defun xref-forward-history-empty-p ()
+  "Whether the xref forward-history is empty."
+  (null (cdr xref--history)))
 \f
 
 (defun xref--goto-char (pos)
@@ -1494,6 +1512,7 @@ xref-apropos-regexp
 ;;;###autoload (define-key esc-map "," #'xref-pop-marker-stack)
 ;;;###autoload (define-key esc-map "?" #'xref-find-references)
 ;;;###autoload (define-key esc-map [?\C-.] #'xref-find-apropos)
+;;;###autoload (define-key esc-map [?\C-,] #'xref-go-forward)
 ;;;###autoload (define-key ctl-x-4-map "." #'xref-find-definitions-other-window)
 ;;;###autoload (define-key ctl-x-5-map "." #'xref-find-definitions-other-frame)
 
-- 
2.21.1 (Apple Git-122.3)

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

> That reminds me of a potential feature: saving the history of searches and
> going back/forward between them, in the manner of help-go-back and
> help-go-forward (maybe even with same keybindings).
>
> From that perspective, we could choose the names in advance. Either
>
>   xref-go-back/forward and xref-history-back/forward
>
> or, I suppose
>
>   xref-back-history/forward-history and xref-searches-back/forward
>
> ...something like that.

I never had such a problem thanks to the customization
that creates a new buffer for every search:

  (add-hook 'xref--xref-buffer-mode-hook 'rename-uniquely)

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 24.10.2021 22:10, Juri Linkov wrote:
>> That reminds me of a potential feature: saving the history of searches and
>> going back/forward between them, in the manner of help-go-back and
>> help-go-forward (maybe even with same keybindings).
>>
>>  From that perspective, we could choose the names in advance. Either
>>
>>    xref-go-back/forward and xref-history-back/forward
>>
>> or, I suppose
>>
>>    xref-back-history/forward-history and xref-searches-back/forward
>>
>> ...something like that.
> 
> I never had such a problem thanks to the customization
> that creates a new buffer for every search:
> 
>    (add-hook 'xref--xref-buffer-mode-hook 'rename-uniquely)

It's a nice intermediate solution, but it would also be nice to be able 
to quickly iterate across previous searches. No?

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 24.10.2021 16:24, Mattias Engdegård wrote:
> Revised patch attached.

LGTM, please install.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

>>> That reminds me of a potential feature: saving the history of searches and
>>> going back/forward between them, in the manner of help-go-back and
>>> help-go-forward (maybe even with same keybindings).
>>>
>>>  From that perspective, we could choose the names in advance. Either
>>>
>>>    xref-go-back/forward and xref-history-back/forward
>>>
>>> or, I suppose
>>>
>>>    xref-back-history/forward-history and xref-searches-back/forward
>>>
>>> ...something like that.
>> I never had such a problem thanks to the customization
>> that creates a new buffer for every search:
>>    (add-hook 'xref--xref-buffer-mode-hook 'rename-uniquely)
>
> It's a nice intermediate solution, but it would also be nice to be able to
> quickly iterate across previous searches. No?

This means adding another layer of complexity.  Remember a mess
of deciding how to switch next-error navigations from different searches?
Switching multiple xref searches will have the same level of confusion.

BTW, shouldn't xref-go-back/forward support next-error-function?  ;-)

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 25.10.2021 10:44, Juri Linkov wrote:

>> It's a nice intermediate solution, but it would also be nice to be able to
>> quickly iterate across previous searches. No?
> 
> This means adding another layer of complexity.  Remember a mess
> of deciding how to switch next-error navigations from different searches?
> Switching multiple xref searches will have the same level of confusion.

Unless we do want to be able to switch next-error navigation sources 
this way?

Alternatively, we could uniquify the Xref buffers in a more predictable 
way (e.g. by using the caller command and the input string, somehow), 
and then have commands xref-searches-back and xref-searches-format 
switch between the resulting buffers.

> BTW, shouldn't xref-go-back/forward support next-error-function?  ;-)

What kind of support? Not sure I understand, sorry.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

25 okt. 2021 kl. 04.13 skrev Dmitry Gutov <dgutov@yandex.ru>:

> LGTM, please install.

Thank you Dmitry, now pushed to master.

Juri, would you help out with adding (or not) `xref-go-forward` to the context menu? I wasn't sure if we had agreed on good names for this and the go-back entries. The current "Back Definition" is a bit nonsensical in English.

I favour (lightly) "Go Back" / "Go Forward" because they make sense in context with the other Xref operations in that separator-delimited group of entries in the menu, and because it's a wording often used for these operations elsewhere such as in web browsers and IDEs (though often it's just "Back" and "Forward").

"Xref {Back,Forward}" or "{Back,Forward} Xref" would also do and displays the link to Xref up front, but the English is decidedly less natural.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

>> LGTM, please install.
>
> Thank you Dmitry, now pushed to master.

I took a look at your change, and noticed an asymmetry
between the names xref-pop-marker-stack and xref-go-forward.
Shouldn't the name xref-pop-marker-stack be obsoleted,
and replaced with a new name xref-go-back?  (I see now
that you added FIXME for this.)

Another question is that in the menu bar currently the order is
"Forward" and "Back".  But more natural order would be
"Back" and "Forward".

> Juri, would you help out with adding (or not) `xref-go-forward` to the
> context menu? I wasn't sure if we had agreed on good names for this and the
> go-back entries. The current "Back Definition" is a bit nonsensical in
> English.

I completely agree that the current title is inappropriate.

> I favour (lightly) "Go Back" / "Go Forward" because they make sense in
> context with the other Xref operations in that separator-delimited group of
> entries in the menu, and because it's a wording often used for these
> operations elsewhere such as in web browsers and IDEs (though often it's
> just "Back" and "Forward").
>
> "Xref {Back,Forward}" or "{Back,Forward} Xref" would also do and displays
> the link to Xref up front, but the English is decidedly less natural.

With such a test case:

0. emacs -Q
1. click mouse-3 on any word in *scratch*, and select from the context menu
   "Find Definition".  It should fail with the error
   "No definitions found for: This".  This is correct.
2. now click mouse-3 at the end of *scratch*, not on a word.

The menu contains the item "Back Definition".  (BTW, why if the search failed?)

This menu item is too ambiguous.  Renaming it to "Go Back" or "Go Forward"
doesn't make the menu item clearer.  Only adding a word "Xref" somewhere
will disambiguate it.  Maybe then "Go Back in Xref" or "Go Back with Xref"?
We need more opinions.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

>>> It's a nice intermediate solution, but it would also be nice to be able to
>>> quickly iterate across previous searches. No?
>> This means adding another layer of complexity.  Remember a mess
>> of deciding how to switch next-error navigations from different searches?
>> Switching multiple xref searches will have the same level of confusion.
>
> Unless we do want to be able to switch next-error navigation sources this
> way?

Maybe.

> Alternatively, we could uniquify the Xref buffers in a more predictable way
> (e.g. by using the caller command and the input string, somehow), and then
> have commands xref-searches-back and xref-searches-format switch between
> the resulting buffers.
>
>> BTW, shouldn't xref-go-back/forward support next-error-function?  ;-)
>
> What kind of support? Not sure I understand, sorry.

Using unique Xref buffers will allow switching to a unique
Xref buffer and typing 'M-x next-error-select-buffer'.

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

25 okt. 2021 kl. 19.05 skrev Juri Linkov <juri@linkov.net>:

> I took a look at your change, and noticed an asymmetry
> between the names xref-pop-marker-stack and xref-go-forward.
> Shouldn't the name xref-pop-marker-stack be obsoleted,
> and replaced with a new name xref-go-back?

Yes, this was the intention. I have now done that; thanks for reminding me!

> Another question is that in the menu bar currently the order is
> "Forward" and "Back".  But more natural order would be
> "Back" and "Forward".

Oh, I didn't realise they had to be given in reverse order. Fixed, thank you.

> The menu contains the item "Back Definition".  (BTW, why if the search failed?)

That appears to be a known imperfection in Xref -- I agree it shouldn't be added if the search failed.

> This menu item is too ambiguous.  Renaming it to "Go Back" or "Go Forward"
> doesn't make the menu item clearer.  Only adding a word "Xref" somewhere
> will disambiguate it.  Maybe then "Go Back in Xref" or "Go Back with Xref"?

Maybe, but if we embrace the idea that there really only should be a single back button in the application (which is what other IDEs typically have), then there is no ambiguity. Until then we may need to add "Xref" somehow, but where is the confusion, exactly?

> We need more opinions.

Only if they are well-reasoned!

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 25.10.2021 20:05, Juri Linkov wrote:

>> I favour (lightly) "Go Back" / "Go Forward" because they make sense in
>> context with the other Xref operations in that separator-delimited group of
>> entries in the menu, and because it's a wording often used for these
>> operations elsewhere such as in web browsers and IDEs (though often it's
>> just "Back" and "Forward").
>>
>> "Xref {Back,Forward}" or "{Back,Forward} Xref" would also do and displays
>> the link to Xref up front, but the English is decidedly less natural.
> 
> With such a test case:
> 
> 0. emacs -Q

Seems like step 0.5 is missing: enable context-menu-mode.

> 1. click mouse-3 on any word in *scratch*, and select from the context menu
>     "Find Definition".  It should fail with the error
>     "No definitions found for: This".  This is correct.
> 2. now click mouse-3 at the end of *scratch*, not on a word.
> 
> The menu contains the item "Back Definition".  (BTW, why if the search failed?)
> 
> This menu item is too ambiguous.  Renaming it to "Go Back" or "Go Forward"
> doesn't make the menu item clearer.  Only adding a word "Xref" somewhere
> will disambiguate it.  Maybe then "Go Back in Xref" or "Go Back with Xref"?
> We need more opinions.

Go Back and Go Forward look okay to me.

Not sure whether adding "Xref " would be an improvement: the Xref 
locations stack is an intended replacement for find-tag-marker-ring, 
which is supposedly used by all kinds of code.

So the location to which we "Go Back" might as well have been tagged by 
a command which otherwise has to relation to Xref

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Glenn Morris]

Causes xref-elisp-test-find-defs-defvar-el to fail; ref eg
https://hydra.nixos.org/build/156713513

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

25 okt. 2021 kl. 23.29 skrev Glenn Morris <rgm@gnu.org>:

> Causes xref-elisp-test-find-defs-defvar-el to fail

Fixed, thank you!

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

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

> I favour (lightly) "Go Back" / "Go Forward" because they make sense in
> context with the other Xref operations in that separator-delimited group of
> entries in the menu, and because it's a wording often used for these
> operations elsewhere such as in web browsers and IDEs (though often it's
> just "Back" and "Forward").

There are more votes for "Go Back" / "Go Forward", so I'll push
two patches - one for emacs-28 (not to be merged to master), and
another for master:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: go-back-emacs-28.patch --]
[-- Type: text/x-diff, Size: 496 bytes --]

diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index 3e1a915e32..4997c2746f 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -51,7 +51,7 @@ prog-context-menu
 
   (unless (xref-marker-stack-empty-p)
     (define-key-after menu [xref-pop]
-      '(menu-item "Back Definition" xref-pop-marker-stack
+      '(menu-item "Go Back" xref-pop-marker-stack
                   :help "Back to the position of the last search")
       'prog-separator))
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: go-back-master.patch --]
[-- Type: text/x-diff, Size: 792 bytes --]

diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index db350a5f70..30b6edf0d9 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -49,9 +49,15 @@ prog-context-menu
   (define-key-after menu [prog-separator] menu-bar-separator
     'middle-separator)
 
+  (unless (xref-forward-history-empty-p)
+    (define-key-after menu [xref-forward]
+      '(menu-item "Go Forward" xref-go-forward
+                  :help "Forward to the position gone Back from")
+      'prog-separator))
+
   (unless (xref-marker-stack-empty-p)
     (define-key-after menu [xref-pop]
-      '(menu-item "Back Definition" xref-go-back
+      '(menu-item "Go Back" xref-go-back
                   :help "Back to the position of the last search")
       'prog-separator))
 

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Juri Linkov]

>> I favour (lightly) "Go Back" / "Go Forward" because they make sense in
>> context with the other Xref operations in that separator-delimited group of
>> entries in the menu, and because it's a wording often used for these
>> operations elsewhere such as in web browsers and IDEs (though often it's
>> just "Back" and "Forward").
>
> There are more votes for "Go Back" / "Go Forward", so I'll push
> two patches - one for emacs-28 (not to be merged to master), and
> another for master:

Now pushed both.  Is this feature request is ready for closing?

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Mattias Engdegård]

9 nov. 2021 kl. 09.30 skrev Juri Linkov <juri@linkov.net>:

> Now pushed both.  Is this feature request is ready for closing?

Thank you and yes, as far as I'm concerned. An unbounded number of improvements that can be made but we don't need to keep the bug open for that, do we?

bug#38797: 27.0.50; Feature request: provide the opposite of xref-pop-marker-stack

[Dmitry Gutov]

On 09.11.2021 16:08, Mattias Engdegård wrote:
> 9 nov. 2021 kl. 09.30 skrev Juri Linkov<juri@linkov.net>:
> 
>> Now pushed both.  Is this feature request is ready for closing?
> Thank you and yes, as far as I'm concerned. An unbounded number of improvements that can be made but we don't need to keep the bug open for that, do we?
> 
> 

Looks good to me as well. Thank you both.

Closing the bug.