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

[Adam Porter <adam@alphapapa.net>]

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

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 #2: 0001-lisp-org.el-org-get-indirect-buffer-Allow-indirect-b.patch --]
[-- Type: text/x-patch, Size: 2778 bytes --]

From 22e7091b5d6dd8a84446b303a2706ab3d422b52f Mon Sep 17 00:00:00 2001
From: Adam Porter <adam@alphapapa.net>
Date: Fri, 4 Nov 2022 14:52:58 -0500
Subject: [PATCH] * lisp/org.el: (org-get-indirect-buffer) Allow indirect base
 buffers

Previously, calling this function on an indirect buffer would fail,
preventing the user from making a new indirect buffer based on an
indirect buffer (e.g. imagine the user makes an indirect buffer for a
large subtree, then wants to make another one for a subtree of that).
Now, the base buffer of the buffer is used, when applicable.

Also, the function is partially rewritten to be clearer, and a
docstring is added.
---
 lisp/org.el | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index d8708f8f2..6c39f351f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6160,25 +6160,22 @@ frame is not changed."
     (run-hook-with-args 'org-cycle-hook 'all)
     (and (window-live-p cwin) (select-window cwin))))
 
-(defun org-get-indirect-buffer (&optional buffer heading)
-  (setq buffer (or buffer (current-buffer)))
-  (let ((n 1) (base (buffer-name buffer)) bname)
-    (while (buffer-live-p
-	    (get-buffer
-	     (setq bname
-		   (concat base "-"
-			   (if heading (concat heading "-" (number-to-string n))
-			     (number-to-string n))))))
-      (setq n (1+ n)))
-    (condition-case nil
-        (let ((indirect-buffer (make-indirect-buffer buffer bname 'clone)))
-          ;; Decouple folding state.  We need to do it manually since
-          ;; `make-indirect-buffer' does not run
-          ;; `clone-indirect-buffer-hook'.
-          (org-fold-core-decouple-indirect-buffer-folds)
-          ;; Return the buffer.
-          indirect-buffer)
-      (error (make-indirect-buffer buffer bname)))))
+(cl-defun org-get-indirect-buffer (&optional (buffer (current-buffer)) heading)
+  "Return an indirect buffer based on BUFFER.
+If HEADING, append it to the name of the new buffer."
+  (let* ((base-buffer (or (buffer-base-buffer buffer) buffer))
+         (buffer-name (generate-new-buffer-name
+                       (format "%s%s"
+                               (buffer-name base-buffer)
+                               (if heading
+                                   (concat "::" heading)
+                                 ""))))
+         (indirect-buffer (make-indirect-buffer base-buffer buffer-name 'clone)))
+    ;; Decouple folding state.  We need to do it manually since
+    ;; `make-indirect-buffer' does not run
+    ;; `clone-indirect-buffer-hook'.
+    (org-fold-core-decouple-indirect-buffer-folds)
+    indirect-buffer))
 
 (defun org-set-frame-title (title)
   "Set the title of the current frame to the string TITLE."
-- 
2.34.0