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

[Ihor Radchenko <yantar92@gmail.com>]

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