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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joaotavora@gmail.com (João Távora)
>> Cc: dgutov@yandex.ru,  28814@debbugs.gnu.org
>> Date: Fri, 27 Oct 2017 00:59:28 +0100
>> 
>> - In node "Looking up identifiers" there are is a repeated explanation
>> of what motivates a *xref* buffer (lines 1831 and 1863). I think its
>> clearer if this only happens once, so I merged the two.
>
> People who use the manual as a reference seldom read the entire node.
> Instead, they read the description of the single subject they were

I think nodes are read from top to bottom, especially if they are short,
and this is a good thing.  It's a small miracle the manual is so good
since it is a giant patchwork of many different writers.  Nevertheless
it suffers from inconsistent style.  I think repetition is very often a
symptom of bad style.  And I think style isn't some abstract and
innocuous thing, it's a carrier for content and carries content in
itself.  And I think this even more of prose than of code.

So here ends my minirant :-) and I hope I fixed everything in these
patches.

João

>From 353d2a44169697772cc0a341edc36a72c8abd9d7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Fri, 13 Oct 2017 15:13:14 +0100
Subject: [PATCH 1/3] 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..63ef901a9e 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.
+Honor `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 ee52f73d441577d1beb48f9d7c5c3ce3f26a48ed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 23 Oct 2017 09:05:32 +0100
Subject: [PATCH 2/3] 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 6c5191316688ccf50c1874b976b1f66741a900ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Fri, 13 Oct 2017 16:37:47 +0100
Subject: [PATCH 3/3] New xref-quit-and-goto-xref command bound to TAB
 (bug#28814)

This is like xref-goto-xref, but quits the *xref* window just before
the user 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.

* doc/emacs/maintaining.texi (Xref Commands): Describe new bindings in
*xref*.

* etc/NEWS (Xref): Describe new binding.
---
 doc/emacs/maintaining.texi |  7 +++++--
 etc/NEWS                   | 10 ++++++++++
 lisp/progmodes/xref.el     | 24 +++++++++++++++++++-----
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/doc/emacs/maintaining.texi b/doc/emacs/maintaining.texi
index dc0a71511f..112f1f4d9e 100644
--- a/doc/emacs/maintaining.texi
+++ b/doc/emacs/maintaining.texi
@@ -1887,8 +1887,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 +1902,10 @@ 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
+@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..561a15dbd7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1185,6 +1185,16 @@ 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, 'TAB' quits and jumps to an xref
+
+A new command 'xref-quit-and-goto-xref', bound to 'TAB' in *xref*
+buffers, quits the window before jumping to the destination. In many
+situations, the intended window configuration is restored, just as if
+the *xref* buffer hadn't been necessary in the first place.
+
 
 * New Modes and Packages in Emacs 26.1
 
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 63ef901a9e..623fd5eb25 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,7 @@ 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 "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