Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument

[D <d.williams@posteo.net>]

>> +	 (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.