From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp2 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id 8GPIAS4V4V50cQAA0tVLHw (envelope-from ) for ; Wed, 10 Jun 2020 17:15:26 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp2 with LMTPS id cIc4OS0V4V7PfAAAB5/wlQ (envelope-from ) for ; Wed, 10 Jun 2020 17:15:25 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 29B079403EE for ; Wed, 10 Jun 2020 17:15:24 +0000 (UTC) Received: from localhost ([::1]:38652 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jj4Ju-0005C3-N8 for larch@yhetil.org; Wed, 10 Jun 2020 13:15:22 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54850) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jj4JP-0005Bb-2q for emacs-orgmode@gnu.org; Wed, 10 Jun 2020 13:14:51 -0400 Received: from relay7-d.mail.gandi.net ([217.70.183.200]:59099) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jj4JM-0008Fv-98 for emacs-orgmode@gnu.org; Wed, 10 Jun 2020 13:14:50 -0400 X-Originating-IP: 185.131.40.67 Received: from localhost (40-67.ipv4.commingeshautdebit.fr [185.131.40.67]) (Authenticated sender: admin@nicolasgoaziou.fr) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 2301320002; Wed, 10 Jun 2020 17:14:43 +0000 (UTC) From: Nicolas Goaziou To: Ihor Radchenko Subject: Re: [patch suggestion] Mitigating the poor Emacs performance on huge org files: Do not use overlays for PROPERTY and LOGBOOK drawers References: <87h7x9e5jo.fsf@localhost> <87r1vu5qmc.fsf@nicolasgoaziou.fr> <87imh5w1zt.fsf@localhost> <87blmxjckl.fsf@localhost> <87y2q13tgs.fsf@nicolasgoaziou.fr> <878si1j83x.fsf@localhost> <87d07bzvhd.fsf@nicolasgoaziou.fr> <87imh34usq.fsf@localhost> <87pnbby49m.fsf@nicolasgoaziou.fr> <87tv0efvyd.fsf@localhost> <874kse1seu.fsf@localhost> <87r1vhqpja.fsf@nicolasgoaziou.fr> <87tv0d2nk7.fsf@localhost> <87o8qkhy3g.fsf@nicolasgoaziou.fr> <87sgfqu5av.fsf@localhost> <87sgfn6qpc.fsf@nicolasgoaziou.fr> <87367d4ydc.fsf@localhost> <87r1uuotw8.fsf@nicolasgoaziou.fr> <87mu5iq618.fsf@localhost> <87ftb9pqop.fsf@nicolasgoaziou.fr> <875zc2du63.fsf@localhost> Mail-Followup-To: Ihor Radchenko , emacs-orgmode@gnu.org Date: Wed, 10 Jun 2020 19:14:43 +0200 In-Reply-To: <875zc2du63.fsf@localhost> (Ihor Radchenko's message of "Mon, 08 Jun 2020 13:05:08 +0800") Message-ID: <87wo4en8qk.fsf@nicolasgoaziou.fr> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=217.70.183.200; envelope-from=mail@nicolasgoaziou.fr; helo=relay7-d.mail.gandi.net X-detected-operating-system: by eggs.gnu.org: First seen = 2020/06/10 13:14:44 X-ACL-Warn: Detected OS = Linux 3.11 and newer X-Spam_score_int: -35 X-Spam_score: -3.6 X-Spam_bar: --- X-Spam_report: (-3.6 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=_AUTOLEARN X-Spam_action: no action X-BeenThere: emacs-orgmode@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: emacs-orgmode@gnu.org Errors-To: emacs-orgmode-bounces+larch=yhetil.org@gnu.org Sender: "Emacs-orgmode" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of emacs-orgmode-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=emacs-orgmode-bounces@gnu.org X-Spam-Score: -1.01 X-TUID: pNlc5ERE25nV Hello, Ihor Radchenko 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;=20 > - 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.=20 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.=20 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=E2=80=A6 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" lib= rary. > + "Remove text properties as in `remove-text-properties', but keep 'invi= sibility 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 'invisibl= e))) 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-pro= perty-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 (=3D 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 p= rop 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 b= uffer return-only) This name is waaaaaaay too long. > + "Return unique symbol suitable to be used as buffer-local in BUFFER fo= r '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-al= ist' > +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 =E2=80=A6) (user-error "%S should be =E2=80=A6" 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 rem= oved. > + (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-al= ias-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-pro= p) 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 'retur= n-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-invisib= le-property-symbol spec) nil)) > + (when flag > + (put-text-property from to (org--get-buffer-local-invisible-proper= ty-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.=20 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 i= t. > +If a valid end line was inserted in the middle of the folded drawer/bloc= k, unfold it." Nitpick: please do not skip lines amidst a function. Empty lines are used to separate functions, so this is distracting.=20 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 =E2=80=A6 folded region. > + (dolist (spec org--invisible-spec-priority-list) > + (when-let ((spec-to (get-text-property to (org--get-buffer-local-inv= isible-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-proper= ty-symbol 'org-hide-drawer)) > + (setq from (previous-single-char-property-change from (org--get-buf= fer-local-invisible-property-symbol 'org-hide-drawer)))) > + (when (get-text-property from (org--get-buffer-local-invisible-proper= ty-symbol 'org-hide-block)) > + (setq from (previous-single-char-property-change from (org--get-buf= fer-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-pro= perty-symbol 'org-hide-drawer)) > + (setq pos (next-single-char-property-change pos > + (org--get-buffer-local-invisible-property-symbol 'org-hide-draw= er)))) > + (while (< pos to) > + (when-let ((drawer-begin (and (get-text-property pos (org--get-bu= ffer-local-invisible-property-symbol 'org-hide-drawer)) > + pos)) > + (drawer-end (next-single-char-property-change pos (org--get-buffer-l= ocal-invisible-property-symbol 'org-hide-drawer)))) > + > + (let (unfold?) > + ;; the line before folded text should be beginning of the dra= wer > + (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-prope= rty-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-pos= ition)))) > + 't))) > (max (point)=20 > (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-pos= ition)))) > + '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 'o= rg-hide-drawer)))) > +=20=20=20=20=20=20=20 > + (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-pro= perty-symbol 'org-hide-block)) > + (setq pos (next-single-char-property-change pos > + (org--get-buffer-local-invisible-property-symbol 'org-hide-bloc= k)))) > + (while (< pos to) > + (when-let ((block-begin (and (get-text-property pos (org--get-buf= fer-local-invisible-property-symbol 'org-hide-block)) > + pos)) > + (block-end (next-single-char-property-change pos (org--get-buffer-lo= cal-invisible-property-symbol 'org-hide-block)))) > + > + (let (unfold?) > + ;; the line before folded text should be beginning of the blo= ck > + (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-pos= ition)))) > + 't))) > + (setq unfold? t))) > + > + (when unfold? (org-flag-region block-begin block-end nil 'org= -hide-block)))) > +=20=20=20=20=20=20=20 > + (setq pos > + (next-single-char-property-change pos > + (org--get-buffer-local-invisible-property-symbol 'org-hide-b= lock))))))) 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 ":"=20 (seq "_"=20 (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 =20 > (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, --=20 Nicolas Goaziou