Re: [PATCH] org-babel-demarcate-block: split using element API

[gerard . vermeulen]

On 08.01.2024 13:08, Ihor Radchenko wrote:
> gerard.vermeulen@posteo.net writes:
Attached you'll find a new version of my patch addressing all your 
issues.
This mail ends with two other ideas in the context of this patch.
[...]
> I've tested your patch and found two problems:
>
> 1. #+name: lines are duplicated, while they should not

Of course. Sometimes I delete lines by a slip of the fingers. Thanks.

> 2. Your patch does not create space between the src blocks, unlike what
>    we have on main.
>    I think that you need to (1) create a single blank lines between
>    blocks (set :post-blank property to 1); (2) keep the number blank
>    lines after the last block the same as in the initial block (copy 
> the
>    :post-blank property and assign it to the last inserted src block).
>
>    For C-u argument, do not do anything special - just keep the 
> original
>    :post-blank as is. It is the closest to what we have on main.

The previous version of the patch had
+            (insert (if arg (concat stars "\n") ""))
and now it has
+            (insert (if arg (concat stars "\n") "\n"))
I prefer this over setting the :post-blank property because it is 
simpler.
(main concats something like .... (if (arg stars "") "\n" ...).

[...]
> > Agreed, this is wrong. A partial explanation is that I attached too
> > much value to the doc-string of `org-babel-get-src-block-info'
> > telling "Return nil if point is not on a source block.  Otherwise,"
> > which
> > is for me in contradiction with documentation (string and start
> > comment) in `org-babel-demarcate-block'.
>
> `org-babel-get-src-block-info' docstring were not wrong. You just 
> missed
> the Org mode's convention that blank lines after src blocks or other
> syntax elements belong to these elements.
>
> That said, we may clarify the `org-babel-get-src-block-info' docstring
> to highlight this fact and avoid future confusion.
I changed the docstring as you suggested below.
> > Now demarcating with point below a source block works again and
> > checking this is part of the ERT test.
>
> The ERT test does not check removing #+caption from the original block.
> Also, as I said above, we should remove #+name.
The ERT test now checks that #+caption and #+name are removed from
the original code.
>
[...]
> Note that indentation of src blocks has been refactored recently on
> main. It should be more reliable now. If not, please report any issues.
I will pay attention.
> > -;; Copyright (C) 2009-2024 Free Software Foundation, Inc.
> > +;; Copyright (C) 2009-2023 Free Software Foundation, Inc.
>
> This is a spurious change :)
Reverted: it shows that I started editing in 2023 and that I am no good 
at git :)
> > -Return nil if point is not on a source block.  Otherwise, return
> > -a list with the following pattern:
> > +Return nil if point is not on a source block or not within blank
> > +lines after a source block.  Otherwise, return a list with the
> > +following pattern:
>
> I'd rather say: Return nil if point is not on a source block (blank
> lines after a source block are considered a part of that source block).
>
> It would be more accurate.
Done.
> > +        (let* ((copy (org-element-copy (org-element-at-point)))
> > +               (before (org-element-begin copy))
> > +               (beyond (org-element-end copy))
> > +               (parts (sort (if (org-region-active-p)
> > +                                (list body-beg (mark) (point) 
> > body-end)
> > +                              (list body-beg (point) body-end))
> > +                            #'<)))
> > +          ;; Prevent #+caption:, #+header:, and #+begin_src lines in 
> > block.
>
> This comment is out of place. Also, we do preserve #+header and
> #+begin_src lines, don't we?
Maybe I should have written:
+          ;; Prevent #+caption:, #+header:, and #+begin_src lines in 
*body*.

It prevents that when splitting with point at the # of #+caption a block 
like

#+caption: caption
#+name: name
#+begin_src emacs-lisp
;; elisp code
...
#+end_src

the first block ends up with

#+caption: caption
#+name: name
#+begin_src emacs-lisp
,#+caption: caption
,#+name: name
,#+begin_src emacs-lisp
;; elisp code
...
#+end_src

This is not easy to capture in a 1-2 line comment.

Anyhow, I have removed the comment and I have replaced check below it 
with
+          (set-mark (point)) ;; To simplify the next (unless ...):
+          (unless (and (>= (point) body-beg) (<= (mark) body-end))
+            (user-error "Select within the source block body to split 
it"))
which also protects against having point in body and mark on or below 
#+end_src

I think it covers everything that can be checked in the "splitting" 
branch.

I think also that the "wrapping" branch can be better protected against 
similar
region selection "user errors".  I will come back on improving the 
"wrapping"
branch shortly.
> And we need to remove #+name lines.
Done.
> > +          (unless (and (>= (point) body-beg))
> > +            (user-error "move point within the source block body to 
> > split it"))
>
> Please start error message from capital letter. It is Elisp convention
> (see `user-error' docstring).
Thanks, done, see above.
> > +          (setq parts (mapcar (lambda (p) (buffer-substring (car p) 
> > (cdr p)))
> > +                              (seq-mapn #'cons parts (cdr parts))))
> > +          (delete-region before beyond)
> > +          (deactivate-mark)
>
> AFAIK, `deactivate-mark' should be unnecessary here. To indicate that
> region should be deactivated after finishing a command, we simply set
> `deactivate-mark' _variable_ to t. See the docstring.

Done

I have two other ideas in the context of this patch:

* Improving the "wrapping" branch

1. It must be easy (possible for me) to get the org-element-copy of the
   preceding code block and use it to insert a new block with the same
   headers and switches as the preceding block.
2. In that case it is easy to raise a user-error when mark is before
   body_end of the preceding block.

I think that with this improvement the user interface of the "wrapping"
branch is closer to that of the "splitting" branch.

That leaves only the "wrapping" branch open for "user errors" in case
info is nil (no preceding code block).

What do you think?

* Adding a user option for properties to set to nil in org-element-copy.

This may be overkill, but something like:

#+begin_src emacs-lisp :results nil :tangle no
(defcustom org-babel-demarcate-block-delete '(:caption :name)
  "List of things to delete from blocks below the upper block when
splitting blocks by demarcation.  Possible things are `:caption' to
delete \"#+CAPTION:\" keywords, `:header' to delete \"#+HEADER:\"
keywords, `:name' to delete \"#+NAME:\" keywords, and `switches'
to delete e.g. \"-i +n 10\" from the \"#+BEGIN_SRC\" line."
  :group 'org-babel
  :package-version '(Org . "9.7")
  :type '(set :tag "Things to delete when splitting blocks by 
demarcation"
              (const :caption)
              (const :header)
              (const :name)
              (const :switches))
  :initialize #'custom-initialize-default
  :set (lambda (sym val)
         (set-default sym val)))
#+end_src

and changing 3 lines in my version of `org-babel-demarcate-block'
allows a user to get close to the behavior of main if he does:
(setopt org-babel-demarcate-block-delete
        '(:caption :header :name :switches)

I think that it is more important to improve the "wrapping" branch
than to add the user option.

Regards -- Gerard

0001-org-babel-demarcate-block-split-using-element-API.patch
Description: Binary data