Subject: bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost )

From: [email-masked] (João Távora)
Date: Wed, 25 Oct 2017 21:56:21 +0100

Eli Zaretskii <[email-masked]> writes:

> As for when we can't pop up a new window: would it be okay to reuse
> the current window only in that (hopefully, rare) case?

We could do that (I kinda like it) but it's actually harder to
implement, I think. We'd have to precisely control the order of
display-buffer fallbacks or repeat somewhere else the logic of
split-window-sensibly. I'd leave this for an implementation on master.

>> > As for quitting the XREF buffer when it's no longer needed: how about
>> > 'q', like other similar modes do, or some variety thereof?  "C-u RET"
>> > is too odd, almost outlandish in Emacs.
>> 
>> ’q’ is already taken by ’quit-window’ in *xref* buffers. It quits the
>> window and does nothing else. I’m looking for a command that quits *and*
>> goes to the target.
>
> How about 'Q'?

OK. I used 'Q'. And added 'TAB'. Because it's a bit what happens in
completion, which also selects and quits the transient completions
window.

I can take 'TAB' out, if you think it's a too high profile a key for
something like this.

> The problem with binding this "quit and go to reference" function to
> RET is that it is unlike any other similar feature we have: RET
> usually selects the item, but does not quit any windows.

I see. Well, I really don't have any strong arguments against that,
other than my feeling that, because *xref* is such a suprise window,
even unexpected, that criteria would somehow not apply here.  Or rather
it is superseeded by a superior mandate to not clutter the user's
windows with unexpected cruft.  As I mentioned, xref is rooted in SLIME,
and I believe (but I might be mistaken now), it used to work like that
in that program.

Anyway, I'm happy with the new command.

>> It’s a bugfix, so emacs-26.
>
> I'd need to see the patches in their last incarnation (if you already
> posted them, and nothing needs to be changed, a URL will be
> appreciated).  In general, this changes user-visible behavior in
> non-trivial ways, so it's borderline between a bugfix and a new
> feature.  But if the patches are small and simple enough, I guess they
> could be okay for emacs-26.

Let's hope they are. Attached are 4 patches. The 2 first are part of the
bugfix. Number 3 is the new xref-quit-and-goto-xref and number 4 are
documentation changes to the .texi manual (also fixed a small bug there)
and NEWS.

>From f4f774416ca0e90d351b10e7da2095ec9a9ba530 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <[email-masked]>
Date: Fri, 13 Oct 2017 15:13:14 +0100
Subject: [PATCH 1/4] Honor window-switching intents in xref-find-definitions
 (bug#28814)

When there is more than one xref to jump to, and an *xref* window
appears to help the user choose, the original intent to open a
definition in another window or frame is remembered when the choice to
go to or show a reference is finally made.

* lisp/progmodes/xref.el (xref--show-pos-in-buf): Rewrite.
(xref--original-window-intent): New variable.
(xref--original-window): Rename from xref--window and move up
here for clarity.
(xref--show-pos-in-buf): Rewrite.  Don't take SELECT arg here.
(xref--show-location): Handle window selection decision here.
(xref--window): Rename to xref--original-window.
(xref-show-location-at-point): Don't attempt window management here.
(xref--show-xrefs): Ensure display-action intent is saved.
---
 lisp/progmodes/xref.el | 69 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 80cdcb3f18..c3e7982205 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -449,43 +449,68 @@ xref--with-dedicated-window
        (when xref-w
          (set-window-dedicated-p xref-w xref-w-dedicated)))))
 
-(defun xref--show-pos-in-buf (pos buf select)
-  (let ((xref-buf (current-buffer))
-        win)
+(defvar-local xref--original-window-intent nil
+  "Original window-switching intent before xref buffer creation.")
+
+(defvar-local xref--original-window nil
+  "The original window this xref buffer was created from.")
+
+(defun xref--show-pos-in-buf (pos buf)
+  "Goto and display position POS of buffer BUF in a window.
+Honour `xref--original-window-intent', run `xref-after-jump-hook'
+and finally return the window."
+  (let* ((xref-buf (current-buffer))
+         (pop-up-frames
+          (or (eq xref--original-window-intent 'frame)
+              pop-up-frames))
+         (action
+          (cond ((memq
+                  xref--original-window-intent
+                  '(window frame))
+                 t)
+                ((and
+                  (window-live-p xref--original-window)
+                  (or (not (window-dedicated-p xref--original-window))
+                      (eq (window-buffer xref--original-window) buf)))
+                 `(,(lambda (buf _alist)
+                      (set-window-buffer xref--original-window buf)
+                      xref--original-window))))))
     (with-selected-window
-        (xref--with-dedicated-window
-         (display-buffer buf))
+        (with-selected-window
+            ;; Just before `display-buffer', place ourselves in the
+            ;; original window to suggest preserving it. Of course, if
+            ;; user has deleted the original window, all bets are off,
+            ;; just use the selected one.
+            (or (and (window-live-p xref--original-window)
+                     xref--original-window)
+                (selected-window))
+          (display-buffer buf action))
       (xref--goto-char pos)
       (run-hooks 'xref-after-jump-hook)
       (let ((buf (current-buffer)))
-        (setq win (selected-window))
         (with-current-buffer xref-buf
-          (setq-local other-window-scroll-buffer buf))))
-    (when select
-      (select-window win))))
+          (setq-local other-window-scroll-buffer buf)))
+      (selected-window))))
 
 (defun xref--show-location (location &optional select)
   (condition-case err
       (let* ((marker (xref-location-marker location))
              (buf (marker-buffer marker)))
-        (xref--show-pos-in-buf marker buf select))
+        (cond (select
+               (select-window (xref--show-pos-in-buf marker buf)))
+              (t
+               (save-selected-window
+                 (xref--with-dedicated-window
+                  (xref--show-pos-in-buf marker buf))))))
     (user-error (message (error-message-string err)))))
 
-(defvar-local xref--window nil
-  "The original window this xref buffer was created from.")
-
 (defun xref-show-location-at-point ()
   "Display the source of xref at point in the appropriate window, if any."
   (interactive)
   (let* ((xref (xref--item-at-point))
          (xref--current-item xref))
     (when xref
-      ;; Try to avoid the window the current xref buffer was
-      ;; originally created from.
-      (if (window-live-p xref--window)
-          (with-selected-window xref--window
-            (xref--show-location (xref-item-location xref)))
-        (xref--show-location (xref-item-location xref))))))
+      (xref--show-location (xref-item-location xref)))))
 
 (defun xref-next-line ()
   "Move to the next xref and display its source in the appropriate window."
@@ -727,7 +752,8 @@ xref--show-xref-buffer
         (xref--xref-buffer-mode)
         (pop-to-buffer (current-buffer))
         (goto-char (point-min))
-        (setq xref--window (assoc-default 'window alist))
+        (setq xref--original-window (assoc-default 'window alist)
+              xref--original-window-intent (assoc-default 'display-action alist))
         (current-buffer)))))
 
 
@@ -754,7 +780,8 @@ xref--show-xrefs
    (t
     (xref-push-marker-stack)
     (funcall xref-show-xrefs-function xrefs
-             `((window . ,(selected-window)))))))
+             `((window . ,(selected-window))
+               (display-action . ,display-action))))))
 
 (defun xref--prompt-p (command)
   (or (eq xref-prompt-for-identifier t)
-- 
2.14.2

>From aae81ea38c6062399c60d0018533e4be359bae6e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <[email-masked]>
Date: Mon, 23 Oct 2017 09:05:32 +0100
Subject: [PATCH 2/4] Allow split-window-sensibly to split threshold in further
 edge case

As a fallback, and to avoid creating a frame, split-window-sensibly
would previously disregard split-height-threshold if the window to be
split is the frame's root window.

This change generalizes that: it disregards the threshold if the
window to be split is the frame's only _usable_ window (it is either
the only one, as before, or all the other windows are dedicated to
some buffer and thus cannot be touched).

This is required for the fix to bug#28814.

* lisp/window.el (split-height-threshold): Adjust doc to match
split-window-sensibly.
(split-window-sensibly): Also disregard threshold if all other
windows are dedicated.
---
 lisp/window.el | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index c0a9ecd093..4814d12400 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6465,8 +6465,9 @@ split-height-threshold
 vertically only if it has at least this many lines.  If this is
 nil, `split-window-sensibly' is not allowed to split a window
 vertically.  If, however, a window is the only window on its
-frame, `split-window-sensibly' may split it vertically
-disregarding the value of this variable."
+frame, or all the other ones are dedicated,
+`split-window-sensibly' may split it vertically disregarding the
+value of this variable."
   :type '(choice (const nil) (integer :tag "lines"))
   :version "23.1"
   :group 'windows)
@@ -6573,15 +6574,27 @@ split-window-sensibly
 	     ;; Split window horizontally.
 	     (with-selected-window window
 	       (split-window-right)))
-	(and (eq window (frame-root-window (window-frame window)))
-	     (not (window-minibuffer-p window))
-	     ;; If WINDOW is the only window on its frame and is not the
-	     ;; minibuffer window, try to split it vertically disregarding
-	     ;; the value of `split-height-threshold'.
-	     (let ((split-height-threshold 0))
-	       (when (window-splittable-p window)
-		 (with-selected-window window
-		   (split-window-below))))))))
+	(and
+         ;; If WINDOW is the only usable window on its frame (it is
+         ;; the only one or, not being the only one, all the other
+         ;; ones are dedicated) and is not the minibuffer window, try
+         ;; to split it vertically disregarding the value of
+         ;; `split-height-threshold'.
+         (let ((frame (window-frame window)))
+           (or
+            (eq window (frame-root-window frame))
+            (catch 'done
+              (walk-window-tree (lambda (w)
+                                  (unless (or (eq w window)
+                                              (window-dedicated-p w))
+                                    (throw 'done nil)))
+                                frame)
+              t)))
+	 (not (window-minibuffer-p window))
+	 (let ((split-height-threshold 0))
+	   (when (window-splittable-p window)
+	     (with-selected-window window
+	       (split-window-below))))))))
 
 (defun window--try-to-split-window (window &optional alist)
   "Try to split WINDOW.
-- 
2.14.2

>From 4cfc9f26f65038d088649789759ac7d2414ac2b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <[email-masked]>
Date: Fri, 13 Oct 2017 16:37:47 +0100
Subject: [PATCH 3/4] Provide new xref-quit-and-goto-xref, bind it to "Q" and
 "TAB" (bug#28814)

This quits the *xref* window just before the user decides jump to ref.

* lisp/progmodes/xref.el (xref--show-location): Handle 'quit
value for SELECT.
(xref-goto-xref): Take optional QUIT arg.
(xref-quit-and-goto-xref): New command.
(xref--xref-buffer-mode-map): Bind "Q" and "TAB" to
xref-quit-and-goto-xref.
---
 lisp/progmodes/xref.el | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index c3e7982205..aeeeba8051 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -493,11 +493,17 @@ xref--show-pos-in-buf
       (selected-window))))
 
 (defun xref--show-location (location &optional select)
+  "Helper for `xref-show-xref' and `xref-goto-xref'.
+Go to LOCATION and if SELECT is non-nil select its window.  If
+SELECT is `quit', also quit the *xref* window."
   (condition-case err
       (let* ((marker (xref-location-marker location))
-             (buf (marker-buffer marker)))
+             (buf (marker-buffer marker))
+             (xref-buffer (current-buffer)))
         (cond (select
-               (select-window (xref--show-pos-in-buf marker buf)))
+               (if (eq select 'quit) (quit-window nil nil))
+               (with-current-buffer xref-buffer
+                 (select-window (xref--show-pos-in-buf marker buf))))
               (t
                (save-selected-window
                  (xref--with-dedicated-window
@@ -529,12 +535,19 @@ xref--item-at-point
     (back-to-indentation)
     (get-text-property (point) 'xref-item)))
 
-(defun xref-goto-xref ()
-  "Jump to the xref on the current line and select its window."
+(defun xref-goto-xref (&optional quit)
+  "Jump to the xref on the current line and select its window.
+Non-interactively, non-nil QUIT says to first quit the *xref*
+buffer."
   (interactive)
   (let ((xref (or (xref--item-at-point)
                   (user-error "No reference at point"))))
-    (xref--show-location (xref-item-location xref) t)))
+    (xref--show-location (xref-item-location xref) (if quit 'quit t))))
+
+(defun xref-quit-and-goto-xref ()
+  "Quit *xref* buffer, then jump to xref on current line."
+  (interactive)
+  (xref-goto-xref t))
 
 (defun xref-query-replace-in-results (from to)
   "Perform interactive replacement of FROM with TO in all displayed xrefs.
@@ -658,6 +671,8 @@ xref--xref-buffer-mode-map
     (define-key map (kbd "p") #'xref-prev-line)
     (define-key map (kbd "r") #'xref-query-replace-in-results)
     (define-key map (kbd "RET") #'xref-goto-xref)
+    (define-key map (kbd "Q") #'xref-quit-and-goto-xref)
+    (define-key map (kbd "TAB")  #'xref-quit-and-goto-xref)
     (define-key map (kbd "C-o") #'xref-show-location-at-point)
     ;; suggested by Johan Claesson "to further reduce finger movement":
     (define-key map (kbd ".") #'xref-next-line)
-- 
2.14.2

>From ae3a5adda7debf77e03c151b84c1062ae90b6970 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <[email-masked]>
Date: Wed, 25 Oct 2017 19:27:15 +0100
Subject: [PATCH 4/4] Document changes to xref behavior in NEWS and texi manual
 (bug#28814)

* doc/emacs/maintaining.texi (Looking Up Identifiers): Mention
new bindings in *xref*, rework slightly to avoid repetition.
(Xref Commands): Describe new bindings in *xref*.

* etc/NEWS (Xref): New section describing bugfix and new bindings.
---
 doc/emacs/maintaining.texi | 52 ++++++++++++++++++++++++++--------------------
 etc/NEWS                   | 20 ++++++++++++++++++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index dc0a71511f..650419a855 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -1828,15 +1828,6 @@ Looking Up Identifiers
 to always prompt, customize @code{xref-prompt-for-identifier} to
 @code{t}.)
 
-If the specified identifier has only one definition, the command jumps
-to it.  If the identifier has more than one possible definition (e.g.,
-in an object-oriented language, or if there's a function and a
-variable by the same name), the command shows the candidate
-definitions in a @file{*xref*} buffer, together with the files in
-which these definitions are found.  Selecting one of these candidates
-by typing @kbd{@key{RET}} or clicking @kbd{mouse-2} will pop a buffer
-showing the corresponding definition.
-
   When entering the identifier argument to @kbd{M-.}, the usual
 minibuffer completion commands can be used (@pxref{Completion}), with
 the known identifier names as completion candidates.
@@ -1860,21 +1851,34 @@ Looking Up Identifiers
 matching of identifiers instead of matching symbol names as fixed
 strings.
 
-  When any of the above commands finds more than one definition, it
-presents the @file{*xref*} buffer showing the definition candidates.
-In that buffer, you have several specialized commands, described in
-@ref{Xref Commands}.
+When any of these commands finds a unique definition for the specified
+identifier, the command jumps to that location.  If, however, the
+identifier has more than one possible definition (e.g., in an
+object-oriented language, or if there's a function and a variable by
+the same name), the command shows the candidate definitions in a
+@file{*xref*} buffer, together with the files in which these
+definitions are found.
+
+Selecting one of these candidates by typing @kbd{@key{RET}} or
+clicking @kbd{mouse-2} will pop a buffer showing the corresponding
+definition.  If you only want to display that buffer, but still remain
+in @file{*xref*} window, you can also type @kbd{C-o} to do so.
+Finally, if you're quite sure you're already at the definition you
+want to jump to, you can press @kbd{@key{TAB}} to jump to the buffer
+and dismiss the @file{*xref*} window.  For a complete list of commands
+available in this buffer, @ref{Xref Commands}.
 
 @kindex M-,
 @findex xref-pop-marker-stack
 @vindex xref-marker-ring-length
-  To go back to places @emph{from where} you found 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.
+  Once you arrive at the desired definition, you may want to go back
+to places @emph{from where} you found the definition. For this, 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.
 
 @node Xref Commands
 @subsubsection Commands Available in the @file{*xref*} Buffer
@@ -1887,8 +1891,7 @@ Xref Commands
 @table @kbd
 @item @key{RET}
 @itemx mouse-2
-Display the reference on the current line and bury the @file{*xref*}
-buffer.
+Display the reference on the current line.
 @item n
 @itemx .
 @findex xref-next-line
@@ -1903,6 +1906,11 @@ Xref Commands
 @findex xref-show-location-at-point
 Display the reference on the current line in the other window
 (@code{xref-show-location-at-point}).
+@item TAB
+@itemx Q
+@findex xref-quit-and-goto-xref
+Display the reference on the current line and bury the @file{*xref*}
+buffer (@code{xref-quit-and-goto-xref}).
 @findex xref-query-replace-in-results
 @item r @var{pattern} @key{RET} @var{replacement} @key{RET}
 Perform interactive query-replace on references that match
diff --git a/etc/NEWS b/etc/NEWS
index 82778932ab..b79a0019d8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1185,6 +1185,26 @@ New user options `term-char-mode-buffer-read-only' and
 are non-nil by default.  Customize these options to nil if you want
 the previous behavior.
 
+** Xref
+
+*** When an *xref* buffer is needed, window-switching intent is preserved
+
+This most visibly affects the commands
+'xref-find-definitions-other-window' and
+'xref-find-definitions-other-frame' bound to 'C-x 4 .' and 'C-x 5 .'
+by default. When selecting a cross reference from the *xref* list,
+Emacs remembers that the user's intention was keep the originally
+selected window/frame's contents. Similarly, the original window is
+always chosen by the regular 'xref-find-definitions' command,
+regardless if an *xref* buffer is needed or not.
+
++++
+*** New command 'xref-quit-and-goto-xref' quits and jumps to an xref
+
+This command is bound to 'Q' and 'TAB' by default. By quitting
+the *xref* window, it makes space for new windows, just as if
+the *xref* hadn't been necessary in the first place.
+
 
 * New Modes and Packages in Emacs 26.1
 
-- 
2.14.2

João