org-forward-heading-same-level and the invisible-ok argument

org-forward-heading-same-level and the invisible-ok argument

[D]

Hi,

I am currently thinking about how to have
org-forward-heading-same-level and its sister command work together
with a setting of my minor mode org-superstar-mode.  The issue that
arises is that when my mode renders the leading stars of a headline
invisible, org-forward-heading-same-level considers the partially
invisible headlines fully invisible, and hence only works correctly
for single-asterisk headlines.

The solution I considered is making two interactive commands that
simply call org-forward-heading-same-level with invisible-ok set to t
and overshadow the bindings (C-c C-f and <menu-bar> <Org>
<Navigate Headings> <Next Same Level>).  The downside to this is of
course is the intrusive nature of it, and I am concerned about side
effects I may be overlooking.  So I thought it is best to ask *why* the
navigation commands take invisibility into account the way they do,
and whether you guys have a suggestion that may be more elegant than
the approach I came up?

Cheers,
D.

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

[Ihor Radchenko]

> So I thought it is best to ask *why* the
>navigation commands take invisibility into account the way they do,
>and whether you guys have a suggestion that may be more elegant than
>the approach I came up?

I guess it is simply because nobody though that the leading stars can be
hidden via fontification.

I think the whole issue can be fixed by changing the call to
org-invisible-p inside org-forward-heading-same-level. org-invisible-p
has an optional third argument to ignore text hidden via fontification.
You can try to make a patch for org-forward-heading-same-level and
similar commands adding that third argument.

Best,
Ihor


D <d.williams@posteo.net> writes:

> Hi,
>
> I am currently thinking about how to have
> org-forward-heading-same-level and its sister command work together
> with a setting of my minor mode org-superstar-mode.  The issue that
> arises is that when my mode renders the leading stars of a headline
> invisible, org-forward-heading-same-level considers the partially
> invisible headlines fully invisible, and hence only works correctly
> for single-asterisk headlines.
>
> The solution I considered is making two interactive commands that
> simply call org-forward-heading-same-level with invisible-ok set to t
> and overshadow the bindings (C-c C-f and <menu-bar> <Org>
> <Navigate Headings> <Next Same Level>).  The downside to this is of
> course is the intrusive nature of it, and I am concerned about side
> effects I may be overlooking.  So I thought it is best to ask *why* the
> navigation commands take invisibility into account the way they do,
> and whether you guys have a suggestion that may be more elegant than
> the approach I came up?
>
> Cheers,
> D.

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

[D]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]


> I guess it is simply because nobody though that the leading stars can be
> hidden via fontification.
> 
> I think the whole issue can be fixed by changing the call to
> org-invisible-p inside org-forward-heading-same-level. org-invisible-p
> has an optional third argument to ignore text hidden via fontification.
> You can try to make a patch for org-forward-heading-same-level and
> similar commands adding that third argument.

I looked into the git repository and noticed that org-invisible-p
already, thanks to a patch from Nicolas Goaziou (b1822760f4).
What I am wondering is whether it would make more sense as an argument
for org-forward-heading-same-level and similar commands, or as a
(potentially buffer-local) org variable to tweak the behavior, given
it's most likely going to affect these functions as interactive commands
(see alternative_org.el.diff).  This version has the advantage of
allowing minor modes to easily mess with Org's behavior buffer-locally.

I considered how adding a third argument to
org-forward-heading-same-level, but realized that adding another
invisibility argument would kinda leak a little complexity, as we'd wind
up with TWO invisibility-related arguments (which only differ in subtle
ways) and their interactions.  So instead I'd recommend to instead allow
a distinct, non-nil option for invisible-ok, for example `t', `nil' and
`except-folding' (see org.el.diff).  This version would be more
transparent, as it would not change the behavior of ALL calls in the
buffer, but comes at the cost of minor modes needing to overshadow
bindings to accomplish the same.

I whipped up a quick diff for both versions, and will gladly make a
patch once we settled on one, but I wanted to discuss which is
preferable before making an uninformed decision on my own.

[-- Attachment #2: org.el.diff --]
[-- Type: text/x-patch, Size: 1423 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index 71dbc611e..b8e6d47c2 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20482,7 +20482,12 @@ entry."
   "Move forward to the ARG'th subheading at same level as this one.
 Stop at the first and last subheadings of a superior heading.
 Normally this only looks at visible headings, but when INVISIBLE-OK is
-non-nil it will also look at invisible ones."
+non-nil it will also look at invisible ones.
+
+If INVISIBLE-OK is set to the symbol `except-folding', continue
+ignoring all parts that are invisible due to folding of a
+headline, a block or a drawer, i.e., not because of
+fontification."
   (interactive "p")
   (let ((backward? (and arg (< arg 0))))
     (if (org-before-first-heading-p)
@@ -20495,12 +20500,14 @@ non-nil it will also look at invisible ones."
 	    (result (point)))
 	(while (and (> count 0)
 		    (funcall f org-outline-regexp-bol nil 'move))
-	  (let ((l (- (match-end 0) (match-beginning 0) 1)))
+	  (let ((l (- (match-end 0) (match-beginning 0) 1))
+		(folding-only (eq 'except-folding invisible-ok)))
 	    (cond ((< l level) (setq count 0))
 		  ((and (= l level)
-			(or invisible-ok
+			(or (and invisible-ok (not folding-only))
 			    (not (org-invisible-p
-				  (line-beginning-position)))))
+				  (line-beginning-position)
+				  folding-only))))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))

[-- Attachment #3: alternative_org.el.diff --]
[-- Type: text/x-patch, Size: 1424 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index 71dbc611e..8e0040814 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20478,6 +20478,18 @@ entry."
 		((looking-at-p re) (forward-line))
 		(t (throw 'exit t))))))))
 
+
+(defcustom org-navigate-invisible-headings nil
+  "If non-nil, navigate through (unfolded) invisible headings normally.
+Commands such as `org-forward-heading-same-level' and
+`org-forward-heading-same-level' will consider all invisible
+headings except for those that are invisible due to folding of a
+headline, a block or a drawer."
+  :group 'org
+  :type '(choice
+	  (const :tag "Don't navigate invisible headings" nil)
+	  (const :tag "Navigate invisible (unfolded) headings" t)))
+
 (defun org-forward-heading-same-level (arg &optional invisible-ok)
   "Move forward to the ARG'th subheading at same level as this one.
 Stop at the first and last subheadings of a superior heading.
@@ -20498,9 +20510,11 @@ non-nil it will also look at invisible ones."
 	  (let ((l (- (match-end 0) (match-beginning 0) 1)))
 	    (cond ((< l level) (setq count 0))
 		  ((and (= l level)
-			(or invisible-ok
+			(or (and invisible-ok
+				 (not org-navigate-invisible-headings))
 			    (not (org-invisible-p
-				  (line-beginning-position)))))
+				  (line-beginning-position)
+				  org-navigate-invisible-headings))))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))

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

[Ihor Radchenko]

> What I am wondering is whether it would make more sense as an argument
> for org-forward-heading-same-level and similar commands, or as a
> (potentially buffer-local) org variable to tweak the behavior, given
> it's most likely going to affect these functions as interactive commands
> (see alternative_org.el.diff).  This version has the advantage of
> allowing minor modes to easily mess with Org's behavior buffer-locally.

I do not think that setting visibility the leading stars is a correct
approach to control the movement commands. After second though about the
issue you raised in the first email, I think that it would make more
sense for org-forward-heading-same-level to check if any part of the
heading line is visible to decide if we need to skip it (instead of
current approach checking only the point at the beginning of the
headline). Any mode aiming to make org-forward-heading-same-level skip a
heading will then just need to make the whole heading invisible.
Skipping partially visible headlines would be a violation of the
docstring.

Best,
Ihor


D <d.williams@posteo.net> writes:

>> I guess it is simply because nobody though that the leading stars can be
>> hidden via fontification.
>> 
>> I think the whole issue can be fixed by changing the call to
>> org-invisible-p inside org-forward-heading-same-level. org-invisible-p
>> has an optional third argument to ignore text hidden via fontification.
>> You can try to make a patch for org-forward-heading-same-level and
>> similar commands adding that third argument.
>
> I looked into the git repository and noticed that org-invisible-p
> already, thanks to a patch from Nicolas Goaziou (b1822760f4).
> What I am wondering is whether it would make more sense as an argument
> for org-forward-heading-same-level and similar commands, or as a
> (potentially buffer-local) org variable to tweak the behavior, given
> it's most likely going to affect these functions as interactive commands
> (see alternative_org.el.diff).  This version has the advantage of
> allowing minor modes to easily mess with Org's behavior buffer-locally.
>
> I considered how adding a third argument to
> org-forward-heading-same-level, but realized that adding another
> invisibility argument would kinda leak a little complexity, as we'd wind
> up with TWO invisibility-related arguments (which only differ in subtle
> ways) and their interactions.  So instead I'd recommend to instead allow
> a distinct, non-nil option for invisible-ok, for example `t', `nil' and
> `except-folding' (see org.el.diff).  This version would be more
> transparent, as it would not change the behavior of ALL calls in the
> buffer, but comes at the cost of minor modes needing to overshadow
> bindings to accomplish the same.
>
> I whipped up a quick diff for both versions, and will gladly make a
> patch once we settled on one, but I wanted to discuss which is
> preferable before making an uninformed decision on my own.
> diff --git a/lisp/org.el b/lisp/org.el
> index 71dbc611e..b8e6d47c2 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -20482,7 +20482,12 @@ entry."
>    "Move forward to the ARG'th subheading at same level as this one.
>  Stop at the first and last subheadings of a superior heading.
>  Normally this only looks at visible headings, but when INVISIBLE-OK is
> -non-nil it will also look at invisible ones."
> +non-nil it will also look at invisible ones.
> +
> +If INVISIBLE-OK is set to the symbol `except-folding', continue
> +ignoring all parts that are invisible due to folding of a
> +headline, a block or a drawer, i.e., not because of
> +fontification."
>    (interactive "p")
>    (let ((backward? (and arg (< arg 0))))
>      (if (org-before-first-heading-p)
> @@ -20495,12 +20500,14 @@ non-nil it will also look at invisible ones."
>  	    (result (point)))
>  	(while (and (> count 0)
>  		    (funcall f org-outline-regexp-bol nil 'move))
> -	  (let ((l (- (match-end 0) (match-beginning 0) 1)))
> +	  (let ((l (- (match-end 0) (match-beginning 0) 1))
> +		(folding-only (eq 'except-folding invisible-ok)))
>  	    (cond ((< l level) (setq count 0))
>  		  ((and (= l level)
> -			(or invisible-ok
> +			(or (and invisible-ok (not folding-only))
>  			    (not (org-invisible-p
> -				  (line-beginning-position)))))
> +				  (line-beginning-position)
> +				  folding-only))))
>  		   (cl-decf count)
>  		   (when (= l level) (setq result (point)))))))
>  	(goto-char result))
> diff --git a/lisp/org.el b/lisp/org.el
> index 71dbc611e..8e0040814 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -20478,6 +20478,18 @@ entry."
>  		((looking-at-p re) (forward-line))
>  		(t (throw 'exit t))))))))
>  
> +
> +(defcustom org-navigate-invisible-headings nil
> +  "If non-nil, navigate through (unfolded) invisible headings normally.
> +Commands such as `org-forward-heading-same-level' and
> +`org-forward-heading-same-level' will consider all invisible
> +headings except for those that are invisible due to folding of a
> +headline, a block or a drawer."
> +  :group 'org
> +  :type '(choice
> +	  (const :tag "Don't navigate invisible headings" nil)
> +	  (const :tag "Navigate invisible (unfolded) headings" t)))
> +
>  (defun org-forward-heading-same-level (arg &optional invisible-ok)
>    "Move forward to the ARG'th subheading at same level as this one.
>  Stop at the first and last subheadings of a superior heading.
> @@ -20498,9 +20510,11 @@ non-nil it will also look at invisible ones."
>  	  (let ((l (- (match-end 0) (match-beginning 0) 1)))
>  	    (cond ((< l level) (setq count 0))
>  		  ((and (= l level)
> -			(or invisible-ok
> +			(or (and invisible-ok
> +				 (not org-navigate-invisible-headings))
>  			    (not (org-invisible-p
> -				  (line-beginning-position)))))
> +				  (line-beginning-position)
> +				  org-navigate-invisible-headings))))
>  		   (cl-decf count)
>  		   (when (= l level) (setq result (point)))))))
>  	(goto-char result))

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

[D]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

> I do not think that setting visibility the leading stars is a correct
> approach to control the movement commands. After second though about the
> issue you raised in the first email, I think that it would make more
> sense for org-forward-heading-same-level to check if any part of the
> heading line is visible to decide if we need to skip it (instead of
> current approach checking only the point at the beginning of the
> headline). Any mode aiming to make org-forward-heading-same-level skip a
> heading will then just need to make the whole heading invisible.
> Skipping partially visible headlines would be a violation of the
> docstring.

Good point!  I think this would be a more or less reasonable patch, then.

Cheers,
D.

[-- Attachment #2: 0001-org.el-let-heading-navigation-check-the-entire-headi.patch --]
[-- Type: text/x-patch, Size: 1439 bytes --]

From 4c0f638104f689780de317af5f715384152459bd Mon Sep 17 00:00:00 2001
From: "D. Williams" <d.williams@posteo.net>
Date: Fri, 28 Aug 2020 14:15:31 +0200
Subject: [PATCH] org.el: let heading navigation check the entire heading for
 visibility

* org.el (org-forward-heading-same-level): check complete heading instead of the first char

TINYCHANGE
---
 lisp/org.el | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 71dbc611e..26f815e19 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20478,6 +20478,15 @@ entry."
 		((looking-at-p re) (forward-line))
 		(t (throw 'exit t))))))))
 
+(defun org--line-visible-p ()
+  "Return t if the current line is partially visible."
+  (and
+   (memq nil
+	 (mapcar #'org-invisible-p
+		 (number-sequence (line-beginning-position)
+				  (1- (line-end-position)))))
+       t))
+
 (defun org-forward-heading-same-level (arg &optional invisible-ok)
   "Move forward to the ARG'th subheading at same level as this one.
 Stop at the first and last subheadings of a superior heading.
@@ -20499,8 +20508,7 @@ non-nil it will also look at invisible ones."
 	    (cond ((< l level) (setq count 0))
 		  ((and (= l level)
 			(or invisible-ok
-			    (not (org-invisible-p
-				  (line-beginning-position)))))
+			    (org--line-visible-p)))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))
-- 
2.26.2

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

[Ihor Radchenko]

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

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

Best,
Ihor


D <d.williams@posteo.net> writes:

>> I do not think that setting visibility the leading stars is a correct
>> approach to control the movement commands. After second though about the
>> issue you raised in the first email, I think that it would make more
>> sense for org-forward-heading-same-level to check if any part of the
>> heading line is visible to decide if we need to skip it (instead of
>> current approach checking only the point at the beginning of the
>> headline). Any mode aiming to make org-forward-heading-same-level skip a
>> heading will then just need to make the whole heading invisible.
>> Skipping partially visible headlines would be a violation of the
>> docstring.
>
> Good point!  I think this would be a more or less reasonable patch, then.
>
> Cheers,
> D.
> From 4c0f638104f689780de317af5f715384152459bd Mon Sep 17 00:00:00 2001
> From: "D. Williams" <d.williams@posteo.net>
> Date: Fri, 28 Aug 2020 14:15:31 +0200
> Subject: [PATCH] org.el: let heading navigation check the entire heading for
>  visibility
>
> * org.el (org-forward-heading-same-level): check complete heading instead of the first char
>
> TINYCHANGE
> ---
>  lisp/org.el | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 71dbc611e..26f815e19 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -20478,6 +20478,15 @@ entry."
>  		((looking-at-p re) (forward-line))
>  		(t (throw 'exit t))))))))
>  
> +(defun org--line-visible-p ()
> +  "Return t if the current line is partially visible."
> +  (and
> +   (memq nil
> +	 (mapcar #'org-invisible-p
> +		 (number-sequence (line-beginning-position)
> +				  (1- (line-end-position)))))
> +       t))
> +
>  (defun org-forward-heading-same-level (arg &optional invisible-ok)
>    "Move forward to the ARG'th subheading at same level as this one.
>  Stop at the first and last subheadings of a superior heading.
> @@ -20499,8 +20508,7 @@ non-nil it will also look at invisible ones."
>  	    (cond ((< l level) (setq count 0))
>  		  ((and (= l level)
>  			(or invisible-ok
> -			    (not (org-invisible-p
> -				  (line-beginning-position)))))
> +			    (org--line-visible-p)))
>  		   (cl-decf count)
>  		   (when (= l level) (setq result (point)))))))
>  	(goto-char result))
> -- 
> 2.26.2

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

[D]

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

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

[Ihor Radchenko]

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

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

[D]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

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

Oh yeah, that's much nicer.  I also made the predicate check
right-to-left, which just causes it to check the text bit of a heading
first, which is useful for the cases where the predicate returns t and
makes no difference otherwise.  I again ran the tests and it seems ready
to go.

Cheers,
D.

[-- Attachment #2: 0001-org.el-let-heading-navigation-check-the-entire-headi.patch --]
[-- Type: text/x-patch, Size: 1425 bytes --]

From 9ae3dd4b73b2b9b41244bb7f9c610ed3f4777398 Mon Sep 17 00:00:00 2001
From: "D. Williams" <d.williams@posteo.net>
Date: Sun, 30 Aug 2020 23:58:55 +0200
Subject: [PATCH] org.el: let heading navigation check the entire heading for
 visibility

* org.el (org-forward-heading-same-level): check complete heading instead of the first char

TINYCHANGE
---
 lisp/org.el | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 71dbc611e..f44f94ec4 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20478,6 +20478,14 @@ entry."
 		((looking-at-p re) (forward-line))
 		(t (throw 'exit t))))))))
 
+(defun org--line-visible-p ()
+  "Return t if the current line is partially visible."
+  (not
+   (seq-every-p #'org-invisible-p
+		(number-sequence (1- (line-end-position))
+				 (line-beginning-position)
+				 -1))))
+
 (defun org-forward-heading-same-level (arg &optional invisible-ok)
   "Move forward to the ARG'th subheading at same level as this one.
 Stop at the first and last subheadings of a superior heading.
@@ -20499,8 +20507,7 @@ non-nil it will also look at invisible ones."
 	    (cond ((< l level) (setq count 0))
 		  ((and (= l level)
 			(or invisible-ok
-			    (not (org-invisible-p
-				  (line-beginning-position)))))
+			    (org--line-visible-p)))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))
-- 
2.26.2

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

[Bastien]

D <d.williams@posteo.net> writes:

>> 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.
>
> Oh yeah, that's much nicer.  I also made the predicate check
> right-to-left, which just causes it to check the text bit of a heading
> first, which is useful for the cases where the predicate returns t and
> makes no difference otherwise.  I again ran the tests and it seems ready
> to go.

Thanks for the patch.  

Does it fix a problem for org-superstar-mode or a more general problem
in Org?

Why do you need to check the visibility status every character in the
headline (even for the org-superstar-mode, where you seem to need to
check for the visibility status /after/ the stars)?

If you use seq* functions, the code will be incompatible with previous
emacsen, right?

Thanks for any follow-up,

-- 
 Bastien

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

[D]

Hi,

> Does it fix a problem for org-superstar-mode or a more general problem
> in Org?

It mostly fixes an org-superstar-mode problem (see
https://github.com/integral-dw/org-superstar-mode/issues/19).

> Why do you need to check the visibility status every character in the
> headline (even for the org-superstar-mode, where you seem to need to
> check for the visibility status /after/ the stars)?

There is no intrinsic need for my current application for it to check
anything past the last star, I just went with the most "naive"
implementation first.  If it were just for org-superstar (and similar
modes [1]) it would be fully sufficient to check the heading from the
position of the last star up to the first character past the space, if
present (which would boil it down to checking 3 chars instead of N).

> If you use seq* functions, the code will be incompatible with previous
> emacsen, right?

Hmm, looking at the oldest available ELPA release (seq-1.0, 2015),
seq-every-p should be fully backwards-compatible.  The current package
itself also has a fallback option for Emacs versions <25, so that should
be fine.

Cheers,
D

[1] https://github.com/TonCherAmi/org-starless

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

[Bastien]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

Hi,

D <d.williams@posteo.net> writes:

>> Does it fix a problem for org-superstar-mode or a more general problem
>> in Org?
>
> It mostly fixes an org-superstar-mode problem (see
> https://github.com/integral-dw/org-superstar-mode/issues/19).

Can you try the attached patch and tell wether it fixes your issue?

>> If you use seq* functions, the code will be incompatible with previous
>> emacsen, right?
>
> Hmm, looking at the oldest available ELPA release (seq-1.0, 2015),
> seq-every-p should be fully backwards-compatible.  The current package
> itself also has a fallback option for Emacs versions <25, so that should
> be fine.

I'd rather not add a dependency over seq.el anyway.

Thanks,

-- 
 Bastien

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: org-forward-heading-same-level.patch --]
[-- Type: text/x-diff, Size: 421 bytes --]

diff --git a/lisp/org.el b/lisp/org.el
index a5c7dcf3b..f6e04e65c 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20529,7 +20529,7 @@ non-nil it will also look at invisible ones."
 		  ((and (= l level)
 			(or invisible-ok
 			    (not (org-invisible-p
-				  (line-beginning-position)))))
+				  (1- (line-end-position))))))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))

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

[Ihor Radchenko]

> Can you try the attached patch and tell wether it fixes your issue?

The patch will fail in the following headline, because of invisible last
char:

* [[file:test.el][test]]

In general, we can only guarantee that partially visible headline is not
skipped only if we check all the positions. Or we can change the
docstring defining the visibility criteria.

> I'd rather not add a dependency over seq.el anyway.

Then, can as well use `mapcar', or even simply manual loop over line
positions.

Best,
Ihor



Bastien <bzg@gnu.org> writes:

> Hi,
>
> D <d.williams@posteo.net> writes:
>
>>> Does it fix a problem for org-superstar-mode or a more general problem
>>> in Org?
>>
>> It mostly fixes an org-superstar-mode problem (see
>> https://github.com/integral-dw/org-superstar-mode/issues/19).
>
> Can you try the attached patch and tell wether it fixes your issue?
>
>>> If you use seq* functions, the code will be incompatible with previous
>>> emacsen, right?
>>
>> Hmm, looking at the oldest available ELPA release (seq-1.0, 2015),
>> seq-every-p should be fully backwards-compatible.  The current package
>> itself also has a fallback option for Emacs versions <25, so that should
>> be fine.
>
> I'd rather not add a dependency over seq.el anyway.
>
> Thanks,
>
> -- 
>  Bastien
> diff --git a/lisp/org.el b/lisp/org.el
> index a5c7dcf3b..f6e04e65c 100644
> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -20529,7 +20529,7 @@ non-nil it will also look at invisible ones."
>  		  ((and (= l level)
>  			(or invisible-ok
>  			    (not (org-invisible-p
> -				  (line-beginning-position)))))
> +				  (1- (line-end-position))))))
>  		   (cl-decf count)
>  		   (when (= l level) (setq result (point)))))))
>  	(goto-char result))

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

[D]

[-- Attachment #1: Type: text/plain, Size: 101 bytes --]

> Then, can as well use `mapcar', or even simply manual loop over line
> positions.

How about this?

[-- Attachment #2: 0001-org.el-let-heading-navigation-check-the-entire-headi.patch --]
[-- Type: text/x-patch, Size: 1558 bytes --]

From 2324d745f12fe8e8d4f7864307eb55c46fc49504 Mon Sep 17 00:00:00 2001
From: "D. Williams" <d.williams@posteo.net>
Date: Mon, 7 Sep 2020 14:13:12 +0200
Subject: [PATCH] org.el: let heading navigation check the entire heading for
 visibility

* org.el (org-forward-heading-same-level): check complete heading instead of the first char

TINYCHANGE
---
 lisp/org.el | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index bc74cedc7..040cfad61 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -20512,6 +20512,16 @@ entry."
 		((looking-at-p re) (forward-line))
 		(t (throw 'exit t))))))))
 
+(defun org--line-visible-p ()
+  "Return t if the current line is partially visible."
+  (let ((line-beg (line-beginning-position))
+	(line-pos (1- (line-end-position)))
+	(is-invisible t))
+    (while (and (< line-beg line-pos) is-invisible)
+      (setq is-invisible (org-invisible-p line-pos))
+      (cl-decf line-pos))
+    (not is-invisible)))
+
 (defun org-forward-heading-same-level (arg &optional invisible-ok)
   "Move forward to the ARG'th subheading at same level as this one.
 Stop at the first and last subheadings of a superior heading.
@@ -20533,8 +20543,7 @@ non-nil it will also look at invisible ones."
 	    (cond ((< l level) (setq count 0))
 		  ((and (= l level)
 			(or invisible-ok
-			    (not (org-invisible-p
-				  (line-beginning-position)))))
+			    (org--line-visible-p)))
 		   (cl-decf count)
 		   (when (= l level) (setq result (point)))))))
 	(goto-char result))
-- 
2.26.2

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

[Bastien]

Hi D,

D <d.williams@posteo.net> writes:

>> Then, can as well use `mapcar', or even simply manual loop over line
>> positions.
>
> How about this?

I applied a small variant of it as a700fadd7, thanks.

(See also the comment I added with f17d301e1, which basically means
that such changes are made as exceptions.)

-- 
 Bastien

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

[D]

Hi Bastien,

> I applied a small variant of it as a700fadd7, thanks.

thank you!

> (See also the comment I added with f17d301e1, which basically means
> that such changes are made as exceptions.)

I fully understand, though I do believe that this change is beneficial
to the way org-forward-heading-same-level works overall, in a "principle
of least astonishment" sort of way.  I would agree with Ihor that it
does reflect the docstring better this way.

Best regards,
D.

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

[Ihor Radchenko]

> I applied a small variant of it as a700fadd7, thanks.
>
> (See also the comment I added with f17d301e1, which basically means
> that such changes are made as exceptions.)

For record, the old behaviour did not only affect a single external
package. For example, https://github.com/TonCherAmi/org-starless was
also affected.

Best,
Ihor

Bastien <bzg@gnu.org> writes:

> Hi D,
>
> D <d.williams@posteo.net> writes:
>
>>> Then, can as well use `mapcar', or even simply manual loop over line
>>> positions.
>>
>> How about this?
>
> I applied a small variant of it as a700fadd7, thanks.
>
> (See also the comment I added with f17d301e1, which basically means
> that such changes are made as exceptions.)
>
> -- 
>  Bastien

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

[Bastien]

Ihor Radchenko <yantar92@gmail.com> writes:

>> I applied a small variant of it as a700fadd7, thanks.
>>
>> (See also the comment I added with f17d301e1, which basically means
>> that such changes are made as exceptions.)
>
> For record, the old behaviour did not only affect a single external
> package. For example, https://github.com/TonCherAmi/org-starless was
> also affected.

Good to know, thanks!

-- 
 Bastien

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

[Bastien]

D <d.williams@posteo.net> writes:

> I fully understand, though I do believe that this change is beneficial
> to the way org-forward-heading-same-level works overall, in a "principle
> of least astonishment" sort of way.

Yes, that's why I allowed this exception, but still, I think it's best
to leave the FIXME for future generations.

-- 
 Bastien