From: Valentin Lab <valentin.lab@kalysto.org>
To: Ihor Radchenko <yantar92@gmail.com>
Cc: emacs-orgmode@gnu.org
Subject: Re: [feature] Consistent fixed indentation of headline data
Date: Mon, 11 Jul 2022 21:02:52 +0200 [thread overview]
Message-ID: <c13cab60-bbc9-e69e-6d0d-7fe75c5908d6@kalysto.org> (raw)
In-Reply-To: <87v8s9urvg.fsf@localhost>
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
next prev parent reply other threads:[~2022-07-11 19:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-05 14:59 [feature] Consistent fixed indentation of headline data Valentin Lab
2022-07-07 10:41 ` Ihor Radchenko
2022-07-11 19:02 ` Valentin Lab [this message]
2022-07-18 13:11 ` [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
2022-07-18 17:28 ` [PATCH] Fix bug in org-indent-region when org-adapt-indentation is set to headline-data Bastien Guerry
2022-07-19 13:59 ` Ihor Radchenko
2022-07-18 13:26 ` [feature] Consistent fixed indentation of headline data Ihor Radchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c13cab60-bbc9-e69e-6d0d-7fe75c5908d6@kalysto.org \
--to=valentin.lab@kalysto.org \
--cc=emacs-orgmode@gnu.org \
--cc=yantar92@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).