[RFC PATCH] Changes to pop-up source buffers

[RFC PATCH] Changes to pop-up source buffers

[Jack Kamm]

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

This patch changes some implementation details of
org-src-switch-to-buffer and org-edit-src-exit, to more consistently use
pop-to-buffer to open the source buffer, and quit-restore-window to
close it. It also adds a new default option to org-src-window-setup.

I'll explain some details and motivation for this now.

First, on killing the source buffer. Currently, we kill it with
kill-buffer, then restore the previous saved window configuration with
set-window-configuration. The downside is that, if changes to the
windows were made while editing the source buffer, for example if a new
window is opened to refer to some other file, then these changes will be
lost after the original window configuration is restored.

By contrast, this patch uses quit-restore-window to close the source
window, which won't affect other windows that may have been
changed. quit-restore-window is also smart enough to decide whether the
source window should be deleted (if the source buffer was popped up in a
new window), or whether the window should be kept and switched to some
previous buffer (if the source buffer was popped up on an existing
window).

Next, I changed org-src-window-setup to consistently use pop-to-buffer
to create the source window. This is needed for quit-restore-window to
figure out whether to delete the source window. Otherwise, if we
separately create the window and switch to it, then quit-restore-window
won't know whether the window was previously used for something else.

Finally, I added a new default option to org-src-window-setup, to use
the user's default configuration of display-buffer. Personally, I have
configured my own display-buffer-base-action and would like org-babel to
use it. Also, if a user is not satisfied with any of the options in
org-src-window-setup, this allows them to use their own configuration.

In most cases, the new default for org-src-window-setup will behave
similarly to the previous default (reorganize-frame). It will only
behave differently when 3 or more windows are currently open.

A final advantage I'd like to note for pop-to-buffer and
quit-restore-window, is that these are the mechanisms used by many
built-in Emacs functions to pop up and close windows, such as *Help*
windows.

There was one previous option for org-src-window-setup,
reorganize-frame, that I couldn't reimplement using pop-to-buffer. That
option required keeping around some logic for deleting windows and
restoring the configuration.

I tested the new implementation for org-src-switch-to-buffer, for all
options of org-src-window-setup, and it appears to work correctly. I
tested this on Emacs 26.3.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-src-use-display-buffer-quit-restore-window-for-s.patch --]
[-- Type: text/x-patch, Size: 6355 bytes --]

From fe2df20f3ddb72ca083c75eee7ece302abecf75a Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 18 Jan 2020 07:55:48 -0800
Subject: [PATCH] org-src: use display-buffer, quit-restore-window for source
 buffers

---
 etc/ORG-NEWS    | 14 ++++++++---
 lisp/org-src.el | 65 +++++++++++++++++++++++--------------------------
 2 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 67c3ca2ed..cbbd50cf0 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -30,10 +30,18 @@ group new datetime entries by month.
 Babel Java blocks recognize header argument =:cmdargs= and pass its
 value in call to =java=.
 
-*** Refinement in window behavior on exiting Org source buffer
+*** Improved window behavior for Org source buffer
 
-After editing a source block, Org will restore the window layout when
-~org-src-window-setup~ is set to a value that modifies the layout.
+More consistently use the ~display-buffer~ framework for popping up
+source buffers.
+
+Added an option ~default~ to ~org-src-window-setup~, that respects the
+user's default configuration of ~display-buffer~ to pop up
+buffers. This is the new default value for ~org-src-window-setup~.
+
+The option ~reorganize-frame~ was also reverted to the previous
+behavior of restoring the window configuration after exiting the
+source buffer.
 
 ** New functions
 *** ~org-columns-toggle-or-columns-quit~
diff --git a/lisp/org-src.el b/lisp/org-src.el
index 878821b14..949638fa0 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -144,26 +144,24 @@ the existing edit buffer."
   :package-version '(Org . "8.0")
   :type 'boolean)
 
-(defcustom org-src-window-setup 'reorganize-frame
+(defcustom org-src-window-setup 'default
   "How the source code edit buffer should be displayed.
 Possible values for this option are:
 
+default            Use default `display-buffer' action.
 current-window     Show edit buffer in the current window, keeping all other
                    windows.
 split-window-below Show edit buffer below the current window, keeping all
                    other windows.
 split-window-right Show edit buffer to the right of the current window,
                    keeping all other windows.
-other-window       Use `switch-to-buffer-other-window' to display edit buffer.
+other-window       Use some other window to display edit buffer.
 reorganize-frame   Show only two windows on the current frame, the current
-                   window and the edit buffer.
-other-frame        Use `switch-to-buffer-other-frame' to display edit buffer.
-                   Also, when exiting the edit buffer, kill that frame.
-
-Values that modify the window layout (reorganize-frame, split-window-below,
-split-window-right) will restore the layout after exiting the edit buffer."
+                   window and the edit buffer. Restore windows after exiting.
+other-frame        Use some other frame to display edit buffer."
   :group 'org-edit-structure
   :type '(choice
+	  (const default)
 	  (const current-window)
 	  (const split-window-below)
 	  (const split-window-right)
@@ -474,9 +472,7 @@ When REMOTE is non-nil, do not try to preserve point or mark when
 moving from the edit area to the source.
 
 Leave point in edit buffer."
-  (when (memq org-src-window-setup '(reorganize-frame
-				     split-window-below
-				     split-window-right))
+  (when (eql org-src-window-setup 'reorganize-frame)
     (setq org-src--saved-temp-window-config (current-window-configuration)))
   (let* ((area (org-src--contents-area datum))
 	 (beg (copy-marker (nth 0 area)))
@@ -801,34 +797,35 @@ Raise an error when current buffer is not a source editing buffer."
 
 (defun org-src-switch-to-buffer (buffer context)
   (pcase org-src-window-setup
+    (`default (pop-to-buffer buffer))
     (`current-window (pop-to-buffer-same-window buffer))
     (`other-window
-     (switch-to-buffer-other-window buffer))
+     (pop-to-buffer buffer '(nil
+			     ((reusable-frames . nil)
+			      (inhibit-same-window . t)))))
     (`split-window-below
-     (if (eq context 'exit)
-	 (delete-window)
-       (select-window (split-window-vertically)))
-     (pop-to-buffer-same-window buffer))
+     (let ((split-width-threshold)
+	   (split-height-threshold 0))
+       (pop-to-buffer buffer '((display-buffer-reuse-window
+				display-buffer-pop-up-window)
+			       ((reusable-frames . nil)
+				(inhibit-same-window . t))))))
     (`split-window-right
-     (if (eq context 'exit)
-	 (delete-window)
-       (select-window (split-window-horizontally)))
-     (pop-to-buffer-same-window buffer))
+     (let ((split-width-threshold 0)
+	   (split-height-threshold))
+       (pop-to-buffer buffer '((display-buffer-reuse-window
+				display-buffer-pop-up-window)
+			       ((reusable-frames . nil)
+				(inhibit-same-window . t))))))
     (`other-frame
-     (pcase context
-       (`exit
-	(let ((frame (selected-frame)))
-	  (switch-to-buffer-other-frame buffer)
-	  (delete-frame frame)))
-       (`save
-	(kill-buffer (current-buffer))
-	(pop-to-buffer-same-window buffer))
-       (_ (switch-to-buffer-other-frame buffer))))
+     (pop-to-buffer buffer '((display-buffer-reuse-window
+			      display-buffer-pop-up-frame)
+			     ((reusable-frames . 0)
+			      (inhibit-same-window . t)))))
     (`reorganize-frame
      (when (eq context 'edit) (delete-other-windows))
-     (org-switch-to-buffer-other-window buffer)
-     (when (eq context 'exit) (delete-other-windows)))
-    (`switch-invisibly (set-buffer buffer))
+     (org-switch-to-buffer-other-window buffer))
+    (`switch-invisibly (pop-to-buffer buffer '(display-buffer-no-window)))
     (_
      (message "Invalid value %s for `org-src-window-setup'"
 	      org-src-window-setup)
@@ -1167,8 +1164,8 @@ Throw an error if there is no such buffer."
     (let ((edit-buffer (current-buffer))
 	  (source-buffer (marker-buffer beg)))
       (unless source-buffer (error "Source buffer disappeared.  Aborting"))
-      (org-src-switch-to-buffer source-buffer 'exit)
-      (kill-buffer edit-buffer))
+      (quit-restore-window nil 'kill)
+      (pop-to-buffer source-buffer))
     ;; Insert modified code.  Ensure it ends with a newline character.
     (org-with-wide-buffer
      (when (and write-back (not (equal (buffer-substring beg end) code)))
-- 
2.25.0

Re: [RFC PATCH] Changes to pop-up source buffers

[Samuel Wales]

i can't comment on your ideas at all, but just have one concern, which
maybe could trip up others like me.  namely, pop-to-buffer, as stated
in its very docstring, prefers not the same window.

if i remember correctly, this is the window management problem i have
been struggling with since 2002.

for me, trying to get commands or functions that call pop-to-buffer to
behave as i need them to, which is to say, for them to use the full
(and same) window for accessibility reasons,* has been so unfixable in
the past that i had to give up.

i've resorted to brittle uses of defadvice, putting up with the rare
completion windows that should work not working, heavy-handed
overly-general solutions with things like same-window-buffer-names,
etc. just to get them to behave.  the nuclear option is redefinitions.

the result is a whole file full of kludges that partly work.

i know in this case i could possibly use yet another defadvice.  i
also know that recent version of emacs have a complex, new layer of
window management functions, which have evolved, and which i tried to
understand but just did not.  so they are useless to me.

packages like shackle do the opposite of what i need.  and are
workarounds. and could end up unsupported at any time.

i guess i just wanted to point out that i, personally, hate
pop-to-buffer?  assuming this is the same issue i have been strugglign
with for 18 years.

of course there are hardcoded other-windowness, also, maybe like in
occur and help buffers.  but pop-to-buffer if i recall correctly is
the cause of most of these problems.


well, for what it's worth, those are my tangential comments.  please
carry on.  probably you have good reasons and i should not interfere.
but i do not look forward to working around them.  kludge file will
grow.


* only exceptions possible are things like rare cases with temporary
completion windows, sometimes.


On 1/18/20, Jack Kamm <jackkamm@gmail.com> wrote:
> This patch changes some implementation details of
> org-src-switch-to-buffer and org-edit-src-exit, to more consistently use
> pop-to-buffer to open the source buffer, and quit-restore-window to
> close it. It also adds a new default option to org-src-window-setup.
>
> I'll explain some details and motivation for this now.
>
> First, on killing the source buffer. Currently, we kill it with
> kill-buffer, then restore the previous saved window configuration with
> set-window-configuration. The downside is that, if changes to the
> windows were made while editing the source buffer, for example if a new
> window is opened to refer to some other file, then these changes will be
> lost after the original window configuration is restored.
>
> By contrast, this patch uses quit-restore-window to close the source
> window, which won't affect other windows that may have been
> changed. quit-restore-window is also smart enough to decide whether the
> source window should be deleted (if the source buffer was popped up in a
> new window), or whether the window should be kept and switched to some
> previous buffer (if the source buffer was popped up on an existing
> window).
>
> Next, I changed org-src-window-setup to consistently use pop-to-buffer
> to create the source window. This is needed for quit-restore-window to
> figure out whether to delete the source window. Otherwise, if we
> separately create the window and switch to it, then quit-restore-window
> won't know whether the window was previously used for something else.
>
> Finally, I added a new default option to org-src-window-setup, to use
> the user's default configuration of display-buffer. Personally, I have
> configured my own display-buffer-base-action and would like org-babel to
> use it. Also, if a user is not satisfied with any of the options in
> org-src-window-setup, this allows them to use their own configuration.
>
> In most cases, the new default for org-src-window-setup will behave
> similarly to the previous default (reorganize-frame). It will only
> behave differently when 3 or more windows are currently open.
>
> A final advantage I'd like to note for pop-to-buffer and
> quit-restore-window, is that these are the mechanisms used by many
> built-in Emacs functions to pop up and close windows, such as *Help*
> windows.
>
> There was one previous option for org-src-window-setup,
> reorganize-frame, that I couldn't reimplement using pop-to-buffer. That
> option required keeping around some logic for deleting windows and
> restoring the configuration.
>
> I tested the new implementation for org-src-switch-to-buffer, for all
> options of org-src-window-setup, and it appears to work correctly. I
> tested this on Emacs 26.3.
>
>


-- 
The Kafka Pandemic

What is misopathy?
https://thekafkapandemic.blogspot.com/2013/10/why-some-diseases-are-wronged.html

The disease DOES progress. MANY people have died from it. And ANYBODY
can get it at any time.

Re: [RFC PATCH] Changes to pop-up source buffers

[Jack Kamm]

Hi Sam,

> for me, trying to get commands or functions that call pop-to-buffer to
> behave as i need them to, which is to say, for them to use the full
> (and same) window for accessibility reasons,* has been so unfixable in
> the past that i had to give up.

Thank you for raising this, I wasn't aware that pop-to-buffer might
cause accessibility issues.

I want to make sure that this patch won't cause any accessibility issues
and hope I can address your concerns.

What setting of org-src-window-setup are you using? If it is
"current-window" or "reorganize-frame", this patch shouldn't affect you
at all, as those implementations are left the same.

I tested all the other options for org-src-window-setup as well, and
their behavior remained the same when I tested them.

Most options do call pop-to-buffer or display-buffer at some point or
other, this patch mainly simplifies and unifies the way they call
pop-to-buffer. This includes the "current-window" option, which calls
"pop-to-buffer-same-window". So I don't think the patch should cause new
accessibility problems.

Re: [RFC PATCH] Changes to pop-up source buffers

[Samuel Wales]

On 1/18/20, Jack Kamm <jackkamm@gmail.com> wrote:
> What setting of org-src-window-setup are you using? If it is
> "current-window" or "reorganize-frame", this patch shouldn't affect you
> at all, as those implementations are left the same.

you're right, i have that set to current-window, as you might expect.
and it is reasonable to expect that any patch would continue to
respect it.

thanks for your response.

jack> A final advantage I'd like to note for pop-to-buffer and
quit-restore-window, is that these are the mechanisms used by many
built-in Emacs functions to pop up and close windows, such as *Help*
windows.

i'm not sure what the latest is on what one should use.  or whether
the latest is on what one should use is problematic for my modest
needs.  maybe you have looked into the former.

help does things like popping up in the fisrt place, opening a linked
help buffer, and opening up source code.  idk if they all use the same
mechanism?  i've had issues with 2-3 of them.  anomalies on rare
occasions occur despite my kludging.

iirc stefan once wrote that using the wrong function will break his
configuration.  there are subtly different purposes for displaying a
buffer.  one covers the minibuffer, or something.

Re: [RFC PATCH] Changes to pop-up source buffers

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> This patch changes some implementation details of
> org-src-switch-to-buffer and org-edit-src-exit, to more consistently use
> pop-to-buffer to open the source buffer, and quit-restore-window to
> close it. It also adds a new default option to org-src-window-setup.
> [...]

Thanks for working on this.  I am not much of a babel user and didn't
follow the recent org-edit-src-exit thread too closely, but in general I
think relying more on display-buffer-based functions is a good thing.

> Subject: [PATCH] org-src: use display-buffer, quit-restore-window for source
>  buffers

I suspect you left out the changelog entry because it's an RFC patch,
but I figured I'd note it just in case.  Also, it'd be nice to include a
shortened version of the motivation you gave above in the commit
message.

> -(defcustom org-src-window-setup 'reorganize-frame
> +(defcustom org-src-window-setup 'default
>    "How the source code edit buffer should be displayed.
>  Possible values for this option are:
>  
> +default            Use default `display-buffer' action.

Hmm, I dislike using "default" as a value for an option.  When it's used
to say "this is the default behavior for this option", it seems
shortsighted because that will be a bad name if the default value
changes.  But here it seems to instead mean "what a plain display-buffer
call would do".  I think it'd be good to choose another name to avoid
confusion.  Perhaps "plain" or "vanilla" or "pop[-to-buffer]" ...

I'm also not sure whether this patch should change the default.

> -other-frame        Use `switch-to-buffer-other-frame' to display edit buffer.
> -                   Also, when exiting the edit buffer, kill that frame.
> -
> -Values that modify the window layout (reorganize-frame, split-window-below,
> -split-window-right) will restore the layout after exiting the edit buffer."
> +                   window and the edit buffer. Restore windows after exiting.

convention nit: Please use two spaces after sentences in docstrings.

> -  (when (memq org-src-window-setup '(reorganize-frame
> -				     split-window-below
> -				     split-window-right))
> +  (when (eql org-src-window-setup 'reorganize-frame)

nit: eq would do here and would be more in line with the codebase:

  $ git grep "(eq " master | wc -l
  1739
  $ git grep "(eql " master | wc -l
  6

>  (defun org-src-switch-to-buffer (buffer context)
>    (pcase org-src-window-setup
> +    (`default (pop-to-buffer buffer))
>      (`current-window (pop-to-buffer-same-window buffer))
>      (`other-window
> -     (switch-to-buffer-other-window buffer))
> +     (pop-to-buffer buffer '(nil
> +			     ((reusable-frames . nil)
> +			      (inhibit-same-window . t)))))

The actions here and elsewhere have incorrectly specified alists.  It's
probably easier to see that this misbehaves with the below example:

    ;; extra parentheses, like above => ignores inhibit-same-window
    (display-buffer (get-buffer-create "*blah*")
                    '(display-buffer-same-window
                      ((inhibit-same-window . t))))
    ;; dropping the outer parentheses
    (display-buffer (get-buffer-create "*blah*")
                    '(display-buffer-same-window
                      (inhibit-same-window . t)))

This means the `other-window' case behaves the same as the `default'
case.

But stepping back, I don't see much point in switching to pop-to-buffer
here.  switch-to-buffer-other-window uses pop-to-buffer underneath, so
you should be able to use it directly and still get the
quit-restore-window behavior you're after.

>      (`split-window-below
> -     (if (eq context 'exit)
> -	 (delete-window)
> -       (select-window (split-window-vertically)))
> -     (pop-to-buffer-same-window buffer))
> +     (let ((split-width-threshold)
> +	   (split-height-threshold 0))
> +       (pop-to-buffer buffer '((display-buffer-reuse-window
> +				display-buffer-pop-up-window)
> +			       ((reusable-frames . nil)
> +				(inhibit-same-window . t))))))

Quickly testing, this has a slight change in behavior.  If there is
already a window below the current Org buffer window, the new source
window will be popped up below the _other_ window rather than the Org
buffer.  I think this could be fixed (and the code in general
simplified) by using display-buffer-below-selected.

>      (`split-window-right
> -     (if (eq context 'exit)
> -	 (delete-window)
> -       (select-window (split-window-horizontally)))
> -     (pop-to-buffer-same-window buffer))
> +     (let ((split-width-threshold 0)
> +	   (split-height-threshold))
> +       (pop-to-buffer buffer '((display-buffer-reuse-window
> +				display-buffer-pop-up-window)
> +			       ((reusable-frames . nil)
> +				(inhibit-same-window . t))))))

This has a similar change in behavior to what's described above (but
with "right" instead of "below").  In this case, though, I don't think
there's an existing display action that retains the original behavior,
so doing so would probably involve writing a custom display action
function.

> -    (`switch-invisibly (set-buffer buffer))
> +     (org-switch-to-buffer-other-window buffer))
> +    (`switch-invisibly (pop-to-buffer buffer '(display-buffer-no-window)))

You need a bit more to get it to not display the buffer.  Compare

  (display-buffer (get-buffer-create "*blah*")
                  '(display-buffer-no-window))

to

  (display-buffer (get-buffer-create "*blah*")
                  '(display-buffer-no-window
                    (allow-no-window . t)))

Re: [RFC PATCH] Changes to pop-up source buffers

[Jack Kamm]

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

Hi Kyle,

Thanks for taking the time to do a thorough review of the patch, I found
your response (especially the many examples you included) to be very
illuminating.

I agree that relying more on display-buffer-based functions is good, but
in retrospect I may have been over-eager, especially since
reorganize-frame can't be switched over, and split-window-right might
require writing a new action function.

My main motivation was to use my own display-buffer configuration to
show the source buffer. So I've rewritten the patch to be smaller and
more conservative, just adding a "plain" option to org-src-window-setup,
and not changing the implementation of any existing options.  I think
this is less likely to disrupt existing workflows or introduce
accidental bugs.

What do you think of using this smaller patch instead?

As an aside, in case we do decide to re-implement some of the display
options, now or in future, I did have a slight discrepancy from the
behavior you describe for split-window-right:

> Quickly testing, this has a slight change in behavior.  If there is
> already a window below the current Org buffer window, the new source
> window will be popped up below the _other_ window rather than the Org
> buffer.  I think this could be fixed (and the code in general
> simplified) by using display-buffer-below-selected.

On my own system, the window pops up below the existing Org buffer, even
if I have several existing horizontal splits. I'm not sure why.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-src-Add-option-plain-to-org-src-window-setup.patch --]
[-- Type: text/x-patch, Size: 2249 bytes --]

From a9cb8889df25697ff73e7c1e72987dac01875c8a Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 19 Jan 2020 08:28:36 -0800
Subject: [PATCH] org-src: Add option 'plain to org-src-window-setup

* lisp/org-src.el (org-src-window-setup): Add option 'plain for
org-src-window-setup, that uses vanilla display-buffer to show the
source window.
---
 etc/ORG-NEWS    | 7 +++++++
 lisp/org-src.el | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 67c3ca2ed..b32d37e65 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -35,6 +35,13 @@ value in call to =java=.
 After editing a source block, Org will restore the window layout when
 ~org-src-window-setup~ is set to a value that modifies the layout.
 
+*** New option to show source buffers using "plain" display-buffer
+
+Added option ~'plain~ to ~org-src-window-setup~ to show source buffers
+using ~display-buffer~. This allows users to control how source
+buffers are displayed by modifying ~display-buffer-alist~ or
+~display-buffer-base-action~.
+
 ** New functions
 *** ~org-columns-toggle-or-columns-quit~
 =<C-c C-c>= bound to ~org-columns-toggle-or-columns-quit~ replaces the
diff --git a/lisp/org-src.el b/lisp/org-src.el
index 878821b14..52e99cf04 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -148,6 +148,9 @@ the existing edit buffer."
   "How the source code edit buffer should be displayed.
 Possible values for this option are:
 
+plain              Show edit buffer using `display-buffer'.  Users can
+                   further control the display behavior by modifying
+                   `display-buffer-alist' and its relatives.
 current-window     Show edit buffer in the current window, keeping all other
                    windows.
 split-window-below Show edit buffer below the current window, keeping all
@@ -801,6 +804,9 @@ Raise an error when current buffer is not a source editing buffer."
 
 (defun org-src-switch-to-buffer (buffer context)
   (pcase org-src-window-setup
+    (`plain
+     (when (eq context 'exit) (quit-restore-window))
+     (pop-to-buffer buffer))
     (`current-window (pop-to-buffer-same-window buffer))
     (`other-window
      (switch-to-buffer-other-window buffer))
-- 
2.25.0

Re: [RFC PATCH] Changes to pop-up source buffers

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> My main motivation was to use my own display-buffer configuration to
> show the source buffer. So I've rewritten the patch to be smaller and
> more conservative, just adding a "plain" option to org-src-window-setup,
> and not changing the implementation of any existing options.  I think
> this is less likely to disrupt existing workflows or introduce
> accidental bugs.
>
> What do you think of using this smaller patch instead?

The more restricted patch is fine by me.

I suppose that to some degree [*] the main benefit of this patch is that
it offers an option that calls quit-restore-window.  For example,
without this patch, you can already configure things with
display-buffer-alist when org-src-window-setup is set to
`current-window'.  Say with

  (add-to-list 'display-buffer-alist
               '("^\\*Org Src" . (display-buffer-at-bottom)))

On exit, though, the window that was popped up remains.

And that makes me think that the current options that go through a
simple display-buffer-based call (current-window and other-window) would
benefit from calling quit-restore-window like your `plain` option does.
If you agree, perhaps it's worth adding another patch on top that does
that.

[*] "to some degree" because the option added by your patch has the
    advantage that it'd work with display-buffer-base-action too.  Plus,
    I think it's good to have a dedicated option that points to
    display-buffer-alist/display-buffer-base-action.

> As an aside, in case we do decide to re-implement some of the display
> options, now or in future, I did have a slight discrepancy from the
> behavior you describe for split-window-right:
>
>> Quickly testing, this has a slight change in behavior.  If there is
>> already a window below the current Org buffer window, the new source
>> window will be popped up below the _other_ window rather than the Org
>> buffer.  I think this could be fixed (and the code in general
>> simplified) by using display-buffer-below-selected.
>
> On my own system, the window pops up below the existing Org buffer, even
> if I have several existing horizontal splits. I'm not sure why.

Hmm, weird.  I tried again (Emacs 26.3, vanilla config) and still see
the behavior I reported.  Oh well.

> Subject: [PATCH] org-src: Add option 'plain to org-src-window-setup
>
> * lisp/org-src.el (org-src-window-setup): Add option 'plain for
> org-src-window-setup, that uses vanilla display-buffer to show the
> source window.

My only minor nitpick is that, in the places you write “'plain”, it be
more common to drop the leading quote, as `plain' and ~plain~ already
suggest a symbol.  (No need to reroll for that if no one else requests
changes; I'll touch it up when applying.)

I'll wait another day or so for others to comment before applying.

Thanks.

Re: [RFC PATCH] Changes to pop-up source buffers

[Jack Kamm]

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

> I suppose that to some degree [*] the main benefit of this patch is that
> it offers an option that calls quit-restore-window.

Yes, I agree with this.  Setting org-src-window-setup to other-window
was almost good enough for me -- it even respected
display-buffer-base-action -- except that it wouldn't close the popped
up window.

> And that makes me think that the current options that go through a
> simple display-buffer-based call (current-window and other-window) would
> benefit from calling quit-restore-window like your `plain` option does.
> If you agree, perhaps it's worth adding another patch on top that does
> that.

I agree other-window would benefit from quit-restore-window, it makes
sense to close the existing window if it's been popped up.

I'm less sure about current-window.  We could certainly call
quit-restore-window here, but I'm not sure there's any benefit, as it
shouldn't open new windows that need to be closed (unless you're
modifying display-buffer-alist, in which case the `plain' option should
be preferred anyways).  I'm also hesitant because Samuel relies on this
option for accessibility reasons, and if I accidentally introduced a
bug, I might not immediately notice, since I don't use this option.

I've attached a patch on top of my previous one, calling
quit-restore-window when using other-window, but leaving current-window
alone.

> Hmm, weird.  I tried again (Emacs 26.3, vanilla config) and still see
> the behavior I reported.  Oh well.

I tested again, with "emacs -q" this time, and got the behavior you
reported. So it must be something with my config.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-src-Add-call-to-quit-restore-window-in-org-src-s.patch --]
[-- Type: text/x-patch, Size: 1068 bytes --]

From 0db0adc4f20d8c664976b89cbe033f5579e1fdc5 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Tue, 21 Jan 2020 20:39:14 -0800
Subject: [PATCH] org-src: Add call to quit-restore-window in
 org-src-switch-to-buffer

* lisp/org-src.el (org-src-switch-to-buffer): Add call to
quit-restore-window in org-src-switch-to-buffer when
org-src-window-setup is other-window
---
 lisp/org-src.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/org-src.el b/lisp/org-src.el
index 52e99cf04..bb1c57c65 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -809,7 +809,9 @@ Raise an error when current buffer is not a source editing buffer."
      (pop-to-buffer buffer))
     (`current-window (pop-to-buffer-same-window buffer))
     (`other-window
-     (switch-to-buffer-other-window buffer))
+     (let ((cur-win (selected-window)))
+       (switch-to-buffer-other-window buffer)
+       (when (eq context 'exit) (quit-restore-window cur-win))))
     (`split-window-below
      (if (eq context 'exit)
 	 (delete-window)
-- 
2.25.0

Re: [RFC PATCH] Changes to pop-up source buffers

[Kyle Meyer]

Kyle Meyer <kyle@kyleam.com> writes:
>> Subject: [PATCH] org-src: Add option 'plain to org-src-window-setup
> I'll wait another day or so for others to comment before applying.

I've applied this patch (with the mentioned tweaks) and the second patch
(with a slight expansion of the commit message).

Thanks.

Re: [RFC PATCH] Changes to pop-up source buffers

[stardiviner]

> A final advantage I'd like to note for pop-to-buffer and
> quit-restore-window, is that these are the mechanisms used by many
> built-in Emacs functions to pop up and close windows, such as *Help*
> windows.

I agree to use =pop-to-buffer=. Propose too.

>
> There was one previous option for org-src-window-setup,
> reorganize-frame, that I couldn't reimplement using pop-to-buffer. That
> option required keeping around some logic for deleting windows and
> restoring the configuration.
>
> I tested the new implementation for org-src-switch-to-buffer, for all
> options of org-src-window-setup, and it appears to work correctly. I
> tested this on Emacs 26.3.


-- 
[ stardiviner ]
       I try to make every word tell the meaning what I want to express.

       Blog: https://stardiviner.github.io/
       IRC(freenode): stardiviner, Matrix: stardiviner
       GPG: F09F650D7D674819892591401B5DF1C95AE89AC3