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