From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyle Meyer Subject: Re: [RFC PATCH] Changes to pop-up source buffers Date: Sun, 19 Jan 2020 03:24:32 +0000 Message-ID: <878sm4nn67.fsf@kyleam.com> References: <87eevw7jqk.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:56398) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1it1Ca-0003wv-DT for emacs-orgmode@gnu.org; Sat, 18 Jan 2020 22:24:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1it1CY-00028X-4y for emacs-orgmode@gnu.org; Sat, 18 Jan 2020 22:24:39 -0500 Received: from pb-smtp1.pobox.com ([64.147.108.70]:62811) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1it1CX-00027A-Sa for emacs-orgmode@gnu.org; Sat, 18 Jan 2020 22:24:38 -0500 In-Reply-To: <87eevw7jqk.fsf@gmail.com> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane-mx.org@gnu.org Sender: "Emacs-orgmode" To: Jack Kamm , emacs-orgmode@gnu.org 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: > > +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)))