Re: [RFC PATCH] Changes to pop-up source buffers
Kyle Meyer 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
> 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. >From 0db0adc4f20d8c664976b89cbe033f5579e1fdc5 Mon Sep 17 00:00:00 2001 From: Jack Kamm 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
Jack Kamm 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
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. >From a9cb8889df25697ff73e7c1e72987dac01875c8a Mon Sep 17 00:00:00 2001 From: Jack Kamm 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~ == 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
> 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
Re: [RFC PATCH] Changes to pop-up source buffers
Jack Kamm 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: > > +defaultUse 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-frameUse `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
Re: [RFC PATCH] Changes to pop-up source buffers
On 1/18/20, Jack Kamm 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
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
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 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.
