[PATCH] org-src.el: add option `org-src-native-defun-movements'

[PATCH] org-src.el: add option `org-src-native-defun-movements'

[Sébastien Miquel]

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

Hi,

The attached patch adds a new option ~org-src-native-defun-movements~
that makes ~beginning-of-defun~, ~end-of-defun~ and ~eval-defun~ work
natively when called from inside an org src block : those functions
are called from an org src edit buffer, in the appropriate language
mode. Without this patch, calling =eval-defun= on elisp code fails.

With this option set to t by default, this is a breaking change. To
get to the beginning/end of a src block you'd have to call
~org-backward-element~ or ~org-forward-element~ directly, instead of
~beginning-of-defun~. Or you could disable the new behaviour by
setting ~org-src-native-defun-movements~ to nil.

Regards,

-- 
Sébastien Miquel

[-- Attachment #2: 0001-org-src.el-add-option-org-src-native-defun-movements.patch --]
[-- Type: text/x-patch, Size: 6006 bytes --]

From 51675d8bbea54db7daf3dcc88a77ccad5174854f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Miquel?= <sebastien.miquel@posteo.eu>
Date: Fri, 27 Aug 2021 21:41:29 +0200
Subject: [PATCH] org-src.el: add option `org-src-native-defun-movements'

* lisp/org-src.el (org-src-native-defun-movements): New option. If t,
`beginning-of-defun', `end-of-defun' and `eval-defun' act natively
when called from inside a src block.
(org-beginning-of-defun):
(org-end-of-defun): New functions.  If
`org-src-native-defun-movements' is t and point is in a src block,
call `beginning-of-defun' or `end-of-defun' from the src edit buffer.

The main goal is to make `eval-defun' work from the org buffer. For
this to work, if point is at the beginning of an #+end_src line,
`org-beginning-of-defun' has to work natively.  To get to the
beginning of the src block, call `org-backward-element` instead.
---
 etc/ORG-NEWS    | 20 ++++++++++++++++++
 lisp/org-src.el |  7 +++++++
 lisp/org.el     | 54 ++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index eb5a5d40d..379870d44 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -60,6 +60,26 @@ list of various table options (between brackets in LaTeX export),
 since certain tabular environments, such as longtblr of the
 tabularray LaTeX package, provides this structure.
 
+*** ~beginning-of-defun~ and ~eval-defun~ now work natively in src blocks
+
+To get to the beginning or end of a src block, use
+~org-backward-element~ and ~org-forward-element~ instead, or disable
+this new behaviour by setting ~org-src-native-defun-movements~ to nil.
+
+** New options
+
+*** New option =org-src-native-defun-movements=
+
+This option, t by default, makes ~beginning-of-defun~, ~end-of-defun~
+and ~eval-defun~ work natively inside src blocks : the corresponding
+function is called from an org src block edit buffer in the language
+specific mode.
+
+For ~eval-defun~ to work natively from the org buffer, if point is at
+the beginning of an #+end_src line, `org-beginning-of-defun` will work
+natively as well.  To get to the beginning of the src block, call
+`org-backward-element` instead.
+
 ** New functions and changes in function arguments
 
 *** New function ~org-element-cache-map~ for quick mapping across Org elements
diff --git a/lisp/org-src.el b/lisp/org-src.el
index 46fed052d..8feced080 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -246,6 +246,13 @@ green, respectability.
   :package-version '(Org . "9.4")
   :group 'org-babel)
 
+(defcustom org-src-native-defun-movements t
+  "If non-nil, the effect of `beginning-of-defun' and
+`end-of-defun' in a code block is as if they were issued in the
+language major mode buffer."
+  :type 'boolean
+  :package-version '(Org . "9.6")
+  :group 'org-babel)
 
 \f
 ;;; Internal functions and variables
diff --git a/lisp/org.el b/lisp/org.el
index ec59ddf44..1ff0d6f87 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -4892,13 +4892,8 @@ The following commands are available:
              org-element-use-cache)
     (org-persist-read 'org-element--cache (current-buffer)))
   ;; Beginning/end of defun
-  (setq-local beginning-of-defun-function 'org-backward-element)
-  (setq-local end-of-defun-function
-	      (lambda ()
-		(if (not (org-at-heading-p))
-		    (org-forward-element)
-		  (org-forward-element)
-		  (forward-char -1))))
+  (setq-local beginning-of-defun-function 'org-beginning-of-defun)
+  (setq-local end-of-defun-function 'org-end-of-defun)
   ;; Next error for sparse trees
   (setq-local next-error-function 'org-occur-next-match)
   ;; Make commit log messages from Org documents easier.
@@ -21525,6 +21520,51 @@ Move to the previous element at the same level, when possible."
 			       (<= (org-element-property :end prev) beg))
 		     (goto-char (org-element-property :begin prev)))))))))))
 
+(defun org-beginning-of-defun ()
+  "If point is in a src block and `org-src-native-defun-movements'
+is non-nil, call `beginning-of-defun' in the src block edit
+buffer, otherwise call `org-backward-element'."
+  (interactive)
+  (if (and org-src-native-defun-movements
+           (let* ((element (save-excursion (beginning-of-line)
+                                           (org-element-at-point)))
+	          (type (org-element-type element)))
+             (and (eq type 'src-block)
+	          (> (line-beginning-position)
+		     (org-element-property :post-affiliated element))
+	          ;; If point is at the beginning of the #+end_src
+                  ;; line, we still act natively.  Otherwise
+                  ;; `eval-defun' wont work, because `end-of-defun'
+                  ;; gets you there.
+	          (<= (point)
+		      (org-with-point-at (org-element-property :end element)
+		        (skip-chars-backward " \t\n")
+		        (line-beginning-position))))))
+      (org-babel-do-in-edit-buffer (beginning-of-defun))
+    (org-backward-element)))
+
+(defun org-end-of-defun ()
+  "If point is in a src block and `org-src-native-defun-movements'
+is non-nil, call `end-of-defun' in the src block edit
+buffer, otherwise call `org-forward-element'."
+  (if (and org-src-native-defun-movements
+           (let* ((element (save-excursion (beginning-of-line)
+                                           (org-element-at-point)))
+	          (type (org-element-type element)))
+             (and (eq type 'src-block)
+	          (> (line-beginning-position)
+		     (org-element-property :post-affiliated element))
+	          (< (line-beginning-position)
+		     (org-with-point-at (org-element-property :end element)
+		       (skip-chars-backward " \t\n")
+		       (line-beginning-position))))))
+      (progn
+        (org-babel-do-in-edit-buffer (end-of-defun)))
+    (if (not (org-at-heading-p))
+	(org-forward-element)
+      (org-forward-element)
+      (forward-char -1))))
+
 (defun org-up-element ()
   "Move to upper element."
   (interactive)
-- 
2.34.1

Re: [PATCH] org-src.el: add option `org-src-native-defun-movements'

[Tim Cross]

Sébastien Miquel <sebastien.miquel@posteo.eu> writes:

> Hi,
>
> The attached patch adds a new option ~org-src-native-defun-movements~
> that makes ~beginning-of-defun~, ~end-of-defun~ and ~eval-defun~ work
> natively when called from inside an org src block : those functions
> are called from an org src edit buffer, in the appropriate language
> mode. Without this patch, calling =eval-defun= on elisp code fails.
>
> With this option set to t by default, this is a breaking change. To
> get to the beginning/end of a src block you'd have to call
> ~org-backward-element~ or ~org-forward-element~ directly, instead of
> ~beginning-of-defun~. Or you could disable the new behaviour by
> setting ~org-src-native-defun-movements~ to nil.
>

As this is a breaking change, it should not be on by default.

This also seems like an edge case and I'm not convinced yet another
option is justified. Why have eilisp in org blocks rather than an
emacs-lisp block?

Re: [PATCH] org-src.el: add option `org-src-native-defun-movements'

[Sébastien Miquel]

Hi,

Thank you for having a look.

Tim Cross writes:
> This also seems like an edge case and I'm not convinced yet another
> option is justified. Why have eilisp in org blocks rather than an
> emacs-lisp block?

By org src blocks I meant an org emacs-lisp src block. The point of
the patch is to be able to eval-defun some lisp code in an emacs-lisp
src block from the org-buffer.

> As this is a breaking change, it should not be on by default.
Currently eval-defun errors out, and fixing that will break things
sooner or later, I think.

I do not mind updating the patch to set the new option to nil by
default, although I'll wait for a second opinion on this.

Regards,

-- 
Sébastien Miquel

Re: [PATCH] org-src.el: add option `org-src-native-defun-movements'

[Tim Cross]

Sébastien Miquel <sebastien.miquel@posteo.eu> writes:

> Hi,
>
> Thank you for having a look.
>
> Tim Cross writes:
>> This also seems like an edge case and I'm not convinced yet another
>> option is justified. Why have eilisp in org blocks rather than an
>> emacs-lisp block?
>
> By org src blocks I meant an org emacs-lisp src block. The point of
> the patch is to be able to eval-defun some lisp code in an emacs-lisp
> src block from the org-buffer.
>

OK, that makes it clearer. However, I'm not convinced this is something
we need or want. You can evaluate emacs lisp blocks and you can use
edit-special buffers to evaluate individual lines in a source block.
Being able to execute arbitrary lisp forms at a top level inside an org
buffer could be considered dangerous. I don't think it should be enabled
by default. 

>> As this is a breaking change, it should not be on by default.
> Currently eval-defun errors out, and fixing that will break things
> sooner or later, I think.
>
> I do not mind updating the patch to set the new option to nil by
> default, although I'll wait for a second opinion on this.
>

That is fine. However, note that this would mean your patch can only be
applied to the next version (development version) of org and not to the
current maintenance branch because you cannot add a breaking change to
an already released version.

I think you are making it harder to get the patch applied by enabling
the option. There is a (rightly) conservative stance on breaking
changes. Adding a new option which is enabled by default and which
breaks existing functionality has almost no chance of being applied.
Adding a new option which is a breaking change that needs to be enabled
by the user is far more likely to be considered.