Re: [PATCH] Re: Re: Re: org-forward-heading-same-level and the invisible-ok argument
D 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
Re: [PATCH] Re: Re: Re: org-forward-heading-same-level and the invisible-ok argument
Ihor Radchenko 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
> 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 writes: > Hi D, > > D 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
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
Hi D, D 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
> Then, can as well use `mapcar', or even simply manual loop over line > positions. How about this? >From 2324d745f12fe8e8d4f7864307eb55c46fc49504 Mon Sep 17 00:00:00 2001 From: "D. Williams" 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
> 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 writes: > Hi, > > D 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
Hi, D 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
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
D 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
[PATCH] Re: Re: Re: org-forward-heading-same-level and the invisible-ok argument
> 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. >From 9ae3dd4b73b2b9b41244bb7f9c610ed3f4777398 Mon Sep 17 00:00:00 2001 From: "D. Williams" 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
