> The issue with that is that it reintroduces the thing the patch is > trying to fix, as it considers any partially invisible line as fully > invisible (while the opposite should be the case).
Oops. Sorry, I wrote that function without much thinking. Indeed, it should be (defun org--line-visible-p () "Return t if the current line is partially visible." (save-restriction (narrow-to-region (line-beginning-position) (1- (line-end-position))) (let ((visible nil) (p (point-min))) (while (and (not visible) (< p (point-max))) (when (not (org-invisible-p p)) (setq visible t)) (setq p (next-single-char-property-change p 'invisible))) visible))) > I also don't see > much of a difference when profiling, unless I severely screwed up. I > just manually prepared a buffer with 10k invisible headings (each 83 > chars long) and plugged a visible one of the same level above and below > the huge invisible block. C-c C-f does take a second plus change in > that case for both cases. I also tested on one of my largest org files. There is not much difference and your version should be acceptable. > which basically removes any relevant slowdown. > But I'd really only recommend going that far if necessary, given it adds > a random magic number. Thoughts? I do not think it will make much difference. Mapcar will anyway apply #'org-invisible-p for all points at the headline (well, all, but 3 points). Probably, it is easier if you just use seq-every-p instead of mapcar on (number-sequence max-pos min-pos -1). The result of seq-every-p will be inverse of the currently used expression. Best, Ihor D <d.willi...@posteo.net> writes: >>> + (mapcar #'org-invisible-p >>> + (number-sequence (line-beginning-position) >>> + (1- (line-end-position))))) >> >> This is a bad idea. org--line-visible-p will be called for every single >> invisible headline. If you check every single point at every single >> invisible headline, it can be extremely slow. > > Hm, of course it would only check invisible headlines of the same level, > thanks to the logic of the navigation command. However, I do see the > concern. > >> Better do something like below (or maybe even without narrow-to-region, >> not sure if that may cause significant overhead): >> >> (defun org--line-visible-p () >> "Return t if the current line is partially visible." >> (save-restriction >> (narrow-to-region (line-beginning-position) (1- (line-end-position))) >> (let ((visible t) >> (p (point-min))) >> (while (and visible (< p (point-max))) >> (when (org-invisible-p p) >> (setq visible nil)) >> (setq p (next-single-char-property-change p 'invisible))) >> visible))) > > The issue with that is that it reintroduces the thing the patch is > trying to fix, as it considers any partially invisible line as fully > invisible (while the opposite should be the case). I also don't see > much of a difference when profiling, unless I severely screwed up. I > just manually prepared a buffer with 10k invisible headings (each 83 > chars long) and plugged a visible one of the same level above and below > the huge invisible block. C-c C-f does take a second plus change in > that case for both cases. > > The main issue is that a hot-circuit approach would only ever optimize > navigating across many visible blocks, if we want a partially visible > heading to be treated like a visible one (instead of an invisible one > like it is at the moment). However, we could use a similar hack as the > original implementation and only check a couple of characters, if > performance really is a concern there. In that case, I think it would > be best to check the *last* few characters (as it makes more sense that > the stars at the beginning are hidden than the actual title at the end). > That would mean to replace the original implementation with something like > > (defun org--line-visible-p () > "Return t if the current line is partially visible." > (let ((min-pos (max (line-beginning-position) > (- (line-end-position) 3))) > (max-pos (1- (line-end-position)))) > (and > (memq nil > (mapcar #'org-invisible-p > (number-sequence min-pos max-pos))) > t))) > > which basically removes any relevant slowdown. > But I'd really only recommend going that far if necessary, given it adds > a random magic number. Thoughts? > > Cheers, > D.