Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> On Sun, Feb 11, 2018 at 3:22 PM Nicolas Goaziou 
> wrote:
>
>>
>> > :SOME_PROP:This is a very long property value that goes beyond the
>>^^^
>> Wild guess: missing spaces?
>>
>
> Yes, that was it.
>
> Committed in
> https://code.orgmode.org/bzg/org-mode/commit/fe7619cd184847227e9c1085b4f3c13a553f007b

Thank you.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sun, Feb 11, 2018 at 3:22 PM Nicolas Goaziou 
wrote:

>
> > :SOME_PROP:This is a very long property value that goes beyond the
>^^^
> Wild guess: missing spaces?
>

Yes, that was it.

Committed in
https://code.orgmode.org/bzg/org-mode/commit/fe7619cd184847227e9c1085b4f3c13a553f007b
-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sun, Feb 11, 2018, 3:13 PM Kaushal Modi  wrote:

Sorry, ignore that, figured out the mistake in my test.

>

-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> I was actually adding a test for org-return as the commit doesn't modify
> the org-auto-fill-function.

OK.

> Interactively, the org-return change works as expected, but the test fails
> with this:
>
> Test test-org/return condition:
> (ert-test-failed
>  ((should
>(equal "* Heading
> :PROPERTIES:
> :SOME_PROP:This is a very long property value that goes beyond the
   ^^^
> fill-column. But this is inside a property drawer, so the auto-filling
> should be disabled.
>
> :END:"
>   (org-test-with-temp-text "* Heading
> :PROPERTIES:
> :SOME_PROP:This is a very long property value that goes beyond the
   ^^^
Wild guess: missing spaces?

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sun, Feb 11, 2018 at 10:54 AM Nicolas Goaziou 
wrote:

>
> Would you mind adding a test to `test-org/auto-fill-function' for good
> measure?


I was actually adding a test for org-return as the commit doesn't modify
the org-auto-fill-function.

The test I added was this.. but doesn't work. Can you please help?

Interactively, the org-return change works as expected, but the test fails
with this:

Test test-org/return condition:
(ert-test-failed
 ((should
   (equal "* Heading
:PROPERTIES:
:SOME_PROP:This is a very long property value that goes beyond the
fill-column. But this is inside a property drawer, so the auto-filling
should be disabled.

:END:"
  (org-test-with-temp-text "* Heading
:PROPERTIES:
:SOME_PROP:This is a very long property value that goes beyond the
fill-column. But this is inside a property drawer, so the auto-filling
should be disabled.
:END:" ... ... ... ...)))
  :form
  (equal "* Heading
:PROPERTIES:
:SOME_PROP:This is a very long property value that goes beyond the
fill-column. But this is inside a property drawer, so the auto-filling
should be disabled.

:END:" "* Heading
:PROPERTIES:
:SOME_PROP:This
is a very
long
property
value that
goes
beyond the
fill-column. But
this is
inside a
property
drawer, so
the
auto-filling
should be
disabled.

:END:")
  :value nil :explanation
  (array-elt 38
 (different-atoms
  (32 "#x20" "? ")
  (10 "#xa" "?
")
   FAILED  741/764  test-org/return

Here's the patch with test included:

>From 416be7c4b7adddffc0c41bba2a070c8849e16d82 Mon Sep 17 00:00:00 2001
From: Kaushal Modi 
Date: Sun, 11 Feb 2018 14:37:10 -0500
Subject: [PATCH] Do not auto-fill when point is in Org property drawer

* lisp/org.el (org-return): Set auto-fill-function to nil when point
  is in an Org property drawer.
* testing/lisp/test-org.el (test-org/return): Add test.


---
 lisp/org.el  | 8 +++-
 testing/lisp/test-org.el | 8 
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index b45cc06187d..b9daba84e6a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -2,7 +2,13 @@ object (e.g., within a comment).  In these case,
you need to use
  (delete-and-extract-region (point) (line-end-position
 (newline-and-indent)
 (save-excursion (insert trailing-data
- (t (if indent (newline-and-indent) (newline))
+ (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)))

 (defun org-return-indent ()
   "Goto next table row or insert a newline and indent.
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 17e7a223c48..1792950bba6 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1190,6 +1190,14 @@
   (should
(equal "| a |\n\n| b |"
   (org-test-with-temp-text "| a |\n| b |"
+(org-return)
+(buffer-string
+  ;; Do not auto-fill on hitting  inside a property drawer.
+  (should
+   (equal "* Heading\n:PROPERTIES:\n:SOME_PROP:This is a very long
property value that goes beyond the fill-column. But this is inside a
property drawer, so the auto-filling should be disabled.\n\n:END:"
+  (org-test-with-temp-text "* Heading\n:PROPERTIES:\n:SOME_PROP:This
is a very long property value that goes beyond the fill-column. But this is
inside a property drawer, so the auto-filling should be
disabled.\n:END:"
+(setq-local fill-column 10)
+(auto-fill-mode 1)
 (org-return)
 (buffer-string)

-- 
2.15.0


-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> I'll convert that to a proper commit with comments and commit message, and
> commit.

Great!

> Btw this is safe enough to commit to maint?

I think so.

Would you mind adding a test to `test-org/auto-fill-function' for good
measure?

Thank you.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

Hi Nicolas,

On Sun, Feb 11, 2018, 8:12 AM Nicolas Goaziou 
wrote:

>
> (and (not (org-at-property-p))
>  auto-fill-function)
>

Thanks for the feedback.

What I emailed earlier was just a buffer diff.

I'll convert that to a proper commit with comments and commit message, and
commit.

Btw this is safe enough to commit to maint?

Thanks.

>

-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Hello,

Kaushal Modi  writes:

> + (t (let ((auto-fill-function (if (org-at-property-p)
> +  nil
> +auto-fill-function)))

(and (not (org-at-property-p)) 
 auto-fill-function)

Could you add a comment explaining why this is needed?

Also, could you add a proper commit message to the patch?

Thank you.

Regards,

-- 
Nicolas Goaziou0x80A93738

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sun, Feb 4, 2018 at 5:16 PM Nicolas Goaziou 
wrote:

> `org-return' doesn't call any auto fill function.
>

It sort of does, indirectly, via (newline). Please see the proposed patch
at the end of this email.

Yet, it doesn't break properties drawers.
>

I agree. Though this patch fixes what  shouldn't do when point is in
the property drawer.

If someone has:

=
* Headline
:PROPERTY:
:FOO: some long line that goes beyond fill column
:END:
=

It would be highly unlikely that some would want to save that file as
(assuming it got formatted as below because of auto filling):

=
* Headline
:PROPERTY:
:FOO: some long line that goes beyond
fill column
:END:
=

So the below patch prevents  from doing the above


> I don't think tweaking `org-return' is a good solution. This is not the
> only way to break a line (e.g. "C-q C-j", or "C-M-o"), so it would not
> be a panacea.
>

I tried C-q C-j and C-M-o, and both don't allow the auto-filling to happen,
even without the below patch.


> You may want to use fill-nobreak-predicate variable instead, e.g. with
> `org-at-property-p'.
>

I gave that a try, by tweaking fill-nobreak-predicate inside the
org-setup-filling function. But that doesn't work. Looks like the predicate
is evaluated *after* `newline' inserts the newline.. and so
`org-at-property-p' would evaluate to nil.

** The patch **

Looking at the source code of `newline' defun, I came up with this and it
works!

diff --git a/lisp/org.el b/lisp/org.el
index 688e48bcc1d..9367304e900 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20471,7 +20471,10 @@ object (e.g., within a comment).  In these case,
you need to use
  (delete-and-extract-region (point) (line-end-position
 (newline-and-indent)
 (save-excursion (insert trailing-data
- (t (if indent (newline-and-indent) (newline))
+ (t (let ((auto-fill-function (if (org-at-property-p)
+  nil
+auto-fill-function)))
+  (if indent (newline-and-indent) (newline)))

 (defun org-return-indent ()
   "Goto next table row or insert a newline and indent.


-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> Of course. But "the right thing" would be to prevent call to auto filling
> within `org-return` (somehow? I haven't yet dug deeper into that) if point
> is in a property drawer.

`org-return' doesn't call any auto fill function.

> If you want to add a new property, use C-c C-x p, or, if you insist on
>> typing it manually, use C-q  instead.
>>
>
> C-c C-x p is a bit unnatural and also longer to type than RET,

Yet, it doesn't break properties drawers.

> especially
> if the point is already inside the property drawer. Yes, if my point is
> inside the subtree, and I think of adding a property, C-c C-x p is great!
>
> C-q  RET is a good tip, but again, RET would be better.

Actually, that would be C-q C-j, not C-q 

> I'll try hacking something around this, tweaking org-return should work
> using a simple advice. Would a patch be welcome for this?

I don't think tweaking `org-return' is a good solution. This is not the
only way to break a line (e.g. "C-q C-j", or "C-M-o"), so it would not
be a panacea.

You may want to use fill-nobreak-predicate variable instead, e.g. with
`org-at-property-p'.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sat, Feb 3, 2018 at 5:16 PM Nicolas Goaziou 
wrote:

>
> Wait... What?
>

Yes :) I had thought I couldn't get my point across correctly the first
time.


> Properties drawers do not contain blank lines. If you hit , this is
> no longer a properties drawer, but a regular one, where auto-filling is
> allowed.
>

Of course. But "the right thing" would be to prevent call to auto filling
within `org-return` (somehow? I haven't yet dug deeper into that) if point
is in a property drawer.

If you want to add a new property, use C-c C-x p, or, if you insist on
> typing it manually, use C-q  instead.
>

C-c C-x p is a bit unnatural and also longer to type than RET, especially
if the point is already inside the property drawer. Yes, if my point is
inside the subtree, and I think of adding a property, C-c C-x p is great!

C-q  RET is a good tip, but again, RET would be better.

I'll try hacking something around this, tweaking org-return should work
using a simple advice. Would a patch be welcome for this?
-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> 1. Download the attached Org file (property-auto-fill.org).
> 2. emacs -Q property-auto-fill.org
> 3. Navigate to the Heading, hit TAB, then to the PROPERTY drawer, hit TAB.
> 4. Go to the end of the DESCRIPTION property, hit RET.

Wait... What?

Properties drawers do not contain blank lines. If you hit , this is
no longer a properties drawer, but a regular one, where auto-filling is
allowed.

If you want to add a new property, use C-c C-x p, or, if you insist on
typing it manually, use C-q  instead.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nick Dokos]

Kaushal Modi  writes:

> On Sat, Feb 3, 2018 at 11:22 AM Nick Dokos  wrote:
>
> FWIW, I tried it too both with emacs -Q and in my normal working emacs: I 
> cannot reproduce it.
>
> Hello Nick, Nicolas,
>
> Thank you for trying out the MWE. I don't know what we could be doing 
> differently.
>
> So here's a recorded screencast ( https://i.imgur.com/8u8KhAy.gifv ) to get 
> rid of any miscommunication.
>

Sorry, I was wrong: I'm not sure why I could not reproduce it earlier, but I 
just retried it
both with -Q (9.1.6 -50) and my working org-mode (9.1.6 -392) and I do get 
auto-filled in both
versions. Sometimes I think I'm going mad...

-- 
Nick

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sat, Feb 3, 2018 at 11:22 AM Nick Dokos  wrote:

>
> FWIW, I tried it too both with emacs -Q and in my normal working emacs: I
> cannot reproduce it.
>

Hello Nick, Nicolas,

Thank you for trying out the MWE. I don't know what we could be doing
differently.

So here's a recorded screencast ( https://i.imgur.com/8u8KhAy.gifv ) to get
rid of any miscommunication.

Please follow this exact steps (just to reiterate, I am relying on
auto-fill-mode to not trigger when RET is pressed.. I am not using M-q
anywhere):

1. Download the attached Org file (property-auto-fill.org).
2. emacs -Q property-auto-fill.org
3. Navigate to the Heading, hit TAB, then to the PROPERTY drawer, hit TAB.
4. Go to the end of the DESCRIPTION property, hit RET.
5. Doesn't that property auto-fill for you? As you see in the screencast,
the auto-filling happens for me.

Here's the C-h l log as you see in the linked screencast:

 C-n [next-line]
 C-n [next-line]
 C-a [org-beginning-of-line]
 C-p [previous-line]
 C-a [org-beginning-of-line]
  [org-cycle]
 C-n [next-line]
  [org-cycle]
 C-n [next-line]
 C-e [org-end-of-line]
  [org-return]
 C-h l [view-lossage]

And finally the emacs and Org versions in that emacs -Q session:

- GNU Emacs 26.0.91 (build 4, x86_64-pc-linux-gnu, GTK+ Version 2.24.23) of
2018-01-31 built using repository revision
http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-26=56c37bbdb60f201791b57a3af2f47b7517de810c
- Org mode version 9.1.6 (release_9.1.6-50-g96b33f @
/home/kmodi/usr_local/apps/6/emacs/emacs-26/share/emacs/26.0.91/lisp/org/)
-- 

Kaushal Modi


property-auto-fill.org
Description: Binary data

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nick Dokos]

Kaushal Modi  writes:

> On Sat, Feb 3, 2018, 9:40 AM Nicolas Goaziou  wrote:
>
> Kaushal Modi  writes:
>
> I think I spotted your mistake. Considering (a part of) your ECM:
>
>    * Heading
>
>    :PROPERTIES:
>     
>
> There's a line between the heading and the properties drawer, so it is
> a regular drawer, 
>
> I don't follow. I don't have that extra line.. I wonder why it shows up like 
> that to you. 
>
> You can see in this screenshot I pasted earlier that I have no empty line 
> between headline and property drawer: 
> http://lists.gnu.org/r/emacs-orgmode/2018-02/png5WGaeZ0gLm.png
>
> I don't see that empty line on the web too: 
> http://lists.gnu.org/r/emacs-orgmode/2018-01/msg00497.html
>

FWIW, I tried it too both with emacs -Q and in my normal working emacs: I 
cannot reproduce it.
-- 
Nick

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> On Sat, Feb 3, 2018, 9:40 AM Nicolas Goaziou  wrote:
>
>> Kaushal Modi  writes:
>>
>> I think I spotted your mistake. Considering (a part of) your ECM:
>>
>>* Heading
>>
>>:PROPERTIES:
>>
>>
>> There's a line between the heading and the properties drawer, so it is
>> a regular drawer,
>>
>
> I don't follow. I don't have that extra line.. I wonder why it shows up
> like that to you.
>
> You can see in this screenshot I pasted earlier that I have no empty line
> between headline and property drawer:
> http://lists.gnu.org/r/emacs-orgmode/2018-02/png5WGaeZ0gLm.png
>
> I don't see that empty line on the web too:
> http://lists.gnu.org/r/emacs-orgmode/2018-01/msg00497.html

It may be Gnus, then, because the message above appears with a blank
line below the heading.

Anyways, I cannot reproduce your issue, even with local variables.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sat, Feb 3, 2018, 9:40 AM Nicolas Goaziou  wrote:

> Kaushal Modi  writes:
>
> I think I spotted your mistake. Considering (a part of) your ECM:
>
>* Heading
>
>:PROPERTIES:
>
>
> There's a line between the heading and the properties drawer, so it is
> a regular drawer,
>

I don't follow. I don't have that extra line.. I wonder why it shows up
like that to you.

You can see in this screenshot I pasted earlier that I have no empty line
between headline and property drawer:
http://lists.gnu.org/r/emacs-orgmode/2018-02/png5WGaeZ0gLm.png

I don't see that empty line on the web too:
http://lists.gnu.org/r/emacs-orgmode/2018-01/msg00497.html

>

-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Kaushal Modi  writes:

> Did you copy and paste the whole ECM including the Local Variables? ..
> because I can reproduce that in emacs -Q (emacs-26 branch).
>
> It's something about auto-fill-mode being on, that's different from
> M-q.

I think I spotted your mistake. Considering (a part of) your ECM:

   * Heading

   :PROPERTIES:
   :DESCRIPTION: This is a very long description that will auto fill at the
   =fill-column=. But this is inside a property drawer, so the auto-filling
   should be disabled.
   :END:

There's a line between the heading and the properties drawer, so it is
a regular drawer, where filling is allowed. `org-lint' warns you about
this.

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

On Sat, Feb 3, 2018, 9:29 AM Nicolas Goaziou  wrote:

> It is already the case. See `org-adaptive-fill-function': it returns nil
> when at a node property.
>

I thought so too, because the filling doesn't happen on M-q. So it took me
a while to come with that complete ECM.

I cannot reproduce your ECM.
>

Did you copy and paste the whole ECM including the Local Variables? ..
because I can reproduce that in emacs -Q (emacs-26 branch).

It's something about auto-fill-mode being on, that's different from M-q.

>

-- 

Kaushal Modi

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Nicolas Goaziou]

Hello,

Kaushal Modi  writes:

> Hello,
>
> I am bumping this thread, as it very likely could have been missed.
>
> If fixing that was already on the radar, sorry for the bump.
>
> On Tue, Jan 30, 2018, 11:50 AM Kaushal Modi  wrote:
>
>> Hello,
>>
>> I have noticed that auto-filling working on long (going past fill-column)
>> values in property drawers.
>>
>> Here's a MWE:
>>
>> =
>> #+TITLE: Test case showing auto-filling happening in Property drawers too
>> * Heading
>> :PROPERTIES:
>> :DESCRIPTION: This is a very long description that will auto fill at the
>> =fill-column=. But this is inside a property drawer, so the auto-filling
>> should be disabled.
>> :END:
>> * COMMENT Local Variables
>> # Local Variables:
>> # fill-column: 70
>> # eval: (auto-fill-mode 1)
>> # End:
>> =
>>
>> The value of that DESCRIPTION property is in a single line (I believe some
>> email clients auto-fill the text). Basically this is what it should look
>> like in emacs -Q after M-x toggle-truncate-lines:
>>
>> [image: image.png]
>>
>> Now if you hit return at the end of that long description, auto-fill kicks
>> in and turns that into this invalid drawer:
>>
>> =
>> #+TITLE: Test case showing auto-filling happening in Property drawers too
>> * Heading
>> :PROPERTIES:
>> :DESCRIPTION: This is a very long description that will auto fill at
>> the =fill-column=. But this is inside a property drawer, so the
>> auto-filling should be disabled.
>>
>> :END:
>> * COMMENT Local Variables
>> # Local Variables:
>> # fill-column: 70
>> # eval: (auto-fill-mode 1)
>> # End:
>> =
>>
>> This looks like a bug. Can the auto-filling be disabled in property
>> drawers?

It is already the case. See `org-adaptive-fill-function': it returns nil
when at a node property.

I cannot reproduce your ECM.

Regards,

-- 
Nicolas Goaziou

Re: [O] Prevent auto-fill-mode from filling Property values in drawers

[Kaushal Modi]

Hello,

I am bumping this thread, as it very likely could have been missed.

If fixing that was already on the radar, sorry for the bump.

On Tue, Jan 30, 2018, 11:50 AM Kaushal Modi  wrote:

> Hello,
>
> I have noticed that auto-filling working on long (going past fill-column)
> values in property drawers.
>
> Here's a MWE:
>
> =
> #+TITLE: Test case showing auto-filling happening in Property drawers too
> * Heading
> :PROPERTIES:
> :DESCRIPTION: This is a very long description that will auto fill at the
> =fill-column=. But this is inside a property drawer, so the auto-filling
> should be disabled.
> :END:
> * COMMENT Local Variables
> # Local Variables:
> # fill-column: 70
> # eval: (auto-fill-mode 1)
> # End:
> =
>
> The value of that DESCRIPTION property is in a single line (I believe some
> email clients auto-fill the text). Basically this is what it should look
> like in emacs -Q after M-x toggle-truncate-lines:
>
> [image: image.png]
>
> Now if you hit return at the end of that long description, auto-fill kicks
> in and turns that into this invalid drawer:
>
> =
> #+TITLE: Test case showing auto-filling happening in Property drawers too
> * Heading
> :PROPERTIES:
> :DESCRIPTION: This is a very long description that will auto fill at
> the =fill-column=. But this is inside a property drawer, so the
> auto-filling should be disabled.
>
> :END:
> * COMMENT Local Variables
> # Local Variables:
> # fill-column: 70
> # eval: (auto-fill-mode 1)
> # End:
> =
>
> This looks like a bug. Can the auto-filling be disabled in property
> drawers?
>
> I can recreate this issue on both:
> - Org stable (9.1.6 that's on emacs-26 branch) :: Org version: Org mode
> version 9.1.6 (release_9.1.6-50-g96b33f @
> /home/kmodi/usr_local/apps/6/emacs/emacs-26/share/emacs/26.0.91/lisp/org/)
> - Org master ::Org mode version 9.1.6 (release_9.1.6-395-g8ecc4c @
> /home/kmodi/usr_local/apps/6/emacs/emacs-26/share/emacs/site-lisp/org/)
>
> Thanks.
> --
>
> Kaushal Modi
>


-- 

Kaushal Modi