bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Hi,

According to isearch.el source, one can use
`isearch-open-invisible-temporary' to implement custom hidden text
display during isearch. However, it is unclear what are the requirements
for that function.

From isearch source code, it appears that the function should accept two
arguments: overlays and a flag to open/close the overlay:

      ;; Some modes would want to open the overlays temporary during
      ;; isearch in their own way, they should set the
      ;; `isearch-open-invisible-temporary' to a function doing this.
      (funcall  (overlay-get ov 'isearch-open-invisible-temporary)  ov nil)

However, it appears that deleting the passed overlay is, in fact,
prohibited. (manifested in
https://orgmode.org/list/87pmc4smdg.fsf@fastmail.fm)

Is there some detailed information about this feature available somewhere?

Best,
Ihor

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2022-12-26 built on localhost
Repository revision: cc29fab3a66c59e77d0ff67c0f3e2e34ec80a03c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Gentoo Linux

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Juri Linkov]

> According to isearch.el source, one can use
> `isearch-open-invisible-temporary' to implement custom hidden text
> display during isearch. However, it is unclear what are the requirements
> for that function.
>
> From isearch source code, it appears that the function should accept two
> arguments: overlays and a flag to open/close the overlay:
>
>       ;; Some modes would want to open the overlays temporary during
>       ;; isearch in their own way, they should set the
>       ;; `isearch-open-invisible-temporary' to a function doing this.
>       (funcall  (overlay-get ov 'isearch-open-invisible-temporary)  ov nil)
>
> However, it appears that deleting the passed overlay is, in fact,
> prohibited. (manifested in
> https://orgmode.org/list/87pmc4smdg.fsf@fastmail.fm)
>
> Is there some detailed information about this feature available somewhere?

I don't know about this function more than you can read from source code,
but can find two mentions in (info "(elisp) Invisible Text") and
(info "(elisp) Overlay Properties").

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

>>       ;; Some modes would want to open the overlays temporary during
>>       ;; isearch in their own way, they should set the
>>       ;; `isearch-open-invisible-temporary' to a function doing this.
>>       (funcall  (overlay-get ov 'isearch-open-invisible-temporary)  ov nil)
>>
>> However, it appears that deleting the passed overlay is, in fact,
>> prohibited. (manifested in
>> https://orgmode.org/list/87pmc4smdg.fsf@fastmail.fm)
>>
>> Is there some detailed information about this feature available somewhere?
>
> I don't know about this function more than you can read from source code,
> but can find two mentions in (info "(elisp) Invisible Text") and
> (info "(elisp) Overlay Properties").

Manual provides no more information.

Would it make sense to mention that overlays passed as an argument of
'isearch-open-invisible-temporary function must not be destroyed?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Eli Zaretskii]

> Cc: 60399@debbugs.gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Sat, 31 Dec 2022 13:03:11 +0000
> 
> Juri Linkov <juri@linkov.net> writes:
> 
> >>       ;; Some modes would want to open the overlays temporary during
> >>       ;; isearch in their own way, they should set the
> >>       ;; `isearch-open-invisible-temporary' to a function doing this.
> >>       (funcall  (overlay-get ov 'isearch-open-invisible-temporary)  ov nil)
> >>
> >> However, it appears that deleting the passed overlay is, in fact,
> >> prohibited. (manifested in
> >> https://orgmode.org/list/87pmc4smdg.fsf@fastmail.fm)
> >>
> >> Is there some detailed information about this feature available somewhere?
> >
> > I don't know about this function more than you can read from source code,
> > but can find two mentions in (info "(elisp) Invisible Text") and
> > (info "(elisp) Overlay Properties").
> 
> Manual provides no more information.
> 
> Would it make sense to mention that overlays passed as an argument of
> 'isearch-open-invisible-temporary function must not be destroyed?

I've now added a warning to this effect to the comments in
isearch-open-overlay-temporary, and I'm therefore closing this bug.

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Manual provides no more information.
>> 
>> Would it make sense to mention that overlays passed as an argument of
>> 'isearch-open-invisible-temporary function must not be destroyed?
>
> I've now added a warning to this effect to the comments in
> isearch-open-overlay-temporary, and I'm therefore closing this bug.

What about the manual?
(elisp) 41.6 Invisible Text

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: juri@linkov.net, 60399-done@debbugs.gnu.org
> Date: Sun, 01 Jan 2023 09:09:42 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Manual provides no more information.
> >> 
> >> Would it make sense to mention that overlays passed as an argument of
> >> 'isearch-open-invisible-temporary function must not be destroyed?
> >
> > I've now added a warning to this effect to the comments in
> > isearch-open-overlay-temporary, and I'm therefore closing this bug.
> 
> What about the manual?
> (elisp) 41.6 Invisible Text

I don't think this obscure issue needs to be mentioned in the manual.
It is quite clear to me that the overlay in question cannot be
deleted, I actually raised a brow when you asked for this to be
explicitly mentioned.

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> What about the manual?
>> (elisp) 41.6 Invisible Text
>
> I don't think this obscure issue needs to be mentioned in the manual.
> It is quite clear to me that the overlay in question cannot be
> deleted, I actually raised a brow when you asked for this to be
> explicitly mentioned.

I think I need to explain the context a bit.

In org-fold, we want isearch matched to be not just revealed according
to the full overlays they are hidden in. We want to rearrange the
overlays in such a way that outline structure above the match is
revealed:

* Folded heading
** Folded sub-heading
Some unrelated text.
:FOLDED-DRAWER:
isearch-match
:END:

which will look like

* Folded heading...

When we do isearch, the aim is

* Folded heading
** Folded sub-heading...
isearch-match
...

which requires re-creating overlays used for folding.

In the process, 'isearch-open-invisible-temporary may affect more than
the overlay used as its argument. In particular, other overlays at point
may be affected.

The 'isearch-open-invisible-temporary takes care about proper saving and
restoring the necessary overlays. However, isearch gets broken.

In this setup, it is not that obvious that isearch will still try to do
something with overlays that got deleted during the call to
'isearch-open-invisible-temporary.

I am not even sure how to achieve the desired custom behavior using the
current isearch implementation.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: juri@linkov.net, 60399-done@debbugs.gnu.org
> Date: Sun, 01 Jan 2023 12:51:40 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> What about the manual?
> >> (elisp) 41.6 Invisible Text
> >
> > I don't think this obscure issue needs to be mentioned in the manual.
> > It is quite clear to me that the overlay in question cannot be
> > deleted, I actually raised a brow when you asked for this to be
> > explicitly mentioned.
> 
> I think I need to explain the context a bit.
> 
> In org-fold, we want isearch matched to be not just revealed according
> to the full overlays they are hidden in. We want to rearrange the
> overlays in such a way that outline structure above the match is
> revealed:
> 
> * Folded heading
> ** Folded sub-heading
> Some unrelated text.
> :FOLDED-DRAWER:
> isearch-match
> :END:
> 
> which will look like
> 
> * Folded heading...
> 
> When we do isearch, the aim is
> 
> * Folded heading
> ** Folded sub-heading...
> isearch-match
> ...
> 
> which requires re-creating overlays used for folding.
> 
> In the process, 'isearch-open-invisible-temporary may affect more than
> the overlay used as its argument. In particular, other overlays at point
> may be affected.
> 
> The 'isearch-open-invisible-temporary takes care about proper saving and
> restoring the necessary overlays. However, isearch gets broken.
> 
> In this setup, it is not that obvious that isearch will still try to do
> something with overlays that got deleted during the call to
> 'isearch-open-invisible-temporary.

Thanks, this background was indeed missing.  However, even after
reading it, I still don't understand how come you need or want to
delete these overlays -- aren't they needed to hide the text to begin
with?

Or IOW, if you are deleting and recreating the overlays, why do you
need to use this isearch feature at all? just delete them when you
want to show the text, and reapply them later when you want the text
hidden again.

> I am not even sure how to achieve the desired custom behavior using the
> current isearch implementation.

It's an obscure feature indeed.

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, this background was indeed missing.  However, even after
> reading it, I still don't understand how come you need or want to
> delete these overlays -- aren't they needed to hide the text to begin
> with?
>
> Or IOW, if you are deleting and recreating the overlays, why do you
> need to use this isearch feature at all? just delete them when you
> want to show the text, and reapply them later when you want the text
> hidden again.

This is pretty much what is being done: I try to hook into isearch code
to (1) when isearch is about to display the match, I save the old
overlays and re-arrange them to display the match as needed; (2) when
isearch moves to the next match, I restore the old overlays.

However, because isearch expects the overlays to exist even upon
revealing the isearch match, I cannot achieve what I want. Even though
my code would work fine, isearch itself errs in
`isearch-close-unnecessary-overlays'/`isearch-open-necessary-overlays'
where it does additional checks involving `overlay-start' and
`overlay-end' that must be non-nil.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: juri@linkov.net, 60399-done@debbugs.gnu.org
> Date: Mon, 02 Jan 2023 08:57:11 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, this background was indeed missing.  However, even after
> > reading it, I still don't understand how come you need or want to
> > delete these overlays -- aren't they needed to hide the text to begin
> > with?
> >
> > Or IOW, if you are deleting and recreating the overlays, why do you
> > need to use this isearch feature at all? just delete them when you
> > want to show the text, and reapply them later when you want the text
> > hidden again.
> 
> This is pretty much what is being done: I try to hook into isearch code
> to (1) when isearch is about to display the match, I save the old
> overlays and re-arrange them to display the match as needed; (2) when
> isearch moves to the next match, I restore the old overlays.
> 
> However, because isearch expects the overlays to exist even upon
> revealing the isearch match, I cannot achieve what I want. Even though
> my code would work fine, isearch itself errs in
> `isearch-close-unnecessary-overlays'/`isearch-open-necessary-overlays'
> where it does additional checks involving `overlay-start' and
> `overlay-end' that must be non-nil.

I guess it's a bug in open/close-unnecessary-overlays, then?  I'm not
really sure I understand their role here.  Juri, Stefan, can you help
out?

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The 'isearch-open-invisible-temporary takes care about proper saving and
> restoring the necessary overlays. However, isearch gets broken.
>
> In this setup, it is not that obvious that isearch will still try to do
> something with overlays that got deleted during the call to
> 'isearch-open-invisible-temporary.

FWIW, I never fully understood Richard's design (installed in commit
0352b205714e1c7ab97c1b6dc7678f6b51ebe089), indeed.  E.g. I don't
understand why there need to be both
a `isearch-open-invisible-temporary` and a `isearch-open-invisible` property.

But it's very natural that the caller of that
`isearch-open-invisible-temporary` may still want to know the boundaries
of this overlay after its content is made visible, so as to know when to
make it invisible again.

The caller could store that information on its own (with a pair of
markers or another overlay), but it's very natural for the caller to
just use that overlay.

> I am not even sure how to achieve the desired custom behavior using the
> current isearch implementation.

`reveal-toggle-invisible` works basically the same way as
`isearch-open-invisible-temporary` and in
`outline-reveal-toggle-invisible` I had the same problem as you do,
which I solved with:

      (let ((o1 (copy-overlay o)))
        (overlay-put o 'invisible nil)  ;Show (most of) the text.


-- Stefan

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> But it's very natural that the caller of that
> `isearch-open-invisible-temporary` may still want to know the boundaries
> of this overlay after its content is made visible, so as to know when to
> make it invisible again.

I am not sure why isearch should decide this instead of letting
`isearch-open-invisible-temporary' decide what to close.

> The caller could store that information on its own (with a pair of
> markers or another overlay), but it's very natural for the caller to
> just use that overlay.

I'd say easy, not natural. The callee may want to do something with
_all_ the overlays it originally opened, not just the overlays it
originally opened, which _are also intersecting some unspecified
boundary_.

>> I am not even sure how to achieve the desired custom behavior using the
>> current isearch implementation.
>
> `reveal-toggle-invisible` works basically the same way as
> `isearch-open-invisible-temporary` and in
> `outline-reveal-toggle-invisible` I had the same problem as you do,
> which I solved with:
>
>       (let ((o1 (copy-overlay o)))
>         (overlay-put o 'invisible nil)  ;Show (most of) the text.

This is a nice trick, which is unfortunately not very useful in my
situation. Some text should still remain invisible in Org even when
opening is requested. Your code reveals everything unconditionally.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60399: 30.0.50; Usage of `isearch-open-invisible-temporary' is not documented

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> But it's very natural that the caller of that
>> `isearch-open-invisible-temporary` may still want to know the boundaries
>> of this overlay after its content is made visible, so as to know when to
>> make it invisible again.
>
> I am not sure why isearch should decide this instead of letting
> `isearch-open-invisible-temporary' decide what to close.

The question is not just what to close but *when*.

Isearch need to tell your code when it thinks you can close.
It's natural to use the overlay's boundaries to decide when to do that,
and it's natural to use that overlay as "the info about what to close"
when i calls you back.

I'm not claiming it's perfect or ideal.  It's just a fairly natural
choice.  Note that in `reveal.el` I use a similar API but with a twist:
after calling `reveal-toggle-invisible`, `reveal.el` checks whether the
points we're interested in have been made visible and if not, we take
the (new) overlays and repeat.  This way, to get the behavior you say
you want for Org (which we get in `outline-mode` with `reveal-mode`) we
just arrange for `reveal-toggle-invisible` to show the headings "one
level down" (and re-hide everything else): if that's not enough,
`reveal.el` then calls `reveal-toggle-invisible` inside one of the
subheadings until we're at the actual leaf.

This addresses a major shortcoming of the
`isearch-open-invisible-temporary' which is that the function is not
told which part of the overlay needs to be opened.  The best the
function can do is presume that `point` is the thing of interest, but
it's not specified anywhere (and in the case of `reveal.el` it's not
sufficient because we may have several points to reveal at the same
time).

>> `reveal-toggle-invisible` works basically the same way as
>> `isearch-open-invisible-temporary` and in
>> `outline-reveal-toggle-invisible` I had the same problem as you do,
>> which I solved with:
>>
>>       (let ((o1 (copy-overlay o)))
>>         (overlay-put o 'invisible nil)  ;Show (most of) the text.
>
> This is a nice trick, which is unfortunately not very useful in my
> situation. Some text should still remain invisible in Org even when
> opening is requested. Your code reveals everything unconditionally.

There's a misunderstanding here: `outline-reveal-toggle-invisible`
reveals only the immediate children.  All it reveals is basically:

                 (outline-show-entry)
                 (outline-show-children)


-- Stefan