From: Kyle Meyer <firstname.lastname@example.org> To: Jack Kamm <email@example.com>, firstname.lastname@example.org Subject: Re: [RFC PATCH] Changes to pop-up source buffers Date: Sun, 19 Jan 2020 03:24:32 +0000 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> Jack Kamm <email@example.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)))
next prev parent reply other threads:[~2020-01-19 3:24 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-18 17:33 Jack Kamm 2020-01-18 18:56 ` Samuel Wales 2020-01-18 19:27 ` Jack Kamm 2020-01-19 2:46 ` Samuel Wales 2020-01-19 3:24 ` Kyle Meyer [this message] 2020-01-19 17:13 ` Jack Kamm 2020-01-21 4:10 ` Kyle Meyer 2020-01-22 5:06 ` Jack Kamm 2020-01-25 4:18 ` Kyle Meyer 2020-01-19 5:07 ` stardiviner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: https://www.orgmode.org/ * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [RFC PATCH] Changes to pop-up source buffers' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).