From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id QPItBPk+Ml8QWwAA0tVLHw (envelope-from ) for ; Tue, 11 Aug 2020 06:47:21 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id qJU8Ovg+Ml+NFwAAbx9fmQ (envelope-from ) for ; Tue, 11 Aug 2020 06:47:20 +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 19554940390 for ; Tue, 11 Aug 2020 06:47:20 +0000 (UTC) Received: from localhost ([::1]:55616 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1k5O45-0007bK-IQ for larch@yhetil.org; Tue, 11 Aug 2020 02:47:17 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:48574) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k5O3W-0007b5-3C for emacs-orgmode@gnu.org; Tue, 11 Aug 2020 02:46:42 -0400 Received: from mail-pl1-x62d.google.com ([2607:f8b0:4864:20::62d]:42246) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1k5O3S-0006jV-0N for emacs-orgmode@gnu.org; Tue, 11 Aug 2020 02:46:41 -0400 Received: by mail-pl1-x62d.google.com with SMTP id f5so2380385plr.9 for ; Mon, 10 Aug 2020 23:46:37 -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=33bzrd7wl8KlirCuBcqCfbllO3owzeFrqX+Sj/mRDas=; b=s0U1821ws6OHFoc710Z5td+tByG4sw1sSgss6mwQ4cdMIJcdzHmf3JuYUF2+EipYDr lEUz1JSWZKKQ8pnh2VMcpB8L9USXsw/bcVKc8nYTPra1eSrzcYgiRLoOda7rO+RT+k3x 4FvhY2Ew5FJUp7AAiG7QR+C8tm9pShXfbUm82NLsvJQrGJWWGny3ir3NOSysSdiKD+KG aRE0tIijWCbHQB2zJzsinG9WTKtbb0JMnMoK/AjGlvaFC3mOR7oINxpQ790LMX0VU3Km fQZka72Ds95MNS0kog1LXMLUjG8ixHa3BBnnmWbSRbAXRCT/b/QHz0/OPRDgvTp3AQY4 R6Bw== 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=33bzrd7wl8KlirCuBcqCfbllO3owzeFrqX+Sj/mRDas=; b=ZHZ545rT8a2gNWYrL1TQUgfNwUnO+L2xpRDm2XsilsCbn8NvLtvXeXu6+U8Ee0Yv3C ARFhS2m0a6G2Ny/5s1NC6vlCyzSpcdRoJqAmKqybxiQRgBqZ1iwGIOtzKIqitihFQ3Ot Ted2bN3RFwmI/DLWW7J91pbOJkhxKOHTbtkDQuAj+cOtTS3czkdMfjOdeVq1vFLNulWf E1MkE3oYYlG8QXOncQJvlP3On5bCbnRjyo1GOgmOZgJpkzTE/rQ9UBJjXPR8ZO/zlDyw Ve9s2qTd24CUwOrlJvnrr/x8xzcGpz1Xs4gXKShYVnokb0YAWGZZRASYGxqclZ3sfrBU Trag== X-Gm-Message-State: AOAM530aJ7qJiMX8lvQHOB9fV5s0lLoD/4tifA4isBRj/51R59/eT/ya NpMQbyGM//Tdh+KVj9bjGBms2ItniyA= X-Google-Smtp-Source: ABdhPJyw0DeGF8bKlAwqEHnvJq50Dwe7Od+fzmg9l4gboglEcAGE56hsRxdncCDYDtrkXduOq0OWKg== X-Received: by 2002:a17:90a:1f8c:: with SMTP id x12mr3182974pja.186.1597128394680; Mon, 10 Aug 2020 23:46:34 -0700 (PDT) Received: from localhost ([104.250.131.79]) by smtp.gmail.com with ESMTPSA id a16sm25337608pfr.45.2020.08.10.23.46.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Aug 2020 23:46:33 -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: Tue, 11 Aug 2020 14:45:45 +0800 Message-ID: <87mu31adeu.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::62d; envelope-from=yantar92@gmail.com; helo=mail-pl1-x62d.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=ham autolearn_force=no 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=s0U1821w; 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: 6W9wK0esQWOd Hello, [The patch itself will be provided in the following email or can be accesse= d via Github [1]] I have finally finished the suggested edits. Most importantly: - All the folding-related code lives in =3Dorg-fold.el=3D and =3Dorg-cycle.= el=3D now. - =3Dorg-fold.el=3D have commentary section explaining how folding works an= d exposing API for external code using folding. - I wrote a patch for =3Disearch.el=3D adding support searching inside text= hidden via text properties [2] and the relevant support of the patch in th= e =3Dorg-fold.el=3D. The current =3Disearch=3D 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 we= lcome. There are still known problems though. The patch currently breaks many org-= mode tests when running =3Dmake test=3D. It is partially because some tests= assume overlays to be used for folding and partially because the patch app= ears to break certain folding conventions. I am still investigating this (a= nd learning =3Dert=3D). 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.=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. See commentary section in =3Dorg-fold.el=3D and comments in =3Dorg-fold--property-symbol-get-create=3D. > 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 =3Dorg-fold.el=3D and =3Dorg-cycle.el=3D and move all t= he 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=E2=80=A6 Done. See =3Dorg-fold-get-folding-spec-for-element=3D. >> +(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. 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 =3Dbuffer-invisibility-spec=3D in Emacs. Internally, they are exactly used as members of =3Dbuffer-invisibility-spec=3D. >> +(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 =E2=80=A6) > (user-error "%S should be =E2=80=A6" 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 r= emoved. 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 =3Dprog1=3D is optional logic. Moving =3Dlocal-prop= =3D to the end may reduce readability. > We cannot use `alist-get', which was added in Emacs 25.3 only. Changed to =3Dassq=3D. > Likewise, we cannot use `seq-find'. Changed to =3Ddolist=3D. > 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.=20 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.=20 > > 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.=20 > 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: text inside folded drawer :END: ---- DRAWER: text inside folded drawer :END: The edited text was not folded, but must affected the following drawer. 2. We modify :END: of a folded drawer :DRAWER: text inside folded drawer :END: --- :DRAWER: text inside folded drawer :END 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 dr= awer >> + (save-excursion >> + (goto-char drawer-begin) >> + (backward-char) > > Why `backward-char'? =3Ddrawer-begin=3D 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 ":"=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. 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/6447754415457927293acda43a= 7fcaef org-fold.el: https://gist.github.com/yantar92/ffc1fc11550c58dae71de0670= 0e7e4c1 org-cycle.el: https://gist.github.com/yantar92/2be75c0e11968c0bbacc0d22= dbca97fd [2] https://lists.gnu.org/archive/html/emacs-devel/2020-07/msg00679.html 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