[BUG] Org 9.5: org-goto UI seems broken

[BUG] Org 9.5: org-goto UI seems broken

[Adam Porter]

Hi,

Running Org 9.5 on Emacs 28.0.60, I noticed that org-goto seems to be
broken:

1. When I press "C-c C-j", instead of displaying the indirect buffer in
one window and the org-goto menu in another, only the org-goto window
(the "*Org Help*" buffer) is displayed.

2. When I type a letter of a heading, instead of going to a heading
matching that character, I see "No variable specified" in the echo area.

3. When I press "C-g" to exit org-goto, the "*Org Help*" buffer doesn't
get buried, and I see "Quit" in the echo area.

4. When I then press "q", the "*Org Help*" buffer is buried and I'm
shown an "*org-goto*" buffer, which appears to be the indirect clone
that should have been shown.

5. Then when I press "C-g" again, I'm returned to the window
configuration that was shown before I called org-goto.

AFAIK, org-goto worked on Org 9.4.6 that I was using before Org 9.5 was
merged into the Emacs 28.0.x branch.

--
Thanks,
Adam

Re: [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 05/10/2021 13:31, Adam Porter wrote:
> 
> Running Org 9.5 on Emacs 28.0.60, I noticed that org-goto seems to be
> broken:
> 
> 1. When I press "C-c C-j", instead of displaying the indirect buffer in
> one window and the org-goto menu in another, only the org-goto window
> (the "*Org Help*" buffer) is displayed.

Confirmed

Regression is caused by

commit 399481bad10845a77f210c9320ff1efee9a312c8
Author: Ihor Radchenko <yantar92@gmail.com>
Date:   Mon May 31 20:47:45 2021 +0800

     Do not ignore user-defined display-buffer-alist in org-insert-link

and may be related to https://list.orgmode.org/871rdtupey.fsf@joshuao.com
On 06/02/2021 23:33, "Joshua O'Connor" wrote:
> I was wondering what the reason is for Org to employ the tactic of
> overriding the variable 'display-buffer-alist' in a few places, using
> the 'org-no-popups' macro?

Sidenote: I usually use completion in C-u C-c C-j and could not 
reproduce the bug till I have realized that I should not press C-u.

[PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Ihor Radchenko]

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

Max Nikulin <manikulin@gmail.com> writes:

> Regression is caused by
>
> commit 399481bad10845a77f210c9320ff1efee9a312c8
> Author: Ihor Radchenko <yantar92@gmail.com>
> Date:   Mon May 31 20:47:45 2021 +0800
>
>      Do not ignore user-defined display-buffer-alist in org-insert-link

See the attached fix.  The fix looks reasonable, though I fail to
understand why org-no-popup was even used in org-goto-location.  We kind
of want a popup there.  git blame did not reveal anything useful either.

Adam, can you test the fix in different scenarios first? I do not use
org-goto interface, so I only did a light testing.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-goto-Fix-window-broken-arrangement-after-399481b.patch --]
[-- Type: text/x-diff, Size: 3879 bytes --]

From 72d62d9f54a356273b2fa8ccf4f71b9faccf280c Mon Sep 17 00:00:00 2001
Message-Id: <72d62d9f54a356273b2fa8ccf4f71b9faccf280c.1633437697.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Tue, 5 Oct 2021 20:37:02 +0800
Subject: [PATCH] org-goto: Fix window broken arrangement after 399481bad

* lisp/org-goto.el (org-goto-location): Do not wrap code into
`org-no-popups' macro.  The macro prevents popup windows and we do
want *Org Help* window as a popup herein.

Fixes https://list.orgmode.org/e169a2f9-72b1-02bb-96c1-6e7368f646da@gmail.com/T/#t
---
 lisp/org-goto.el | 67 ++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/lisp/org-goto.el b/lisp/org-goto.el
index 0a3470f54..352bf9f2e 100644
--- a/lisp/org-goto.el
+++ b/lisp/org-goto.el
@@ -203,40 +203,39 @@ (defun org-goto-location (&optional _buf help)
   "Let the user select a location in current buffer.
 This function uses a recursive edit.  It returns the selected
 position or nil."
-  (org-no-popups
-   (let ((isearch-mode-map org-goto-local-auto-isearch-map)
-	 (isearch-hide-immediately nil)
-	 (isearch-search-fun-function
-	  (lambda () #'org-goto--local-search-headings))
-	 (help (or help org-goto-help)))
-     (save-excursion
-       (save-window-excursion
-	 (delete-other-windows)
-	 (and (get-buffer "*org-goto*") (kill-buffer "*org-goto*"))
-	 (pop-to-buffer-same-window
-	  (condition-case nil
-	      (make-indirect-buffer (current-buffer) "*org-goto*" t)
-	    (error (make-indirect-buffer (current-buffer) "*org-goto*" t))))
-	 (let (temp-buffer-show-function temp-buffer-show-hook)
-	   (with-output-to-temp-buffer "*Org Help*"
-	     (princ (format help (if org-goto-auto-isearch
-				     "  Just type for auto-isearch."
-				   "  n/p/f/b/u to navigate, q to quit.")))))
-	 (org-fit-window-to-buffer (get-buffer-window "*Org Help*"))
-	 (org-overview)
-	 (setq buffer-read-only t)
-	 (if (and (boundp 'org-goto-start-pos)
-		  (integer-or-marker-p org-goto-start-pos))
-	     (progn (goto-char org-goto-start-pos)
-		    (when (org-invisible-p)
-		      (org-show-set-visibility 'lineage)))
-	   (goto-char (point-min)))
-	 (let (org-special-ctrl-a/e) (org-beginning-of-line))
-	 (message "Select location and press RET")
-	 (use-local-map org-goto-map)
-	 (recursive-edit)))
-     (kill-buffer "*org-goto*")
-     (cons org-goto-selected-point org-goto-exit-command))))
+  (let ((isearch-mode-map org-goto-local-auto-isearch-map)
+	(isearch-hide-immediately nil)
+	(isearch-search-fun-function
+	 (lambda () #'org-goto--local-search-headings))
+	(help (or help org-goto-help)))
+    (save-excursion
+      (save-window-excursion
+	(delete-other-windows)
+	(and (get-buffer "*org-goto*") (kill-buffer "*org-goto*"))
+	(pop-to-buffer-same-window
+	 (condition-case nil
+	     (make-indirect-buffer (current-buffer) "*org-goto*" t)
+	   (error (make-indirect-buffer (current-buffer) "*org-goto*" t))))
+	(let (temp-buffer-show-function temp-buffer-show-hook)
+	  (with-output-to-temp-buffer "*Org Help*"
+	    (princ (format help (if org-goto-auto-isearch
+				    "  Just type for auto-isearch."
+				  "  n/p/f/b/u to navigate, q to quit.")))))
+	(org-fit-window-to-buffer (get-buffer-window "*Org Help*"))
+	(org-overview)
+	(setq buffer-read-only t)
+	(if (and (boundp 'org-goto-start-pos)
+		 (integer-or-marker-p org-goto-start-pos))
+	    (progn (goto-char org-goto-start-pos)
+		   (when (org-invisible-p)
+		     (org-show-set-visibility 'lineage)))
+	  (goto-char (point-min)))
+	(let (org-special-ctrl-a/e) (org-beginning-of-line))
+	(message "Select location and press RET")
+	(use-local-map org-goto-map)
+	(recursive-edit)))
+    (kill-buffer "*org-goto*")
+    (cons org-goto-selected-point org-goto-exit-command)))
 
 ;;;###autoload
 (defun org-goto (&optional alternative-interface)
-- 
2.32.0

Re: [BUG] Org 9.5: org-goto UI seems broken

[Adam Porter]

Max Nikulin <manikulin@gmail.com> writes:

>> Running Org 9.5 on Emacs 28.0.60, I noticed that org-goto seems to be
>> broken:
>>
>> 1. When I press "C-c C-j", instead of displaying the indirect buffer in
>> one window and the org-goto menu in another, only the org-goto window
>> (the "*Org Help*" buffer) is displayed.
>
> Confirmed
>
> Regression is caused by

Thanks.  :)

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Adam Porter]

Hi Ihor,

> See the attached fix.  The fix looks reasonable, though I fail to
> understand why org-no-popup was even used in org-goto-location.  We kind
> of want a popup there.  git blame did not reveal anything useful either.
>
> Adam, can you test the fix in different scenarios first? I do not use
> org-goto interface, so I only did a light testing.

A quick test seems to indicate that it works again.  Please note that I
only recently started using org-goto myself, so I can't claim to know
all the ways in which it ought to be tested.  :)

Thanks.

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 05/10/2021 19:45, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> Regression is caused by
>>
>> commit 399481bad10845a77f210c9320ff1efee9a312c8
>> Author: Ihor Radchenko <yantar92@gmail.com>
>> Date:   Mon May 31 20:47:45 2021 +0800
>>
>>       Do not ignore user-defined display-buffer-alist in org-insert-link
> 
> See the attached fix.  The fix looks reasonable, though I fail to
> understand why org-no-popup was even used in org-goto-location.  We kind
> of want a popup there.  git blame did not reveal anything useful either.

Thank you, Ihor. I am a user of alternative `org-goto' interface. I have 
tried default one having a couple of windows in the frame (indirect 
buffer for subtree, indirect for src block). It seems, previous window 
configuration is restored correctly when `org-goto' is finished.

Curiously `org-no-popup' was introduced namely for `org-goto`

> commit b508943d329fed2af457c50afd5f63938b23c58c
> Author: Achim Gratz <Stromeko@Stromeko.DE>
> Date:   Thu Dec 20 10:18:02 2012 +0100
> 
>     org-compat: new macro org-no-popups
>     
>     * lisp/org-compat.el (org-no-popups): New wrapper macro which
>       let-binds the correct variables to suppress popup windows depending
>       on the Emacs version in use.  This is a compile-time decision when
>       byte-compiling.
>     
>     * lisp/org.el (org-get-location, org-switch-to-buffer-other-window):
>       Use the wrapper `org-no-popups´ to let-bind the correct variables
>       for suppression of popup window
To emphasize actual changes due to the patch just suggested by Ihor:

git diff -b HEAD~
diff --git a/lisp/org-goto.el b/lisp/org-goto.el
index 0a3470f54..352bf9f2e 100644
--- a/lisp/org-goto.el
+++ b/lisp/org-goto.el
@@ -203,7 +203,6 @@ When nil, you can use these keybindings to navigate 
the buffer:
    "Let the user select a location in current buffer.
  This function uses a recursive edit.  It returns the selected
  position or nil."
-  (org-no-popups
    (let ((isearch-mode-map org-goto-local-auto-isearch-map)
         (isearch-hide-immediately nil)
         (isearch-search-fun-function
@@ -236,7 +235,7 @@ position or nil."
         (use-local-map org-goto-map)
         (recursive-edit)))
      (kill-buffer "*org-goto*")
-     (cons org-goto-selected-point org-goto-exit-command))))
+    (cons org-goto-selected-point org-goto-exit-command)))

  ;;;###autoload
  (defun org-goto (&optional alternative-interface)

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Ihor Radchenko]

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

Max Nikulin <manikulin@gmail.com> writes:

> Thank you, Ihor. I am a user of alternative `org-goto' interface. I have 
> tried default one having a couple of windows in the frame (indirect 
> buffer for subtree, indirect for src block). It seems, previous window 
> configuration is restored correctly when `org-goto' is finished.
>
> Curiously `org-no-popup' was introduced namely for `org-goto`

Thanks for the testing and digging the old commits.

I tried come up with the reason why org-no-popup was used in the initial
implementation. I think, the reason is avoiding situation like what you
may see after running
(let ((pop-up-frames t)) (funcall-interactively #'org-goto))
So, removing the macro completely is not a good idea.

I have updated the patch that should work without dropping the macro.
See the attached.

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-goto-Fix-window-broken-arrangement-after-399481b.patch --]
[-- Type: text/x-diff, Size: 1983 bytes --]

From 438c476066e897b1fc3d758fd48712c1846cd845 Mon Sep 17 00:00:00 2001
Message-Id: <438c476066e897b1fc3d758fd48712c1846cd845.1633451289.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Tue, 5 Oct 2021 20:37:02 +0800
Subject: [PATCH] org-goto: Fix window broken arrangement after 399481bad

* lisp/org-goto.el (org-goto-location): Make sure that `org-on-popups'
macro does not suppress the *Org Help* window pop-up.

Fixes https://list.orgmode.org/e169a2f9-72b1-02bb-96c1-6e7368f646da@gmail.com/T/#t
---
 lisp/org-goto.el | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lisp/org-goto.el b/lisp/org-goto.el
index 0a3470f54..cd832d72c 100644
--- a/lisp/org-goto.el
+++ b/lisp/org-goto.el
@@ -217,15 +217,16 @@ (defun org-goto-location (&optional _buf help)
 	  (condition-case nil
 	      (make-indirect-buffer (current-buffer) "*org-goto*" t)
 	    (error (make-indirect-buffer (current-buffer) "*org-goto*" t))))
-	 (let (temp-buffer-show-function temp-buffer-show-hook)
-	   (with-output-to-temp-buffer "*Org Help*"
-	     (princ (format help (if org-goto-auto-isearch
-				     "  Just type for auto-isearch."
-				   "  n/p/f/b/u to navigate, q to quit.")))))
-	 (org-fit-window-to-buffer (get-buffer-window "*Org Help*"))
-	 (org-overview)
-	 (setq buffer-read-only t)
-	 (if (and (boundp 'org-goto-start-pos)
+         (let ((pop-up-windows t))
+	   (let (temp-buffer-show-function temp-buffer-show-hook)
+	     (with-output-to-temp-buffer "*Org Help*"
+	       (princ (format help (if org-goto-auto-isearch
+				       "  Just type for auto-isearch."
+				     "  n/p/f/b/u to navigate, q to quit.")))))
+	   (org-fit-window-to-buffer (get-buffer-window "*Org Help*")))
+         (org-overview)
+         (setq buffer-read-only t)
+         (if (and (boundp 'org-goto-start-pos)
 		  (integer-or-marker-p org-goto-start-pos))
 	     (progn (goto-char org-goto-start-pos)
 		    (when (org-invisible-p)
-- 
2.32.0

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 05/10/2021 23:32, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
> I tried come up with the reason why org-no-popup was used in the initial
> implementation. I think, the reason is avoiding situation like what you
> may see after running
> (let ((pop-up-frames t)) (funcall-interactively #'org-goto))
> So, removing the macro completely is not a good idea.
> 
> I have updated the patch that should work without dropping the macro.
> See the attached.

Thank you, Ihor.

Your updated patch works in default configuration (-Q). I am not 
familiar with knobs of `display-buffer' function so I have no idea if 
someone may complain that such variant overrides his setup or works 
incorrectly with his preferences.

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Marco Wahl]

Max Nikulin <manikulin@gmail.com> writes:

> On 05/10/2021 23:32, Ihor Radchenko wrote:
>> Max Nikulin writes:
>> I tried come up with the reason why org-no-popup was used in the
>> initial
>> implementation. I think, the reason is avoiding situation like what you
>> may see after running
>> (let ((pop-up-frames t)) (funcall-interactively #'org-goto))
>> So, removing the macro completely is not a good idea.
>> I have updated the patch that should work without dropping the
>> macro.
>> See the attached.
>
> Thank you, Ihor.
>
> Your updated patch works in default configuration (-Q). I am not
> familiar with knobs of `display-buffer' function so I have no idea if
> someone may complain that such variant overrides his setup or works
> incorrectly with his preferences.

Please note the documentation of variable `pop-up-windows'.

    This variable is provided mainly for backward compatibility and
    should not be used in new code.

The same holds for `pop-up-frames'.

The drop of the macro looks like a good idea to me.  Can someone please
describe the price for dropping macro `org-no-popups'?

@Ihor I do not understand what "situation" you mean.

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 08/10/2021 17:22, Marco Wahl wrote:
> Max Nikulin writes:
>> On 05/10/2021 23:32, Ihor Radchenko wrote:
>>> Max Nikulin writes:
>>> I tried come up with the reason why org-no-popup was used in the
>>> initial
>>> implementation. I think, the reason is avoiding situation like what you
>>> may see after running
>>> (let ((pop-up-frames t)) (funcall-interactively #'org-goto))
>>> So, removing the macro completely is not a good idea.
>>> I have updated the patch that should work without dropping the
>>> macro.
>>> See the attached.
> 
> Please note the documentation of variable `pop-up-windows'.
> 
>      This variable is provided mainly for backward compatibility and
>      should not be used in new code.
> 
> The same holds for `pop-up-frames'.
> 
> The drop of the macro looks like a good idea to me.  Can someone please
> describe the price for dropping macro `org-no-popups'?
> 
> @Ihor I do not understand what "situation" you mean.

Marco, have you tried
   (setq pop-up-frames t)
with first version of patch? It shows help in a new separate frame. 
Unsure if it is expected behavior even with such customization.

However current version of macro does not protect against

   (setq display-buffer-base-action
         '((display-buffer-reuse-window display-buffer-pop-up-frame)
           (reusable-frames . 0)))

The example is taken from (info "(elisp) Choosing Window Options"). I 
have no idea if such customization can be considered as shooting a foot.

P.S.
http://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=399481bad10845a77f210c9320ff1efee9a312c8
that caused the current problem changed namely `pop-up-windows'

Joshua mentioned `display-buffer-overriding-action'.

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Marco Wahl]

Hi Max and all!

> On 08/10/2021 17:22, Marco Wahl wrote:
>> Max Nikulin writes:
>>> On 05/10/2021 23:32, Ihor Radchenko wrote:
>>>> Max Nikulin writes:
>>>> I tried come up with the reason why org-no-popup was used in the
>>>> initial
>>>> implementation. I think, the reason is avoiding situation like what you
>>>> may see after running
>>>> (let ((pop-up-frames t)) (funcall-interactively #'org-goto))
>>>> So, removing the macro completely is not a good idea.
>>>> I have updated the patch that should work without dropping the
>>>> macro.
>>>> See the attached.
>>
>> Please note the documentation of variable `pop-up-windows'.
>>
>>      This variable is provided mainly for backward compatibility and
>>      should not be used in new code.
>>
>> The same holds for `pop-up-frames'.
>>
>> The drop of the macro looks like a good idea to me.  Can someone please
>> describe the price for dropping macro `org-no-popups'?
>>
>> @Ihor I do not understand what "situation" you mean.
>
> Marco, have you tried
>   (setq pop-up-frames t)
> with first version of patch? It shows help in a new separate frame.
> Unsure if it is expected behavior even with such customization.
>
> However current version of macro does not protect against
>
>   (setq display-buffer-base-action
>         '((display-buffer-reuse-window display-buffer-pop-up-frame)
>           (reusable-frames . 0)))
>
> The example is taken from (info "(elisp) Choosing Window Options"). I
> have no idea if such customization can be considered as shooting a foot.

TBH I don't fully understand that display-buffer stuff.  I experimented
a little with regards to org-goto.

AFAICT org-goto does a good job without macro org-no-popups.  

IIUC the use of macro org-no-popups in org-goto shall "protect" the
org-goto user interface in some sense.  You already mentioned that.

My feeling is that the "protection" is good intention but brings more
harm than good.  I think it's not a good idea to enforce a certain
window setting.  I guess the knowing user has an easier path to fine
tune the org-goto user interface when there is less "protection".

> http://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=399481bad10845a77f210c9320ff1efee9a312c8
> that caused the current problem changed namely `pop-up-windows'

Thanks for the link!

AFAICT the change in org-macs.el should not be in that commit since it
has nothing to do with org links -- which are the actual concern of that
commit.  And indeed the change in org-macs.el of that commit broke the
org-goto interface some.

Summary: The org-goto interface today is somewhat broken.  I vote for
taking the occasion and kicking out the macro org-no-popups entirely.
This way the org-goto interface is functional AFAICS.  If problems occur
on that path we'll take care and action.

Do you agree?


Regards,
-- 
Marco

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Ihor Radchenko]

Marco Wahl <marcowahlsoft@gmail.com> writes:

> My feeling is that the "protection" is good intention but brings more
> harm than good.  I think it's not a good idea to enforce a certain
> window setting.  I guess the knowing user has an easier path to fine
> tune the org-goto user interface when there is less "protection".

I fully agree. That was the motivation behind removing
dislpay-buffer-alist in 399481bad1. It is indeed not a good idea to
overwrite user customisations. They can be deliberate. For example, see
https://orgmode.org/list/87h7ij12t8.fsf@localhost

Max Nikulin <manikulin@gmail.com> writes:
> However current version of macro does not protect against
> 
>    (setq display-buffer-base-action
> 	 '((display-buffer-reuse-window display-buffer-pop-up-frame)
> 	   (reusable-frames . 0)))
> 
> The example is taken from (info "(elisp) Choosing Window Options"). I 
> have no idea if such customization can be considered as shooting a foot.

display-buffer-base-action, if customised by user, can later be
fine-tuned using display-buffer-alist. If necessary, the user can easily
add org-goto popup as an exception. At least, it is my understanding
from reading docs.

However, pop-up-frames and pop-up-windows are different beasts. They
cannot be fine-tuned by the user to not affect org-goto.  AFAIK, the
only way for the user to overcome the problem would be advicing
org-goto.

> Summary: The org-goto interface today is somewhat broken.  I vote for
> taking the occasion and kicking out the macro org-no-popups entirely.
> This way the org-goto interface is functional AFAICS.  If problems occur
> on that path we'll take care and action.
>
> Do you agree?

My second version of the patch also fixes org-goto interface :)
On the other hand, kicking org-no-popups macro completely may be an
option. pop-up-windows and pop-up-frames are obsolete and should not be
used anymore.

Also, a compromise could be changing org-no-popups to just
(let (pop-up-frames) ...)

WDYT?

Best,
Ihor

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Marco Wahl]

Thanks Ihor!

> Marco Wahl <marcowahlsoft@gmail.com> writes:
>
>> My feeling is that the "protection" is good intention but brings more
>> harm than good.  I think it's not a good idea to enforce a certain
>> window setting.  I guess the knowing user has an easier path to fine
>> tune the org-goto user interface when there is less "protection".
>
> I fully agree. That was the motivation behind removing
> dislpay-buffer-alist in 399481bad1. It is indeed not a good idea to
> overwrite user customisations. They can be deliberate. For example, see
> https://orgmode.org/list/87h7ij12t8.fsf@localhost
>
> Max Nikulin <manikulin@gmail.com> writes:
>> However current version of macro does not protect against
>> 
>>    (setq display-buffer-base-action
>> 	 '((display-buffer-reuse-window display-buffer-pop-up-frame)
>> 	   (reusable-frames . 0)))
>> 
>> The example is taken from (info "(elisp) Choosing Window Options"). I 
>> have no idea if such customization can be considered as shooting a foot.
>
> display-buffer-base-action, if customised by user, can later be
> fine-tuned using display-buffer-alist. If necessary, the user can easily
> add org-goto popup as an exception. At least, it is my understanding
> from reading docs.
>
> However, pop-up-frames and pop-up-windows are different beasts. They
> cannot be fine-tuned by the user to not affect org-goto.  AFAIK, the
> only way for the user to overcome the problem would be advicing
> org-goto.
>
>> Summary: The org-goto interface today is somewhat broken.  I vote for
>> taking the occasion and kicking out the macro org-no-popups entirely.
>> This way the org-goto interface is functional AFAICS.  If problems occur
>> on that path we'll take care and action.
>>
>> Do you agree?
>
> My second version of the patch also fixes org-goto interface :)

Indeed, thanks.

> On the other hand, kicking org-no-popups macro completely may be an
> option. pop-up-windows and pop-up-frames are obsolete and should not be
> used anymore.
>
> Also, a compromise could be changing org-no-popups to just
> (let (pop-up-frames) ...)
>
> WDYT?

Clearly I'm for kicking out org-no-popups completely.  Many details have
been mentioned already.  The big argument for that change for me is that
the code gets simpler.

But that's just me.  All the other fixes may have advantages too.  In
the first place I want back a usable org-goto interface.

I'd like to leave it to you to commit a fix.


Best regards,
-- 
Marco

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 13/10/2021 16:44, Marco Wahl wrote:
> 
> Clearly I'm for kicking out org-no-popups completely.  Many details have
> been mentioned already.  The big argument for that change for me is that
> the code gets simpler.

I have no strong opinion. Second patch locally restoring 
`pop-up-windows' is more suitable for the bugfix branch since it is 
closer to older behavior. Dropping the macro completely may be a step to 
future, so first version of patch is more suitable for main (do not 
forget to drop other variant during merge however).

I am not confident with complicated `display-buffer' machinery. It has 
action argument that looks promising to specify local hint to override 
default actions but to let users customize `display-buffer-alist'. I 
have no idea of particular hint for `org-goto' though. Unfortunately 
`with-output-to-temp-buffer' used there does not allow to specify action 
argument for `display-buffer'. Unsure if `temp-buffer-setup-hook' can be 
used to prepare a window for `display-buffer'.

Does someone have settings that pins help buffer to particular 
window/frame of location in a frame (e.g. bottom of "sidebar")?

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Eric S Fraga]

On Wednesday, 13 Oct 2021 at 19:23, Max Nikulin wrote:
> Does someone have settings that pins help buffer to particular
> window/frame of location in a frame (e.g. bottom of "sidebar")?

This is what I use, which is slightly more complex because I have a wide
landscape monitor and a tall portrait one and want different behaviour
in each:

#+begin_src emacs-lisp
  (defun esf/display-buffer-in-side-window (buffer alist)
    (let ((fw (/ 80.0 (frame-width))))
      (display-buffer-in-side-window buffer
                                     (if (> (frame-width) 120)
                                         (list (cons 'window-width fw)
                                               '(side . left)
                                               '(slot . 0)) 
                                       '((window-height . 0.25)
                                         (side . bottom)
                                         (slot . 0)))))) 
  (setq display-buffer-alist
        '(("^\\*Async Shell Command*" . (display-buffer-no-window))
          ("^magit-[a-z]+: " . (esf/display-buffer-in-side-window))
          ("\\*\\(Backtrace\\|Compile-Log\\|DICT .*\\|grep\\|[Hh]elp.*\\|Messages\\|Occur\\|tex-shell\\|vc-\\(diff\\|change-log\\)\\|Warnings\\|WoMan .*\\)\\*"
           (esf/display-buffer-in-side-window))))
#+end_src 

This doesn't pin to a specific frame but does make the pop-ups appear in
the same place always in each respectively frame.  By the way, I use
exwm so I have one frame per monitor, full screen, generally.

HTH,
eric

-- 
: Eric S Fraga via Emacs 28.0.60, Org release_9.5-93-gd87250
: Latest paper written in org: https://arxiv.org/abs/2106.05096

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Marco Wahl]

Thanks Eric and Max!

> On Wednesday, 13 Oct 2021 at 19:23, Max Nikulin wrote:
>> Does someone have settings that pins help buffer to particular
>> window/frame of location in a frame (e.g. bottom of "sidebar")?
>
> This is what I use, which is slightly more complex because I have a wide
> landscape monitor and a tall portrait one and want different behaviour
> in each:
>
> #+begin_src emacs-lisp
>   (defun esf/display-buffer-in-side-window (buffer alist)
>     (let ((fw (/ 80.0 (frame-width))))
>       (display-buffer-in-side-window buffer
>                                      (if (> (frame-width) 120)
>                                          (list (cons 'window-width fw)
>                                                '(side . left)
>                                                '(slot . 0))
>                                        '((window-height . 0.25)
>                                          (side . bottom)
>                                          (slot . 0))))))
>   (setq display-buffer-alist
>         '(("^\\*Async Shell Command*" . (display-buffer-no-window))
>           ("^magit-[a-z]+: " . (esf/display-buffer-in-side-window))
>           ("\\*\\(Backtrace\\|Compile-Log\\|DICT .*\\|grep\\|[Hh]elp.*\\|Messages\\|Occur\\|tex-shell\\|vc-\\(diff\\|change-log\\)\\|Warnings\\|WoMan .*\\)\\*"
>            (esf/display-buffer-in-side-window))))
> #+end_src
>
> This doesn't pin to a specific frame but does make the pop-ups appear in
> the same place always in each respectively frame.  By the way, I use
> exwm so I have one frame per monitor, full screen, generally.

Thanks for the example and the implied teaching!

I experimented with the use of display-buffer-alist and the org-goto UI.
E.g. with the config:

    (defun experiment/202110141141 (buffer alist)
      (display-buffer-in-side-window
       buffer
       (list '(window-width . 23)
             '(side . right)
             '(slot . 0))))

    (setq display-buffer-alist
          '(("\\*Org Help\\*" . (experiment/202110141141))))

AFAICS this has an effect for org-goto.  The user can control the
appearance of the org-goto UI.

BTW I think the name *Org Help* for the UI buffer could be better.

Since org-goto in main is still broken I'll commit the fix for org-goto
which kicks out the use of the macro org-no-popups (but not the macro
itself since it's used elsewhere AFAICS.)

Max, Ihor!  If you see the necessity of refinement please keep going!  


Best regards,
-- 
Marco

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Ihor Radchenko]

Marco Wahl <marcowahlsoft@gmail.com> writes:

> Since org-goto in main is still broken I'll commit the fix for org-goto
> which kicks out the use of the macro org-no-popups (but not the macro
> itself since it's used elsewhere AFAICS.)
>
> Max, Ihor!  If you see the necessity of refinement please keep going!  

I am inclined to think that org-no-popups may still be useful to
suppress pop-up-frames.  However, no so much for pop-up-windows.  Note
that I introduced pop-up-windows let-binding in org-no-popups in place
of overriding display-buffer-alist.  However, I did not fully understand
pop-up-windows does and the current let-binding actually changes
pop-up-windows value to nil (t is the Emacs default). I think we can
simply remove pop-up-windows binding from org-no-popups. Feel free to
commit this change. I am currently working on org-element-cache and do
not spend much time on other patches.

> BTW I think the name *Org Help* for the UI buffer could be better.

Do you have better ideas? Maybe something like *Org-goto Help*? If we
want to change it, we may want to do it now, before users actually use
*Org Help* name i.e. in display-buffer-alist. display-buffer-alist did
not affect *Org Help* before 399481bad, so we have low odds to break
anybody's config yet.

Best,
Ihor

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 14/10/2021 17:16, Ihor Radchenko wrote:
> Marco Wahl writes:
> 
>> Since org-goto in main is still broken I'll commit the fix for org-goto
>> which kicks out the use of the macro org-no-popups (but not the macro
>> itself since it's used elsewhere AFAICS.)
>>
>> Max, Ihor!  If you see the necessity of refinement please keep going!
> 
> I am inclined to think that org-no-popups may still be useful to
> suppress pop-up-frames.  However, no so much for pop-up-windows.  Note
> that I introduced pop-up-windows let-binding in org-no-popups in place
> of overriding display-buffer-alist.  However, I did not fully understand
> pop-up-windows does and the current let-binding actually changes
> pop-up-windows value to nil (t is the Emacs default). I think we can
> simply remove pop-up-windows binding from org-no-popups. Feel free to
> commit this change. I am currently working on org-element-cache and do
> not spend much time on other patches.

Today I am against dropping of `org-no-popups'. Since Ihor confirmed 
that he had no particular reason to add `pop-up-windows', the minimal 
change (that should be applied to the bugfix branch)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 52fc09a06..5f2c29c42 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -209,7 +209,7 @@ because otherwise all these markers will point to 
nowhere."

  (defmacro org-no-popups (&rest body)
    "Suppress popup windows and evaluate BODY."
-  `(let (pop-up-frames pop-up-windows)
+  `(let (pop-up-frames)
       ,@body))

  ^L

I think, something should be done with `org-no-popups'. Assume a user 
who has (I have no idea concerning the goal though)

   (setq pop-up-frames t)
   (setq display-buffer-base-action
         '((display-buffer-reuse-window display-buffer-pop-up-frame)
           (reusable-frames . 0)))

e.g. https://list.orgmode.org/20200503033854.GA28741@singpolyma-beefy

With "emacs -Q" and above settings completion e.g. for "C-h f" does not 
cause creation of a new frame. Org help windows appear in new frames 
though. That is why `org-no-popups' should have more code.

Minimal patch works in the cases like 
https://list.orgmode.org/87h7ij12t8.fsf@localhost and the one raised in 
this thread. I hope it will not break 
https://list.orgmode.org/orgmode/871rdtupey.fsf@joshuao.com/T/#u as well.

In the meanwhile I found the following thread on window creation in Org 
and `display-buffer' machinery (accessibility concerns were mentioned 
besides other things):
https://list.orgmode.org/87eevw7jqk.fsf@gmail.com/T/#u
Jack Kamm. [RFC PATCH] Changes to pop-up source buffers. 2020-01-18 17:33

Unfortunately info "(elisp) Displaying Buffers" 
https://www.gnu.org/software/emacs/manual/html_node/elisp/Displaying-Buffers.html 
is a reference rather than a guide. I have not realized when 
`pop-up-frames' or its modern equivalent can be convenient. I have not 
tried Eric's setup yet.

>> BTW I think the name *Org Help* for the UI buffer could be better.
> 
> Do you have better ideas? Maybe something like *Org-goto Help*? If we
> want to change it, we may want to do it now, before users actually use
> *Org Help* name i.e. in display-buffer-alist. display-buffer-alist did
> not affect *Org Help* before 399481bad, so we have low odds to break
> anybody's config yet.

Are there use cases when "*Org Help*" is not specific enough and more 
than one help window should be shown simultaneously or they should be 
shown in different sides?

P.S. Example of complex window setup unrelated to Org:
info "(elisp) Frame Layouts with Side Windows"
https://www.gnu.org/software/emacs/manual/html_node/elisp/Frame-Layouts-with-Side-Windows.html

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

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

On 14/10/2021 22:44, Max Nikulin wrote:
> 
> I think, something should be done with `org-no-popups'. Assume a user 
> who has (I have no idea concerning the goal though)
> 
>    (setq pop-up-frames t)
>    (setq display-buffer-base-action
>          '((display-buffer-reuse-window display-buffer-pop-up-frame)
>            (reusable-frames . 0)))
> 
> With "emacs -Q" and above settings completion e.g. for "C-h f" does not 
> cause creation of a new frame. Org help windows appear in new frames 
> though. That is why `org-no-popups' should have more code.

I was wrong, (setq pop-up-frames t) leads to creation of new frame for 
*Completion* buffer at least in Emacs-26.3.

It seems, each case of `org-no-popups' may require specific code. I have 
tried to take some code related to completion. It overrides 
display-buffer-base-action, but something more is required for 
pop-up-frames.
That code uses `with-current-buffer-window' while org-goto uses 
`with-output-to-temp-buffer'. I am unsure what variant is more suitable 
for org-goto.

I am attaching my draft with minimal changes. I do not like to rely on 
internal functions but I have not found high level replacement to 
achieve the same result. Maybe emacs code has a better variant somewhere.

[-- Attachment #2: org-goto-hack-1.patch --]
[-- Type: text/x-patch, Size: 2400 bytes --]

diff --git a/lisp/org-goto.el b/lisp/org-goto.el
index 0a3470f54..26fc2b735 100644
--- a/lisp/org-goto.el
+++ b/lisp/org-goto.el
@@ -203,7 +203,6 @@ When nil, you can use these keybindings to navigate the buffer:
   "Let the user select a location in current buffer.
 This function uses a recursive edit.  It returns the selected
 position or nil."
-  (org-no-popups
    (let ((isearch-mode-map org-goto-local-auto-isearch-map)
 	 (isearch-hide-immediately nil)
 	 (isearch-search-fun-function
@@ -217,7 +216,35 @@ position or nil."
 	  (condition-case nil
 	      (make-indirect-buffer (current-buffer) "*org-goto*" t)
 	    (error (make-indirect-buffer (current-buffer) "*org-goto*" t))))
-	 (let (temp-buffer-show-function temp-buffer-show-hook)
+	 (let (temp-buffer-show-hook
+	       (temp-buffer-show-function
+		(lambda (buffer)
+		  "Prevent new frame in the case of
+
+  (setq display-buffer-base-action
+        '((display-buffer-reuse-window display-buffer-pop-up-frame)
+          (reusable-frames . 0)))
+
+It is not immune to
+
+  (setq pop-up-frames t)
+
+just as \"*Completion*\" buffer.
+The idea is borrowed from `minibuffer-completion-help'."
+		  (display-buffer
+                   buffer
+                   `((display-buffer--maybe-same-window
+                      display-buffer-reuse-window
+                      ,(if (functionp 'display-buffer--maybe-pop-up-frame)
+                           ;; Unavailable in emacs-26
+                           'display-buffer--maybe-pop-up-frame
+                         'display-buffer--maybe-pop-up-frame-or-window)
+                      display-buffer-below-selected)
+                     ,(if temp-buffer-resize-mode
+                          '(window-height . resize-temp-buffer-window)
+                        '(window-height . fit-window-to-buffer))
+                     ,(when temp-buffer-resize-mode
+                        '(preserve-size . (nil . t))))))))
 	   (with-output-to-temp-buffer "*Org Help*"
 	     (princ (format help (if org-goto-auto-isearch
 				     "  Just type for auto-isearch."
@@ -236,7 +263,7 @@ position or nil."
 	 (use-local-map org-goto-map)
 	 (recursive-edit)))
      (kill-buffer "*org-goto*")
-     (cons org-goto-selected-point org-goto-exit-command))))
+     (cons org-goto-selected-point org-goto-exit-command)))
 
 ;;;###autoload
 (defun org-goto (&optional alternative-interface)

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> It seems, each case of `org-no-popups' may require specific code. I have 
> tried to take some code related to completion. It overrides 
> display-buffer-base-action, but something more is required for 
> pop-up-frames.

I think you went too far. display-buffer-base-action is mostly for user
to specify. We should not override it. If user needs adjustments for
*Org Help*, it is always possible using a custom function inside
display-buffer-base-action. I do not think that we need to do this job
for the user.

From my perspective, there is no need to prevent users from shooting
their own leg (we are in Emacs after all). The cases we may need to
interfere are: (1) when some user customisations are mostly ok, except
they affect Org's usability, and the user has no easy way to fix it; (2)
when some customisation is very common and we significantly help many
users by supporting such customisation in Org itself.

I think pop-up-windows customisation qualifies for "(1)". Not
display-buffer-base-action.

Best,
Ihor

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 16/10/2021 13:52, Ihor Radchenko wrote:
> Max Nikulin writes:
> 
>> It seems, each case of `org-no-popups' may require specific code. I have
>> tried to take some code related to completion. It overrides
>> display-buffer-base-action, but something more is required for
>> pop-up-frames.
> 
> I think you went too far. display-buffer-base-action is mostly for user
> to specify. We should not override it. If user needs adjustments for
> *Org Help*, it is always possible using a custom function inside
> display-buffer-base-action. I do not think that we need to do this job
> for the user.
> 
>  From my perspective, there is no need to prevent users from shooting
> their own leg (we are in Emacs after all). The cases we may need to
> interfere are: (1) when some user customisations are mostly ok, except
> they affect Org's usability, and the user has no easy way to fix it; (2)
> when some customisation is very common and we significantly help many
> users by supporting such customisation in Org itself.
> 
> I think pop-up-windows customisation qualifies for "(1)". Not
> display-buffer-base-action.

I may be wrong but from 
https://www.gnu.org/software/emacs/manual/html_node/elisp/Displaying-Buffers.html 
"Displaying a Buffer in a Suitable Window" I have an impression that the 
purpose of `display-buffer-base-action' is e.g. to say "please, show 
every buffer in a new frame". However there are certainly small buffers 
(e.g. "*Completion*") that do not deserve own frames, so it is 
responsibility of `display-buffer' caller to pass an argument that 
overrides *default* action. User may still customize 
`display-buffer-alist' to override `display-buffer' argument. Notice 
that I left `display-buffer-overriding-action' (highest priority 
variable) untouched.

There are another type of buffers, e.g. "*Backtrace*". New frame is 
created for them but on quit from debugger they disappear from the 
screen (actually they become iconified) till next error.

I believe, Org should provide similar hints ("show this small window 
below this buffer" for org-goto help, "hide this window on quit" for 
edit src) with priority in between of `display-buffer-base-action' and 
`display-buffer-alist'. Since `org-no-popups' is used in both cases and 
due to API related to `display-buffer', it is impossible to just fix 
`org-no-popups'. Some new macros or functions should be introduced.

So

    (setq display-buffer-base-action
	 '((display-buffer-reuse-window display-buffer-pop-up-frame)
	   (reusable-frames . 0)))

should not be considered as shooting a foot.

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Eric S Fraga]

On Sunday, 17 Oct 2021 at 23:35, Max Nikulin wrote:
> So
>
>    (setq display-buffer-base-action
>         '((display-buffer-reuse-window display-buffer-pop-up-frame)
>           (reusable-frames . 0)))
>
> should not be considered as shooting a foot.

I am not sure I understand what you are or are not proposing but org
should not be setting this variable.  This variable is for the "user",
not the package, to set.

I actually think that org imposes too much of its own view on how
buffers should be managed & displayed.  I am constantly annoyed (and
posted about this recently to the list with no response) that capture
buffers pop up all over the place and it doesn't seem like I have any
control at all.

-- 
: Eric S Fraga via Emacs 28.0.60, Org release_9.5-93-gd87250
: Latest paper written in org: https://arxiv.org/abs/2106.05096

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Max Nikulin]

On 18/10/2021 16:25, Eric S Fraga wrote:
> On Sunday, 17 Oct 2021 at 23:35, Max Nikulin wrote:
>> So
>>
>>     (setq display-buffer-base-action
>>          '((display-buffer-reuse-window display-buffer-pop-up-frame)
>>            (reusable-frames . 0)))
>>
>> should not be considered as shooting a foot.
> 
> I am not sure I understand what you are or are not proposing but org
> should not be setting this variable.  This variable is for the "user",
> not the package, to set.

I was trying to say that even with such *user* setup, behavior of Org 
should be reasonable. In particular, with draft patch I posted, *small* 
org-goto help buffer should not cause appearance of a new frame despite 
of `display-buffer-base-action' setting cited above, that is suitable 
for *regular* buffers. User still has opportunity to request arbitrary 
action for "*Org Help*" buffer through `display-buffer-alist'.

> I actually think that org imposes too much of its own view on how
> buffers should be managed & displayed.  I am constantly annoyed (and
> posted about this recently to the list with no response) that capture
> buffers pop up all over the place and it doesn't seem like I have any
> control at all.

I agree that behavior of `org-add-note' is extremely intrusive. On the 
other hand I have no idea concerning its preferred behavior by default.

I do not think that something like `org-src-window-setup' should be 
introduced for notes and captures. Ideally it should have reasonable 
behavior by default and should be adjustable through `display-buffer-alist'.

Currently I can not suggest a snippet for `display-buffer-alist' to 
exclude zoom or teams windows from candidates for 
`switch-to-buffer-other-window' (that is called from `org-capture').

Re: [PATCH] [BUG] Org 9.5: org-goto UI seems broken

[Eric S Fraga]

On Monday, 18 Oct 2021 at 23:53, Max Nikulin wrote:
> I was trying to say that even with such *user* setup, behavior of Org
> should be reasonable. 

Ah, okay.  I agree.  I also do not know what should be the default
behaviour but I do know that I don't like the current default behaviour!
I would, however, be happy with anything that I could control using
display-buffer-alist.

-- 
: Eric S Fraga via Emacs 28.0.60, Org release_9.5-93-gd87250
: Latest paper written in org: https://arxiv.org/abs/2106.05096