[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok
From: |
Ihor Radchenko |
Subject: |
Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument |
Date: |
Sat, 29 Aug 2020 13:10:03 +0800 |
> 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.
- org-forward-heading-same-level and the invisible-ok argument, D, 2020/08/25
- Re: org-forward-heading-same-level and the invisible-ok argument, Ihor Radchenko, 2020/08/25
- [PATCH] Re: org-forward-heading-same-level and the invisible-ok argument, D, 2020/08/26
- Re: [PATCH] Re: org-forward-heading-same-level and the invisible-ok argument, Ihor Radchenko, 2020/08/27
- [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument, D, 2020/08/28
- Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument, Ihor Radchenko, 2020/08/28
- Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument, D, 2020/08/28
- Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument,
Ihor Radchenko <=
- [PATCH] Re: Re: Re: org-forward-heading-same-level and the invisible-ok argument, D, 2020/08/30