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