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

[Ihor Radchenko]

> 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 am currently working on org-fold.el. However, I am not sure if it is
acceptable to move some of the existing functions from org.el to
org-fold.el.

Specifically, functions from the following sections of org.el might be
moved to org-fold.el:
> ;;; Visibility (headlines, blocks, drawers)
> ;;;; Reveal point location
> ;;;; Visibility cycling

Should I do it?

Best,
Ihor

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

-- 
Ihor Radchenko,
PhD,
Center for Advancing Materials Performance from the Nanoscale (CAMP-nano)
State Key Laboratory for Mechanical Behavior of Materials, Xi'an Jiaotong 
University, Xi'an, China
Email: yantar92@gmail.com, ihor_radchenko@alumni.sutd.edu.sg