bug#28814: 26.0.90; When *xref* window is needed, original window-switching intent is lost

bug#28814: 26.0.90; When *xref* window is needed, original window-switching intent is lost

[João Távora]

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

Hi Dmitry, maintainters,

Here are two patches to fix what I believe is a small but annoying bug
in xref.el.

I'll try to explain as clearly as possible: As you know, if the user
presses 'M-.' in a single-ref definition, he/she is transported to a new
buffer.

But there are other situations: when xref-find-definitions finds more
than one definition, a list is shown in an *xref* buffer, normally in a
new window. When the user selects an "xref" with xref-goto-xref, a
buffer and window switch happen.  Anyway, so far so good.

The problem is there is also 'C-x 4 .' and 'C-x 5 .' for
xref-find-definitions-other-window and xref-find-definitions-other-frame
respectively. These work just fine when an *xref* buffer isn't needed,
but when it is, the original intent of using another window or frame
will be lost when the user eventually selects a definition.

It shouldn't be so, in my opinion.

The first patch I attach (0001-Honor-window....patch) fixes this bug. I
hope it is readable enough but I can explain how it works in detail.

I also attach a second patch (0002-Quit-the....patch), that does not
really fix a bug, but changes the behavior of xref-goto-xref to
something much nicer: it quits the *xref* window before going to the
reference.

This brings a nice result: As always 'M-.' switches buffers if there is
only one definition.  Now, if there is more than one, the final state
after selecting one of these definitions is the same as if there had
only been one in the first place.  I think this makes sense because it
preserves the expectations of the user who probably wants M-. to behave
as predictably as possible.

Here's hoping you're not really confused by this report,
João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Honor-window-switching-intents-in-xref-find-definiti.patch --]
[-- Type: text/x-diff, Size: 5872 bytes --]

From 1cba860d6a2c45e0fa690065b2bf4e6658e87628 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/2] Honor window-switching intents in xref-find-definitions

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 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 | 73 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 80cdcb3f18..768fa15a6b 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -449,43 +449,72 @@ 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
+                  (let (;; save-selected-window doesn't resist frame
+                        ;; raises
+                        (display-buffer-overriding-action
+                         '(nil . ((inhibit-switch-frame . t)))))
+                    (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 +756,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)))))
 
 \f
@@ -754,7 +784,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.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Quit-the-xref-window-if-user-decides-to-go-to-a-ref.patch --]
[-- Type: text/x-diff, Size: 1248 bytes --]

From 228cb812197bd68b2fb6eccc60d7956675a728f3 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 2/2] Quit the *xref* window if user decides to go to a ref

* lisp/progmodes/xref.el (xref--show-location): When SELECT is
t, quit window.
---
 lisp/progmodes/xref.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 768fa15a6b..3a5e9e53ed 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -495,9 +495,12 @@ xref--show-pos-in-buf
 (defun xref--show-location (location &optional select)
   (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)))
+               (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
-- 
2.11.0

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

[João Távora]

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

tags 28814 patch

On Fri, Oct 13, 2017 at 5:08 PM, GNU bug Tracking System <
help-debbugs@gnu.org> wrote:

> Thank you for filing a new bug report with debbugs.gnu.org.
>
> This is an automatically generated reply to let you know your message
> has been received.
>
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
>
> Your message has been sent to the package maintainer(s):
>  bug-gnu-emacs@gnu.org
>
> If you wish to submit further information on this problem, please
> send it to 28814@debbugs.gnu.org.
>
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.
>
> --
> 28814: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=28814
> GNU Bug Tracking System
> Contact help-debbugs@gnu.org with problems
>



-- 
João Távora

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

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

[João Távora]

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

Ping,

Hoping someone can take a look at this bug I reported a week ago.  Here
are two very simple Emacs -Q recipes that demonstrate it.

   emacs -Q
   C-x 3 [split-window-right]
   C-x 2 [split-window-below]
   M-. xref-backend-definitions RET [xref-find-definitions]
   C-n [next-line]
   RET [xref-goto-xref]

Expected the definition to be found in the original window where I
pressed M-. but instead it was found in another. Another case:

   emacs -Q
   C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
   C-n
   RET

Expected the definition to be found in some other window, different from
the one I pressed M-. on. Instead went to the same one. Also, in both
situations, expected the window configuration to be the same as if I had
searched for, say xref-backend-functions.

This is fixed by the patches that I reattach after minor tweaks. The
general idea is to have the *xref*, whose sudden appearance is hard to
predict, obtrude as little as possible on the user's window
configuration.

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Honor-window-switching-intents-in-xref-find-definiti.patch --]
[-- Type: text/x-diff, Size: 5639 bytes --]

From 1b760816ac8886313996c635ee1cf696b937c20e 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/2] Honor window-switching intents in xref-find-definitions

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 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)))))
 
 \f
@@ -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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Quit-the-xref-window-if-user-decides-to-go-to-a-ref.patch --]
[-- Type: text/x-diff, Size: 1392 bytes --]

From 9feaf473479dcd8edcf2c0bb14044995abb5eeb0 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 2/2] Quit the *xref* window if user decides to go to a ref

The idea is to have this window, whose sudden appearance is hard to
predict, obtrude as little as possible on the user's window
configuration.

* lisp/progmodes/xref.el (xref--show-location): When SELECT is
t, quit window.
---
 lisp/progmodes/xref.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index c3e7982205..cf9e027ba0 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -495,9 +495,12 @@ xref--show-pos-in-buf
 (defun xref--show-location (location &optional select)
   (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)))
+               (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
-- 
2.14.2


[-- Attachment #4: Type: text/plain, Size: 681 bytes --]










help-debbugs@gnu.org (GNU bug Tracking System) writes:

> Thank you for filing a new bug report with debbugs.gnu.org.
>
> This is an automatically generated reply to let you know your message
> has been received.
>
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
>
> Your message has been sent to the package maintainer(s):
>  bug-gnu-emacs@gnu.org
>
> If you wish to submit further information on this problem, please
> send it to 28814@debbugs.gnu.org.
>
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.

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

[Dmitry Gutov]

Hi Joao,

On 10/20/17 12:13 PM, João Távora wrote:

> Hoping someone can take a look at this bug I reported a week ago.

Sorry for the late reply. I'll look at it this weekend.

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

[Dmitry Gutov]

Hi Joao,

On 10/20/17 12:13 PM, João Távora wrote:
> Hoping someone can take a look at this bug I reported a week ago.  Here
> are two very simple Emacs -Q recipes that demonstrate it.

You are working on something that I agree is a real problem, but doing 
this would effectively revert commit 
5698947ff9335cf150c73cca257a5e7e5951b045, which was based on a previous 
discussion (see the link in the commit message), and in particular, 
would bring back "disappearing *xref* buffer problem", as Eli put it.

The essence of the problem is that xref buffers try to serve two related 
but distinct purposes that the user might be aiming at:

- Pick a result from the list and jump to it, forgetting the rest 
(basically providing a completion interface).
- Iterate through results and do something with each of them in turn.

Even for xref-find-definitions, we can't rule out the purpose #2. Again, 
also see the previous discussion.

I have a rough idea on how to fix the situation, but nothing even close 
to an implementation.

>     emacs -Q
>     C-x 3 [split-window-right]
>     C-x 2 [split-window-below]
>     M-. xref-backend-definitions RET [xref-find-definitions]
>     C-n [next-line]
>     RET [xref-goto-xref]
> 
> Expected the definition to be found in the original window where I
> pressed M-. but instead it was found in another. Another case:
> 
>     emacs -Q
>     C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
>     C-n
>     RET
> 
> Expected the definition to be found in some other window, different from
> the one I pressed M-. on. Instead went to the same one.

With your patches applied, this example pops a new frame for me if I 
press 'n' instead of 'C-n'. This is not hugely important in the light of 
the larger problems above, but demonstrated difficulties with window 
management when we try to pretend that the xref buffer "was never there".

> Also, in both
> situations, expected the window configuration to be the same as if I had
> searched for, say xref-backend-functions.
> 
> This is fixed by the patches that I reattach after minor tweaks. The
> general idea is to have the *xref*, whose sudden appearance is hard to
> predict, obtrude as little as possible on the user's window
> configuration.

If we don't bring back the "disappearing *xref* buffer problem", *xref* 
has to stay obtrusive. Do you think the rest of your patch will make 
sense with that change?

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

[João Távora]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> You are working on something that I agree is a real problem, but doing
> this would effectively revert commit
> 5698947ff9335cf150c73cca257a5e7e5951b045, which was based on a
> previous discussion (see the link in the commit message), and in
> particular, would bring back "disappearing *xref* buffer problem", as
> Eli put it.
> [...]
>
> I have a rough idea on how to fix the situation, but nothing even
> close to an implementation.
>
Thanks for the pointer. After I read that discussion I will probably ask
you about that.

>>     emacs -Q
>>     C-x 3 [split-window-right]
>>     C-x 2 [split-window-below]
>>     M-. xref-backend-definitions RET [xref-find-definitions]
>>     C-n [next-line]
>>     RET [xref-goto-xref]
>>
>> Expected the definition to be found in the original window where I
>> pressed M-. but instead it was found in another. Another case:
>>
>>     emacs -Q
>>     C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
>>     C-n
>>     RET
>>
>> Expected the definition to be found in some other window, different from
>> the one I pressed M-. on. Instead went to the same one.
>
> With your patches applied, this example pops a new frame for me if I
> press 'n' instead of 'C-n'. This is not hugely important in the light
> of the larger problems above, but demonstrated difficulties with
> window management when we try to pretend that the xref buffer "was
> never there".

You are absolutely right (and you don't have to tell me how hairy
window-management code is). This particular problem, which I had also
noticed, slipped through. I have a fix attached as a patch. 

The cause of this problem is that split-window-sensibly refuses to split
a window whose dimensions are below those of split-height-threshold and
split-width-threshold. The reason you don't see frames popping up every
time you do C-x 4 b on a small frame is that this function contains a
safety net for these cases: if the window to be split is the only one
available in the frame, it disregards the dimension threshholds and
splits anyway. The attached window.el patch is a correct way to
generalize this to account for dedicated windows.

If this is not accepted this local fix in xref.el will also work fine.

   @@ -504,7 +504,8 @@ xref--show-location
                  (t
                   (save-selected-window
                     (xref--with-dedicated-window
   -                  (xref--show-pos-in-buf marker buf))))))
   +                  (let ((split-height-threshold 0))
   +                    (xref--show-pos-in-buf marker buf)))))))
        (user-error (message (error-message-string err)))))
    
    (defun xref-show-location-at-point ()

>> Also, in both
>> situations, expected the window configuration to be the same as if I had
>> searched for, say xref-backend-functions.
>>
>> This is fixed by the patches that I reattach after minor tweaks. The
>> general idea is to have the *xref*, whose sudden appearance is hard to
>> predict, obtrude as little as possible on the user's window
>> configuration.p
>
> If we don't bring back the "disappearing *xref* buffer problem",
> *xref* has to stay obtrusive. Do you think the rest of your patch will
> make sense with that change?

I see and I will try to answer. I proposed two patches previously:

* a first one to fix the non-determinism of window popping/selecting
behaviour;
* a second one to make the *xref* buffer less obstrusive.
* (now there is the third one that fixes the frame-popping glitch)

IIUC it is the second one that clashes with "the dissapearing *xref*
problem" that I have yet to read up on. If we don't come up with a
solution for that, I would be OK with a solution that leaves it unsolved
but adds some customization point (hook) for the user to put this
behaviour in.

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch --]
[-- Type: text/x-diff, Size: 3151 bytes --]

From 47480926f328db5d840ba518125e0866d29f8665 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 3/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 slightly expands on 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).

* 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 | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 5ba9a305f9..21271944c0 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6449,8 +6449,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)
@@ -6557,15 +6558,25 @@ 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
+         (let ((frame (window-frame window)))
+           (or
+            (eq window (frame-root-window frame))
+            (let ((windows (delete window (window-list frame)))
+                  w)
+              (while (and (setq w (pop windows))
+                          (window-dedicated-p w)))
+              (not windows))))
+	 (not (window-minibuffer-p window))
+	 ;; 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 ((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

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

[João Távora]

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

Hi Dmitry, Eli,

I read the discussion you pointed me to me by Dmitry in
http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01235.html.

Eli, If I understand your concerns there, then the first and third
patches I proposed shouldn't in any way interfere with your use of
*xref*-related facilities.  If anything, they should improve it.

The second patch does indeed bring the problem known as "the
disappearing *xref* problem", so I mended that with a very simple fourth
patch, described below.

So now, to summarize, there are 4 patches in total, that I reattach:

* 0001-Honor-window-switching-intents-in-xref-find-definiti.patch

This fixes the problem with the non-deterministic behaviour of selecting
a window for displaying cross-references, as well the problem where the
initial window/frame switching intent is lost.

* 0002-Quit-the-xref-window-if-user-decides-to-go-to-a-ref.patch

This makes the xref buffer non-obstrusive by quitting it on
xref-goto-xref. This is a change to a current behaviour that was
specifically requested by Eli (the "the disappearing *xref* problem").

* 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch

This extends the exception granted by split-window-sensibly to
single-window frames whose dimensions are below those of splitting
thresholds to consider multi-window frames where all but one window is
dedicated.

In practice, it fixes the case where

  C-x 4 . xref-backend-definitions RET n

would surprisingly pop-up a new frame if the original frame was already
small to start with.

This fix to window.el appears very sound to me, but if it is not desired
for whatever reason, a more localized fix in xref.el is also possible.

* 0004-Don-t-quit-xref-window-on-RET-only-on-C-u-RET.patch

This fixes the "disappearing *xref* problem", by bringing back the
default behaviour of not quitting the *xref* window on RET, although
allowing for that if the user types C-u RET instead.

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Honor-window-switching-intents-in-xref-find-definiti.patch --]
[-- Type: text/x-diff, Size: 5639 bytes --]

From 1b760816ac8886313996c635ee1cf696b937c20e 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/4] Honor window-switching intents in xref-find-definitions

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 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)))))
 
 \f
@@ -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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Quit-the-xref-window-if-user-decides-to-go-to-a-ref.patch --]
[-- Type: text/x-diff, Size: 1392 bytes --]

From 9feaf473479dcd8edcf2c0bb14044995abb5eeb0 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 2/4] Quit the *xref* window if user decides to go to a ref

The idea is to have this window, whose sudden appearance is hard to
predict, obtrude as little as possible on the user's window
configuration.

* lisp/progmodes/xref.el (xref--show-location): When SELECT is
t, quit window.
---
 lisp/progmodes/xref.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index c3e7982205..cf9e027ba0 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -495,9 +495,12 @@ xref--show-pos-in-buf
 (defun xref--show-location (location &optional select)
   (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)))
+               (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
-- 
2.14.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch --]
[-- Type: text/x-diff, Size: 3151 bytes --]

From 47480926f328db5d840ba518125e0866d29f8665 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 3/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 slightly expands on 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).

* 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 | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 5ba9a305f9..21271944c0 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6449,8 +6449,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)
@@ -6557,15 +6558,25 @@ 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
+         (let ((frame (window-frame window)))
+           (or
+            (eq window (frame-root-window frame))
+            (let ((windows (delete window (window-list frame)))
+                  w)
+              (while (and (setq w (pop windows))
+                          (window-dedicated-p w)))
+              (not windows))))
+	 (not (window-minibuffer-p window))
+	 ;; 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 ((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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Don-t-quit-xref-window-on-RET-only-on-C-u-RET.patch --]
[-- Type: text/x-diff, Size: 2127 bytes --]

From 1b527b10ad44ee4863e87700fb50dcfda14c72f5 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 20:51:54 +0100
Subject: [PATCH 4/4] Don't quit *xref* window on RET, only on C-u RET

* lisp/progmodes/xref.el (xref--show-location): Handle new
'quit value for SELECT.
(xref-goto-xref): Allow prefix argument.
---
 lisp/progmodes/xref.el | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index cf9e027ba0..9dc78397eb 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -493,12 +493,15 @@ 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))
              (xref-buffer (current-buffer)))
         (cond (select
-               (quit-window nil nil)
+               (if (eq select 'quit) (quit-window nil nil))
                (with-current-buffer xref-buffer
                  (select-window (xref--show-pos-in-buf marker buf))))
               (t
@@ -532,12 +535,13 @@ 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."
-  (interactive)
+(defun xref-goto-xref (&optional quit)
+  "Jump to the xref on the current line and select its window.
+With QUIT (the prefix argument) also quit the *xref* window."
+  (interactive "P")
   (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-query-replace-in-results (from to)
   "Perform interactive replacement of FROM with TO in all displayed xrefs.
-- 
2.14.2

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

[Dmitry Gutov]

On 10/23/17 11:23 AM, João Távora wrote:

> The cause of this problem is that split-window-sensibly refuses to split
> a window whose dimensions are below those of split-height-threshold and
> split-width-threshold. The reason you don't see frames popping up every
> time you do C-x 4 b on a small frame is that this function contains a
> safety net for these cases: if the window to be split is the only one
> available in the frame, it disregards the dimension threshholds and
> splits anyway. The attached window.el patch is a correct way to
> generalize this to account for dedicated windows.

OK, but is it the correct thing to do? The thresholds are there for a 
reason, and having a window that's only a few lines tall (which could 
happen in some example) will hardly be more helpful than showing it in a 
different window, even if the user expected xref to use the "other window".

This stuff is difficult, and personally I don't like either of the 
easily reachable solutions.

> I see and I will try to answer. I proposed two patches previously:
> 
> * a first one to fix the non-determinism of window popping/selecting
> behaviour;
> * a second one to make the *xref* buffer less obstrusive.
> * (now there is the third one that fixes the frame-popping glitch)
> 
> IIUC it is the second one that clashes with "the dissapearing *xref*
> problem" that I have yet to read up on. If we don't come up with a
> solution for that, I would be OK with a solution that leaves it unsolved
> but adds some customization point (hook) for the user to put this
> behaviour in.

Indeed, but there's also a matter of consistency, and of making the 
overall design work in a predictable fashion. More in the follow-up email.

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

[Dmitry Gutov]

On 10/23/17 11:03 PM, João Távora wrote:

> I read the discussion you pointed me to me by Dmitry in
> http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01235.html.
> 
> Eli, If I understand your concerns there, then the first and third
> patches I proposed shouldn't in any way interfere with your use of
> *xref*-related facilities.  If anything, they should improve it.

Overall, I don't have strong objections, so if Eli is fine with the new 
behavior all around, I don't mind getting them in (with maybe a few 
modifications), and hopefully we'll reach some better solutions by the 
next release.

For some more context though: previously, I've tried to make it seem 
"like xref buffer was never there" after the user performed the 
navigation, in other respects too:

- As we recall, the xref buffer was buried after the user performed the 
navigation.

- The window configuration was fully restored to the one before the xref 
buffer was shown (with some checks like making sure the user didn't 
change the configuration manually thereafter), and then the navigation 
was performed. After that, using the "originally intended" window or 
frame was much easier.

- All buffers that were opened just to show the xref locations were 
cleaned up (closed) after the navigation was performed.

...But in the end, all this stuff worked not reliably/fast/intuitively 
enough, and I came to the conclusion that it's not a good choice when we 
have an *xref* buffer that stays around for a long time. So it was removed.

Now to your patches.

> * 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch
> 
> This extends the exception granted by split-window-sensibly to
> single-window frames whose dimensions are below those of splitting
> thresholds to consider multi-window frames where all but one window is
> dedicated.

If there are other non-dedicated windows, will one of them be used (that 
would seem fine)?

> In practice, it fixes the case where
> 
>    C-x 4 . xref-backend-definitions RET n
> 
> would surprisingly pop-up a new frame if the original frame was already
> small to start with.
> 
> This fix to window.el appears very sound to me, but if it is not desired
> for whatever reason, a more localized fix in xref.el is also possible.

A fix in xref.el sounds more prudent to me, but someone else would have 
to comment on window.el.

> * 0004-Don-t-quit-xref-window-on-RET-only-on-C-u-RET.patch
> 
> This fixes the "disappearing *xref* problem", by bringing back the
> default behaviour of not quitting the *xref* window on RET, although
> allowing for that if the user types C-u RET instead.

I have to wonder if we have other commands that quit the current window 
when called with a prefix. Particularly commands bound to RET.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> OK, but is it the correct thing to do? The thresholds are there for a
> reason, and having a window that's only a few lines tall (which could
> happen in some example) will hardly be more helpful than showing it in
> a different window, even if the user expected xref to use the "other
> window".

Well, I don’t think it’s that bad if a tiny window pops up, considering:

1. The user *did* type C-x 4 . , meaning he specifically requested "a
different window", so that’s life. Tiny windows can pop up via the
existing exception in split-window-sensibly, too.

2. I assume we want to stand by Eli’s wish that the *xref* buffer should
stay visible (and anyway the user has a failsafe C-u RET command that
buries it and should improve the situation).

> This stuff is difficult, and personally I don't like either of the
> easily reachable solutions.

We’re talking about the edge cases here. Would you like a solution where
the user’s intention (1) is disregarded in extreme cases (and thus the
original window is considered even if the user did C-x 4 .)

>> I see and I will try to answer. I proposed two patches previously:
>>
>> * a first one to fix the non-determinism of window popping/selecting
>> behaviour;
>> * a second one to make the *xref* buffer less obstrusive.
>> * (now there is the third one that fixes the frame-popping glitch)
>>
>> IIUC it is the second one that clashes with "the dissapearing *xref*
>> problem" that I have yet to read up on. If we don't come up with a
>> solution for that, I would be OK with a solution that leaves it unsolved
>> but adds some customization point (hook) for the user to put this
>> behaviour in.
> 
>Indeed, but there's also a matter of consistency, and of making the
>overall design work in a predictable fashion. More in the follow-up
>email.

Any of the two alternatives are more predictable than what we have
now. A gain in predictability.

> Overall, I don't have strong objections, so if Eli is fine with the
> new behavior all around, I don't mind getting them in (with maybe a
> few modifications), and hopefully we'll reach some better solutions by
> the next release.
>
> For some more context though: previously, I've tried to make it seem
> "like xref buffer was never there" after the user performed the
> navigation, in other respects too:
>
> - As we recall, the xref buffer was buried after the user performed
> the navigation.
>
> - The window configuration was fully restored to the one before the
> xref buffer was shown (with some checks like making sure the user
> didn't change the configuration manually thereafter), and then the
> navigation was performed. After that, using the "originally intended"
> window or frame was much easier.
>
> - All buffers that were opened just to show the xref locations were
> cleaned up (closed) after the navigation was performed.

I see, there’s prior art here. You approach is much more ambitious than
mine and given the hairiness of window code, it’s no wonder it didn’t
work well, if you will excuse the hindsight 20/20 :-)

My approach is less ambitious but works well and predictably for these
(more than) common cases:

The case where I *do want* my current window (and only that one) to get
clobbered.

   M-. symbol-with-multiple-definitions RET
   n [0 or more times]
   p [0 or more times]
   RET [the final decision, or C-u RET to ditch the *xref* buffer]

And also the case where I *don’t want* my window to get clobbered, and
don’t care about the other ones.

   C-x 4 . symbol-with-multiple-definitions RET
   n [0 or more times]
   p [0 or more times]
   RET [the final decision, or C-u RET to ditch the *xref* buffer]

If it helps, this itch didn’t pop out of thin air: as you know, xref.el
is lifted from SLIME’s UI. SLY, my fork of SLIME, does the same, and a
user complained about SLY’s use of popup windows in a way that finally
convinced me. See https://github.com/joaotavora/sly/issues/121

>> * 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch

> If there are other non-dedicated windows, will one of them be used
> (that would seem fine)?

Yes, of course, in keeping with the current spirit that splitting a
small window is a last resort before popping a frame.

> I have to wonder if we have other commands that quit the current
> window when called with a prefix. Particularly commands bound to RET.

I see, it’s a bit odd indeed, but I had no idea of any kind of rule or
policy in that direction.

Anyway, In the thread you pointed me to, there was talk of an ’a’
command but I never saw it. It was some hypothetical
xref-quit-and-goto-xref. I’m perfectly fine with implementing that
instead.

Of course my priority goes towards RET doing xref-quit-and-goto-xref and
something else doing xref-goto-xref. If that default isn’t changed, I’ll
probably to that remap in my init file..




João

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

[martin rudalics]

 > * 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch
 >
 > This extends the exception granted by split-window-sensibly to
 > single-window frames whose dimensions are below those of splitting
 > thresholds to consider multi-window frames where all but one window is
 > dedicated.

Maybe the new behavior should be made customizable but this is for users
of dedicated windows to decide.  In either case, instead of constructing
‘window-list’ please consider using ‘walk-window-tree’ for that part

+            (let ((windows (delete window (window-list frame)))
+                  w)
+              (while (and (setq w (pop windows))
+                          (window-dedicated-p w)))
+              (not windows))))

Thank you, martin

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

[Dmitry Gutov]

On 10/25/17 10:43 AM, João Távora wrote:

>> OK, but is it the correct thing to do? The thresholds are there for a
>> reason, and having a window that's only a few lines tall (which could
>> happen in some example) will hardly be more helpful than showing it in
>> a different window, even if the user expected xref to use the "other
>> window".
> 
> Well, I don’t think it’s that bad if a tiny window pops up, considering:

Yeah, maybe it's not too bad. Like I said, no strong objections.

> I see, there’s prior art here. You approach is much more ambitious than
> mine and given the hairiness of window code, it’s no wonder it didn’t
> work well, if you will excuse the hindsight 20/20 :-)

It would have given a more consistent mental model, though. And it's 
something that corresponds to some users expectations already.

Think Ctrl-P or "Goto Definition" preview in Sublime Text. You can look 
at the destinations and pick one, or not pick anything, and the tabs 
list would be intact.

> If it helps, this itch didn’t pop out of thin air: as you know, xref.el
> is lifted from SLIME’s UI. SLY, my fork of SLIME, does the same, and a
> user complained about SLY’s use of popup windows in a way that finally
> convinced me. See https://github.com/joaotavora/sly/issues/121

I agree that it might be a step forward, and help you retain some main 
aspects of behavior that the users are accustomed to. I just wish it was 
a step toward a more well-rounded UI.

>> If there are other non-dedicated windows, will one of them be used
>> (that would seem fine)?
> 
> Yes, of course, in keeping with the current spirit that splitting a
> small window is a last resort before popping a frame.

Good.

>> I have to wonder if we have other commands that quit the current
>> window when called with a prefix. Particularly commands bound to RET.
> 
> I see, it’s a bit odd indeed, but I had no idea of any kind of rule or
> policy in that direction.

If we don't, maybe we should. Consistency in the way modes work is a 
good thing, and allows the user to adapt to each of them much quicker. 
Not to mention having to keep the effects of different bindings in 
memory (both muscle and cranial).

> Anyway, In the thread you pointed me to, there was talk of an ’a’
> command but I never saw it. It was some hypothetical
> xref-quit-and-goto-xref. I’m perfectly fine with implementing that
> instead.

But 'a' (correct me if I'm wrong) normally replaces a buffer in the 
*current* window. And kill the previous buffer.

> Of course my priority goes towards RET doing xref-quit-and-goto-xref and
> something else doing xref-goto-xref. If that default isn’t changed, I’ll
> probably to that remap in my init file..

So you'd always use "something else" to navigate to project-find-regexp 
search results?

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

[João Távora]

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

martin rudalics <rudalics@gmx.at> writes:

>> * 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch
>>
>> This extends the exception granted by split-window-sensibly to
>> single-window frames whose dimensions are below those of splitting
>> thresholds to consider multi-window frames where all but one window is
>> dedicated.
>
> Maybe the new behavior should be made customizable but this is for users
> of dedicated windows to decide.
>
Hmmm, adding a defcustom sounds a bit heavy-handed to me. We’re talking
about adding an option to preserve the behaviour of a failure. The
docstring would certainly be hard to phrase.

Perhaps we could just wait for the (quite improbable in my opinion)
complaints of dedicated window users that expected their split-window
operations to fail in certain extreme situations hence causing
(un)expected frames to pop up?

> In either case, instead of constructing
> ‘window-list’ please consider using ‘walk-window-tree’ for that part

Done. See attached patch.

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Allow-split-window-sensibly-to-split-threshold-in-fu.patch --]
[-- Type: text/x-diff, Size: 3288 bytes --]

From eb1a0ce0c66502bee41c090afcb04c65271196a1 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 3/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 slightly expands on 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).

* 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

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It would have given a more consistent mental model, though. And it's
> something that corresponds to some users expectations already.
>
> Think Ctrl-P or "Goto Definition" preview in Sublime Text. You can
> look at the destinations and pick one, or not pick anything, and the
> tabs list would be intact.
> ...
> I agree that it might be a step forward, and help you retain some main
> aspects of behavior that the users are accustomed to. I just wish it
> was a step toward a more well-rounded UI.

It seems you’re talking, in part, of the expectations of users coming
from more "modern" UI’s, like Sublime’s. I can understand that argument,
but I also argue that not drifting too much from the (twitchy, I’ll
admit) window popping behavior of Emacs is useful in its own right.

For example, I’d argue that Emacs users are almost universally used to
’C-h f/c/m’ stuff bringing up obtrusive windows that they can quit by
typing ’q’ and get back to their original configuration (provided, yes,
that they don’t mess with the window configuration in the
meantime). It’s the number one thing that annoyed me in my early Emacs
days, and I see a lot of people confused by it, until they learn how to
’q’. Other examples are the default completion UI and the "disabled
command" interface.

If that UI can be improved, it certainly should. (I have some very old
ideas about a single window dedicated frame for help windows that could
be discussed and developed). But whatever is done it should be done to
Emacs as a whole, to preserve consistency.

In the meantime, my xref patches try to stay close to the current
paradigm.  Additionally, they should benefit automatically from any
future improvements.

> But 'a' (correct me if I'm wrong) normally replaces a buffer in the
> *current* window. And kill the previous buffer.

I can’t correct you because I had no idea of that convention either. I
just mentioned ’a’ because I read it somewhere in the discussion.  I’ll
be fine with any key that means "yes I’ve really decided I want to go
here now take me there and bury this buffer".  As a last resort, an
unbound command that I can remap RET to, or a decent interface that
allows me to write such a command.

>> Of course my priority goes towards RET doing xref-quit-and-goto-xref and
>> something else doing xref-goto-xref. If that default isn’t changed, I’ll
>> probably to that remap in my init file..
>
> So you'd always use "something else" to navigate to
> project-find-regexp search results?

No, I’d use "n" or "SPC". These work fine as always. When I find the one
I want to edit, I press "RET". I’m a big boy, I can find the *xref*
buffer again :-)

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

[Dmitry Gutov]

On 10/25/17 4:29 PM, João Távora wrote:

> It seems you’re talking, in part, of the expectations of users coming
> from more "modern" UI’s, like Sublime’s.

Naturally, I want all kinds of users to be happy with Emacs. And the 
"modern" editors have the right idea in some of their design decisions.

> I can understand that argument,
> but I also argue that not drifting too much from the (twitchy, I’ll
> admit) window popping behavior of Emacs is useful in its own right.

I'm arguing that what in the cases we want to quit the xref buffer right 
after, we actually want some sort of richly-decorated *completion* UI 
where the user picks one destination (or we could call it a selection 
UI; something other editors often use popups for).

And once they pick it, Emacs can pop the destination in some window or 
other to its heart content.

> For example, I’d argue that Emacs users are almost universally used to
> ’C-h f/c/m’ stuff bringing up obtrusive windows that they can quit by
> typing ’q’ and get back to their original configuration (provided, yes,
> that they don’t mess with the window configuration in the
> meantime).

But the window configuration changes that happen while the user selects 
the destination in the xref buffer, can't be undone with 'quit', with 
out without your patches.

> If that UI can be improved, it certainly should. (I have some very old
> ideas about a single window dedicated frame for help windows that could
> be discussed and developed). But whatever is done it should be done to
> Emacs as a whole, to preserve consistency.

If you're talking about window management in general, that seems 
orthogonal to me.

> In the meantime, my xref patches try to stay close to the current
> paradigm.  Additionally, they should benefit automatically from any
> future improvements.

Let's wait for Eli's opinion.

>> But 'a' (correct me if I'm wrong) normally replaces a buffer in the
>> *current* window. And kill the previous buffer.
> 
> I can’t correct you because I had no idea of that convention either. I
> just mentioned ’a’ because I read it somewhere in the discussion.

The binding I have in mind is in dired-mode. There, 'a' replaces the 
current buffer with another.

I don't recall any other 'a' bindings, off the top of my head.

> I’ll
> be fine with any key that means "yes I’ve really decided I want to go
> here now take me there and bury this buffer".  As a last resort, an
> unbound command that I can remap RET to, or a decent interface that
> allows me to write such a command.

I'd also be fine with any of those. :) 2 or 3 sound best, though.

>>> Of course my priority goes towards RET doing xref-quit-and-goto-xref and
>>> something else doing xref-goto-xref. If that default isn’t changed, I’ll
>>> probably to that remap in my init file..
>>
>> So you'd always use "something else" to navigate to
>> project-find-regexp search results?
> 
> No, I’d use "n" or "SPC". These work fine as always.

SPC is not bound by default. And you'll probably use 'n' in 
xref-find-definitions output as well.

> When I find the one
> I want to edit, I press "RET". I’m a big boy, I can find the *xref*
> buffer again :-)

Would you prefer similar behavior in *Grep* buffers as well? If you 
still use those.

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

[Eli Zaretskii]

> Cc: 28814@debbugs.gnu.org, eliz@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 25 Oct 2017 03:24:58 +0300
> 
> On 10/23/17 11:03 PM, João Távora wrote:
> 
> > I read the discussion you pointed me to me by Dmitry in
> > http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01235.html.
> > 
> > Eli, If I understand your concerns there, then the first and third
> > patches I proposed shouldn't in any way interfere with your use of
> > *xref*-related facilities.  If anything, they should improve it.
> 
> Overall, I don't have strong objections, so if Eli is fine with the new 
> behavior all around, I don't mind getting them in (with maybe a few 
> modifications), and hopefully we'll reach some better solutions by the 
> next release.

Sorry, guys: I will have to take you several steps back, because I got
lost in the details of your discussion.

I understand that the motivation for changing the existing solution
for the "disappearing XREF buffer" was the unexpected or even
incorrect behavior when the "C-x 4" variety of the commands is
invoked.  That is, the existing code would display the definition in
the original window, not in "the other" window (which is taken by the
XREF buffer).  And the original solution was to display the definition
in the window where the XREF buffer was displayed, thus making it
"disappear".  Is that correct?

If so, then the easiest solution IMO would be to pop up one more
window, i.e. behave as if the window displaying the XREF buffer didn't
exist.  That would both keep the contract of "C-x 4" and leave the
XREF buffer visible.

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.

If the above solves the remaining issues, and if you have no other
problems, can we have patches to that effect?

And then the next question would be: what branch to install this on?

Thanks, and sorry for keeping silence until now.

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:

> I understand that the motivation for changing the existing solution
> for the "disappearing XREF buffer" was the unexpected or even
> incorrect behavior when the "C-x 4" variety of the commands is
> invoked.  That is, the existing code would display the definition in
> the original window, not in "the other" window (which is taken by the
> XREF buffer).  And the original solution was to display the definition
> in the window where the XREF buffer was displayed, thus making it
> "disappear".  Is that correct?

More or less. You are 90% correct. I wouldn’t link unexpected behaviour
with the "disappear XREF buffer" because they can be seen as independent
issues, though closely related. Anyway, it’s OK because the fix you
propose further below is exactly the current one I propose (minus some
subtleties for when we "can’t" pop a new window because of insuficient
window dimensions).

> If so, then the easiest solution IMO would be to pop up one more
> window, i.e. behave as if the window displaying the XREF buffer didn't
> exist.  That would both keep the contract of "C-x 4" and leave the
> XREF buffer visible.

Yes, this is the default behaviour in the current patches.

> 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. If done in this order, it has the nice benefit that
the window thus immediately released is available for showing the cross
reference (in the C-x 4 case, that is)

If no suitable key for this command can be found (’a’, ’o’ would perhaps
be nice), then let’s make an unbound command that I can bound to RET.

Then let’s open a separate discussion on whether that behaviour should
become the default (I think it should).

> If the above solves the remaining issues, and if you have no other
> problems, can we have patches to that effect?

Sure, as soon as you (or we) decide on one of the following:

* C-u RET (for completeness sake, you seem to be against this one)
* ’a’ bound to a new command xref-quit-and-goto-xref
* ’o’ bound to a new command xref-quit-and-goto-xref
* a new unbound command xref-quit-and-goto-xref

> And then the next question would be: what branch to install this on?

It’s a bugfix, so emacs-26.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10/25/17 4:29 PM, João Távora wrote:

> But the window configuration changes that happen while the user
> selects the destination in the xref buffer, can't be undone with
> 'quit', with out without your patches.

You’re right, my patches they keep the window configuration for some
definition of "keep" :-).  However, a much more correct definition of
"keep" than the one we have now.

>> If that UI can be improved, it certainly should. (I have some very old
>> ideas about a single window dedicated frame for help windows that could
>> be discussed and developed). But whatever is done it should be done to
>> Emacs as a whole, to preserve consistency.
>
> If you're talking about window management in general, that seems
> orthogonal to me.

As it should be.  But if we give xref.el a special interface it ceases
to be :-)

> Let's wait for Eli's opinion.

It seems Eli’s OK with the current behaviour minus the C-u RET.

> The binding I have in mind is in dired-mode. There, 'a' replaces the
> current buffer with another.
>
> I don't recall any other 'a' bindings, off the top of my head.

Then perhaps ’a’ can be xref-quit-and-goto-xref if you’re not opposed to
that.

> SPC is not bound by default. And you'll probably use 'n' in
> xref-find-definitions output as well.

You’re right. SPC should be bound to xref-show-xref though, if we are
honour the SLIME ancestry.

>> When I find the one
>> I want to edit, I press "RET". I’m a big boy, I can find the *xref*
>> buffer again :-)
>
> Would you prefer similar behavior in *Grep* buffers as well? If you
> still use those.

Meh... Maybe. I don’t know. I would prefer to always use XREF and
project-find-regexp (that I just learned about, thanks!), if only that
could be enhanced to the much much faster ‘git grep’ in Git projects.
Perhaps we could work on that as a separate modification.

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  28814@debbugs.gnu.org
> Date: Wed, 25 Oct 2017 16:27:13 +0100
> 
> > If so, then the easiest solution IMO would be to pop up one more
> > window, i.e. behave as if the window displaying the XREF buffer didn't
> > exist.  That would both keep the contract of "C-x 4" and leave the
> > XREF buffer visible.
> 
> Yes, this is the default behaviour in the current patches.

Good.  Then I'm happy.

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?

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

> Then let’s open a separate discussion on whether that behaviour should
> become the default (I think it should).

What behavior should become the default?

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.

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

Thanks.

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

[Dmitry Gutov]

On 10/25/17 6:35 PM, João Távora wrote:

>> The binding I have in mind is in dired-mode. There, 'a' replaces the
>> current buffer with another.
>>
>> I don't recall any other 'a' bindings, off the top of my head.
> 
> Then perhaps ’a’ can be xref-quit-and-goto-xref if you’re not opposed to
> that.

Just the objection that I've already voiced.

'o' seems better, if you go that route. But that's just because I don't 
recall exactly what other 'o' bindings do.

> I would prefer to always use XREF and
> project-find-regexp (that I just learned about, thanks!), if only that
> could be enhanced to the much much faster ‘git grep’ in Git projects.
> Perhaps we could work on that as a separate modification.

Yup. Continuing that discussion is on my list.

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

[João Távora]

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

Eli Zaretskii <eliz@gnu.org> 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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Honor-window-switching-intents-in-xref-find-definiti.patch --]
[-- Type: text/x-diff, Size: 5655 bytes --]

From f4f774416ca0e90d351b10e7da2095ec9a9ba530 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/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)))))
 
 \f
@@ -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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Allow-split-window-sensibly-to-split-threshold-in-fu.patch --]
[-- Type: text/x-diff, Size: 3326 bytes --]

From aae81ea38c6062399c60d0018533e4be359bae6e 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/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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Provide-new-xref-quit-and-goto-xref-bind-it-to-Q-and.patch --]
[-- Type: text/x-diff, Size: 3152 bytes --]

From 4cfc9f26f65038d088649789759ac7d2414ac2b8 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/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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Document-changes-to-xref-behavior-in-NEWS-and-texi-m.patch --]
[-- Type: text/x-diff, Size: 6032 bytes --]

From ae3a5adda7debf77e03c151b84c1062ae90b6970 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
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.
+
 \f
 * New Modes and Packages in Emacs 26.1
 
-- 
2.14.2


[-- Attachment #6: Type: text/plain, Size: 9 bytes --]


João

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

[martin rudalics]

 > Perhaps we could just wait for the (quite improbable in my opinion)
 > complaints of dedicated window users that expected their split-window
 > operations to fail in certain extreme situations hence causing
 > (un)expected frames to pop up?

OK.

 >> In either case, instead of constructing
 >> ‘window-list’ please consider using ‘walk-window-tree’ for that part
 >
 > Done. See attached patch.

Thanks, martin

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

[martin rudalics]

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

"all the other windows" should become "all other windows on this frame"

And please add this to the appropriate place (section 28.15 Additional
Options for Displaying Buffers) of the Elisp manual.

Thanks, martin

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

[Dmitry Gutov]

On 10/25/17 6:39 PM, Eli Zaretskii wrote:

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

Isn't it almost as odd? The main goal of the command is to perform 
navigation. Quitting the window is an important side-effect, but it's 
still a side-effect.

I think 'o' or 'j' would be much better.

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: dgutov@yandex.ru,  28814@debbugs.gnu.org
> Date: Wed, 25 Oct 2017 21:56:21 +0100
> 
> > 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.

If Dmitry doesn't like 'Q', I'm not wedded to it.  Feel free to change
the keybinding.

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

Thanks.  What can I say? the patches are really not trivial, and I
hesitate to put them on emacs-26.  During development of Emacs 25 it
took us some time and effort to stabilize these features, so I'd hate
to destabilize them now because of some unintended consequence we
don't currently envision.  OTOH, we are only at the first pretest.

Dmitry, what's your opinion?

A few comments to the documentation:

 . Please proofread the text for UK English spellings (e.g., "honour")
   and only one space between sentences.
 . I don't understand the reason for such extensive changes in the
   manual.  Most of them look purely stylistic, and I see no problems
   with the original text to justify that.  What did I miss?
 . I don't see a need for a NEWS entry.  If this is a bugfix, then it
   doesn't belong there, as we don't describe bugfixes in NEWS (there
   are too many to describe).

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

[Dmitry Gutov]

On 10/26/17 7:42 PM, Eli Zaretskii wrote:

> If Dmitry doesn't like 'Q', I'm not wedded to it.  Feel free to change
> the keybinding.

Thanks.

> Dmitry, what's your opinion?

IMO, not a bugfix, but I don't mind getting the changes in and seeing 
how many people disapprove during the pretest.

It's your choice, though.

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:

> A few comments to the documentation:
>
>  . Please proofread the text for UK English spellings (e.g., "honour")
>    and only one space between sentences.

Done.

>  . I don't understand the reason for such extensive changes in the
>    manual.  Most of them look purely stylistic, and I see no problems
>    with the original text to justify that.

Here are the (minor) problems I detected:

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

- When the text in the same node talks about RET, I took that
opportunity to mention existing C-o binding and the proposed TAB
binding. This adds information.

- Finally, I changed "To go back to places @emph{from where} you found
the definition" to "Once you are at the definition, you may want to go
back to places @{from where}". This is indeed purely stylistic, but I
thought it was a less abrupt transition from the preceding paragraph
that talks about going to definitions. The change looks larger than it
really is because of the paragraph filling, but I did only change the
first sentence.

- A real problem is that the description for RET in the node "Xref
Commands" incorrectly states that RET buries the *xref* buffer, which we
know it doesn't.

> What did I miss?

Nothing, I just thought this improved the manual slightly. Tell me which
parts, maybe all, you think I should scrap. (though I do believe updates
to the "Xref Commands" node are in order)

> I don't see a need for a NEWS entry.  If this is a
> bugfix, then it doesn't belong there, as we don't describe bugfixes in
> NEWS (there are too many to describe).

I didn't know that, sorry. Apart from the bugfix there is also the new
quit-and-jump behaviour, but maybe also not worth it. Your call.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10/26/17 7:42 PM, Eli Zaretskii wrote:
>
>> If Dmitry doesn't like 'Q', I'm not wedded to it.  Feel free to change
>> the keybinding.
>
> Thanks.

OK, scrap 'Q', keep 'TAB'. 

>> Dmitry, what's your opinion?
>
> IMO, not a bugfix, but I don't mind getting the changes in and seeing
> how many people disapprove during the pretest.

I think the C-x 4 . and C-x 5 . thing is indeed a bugfix and should go
into emacs-26.

The xref-quit-and-goto-xref is just a new feature, so stricly it should
go to master.

I'm fine either way, really.  I think if these things are in emacs-26
they will increase visibility to this relatively new feature, but since
I'm mostly scratching my own itch I don't mind if they go to emacs-27.

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

[Eli Zaretskii]

> 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
looking for, and move on.  If the reader was only interested in
reading about "M-.", with your version she will not see the
description of the XREF buffer, unless she reads on.

So I don't think the repetition here is a bad idea, especially since
it doesn't really repeat the same text.

> - Finally, I changed "To go back to places @emph{from where} you found
> the definition" to "Once you are at the definition, you may want to go
> back to places @{from where}". This is indeed purely stylistic, but I
> thought it was a less abrupt transition from the preceding paragraph
> that talks about going to definitions. The change looks larger than it
> really is because of the paragraph filling, but I did only change the
> first sentence.

When the original style is clear and unambiguous, I usually avoid
changing it, because style preferences are personal.

The additions for the new and changed bindings are, of course, okay,
that wasn't what prompted my comment.

Please show the final patch with the above comments fixed, and I think
it's okay to put this on emacs-26 after all.

Thanks.

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

[João Távora]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Honor-window-switching-intents-in-xref-find-definiti.patch --]
[-- Type: text/x-diff, Size: 5654 bytes --]

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)))))
 
 \f
@@ -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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Allow-split-window-sensibly-to-split-threshold-in-fu.patch --]
[-- Type: text/x-diff, Size: 3326 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-New-xref-quit-and-goto-xref-command-bound-to-TAB-bug.patch --]
[-- Type: text/x-diff, Size: 4900 bytes --]

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.
+
 \f
 * 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

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

[João Távora]

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

Ping. This was very close to wrapping up and so I wonder if you
had a chance to look at the patches...

I hope my rant isn't putting you off, because it's just that, an
opinion, a data point. I did fix all the parts according to your comments,
I believe.

It's only the last patch with the doc changes that changed significantly
since the last time you looked.

João


On Sat, Oct 28, 2017 at 8:19 PM, João Távora <joaotavora@gmail.com> wrote:

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


-- 
João Távora

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

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

[Dmitry Gutov]

On 10/27/17 3:05 AM, João Távora wrote:

> OK, scrap 'Q', keep 'TAB'.

TAB is an interesting choice, BTW.

Let's try it.

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 2 Nov 2017 17:03:18 +0000
> Cc: Dmitry Gutov <dgutov@yandex.ru>, 28814@debbugs.gnu.org
> 
> Ping. This was very close to wrapping up and so I wonder if you 
> had a chance to look at the patches...

Your opinion on the amount of free time I have is too optimistic...

Don't worry, I didn't forget, and will get to this soon.

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

[João Távora]

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

On Thu, Nov 2, 2017 at 5:07 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Thu, 2 Nov 2017 17:03:18 +0000
> > Cc: Dmitry Gutov <dgutov@yandex.ru>, 28814@debbugs.gnu.org
> >
> > Ping. This was very close to wrapping up and so I wonder if you
> > had a chance to look at the patches...
>
> Your opinion on the amount of free time I have is too optimistic...
>

Sorry, I indeed took it from the otherwise quick rhythm of the exchange
that it had slipped your queue, but I don't have any kind of opinion on
your
free time (if anything sometimes I wonder if you guys sleep at all).

-- 
João Távora

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

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 2 Nov 2017 19:07:00 +0000
> Cc: Dmitry Gutov <dgutov@yandex.ru>, 28814@debbugs.gnu.org
> 
> sometimes I wonder if you guys sleep at all).

If you analyze the time stamps of my postings, you will see...

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: dgutov@yandex.ru,  28814@debbugs.gnu.org
> Date: Sat, 28 Oct 2017 20:19:00 +0100
> 
> I hope I fixed everything in these patches.

Thanks, let's get this into the emacs-26 branch, after fixing the
gotchas below.

> 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

Please end this sentence with a period.

> +A new command 'xref-quit-and-goto-xref', bound to 'TAB' in *xref*
> +buffers, quits the window before jumping to the destination. In many
                                                              ^^
Two spaces between sentences, please.

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

The first line of every doc string should be a single complete
sentence.

> +(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*
                                   ^^^^
"means", not "says"

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: Sat, 28 Oct 2017 20:19:00 +0100
>> 
>> I hope I fixed everything in these patches.
>
> Thanks, let's get this into the emacs-26 branch, after fixing the
> gotchas below.

Done.  Pushed 3 commits to emacs-26.

>> +buffers, quits the window before jumping to the destination. In many
>                                                               ^^
> Two spaces between sentences, please.

Sigh. This is the hardest for me to remember.  Are there any tools to
help with this?

João

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

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: dgutov@yandex.ru,  28814@debbugs.gnu.org, 28814-done@debbugs.gnu.org
> Date: Fri, 03 Nov 2017 16:18:13 +0000
> 
> > Two spaces between sentences, please.
> 
> Sigh. This is the hardest for me to remember.  Are there any tools to
> help with this?

I'd try checkdoc.el.