Re: Reconciling org-mode idiosyncrasies with Emacs core

Re: Reconciling org-mode idiosyncrasies with Emacs core

[Nicolas Goaziou]

Hello,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
>
>>                                               For example, having RET
>> inserting a plain newline would be a disaster in many locations.
>
> I don't think I've seen anybody advocating for that!  If you are
> referring to this bit:
>
>>   (Frustratingly, org-mode uses what I think of as the "old" convention
>>   to use RET as "plain newline" and C-j as "smart newline with indent".)

No I wasn't. I was referring to Juri Linkov's idea of providing a minor
mode to disable every single Org specific binding.

> Do you think a patch that
>
> - tweaked org-return (bound to RET) to default its INDENT argument to
>   the current value of electric-indent-mode,
>
> - tweaked org-return-indent (bound to C-j) to call
>   electric-newline-and-maybe-indent (the new default binding for C-j
>   everywhere else in Emacs, which takes care of consulting
>   electric-indent-mode) instead of newline-and-indent,
>
> would be well-received?  I would love to cook up such a patch, but I
> would be loath to break the workflows of long-time Org users who have
> come to rely on C-j indenting and RET inserting plain newlines …

It will break some workflows (mine, at least), undoubtedly, but it is
a step forward anyway. Org is a part of Emacs, there's no reason for the
former to follow a different path than the latter. Besides, undoing the
change is easy enough, since you only need to disable Electric Indent
mode.

The change will not appear overnight in Org, i.e., not in Org stable's
branch (Org 9.3.X), and it will be announced in ORG-NEWS. I do think it
is a very welcome change. Thank you.

I cleaned a bit Cc header and added Org mode list for further comments.

Regards,

-- 
Nicolas Goaziou

Re: Reconciling org-mode idiosyncrasies with Emacs core

[Kévin Le Gouguec]

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

Hi Nicolas,

I took a stab at making RET obey electric-indent-mode in org-mode.  I've
got something working; I'd like to ask for a review before moving on to
Changelog and ORG-NEWS entries (and tackling C-j… and maybe writing a
few unit tests?).

Here's the patch, with some additional comments below:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-return.patch --]
[-- Type: text/x-patch, Size: 2515 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index e82463046..681328d96 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -17644,20 +17644,32 @@ call `open-line' on the very first character."
       (org-table-insert-row)
     (open-line n)))
 
-(defun org-return (&optional indent)
+(defun org--newline (indent arg interactive)
+  "Call `newline-and-indent' or just `newline'.
+
+If INDENT is non-nil, call `newline-and-indent' with ARG to
+indent unconditionally; otherwise, call `newline' with ARG and
+INTERACTIVE, which can trigger indentation if
+`electric-indent-mode' is enabled."
+  (if indent
+      (newline-and-indent arg)
+    (newline arg interactive)))
+
+(defun org-return (&optional indent arg interactive)
   "Goto next table row or insert a newline.
 
 Calls `org-table-next-row' or `newline', depending on context.
 
 When optional INDENT argument is non-nil, call
-`newline-and-indent' instead of `newline'.
+`newline-and-indent' with ARG, otherwise call `newline' with ARG
+and INTERACTIVE.
 
 When `org-return-follows-link' is non-nil and point is on
 a timestamp or a link, call `org-open-at-point'.  However, it
 will not happen if point is in a table or on a \"dead\"
 object (e.g., within a comment).  In these case, you need to use
 `org-open-at-point' directly."
-  (interactive)
+  (interactive "*i\nP\np")
   (let ((context (if org-return-follows-link (org-element-context)
 		   (org-element-at-point))))
     (cond
@@ -17708,23 +17720,20 @@ object (e.g., within a comment).  In these case, you need to use
 	 (t (org--align-tags-here tags-column))) ;preserve tags column
 	(end-of-line)
 	(org-show-entry)
-	(if indent (newline-and-indent) (newline))
+	(org--newline indent arg interactive)
 	(when string (save-excursion (insert (org-trim string))))))
      ;; In a list, make sure indenting keeps trailing text within.
-     ((and indent
-	   (not (eolp))
+     ((and (not (eolp))
 	   (org-element-lineage context '(item)))
       (let ((trailing-data
 	     (delete-and-extract-region (point) (line-end-position))))
-	(newline-and-indent)
+	(org--newline indent arg interactive)
 	(save-excursion (insert trailing-data))))
      (t
       ;; Do not auto-fill when point is in an Org property drawer.
       (let ((auto-fill-function (and (not (org-at-property-p))
 				     auto-fill-function)))
-	(if indent
-	    (newline-and-indent)
-	  (newline)))))))
+	(org--newline indent arg interactive))))))
 
 (defun org-return-indent ()
   "Goto next table row or insert a newline and indent.

[-- Attachment #3: Type: text/plain, Size: 1918 bytes --]


Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> Do you think a patch that
>>
>> - tweaked org-return (bound to RET) to default its INDENT argument to
>>   the current value of electric-indent-mode,

After taking an in-depth look at 'org-return' and 'newline', I decided
to "let the knife do the work" and simply keep calling 'newline', though
with additional arguments:

- INTERACTIVE is what makes 'newline' run 'post-self-insert-hook' (thus
  triggering indentation through electric-indent-mode),

- ARG wasn't strictly necessary, but it seemed harmless to add it, and
  it allows inserting multiple newlines, thus removing one more "Org
  idiosyncrasy".

I felt that introducing org--newline made the code clearer, but I can
understand if it seems too trivial to keep.  I took the liberty of using
this function in the "list item" case too, otherwise there's no way to
indent the trailing text.


> The change will not appear overnight in Org, i.e., not in Org stable's
> branch (Org 9.3.X), and it will be announced in ORG-NEWS.

I'll work on ORG-NEWS (plus Changelog entries, plus unit tests) as soon
as I'm confident that my approach is satisfactory.

(Out of curiosity, could it be argued that this is solving a "bug" in
org-mode and, as such, could be committed to Emacs core first, then
backported to the org-mode repository?  I don't feel strongly either
way, I wouldn't want to make things more complicated for Org
maintainers.)


Now for C-j, in order to minimize breakage (for anyone calling
org-return-indent from Lisp code) and simplify disabling the new
behaviour (by simply turning off electric-indent-mode in Org), should we
bind C-j to a new function?  E.g.:

(defun org-return-and-maybe-indent ()
  (interactive)
  (org-return (not electric-indent-mode)))


Thank you for your time.

Re: Reconciling org-mode idiosyncrasies with Emacs core

[Nicolas Goaziou]

Hello,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I took a stab at making RET obey electric-indent-mode in org-mode.

Thank you!

> I've got something working; I'd like to ask for a review before moving
> on to Changelog and ORG-NEWS entries (and tackling C-j… and maybe
> writing a few unit tests?).

Tests for `org-return' (named "test-org/return") are in the
"test-org.el" file in the "testing/lisp" directory. We only need to test
if electric-indent-mode has an effect, but only in regular cases.

> Here's the patch, with some additional comments below:

It looks good.

> - INTERACTIVE is what makes 'newline' run 'post-self-insert-hook' (thus
>   triggering indentation through electric-indent-mode),

OK. I thought it was necessary to call
`electric-newline-and-maybe-indent'.

> - ARG wasn't strictly necessary, but it seemed harmless to add it, and
>   it allows inserting multiple newlines, thus removing one more "Org
>   idiosyncrasy".

Good idea.

> I felt that introducing org--newline made the code clearer, but I can
> understand if it seems too trivial to keep.

No, that's fine.

> I took the liberty of using
> this function in the "list item" case too, otherwise there's no way to
> indent the trailing text.

I'm not sure what you mean. It would be a regression if you didn't use
the function there, too, wouldn't it?

> (Out of curiosity, could it be argued that this is solving a "bug" in
> org-mode and, as such, could be committed to Emacs core first, then
> backported to the org-mode repository?  I don't feel strongly either
> way, I wouldn't want to make things more complicated for Org
> maintainers.)

I cannot speak for the Emacs side, but it should land in Org 9.4, not
Org 9.3.6.

It is a very visible change, one that every Org user is going to face.
This requires a new ORG-NEWS entry. Those only appear in new minor+
releases. Therefore, if you apply it in Emacs 27.1, the change will be
announced nowhere.

> Now for C-j, in order to minimize breakage (for anyone calling
> org-return-indent from Lisp code) and simplify disabling the new
> behaviour (by simply turning off electric-indent-mode in Org), should we
> bind C-j to a new function?  E.g.:
>
> (defun org-return-and-maybe-indent ()
>   (interactive)
>   (org-return (not electric-indent-mode)))

I think so. Then we can mark `org-return-indent' as obsolete and suggest
to call `org-return' instead.

Regards,

-- 
Nicolas Goaziou

Re: Reconciling org-mode idiosyncrasies with Emacs core

[Kévin Le Gouguec]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Tests for `org-return' (named "test-org/return") are in the
> "test-org.el" file in the "testing/lisp" directory. We only need to test
> if electric-indent-mode has an effect, but only in regular cases.

Thanks for the pointer!

(I forgot to mention that AFAICT, all current tests pass with this
patch.)

>> - INTERACTIVE is what makes 'newline' run 'post-self-insert-hook' (thus
>>   triggering indentation through electric-indent-mode),
>
> OK. I thought it was necessary to call
> `electric-newline-and-maybe-indent'.

Nope; took me some time to piece everything together, but the logic (as
of 24.4) seems to be

- RET runs `newline': if electric-indent-mode is disabled, then it's a
  dumb newline, otherwise electric-indent-post-self-insert-function
  kicks in *if run interactively*;

- C-j runs `electric-newline-and-maybe-indent': it's meant to be the
  "smart newline" command when electric-indent-mode is disabled,
  otherwise it shrugs and just inserts a newline.

>> I took the liberty of using
>> this function in the "list item" case too, otherwise there's no way to
>> indent the trailing text.
>
> I'm not sure what you mean. It would be a regression if you didn't use
> the function there, too, wouldn't it?

The "list-item" case currently calls `newline-and-indent'
unconditionally, because the condition for that cond branch starts with
(and indent …).  Therefore, to make this case work with
electric-indent-mode, I had to tweak the condition; I just wanted to
bring attention to this change, since it was not as "mechanical" as for
the "at-headline" and "default" branches.

> I cannot speak for the Emacs side, but it should land in Org 9.4, not
> Org 9.3.6.
>
> It is a very visible change, one that every Org user is going to face.
> This requires a new ORG-NEWS entry. Those only appear in new minor+
> releases. Therefore, if you apply it in Emacs 27.1, the change will be
> announced nowhere.

Right.  Org master it is, then.

>> Now for C-j, in order to minimize breakage (for anyone calling
>> org-return-indent from Lisp code) and simplify disabling the new
>> behaviour (by simply turning off electric-indent-mode in Org), should we
>> bind C-j to a new function?  E.g.:
>>
>> (defun org-return-and-maybe-indent ()
>>   (interactive)
>>   (org-return (not electric-indent-mode)))
>
> I think so. Then we can mark `org-return-indent' as obsolete and suggest
> to call `org-return' instead.

Alright.


I'll try to follow-up with this additional command, tests, and
changelog/news entries in the next few days.

Thank you for the review!

[PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode (was: Reconciling org-mode idiosyncrasies with Emacs core)

[Kévin Le Gouguec]

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

Hello folks,

Here's a complete patch to make RET and C-j honor electric-indent-mode
in org-mode, targeting Org's master branch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-RET-and-C-j-obey-electric-indent-mode.patch --]
[-- Type: text/x-patch, Size: 12251 bytes --]

From ec3b06f02aa875b3c78b076e846081ce4560ec18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Tue, 5 May 2020 19:01:07 +0200
Subject: [PATCH] Make RET and C-j obey `electric-indent-mode'

* etc/ORG-NEWS: Announce the change.

* lisp/org-compat.el (org-return-indent): Deprecate this command.

* lisp/org-keys.el (org-mode-map): Rebind C-j to a command emulating
`electric-newline-and-maybe-indent'.

* lisp/org.el (org-cdlatex-environment-indent): Stop using the now
obsolete function.
(org--newline): New helper function.
(org-return): Use it to transparently handle `electric-indent-mode'.
(org-return-and-maybe-indent): New command to emulate
`electric-newline-and-maybe-indent' while taking care of Org special
cases (tables, links, timestamps).

* testing/lisp/test-org.el (test-org/with-electric-indent,
test-org/without-electric-indent): New tests.

* testing/org-test.el (org-test-with-minor-mode): New helper to set a
minor mode to a specific state, and reset it afterward.
---
 etc/ORG-NEWS             | 33 +++++++++++++++++
 lisp/org-compat.el       |  9 +++++
 lisp/org-keys.el         |  2 +-
 lisp/org.el              | 43 ++++++++++++++---------
 testing/lisp/test-org.el | 76 ++++++++++++++++++++++++++++++++++++++++
 testing/org-test.el      |  9 +++++
 6 files changed, 155 insertions(+), 17 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index c10e82f53..9c7e0d604 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -215,6 +215,32 @@ configured for ClojureScript will /not/ work.
 Babel Java blocks recognize header argument =:cmdargs= and pass its
 value in call to =java=.
 
+*** =RET= and =C-j= now obey ~electric-indent-mode~
+
+Since Emacs 24.4, ~electric-indent-mode~ is enabled by default.  In
+most major modes, this causes =RET= to reindent the current line and
+indent the new line, and =C-j= to insert a newline without indenting.
+
+Org-mode now obeys this minor mode: when ~electric-indent-mode~ is
+enabled, and point is neither in a table nor on a timestamp or a link:
+
+- =RET= (bound to ~org-return~) reindents the current line and indents
+  the new line;
+- =C-j= (bound to the new command ~org-return-and-maybe-indent~)
+  merely inserts a newline.
+
+To get the previous behaviour back, disable ~electric-indent-mode~
+explicitly:
+
+#+begin_src emacs-lisp
+(add-hook 'org-mode-hook (lambda () (electric-indent-mode -1)))
+#+end_src
+
+*** New optional numeric argument for ~org-return~
+
+In situations where ~org-return~ calls ~newline~, multiple newlines
+can now be inserted with this prefix argument.
+
 ** New commands
 *** ~org-table-header-line-mode~
 
@@ -303,6 +329,13 @@ Use ~org-hide-block-toggle~ instead.
 This function was not used in the code base, and has no clear use
 either.  It has been marked for future removal.  Please contact the
 mailing list if you use this function.
+
+*** Deprecated ~org-return-indent~ function
+
+In Elisp code, use ~(org-return t)~ instead.  Interactively, =C-j= is
+now bound to ~org-return-and-maybe-indent~, which indents the new line
+when ~electric-indent-mode~ is disabled.
+
 *** Removed ~org-maybe-keyword-time-regexp~
 
 The variable was not used in the code base.
diff --git a/lisp/org-compat.el b/lisp/org-compat.el
index aae8efbd3..2b35535fa 100644
--- a/lisp/org-compat.el
+++ b/lisp/org-compat.el
@@ -678,6 +678,15 @@ an error.  Return a non-nil value when toggling is successful."
             (goto-char (match-beginning 0))
             (org-hide-block-toggle)))))))
 
+(defun org-return-indent ()
+  "Goto next table row or insert a newline and indent.
+Calls `org-table-next-row' or `newline-and-indent', depending on
+context.  See the individual commands for more information."
+  (declare (obsolete "use `org-return' with INDENT set to t instead."
+		     "Org 9.4"))
+  (interactive)
+  (org-return t))
+
 (defmacro org-with-silent-modifications (&rest body)
   (declare (obsolete "use `with-silent-modifications' instead." "Org 9.2")
 	   (debug (body)))
diff --git a/lisp/org-keys.el b/lisp/org-keys.el
index d358da763..7c0cc9216 100644
--- a/lisp/org-keys.el
+++ b/lisp/org-keys.el
@@ -618,7 +618,7 @@ COMMANDS is a list of alternating OLDDEF NEWDEF command names."
 (org-defkey org-mode-map (kbd "C-c C-k") #'org-kill-note-or-show-branches)
 (org-defkey org-mode-map (kbd "C-c #") #'org-update-statistics-cookies)
 (org-defkey org-mode-map (kbd "RET") #'org-return)
-(org-defkey org-mode-map (kbd "C-j") #'org-return-indent)
+(org-defkey org-mode-map (kbd "C-j") #'org-return-and-maybe-indent)
 (org-defkey org-mode-map (kbd "C-c ?") #'org-table-field-info)
 (org-defkey org-mode-map (kbd "C-c SPC") #'org-table-blank-field)
 (org-defkey org-mode-map (kbd "C-c +") #'org-table-sum)
diff --git a/lisp/org.el b/lisp/org.el
index 63de7306c..dbd072aff 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15580,7 +15580,7 @@ environment remains unintended."
       ;; Get indentation of next line unless at column 0.
       (let ((ind (if (bolp) 0
 		   (save-excursion
-		     (org-return-indent)
+		     (org-return t)
 		     (prog1 (current-indentation)
 		       (when (progn (skip-chars-forward " \t") (eolp))
 			 (delete-region beg (point)))))))
@@ -17647,20 +17647,31 @@ call `open-line' on the very first character."
       (org-table-insert-row)
     (open-line n)))
 
-(defun org-return (&optional indent)
+(defun org--newline (indent arg interactive)
+  "Call `newline-and-indent' or just `newline'.
+If INDENT is non-nil, call `newline-and-indent' with ARG to
+indent unconditionally; otherwise, call `newline' with ARG and
+INTERACTIVE, which can trigger indentation if
+`electric-indent-mode' is enabled."
+  (if indent
+      (newline-and-indent arg)
+    (newline arg interactive)))
+
+(defun org-return (&optional indent arg interactive)
   "Goto next table row or insert a newline.
 
 Calls `org-table-next-row' or `newline', depending on context.
 
 When optional INDENT argument is non-nil, call
-`newline-and-indent' instead of `newline'.
+`newline-and-indent' with ARG, otherwise call `newline' with ARG
+and INTERACTIVE.
 
 When `org-return-follows-link' is non-nil and point is on
 a timestamp or a link, call `org-open-at-point'.  However, it
 will not happen if point is in a table or on a \"dead\"
 object (e.g., within a comment).  In these case, you need to use
 `org-open-at-point' directly."
-  (interactive)
+  (interactive "*i\nP\np")
   (let ((context (if org-return-follows-link (org-element-context)
 		   (org-element-at-point))))
     (cond
@@ -17711,30 +17722,30 @@ object (e.g., within a comment).  In these case, you need to use
 	 (t (org--align-tags-here tags-column))) ;preserve tags column
 	(end-of-line)
 	(org-show-entry)
-	(if indent (newline-and-indent) (newline))
+	(org--newline indent arg interactive)
 	(when string (save-excursion (insert (org-trim string))))))
      ;; In a list, make sure indenting keeps trailing text within.
-     ((and indent
-	   (not (eolp))
+     ((and (not (eolp))
 	   (org-element-lineage context '(item)))
       (let ((trailing-data
 	     (delete-and-extract-region (point) (line-end-position))))
-	(newline-and-indent)
+	(org--newline indent arg interactive)
 	(save-excursion (insert trailing-data))))
      (t
       ;; Do not auto-fill when point is in an Org property drawer.
       (let ((auto-fill-function (and (not (org-at-property-p))
 				     auto-fill-function)))
-	(if indent
-	    (newline-and-indent)
-	  (newline)))))))
+	(org--newline indent arg interactive))))))
 
-(defun org-return-indent ()
-  "Goto next table row or insert a newline and indent.
-Calls `org-table-next-row' or `newline-and-indent', depending on
-context.  See the individual commands for more information."
+(defun org-return-and-maybe-indent ()
+  "Goto next table row, or insert a newline.
+Call `org-table-next-row' or `org-return', depending on context.
+See the individual commands for more information.
+
+When inserting a newline, indent the new line if
+`electric-indent-mode' is disabled."
   (interactive)
-  (org-return t))
+  (org-return (not electric-indent-mode)))
 
 (defun org-ctrl-c-tab (&optional arg)
   "Toggle columns width in a table, or show children.
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 2c78e1e23..d481d28e3 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1310,6 +1310,82 @@
 	    (org-return)
 	    (buffer-string)))))
 
+(ert-deftest test-org/with-electric-indent ()
+  "Test RET and C-j specifications with `electric-indent-mode' on."
+  ;; Call commands interactively, since this is how `newline' knows it
+  ;; must run `post-self-insert-hook'.
+  (org-test-with-minor-mode electric-indent-mode t
+    ;; RET, like `newline', should indent.
+    (should
+     (equal "  Para\n  graph"
+	    (org-test-with-temp-text "  Para<point>graph"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    (should
+     (equal "- item1\n  item2"
+	    (org-test-with-temp-text "- item1<point>item2"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    (should
+     (equal "* heading\n  body"
+	    (org-test-with-temp-text "* heading<point>body"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    ;; C-j, like `electric-newline-and-maybe-indent', should not indent.
+    (should
+     (equal "  Para\ngraph"
+	    (org-test-with-temp-text "  Para<point>graph"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))
+    (should
+     (equal "- item1\nitem2"
+	    (org-test-with-temp-text "- item1<point>item2"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))
+    (should
+     (equal "* heading\nbody"
+	    (org-test-with-temp-text "* heading<point>body"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))))
+
+(ert-deftest test-org/without-electric-indent ()
+  "Test RET and C-j specifications with `electric-indent-mode' off."
+  ;; Call commands interactively, since this is how `newline' knows it
+  ;; must run `post-self-insert-hook'.
+  (org-test-with-minor-mode electric-indent-mode nil
+    ;; RET, like `newline', should not indent.
+    (should
+     (equal "  Para\ngraph"
+	    (org-test-with-temp-text "  Para<point>graph"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    (should
+     (equal "- item1\nitem2"
+	    (org-test-with-temp-text "- item1<point>item2"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    (should
+     (equal "* heading\nbody"
+	    (org-test-with-temp-text "* heading<point>body"
+	      (call-interactively 'org-return)
+	      (buffer-string))))
+    ;; C-j, like `electric-newline-and-maybe-indent', should indent.
+    (should
+     (equal "  Para\n  graph"
+	    (org-test-with-temp-text "  Para<point>graph"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))
+    (should
+     (equal "- item1\n  item2"
+	    (org-test-with-temp-text "- item1<point>item2"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))
+    (should
+     (equal "* heading\n  body"
+	    (org-test-with-temp-text "* heading<point>body"
+	      (call-interactively 'org-return-and-maybe-indent)
+	      (buffer-string))))))
+
 (ert-deftest test-org/meta-return ()
   "Test M-RET (`org-meta-return') specifications."
   ;; In a table field insert a row above.
diff --git a/testing/org-test.el b/testing/org-test.el
index 6904e16d1..69cdb905b 100644
--- a/testing/org-test.el
+++ b/testing/org-test.el
@@ -230,6 +230,15 @@ point at the beginning of the buffer."
 	 (delete-file file)))))
 (def-edebug-spec org-test-with-temp-text-in-file (form body))
 
+(defmacro org-test-with-minor-mode (mode state &rest body)
+  "Run BODY after setting MODE to STATE.
+Restore MODE to its former state afterward."
+  (declare (debug (sexp sexp body)) (indent 2))
+  `(let ((old-state ,mode))
+       (,mode (if ,state 1 0))
+       ,@body
+       (,mode (if old-state 1 0))))
+
 (defun org-test-table-target-expect (target &optional expect laps
 &rest tblfm)
   "For all TBLFM: Apply the formula to TARGET, compare EXPECT with result.
-- 
2.26.2


[-- Attachment #3: Type: text/plain, Size: 1319 bytes --]


To summarize (see also the patch's changelog message, and the ORG-NEWS
entries which explain how to restore the current behaviour):

- `org-return' (bound to RET) gains new arguments, and an interactive
  spec matching `newline'.

- In situations where `org-return' must insert a newline,

    - if the INDENT argument is non-nil, `newline-and-indent' is called
      unconditionally, as before,

    - otherwise, `newline' is called with ARG and INTERACTIVE; if
      electric-indent-mode is on and INTERACTIVE is non-nil, this will
      trigger indentation.

- `org-return-indent', the previous binding for C-j, is deprecated.

- C-j is now bound to `org-return-and-maybe-indent', which calls
  (org-return t) when electric-indent-mode is disabled, and
  (org-return nil) otherwise.

- In addition to the new unit tests, there is a new test helper to
  temporarily set or unset a minor mode.


Let me know if I messed up something, or if there are things to tweak
here and there.  org-test-with-minor-mode might be overkill (and I am
not used to writing macros, let alone edebug specs, so I might have
goofed somewhere), but it does make the tests easier to read & write,
so… 🤷

(I'm surprised such a macro does not already exist; did I miss it?)


Thank you for your time.

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Nicolas Goaziou]

Hello,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Here's a complete patch to make RET and C-j honor electric-indent-mode
> in org-mode, targeting Org's master branch.

Thank you very much.

I fixed a typo and applied your patch.

> +(defmacro org-test-with-minor-mode (mode state &rest body)
> +  "Run BODY after setting MODE to STATE.
> +Restore MODE to its former state afterward."
> +  (declare (debug (sexp sexp body)) (indent 2))
> +  `(let ((old-state ,mode))
> +       (,mode (if ,state 1 0))
> +       ,@body
> +       (,mode (if old-state 1 0))))

This is a nice macro.

However, when I have to reproduce a failing test, I don't even want to
think about the recipe and rather concentrate on the results. Hence,
I expect `should' macro's body to be self-sufficient. Therefore,
I skipped this part of the patch.

Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Kévin Le Gouguec]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> I fixed a typo and applied your patch.

Thank you for fixing the typo in ORG-NEWS.

I see you've removed ARG from the call to `newline-and-indent'; I don't
have a strong opinion about this (though I don't see a reason not to
keep it), but I guess the docstring of `org--newline' should be changed
to match?

> However, when I have to reproduce a failing test, I don't even want to
> think about the recipe and rather concentrate on the results. Hence,
> I expect `should' macro's body to be self-sufficient. Therefore,
> I skipped this part of the patch.

Fair enough; toggling the local variant of the minor mode is probably
cleaner anyway!


Thanks for the review, and for applying.

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Nicolas Goaziou]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I see you've removed ARG from the call to `newline-and-indent'; I don't
> have a strong opinion about this (though I don't see a reason not to
> keep it), but I guess the docstring of `org--newline' should be changed
> to match?

AFAICT, `newline-and-indent' doesn't accept any argument. Keeping it
introduces a build warning and test failures. Hence the removal.

Since you were calling it with an argument I assume this may be
a novelty in Emacs 27. However Org still supports Emacs 24.4. If that's
the case, we need an additional compatibility layer to support both
cases. WDYT?

Meanwhile, I fixed the docstring. Thanks!

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Kévin Le Gouguec]

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

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> AFAICT, `newline-and-indent' doesn't accept any argument. Keeping it
> introduces a build warning and test failures. Hence the removal.
>
> Since you were calling it with an argument I assume this may be
> a novelty in Emacs 27.

Wow, you're right.  That caught me off-guard.

>                        However Org still supports Emacs 24.4. If that's
> the case, we need an additional compatibility layer to support both
> cases. WDYT?

I don't know if we want to jump through these hoops for a feature that
people have done without so far?  FWIW though, the following patch seems
to work ("make test" works with both 26.3 and 28.0 on my end):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: newline-and-indent-compat.patch --]
[-- Type: text/x-patch, Size: 1307 bytes --]

diff --git a/lisp/org-compat.el b/lisp/org-compat.el
index 2b35535fa..ed12b9d18 100644
--- a/lisp/org-compat.el
+++ b/lisp/org-compat.el
@@ -102,6 +102,11 @@ is nil)."
   (defun org-time-convert-to-list (time)
     (seconds-to-time (float-time time))))
 
+(if (version< emacs-version "27")
+    (defsubst org-newline-and-indent (&optional _arg)
+      (newline-and-indent))
+  (defalias 'org-newline-and-indent #'newline-and-indent))
+
 \f
 ;;; Emacs < 26.1 compatibility
 
diff --git a/lisp/org.el b/lisp/org.el
index 8ad437a20..57e78599f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -17649,12 +17649,12 @@ call `open-line' on the very first character."
 
 (defun org--newline (indent arg interactive)
   "Call `newline-and-indent' or just `newline'.
-If INDENT is non-nil, call `newline-and-indent' to indent
-unconditionally; otherwise, call `newline' with ARG and
-INTERACTIVE, which can trigger indentation if
+If INDENT is non-nil, call `newline-and-indent' with ARG (if
+supported) )to indent unconditionally; otherwise, call `newline'
+with ARG and INTERACTIVE, which can trigger indentation if
 `electric-indent-mode' is enabled."
   (if indent
-      (newline-and-indent)
+      (org-newline-and-indent arg)
     (newline arg interactive)))
 
 (defun org-return (&optional indent arg interactive)

[-- Attachment #3: Type: text/plain, Size: 92 bytes --]


(I hope I got that right.)

> Meanwhile, I fixed the docstring. Thanks!

And thanks again.

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Kévin Le Gouguec]

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

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> (I hope I got that right.)

Except I forgot the explanatory comment, and I left a typo in the
docstring.  Ahem.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1376 bytes --]

diff --git a/lisp/org-compat.el b/lisp/org-compat.el
index 2b35535fa..caaf5ce58 100644
--- a/lisp/org-compat.el
+++ b/lisp/org-compat.el
@@ -102,6 +102,12 @@ is nil)."
   (defun org-time-convert-to-list (time)
     (seconds-to-time (float-time time))))
 
+;; `newline-and-indent' did not take a numeric argument before 27.1.
+(if (version< emacs-version "27")
+    (defsubst org-newline-and-indent (&optional _arg)
+      (newline-and-indent))
+  (defalias 'org-newline-and-indent #'newline-and-indent))
+
 \f
 ;;; Emacs < 26.1 compatibility
 
diff --git a/lisp/org.el b/lisp/org.el
index 8ad437a20..142bfb999 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -17649,12 +17649,12 @@ call `open-line' on the very first character."
 
 (defun org--newline (indent arg interactive)
   "Call `newline-and-indent' or just `newline'.
-If INDENT is non-nil, call `newline-and-indent' to indent
-unconditionally; otherwise, call `newline' with ARG and
-INTERACTIVE, which can trigger indentation if
+If INDENT is non-nil, call `newline-and-indent' with ARG (if
+supported) to indent unconditionally; otherwise, call `newline'
+with ARG and INTERACTIVE, which can trigger indentation if
 `electric-indent-mode' is enabled."
   (if indent
-      (newline-and-indent)
+      (org-newline-and-indent arg)
     (newline arg interactive)))
 
 (defun org-return (&optional indent arg interactive)

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Nicolas Goaziou]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

>> (I hope I got that right.)

LGTM. I applied the patch on your behalf.

Now Org has future-proof support for Electric indent mode.

Thank you.

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Bastien]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>>> (I hope I got that right.)
>
> LGTM. I applied the patch on your behalf.
>
> Now Org has future-proof support for Electric indent mode.

Thanks to both of you for this welcome improvement!

-- 
 Bastien

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Stefan Monnier]

> +(defmacro org-test-with-minor-mode (mode state &rest body)
> +  "Run BODY after setting MODE to STATE.
> +Restore MODE to its former state afterward."
> +  (declare (debug (sexp sexp body)) (indent 2))
> +  `(let ((old-state ,mode))
> +       (,mode (if ,state 1 0))
> +       ,@body
> +       (,mode (if old-state 1 0))))

This should probably use `unwind-protect` in case `body` exits
non-locally.
[ And also, for buffer-local minor modes, we should try and be careful
  to restore the mode in the same buffer, tho this can be pushed as
  a responsability of `body`.  ]


        Stefan

Re: [PATCH] Make RET and C-j obey `electric-indent-mode' in org-mode

[Kévin Le Gouguec]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +(defmacro org-test-with-minor-mode (mode state &rest body)
>> +  "Run BODY after setting MODE to STATE.
>> +Restore MODE to its former state afterward."
>> +  (declare (debug (sexp sexp body)) (indent 2))
>> +  `(let ((old-state ,mode))
>> +       (,mode (if ,state 1 0))
>> +       ,@body
>> +       (,mode (if old-state 1 0))))
>
> This should probably use `unwind-protect` in case `body` exits
> non-locally.
> [ And also, for buffer-local minor modes, we should try and be careful
>   to restore the mode in the same buffer, tho this can be pushed as
>   a responsability of `body`.  ]

Thanks for confirming my nagging feeling that this macro was a bit too
naive :)

Nicolas actually ditched it and turned on/off electric-indent-local-mode
in the temporary buffer where the Org commands are run; that should take
care of these issues IIUC.