all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Ihor Radchenko <yantar92@gmail.com>
To: Valentin Lab <valentin.lab@kalysto.org>
Cc: emacs-orgmode@gnu.org
Subject: Re: [feature] Consistent fixed indentation of headline data
Date: Mon, 18 Jul 2022 21:26:19 +0800	[thread overview]
Message-ID: <87cze2h7r8.fsf@localhost> (raw)
In-Reply-To: <c13cab60-bbc9-e69e-6d0d-7fe75c5908d6@kalysto.org>

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


      parent reply	other threads:[~2022-07-18 13:26 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
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     ` Ihor Radchenko [this message]

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87cze2h7r8.fsf@localhost \
    --to=yantar92@gmail.com \
    --cc=emacs-orgmode@gnu.org \
    --cc=valentin.lab@kalysto.org \
    /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 external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.