On 08.01.2024 13:08, Ihor Radchenko wrote:
gerard.vermeu...@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,"
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.


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

the first block ends up with

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

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.


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


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

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

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

Reply via email to