emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Ihor Radchenko <yantar92@gmail.com>
To: D <d.williams@posteo.net>
Cc: emacs-orgmode@gnu.org
Subject: Re: [PATCH] Re: Re: org-forward-heading-same-level and the invisible-ok argument
Date: Sat, 29 Aug 2020 13:10:03 +0800	[thread overview]
Message-ID: <87364611hw.fsf@localhost> (raw)
In-Reply-To: <8e63089c-ba3e-788a-f80f-c05b14d3228d@posteo.net>

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


  reply	other threads:[~2020-08-29  5:14 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-25 16:46 org-forward-heading-same-level and the invisible-ok argument D
2020-08-26  1:30 ` Ihor Radchenko
2020-08-26 21:33   ` [PATCH] " D
2020-08-27 11:49     ` Ihor Radchenko
2020-08-28 12:27       ` [PATCH] " D
2020-08-28 13:43         ` Ihor Radchenko
2020-08-28 17:49           ` D
2020-08-29  5:10             ` Ihor Radchenko [this message]
2020-08-30 22:07               ` [PATCH] " D
2020-09-06  6:35                 ` Bastien
2020-09-06 11:09                   ` D
2020-09-07  5:06                     ` Bastien
2020-09-07  6:25                       ` Ihor Radchenko
2020-09-07 18:31                         ` D
2020-09-08  9:28                           ` Bastien
2020-09-08 20:00                             ` D
2020-09-09  8:09                               ` Bastien
2020-09-09  4:15                             ` Ihor Radchenko
2020-09-09  8:08                               ` Bastien

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87364611hw.fsf@localhost \
    --to=yantar92@gmail.com \
    --cc=d.williams@posteo.net \
    --cc=emacs-orgmode@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).