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

[Ihor Radchenko <yantar92@gmail.com>]

> 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.williams@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.