* bug#28814: 26.0.90; When *xref* window is needed, original window-switching intent is lost @ 2017-10-13 16:07 João Távora [not found] ` <handler.28814.B.150791088020837.ack@debbugs.gnu.org> 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-13 16:07 UTC (permalink / raw) To: 28814, dgutov [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
[parent not found: <handler.28814.B.150791088020837.ack@debbugs.gnu.org>]
* bug#28814: Acknowledgement (26.0.90; When *xref* window is needed, original window-switching intent is lost ) [not found] ` <handler.28814.B.150791088020837.ack@debbugs.gnu.org> @ 2017-10-16 17:58 ` João Távora 2017-10-20 9:13 ` bug#28814: [BUMP, PATCH] " João Távora 1 sibling, 0 replies; 38+ messages in thread From: João Távora @ 2017-10-16 17:58 UTC (permalink / raw) To: 28814 [-- 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 --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) [not found] ` <handler.28814.B.150791088020837.ack@debbugs.gnu.org> 2017-10-16 17:58 ` bug#28814: Acknowledgement (26.0.90; When *xref* window is needed, original window-switching intent is lost ) João Távora @ 2017-10-20 9:13 ` João Távora 2017-10-20 10:39 ` Dmitry Gutov 2017-10-23 2:06 ` Dmitry Gutov 1 sibling, 2 replies; 38+ messages in thread From: João Távora @ 2017-10-20 9:13 UTC (permalink / raw) To: 28814; +Cc: dgutov [-- 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. ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-20 9:13 ` bug#28814: [BUMP, PATCH] " João Távora @ 2017-10-20 10:39 ` Dmitry Gutov 2017-10-23 2:06 ` Dmitry Gutov 1 sibling, 0 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-10-20 10:39 UTC (permalink / raw) To: João Távora, 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-20 9:13 ` bug#28814: [BUMP, PATCH] " João Távora 2017-10-20 10:39 ` Dmitry Gutov @ 2017-10-23 2:06 ` Dmitry Gutov 2017-10-23 8:23 ` João Távora 1 sibling, 1 reply; 38+ messages in thread From: Dmitry Gutov @ 2017-10-23 2:06 UTC (permalink / raw) To: João Távora, 28814 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? ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-23 2:06 ` Dmitry Gutov @ 2017-10-23 8:23 ` João Távora 2017-10-23 20:03 ` João Távora 2017-10-25 0:07 ` Dmitry Gutov 0 siblings, 2 replies; 38+ messages in thread From: João Távora @ 2017-10-23 8:23 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-23 8:23 ` João Távora @ 2017-10-23 20:03 ` João Távora 2017-10-25 0:24 ` Dmitry Gutov 2017-10-25 7:46 ` martin rudalics 2017-10-25 0:07 ` Dmitry Gutov 1 sibling, 2 replies; 38+ messages in thread From: João Távora @ 2017-10-23 20:03 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-23 20:03 ` João Távora @ 2017-10-25 0:24 ` Dmitry Gutov 2017-10-25 7:43 ` João Távora 2017-10-25 15:11 ` Eli Zaretskii 2017-10-25 7:46 ` martin rudalics 1 sibling, 2 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-10-25 0:24 UTC (permalink / raw) To: João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 0:24 ` Dmitry Gutov @ 2017-10-25 7:43 ` João Távora 2017-10-25 10:24 ` Dmitry Gutov 2017-10-25 15:11 ` Eli Zaretskii 1 sibling, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-25 7:43 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 7:43 ` João Távora @ 2017-10-25 10:24 ` Dmitry Gutov 2017-10-25 13:29 ` João Távora 0 siblings, 1 reply; 38+ messages in thread From: Dmitry Gutov @ 2017-10-25 10:24 UTC (permalink / raw) To: João Távora; +Cc: 28814 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? ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 10:24 ` Dmitry Gutov @ 2017-10-25 13:29 ` João Távora 2017-10-25 14:49 ` Dmitry Gutov 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-25 13:29 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 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 :-) ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 13:29 ` João Távora @ 2017-10-25 14:49 ` Dmitry Gutov 2017-10-25 15:35 ` João Távora 0 siblings, 1 reply; 38+ messages in thread From: Dmitry Gutov @ 2017-10-25 14:49 UTC (permalink / raw) To: João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 14:49 ` Dmitry Gutov @ 2017-10-25 15:35 ` João Távora 2017-10-25 15:46 ` Dmitry Gutov 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-25 15:35 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 15:35 ` João Távora @ 2017-10-25 15:46 ` Dmitry Gutov 0 siblings, 0 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-10-25 15:46 UTC (permalink / raw) To: João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 0:24 ` Dmitry Gutov 2017-10-25 7:43 ` João Távora @ 2017-10-25 15:11 ` Eli Zaretskii 2017-10-25 15:27 ` João Távora 1 sibling, 1 reply; 38+ messages in thread From: Eli Zaretskii @ 2017-10-25 15:11 UTC (permalink / raw) To: Dmitry Gutov; +Cc: joaotavora, 28814 > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 15:11 ` Eli Zaretskii @ 2017-10-25 15:27 ` João Távora 2017-10-25 15:39 ` Eli Zaretskii 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-25 15:27 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, Dmitry Gutov 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 15:27 ` João Távora @ 2017-10-25 15:39 ` Eli Zaretskii 2017-10-25 20:56 ` João Távora 2017-10-26 12:39 ` Dmitry Gutov 0 siblings, 2 replies; 38+ messages in thread From: Eli Zaretskii @ 2017-10-25 15:39 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 15:39 ` Eli Zaretskii @ 2017-10-25 20:56 ` João Távora 2017-10-26 7:56 ` martin rudalics 2017-10-26 16:42 ` Eli Zaretskii 2017-10-26 12:39 ` Dmitry Gutov 1 sibling, 2 replies; 38+ messages in thread From: João Távora @ 2017-10-25 20:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, dgutov [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 20:56 ` João Távora @ 2017-10-26 7:56 ` martin rudalics 2017-10-26 16:42 ` Eli Zaretskii 1 sibling, 0 replies; 38+ messages in thread From: martin rudalics @ 2017-10-26 7:56 UTC (permalink / raw) To: João Távora, Eli Zaretskii; +Cc: 28814, dgutov > 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 20:56 ` João Távora 2017-10-26 7:56 ` martin rudalics @ 2017-10-26 16:42 ` Eli Zaretskii 2017-10-26 22:40 ` Dmitry Gutov 2017-10-26 23:59 ` João Távora 1 sibling, 2 replies; 38+ messages in thread From: Eli Zaretskii @ 2017-10-26 16:42 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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). ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-26 16:42 ` Eli Zaretskii @ 2017-10-26 22:40 ` Dmitry Gutov 2017-10-27 0:05 ` João Távora 2017-10-26 23:59 ` João Távora 1 sibling, 1 reply; 38+ messages in thread From: Dmitry Gutov @ 2017-10-26 22:40 UTC (permalink / raw) To: Eli Zaretskii, João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-26 22:40 ` Dmitry Gutov @ 2017-10-27 0:05 ` João Távora 2017-11-02 17:06 ` Dmitry Gutov 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-27 0:05 UTC (permalink / raw) To: Dmitry Gutov; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-27 0:05 ` João Távora @ 2017-11-02 17:06 ` Dmitry Gutov 0 siblings, 0 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-11-02 17:06 UTC (permalink / raw) To: João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-26 16:42 ` Eli Zaretskii 2017-10-26 22:40 ` Dmitry Gutov @ 2017-10-26 23:59 ` João Távora 2017-10-28 9:41 ` Eli Zaretskii 1 sibling, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-26 23:59 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, dgutov 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-26 23:59 ` João Távora @ 2017-10-28 9:41 ` Eli Zaretskii 2017-10-28 19:19 ` João Távora 0 siblings, 1 reply; 38+ messages in thread From: Eli Zaretskii @ 2017-10-28 9:41 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-28 9:41 ` Eli Zaretskii @ 2017-10-28 19:19 ` João Távora 2017-11-02 17:03 ` João Távora 2017-11-03 13:47 ` Eli Zaretskii 0 siblings, 2 replies; 38+ messages in thread From: João Távora @ 2017-10-28 19:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, dgutov [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-28 19:19 ` João Távora @ 2017-11-02 17:03 ` João Távora 2017-11-02 17:07 ` Eli Zaretskii 2017-11-03 13:47 ` Eli Zaretskii 1 sibling, 1 reply; 38+ messages in thread From: João Távora @ 2017-11-02 17:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, Dmitry Gutov [-- 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 --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-11-02 17:03 ` João Távora @ 2017-11-02 17:07 ` Eli Zaretskii 2017-11-02 19:07 ` João Távora 0 siblings, 1 reply; 38+ messages in thread From: Eli Zaretskii @ 2017-11-02 17:07 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-11-02 17:07 ` Eli Zaretskii @ 2017-11-02 19:07 ` João Távora 2017-11-02 19:32 ` Eli Zaretskii 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-11-02 19:07 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, Dmitry Gutov [-- 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 --] ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-11-02 19:07 ` João Távora @ 2017-11-02 19:32 ` Eli Zaretskii 0 siblings, 0 replies; 38+ messages in thread From: Eli Zaretskii @ 2017-11-02 19:32 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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... ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-28 19:19 ` João Távora 2017-11-02 17:03 ` João Távora @ 2017-11-03 13:47 ` Eli Zaretskii 2017-11-03 16:18 ` João Távora 1 sibling, 1 reply; 38+ messages in thread From: Eli Zaretskii @ 2017-11-03 13:47 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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" ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-11-03 13:47 ` Eli Zaretskii @ 2017-11-03 16:18 ` João Távora 2017-11-03 19:06 ` Eli Zaretskii 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-11-03 16:18 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 28814, 28814-done, dgutov 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-11-03 16:18 ` João Távora @ 2017-11-03 19:06 ` Eli Zaretskii 0 siblings, 0 replies; 38+ messages in thread From: Eli Zaretskii @ 2017-11-03 19:06 UTC (permalink / raw) To: João Távora; +Cc: 28814, dgutov > 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 15:39 ` Eli Zaretskii 2017-10-25 20:56 ` João Távora @ 2017-10-26 12:39 ` Dmitry Gutov 1 sibling, 0 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-10-26 12:39 UTC (permalink / raw) To: Eli Zaretskii, João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-23 20:03 ` João Távora 2017-10-25 0:24 ` Dmitry Gutov @ 2017-10-25 7:46 ` martin rudalics 2017-10-25 13:19 ` João Távora 1 sibling, 1 reply; 38+ messages in thread From: martin rudalics @ 2017-10-25 7:46 UTC (permalink / raw) To: João Távora, Dmitry Gutov; +Cc: 28814 > * 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 7:46 ` martin rudalics @ 2017-10-25 13:19 ` João Távora 2017-10-26 7:56 ` martin rudalics 0 siblings, 1 reply; 38+ messages in thread From: João Távora @ 2017-10-25 13:19 UTC (permalink / raw) To: martin rudalics; +Cc: 28814, Dmitry Gutov [-- 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-25 13:19 ` João Távora @ 2017-10-26 7:56 ` martin rudalics 0 siblings, 0 replies; 38+ messages in thread From: martin rudalics @ 2017-10-26 7:56 UTC (permalink / raw) To: João Távora; +Cc: 28814, Dmitry Gutov > 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* bug#28814: [BUMP, PATCH] (26.0.90; When *xref* window is needed, original window-switching intent is lost ) 2017-10-23 8:23 ` João Távora 2017-10-23 20:03 ` João Távora @ 2017-10-25 0:07 ` Dmitry Gutov 1 sibling, 0 replies; 38+ messages in thread From: Dmitry Gutov @ 2017-10-25 0:07 UTC (permalink / raw) To: João Távora; +Cc: 28814 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. ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2017-11-03 19:06 UTC | newest] Thread overview: 38+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-13 16:07 bug#28814: 26.0.90; When *xref* window is needed, original window-switching intent is lost João Távora [not found] ` <handler.28814.B.150791088020837.ack@debbugs.gnu.org> 2017-10-16 17:58 ` bug#28814: Acknowledgement (26.0.90; When *xref* window is needed, original window-switching intent is lost ) João Távora 2017-10-20 9:13 ` bug#28814: [BUMP, PATCH] " João Távora 2017-10-20 10:39 ` Dmitry Gutov 2017-10-23 2:06 ` Dmitry Gutov 2017-10-23 8:23 ` João Távora 2017-10-23 20:03 ` João Távora 2017-10-25 0:24 ` Dmitry Gutov 2017-10-25 7:43 ` João Távora 2017-10-25 10:24 ` Dmitry Gutov 2017-10-25 13:29 ` João Távora 2017-10-25 14:49 ` Dmitry Gutov 2017-10-25 15:35 ` João Távora 2017-10-25 15:46 ` Dmitry Gutov 2017-10-25 15:11 ` Eli Zaretskii 2017-10-25 15:27 ` João Távora 2017-10-25 15:39 ` Eli Zaretskii 2017-10-25 20:56 ` João Távora 2017-10-26 7:56 ` martin rudalics 2017-10-26 16:42 ` Eli Zaretskii 2017-10-26 22:40 ` Dmitry Gutov 2017-10-27 0:05 ` João Távora 2017-11-02 17:06 ` Dmitry Gutov 2017-10-26 23:59 ` João Távora 2017-10-28 9:41 ` Eli Zaretskii 2017-10-28 19:19 ` João Távora 2017-11-02 17:03 ` João Távora 2017-11-02 17:07 ` Eli Zaretskii 2017-11-02 19:07 ` João Távora 2017-11-02 19:32 ` Eli Zaretskii 2017-11-03 13:47 ` Eli Zaretskii 2017-11-03 16:18 ` João Távora 2017-11-03 19:06 ` Eli Zaretskii 2017-10-26 12:39 ` Dmitry Gutov 2017-10-25 7:46 ` martin rudalics 2017-10-25 13:19 ` João Távora 2017-10-26 7:56 ` martin rudalics 2017-10-25 0:07 ` Dmitry Gutov
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.