emacs-orgmode
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] * lisp/org.el: (org-get-indirect-buffer) Allow indirect base


From: Adam Porter
Subject: Re: [PATCH] * lisp/org.el: (org-get-indirect-buffer) Allow indirect base buffers
Date: Sun, 6 Nov 2022 15:40:15 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1

Hi Ihor,

On 11/5/22 03:09, Ihor Radchenko wrote:
Adam Porter <adam@alphapapa.net> writes:

The attached patch improves the function org-get-indirect-buffer, fixing
a bug, clarifying the code, and adding a docstring.

Thanks! I have some comments.

Thanks for your review.  I've attached a new patch.

+(cl-defun org-get-indirect-buffer (&optional (buffer (current-buffer)) heading)
+  "Return an indirect buffer based on BUFFER.
+If HEADING, prepend it to the name of the new buffer."

Maybe append to the name?

Yes, thanks. In the new patch I took the liberty of improving the format to make it more distinctive (i.e. it won't resemble a normal filename).

+  (let* ((base-buffer (or (buffer-base-buffer buffer) buffer))
+         (suffix-prefix (if heading
+                            (concat heading "-")
+                          ""))

Why not pre-define the whole prefix instead?
(prefix (format "%s-%s" (buffer-name base-buffer)
                         (if heading (concat heading "-") "")))

then, can just say (format "%s%s" prefix n) in the loop.

+         (buffer-name (cl-loop for n from 1 to 100

why to 100? It may fail (even though unlikely) and also unnecessary.
Can just say for n from 1.

I chose to limit the index because IME it's better to signal an error than to potentially go into an infinite loop. Although it shouldn't happen, it could in the case of unforeseen circumstances, one of which I experienced while writing the patch.

But, even better, the new patch uses the built-in function generate-new-buffer-name, which handles it for us, and more efficiently.

+      ;; FIXME: Explain why this `condition-case' is necessary.  Why
+      ;; could an error be signaled with the CLONE argument non-nil,
+      ;; and why would trying again without CLONE solve the problem?
+      (error (make-indirect-buffer base-buffer buffer-name)))))

I did not find why in the git logs. It looks like some ancient code. You
can remove it in a followup patch.

Very well, the new patch omits it.

Thanks,
Adam

Attachment: 0001-lisp-org.el-org-get-indirect-buffer-Allow-indirect-b.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]