[feature] Consistent fixed indentation of headline data

[feature] Consistent fixed indentation of headline data

[Valentin Lab]

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

Hi everybody,

I'm using org-mode for a long time, and I never understood quite well 
how headline data were supposed to be indented, however I was happy with 
what emerged to me as the default of 2 spaces (with my emacs and 
org-mode version at the time). I recently updated my old emacs to 
=9.5.3=, and what I thought was a default indentation was removed.

Suddenly, I had no indentation at all for these headline-data and this 
bugged me.

I went through documentation, and code, and (re-)discovered 
`org-adapt-indentation' that was nil in my case and is intended to stay 
this way as far as I am concerned : I'm looking for a fixed indentation 
whatever the depth of my outlines.

I'm far from sure it was a default one day, but sure it was at least 
suggested/enforced in my workflow with my emacs at some time. And even 
if it didn't feel like it was clad in iron, it seems I'm not the only 
one who was using that as I can find some examples remaining in the 
current 'testing/examples' org files.

This indentation concerns only what is called "headline data" in the 
documentation of `org-adapt-indentation'. To be precise: schedules 
("SCHEDULE:", "DEADLINE:"...), clock drawer (":LOGBOOK:..."), property 
drawer (":PROPERTY:..."). These are "data" appearing after the headline 
as I understand them.

If I'm a user of org-mode, I'm fairly new in the emacs lisp and hacking 
community and I need to know:
- if my proposal is useful and has any chance to be accepted,
- if there are any pitfalls I delved into in matter of coding, 
conventions, ...
- if it make sense for others to include this,

Many thanks !

[-- Attachment #2: 0001-org-el-Add-fixed-indentation-of-headline-data.patch --]
[-- Type: text/x-patch, Size: 10245 bytes --]

From 54ee0ce45c4a0c31a8a701047d4d56c1592fb5bb Mon Sep 17 00:00:00 2001
From: Valentin Lab <valentin.lab@kalysto.org>
Date: Fri, 1 Jul 2022 14:03:41 +0200
Subject: [PATCH] org-el: Add fixed indentation of headline data

* lisp/org.el (org-headline-data-fixed-indent-level): Definition of
new customizable variable and doc.
(org-add-planning-info): When creating planning line, force a
`org-indent-line' to indent it correctly.
(org--get-expected-indentation): If variable
`org-headline-data-fixed-indent-level' is set and line is header,
inform `org-indent-line' to indent from specified amount.
(org-adapt-indentation): Update documentation to mention new
`org-headline-data-fixed-indent-level'.

TINYCHANGE

Signed-off-by: Valentin Lab <valentin.lab@kalysto.org>
---
 lisp/org.el              |  22 ++++++-
 testing/lisp/test-org.el | 139 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 38a50d231..377a54edd 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -1428,7 +1428,8 @@ The following issues are influenced by this variable:
   indentation is not changed at all.
 
 - Property drawers and planning information is inserted indented
-  when this variable is set.  When nil, they will not be indented.
+  when this variable is set.  When nil, they will be indented
+  following `org-headline-data-fixed-indent-level'.
 
 - TAB indents a line relative to current level.  The lines below
   a headline will be indented when this variable is set to t.
@@ -1445,6 +1446,19 @@ time in Emacs."
 	  (const :tag "Do not adapt indentation at all" nil))
   :safe (lambda (x) (memq x '(t nil headline-data))))
 
+(defcustom org-headline-data-fixed-indent-level nil
+  "Indentation level for org property drawer.
+
+`org-adapt-indentation' need to be set to nil for this value
+to be considered.
+
+Note that this is all about true indentation, by adding and
+removing space characters.  See also \"org-indent.el\" which does
+level-dependent indentation in a virtual way, i.e. at display
+time in Emacs."
+  :group 'org-edit-structure
+  :type 'integer)
+
 (defvaralias 'org-special-ctrl-a 'org-special-ctrl-a/e)
 
 (defcustom org-special-ctrl-a/e nil
@@ -10060,7 +10074,8 @@ WHAT entry will also be removed."
 		    (eq what 'closed)
 		    nil nil (list org-end-time-was-given))))
 	   (unless (eolp) (insert " "))
-	   ts))))))
+	   ts))
+        (org-indent-line)))))
 
 (defvar org-log-note-marker (make-marker)
   "Marker pointing at the entry where the note is to be inserted.")
@@ -18371,6 +18386,9 @@ ELEMENT."
 			    ;; a footnote definition.
 			    (org--get-expected-indentation
 			     (org-element-property :parent previous) t))))))))))
+      ((and (not (eq org-headline-data-fixed-indent-level nil))
+         (memq type '(drawer property-drawer planning node-property clock)))
+         org-headline-data-fixed-indent-level)
       ;; Otherwise, move to the first non-blank line above.
       (t
        (beginning-of-line)
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index fcf2d0b5f..600d647e4 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -1069,6 +1069,49 @@
 	 " #+BEGIN_CENTER\n<point>  Contents\n#+END_CENTER"
 	 (org-indent-line)
 	 (org-get-indentation)))))
+  (let ((org-adapt-indentation nil)
+         (org-headline-data-fixed-indent-level 3))
+    (should
+     ;; First line should be indented using
+     ;; `org-headline-data-fixed-indent-level'.
+     (= 3
+        (org-test-with-temp-text "* H\n<point>:PROPERTY:\n:END:"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    (should
+     (= 3
+        (org-test-with-temp-text "* H\n<point>SCHEDULED:\n"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    (should
+     (= 3
+        (org-test-with-temp-text "* H\n<point>:LOGBOOK:\n:END:\n"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    ;; same for old format on CLOCKing (no LOGBOOK drawer).
+    (should
+     (= 3
+        (org-test-with-temp-text "* H\n<point>CLOCK:\n"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    ;; Lines that are not the first should not use `org-headline-data-fixed-indent-level'
+    (should
+     (= 0
+        (org-test-with-temp-text "* H\n:PROPERTY:\n<point>:foo: 1\n:END:"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    (should
+     (= 0
+        (org-test-with-temp-text "* H\n<point> foo"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    ;; First lines that are not planning/drawers/clocking should not be incremented
+    (should
+     (= 0
+        (org-test-with-temp-text "* H\n:LOGBOOK:\n<point>:foo: 1\n:END:"
+	  (org-indent-line)
+	  (org-get-indentation))))
+    )
   ;; Within code part of a source block, use language major mode if
   ;; `org-src-tab-acts-natively' is non-nil.  Otherwise, indent
   ;; according to line above.
@@ -1216,6 +1259,13 @@
             (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:"
               (org-indent-region (point-min) (point-max))
               (buffer-string)))))
+  ;; ;; Indent property drawers according to `org-adapt-indentation'.
+  ;; (let ((org-adapt-indentation 'headline-data))
+  ;;   (should
+  ;;    (equal "* H\n  :PROPERTIES:\n  :key:\n  :END:\n\ncontent2"
+  ;;           (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:\n\ncontent"
+  ;;             (org-indent-region (point-min) (point-max))
+  ;;             (buffer-string)))))
   ;; Indent plain lists.
   (let ((org-adapt-indentation t))
     (should
@@ -1228,6 +1278,33 @@
 	    (org-test-with-temp-text " - A\n\n - B"
 	      (org-indent-region (point-min) (point-max))
 	      (buffer-string)))))
+  ;; Increment list's indent level freely without following
+  ;; `org-headline-data-fixed-indent-level' if `org-adapt-indentation'
+  ;; is nil.
+  (let ((org-adapt-indentation nil)
+        (org-headline-data-fixed-indent-level nil))
+    (should
+     (equal "  - A\n    B\n    - C\n\n      X"
+	    (org-test-with-temp-text "- A\n   B\n  - C\n\n     X"
+	      (org-indent-region (point-min) (point-max))
+	      (buffer-string))))
+    (should
+     (equal "   - A\n\n   - B"
+	    (org-test-with-temp-text " - A\n\n - B"
+	      (org-indent-region (point-min) (point-max))
+	      (buffer-string)))))
+  (let ((org-adapt-indentation nil)
+        (org-headline-data-fixed-indent-level 3))
+    (should
+     (equal "  - A\n    B\n    - C\n\n      Y"
+	    (org-test-with-temp-text "- A\n   B\n  - C\n\n     Y"
+	      (org-indent-region (point-min) (point-max))
+	      (buffer-string))))
+    (should
+     (equal "     - A\n\n     - B"
+	    (org-test-with-temp-text "   - A\n\n   - B"
+	      (org-indent-region (point-min) (point-max))
+	      (buffer-string)))))
   ;; Indent footnote definitions.
   (should
    (equal "[fn:1] Definition\n\nDefinition"
@@ -5291,6 +5368,17 @@ Text.
 	    (replace-regexp-in-string
 	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
 	     nil nil 1))))
+  ;; Create deadline when `org-adapt-indentation' is nil and
+  ;; `org-headline-data-fixed-indent-level' is non-nil
+  (should
+   (equal "* H\n   DEADLINE: <2015-06-25>\nParagraph"
+	  (org-test-with-temp-text "* H\nParagraph<point>"
+	    (let ((org-adapt-indentation nil)
+                   (org-headline-data-fixed-indent-level 3))
+	      (org-add-planning-info 'deadline "<2015-06-25 Thu>"))
+	    (replace-regexp-in-string
+	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
+	     nil nil 1))))
   ;; Update deadline when `org-adapt-indentation' is non-nil.
   (should
    (equal "* H\n  DEADLINE: <2015-06-25>\nParagraph"
@@ -5315,6 +5403,20 @@ Paragraph<point>"
 	    (replace-regexp-in-string
 	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
 	     nil nil 1))))
+  ;; Update deadline when `org-adapt-indentation' is nil and
+  ;; `org-headline-data-fixed-indent-level' is non-nil
+  (should
+   (equal "* H\n   DEADLINE: <2015-06-25>\nParagraph"
+	  (org-test-with-temp-text "\
+* H
+DEADLINE: <2015-06-24 Wed>
+Paragraph<point>"
+	    (let ((org-adapt-indentation nil)
+                  (org-headline-data-fixed-indent-level 3))
+	      (org-add-planning-info 'deadline "<2015-06-25 Thu>"))
+	    (replace-regexp-in-string
+	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
+	     nil nil 1))))
   ;; Schedule when `org-adapt-indentation' is non-nil.
   (should
    (equal "* H\n  SCHEDULED: <2015-06-25>\nParagraph"
@@ -5333,6 +5435,17 @@ Paragraph<point>"
 	    (replace-regexp-in-string
 	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
 	     nil nil 1))))
+  ;; Schedule when `org-adapt-indentation' is nil and
+  ;; `org-headline-data-fixed-indent-level' is non-nil
+  (should
+   (equal "* H\n   SCHEDULED: <2015-06-25>\nParagraph"
+	  (org-test-with-temp-text "* H\nParagraph<point>"
+	    (let ((org-adapt-indentation nil)
+                   (org-headline-data-fixed-indent-level 3))
+	      (org-add-planning-info 'scheduled "<2015-06-25 Thu>"))
+	    (replace-regexp-in-string
+	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
+	     nil nil 1))))
   ;; Add deadline when scheduled.
   (should
    (equal "\
@@ -5425,6 +5538,32 @@ Paragraph<point>"
 	    (replace-regexp-in-string
 	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
 	     nil nil 1))))
+  ;; Remove closed when `org-adapt-indentation' is nil and
+  ;; `org-headline-data-fixed-indent-level' is non-nil
+  (should
+   (equal "* H\n   DEADLINE: <2015-06-25>\nParagraph"
+	  (org-test-with-temp-text "\
+* H
+CLOSED: [2015-06-25 Thu] DEADLINE: <2015-06-25 Thu>
+Paragraph<point>"
+	    (let ((org-adapt-indentation nil)
+                   (org-headline-data-fixed-indent-level 3))
+	      (org-add-planning-info nil nil 'closed))
+	    (replace-regexp-in-string
+	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
+	     nil nil 1))))
+  (should
+   (equal "* H\nParagraph"
+	  (org-test-with-temp-text "\
+* H
+  CLOSED: [2015-06-25 Thu]
+Paragraph<point>"
+	    (let ((org-adapt-indentation nil)
+                   (org-headline-data-fixed-indent-level 3))
+	      (org-add-planning-info nil nil 'closed))
+	    (replace-regexp-in-string
+	     "\\( [.A-Za-z]+\\)>" "" (buffer-string)
+	     nil nil 1))))
   ;; Remove closed entry and delete empty line.
   (should
    (equal "\
-- 
2.25.1

Re: [feature] Consistent fixed indentation of headline data

[Ihor Radchenko]

Valentin Lab <valentin.lab@kalysto.org> writes:

> I'm using org-mode for a long time, and I never understood quite well 
> how headline data were supposed to be indented, however I was happy with 
> what emerged to me as the default of 2 spaces (with my emacs and 
> org-mode version at the time). I recently updated my old emacs to 
> =9.5.3=, and what I thought was a default indentation was removed.
>
> Suddenly, I had no indentation at all for these headline-data and this 
> bugged me.

First of all, thanks for the patch! New contributions are always
welcome.

Note that we rarely change the defaults. This particular change came
after extensive discussion:
https://orgmode.org/list/878s4x3bwh.fsf@gnu.org
Also, it has been documented in https://orgmode.org/Changes.html
I recommend reviewing the changes every time you update Org to new
version.

> I went through documentation, and code, and (re-)discovered 
> `org-adapt-indentation' that was nil in my case and is intended to stay 
> this way as far as I am concerned : I'm looking for a fixed indentation 
> whatever the depth of my outlines.
>
> I'm far from sure it was a default one day, but sure it was at least 
> suggested/enforced in my workflow with my emacs at some time. And even 
> if it didn't feel like it was clad in iron, it seems I'm not the only 
> one who was using that as I can find some examples remaining in the 
> current 'testing/examples' org files.
>
> This indentation concerns only what is called "headline data" in the 
> documentation of `org-adapt-indentation'. To be precise: schedules 
> ("SCHEDULE:", "DEADLINE:"...), clock drawer (":LOGBOOK:..."), property 
> drawer (":PROPERTY:..."). These are "data" appearing after the headline 
> as I understand them.

This sounds like a reasonable addition.

> If I'm a user of org-mode, I'm fairly new in the emacs lisp and hacking 
> community and I need to know:
> - if my proposal is useful and has any chance to be accepted,

It looks useful for me.

> - if there are any pitfalls I delved into in matter of coding, 
> conventions, ...

I will provide some comments on the patch below.
In general, the patch looks nice. Providing tests is especially welcome.

> Subject: [PATCH] org-el: Add fixed indentation of headline data
>
> * lisp/org.el (org-headline-data-fixed-indent-level): Definition of
> new customizable variable and doc.
> (org-add-planning-info): When creating planning line, force a
> `org-indent-line' to indent it correctly.
> (org--get-expected-indentation): If variable
> `org-headline-data-fixed-indent-level' is set and line is header,
> inform `org-indent-line' to indent from specified amount.
> (org-adapt-indentation): Update documentation to mention new
> `org-headline-data-fixed-indent-level'.

Note that introducing a new customization should be documented in
etc/ORG-NEWS file and probably also in the manual (17.4.2 Hard
indentation).

Also, I am not sure if we really need a new custom variable. We already
have many. What about simply allowing an integer value of
org-adapt-indentation?

> TINYCHANGE
>
> Signed-off-by: Valentin Lab <valentin.lab@kalysto.org>

Note that your patch is _not_ a tiny change (not <15 LOC).
See https://orgmode.org/worg/org-contribute.html#first-patch and
https://orgmode.org/worg/org-contribute.html#copyright
Would you consider signing the copyright assignment papers with FSF?

> @@ -18371,6 +18386,9 @@ ELEMENT."
>  			    ;; a footnote definition.
>  			    (org--get-expected-indentation
>  			     (org-element-property :parent previous) t))))))))))
> +      ((and (not (eq org-headline-data-fixed-indent-level nil))
> +         (memq type '(drawer property-drawer planning node-property clock)))
> +         org-headline-data-fixed-indent-level)
>        ;; Otherwise, move to the first non-blank line above.

Why clock? It does not belong to property drawers.

> @@ -1216,6 +1259,13 @@
>              (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:"
>                (org-indent-region (point-min) (point-max))
>                (buffer-string)))))
> +  ;; ;; Indent property drawers according to `org-adapt-indentation'.
> +  ;; (let ((org-adapt-indentation 'headline-data))
> +  ;;   (should
> +  ;;    (equal "* H\n  :PROPERTIES:\n  :key:\n  :END:\n\ncontent2"
> +  ;;           (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:\n\ncontent"
> +  ;;             (org-indent-region (point-min) (point-max))
> +  ;;             (buffer-string)))))

This test is commented. Is it intentional?

Best,
Ihor

Re: [feature] Consistent fixed indentation of headline data

[Valentin Lab]

Many thanks for your warm welcome, kind review and suggestions.
I do not provide a corrective patch in this email, but it'll come soon 
depending on possible other remarks or follow-ups of this email.

On 07/07/2022 12:41, Ihor Radchenko wrote:
> Valentin Lab <valentin.lab@kalysto.org> writes:
> 
> Note that we rarely change the defaults. This particular change came
> after extensive discussion:
> https://orgmode.org/list/878s4x3bwh.fsf@gnu.org
> Also, it has been documented in https://orgmode.org/Changes.html
> I recommend reviewing the changes every time you update Org to new
> version.
> 

These links are very informative and give me much more insight on the 
current situation. Sorry if I didn't know how to get this info by 
myself. I've read the full thread you've linked.

> Note that introducing a new customization should be documented in
> etc/ORG-NEWS file and probably also in the manual (17.4.2 Hard
> indentation).

Yes, I was aware of that, but didn't want to spend to much time if my 
contribution was deemed not pertinent.

> 
> Also, I am not sure if we really need a new custom variable. We already
> have many. What about simply allowing an integer value of
> org-adapt-indentation?
> 

Well, my guess was that the "adapt" word in `org-adapt-indentation' was 
referring to adaptive (in other words: "changing") indentation depending 
on the depth of the outline. It seemed at first un-natural to force a 
fixed behavior here. This is why I went with a sub-behavior of when 
`org-adapt-indentation' was set to nil, a way to tell that we didn't 
want adaptive indentation. It also had the advantage to separate the 
implementation and help writing code that would not break the existing 
behaviors.

Of course, I'm completely okay to go with your suggestion. Although, I'm 
at a lack of inspiration about what would be the best option here: 
wouldn't a simple integer as you suggest hide the fact that this will 
target only the headline data ? Could we think of more structured value, 
perhaps like a cons =(`headline-data . 2)= ? I'm afraid these additions 
could be seen as bringing some unwanted complexity.

Although I'm expressing some doubts, I'm ready to implement it using an 
integer on `org-adapt-indentation' as you suggest. Just wanted to make 
sure it seem sound to everyone before committing to a change that is not 
that trivial (well, compared to actual changes).


>> TINYCHANGE
>>
>> Signed-off-by: Valentin Lab <valentin.lab@kalysto.org>
> 
> Note that your patch is _not_ a tiny change (not <15 LOC).
> See https://orgmode.org/worg/org-contribute.html#first-patch and
> https://orgmode.org/worg/org-contribute.html#copyright
> Would you consider signing the copyright assignment papers with FSF?

Ok, I was not sure about that (counting the actual functional code 
change was still under 15LOC, but I guess test counts also). I'm in the 
process of signing the copyright assignment papers.

> 
>> @@ -18371,6 +18386,9 @@ ELEMENT."
>>   			    ;; a footnote definition.
>>   			    (org--get-expected-indentation
>>   			     (org-element-property :parent previous) t))))))))))
>> +      ((and (not (eq org-headline-data-fixed-indent-level nil))
>> +         (memq type '(drawer property-drawer planning node-property clock)))
>> +         org-headline-data-fixed-indent-level)
>>         ;; Otherwise, move to the first non-blank line above.
> 
> Why clock? It does not belong to property drawers.
>

I probably lack some important knowledge here to understand your point. 
Here's what I understand: `clock' type targets the 'CLOCK:' keyword (and 
not direct timestamps, I added a test to make sure). AFAIK, these 
'CLOCK:' lines are made with 'C-c C-x C-i/o' and usually appears in 
between a ':LOGBOOK:' drawer. However older implementation of org-mode 
did not include these 'CLOCK: ...' lines in logbook drawers. My 
intention here, was to make sure that even these orphaned 'CLOCK: ...' 
lines, when appearing before any content, would be considered as 
headline data, and as such, be indented by the fixed amount.

I definitively consider the logbook (and clock out of logbooks), 
property drawer, and planning info ("SCHEDULED", "DEADLINE") as headline 
data and are all targeted for indentation. In my code, if I remove 
`clock' in the list of types, the intended test about 'CLOCK:' fails...

>> @@ -1216,6 +1259,13 @@
>>               (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:"
>>                 (org-indent-region (point-min) (point-max))
>>                 (buffer-string)))))
>> +  ;; ;; Indent property drawers according to `org-adapt-indentation'.
>> +  ;; (let ((org-adapt-indentation 'headline-data))
>> +  ;;   (should
>> +  ;;    (equal "* H\n  :PROPERTIES:\n  :key:\n  :END:\n\ncontent2"
>> +  ;;           (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:\n\ncontent"
>> +  ;;             (org-indent-region (point-min) (point-max))
>> +  ;;             (buffer-string)))))
> 
> This test is commented. Is it intentional?

My bad ! and an interesting talking point. I'm removing these commented 
line in the upcoming patch. They were here (and inadvertently committed) 
because while trying to test that my addition would not indent beyond 
the headline data, I noticed that actually `org-adapt-indentation' set 
to `headline-data' was actually indenting beyond headline data ! As I 
don't want to break anything, I was left quite puzzled with what to do:
- I can fix this, but fixing this is for me subject to another 
submission, and will touch behaviors that might be wanted,
- Not fixing this make me submitting a feature that carries what I see 
like a "bug".

But, is that a bug ? Here is the case:

--8<---------------cut here---------------start------------->8---
* H
:PROPERTIES:
:key:
:END:

content
--8<---------------cut here---------------start------------->8---

Using `org-indent-region' on all the content, with 
`org-adapt-indentation' set to `headline-data', will result to:

--8<---------------cut here---------------start------------->8---
* H
   :PROPERTIES:
   :key:
   :END:

   content
--8<---------------cut here---------------start------------->8---

My issue is with the treatment of the 'content' line that is not 
headline-data for me, and should not have been indented. Am I right in 
my expectation ?

Here is the test that fails (that can be copy/pasted):

#+begin_src emacs-lisp
   ;; ...
   (let ((org-adapt-indentation 'headline-data))
     (should
      (equal "* H\n  :PROPERTIES:\n  :key:\n  :END:\n\ncontent"
             (org-test-with-temp-text "* 
H\n:PROPERTIES:\n:key:\n:END:\n\ncontent"
               (org-indent-region (point-min) (point-max))
               (buffer-string)))))
   ;; ...
#+end_src

Many thanks for any insights on this point.


Thanks for your review, suggestions and advices,

Best,
Valentin Lab

[PATCH] Fix bug in org-indent-region when org-adapt-indentation is set to headline-data (was: [feature] Consistent fixed indentation of headline data)

[Ihor Radchenko]

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

Valentin Lab <valentin.lab@kalysto.org> writes:

>>> @@ -1216,6 +1259,13 @@
>>>               (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:"
>>>                 (org-indent-region (point-min) (point-max))
>>>                 (buffer-string)))))
>>> +  ;; ;; Indent property drawers according to `org-adapt-indentation'.
>>> +  ;; (let ((org-adapt-indentation 'headline-data))
>>> +  ;;   (should
>>> +  ;;    (equal "* H\n  :PROPERTIES:\n  :key:\n  :END:\n\ncontent2"
>>> +  ;;           (org-test-with-temp-text "* H\n:PROPERTIES:\n:key:\n:END:\n\ncontent"
>>> +  ;;             (org-indent-region (point-min) (point-max))
>>> +  ;;             (buffer-string)))))
>> 
>> This test is commented. Is it intentional?
>
> My bad ! and an interesting talking point. I'm removing these commented 
> line in the upcoming patch. They were here (and inadvertently committed) 
> because while trying to test that my addition would not indent beyond 
> the headline data, I noticed that actually `org-adapt-indentation' set 
> to `headline-data' was actually indenting beyond headline data ! As I 
> don't want to break anything, I was left quite puzzled with what to do:
> - I can fix this, but fixing this is for me subject to another 
> submission, and will touch behaviors that might be wanted,
> - Not fixing this make me submitting a feature that carries what I see 
> like a "bug".
>
> But, is that a bug ? Here is the case:
>
> --8<---------------cut here---------------start------------->8---
> * H
> :PROPERTIES:
> :key:
> :END:
>
> content
> --8<---------------cut here---------------start------------->8---
>
> Using `org-indent-region' on all the content, with 
> `org-adapt-indentation' set to `headline-data', will result to:
>
> --8<---------------cut here---------------start------------->8---
> * H
>    :PROPERTIES:
>    :key:
>    :END:
>
>    content
> --8<---------------cut here---------------start------------->8---
>
> My issue is with the treatment of the 'content' line that is not 
> headline-data for me, and should not have been indented. Am I right in 
> my expectation ?

Yes are right and we got a bug here.

I am attaching the tentative fix.

Bastien, I feel that the current implementation of
`org--get-expected-indentation' is wrong since we introduced
`org-adapt-indentation' 'headline-data value. AFAIU, it was done by you
in e3b79ad2b. See the commit message in the patch below for the
explanation why I think that something is going wrong.

Am I missing something?

Best,
Ihor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-indent-region-Fix-when-org-adapt-indentation-is-.patch --]
[-- Type: text/x-patch, Size: 2942 bytes --]

From 9917d69543226c68ca9e749e4f53e5f3e8ec8e79 Mon Sep 17 00:00:00 2001
Message-Id: <9917d69543226c68ca9e749e4f53e5f3e8ec8e79.1658149652.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Mon, 18 Jul 2022 21:00:13 +0800
Subject: [PATCH] org-indent-region: Fix when `org-adapt-indentation' is
 'headline-data

* lisp/org.el (org--get-expected-indentation): Remove the extra
condition added in e3b79ad2b in the cond branch for first line in an
element.  Checking `org-adapt-indentation' t value here trigger the
last default cond branch that assumes that we are _not_ at the first
line.

The new logic explicitly avoids inheriting indentation from previous
sibling when `org-adapt-indentation' is set to 'headline-data and the
previous sibling is satisfying "headline data" condition as in
`org-indent-line'.  The case when `org-adapt-indentation' is set to t
is already handled correctly when calculating the CONTENTSP
indentation for parent headline.

Fixes https://orgmode.org/list/c13cab60-bbc9-e69e-6d0d-7fe75c5908d6@kalysto.org
---
 lisp/org.el | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 99e5d0dc7..64b148d9c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -18455,9 +18455,9 @@ (defun org--get-expected-indentation (element contentsp)
 	(org-element-property :parent element) t))
       ;; At first line: indent according to previous sibling, if any,
       ;; ignoring footnote definitions and inline tasks, or parent's
-      ;; contents.
-      ((and ( = (line-beginning-position) start)
-	    (eq org-adapt-indentation t))
+      ;; contents.  If `org-adapt-indentation' is `headline-data', ignore
+      ;; previous headline data siblings.
+      ((= (line-beginning-position) start)
        (catch 'exit
 	 (while t
 	   (if (= (point-min) start) (throw 'exit 0)
@@ -18474,6 +18474,21 @@ (defun org--get-expected-indentation (element contentsp)
 		((memq (org-element-type previous)
 		       '(footnote-definition inlinetask))
 		 (setq start (org-element-property :begin previous)))
+                ;; Do not indent like previous when the previous
+                ;; element is headline data and `org-adapt-indentation'
+                ;; is set to `headline-data'.
+                ((save-excursion
+                   (goto-char start)
+                   (and
+                    (eq org-adapt-indentation 'headline-data)
+                    (not (or (org-at-clock-log-p)
+                           (org-at-planning-p)))
+                    (progn
+                      (beginning-of-line 1)
+                      (skip-chars-backward "\n")
+                      (or (org-at-heading-p)
+                          (looking-back ":END:.*" (point-at-bol))))))
+                 (throw 'exit 0))
 		(t (goto-char (org-element-property :begin previous))
 		   (throw 'exit
 			  (if (bolp) (current-indentation)
-- 
2.35.1

Re: [feature] Consistent fixed indentation of headline data

[Ihor Radchenko]

Valentin Lab <valentin.lab@kalysto.org> writes:

>> Also, I am not sure if we really need a new custom variable. We already
>> have many. What about simply allowing an integer value of
>> org-adapt-indentation?
>> 
>
> Well, my guess was that the "adapt" word in `org-adapt-indentation' was 
> referring to adaptive (in other words: "changing") indentation depending 
> on the depth of the outline. It seemed at first un-natural to force a 
> fixed behavior here. This is why I went with a sub-behavior of when 
> `org-adapt-indentation' was set to nil, a way to tell that we didn't 
> want adaptive indentation. It also had the advantage to separate the 
> implementation and help writing code that would not break the existing 
> behaviors.
>
> Of course, I'm completely okay to go with your suggestion. Although, I'm 
> at a lack of inspiration about what would be the best option here: 
> wouldn't a simple integer as you suggest hide the fact that this will 
> target only the headline data ? Could we think of more structured value, 
> perhaps like a cons =(`headline-data . 2)= ? I'm afraid these additions 
> could be seen as bringing some unwanted complexity.
>
> Although I'm expressing some doubts, I'm ready to implement it using an 
> integer on `org-adapt-indentation' as you suggest. Just wanted to make 
> sure it seem sound to everyone before committing to a change that is not 
> that trivial (well, compared to actual changes).

Your doubts make sense. In fact, I somehow missed that you only offered
to indent the headline data to fixed indentation.

Now, after more thinking and thoughtfully reading the relevant code, I
find a new variable more reasonable. However, I do not think that the
new variable should be `org-headline-data-fixed-indent-level'. Instead,
we can offer something like `org-indent-level' with default value 'auto
referring to (1+ (org-current-level)) and possible value of integer -
what you propose. Then, org-adapt-indentation will control the
indentation as usual, but the actual indentation will be calculated
depending on the `org-indent-level' value.

As an added bonus, users will also be able to set fixed indentation
using the combination of org-adapt-indentation=t and
org-indent-level=some-number. Not that I find it useful, but it will be
consistent. Moreover, the implementation will be nothing but changing
the calculation of headline contents indentation from
(1+ (org-current-level)) to a slightly more complex branching.

>> Note that your patch is _not_ a tiny change (not <15 LOC).
>> See https://orgmode.org/worg/org-contribute.html#first-patch and
>> https://orgmode.org/worg/org-contribute.html#copyright
>> Would you consider signing the copyright assignment papers with FSF?
>
> Ok, I was not sure about that (counting the actual functional code 
> change was still under 15LOC, but I guess test counts also). I'm in the 
> process of signing the copyright assignment papers.

Note that FSF should reply within 5 working days. If they don't, let us know.

>>> @@ -18371,6 +18386,9 @@ ELEMENT."
>>>   			    ;; a footnote definition.
>>>   			    (org--get-expected-indentation
>>>   			     (org-element-property :parent previous) t))))))))))
>>> +      ((and (not (eq org-headline-data-fixed-indent-level nil))
>>> +         (memq type '(drawer property-drawer planning node-property clock)))
>>> +         org-headline-data-fixed-indent-level)
>>>         ;; Otherwise, move to the first non-blank line above.
>> 
>> Why clock? It does not belong to property drawers.
>>
>
> I probably lack some important knowledge here to understand your point. 
> Here's what I understand: `clock' type targets the 'CLOCK:' keyword (and 
> not direct timestamps, I added a test to make sure). AFAIK, these 
> 'CLOCK:' lines are made with 'C-c C-x C-i/o' and usually appears in 
> between a ':LOGBOOK:' drawer. However older implementation of org-mode 
> did not include these 'CLOCK: ...' lines in logbook drawers. My 
> intention here, was to make sure that even these orphaned 'CLOCK: ...' 
> lines, when appearing before any content, would be considered as 
> headline data, and as such, be indented by the fixed amount.
>
> I definitively consider the logbook (and clock out of logbooks), 
> property drawer, and planning info ("SCHEDULED", "DEADLINE") as headline 
> data and are all targeted for indentation. In my code, if I remove 
> `clock' in the list of types, the intended test about 'CLOCK:' fails...

AFAIU, the correct test is the one used inside org-indent-line:

(and (eq org-adapt-indentation 'headline-data)
                   (not (or (org-at-clock-log-p)
                            (org-at-planning-p)))
                   (save-excursion
                     (beginning-of-line 1)
                     (skip-chars-backward "\n")
                     (or (org-at-heading-p)
                         (looking-back ":END:.*" (point-at-bol)))))

(inverse of)

Best,
Ihor

Re: [PATCH] Fix bug in org-indent-region when org-adapt-indentation is set to headline-data

[Bastien Guerry]

Hi Ihor,

I'm mostly AFK this week so I won't be able to investigate, I remember
this area was fragile.

Feel free to push the fix if it seems right to you.  We can revert it
back or improve it if needed.

Thanks a lot!

-- 
 Bastien

Re: [PATCH] Fix bug in org-indent-region when org-adapt-indentation is set to headline-data

[Ihor Radchenko]

Bastien Guerry <bzg@gnu.org> writes:

> Feel free to push the fix if it seems right to you.  We can revert it
> back or improve it if needed.

Applied onto main via 9917d6954.

Best,
Ihor