Re: [patch suggestion] Mitigating the poor Emacs performance on huge org files: Do not use overlays for PROPERTY and LOGBOOK drawers

[Ihor Radchenko <yantar92@gmail.com>]

Hello,

[The patch itself will be provided in the following email or can be accessed via Github [1]]

I have finally finished the suggested edits. Most importantly:

- All the folding-related code lives in =org-fold.el= and =org-cycle.el= now.
- =org-fold.el= have commentary section explaining how folding works and exposing API for external code using folding.
- I wrote a patch for =isearch.el= adding support searching inside text hidden via text properties [2] and the relevant support of the patch in the =org-fold.el=. The current =isearch= behaviour is also supported. Hope the patch will go through eventually.

The patch is fairly stable on my system. Any feedback or bug reports are welcome.

There are still known problems though. The patch currently breaks many org-mode tests when running =make test=. It is partially because some tests assume overlays to be used for folding and partially because the patch appears to break certain folding conventions. I am still investigating this (and learning =ert=).

More details:

>> 2. The text property stack is rewritten using char-property-alias-alist.
>>    This is faster in comparison with previous approach, which involved
>>    modifying all the text properties every timer org-flag-region was
>>    called. 

> I'll need information about this, as I'm not sure to fully understand
> all the consequences of this. But more importantly, this needs to be
> copiously documented somewhere for future hackers.

See commentary section in =org-fold.el= and comments in
=org-fold--property-symbol-get-create=.

> As a reminder, Org 9.4 is about to be released, but Org 9.5 will take
> months to go out. So, even though I hope your changes will land into
> Org, there is no reason for us to refrain from improving (actually
> fixing a regression in) 9.4 release. Hopefully, once 9.4 is out, such
> changes are not expected to happen anymore.
>
> I hope you understand.

Probably my message sounded harsher than it should. I totally understand
why such changes are needed, but wanted to make people aware that old
folding implementation will be likely changed.

> First, it includes a few unrelated changes that should be removed (e.g.,
> white space fixes in unrelated parts of the code). Also, as written
> above, the changes about `org-custom-properties-hide-emptied-drawers'
> should be removed for the time being.

Let's leave this until the patch is ready to be pushed. I want to focus
on handling bugs first without a need to check for the whitespace
changes.

> Once done, I think we should move (or copy, first) _all_ folding-related
> functions into a new "org-fold.el" library. Functions and variables
> included there should have a proper "org-fold-" prefix. More on this in
> the detailed report.

I decided to create =org-fold.el= and =org-cycle.el= and move all the
relevant functions there. The org-cycle code seems to be so frequently
used that I did not want to break the org-fold prefix to org-fold-cycle
and decided to separate the cycle code into standalone file.

> Then, another patch can integrate "org-fold.el" into Org folding. I also
> suggest to move the Outline -> Org transition to yet another patch.
> I think there's more work to do on this part.

Agree. For the time being, I will still provide the full patch if anyone
wants to test the whole thing on their system.

> 1. we still support Emacs 24.4 (and probably Emacs 24.3, but I'm not
>    sure), so some functions cannot be used.

I tried my best to cleanup the functions as you suggested, but I do not
know a good way to check which functions are not supported by old Emacs
versions.

> 2. we don't use "subr-x.el" in the code base. In particular, it would be
>    nice to replace `when-let' with `when' + `let'. This change costs
>    only one loc.

Done.

> 3. Some docstrings need more work. In particular, Emacs documentation
>    expects all arguments to be explained in the docstring, if possible
>    in the order in which they appear. There are exceptions, though. For
>    example, in a function like `org-remove-text-properties', you can
>    mention arguments are simply the same as in `remove-text-properties'.

Done.

> 5. I didn't dive much into the Isearch code so far. I tested it a bit
>    and seems to work nicely. I noticed one bug though. In the following
>    document:
>
>        #+begin: foo
>        :FOO:
>        bar
>        :END:
>        #+end
>        bar
>
>    when both the drawer and the block are folded (i.e., you fold the
>    drawer first, then the block), searching for "bar" first find the
>    last one, then overwraps and find the first one.

Fixed now.

> 6. Since we're rewriting folding code, we might as well rename folding
>    properties: org-hide-drawer -> org-fold-drawer, outline ->
>    org-fold-headline…

Done. See =org-fold-get-folding-spec-for-element=.

>> +(defun org-remove-text-properties (start end properties &optional object)
>
> IMO, this generic name doesn't match the specialized nature of the
> function. It doesn't belong to "org-macs.el", but to the new "Org Fold" library.

This function is unused. I simply removed the function altogether.

>> +(defun org--find-text-property-region (pos prop)
>
> I think this is a function useful enough to have a name without double
> dashes. It can be left in "org-macs.el". It would be nice to have
> a wrapper for `invisible' property in "org-fold.el", tho.

Done. See org-find-text-property-region and
org-fold-get-region-at-point.

>> +  "Find a region containing PROP text property around point POS."
>
> Reverse the order of arguments in the docstring:

Done

>> +  (let* ((beg (and (get-text-property pos prop) pos))
>> +	 (end beg))
>> +    (when beg
>
> BEG can only be nil if arguments are wrong. In this case, you can
> throw an error (assuming this is no longer an internal function):

I added "Return nil when PROP is not set at POS." to the docstring.
I believe it is better not to force the user to check the property at
point before calling this function or catch errors in the code.

> I assume this will be the case in an empty buffer. Anyway, (1 . 1)
> sounds more regular than a nil return value, not specified in the
> docstring. IOW, I suggest to remove this check.

Removed.

>> +(defun org--add-to-list-text-property (from to prop element)
>> +  "Add element to text property PROP, whos value should be a list."
>
> The docstring is incomplete. All arguments need to be described. Also,

This functions is unused. I removed it completely.

>> +(defvar org--invisible-spec-priority-list '(outline org-hide-drawer org-hide-block)
>> +  "Priority of invisibility specs.")
>
> This should be the constant I wrote about earlier. Note that those are
> not "specs", just properties. I suggest to rename it.

Please note that 'outline, 'out-hide-drawer, and 'org-hide-block (now
renamed to 'org-fold-outline, 'org-fold-drawer, and 'org-fold-block) are
not text property names. They are values stored in text properties used
to fold the text. That's why I call them "folding specs", similarly to
=buffer-invisibility-spec= in Emacs. Internally, they are exactly used
as members of =buffer-invisibility-spec=.

>> +(defun org--get-buffer-local-invisible-property-symbol (spec &optional buffer return-only)
>
> This name is waaaaaaay too long.

Changed to org-fold--property-symbol-get-create. It is still long, but
it don't need to (and should not) be used outside org-fold.el from now.

> Maybe:
>
>
>   Return a unique symbol suitable for `invisible' property.
>
> Then:
>
>   Return value is meant to be used as a buffer-local variable in
>   current buffer, or BUFFER if this is non-nil.

Changed the docstring in similar manner.

> No need to waste an indentation level for that:
>
>   (unless (member …)
>    (user-error "%S should be …" spec))

Done

>> +    (let* ((buf (or buffer (current-buffer))))
>> +      (let ((local-prop (intern (format "org--invisible-%s-buffer-local-%S"
>
> This clearly needs a shorter name. In particular, "buffer-local" can be removed.

Changed to "org-fold--spec-%s-%S".

>> +        (prog1
>> +            local-prop
>
> Please move LOCAL-PROP after the (unless return-only ...) sexp.

I am not sure I understand why this needs to be changed. I feel that
listing the return value will be more clear while reading the code. The
remaining part of the =prog1= is optional logic. Moving =local-prop= to
the end may reduce readability.

> We cannot use `alist-get', which was added in Emacs 25.3 only.

Changed to =assq=.

> Likewise, we cannot use `seq-find'.

Changed to =dolist=.

> This begs for explainations in the docstring or as comments. In
> particular, just by reading the code, I have no clue about how this is
> going to be used, how it is going to solve issues with indirect
> buffers, with invisibility stacking, etc.
>
> I don't mind if there are more comment lines than lines of code in
> that area.

Done.

> I don't think there is a need for `remove-text-properties' in every
> case. Also, (org--get-buffer-local-invisible-property-symbol spec)
> should be factored out. 

Done.

>> +(defun org-after-change-function (from to len)
>
> This is a terrible name. Org may add different functions in a-c-f,
> they cannot all be called like this. Assuming the "org-fold" prefix,
> it could be:
>
>   org-fold--fix-folded-region

Changed as you suggested.

> Nitpick: please do not skip lines amidst a function. Empty lines are
> used to separate functions, so this is distracting. 
>
> If a part of the function should stand out, a comment explaining what
> the part is doing is enough.

Done. Though many docstrings in org have empty lines creating the same
problem. 

> This part should first check if we're really after an insertion, e.g.,
> if FROM is different from TO, and exit early if that's not the case.

Done.

>> +   (if (< to from)
>> +       (let ((tmp from))
>> +	 (setq from to)
>> +         (setq to tmp)))
>
> I'm surprised you need to do that. Did you encounter a case where
> a-c-f was called with boundaries in reverse order?

I removed it and saw no issues. You are right, it does not seem to
happen at all.

> (forward-line) (point)  ---> (line-beginning-position 2)
> (forward-line -1) (point)  ---> (line-beginning-position 0)

Done.

> Anyway, I have the feeling this is not a good idea to extend it now,
> without first checking that we are in a folded drawer or block. It may
> also catch unwanted parts, e.g., a folded drawer ending on the line
> above.

This code is specifically written for cases when we are outside folded
text, but the edit can still affect folded text right before/after the
edited region.

Consider two examples:

1. We can change the first visible line of a folded drawer

:DRAWER:<begin fold>
text inside folded drawer
:END:<end-fold>

<deleted first : in drawer header>
----

DRAWER:<begin fold, which should be unfolded>
text inside folded drawer
:END:<end-fold>

The edited text was not folded, but must affected the following drawer.

2. We modify :END: of a folded drawer

:DRAWER:<begin fold>
text inside folded drawer
:END:<end-fold>

<deleted : in :END:>
---

:DRAWER:<begin fold>
text inside folded drawer
:END<end-fold><changed text region is after the fold>

Again, the effected region is not folded, anymore, but it should affect
the preceding drawer.

> What about first finding the whole region with property
>
>   (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer)
>
> then extending the initial part to include the drawer opening? I don't
> think we need to extend past the ending part, because drawer closing
> line is always included in the invisible part of the drawer.

As I just showed, we may not really have any folded text in the modified
region and thus cannot know if we need to update nearby drawers without
looking at them. This code allow handling the described cases and also
correctly keep folded drawers folded if they were not really modified.

>> +	 (let (unfold?)
>> +           ;; the line before folded text should be beginning of the drawer
>> +           (save-excursion
>> +             (goto-char drawer-begin)
>> +             (backward-char)
>
> Why `backward-char'?

=drawer-begin= is pointing to the beginning of folded part of the
drawer, so we need to move the line containing the :drawer:

>    looking-at-p ---> looking-at
>
> However, you must wrap this function within `save-match-data'.

Is there any particular reason to use looking-at in favour of
looking-at-p? I have seen looking-at-p many times in org-mode code.

> In the phase above, you need to bail out as soon as unfold? is non-nil:
>
>  (catch :exit
>   ...
>   (throw :exit (setq unfold? t))
>   ...)
>
> Also last two checks should be lumped together, with an appropriate
> regexp.
>
> Finally, I have the feeling we're missing out some early exits when
> nothing is folded around point (e.g., most of the case).

Done.

> Also `org-dblock-start-re' and `org-dblock-end-re' are not regexps we
> want here. The correct regexps would be:
>
>   (rx bol
>       (zero-or-more (any " " "\t"))
>       "#+begin"
>       (or ":" 
>           (seq "_" 
>               (group (one-or-more (not (syntax whitespace)))))))
>
> and closing line should match match-group 1 from the regexp above, e.g.:
>
>   (concat (rx bol (zero-or-more (any " " "\t")) "#+end")
>           (if block-type
>               (concat "_"
>                       (regexp-quote block-type)
>                       (rx (zero-or-more (any " " "\t")) eol))
>             (rx (opt ":") (zero-or-more (any " " "\t")) eol)))
>
> assuming `block-type' is the type of the block, or nil, i.e.,
> (match-string 1) in the previous regexp.

Fixed.

> 'outline --> `outline

Could you explain why?

> Does this move to the beginning of the widest invisible part around
> point? If that's not the case, we need a function in "org-fold.el"
> doing just that. Or we need to nest `while' loops as it was the case
> in the code you reverted.

See org-fold-next-visibility-change.

Best,
Ihor

[1] Full patch: https://gist.github.com/yantar92/6447754415457927293acda43a7fcaef
    org-fold.el: https://gist.github.com/yantar92/ffc1fc11550c58dae71de06700e7e4c1
    org-cycle.el: https://gist.github.com/yantar92/2be75c0e11968c0bbacc0d22dbca97fd
[2] https://lists.gnu.org/archive/html/emacs-devel/2020-07/msg00679.html



Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Ihor Radchenko <yantar92@gmail.com> writes:
>
>> [The patch itself will be provided in the following email]
>
> Thank you! I'll first make some generic remarks, then comment the diff
> in more details.
>
>> I have four more updates from the previous version of the patch:
>>
>> 1. All the code handling modifications in folded drawers/blocks is moved
>>    to after-change-function. It works as follows:
>>    - if any text is inserted in the middle of hidden region, that text
>>      is also hidden;
>>    - if BEGIN/END line of a folded drawer do not match org-drawer-regexp
>>      and org-property-end-re, unfold it; 
>>    - if org-property-end-re or new org-outline-regexp-bol is inserted in
>>      the middle of the drawer, unfold it;
>>    - the same logic for blocks.
>
> This sounds good, barring a minor error in the regexp for blocks, and
> missing optimizations. More on this in the detailed comments.
>
>> 2. The text property stack is rewritten using char-property-alias-alist.
>>    This is faster in comparison with previous approach, which involved
>>    modifying all the text properties every timer org-flag-region was
>>    called. 
>
> I'll need information about this, as I'm not sure to fully understand
> all the consequences of this. But more importantly, this needs to be
> copiously documented somewhere for future hackers.
>
>> 3. org-toggle-custom-properties-visibility is rewritten using text
>>    properties. I also took a freedom to implement a new feature here.
>>    Now, setting new `org-custom-properties-hide-emptied-drawers' to
>>    non-nil will result in hiding the whole property drawer if it
>>    contains only org-custom-properties.
>
> I don't think this is a good idea. AFAIR, we always refused to hide
> completely anything, including empty drawers. The reason is that if the
> drawer is completely hidden, you cannot expand it easily, or even know
> there is one.
>
> In any case, this change shouldn't belong to this patch set, and should
> be discussed separately.
>
>> 4. This patch should work against 1aa095ccf. However, the merge was not
>>    trivial here. Recent commits actively used the fact that drawers and
>>    outlines are hidden via 'outline invisibility spec, which is not the
>>    case in this branch. I am not confident that I did not break anything
>>    during the merge, especially 1aa095ccf.
>
> [...]
>
>> Also, I have seen some optimisations making use of the fact that drawers
>> and headlines both use 'outline invisibility spec. This change in the
>> implementation details supposed to improve performance and should not be
>> necessary if this patch is going to be merged. Would it be possible to
>> refrain from abusing this particular implementation detail in the
>> nearest commits on master (unless really necessary)?
>
> To be clear, I didn't intend to make your life miserable.
>
> However, I had to fix regression on drawers visibility before Org 9.4
> release. Also, merging invisibility properties for drawers and outline
> was easier for me. So, I had the opportunity to kill two birds with one
> stone. 
>
> As a reminder, Org 9.4 is about to be released, but Org 9.5 will take
> months to go out. So, even though I hope your changes will land into
> Org, there is no reason for us to refrain from improving (actually
> fixing a regression in) 9.4 release. Hopefully, once 9.4 is out, such
> changes are not expected to happen anymore.
>
> I hope you understand.
>
>> I would like to finalise the current patch and work on other code using
>> overlays separately. This patch is already quite complicated as is. I do
>> not want to introduce even more potential bugs by working on things not
>> directly affected by this version of the patch.
>
> The patch is technically mostly good, but needs more work for
> integration into Org.
>
> First, it includes a few unrelated changes that should be removed (e.g.,
> white space fixes in unrelated parts of the code). Also, as written
> above, the changes about `org-custom-properties-hide-emptied-drawers'
> should be removed for the time being.
>
> Once done, I think we should move (or copy, first) _all_ folding-related
> functions into a new "org-fold.el" library. Functions and variables
> included there should have a proper "org-fold-" prefix. More on this in
> the detailed report.
>
> The functions `org-find-text-property-region',
> `org-add-to-list-text-property', and
> `org-remove-from-list-text-property' can be left in "org-macs.el", since
> they do not directly depend on the `invisible' property. Note the last
> two functions I mentioned are not used throughout your patch. They might
> be removed.
>
> This first patch can coexist with overlay folding since functions in
> both mechanisms are named differently.
>
> Then, another patch can integrate "org-fold.el" into Org folding. I also
> suggest to move the Outline -> Org transition to yet another patch.
> I think there's more work to do on this part.
>
> Now, into the details of your patch. The first remarks are:
>
> 1. we still support Emacs 24.4 (and probably Emacs 24.3, but I'm not
>    sure), so some functions cannot be used.
>
> 2. we don't use "subr-x.el" in the code base. In particular, it would be
>    nice to replace `when-let' with `when' + `let'. This change costs
>    only one loc.
>
> 3. Some docstrings need more work. In particular, Emacs documentation
>    expects all arguments to be explained in the docstring, if possible
>    in the order in which they appear. There are exceptions, though. For
>    example, in a function like `org-remove-text-properties', you can
>    mention arguments are simply the same as in `remove-text-properties'.
>
> 4. Some refactorization is needed in some places. I mentioned them in
>    the report below.
>
> 5. I didn't dive much into the Isearch code so far. I tested it a bit
>    and seems to work nicely. I noticed one bug though. In the following
>    document:
>
>        #+begin: foo
>        :FOO:
>        bar
>        :END:
>        #+end
>        bar
>
>    when both the drawer and the block are folded (i.e., you fold the
>    drawer first, then the block), searching for "bar" first find the
>    last one, then overwraps and find the first one.
>
> 6. Since we're rewriting folding code, we might as well rename folding
>    properties: org-hide-drawer -> org-fold-drawer, outline ->
>    org-fold-headline…
>
> Now, here are more comments about the code.
>
> -----
>
>> +(defun org-remove-text-properties (start end properties &optional object)
>
> IMO, this generic name doesn't match the specialized nature of the
> function. It doesn't belong to "org-macs.el", but to the new "Org Fold" library.
>
>> +  "Remove text properties as in `remove-text-properties', but keep 'invisibility specs for folded regions.
>
> Line is too long. Suggestion:
>
>    Remove text properties except folding-related ones.
>
>> +Do not remove invisible text properties specified by 'outline,
>> +'org-hide-block, and 'org-hide-drawer (but remove i.e. 'org-link) this
>> +is needed to keep outlines, drawers, and blocks hidden unless they are
>> +toggled by user.
>
> Said properties should be moved into a defconst, e.g.,
> `org-fold-properties', then:
>
>   Remove text properties as in `remove-text-properties'.  See the
>   function for the description of the arguments.
>
>   However, do not remove invisible text properties defined in
>   `org-fold-properties'. Those are required to keep headlines, drawers
>   and blocks folded.
>
>> +Note: The below may be too specific and create troubles if more
>> +invisibility specs are added to org in future"
>
> You can remove the note. If you think the note is important, it should
> put a comment in the code instead.
>
>> +  (when (plist-member properties 'invisible)
>> +    (let ((pos start)
>> +	  next spec)
>> +      (while (< pos end)
>> +	(setq next (next-single-property-change pos 'invisible nil end)
>> +              spec (get-text-property pos 'invisible))
>> +	(unless (memq spec (list 'org-hide-block
>> +				 'org-hide-drawer
>> +				 'outline))
>
> The (list ...) should be moved outside the `while' loop. Better, this
> should be a constant defined somewhere. I also suggest to move
> `outline' to `org-outline' since we differ from Outline mode.
>
>> +          (remove-text-properties pos next '(invisible nil) object))
>> +	(setq pos next))))
>> +  (when-let ((properties-stripped (org-plist-delete properties 'invisible)))
>
> Typo here. There should a single pair of parenthesis, but see above
> about `when-let'.
>
>> +    (remove-text-properties start end properties-stripped object)))
>> +
>> +(defun org--find-text-property-region (pos prop)
>
> I think this is a function useful enough to have a name without double
> dashes. It can be left in "org-macs.el". It would be nice to have
> a wrapper for `invisible' property in "org-fold.el", tho.
>
>> +  "Find a region containing PROP text property around point POS."
>
> Reverse the order of arguments in the docstring:
>
>   Find a region around POS containing PROP text property.
>
>> +  (let* ((beg (and (get-text-property pos prop) pos))
>> +	 (end beg))
>> +    (when beg
>
> BEG can only be nil if arguments are wrong. In this case, you can
> throw an error (assuming this is no longer an internal function):
>
>   (unless beg (user-error "..."))
>
>> +      ;; when beg is the first point in the region, `previous-single-property-change'
>> +      ;; will return nil.
>
> when -> When
>
>> +      (setq beg (or (previous-single-property-change pos prop)
>> +		    beg))
>> +      ;; when end is the last point in the region, `next-single-property-change'
>> +      ;; will return nil.
>
> Ditto.
>
>> +      (setq end (or (next-single-property-change pos prop)
>> +		    end))
>> +      (unless (= beg end) ; this should not happen
>
> I assume this will be the case in an empty buffer. Anyway, (1 . 1)
> sounds more regular than a nil return value, not specified in the
> docstring. IOW, I suggest to remove this check.
>
>> +        (cons beg end)))))
>> +
>> +(defun org--add-to-list-text-property (from to prop element)
>> +  "Add element to text property PROP, whos value should be a list."
>
> The docstring is incomplete. All arguments need to be described. Also,
> I suggest:
>
>   Append ELEMENT to the value of text property PROP.
>
>> +  (add-text-properties from to `(,prop ,(list element))) ; create if none
>
> Here, you are resetting all the properties before adding anything,
> aren't you? IOW, there might be a bug there.
>
>> +  ;; add to existing
>> +  (alter-text-property from to
>> +		       prop
>> +		       (lambda (val)
>> +			 (if (member element val)
>> +                             val
>> +			   (cons element val)))))
>
>> +(defun org--remove-from-list-text-property (from to prop element)
>> +  "Remove ELEMENT from text propery PROP, whos value should be a list."
>
> The docstring needs to be improved.
>
>> +  (let ((pos from))
>> +    (while (< pos to)
>> +      (when-let ((val (get-text-property pos prop)))
>> +	(if (equal val (list element))
>
> (list element) needs to be moved out of the `while' loop.
>
>> +	    (remove-text-properties pos (next-single-char-property-change pos prop nil to) (list prop nil))
>> +	  (put-text-property pos (next-single-char-property-change pos prop nil to)
>> +			     prop (remove element (get-text-property pos prop)))))
>
> If we specialize the function, `remove' -> `remq'
>
>> +      (setq pos (next-single-char-property-change pos prop nil to)))))
>
> Please factor out `next-single-char-property-change'.
>
> Note that `org--remove-from-list-text-property' and
> `org--add-to-list-text-property' do not seem to be used throughout
> your patch.
>
>> +(defvar org--invisible-spec-priority-list '(outline org-hide-drawer org-hide-block)
>> +  "Priority of invisibility specs.")
>
> This should be the constant I wrote about earlier. Note that those are
> not "specs", just properties. I suggest to rename it.
>
>> +(defun org--get-buffer-local-invisible-property-symbol (spec &optional buffer return-only)
>
> This name is waaaaaaay too long.
>
>> +  "Return unique symbol suitable to be used as buffer-local in BUFFER for 'invisible SPEC.
>
> Maybe:
>
>
>   Return a unique symbol suitable for `invisible' property.
>
> Then:
>
>   Return value is meant to be used as a buffer-local variable in
>   current buffer, or BUFFER if this is non-nil.
>
>> +If the buffer already have buffer-local setup in `char-property-alias-alist'
>> +and the setup appears to be created for different buffer,
>> +copy the old invisibility state into new buffer-local text properties,
>> +unless RETURN-ONLY is non-nil."
>> +  (if (not (member spec org--invisible-spec-priority-list))
>> +      (user-error "%s should be a valid invisibility spec" spec)
>
> No need to waste an indentation level for that:
>
>   (unless (member …)
>    (user-error "%S should be …" spec))
>
> Also, this is a property, not a "spec".
>
>> +    (let* ((buf (or buffer (current-buffer))))
>> +      (let ((local-prop (intern (format "org--invisible-%s-buffer-local-%S"
>
> This clearly needs a shorter name. In particular, "buffer-local" can be removed.
>
>> +					(symbol-name spec)
>> +					;; (sxhash buf) appears to be not constant over time.
>> +					;; Using buffer-name is safe, since the only place where
>> +					;; buffer-local text property actually matters is an indirect
>> +					;; buffer, where the name cannot be same anyway.
>> +					(sxhash (buffer-name buf))))))
>
>
>> +        (prog1
>> +            local-prop
>
> Please move LOCAL-PROP after the (unless return-only ...) sexp.
>
>> +          (unless return-only
>> +	    (with-current-buffer buf
>> +	      (unless (member local-prop (alist-get 'invisible char-property-alias-alist))
>> +		;; copy old property
>
> "Copy old property."
>
>> +		(dolist (old-prop (alist-get 'invisible char-property-alias-alist))
>
> We cannot use `alist-get', which was added in Emacs 25.3 only.
>
>> +		  (org-with-wide-buffer
>> +		   (let* ((pos (point-min))
>> +			  (spec (seq-find (lambda (spec)
>> +					    (string-match-p (symbol-name spec)
>> +							    (symbol-name old-prop)))
>> +					  org--invisible-spec-priority-list))
>
> Likewise, we cannot use `seq-find'.
>
>> +			  (new-prop (org--get-buffer-local-invisible-property-symbol spec nil 'return-only)))
>> +		     (while (< pos (point-max))
>> +		       (when-let (val (get-text-property pos old-prop))
>> +			 (put-text-property pos (next-single-char-property-change pos old-prop) new-prop val))
>> +		       (setq pos (next-single-char-property-change pos old-prop))))))
>> +		(setq-local char-property-alias-alist
>> +			    (cons (cons 'invisible
>> +					(mapcar (lambda (spec)
>> +						  (org--get-buffer-local-invisible-property-symbol spec nil 'return-only))
>> +						org--invisible-spec-priority-list))
>> +				  (remove (assq 'invisible char-property-alias-alist)
>> +					  char-property-alias-alist)))))))))))
>
> This begs for explainations in the docstring or as comments. In
> particular, just by reading the code, I have no clue about how this is
> going to be used, how it is going to solve issues with indirect
> buffers, with invisibility stacking, etc.
>
> I don't mind if there are more comment lines than lines of code in
> that area.
>
>> -  (remove-overlays from to 'invisible spec)
>> -  ;; Use `front-advance' since text right before to the beginning of
>> -  ;; the overlay belongs to the visible line than to the contents.
>> -  (when flag
>> -    (let ((o (make-overlay from to nil 'front-advance)))
>> -      (overlay-put o 'evaporate t)
>> -      (overlay-put o 'invisible spec)
>> -      (overlay-put o 'isearch-open-invisible #'delete-overlay))))
>> -
>> +  (with-silent-modifications
>> +    (remove-text-properties from to (list (org--get-buffer-local-invisible-property-symbol spec) nil))
>> +    (when flag
>> +      (put-text-property from to (org--get-buffer-local-invisible-property-symbol spec) spec))))
>
> I don't think there is a need for `remove-text-properties' in every
> case. Also, (org--get-buffer-local-invisible-property-symbol spec)
> should be factored out. 
>
> I suggest:
>
>   (with-silent-modifications
>     (let ((property (org--get-buffer-local-invisible-property-symbol spec)))
>       (if flag
>           (put-text-property from to property spec)
>         (remove-text-properties from to (list property nil)))))
>
>> +(defun org-after-change-function (from to len)
>
> This is a terrible name. Org may add different functions in a-c-f,
> they cannot all be called like this. Assuming the "org-fold" prefix,
> it could be:
>
>   org-fold--fix-folded-region
>
>> +  "Process changes in folded elements.
>> +If a text was inserted into invisible region, hide the inserted text.
>> +If the beginning/end line of a folded drawer/block was changed, unfold it.
>> +If a valid end line was inserted in the middle of the folded drawer/block, unfold it."
>
> Nitpick: please do not skip lines amidst a function. Empty lines are
> used to separate functions, so this is distracting. 
>
> If a part of the function should stand out, a comment explaining what
> the part is doing is enough.
>
>> +  ;; re-hide text inserted in the middle of a folded region
>
>   Re-hide … folded region.
>
>> +  (dolist (spec org--invisible-spec-priority-list)
>> +    (when-let ((spec-to (get-text-property to (org--get-buffer-local-invisible-property-symbol spec)))
>> +	       (spec-from (get-text-property (max (point-min) (1- from)) (org--get-buffer-local-invisible-property-symbol spec))))
>> +      (when (eq spec-to spec-from)
>> +	(org-flag-region from to 't spec-to))))
>
> This part should first check if we're really after an insertion, e.g.,
> if FROM is different from TO, and exit early if that's not the case.
>
> Also, no need to quote t.
>
>> +  ;; Process all the folded text between `from' and `to'
>> +  (org-with-wide-buffer
>> +
>> +   (if (< to from)
>> +       (let ((tmp from))
>> +	 (setq from to)
>> +         (setq to tmp)))
>
> I'm surprised you need to do that. Did you encounter a case where
> a-c-f was called with boundaries in reverse order?
>
>> +   ;; Include next/previous line into the changed region.
>> +   ;; This is needed to catch edits in beginning line of a folded
>> +   ;; element.
>> +   (setq to (save-excursion (goto-char to) (forward-line) (point)))
>
> (forward-line) (point)  ---> (line-beginning-position 2)
>
>> +   (setq from (save-excursion (goto-char from) (forward-line -1) (point)))
>
> (forward-line -1) (point)  ---> (line-beginning-position 0)
>
> Anyway, I have the feeling this is not a good idea to extend it now,
> without first checking that we are in a folded drawer or block. It may
> also catch unwanted parts, e.g., a folded drawer ending on the line
> above.
>
> What about first finding the whole region with property
>
>   (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer)
>
> then extending the initial part to include the drawer opening? I don't
> think we need to extend past the ending part, because drawer closing
> line is always included in the invisible part of the drawer.
>
>> +   ;; Expand the considered region to include partially present folded
>> +   ;; drawer/block.
>> +   (when (get-text-property from (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))
>> +     (setq from (previous-single-char-property-change from (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))))
>> +   (when (get-text-property from (org--get-buffer-local-invisible-property-symbol 'org-hide-block))
>> +     (setq from (previous-single-char-property-change from (org--get-buffer-local-invisible-property-symbol 'org-hide-block))))
>
> Please factor out (org--get-buffer-local-invisible-property-symbol
> XXX), this is difficult to read.
>
>> +   ;; check folded drawers
>
>   Check folded drawers.
>
>> +   (let ((pos from))
>> +     (unless (get-text-property pos (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))
>> +       (setq pos (next-single-char-property-change pos
>> +						   (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))))
>> +     (while (< pos to)
>> +       (when-let ((drawer-begin (and (get-text-property pos (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))
>> +				     pos))
>> +		  (drawer-end (next-single-char-property-change pos (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer))))
>> +
>> +	 (let (unfold?)
>> +           ;; the line before folded text should be beginning of the drawer
>> +           (save-excursion
>> +             (goto-char drawer-begin)
>> +             (backward-char)
>
> Why `backward-char'?
>
>> +             (beginning-of-line)
>> +             (unless (looking-at-p org-drawer-regexp)
>
>    looking-at-p ---> looking-at
>
> However, you must wrap this function within `save-match-data'.
>
>> +	       (setq unfold? t)))
>> +           ;; the last line of the folded text should be :END:
>> +           (save-excursion
>> +             (goto-char drawer-end)
>> +             (beginning-of-line)
>> +             (unless (let ((case-fold-search t)) (looking-at-p org-property-end-re))
>> +	       (setq unfold? t)))
>> +           ;; there should be no :END: anywhere in the drawer body
>> +           (save-excursion
>> +             (goto-char drawer-begin)
>> +             (when (save-excursion
>> +		     (let ((case-fold-search t))
>> +		       (re-search-forward org-property-end-re
>> +					  (max (point)
>> +					       (1- (save-excursion
>> +						     (goto-char drawer-end)
>> +                                                     (line-beginning-position))))
>> +                                          't)))
>
>>  (max (point) 
>>       (save-excursion (goto-char drawer-end) (line-end-position 0))
>
>> +	       (setq unfold? t)))
>> +           ;; there should be no new entry anywhere in the drawer body
>> +           (save-excursion
>> +             (goto-char drawer-begin)
>> +             (when (save-excursion
>> +		     (let ((case-fold-search t))
>> +		       (re-search-forward org-outline-regexp-bol
>> +					  (max (point)
>> +					       (1- (save-excursion
>> +						     (goto-char drawer-end)
>> +                                                     (line-beginning-position))))
>> +                                          't)))
>> +	       (setq unfold? t)))
>
> In the phase above, you need to bail out as soon as unfold? is non-nil:
>
>  (catch :exit
>   ...
>   (throw :exit (setq unfold? t))
>   ...)
>
> Also last two checks should be lumped together, with an appropriate
> regexp.
>
> Finally, I have the feeling we're missing out some early exits when
> nothing is folded around point (e.g., most of the case).
>
>> +
>> +           (when unfold? (org-flag-region drawer-begin drawer-end nil 'org-hide-drawer))))
>> +       
>> +       (setq pos (next-single-char-property-change pos (org--get-buffer-local-invisible-property-symbol 'org-hide-drawer)))))
>> +
>> +   ;; check folded blocks
>> +   (let ((pos from))
>> +     (unless (get-text-property pos (org--get-buffer-local-invisible-property-symbol 'org-hide-block))
>> +       (setq pos (next-single-char-property-change pos
>> +						   (org--get-buffer-local-invisible-property-symbol 'org-hide-block))))
>> +     (while (< pos to)
>> +       (when-let ((block-begin (and (get-text-property pos (org--get-buffer-local-invisible-property-symbol 'org-hide-block))
>> +				    pos))
>> +		  (block-end (next-single-char-property-change pos (org--get-buffer-local-invisible-property-symbol 'org-hide-block))))
>> +
>> +	 (let (unfold?)
>> +           ;; the line before folded text should be beginning of the block
>> +           (save-excursion
>> +             (goto-char block-begin)
>> +             (backward-char)
>> +             (beginning-of-line)
>> +             (unless (looking-at-p org-dblock-start-re)
>> +	       (setq unfold? t)))
>> +           ;; the last line of the folded text should be end of the block
>> +           (save-excursion
>> +             (goto-char block-end)
>> +             (beginning-of-line)
>> +             (unless (looking-at-p org-dblock-end-re)
>> +	       (setq unfold? t)))
>> +           ;; there should be no #+end anywhere in the block body
>> +           (save-excursion
>> +             (goto-char block-begin)
>> +             (when (save-excursion
>> +		     (re-search-forward org-dblock-end-re
>> +					(max (point)
>> +					     (1- (save-excursion
>> +						   (goto-char block-end)
>> +						   (line-beginning-position))))
>> +                                        't))
>> +	       (setq unfold? t)))
>> +           ;; there should be no new entry anywhere in the block body
>> +           (save-excursion
>> +             (goto-char block-begin)
>> +             (when (save-excursion
>> +		     (let ((case-fold-search t))
>> +		       (re-search-forward org-outline-regexp-bol
>> +					  (max (point)
>> +					       (1- (save-excursion
>> +						     (goto-char block-end)
>> +                                                     (line-beginning-position))))
>> +                                          't)))
>> +	       (setq unfold? t)))
>> +
>> +           (when unfold? (org-flag-region block-begin block-end nil 'org-hide-block))))
>> +       
>> +       (setq pos
>> +	     (next-single-char-property-change pos
>> +					       (org--get-buffer-local-invisible-property-symbol 'org-hide-block)))))))
>
> See remarks above. The parts related to drawers and blocks are so
> similar they should be factorized out.
>
> Also `org-dblock-start-re' and `org-dblock-end-re' are not regexps we
> want here. The correct regexps would be:
>
>   (rx bol
>       (zero-or-more (any " " "\t"))
>       "#+begin"
>       (or ":" 
>           (seq "_" 
>               (group (one-or-more (not (syntax whitespace)))))))
>
> and closing line should match match-group 1 from the regexp above, e.g.:
>
>   (concat (rx bol (zero-or-more (any " " "\t")) "#+end")
>           (if block-type
>               (concat "_"
>                       (regexp-quote block-type)
>                       (rx (zero-or-more (any " " "\t")) eol))
>             (rx (opt ":") (zero-or-more (any " " "\t")) eol)))
>
> assuming `block-type' is the type of the block, or nil, i.e.,
> (match-string 1) in the previous regexp.
>
>> -	  (pcase (get-char-property-and-overlay (point) 'invisible)
>> +	  (pcase (get-char-property (point) 'invisible)
>> 	    ;; Do not fold already folded drawers.
>> -	    (`(outline . ,o) (goto-char (overlay-end o)))
>> +	    ('outline
>
> 'outline --> `outline
>  
>>      (end-of-line))
>>    (while (and (< arg 0) (re-search-backward regexp nil :move))
>>      (unless (bobp)
>> -	(while (pcase (get-char-property-and-overlay (point) 'invisible)
>> -		 (`(outline . ,o)
>> -		  (goto-char (overlay-start o))
>> -		  (re-search-backward regexp nil :move))
>> -		 (_ nil))))
>> +	(pcase (get-char-property (point) 'invisible)
>> +	  ('outline
>> +	   (goto-char (car (org--find-text-property-region (point) 'invisible)))
>> +	   (beginning-of-line))
>> +	  (_ nil)))
>
> Does this move to the beginning of the widest invisible part around
> point? If that's not the case, we need a function in "org-fold.el"
> doing just that. Or we need to nest `while' loops as it was the case
> in the code you reverted.
>
> -----
>
> Regards,
>
> -- 
> Nicolas Goaziou