From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id cP1sNDQv716RVQAA0tVLHw (envelope-from ) for ; Sun, 21 Jun 2020 09:58:12 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id GEpKMDQv717XCgAA1q6Kng (envelope-from ) for ; Sun, 21 Jun 2020 09:58:12 +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 127C3940276 for ; Sun, 21 Jun 2020 09:58:12 +0000 (UTC) Received: from localhost ([::1]:37802 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jmwjq-0007wT-C0 for larch@yhetil.org; Sun, 21 Jun 2020 05:58:10 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:52362) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jmwjI-0007uS-TM for emacs-orgmode@gnu.org; Sun, 21 Jun 2020 05:57:36 -0400 Received: from mail-pl1-x635.google.com ([2607:f8b0:4864:20::635]:43996) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jmwjF-00050F-PM for emacs-orgmode@gnu.org; Sun, 21 Jun 2020 05:57:36 -0400 Received: by mail-pl1-x635.google.com with SMTP id g12so6172939pll.10 for ; Sun, 21 Jun 2020 02:57:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-transfer-encoding; bh=9x8wpZlNGQD5LsY5xOqhIpRNsC0doUDxXn6RGa1gIac=; b=VjOb7Gyz6QK41YYZf9+IqLi+fLXqZQayMYuIN84GlrbqYZUC2iulqU2E7dZNO6NiwG yHvgTVJaMOb5Z+6OIBSTAPIEWlvhycUDRyhwKj2XVjRHkLD0fWuVCZNlW9lbuUcvDHpr 6+GnQpJUfag/Pa/OnB1QSe1Sh3GqdYBAxd7RIpY2QxmLlJf1cvrCTJvhfXs5HECJ0+tj AOksqEkEMbbns8sgZ+p/Mwu+EAA6Hyx3B7x6g8HwcVpEtZPvaMzKXmIGqTgTYcwrWoEX JcMSmPxPvZ1ydz2CA2zt3nl/CApRnk/kwuoS34rzOZcNHnUiATZHwt3uCOJF8J5MbD8R fuXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version:content-transfer-encoding; bh=9x8wpZlNGQD5LsY5xOqhIpRNsC0doUDxXn6RGa1gIac=; b=DDbyVrn/AG9esW3Gj4E0V2QmsMiZw2uWIz1+9PEVVAl3uqsM3UCm5epy5GznRiYGuN 7LUXoMappM0sQl79NHmsL0p5ULFSg0Wme5/vm8RTtq3FrxChR4L2k2E/GfV91VzfDU6w wsc0aVTe2ZLL7d72EVHnmZtuiUYUJxsRTXSVToS8QOZ6iiDI/FBESzhrv1nSGb6JLceX TGmXL82UZhiqxD2AMnqk+ZI9bUZERPU0aa7xWe3XZm9LfwP6Mha+JuL2OydH1M6+3Ywq kuzZ9wDa7k71TQnx34DsuLEgyHEJbqPgzjlmHhQ9uwzVkg2+eKgQw1icNixFiP+4B83B +L9w== X-Gm-Message-State: AOAM532SvE9bmiL4N5V3wlbWpwCNt4SkEppb4/P+9+sSuKwdLHPLG4d+ VdFzX1Z+ckSBBO5aHO43Lx4hvYAA330= X-Google-Smtp-Source: ABdhPJytyFIQgnVLKVpb7iOkZtlTwdgJFgehXa7z5Z35JfEQ1N3w89tsHtNnT0qQTd/Spt4WkmpTww== X-Received: by 2002:a17:90b:1495:: with SMTP id js21mr12594069pjb.48.1592733451200; Sun, 21 Jun 2020 02:57:31 -0700 (PDT) Received: from localhost ([101.99.64.65]) by smtp.gmail.com with ESMTPSA id m8sm9910469pjk.20.2020.06.21.02.57.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 21 Jun 2020 02:57:30 -0700 (PDT) From: Ihor Radchenko To: Nicolas Goaziou Subject: Re: [patch suggestion] Mitigating the poor Emacs performance on huge org files: Do not use overlays for PROPERTY and LOGBOOK drawers In-Reply-To: <87wo4en8qk.fsf@nicolasgoaziou.fr> References: <87h7x9e5jo.fsf@localhost> <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> <87wo4en8qk.fsf@nicolasgoaziou.fr> Date: Sun, 21 Jun 2020 17:52:33 +0800 Message-ID: <87pn9s7nku.fsf@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=2607:f8b0:4864:20::635; envelope-from=yantar92@gmail.com; helo=mail-pl1-x635.google.com X-detected-operating-system: by eggs.gnu.org: No matching host in p0f cache. That's all we know. X-Spam_score_int: -17 X-Spam_score: -1.8 X-Spam_bar: - X-Spam_report: (-1.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_ENVFROM_END_DIGIT=0.25, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, 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=pass header.d=gmail.com header.s=20161025 header.b=VjOb7Gyz; dmarc=pass (policy=none) header.from=gmail.com; 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.21 X-TUID: CMu9DGpFRZHe > 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 writes: > 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 objec= t) > > 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" l= ibrary. > >> + "Remove text properties as in `remove-text-properties', but keep 'inv= isibility 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 'invisib= le))) > > 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-pr= operty-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-propert= y-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 no= ne > > 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 ni= l 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 f= or '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-a= list' >> +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 r= emoved. > >> + (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-a= lias-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 ni= l '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-pr= op) 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 'retu= rn-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-invisi= ble-property-symbol spec) nil)) >> + (when flag >> + (put-text-property from to (org--get-buffer-local-invisible-prope= rty-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 = it. >> +If a valid end line was inserted in the middle of the folded drawer/blo= ck, 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-in= visible-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-prope= rty-symbol 'org-hide-drawer)) >> + (setq from (previous-single-char-property-change from (org--get-bu= ffer-local-invisible-property-symbol 'org-hide-drawer)))) >> + (when (get-text-property from (org--get-buffer-local-invisible-prope= rty-symbol 'org-hide-block)) >> + (setq from (previous-single-char-property-change from (org--get-bu= ffer-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-pr= operty-symbol 'org-hide-drawer)) >> + (setq pos (next-single-char-property-change pos >> + (org--get-buffer-local-invisible-property-symbol 'org-hide-dra= wer)))) >> + (while (< pos to) >> + (when-let ((drawer-begin (and (get-text-property pos (org--get-b= uffer-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 dr= awer >> + (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-prop= erty-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-po= sition)))) >> + '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-po= sition)))) >> + '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)))) >> +=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-pr= operty-symbol 'org-hide-block)) >> + (setq pos (next-single-char-property-change pos >> + (org--get-buffer-local-invisible-property-symbol 'org-hide-blo= ck)))) >> + (while (< pos to) >> + (when-let ((block-begin (and (get-text-property pos (org--get-bu= ffer-local-invisible-property-symbol 'org-hide-block)) >> + pos)) >> + (block-end (next-single-char-property-change pos (org--get-buffer-l= ocal-invisible-property-symbol 'org-hide-block)))) >> + >> + (let (unfold?) >> + ;; the line before folded text should be beginning of the bl= ock >> + (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 blo= ck >> + (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-po= sition)))) >> + 't))) >> + (setq unfold? t))) >> + >> + (when unfold? (org-flag-region block-begin block-end nil 'or= g-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-= 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 ":"=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=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 --=20 Ihor Radchenko, PhD, Center for Advancing Materials Performance from the Nanoscale (CAMP-nano) State Key Laboratory for Mechanical Behavior of Materials, Xi'an Jiaotong U= niversity, Xi'an, China Email: yantar92@gmail.com, ihor_radchenko@alumni.sutd.edu.sg