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

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

[Adam Porter]

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

Hi,

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

Thanks,
Adam

[-- Attachment #2: 0001-lisp-org.el-org-get-indirect-buffer-Allow-indirect-b.patch --]
[-- Type: text/x-patch, Size: 3127 bytes --]

From 8e70024cae3f4569d6a0c86a0e4d8079126fe9e5 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 and a FIXME are added.
---
 lisp/org.el | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index d8708f8f2..3b67040f7 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6160,25 +6160,32 @@ 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)))
+(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."
+  (let* ((base-buffer (or (buffer-base-buffer buffer) buffer))
+         (suffix-prefix (if heading
+                            (concat heading "-")
+                          ""))
+         (buffer-name (cl-loop for n from 1 to 100
+                               for suffix = (format "%s%s" suffix-prefix n)
+                               for name = (format "%s-%s"
+                                                  (buffer-name base-buffer)
+                                                  suffix)
+                               while (buffer-live-p (get-buffer name))
+                               finally return name)))
     (condition-case nil
-        (let ((indirect-buffer (make-indirect-buffer buffer bname 'clone)))
+        (let ((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)
           ;; Return the buffer.
           indirect-buffer)
-      (error (make-indirect-buffer buffer bname)))))
+      ;; 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)))))
 
 (defun org-set-frame-title (title)
   "Set the title of the current frame to the string TITLE."
-- 
2.34.0

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

[Ihor Radchenko]

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.

> +(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?

> +  (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.

> +      ;; 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.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Adam Porter]

[-- 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

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

[Ihor Radchenko]

Adam Porter <adam@alphapapa.net> writes:

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

Thanks!
Applied onto main with amendment to the commit message.
I added a link to this discussion for future reference.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=155dc778e8cd12bdfb4c7cac7e03f166ece1059c

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>