Re: [PATCH] Fix org-comment-line-break-function

[Ihor Radchenko]

Richard Lawrence  writes:

>> I am attaching an alternative patch to fix the issue using Org element
>> parser. Note that I reused your tests.
>
> FWIW I've tested this patch for a few days and it fixes the issue for
> me. I'd be happy if it were merged.

Thanks for testing.
Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=506d5b12ddbb319f0f70593cda54d8d9c580db7b

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at .
Support Org development at ,
or support my work at 

Fixed.
Applied.

Re: [PATCH] Fix org-comment-line-break-function

[Ihor Radchenko]

Kaushal Modi  writes:

> Hi Nicolas,
>
> I have added few tests in the updated patch pasted in this email.
> I have made the tests for (call-interactive #'default-indent-new-line)
> because that the interactive function M-j is bound to by default.
>
> Can you please review and commit it? The machine I am on right now does not
> allow external ssh access.

Unfortunately, we cannot rely on the built-in `comment-indent-new-line'
to fill Org comments. This is because Emacs uses a complex entanglement
of regexp heuristics to determine comment at point and its boundaries.

I am attaching an alternative patch to fix the issue using Org element
parser. Note that I reused your tests.

Best,
Ihor

>From ded35b55ca694e3eb831878160ac37ceec48b08e Mon Sep 17 00:00:00 2001
Message-Id: 
From: Ihor Radchenko 
Date: Thu, 29 Sep 2022 13:02:46 +0800
Subject: [PATCH] org-comment-line-break-function: Avoid built-in Emacs comment
 machinery

* lisp/org.el (org-comment-line-break-function): Rely on Org
parser (`org-adaptive-fill-function') to determine comment filling.
Handle nil values of `fill-prefix' correctly.

* testing/lisp/test-org.el (test-org/default-indent-new-line): New
test.  Written by Kaushal Modi 
https://list.orgmode.org/cafyqvy36dkbsny2mpxdnzweotjuk8maqgjm-zhxnamfreqg...@mail.gmail.com/

Reported-by: Richard Lawrence 
Link: https://list.orgmode.org/87lf18fue9.fsf@aquinas.i-did-not-set--mail-host-address--so-tickle-me/
---
 lisp/org.el  | 18 --
 testing/lisp/test-org.el | 20 
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 036384a04..5ff60baf6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19295,12 +19295,18 @@ (defun org-comment-line-break-function ( soft)
   "Break line at point and indent, continuing comment if within one.
 The inserted newline is marked hard if variable
 `use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
+non-nil.
+
+This function is a simplified version of `comment-indent-new-line'
+that bypasses the complex Emacs machinery dealing with comments.
+We instead rely on Org parser, utilizing `org-adaptive-fill-function'"
+  (let ((fill-prefix (org-adaptive-fill-function)))
+(if soft (insert-and-inherit ?\n) (newline 1))
+(save-excursion (forward-char -1) (delete-horizontal-space))
+(delete-horizontal-space)
+(indent-to-left-margin)
+(when fill-prefix
+  (insert-before-markers-and-inherit fill-prefix
 
 
 ;;; Fixed Width Areas
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 1b4157d0e..4a6a3a0b0 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1405,6 +1405,26 @@ (ert-deftest test-org/indent-region ()
 	(org-indent-region (point-min) (point-max))
 	(buffer-string)
 
+(ert-deftest test-org/default-indent-new-line ()
+  "Test behavior of default binding `M-j'."
+  ;; Calling `M-j' when point is not in an Org comment:
+  (should
+   (equal "* Some heading\n"
+  (org-test-with-temp-text "* Some heading"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string
+  ;; Calling `M-j' when point is in an Org comment:
+  (should
+   (equal "# Some Org comment\n# "
+  (org-test-with-temp-text "# Some Org comment"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string
+  (should
+   (equal "# Some Org\n# comment"
+  (org-test-with-temp-text "# Some Org comment"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string)
+
 
 
 ;;; Editing
-- 
2.35.1


-- 
Ihor Radchenko,
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] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi  writes:

> I have added few tests in the updated patch pasted in this email.
> I have made the tests for (call-interactive #'default-indent-new-line)
> because that the interactive function M-j is bound to by default.
>
> Can you please review and commit it? The machine I am on right now does not
> allow external ssh access.

Unfortunately, this seems to break "test-org/auto-fill-function" test.
Could you have a look at it?

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

Hi Nicolas,

I have added few tests in the updated patch pasted in this email.
I have made the tests for (call-interactive #'default-indent-new-line)
because that the interactive function M-j is bound to by default.

Can you please review and commit it? The machine I am on right now does not
allow external ssh access.



>From de607dff518eaa91149ff1aa8c255f67fb6ee887 Mon Sep 17 00:00:00 2001
From: Kaushal Modi 
Date: Tue, 30 Nov 2021 20:37:10 -0500
Subject: [PATCH] org: Remove `org-comment-line-break-function'

* lisp/org.el: Remove `org-comment-line-break-function' and let
`comment-line-break-function' be the default value.
* testing/lisp/test-org.el (test-org/default-indent-new-line): Add
tests for M-j behavior when point is inside or outside an Org comment.

This fixes the `M-j' binding issue reported by Richard Lawrence in
.
---
 lisp/org.el  | 17 ++---
 testing/lisp/test-org.el | 19 +++
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ec59ddf44..ee8ca1f03 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19624,8 +19624,7 @@ assumed to be significant there."

 ;; `org-auto-fill-function' takes care of auto-filling.  It calls
 ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
-;; `org-adaptive-fill-function' value.  Internally,
-;; `org-comment-line-break-function' breaks the line.
+;; `org-adaptive-fill-function' value.

 ;; `org-setup-filling' installs filling and auto-filling related
 ;; variables during `org-mode' initialization.
@@ -19647,8 +19646,7 @@ assumed to be significant there."
   (setq-local fill-paragraph-function 'org-fill-paragraph)
   (setq-local auto-fill-inhibit-regexp nil)
   (setq-local adaptive-fill-function 'org-adaptive-fill-function)
-  (setq-local normal-auto-fill-function 'org-auto-fill-function)
-  (setq-local comment-line-break-function
'org-comment-line-break-function))
+  (setq-local normal-auto-fill-function 'org-auto-fill-function))

 (defun org-fill-line-break-nobreak-p ()
   "Non-nil when a new line at point would create an Org line break."
@@ -19903,17 +19901,6 @@ filling the current element."
  (adaptive-fill-mode (not (equal fill-prefix ""
  (when fill-prefix (do-auto-fill))

-(defun org-comment-line-break-function ( soft)
-  "Break line at point and indent, continuing comment if within one.
-The inserted newline is marked hard if variable
-`use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
-

 ;;; Fixed Width Areas

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 056ea7d87..de4ac7ea9 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1200,6 +1200,25 @@
 (org-indent-region (point-min) (point-max))
 (buffer-string)

+(ert-deftest test-org/default-indent-new-line ()
+  "Test behavior of default binding `M-j'."
+  ;; Calling `M-j' when point is not in an Org comment:
+  (should
+   (equal "* Some heading\n"
+  (org-test-with-temp-text "* Some heading"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string
+  ;; Calling `M-j' when point is in an Org comment:
+  (should
+   (equal "# Some Org comment\n# "
+  (org-test-with-temp-text "# Some Org comment"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string
+  (should
+   (equal "# Some Org\n# comment"
+  (org-test-with-temp-text "# Some Org comment"
+   (call-interactively #'default-indent-new-line)
+   (buffer-string)


 ;;; Editing
-- 
2.34.0





--
Kaushal Modi


On Mon, Dec 6, 2021 at 8:17 AM Richard Lawrence <
richard.lawre...@uni-tuebingen.de> wrote:

> Kaushal Modi  writes:
>
> > On Sat, Dec 4, 2021, 5:25 PM Tim Cross  wrote:
> >
> >> Given that Nicholas cannot remember the reason for the original function
> >> and suspects it was meant to be an internal only function, I think this
> >> patch is probably the best way forward and should be applied.
> >
> > Thanks. Nicolas asked me to add tests for this patch. But I need to look
> > into how to add tests for behavior of bindings. Need to add tests for M-j
> > binding behavior when cursor is within a comment or outside.
>
> FWIW I have been running the equivalent of Kaushal's patch (I just
> removed the local binding of comment-line-break-function to
> 'org-comment-line-break-function without deleting the function) and have
> also set
>
> (debug-on-entry 'org-comment-line-break-function)
>
> I've been running this for at least a week with no issues, and have
> never been dropped into the debugger. So at least 

Re: [PATCH] Fix org-comment-line-break-function

[Richard Lawrence]

Kaushal Modi  writes:

> On Sat, Dec 4, 2021, 5:25 PM Tim Cross  wrote:
>
>> Given that Nicholas cannot remember the reason for the original function
>> and suspects it was meant to be an internal only function, I think this
>> patch is probably the best way forward and should be applied.
>
> Thanks. Nicolas asked me to add tests for this patch. But I need to look
> into how to add tests for behavior of bindings. Need to add tests for M-j
> binding behavior when cursor is within a comment or outside.

FWIW I have been running the equivalent of Kaushal's patch (I just
removed the local binding of comment-line-break-function to
'org-comment-line-break-function without deleting the function) and have
also set

(debug-on-entry 'org-comment-line-break-function)

I've been running this for at least a week with no issues, and have
never been dropped into the debugger. So at least the testing through my
normal work indicates there are no problems with the patch.

-- 
Best,
Richard

Re: [PATCH] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi  writes:

> Thanks. Nicolas asked me to add tests for this patch. But I need to look
> into how to add tests for behavior of bindings. Need to add tests for M-j
> binding behavior when cursor is within a comment or outside.

I don't think you need to test the binding. You could test
(call-interactively o-c-l-b-f) instead. 

Besides, tests are not a blocker.

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

On Sat, Dec 4, 2021, 5:25 PM Tim Cross  wrote:

>
> Given that Nicholas cannot remember the reason for the original function
> and suspects it was meant to be an internal only function, I think this
> patch is probably the best way forward and should be applied.
>

Thanks. Nicolas asked me to add tests for this patch. But I need to look
into how to add tests for behavior of bindings. Need to add tests for M-j
binding behavior when cursor is within a comment or outside.

>

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Given that Nicholas cannot remember the reason for the original function
and suspects it was meant to be an internal only function, I think this
patch is probably the best way forward and should be applied.

Kaushal Modi  writes:

> On Tue, Nov 30, 2021 at 6:29 PM Tim Cross  wrote:
>
>  It would be good to get Nicholas' input here as I think he wrote the
>  original function back in 2012. 
>
> Just to see what happens, I tried this:
>
> M-: (setq-local comment-line-break-function #'comment-indent-new-line)
>
> .. and then M-j started working perfectly! It worked fine both: in Org 
> comment lines and out of comment lines.
>
> I see that comment-indent-new-line was added to emacs in newcomment.el back 
> in 2000. So I don't know why
> org-comment-line-break-function was added. May be Nicolas can comment more on 
> that.
>
> So would this patch work?
>
> =
>
> From 1a9187b82ed8d4e8d54ddd369a44d34295281fc3 Mon Sep 17 00:00:00 2001
> From: Kaushal Modi 
> Date: Tue, 30 Nov 2021 20:37:10 -0500
> Subject: [PATCH] org: Remove `org-comment-line-break-function'
>
> * lisp/org.el: Remove `org-comment-line-break-function' and let
> `comment-line-break-function' be the default value.
>
> This fixes the `M-j' binding issue reported by Richard Lawrence in
> .
> ---
>  lisp/org.el | 17 ++---
>  1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index ec59ddf44..ee8ca1f03 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -19624,8 +19624,7 @@ assumed to be significant there."
>  
>  ;; `org-auto-fill-function' takes care of auto-filling.  It calls
>  ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
> -;; `org-adaptive-fill-function' value.  Internally,
> -;; `org-comment-line-break-function' breaks the line.
> +;; `org-adaptive-fill-function' value.  
>  
>  ;; `org-setup-filling' installs filling and auto-filling related
>  ;; variables during `org-mode' initialization.
> @@ -19647,8 +19646,7 @@ assumed to be significant there."
>(setq-local fill-paragraph-function 'org-fill-paragraph)
>(setq-local auto-fill-inhibit-regexp nil)
>(setq-local adaptive-fill-function 'org-adaptive-fill-function)
> -  (setq-local normal-auto-fill-function 'org-auto-fill-function)
> -  (setq-local comment-line-break-function 'org-comment-line-break-function))
> +  (setq-local normal-auto-fill-function 'org-auto-fill-function))
>  
>  (defun org-fill-line-break-nobreak-p ()
>"Non-nil when a new line at point would create an Org line break."
> @@ -19903,17 +19901,6 @@ filling the current element."
>   (adaptive-fill-mode (not (equal fill-prefix ""
>   (when fill-prefix (do-auto-fill))
>  
> -(defun org-comment-line-break-function ( soft)
> -  "Break line at point and indent, continuing comment if within one.
> -The inserted newline is marked hard if variable
> -`use-hard-newlines' is true, unless optional argument SOFT is
> -non-nil."
> -  (if soft (insert-and-inherit ?\n) (newline 1))
> -  (save-excursion (forward-char -1) (delete-horizontal-space))
> -  (delete-horizontal-space)
> -  (indent-to-left-margin)
> -  (insert-before-markers-and-inherit fill-prefix))
> -
>  
>  ;;; Fixed Width Areas

Re: [PATCH] Fix org-comment-line-break-function

[Nicolas Goaziou]

Hello,

Kaushal Modi  writes:

> I see that comment-indent-new-line was added to emacs in newcomment.el back
> in 2000. So I don't know why org-comment-line-break-function was added. May
> be Nicolas can comment more on that.

Sorry, I do not remember.

As pointed out in the thread, this function probably wasn't meant to be
public in the first place. Now it is.

> So would this patch work?

Such a patch must be accompanied with tests.

Regards,
-- 
Nicolas Goaziou

Re: [PATCH] Fix org-comment-line-break-function

[Marco Wahl]

Tim and all!

>> diff --git a/lisp/org.el b/lisp/org.el
>> index 1a1375461..fdeec0d67 100644
>> --- a/lisp/org.el
>> +++ b/lisp/org.el
>> @@ -19695,7 +19695,8 @@ non-nil."
>>(save-excursion (forward-char -1) (delete-horizontal-space))
>>(delete-horizontal-space)
>>(indent-to-left-margin)
>> -  (insert-before-markers-and-inherit fill-prefix))
>> +  (when fill-prefix
>> +(insert-before-markers-and-inherit fill-prefix)))
>>  
>> I don't have anything better.  I think this is a good patch.  It makes
>> M-j work again.
>>
>> Possible refinements and improvements can follow.
>>
>> +1 for applying of your patch.
>
> I was finally able to reproduce the error. It depends to some degree on
> the text in the buffer and where the cursor is when you hit M-j. Adding
> some additional text and moving the cursor to different locations
> enabled me to reproduce the error and I now agree it is a bug in
> org-comment-line-break-function.
>
> I don't know if your patch is the right fix or not because I don't
> understand what the purpose of insert-before-marks-and-inherit is - in
> fact, the doc string for that function doesn't even state what the @rest
> args argument is for, so I don't understand why it is passing in
> fill-prefix. For example, is it safe to assume
> insert-before-merks-and-inherit does not need to be called if
> fill-prefix is nil? Why is that function even called with the
> fill-prefix as an argument? 

Thanks for staying critical!

I can't answer your questions.  I agree that we should deepen the
understanding.  Your questions are a start.

Pragmatically I still vote for applying the patch immediately since it
is a step in the right direction AFAICS.


Ciao,
-- 
Marco

Re: [PATCH] Fix org-comment-line-break-function

[Richard Lawrence]

Tim Cross  writes:

> Well, that is the big question - why was
> org-comment-line-break-function added instead of just using the
> default comment-indent-new-line?

Looking back at commit d58d40f0c864ae3a6d7c66df34769619ad2486c1, I see
this comment was added by Nicolas (still in org.el):

;; `org-auto-fill-function' takes care of auto-filling. It calls
;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
;; `org-adaptive-fill-function' value. Internally,
;; `org-comment-line-break-function' breaks the line.

which at least suggests ("Internally, ...") that
org-comment-line-break-function was never intended to be called to break
a comment via a user command like M-j, and was only intended to be
called during filling, in a context where fill-prefix would be set correctly.

In that case, the problem might be that org-setup-filling sets
comment-line-break-function to 'org-comment-line-break-function
(overriding the default value of 'comment-indent-new-line) "globally" in
Org buffers, causing it to be called even when fill-prefix is not
set correctly.

The documentation for (the variable) comment-line-break-function is a
bit confusing:

"Mode-specific function that line breaks and continues a comment.
This function is called during auto-filling when a comment syntax
is defined."

which also suggests that this function will only be called during
filling. But that is clearly not true (even if it once was), since
default-indent-new-line calls it directly.

So maybe org-comment-line-break-function was written under assumptions
that no longer hold?

It might also be worth pointing out here that
org-comment-line-break-function is *almost* a line-for-line copy of part of
default-indent-new-line, except the latter is more careful about
checking that fill-prefix is not nil before calling insert-* on it.

> Until this is determined, I think the only 'safe' approach would be to
> just advise those who are impacted by the M-j issue to set
> comment-line-break-function to comment-indent-new-line and then wait
> to see if someone who has more historical context to comment. 

This is a bit trickier than it sounds, because in Org buffers,
org-setup-filling calls

(setq-local comment-line-break-function 'org-comment-line-break-function)

which will override any global value for comment-line-break-function
(e.g. in your init file).  So you'd have to reset it in Org
buffers locally, via a hook that runs after org-setup-filling.

-- 
Best,
Richard

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Kaushal Modi  writes:

> On Tue, Nov 30, 2021 at 6:29 PM Tim Cross  wrote:
>
>  It would be good to get Nicholas' input here as I think he wrote the
>  original function back in 2012. 
>
> Just to see what happens, I tried this:
>
> M-: (setq-local comment-line-break-function #'comment-indent-new-line)
>
> .. and then M-j started working perfectly! It worked fine both: in Org 
> comment lines and out of comment lines.
>
> I see that comment-indent-new-line was added to emacs in newcomment.el back 
> in 2000. So I don't know why
> org-comment-line-break-function was added. May be Nicolas can comment more on 
> that.
>
> So would this patch work?
>

Well, that is the big question - why was org-comment-line-break-function
added instead of just using the default comment-indent-new-line? My
only thoughts are there must be some subtle difference in org which the
default function was not sophisticated enough to work with. Problem is,
not knowing what that might be makes it hard to test and verify the real
impact of making the change.

Until this is determined, I think the only 'safe' approach would be to
just advise those who are impacted by the M-j issue to set
comment-line-break-function to comment-indent-new-line and then wait to
see if someone who has more historical context to comment. If nothing or
nobody says anything after a couple of months, then maybe apply your
patch.

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

On Tue, Nov 30, 2021 at 6:29 PM Tim Cross  wrote:

>
> It would be good to get Nicholas' input here as I think he wrote the
> original function back in 2012.
>

Just to see what happens, I tried this:

M-: (setq-local comment-line-break-function #'comment-indent-new-line)

.. and then M-j started working perfectly! It worked fine both: in Org
comment lines and out of comment lines.

I see that comment-indent-new-line was added to emacs in newcomment.el back
in 2000. So I don't know why org-comment-line-break-function was added. May
be Nicolas can comment more on that.

So would this patch work?

=

>From 1a9187b82ed8d4e8d54ddd369a44d34295281fc3 Mon Sep 17 00:00:00 2001
From: Kaushal Modi 
Date: Tue, 30 Nov 2021 20:37:10 -0500
Subject: [PATCH] org: Remove `org-comment-line-break-function'

* lisp/org.el: Remove `org-comment-line-break-function' and let
`comment-line-break-function' be the default value.

This fixes the `M-j' binding issue reported by Richard Lawrence in
.
---
 lisp/org.el | 17 ++---
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ec59ddf44..ee8ca1f03 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19624,8 +19624,7 @@ assumed to be significant there."

 ;; `org-auto-fill-function' takes care of auto-filling.  It calls
 ;; `do-auto-fill' only on valid areas with `fill-prefix' shadowed with
-;; `org-adaptive-fill-function' value.  Internally,
-;; `org-comment-line-break-function' breaks the line.
+;; `org-adaptive-fill-function' value.

 ;; `org-setup-filling' installs filling and auto-filling related
 ;; variables during `org-mode' initialization.
@@ -19647,8 +19646,7 @@ assumed to be significant there."
   (setq-local fill-paragraph-function 'org-fill-paragraph)
   (setq-local auto-fill-inhibit-regexp nil)
   (setq-local adaptive-fill-function 'org-adaptive-fill-function)
-  (setq-local normal-auto-fill-function 'org-auto-fill-function)
-  (setq-local comment-line-break-function
'org-comment-line-break-function))
+  (setq-local normal-auto-fill-function 'org-auto-fill-function))

 (defun org-fill-line-break-nobreak-p ()
   "Non-nil when a new line at point would create an Org line break."
@@ -19903,17 +19901,6 @@ filling the current element."
  (adaptive-fill-mode (not (equal fill-prefix ""
  (when fill-prefix (do-auto-fill))

-(defun org-comment-line-break-function ( soft)
-  "Break line at point and indent, continuing comment if within one.
-The inserted newline is marked hard if variable
-`use-hard-newlines' is true, unless optional argument SOFT is
-non-nil."
-  (if soft (insert-and-inherit ?\n) (newline 1))
-  (save-excursion (forward-char -1) (delete-horizontal-space))
-  (delete-horizontal-space)
-  (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
-

 ;;; Fixed Width Areas

-- 
2.34.0

=

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

On Tue, Nov 30, 2021, 6:29 PM Tim Cross  wrote:

>
> Regardless, I think that unless we understand the purpose of
> insert-before-markers-and-inherit, we should make the patch such that it
> still calls that function. Even if fill-prefix is nil there is
> probably a good reason why the markers and properties need to be
> modified for some situations.
>

Oops! You're right; I did not verify that binding within a comment. I just
checked that it prevented the error.

It would be good to understand the purpose of that function call and have a
proper fix.

>

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Kaushal Modi  writes:

> On Tue, Nov 30, 2021 at 3:20 PM Marco Wahl  wrote:
>
>  diff --git a/lisp/org.el b/lisp/org.el
>  index 1a1375461..fdeec0d67 100644
>  --- a/lisp/org.el
>  +++ b/lisp/org.el
>  @@ -19695,7 +19695,8 @@ non-nil."
> (save-excursion (forward-char -1) (delete-horizontal-space))
> (delete-horizontal-space)
> (indent-to-left-margin)
>  -  (insert-before-markers-and-inherit fill-prefix))
>  +  (when fill-prefix
>  +(insert-before-markers-and-inherit fill-prefix)))
>
>  I don't have anything better.  I think this is a good patch.  It makes
>  M-j work again.
>
>  Possible refinements and improvements can follow.
>
>  +1 for applying of your patch.
>
> I am able to reproduce that M-j issue (using Emacs version: GNU Emacs 28.0.60 
> (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.22.30,
> cairo version 1.15.12)
>  of 2021-11-29, built using commit c4daff9cf844ec85930bdcd2064787c92c260861, 
> and Org mode version 9.5
> (release_9.5-292-g5e35de)).
>
> And this patch fixes that for me as well.
>
> +1 for applying this patch.
>
> =
>
> Before this patch, M-j gave this backtrace with debug enabled:
>
> Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
>   insert-before-markers-and-inherit(nil)
>   org-comment-line-break-function(nil)
>   default-indent-new-line(nil t)
>   funcall-interactively(default-indent-new-line nil t)
>   call-interactively(default-indent-new-line nil nil)
>   command-execute(default-indent-new-line)
>
> Output of C-h k M-j:
>
> M-j runs the command default-indent-new-line (found in global-map),
> which is an interactive compiled Lisp function in ‘simple.el’.
>
> It is bound to C-M-j, M-j.
>
> (default-indent-new-line  SOFT FORCE)
>
> Break line at point and indent.
> If a comment syntax is defined, call ‘comment-line-break-function’.
>
> The inserted newline is marked hard if variable ‘use-hard-newlines’ is true,
> unless optional argument SOFT is non-nil.

I'm not sure this is the right patch to apply. While it does fix the
immediate error, it really does so by just avoiding the call to
insert-before-markers-and-inherit when fill-prefix is nil. It does not
address the question of what that function is supposed to do or whether
the correct fix is either to call the function without the fill-prefix
argument (which also works in that it avoids the error) or if instead,
the patch should be

(if fill-prefix
  (insert-before-markers-and-inherit fill-prefix)
 (insert-before-markers-and-inherit))

I note also that with or without the patch, the function does not appear
to work correctly anyway. If you hit M-j while in a comment, the new
line should be indented appropriately and have the comment character
prefix i.e. start a new comment line. It does not do that. This is
supposed to be the key difference between C-j and M-j.

Regardless, I think that unless we understand the purpose of
insert-before-markers-and-inherit, we should make the patch such that it
still calls that function. Even if fill-prefix is nil there is
probably a good reason why the markers and properties need to be
modified for some situations. 

It would be good to get Nicholas' input here as I think he wrote the
original function back in 2012. 

Re: [PATCH] Fix org-comment-line-break-function

[Tim Cross]

Marco Wahl  writes:

> Hi Richard and all,
>
> [...]
>
>> Just to be extra, super sure, I built Emacs this afternoon from a
>> checkout of the repo, and the error is *still* there, with the same
>> cause. In that build, with emacs -Q, I have:
>>
>> (org-version)
>> "9.5"
>>
>> (emacs-version)
>> "GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo 
>> version 1.16.0)
>>  of 2021-11-30"
>>
>> At this point I've replicated the bug on my machine in four different
>> builds of Emacs (version 26.1 from Debian, 27.2 and "emacs-next" from
>> Guix, and version 29.0.50 I built myself from source) with several
>> versions of Org (the built-in ones in these Emacsen and a recent build
>> of the bugfix branch). It is robustly reproducible for me, and the cause
>> is clear: default-indent-new-line calls org-comment-line-break-function,
>> which calls
>>
>> (insert-before-markers-and-inherit nil)
>>
>> which is a type error. I'm looking for help figuring out what the right
>> fix is. I attach a patch for the simplest fix I can think of; please let
>> me know if something else would be better.
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 1a1375461..fdeec0d67 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -19695,7 +19695,8 @@ non-nil."
>(save-excursion (forward-char -1) (delete-horizontal-space))
>(delete-horizontal-space)
>(indent-to-left-margin)
> -  (insert-before-markers-and-inherit fill-prefix))
> +  (when fill-prefix
> +(insert-before-markers-and-inherit fill-prefix)))
>  
> I don't have anything better.  I think this is a good patch.  It makes
> M-j work again.
>
> Possible refinements and improvements can follow.
>
> +1 for applying of your patch.
>
>

I was finally able to reproduce the error. It depends to some degree on
the text in the buffer and where the cursor is when you hit M-j. Adding
some additional text and moving the cursor to different locations
enabled me to reproduce the error and I now agree it is a bug in
org-comment-line-break-function.

I don't know if your patch is the right fix or not because I don't
understand what the purpose of insert-before-marks-and-inherit is - in
fact, the doc string for that function doesn't even state what the @rest
args argument is for, so I don't understand why it is passing in
fill-prefix. For example, is it safe to assume
insert-before-merks-and-inherit does not need to be called if
fill-prefix is nil? Why is that function even called with the
fill-prefix as an argument? 

Re: [PATCH] Fix org-comment-line-break-function

[Kaushal Modi]

On Tue, Nov 30, 2021 at 3:20 PM Marco Wahl  wrote:

>
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 1a1375461..fdeec0d67 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -19695,7 +19695,8 @@ non-nil."
>(save-excursion (forward-char -1) (delete-horizontal-space))
>(delete-horizontal-space)
>(indent-to-left-margin)
> -  (insert-before-markers-and-inherit fill-prefix))
> +  (when fill-prefix
> +(insert-before-markers-and-inherit fill-prefix)))
>
> I don't have anything better.  I think this is a good patch.  It makes
> M-j work again.
>
> Possible refinements and improvements can follow.
>
> +1 for applying of your patch.
>

I am able to reproduce that M-j issue (using Emacs version: GNU Emacs
28.0.60 (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version
1.15.12)
 of 2021-11-29, built using commit
c4daff9cf844ec85930bdcd2064787c92c260861, and Org mode version 9.5
(release_9.5-292-g5e35de)).

And this patch fixes that for me as well.

+1 for applying this patch.

=

Before this patch, M-j gave this backtrace with debug enabled:

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  insert-before-markers-and-inherit(nil)
  org-comment-line-break-function(nil)
  default-indent-new-line(nil t)
  funcall-interactively(default-indent-new-line nil t)
  call-interactively(default-indent-new-line nil nil)
  command-execute(default-indent-new-line)


Output of C-h k M-j:

M-j runs the command default-indent-new-line (found in global-map),
which is an interactive compiled Lisp function in ‘simple.el’.

It is bound to C-M-j, M-j.

(default-indent-new-line  SOFT FORCE)

Break line at point and indent.
If a comment syntax is defined, call ‘comment-line-break-function’.

The inserted newline is marked hard if variable ‘use-hard-newlines’ is true,
unless optional argument SOFT is non-nil.

Re: [PATCH] Fix org-comment-line-break-function

[Marco Wahl]

Hi Richard and all,

[...]

> Just to be extra, super sure, I built Emacs this afternoon from a
> checkout of the repo, and the error is *still* there, with the same
> cause. In that build, with emacs -Q, I have:
>
> (org-version)
> "9.5"
>
> (emacs-version)
> "GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo 
> version 1.16.0)
>  of 2021-11-30"
>
> At this point I've replicated the bug on my machine in four different
> builds of Emacs (version 26.1 from Debian, 27.2 and "emacs-next" from
> Guix, and version 29.0.50 I built myself from source) with several
> versions of Org (the built-in ones in these Emacsen and a recent build
> of the bugfix branch). It is robustly reproducible for me, and the cause
> is clear: default-indent-new-line calls org-comment-line-break-function,
> which calls
>
> (insert-before-markers-and-inherit nil)
>
> which is a type error. I'm looking for help figuring out what the right
> fix is. I attach a patch for the simplest fix I can think of; please let
> me know if something else would be better.

diff --git a/lisp/org.el b/lisp/org.el
index 1a1375461..fdeec0d67 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -19695,7 +19695,8 @@ non-nil."
   (save-excursion (forward-char -1) (delete-horizontal-space))
   (delete-horizontal-space)
   (indent-to-left-margin)
-  (insert-before-markers-and-inherit fill-prefix))
+  (when fill-prefix
+(insert-before-markers-and-inherit fill-prefix)))
 
I don't have anything better.  I think this is a good patch.  It makes
M-j work again.

Possible refinements and improvements can follow.

+1 for applying of your patch.


Ciao,
-- 
Marco