JV <[email protected]> writes: >> I have a handful of comments. > > I appreciate it. Replies are below, and a revised patch set is attached > after a good bit of performance testing.
Before we go too far, I not that you do not appear to have FSF copyright assignment. For non-trivial patches we need to have the paperwork with FSF. Would you consider completing it? See https://orgmode.org/worg/org-contribute.html#copyright > The revised patch set is very lightweight in the after-change function. > No parser; it just checks line beginnings for a single character or > org-item-re. Try opening the attached file and scrolling. The performance is severely degraded with your patch. >> It is OK to provide a group in regexp, but adding groups make the >> matching much slower. So, let's not modify the default regexp. Instead, >> I recommend adding an optional parameter to org-item-re that will make >> it return a version of regexp with groups. > > Makes sense. Done. I should note that default regexp also had one > superfluous group, which the attached patch removes. The updated tests > cover both the bullet capturing (7 groups) and default (previously 5, > now 4 group) regexps. We should not remove existing groups. Not because it is a good thing to have them, but because third-party code may rely on the current behavior. > For context, compared to the 5 group regexp: > - The 7 group one adds 2% to non-GC execution time and 13% to total time > including GC on a 1000000 line (27MB) test file. > - The 4 group one reduces total time by 1.4% on the same file. > > On a 100000 line (2.7MB) file, the relative gaps narrow substantially; > there is almost no measurable difference. > > I also couldn't find any measurable difference between capturing and > non-capturing regexp groups at any scale. This is simply a rule of thumb from the maintainer of Emacs regexp matcher. Groups may or may not matter depending on the file in question. But it is better to avoid them unless necessary. > Subject: [PATCH 2/4] org: Improve org-in-block-p > > * lisp/org.el (org-in-block-p): Replace regexp-based approach with > element API by renaming and generalizing existing function > org-in-src-block-p. This adds flexibility and is more reliable with Note that we `quote` Elisp symbols in the commit message and use double space between sentences. See https://orgmode.org/worg/org-contribute.html > +(defun org-in-block-p (&optional types inside element) > + "Return t when point is in a block element. > + > +Block TYPES may be constrained using a type symbol, a name string, > +or a list including either. Name strings are mapped to type symbols > +for defined block types and compared as a \\='special-block :type Just `special-bloc', not need to put '. > When ELEMENT is provided, it is considered to be element at point." > (save-match-data (setq element (or element (org-element-at-point)))) > - (when (org-element-type-p element 'src-block) > + (when-let* ((types (or (org--block-types types) > org-element-block-elements)) > + (element (or (org-element-lineage element types t) > + (org-element-lineage-map element > + `(let ((type (org-element-property :type > node))) > + (when (member type ',types) node)) > + 'special-block t t)))) Why only 'special-block? > * lisp/org-faces.el (org-structure): Add new face. > (org-headline-stars, org-list-bullet, org-list-indent): Add new faces, > inheriting from org-structure. For new faces and features, we need an entry in ORG-NEWS. Also, :package-version should be added to defface I think. > (defface org-hide > - '((((background light)) (:foreground "white")) > + '((default :inherit org-structure) > + (((background light)) (:foreground "white")) > (((background dark)) (:foreground "black"))) I think we also need (t) to apply default there. > +(defun org-fontify-extend-space (beg end _old-len) > + "Extend the region to refontify for plain list indentation. > + > +If an outermost plain list is either created or ended by a change, it > +alters whether indentation is significant for child contents. To > +accommodate, extend the region to include any indented lines between > +the end of the change and where either the next list item begins or > +an outermost list would necessarily end (a non-indented line or the > +end of buffer)." > + (goto-char end) > + (forward-line) > + (let ((re (concat "\\S-\\|\\'\\|" (org-item-re)))) > + (if (looking-at-p re) > + (cons beg end) > + (while (not (looking-at-p re)) (forward-line)) > + (cons beg (point))))) You change this new function in the next patch. Can squash the changes here. Also, there are compiler warnings after applying your patch.
test2.org
Description: Lotus Organizer
-- Ihor Radchenko // yantar92, Org mode maintainer, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92>
