Is it valid to call isearch-filter-predicate outside isearch?

Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Hi,

I am now investigating an Org mode issue related to interaction between
Org mode folding and isearch.
https://list.orgmode.org/orgmode/CAP7OBx+L11ck3Ni6rv94HGU3otdj6C4rG-rMDzkwR1LTj=BWiw@mail.gmail.com/

Org mode overrides the default value of isearch-filter-predicate with

(defun org-fold-core--isearch-filter-predicate-overlays (beg end)
  "Return non-nil if text between BEG and END is deemed visible by isearch.
This function is intended to be used as `isearch-filter-predicate'."
  (org-fold-core--create-isearch-overlays beg end) ;; trick isearch by creating overlays in place of invisible text
  (isearch-filter-visible beg end))

As you can see, Org produces side effects when the predicate is called.

I thought that side effects are acceptable since
`isearch-filter-visible' itself also manipulates buffer visibility in
`isearch-range-invisible' - it calls
`isearch-close-unnecessary-overlays' and
`isearch-open-overlay-temporary' for side effects.
However, we have found that `query-replace' calls
`isearch-filter-predicate' outside isearch, and it may not be safe to
assume that isearch hooks will be executed.

The documentation for `isearch-filter-visible' sounds like no side
effects are expected:

    Predicate to filter hits of Isearch and replace commands.
    
    Isearch hits that don't satisfy the predicate will be skipped.
    The value should be a function of two arguments; it will be
    called with the positions of the start and the end of the text
    matched by Isearch and replace commands.  If this function
    returns nil, Isearch and replace commands will continue searching
    without stopping at resp. replacing this match.
    This function is expected to be careful not to clobber the match data.
    
Either the docstring is not accurate or the implementation of
`isearch-range-invisible' is not safe.

Am I missing something?

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Juri Linkov]

> I am now investigating an Org mode issue related to interaction between
> Org mode folding and isearch.
> https://list.orgmode.org/orgmode/CAP7OBx+L11ck3Ni6rv94HGU3otdj6C4rG-rMDzkwR1LTj=BWiw@mail.gmail.com/
>
> Org mode overrides the default value of isearch-filter-predicate with
>
> (defun org-fold-core--isearch-filter-predicate-overlays (beg end)
>   "Return non-nil if text between BEG and END is deemed visible by isearch.
> This function is intended to be used as `isearch-filter-predicate'."
>   (org-fold-core--create-isearch-overlays beg end) ;; trick isearch by creating overlays in place of invisible text
>   (isearch-filter-visible beg end))
>
> As you can see, Org produces side effects when the predicate is called.
>
> I thought that side effects are acceptable since
> `isearch-filter-visible' itself also manipulates buffer visibility in
> `isearch-range-invisible' - it calls
> `isearch-close-unnecessary-overlays' and
> `isearch-open-overlay-temporary' for side effects.
> However, we have found that `query-replace' calls
> `isearch-filter-predicate' outside isearch, and it may not be safe to
> assume that isearch hooks will be executed.

Emacs 29 introduced a new value 'can-be-opened' of the variable
'search-invisible' for side-effect-free uses.

Probably you can bind it to this value in Org mode functions.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

> Emacs 29 introduced a new value 'can-be-opened' of the variable
> 'search-invisible' for side-effect-free uses.
>
> Probably you can bind it to this value in Org mode functions.

Not really. Because Org produces its own side effects to make isearch
support revealing text hidden via text properties.

I guess the answer to my question about `isearch-filter-predicate' is
yes - it may be called outside isearch and thus custom
`isearch-filter-predicate' must consider scenarios outside isearch.

It would be nice if the docstring of `isearch-filter-predicate'
mentioned that it might be called outside isearch-mode.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: emacs-devel@gnu.org
> Date: Wed, 31 May 2023 10:08:55 +0000
> 
> Juri Linkov <juri@linkov.net> writes:
> 
> > Emacs 29 introduced a new value 'can-be-opened' of the variable
> > 'search-invisible' for side-effect-free uses.
> >
> > Probably you can bind it to this value in Org mode functions.
> 
> Not really. Because Org produces its own side effects to make isearch
> support revealing text hidden via text properties.
> 
> I guess the answer to my question about `isearch-filter-predicate' is
> yes - it may be called outside isearch and thus custom
> `isearch-filter-predicate' must consider scenarios outside isearch.
> 
> It would be nice if the docstring of `isearch-filter-predicate'
> mentioned that it might be called outside isearch-mode.

I don't really understand what you are asking here to document.  From
reading the discussion, it sounds like you set
isearch-filter-predicate to a function that relies on some
(unidentified) isearch hooks to undo its side effects?  And you did
that without studying all of the existing callers of
isearch-filter-predicate?  Not sure how documentation could have
solved this for you.

In general, a predicate is not expected to have side effects, it is
expected to return a boolean value and leave everything else intact.

(I also don't understand what does isearch-range-invisible have to do
with this.)

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> It would be nice if the docstring of `isearch-filter-predicate'
>> mentioned that it might be called outside isearch-mode.
>
> I don't really understand what you are asking here to document.  From
> reading the discussion, it sounds like you set
> isearch-filter-predicate to a function that relies on some
> (unidentified) isearch hooks to undo its side effects?  And you did
> that without studying all of the existing callers of
> isearch-filter-predicate?  Not sure how documentation could have
> solved this for you.

I expected `isearch-filter-predicate' to be only used by isearch in
isearch-mode. And thus I expected `isearch-mode-end-hook' to be called
later.

Isn't it a natural expectation?

> In general, a predicate is not expected to have side effects, it is
> expected to return a boolean value and leave everything else intact.
>
> (I also don't understand what does isearch-range-invisible have to do
> with this.)

`isearch-range-invisible' can modify `isearch-opened-overlays' and
`isearch-hidden' by side effect.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: juri@linkov.net, emacs-devel@gnu.org
> Date: Wed, 31 May 2023 12:56:16 +0000
> 
> I expected `isearch-filter-predicate' to be only used by isearch in
> isearch-mode. And thus I expected `isearch-mode-end-hook' to be called
> later.
> 
> Isn't it a natural expectation?

Evidently, that ship sailed a long time ago: "grep isearch-" yields
more than 60 hits in replace.el.  And then there are many hits in
comint.el, dired-aux.el, info.el, and even in simple.el.

> > In general, a predicate is not expected to have side effects, it is
> > expected to return a boolean value and leave everything else intact.
> >
> > (I also don't understand what does isearch-range-invisible have to do
> > with this.)
> 
> `isearch-range-invisible' can modify `isearch-opened-overlays' and
> `isearch-hidden' by side effect.

Yes, but why it is relevant to your problem?

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> I expected `isearch-filter-predicate' to be only used by isearch in
>> isearch-mode. And thus I expected `isearch-mode-end-hook' to be called
>> later.
>> 
>> Isn't it a natural expectation?
>
> Evidently, that ship sailed a long time ago: "grep isearch-" yields
> more than 60 hits in replace.el.  And then there are many hits in
> comint.el, dired-aux.el, info.el, and even in simple.el.

Sure. I have no issue with this. That's why I asked to add a word of
warning about the state of affairs to the docstring. It is not normal
that major mode-specific predicates are used elsewhere.

>> > (I also don't understand what does isearch-range-invisible have to do
>> > with this.)
>> 
>> `isearch-range-invisible' can modify `isearch-opened-overlays' and
>> `isearch-hidden' by side effect.
>
> Yes, but why it is relevant to your problem?

This was one of the reasons I thought that side effects are ok.

Also, the fact that `isearch-hidden' can be modified outside isearch
might yield edge cases when something like M-x query-replace calls
`isearch-range-invisible' that happens to set `isearch-hidden' to t.
Later, if there is a followup M-x isearch call, `isearch-hidden' will
remain t.

Modifying `isearch-opened-overlays' should not cause such interaction,
AFAIU. It is initialized to nil at the beginning of isearch.

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

RE: [External] : Re: Is it valid to call isearch-filter-predicate outside isearch?

[Drew Adams]

> >> I expected `isearch-filter-predicate' to be only used by isearch in
> >> isearch-mode. And thus I expected `isearch-mode-end-hook' to be called
> >> later.
> >>
> >> Isn't it a natural expectation?
> >
> > Evidently, that ship sailed a long time ago: "grep isearch-" yields
> > more than 60 hits in replace.el.  And then there are many hits in
> > comint.el, dired-aux.el, info.el, and even in simple.el.
> 
> Sure. I have no issue with this. That's why I asked to add a word of
> warning about the state of affairs to the docstring. It is not normal
> that major mode-specific predicates are used elsewhere.

Where do you find that "not normal" guideline? 

In any case, `isearch-filter-predicate' is not a
"major mode-specific" predicate.  `isearch-mode'
is a minor mode.

And note that the first line of its doc string
says that it's for "Isearch and replace commands."
                            ^^^^^^^^^^^
___

Wrt whether `isearch-done' should restore the
original/previous value of `isearch-filter-predicate':

Yes, there's a certain logic to expecting that
`isearch-done' would do that.

In my library `isearch+.el' `isearch-done' does it.

More precisely, it does it depending on a user
option, because the library has a feature that
lets you alter the predicate on the fly, and you
sometimes want to keep the modified predicate for
subsequent searching.  (You can toggle the option
value during Isearch using `C-z S'.)

The feature: You can add and remove any number
of search filters (predicates) while searching
incrementally.

The value of `isearch-filter-predicate' is
advised by predicates that you add, creating a
suite of predicates that act together.

(AFAIK, this is the only example of dynamically,
interactively, incrementally advising a function,
in this case the function value of a variable.)

See https://www.emacswiki.org/emacs/DynamicIsearchFiltering.

This is the user option that `isearch-done' uses:

 `isearchp-auto-keep-filter-predicate-flag' is a variable
 defined in `isearch+.el'.

 Its value is nil

 Documentation:

 Non-nil means automatically apply `C-z s'.
 Changes to `isearch-filter-predicate' are automatically kept for
 subsequent searches in this Emacs session when you exit Isearch'.
 You can toggle this option using `C-z S during Isearch.

And this is the advice that implements whether to
restore the value:

(defadvice isearch-done (after isearchp-restore/update-filter-pred
                         activate)
  "Reset `isearch-filter-predicate' or `isearchp-kept-filter-predicate'.
If `isearchp-auto-keep-filter-predicate-flag' is non-nil then set
`isearchp-kept-filter-predicate' to the current value of
`isearch-filter-predicate'.  Otherwise, do the opposite."
  (if isearchp-auto-keep-filter-predicate-flag
      (setq isearchp-kept-filter-predicate  isearch-filter-predicate)
    (setq isearch-filter-predicate  isearchp-kept-filter-predicate)))

So yes, in general it makes sense for `isearch-done'
to restore the predicate value.  But it can sometimes
make sense for it not to do so.  Whether it does so
or not should be under user and programmatic control.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> > Evidently, that ship sailed a long time ago: "grep isearch-" yields
> > more than 60 hits in replace.el.  And then there are many hits in
> > comint.el, dired-aux.el, info.el, and even in simple.el.
>
> Sure. I have no issue with this. That's why I asked to add a word of
> warning about the state of affairs to the docstring. It is not normal
> that major mode-specific predicates are used elsewhere.

This is not an appropriate description of reality: replace is more or
less a part of isearch, and nearly all of the other hits are of the type
"implement isearch behavior for this mode/ this place" (e.g. for the
minibuffer in "simple.el").

In few other places high-level isearch or query-replace functions are
called directly to start a search or a query-replace.

I found not one place where that predicate is called just because the
semantics seem useful.  In most places only the variable binding is
modified.


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> The documentation for `isearch-filter-visible' sounds like no side
> effects are expected:
>
>     Predicate to filter hits of Isearch and replace commands.
>
>     Isearch hits that don't satisfy the predicate will be skipped.
>     The value should be a function of two arguments; it will be
>     called with the positions of the start and the end of the text
>     matched by Isearch and replace commands.  If this function
>     returns nil, Isearch and replace commands will continue searching
>     without stopping at resp. replacing this match.
>     This function is expected to be careful not to clobber the match data.
>
> Either the docstring is not accurate or the implementation of
> `isearch-range-invisible' is not safe.
>
> Am I missing something?

No.  It has does have side effects that are not documented.

Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Hi,
>
> I am now investigating an Org mode issue related to interaction between
> Org mode folding and isearch.
> https://list.orgmode.org/orgmode/CAP7OBx+L11ck3Ni6rv94HGU3otdj6C4rG-rMDzkwR1LTj=BWiw@mail.gmail.com/

The answer to the subject is probably: query-replace is also ok, else
"no".

> Org mode overrides the default value of isearch-filter-predicate with
>
> (defun org-fold-core--isearch-filter-predicate-overlays (beg end)
>   "Return non-nil if text between BEG and END is deemed visible by isearch.
> This function is intended to be used as `isearch-filter-predicate'."
>   (org-fold-core--create-isearch-overlays beg end) ;; trick isearch by creating overlays in place of invisible text
>   (isearch-filter-visible beg end))
>
> As you can see, Org produces side effects when the predicate is
> called.

                (if (org-fold-core-get-folding-spec-property spec :isearch-open)
	            (overlay-put o 'isearch-open-invisible #'delete-overlay)
                  (overlay-put o 'isearch-open-invisible #'ignore)
                  (overlay-put o 'isearch-open-invisible-temporary #'ignore))
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can't you bind this to a function that does what you want?  AFAIU it is
called with two arguments - the overlay itself and a bool saying "open"
(nil) or "re-hide" (non-nil).  That should allow to not rely on any
hooks to remove your helper overlays.  Unless, maybe, not all of them
are opened.  Can that happen?

If it does, maybe using `post-command-hook' would be a better choice
than `isearch-end-hook' (which is, obviously, not a good choice)?


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: emacs-devel@gnu.org
> Date: Thu, 01 Jun 2023 01:17:57 +0200
> 
> Ihor Radchenko <yantar92@posteo.net> writes:
> 
> > The documentation for `isearch-filter-visible' sounds like no side
> > effects are expected:
> >
> >     Predicate to filter hits of Isearch and replace commands.
> >
> >     Isearch hits that don't satisfy the predicate will be skipped.
> >     The value should be a function of two arguments; it will be
> >     called with the positions of the start and the end of the text
> >     matched by Isearch and replace commands.  If this function
> >     returns nil, Isearch and replace commands will continue searching
> >     without stopping at resp. replacing this match.
> >     This function is expected to be careful not to clobber the match data.
> >
> > Either the docstring is not accurate or the implementation of
> > `isearch-range-invisible' is not safe.
> >
> > Am I missing something?
> 
> No.  It has does have side effects that are not documented.

The question is: why do we have to document every possible nit of the
internals?  I still don't think I understand the rationale.  People
who want to know _everything_ about the code should consult the source
code; it's the power of Free Software that they can do it.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Juri Linkov]

> Org produces its own side effects to make isearch
> support revealing text hidden via text properties.

When your isearch-filter-predicate can't be improved to be
usable also during query-replace, then probably you need to
set isearch-filter-predicate in isearch-mode-hook, and remove
afterwards in isearch-mode-end-hook.

> I guess the answer to my question about `isearch-filter-predicate' is
> yes - it may be called outside isearch and thus custom
> `isearch-filter-predicate' must consider scenarios outside isearch.
>
> It would be nice if the docstring of `isearch-filter-predicate'
> mentioned that it might be called outside isearch-mode.

isearch.el is both a core library and UI with the search commands.
UI commands for isearch-mode are enabled by C-s, etc.
But as a core library its functions including isearch-filter-predicate
are used by other packages such as replace.el.

RE: [External] : Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Drew Adams <drew.adams@oracle.com> writes:

>> Sure. I have no issue with this. That's why I asked to add a word of
>> warning about the state of affairs to the docstring. It is not normal
>> that major mode-specific predicates are used elsewhere.
>
> Where do you find that "not normal" guideline? 

That's my intuition. Is some major or minor mode is defining some
customizable predicate function, it is probably going to be called by
that mode with mode-specific settings available to that function.

> In any case, `isearch-filter-predicate' is not a
> "major mode-specific" predicate.  `isearch-mode'
> is a minor mode.

Sure. But I still find calling the predicate outside isearch unexpected.

> And note that the first line of its doc string
> says that it's for "Isearch and replace commands."
>                             ^^^^^^^^^^^

When reading this docstring, I though that "replace" is referring to
`isearch-query-replace'.

> Wrt whether `isearch-done' should restore the
> original/previous value of `isearch-filter-predicate':

I think there is some misunderstanding. I did not intend to mention
restoring the original/previous values.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> I am now investigating an Org mode issue related to interaction between
>> Org mode folding and isearch.
>> https://list.orgmode.org/orgmode/CAP7OBx+L11ck3Ni6rv94HGU3otdj6C4rG-rMDzkwR1LTj=BWiw@mail.gmail.com/
>
> The answer to the subject is probably: query-replace is also ok, else
> "no".

AFAIK, at least evil does it:
https://github.com/emacs-evil/evil/blob/be736b8dbc468d53e6cb3e76db66c1141330b030/evil-search.el#L183

I asked them to respect `isearch-mode-end-hook' in
https://github.com/emacs-evil/evil/issues/1630, but after seeing
`query-replace' I started to doubt my confidence.

>> Org mode overrides the default value of isearch-filter-predicate with
>>
>> (defun org-fold-core--isearch-filter-predicate-overlays (beg end)
>>   "Return non-nil if text between BEG and END is deemed visible by isearch.
>> This function is intended to be used as `isearch-filter-predicate'."
>>   (org-fold-core--create-isearch-overlays beg end) ;; trick isearch by creating overlays in place of invisible text
>>   (isearch-filter-visible beg end))
>>
>> As you can see, Org produces side effects when the predicate is
>> called.
>
>                 (if (org-fold-core-get-folding-spec-property spec :isearch-open)
> 	            (overlay-put o 'isearch-open-invisible #'delete-overlay)
>                   (overlay-put o 'isearch-open-invisible #'ignore)
>                   (overlay-put o 'isearch-open-invisible-temporary #'ignore))
>                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Can't you bind this to a function that does what you want?  AFAIU it is
> called with two arguments - the overlay itself and a bool saying "open"
> (nil) or "re-hide" (non-nil).  That should allow to not rely on any
> hooks to remove your helper overlays.  Unless, maybe, not all of them
> are opened.  Can that happen?

I tried... But it is prohibited to remove overlays when isearch is
tracking them: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60399
Also, closing an overlay does not necessarily mean that isearch is over
- Org cannot yet convert it back to text properties.

That said, the idea is good for other reasons.

> If it does, maybe using `post-command-hook' would be a better choice
> than `isearch-end-hook' (which is, obviously, not a good choice)?

Surely not. `post-command-hook' is triggered after each
`self-insert-command' during isearch/query-replace.

I had to advice `isearch-clean-overlays', which appears to be called by
all the users of `isearch-filter-predicate' (replace-regexp, evil-mode,
and swipet). Still a workaround. Ugly.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=7dee2c07f

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

>> Org produces its own side effects to make isearch
>> support revealing text hidden via text properties.
>
> When your isearch-filter-predicate can't be improved to be
> usable also during query-replace, then probably you need to
> set isearch-filter-predicate in isearch-mode-hook, and remove
> afterwards in isearch-mode-end-hook.

Good point. Although I want things to work in query-replace as well.

>> It would be nice if the docstring of `isearch-filter-predicate'
>> mentioned that it might be called outside isearch-mode.
>
> isearch.el is both a core library and UI with the search commands.
> UI commands for isearch-mode are enabled by C-s, etc.
> But as a core library its functions including isearch-filter-predicate
> are used by other packages such as replace.el.

Which is not documented. I am asking this fact to be documented.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> Sure. I have no issue with this. That's why I asked to add a word of
>> warning about the state of affairs to the docstring. It is not normal
>> that major mode-specific predicates are used elsewhere.
>
> This is not an appropriate description of reality: replace is more or
> less a part of isearch, and nearly all of the other hits are of the type
> "implement isearch behavior for this mode/ this place" (e.g. for the
> minibuffer in "simple.el").

Yes. And they selectively use some parts of isearch but not other. If a
third-party library is to re-bind `isearch-filter-predicate' what should
it expect about the environment? For now, it appears that even though
`isearch-mode-end-hook' is not always called, `isearch-clean-overlays'
does. But it may not be in future, for example. Potentially breaking the
working code. More accurate documentation would at least provide a guide
what to expect.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Juri Linkov]

>>       (if (org-fold-core-get-folding-spec-property spec :isearch-open)
>>        (overlay-put o 'isearch-open-invisible #'delete-overlay)
>>         (overlay-put o 'isearch-open-invisible #'ignore)
>>         (overlay-put o 'isearch-open-invisible-temporary #'ignore))
>>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> Can't you bind this to a function that does what you want?  AFAIU it is
>> called with two arguments - the overlay itself and a bool saying "open"
>> (nil) or "re-hide" (non-nil).  That should allow to not rely on any
>> hooks to remove your helper overlays.  Unless, maybe, not all of them
>> are opened.  Can that happen?
>
> I tried... But it is prohibited to remove overlays when isearch is
> tracking them: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60399

If it helps, you could design a new property that will obsolete the
deficient 'isearch-open-invisible-temporary'.  For a few releases
the old property should be supported, but then eventually it will
simplify code in Org-mode.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > No.  It has does have side effects that are not documented.
>
> The question is: why do we have to document every possible nit of the
> internals?  I still don't think I understand the rationale.  People
> who want to know _everything_ about the code should consult the source
> code; it's the power of Free Software that they can do it.

It would not harm in this marginal case, but keeping all
such things up to date is probably beyond our capabilities.

And to prevent this specific problem in Org would have required
documenting a lot of more or less internal things.  In this case it is
indeed better to consult the code.

Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> > If it does, maybe using `post-command-hook' would be a better choice
> > than `isearch-end-hook' (which is, obviously, not a good choice)?
>
> Surely not. `post-command-hook' is triggered after each
> `self-insert-command' during isearch/query-replace.

Of course the idea is not to so something blindly.  One could check
whether isearch is still active and which overlays need to be kept.

> I had to advice `isearch-clean-overlays', which appears to be called by
> all the users of `isearch-filter-predicate' (replace-regexp, evil-mode,
> and swipet). Still a workaround. Ugly.

Hmm.  Maybe the optimum.

BTW, did you consider to :before advice the local variable
`isearch-filter-predicate' instead of overwriting it?  That would be a
bit nicer for users and other modes that also need to change it.


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Yes. And they selectively use some parts of isearch but not other. If a
> third-party library is to re-bind `isearch-filter-predicate' what should
> it expect about the environment? For now, it appears that even though
> `isearch-mode-end-hook' is not always called, `isearch-clean-overlays'
> does. But it may not be in future, for example. Potentially breaking the
> working code. More accurate documentation would at least provide a guide
> what to expect.

Your case is special, though, since you need to hack _into_ an internal
Isearch mechanism, your goal is the interoperation with isearch code.

To me the appropriate solution is clearly: extend Isearch so that it can
handle the invisible text property itself (or provide a clear interface
that allows to implement that cleanly).  Thos would be a better investment
of time, more useful and maybe even not much harder than documenting all
of those details.


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

>> I tried... But it is prohibited to remove overlays when isearch is
>> tracking them: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=60399
>
> If it helps, you could design a new property that will obsolete the
> deficient 'isearch-open-invisible-temporary'.  For a few releases
> the old property should be supported, but then eventually it will
> simplify code in Org-mode.

I am not yet sure how to approach this re-design. Will think about it.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> To me the appropriate solution is clearly: extend Isearch so that it can
> handle the invisible text property itself (or provide a clear interface
> that allows to implement that cleanly).  Thos would be a better investment
> of time, more useful and maybe even not much harder than documenting all
> of those details.

Sure. I have a patch lying around. But need to rebase it onto master at
this point.

I would still need to deal with the existing state of affairs though -
Org has to work with older Emacs versions as well.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> Surely not. `post-command-hook' is triggered after each
>> `self-insert-command' during isearch/query-replace.
>
> Of course the idea is not to so something blindly.  One could check
> whether isearch is still active and which overlays need to be kept.

This may work for isearch, but not in query-replace, evil, swiper and
other places where isearch machinery is used. Or I will have to keep
things up-to-date against every new user of isearch machinery. Not good
idea, IMHO.

>> I had to advice `isearch-clean-overlays', which appears to be called by
>> all the users of `isearch-filter-predicate' (replace-regexp, evil-mode,
>> and swipet). Still a workaround. Ugly.
>
> Hmm.  Maybe the optimum.
>
> BTW, did you consider to :before advice the local variable
> `isearch-filter-predicate' instead of overwriting it?  That would be a
> bit nicer for users and other modes that also need to change it.

Do you refer to
 (add-function :before (local 'isearch-filter-predicate) #'foo)
?

Two reasons:
1. I only vaguely understand how this works
2. It feels against the interface. If advising this predicate is
   expected, why not convert it into an abnormal hook?

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

RE: [External] : Re: Is it valid to call isearch-filter-predicate outside isearch?

[Drew Adams]

> Do you refer to
>  (add-function :before
>                (local 'isearch-filter-predicate)
>                #'foo)?
> 
> Two reasons:
> 1. I only vaguely understand how this works
> 2. It feels against the interface. If advising this predicate is
>    expected, why not convert it into an abnormal hook?

Caveat: Not really following this thread.  Might be misunderstanding you here.

Wrt #2: A hook doesn't provide the same flexibility as advice, e.g., before, after, around.

(Wrt #1: Yes, advice is trickier than hooks.)

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Do you refer to
>  (add-function :before (local 'isearch-filter-predicate) #'foo)
> ?

Yes.

> Two reasons:
> 1. I only vaguely understand how this works

This can be changed.  Should.

> 2. It feels against the interface. If advising this predicate is
>    expected, why not convert it into an abnormal hook?

It's more flexible and expressive, as Drew already mentioned.  For
example, how the members of a hook are logically combined (`and'ed,
`or'ed) is fixed in a hook, but not when using advising.

I also wonder about the `kill-variable' calls: what if the user or a
third-party mode want to have own buffer-local settings for these?  We then erase
them when killing the local variables.  With using an advice on these
the worst thing that could happen is that we leave a buffer local
variable with the same binding as the global one, where we started with
no buffer local binding.

Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> 2. It feels against the interface. If advising this predicate is
>>    expected, why not convert it into an abnormal hook?
>
> It's more flexible and expressive, as Drew already mentioned.  For
> example, how the members of a hook are logically combined (`and'ed,
> `or'ed) is fixed in a hook, but not when using advising.

Interesting.
From Elisp Tips in the manual, I always felt that using advices is
always frowned upon. And you are suggesting that they are the better way
to go in these situations.
I am wondering if this thing with modifying predicates should be
documented somewhere and recommended approach.

> I also wonder about the `kill-variable' calls: what if the user or a
> third-party mode want to have own buffer-local settings for these?  We then erase
> them when killing the local variables.  With using an advice on these
> the worst thing that could happen is that we leave a buffer local
> variable with the same binding as the global one, where we started with
> no buffer local binding.

May you please elaborate? I am not sure what `kill-variable' calls you
are referring to here.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
> >> 2. It feels against the interface. If advising this predicate is
> >>    expected, why not convert it into an abnormal hook?
> >
> > It's more flexible and expressive, as Drew already mentioned.  For
> > example, how the members of a hook are logically combined (`and'ed,
> > `or'ed) is fixed in a hook, but not when using advising.
>
> Interesting.
> From Elisp Tips in the manual, I always felt that using advices is
> always frowned upon. And you are suggesting that they are the better way
> to go in these situations.
> I am wondering if this thing with modifying predicates should be
> documented somewhere and recommended approach.

Not sure if we have a consensual approach at all.  But Note this is not
about modifying predicates, it's about modifying variables with function
bindings (in my eyes).  Yes, we don't want to use advices to change
named functions.

But when speaking about modifying function valued variables (or places)
advising has clear advantages: getting to the old value works more
transparently, and interfering actors have some more ways to control
their interaction.

I guess we will have to decide by case whether introducing a hook or
using advices on function valued variables or overwriting a binding with
plain `setq' makes more sense.

But maybe let's continue discussing this in the thread that Drew has
started.


> > I also wonder about the `kill-variable' calls: what if the user or a
> > third-party mode want to have own buffer-local settings for these?
> > We then erase
> > them when killing the local variables.  With using an advice on these
> > the worst thing that could happen is that we leave a buffer local
> > variable with the same binding as the global one, where we started with
> > no buffer local binding.
>
> May you please elaborate? I am not sure what `kill-variable' calls you
> are referring to here.

To those in `wdired-change-to-dired-mode:

#+begin_src emacs-lisp
  (when wdired-search-replace-filenames
    (remove-function (local 'isearch-search-fun-function)
                     #'dired-isearch-search-filenames)
    (kill-local-variable 'replace-search-function)
    (kill-local-variable 'replace-re-search-function)
    ;; Restore dired hook
    (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t))
#+end_src

This demonstrates a problem when not using an advice: to get rid of the
temporary new binding you simply kill the local variable, but this can
be problematic, especially when we did not create it (but maybe someone
third).


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
> >> 2. It feels against the interface. If advising this predicate is
> >>    expected, why not convert it into an abnormal hook?
> >
> > It's more flexible and expressive, as Drew already mentioned.  For
> > example, how the members of a hook are logically combined (`and'ed,
> > `or'ed) is fixed in a hook, but not when using advising.
>
> Interesting.  From Elisp Tips in the manual, I always felt that using
> advices is always frowned upon. And you are suggesting that they are
> the better way to go in these situations.  I am wondering if this
> thing with modifying predicates should be documented somewhere and
> recommended approach.

Though, "better" does not necessarily mean "best".

Why do we have to change the bindings (of those diverse Isearch
variables) at all?  If Isearch behaves specially in Dired, and we maybe
consider WDired a submode of Dired, we could say: we make the respective
variables permanently buffer local in Dired buffers and bind them to a
function that always works in all cases: the bound functions could just
test whether the buffer is in dired-mode or wdired-mode, and whether
`dired-isearch-filenames-mode' is enabled, and do the right thing.

Would that be possible?


Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> May you please elaborate? I am not sure what `kill-variable' calls you
>> are referring to here.
>
> To those in `wdired-change-to-dired-mode:
>
> #+begin_src emacs-lisp
>   (when wdired-search-replace-filenames
>     (remove-function (local 'isearch-search-fun-function)
>                      #'dired-isearch-search-filenames)
>     (kill-local-variable 'replace-search-function)
>     (kill-local-variable 'replace-re-search-function)
>     ;; Restore dired hook
>     (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t))
> #+end_src
>
> This demonstrates a problem when not using an advice: to get rid of the
> temporary new binding you simply kill the local variable, but this can
> be problematic, especially when we did not create it (but maybe someone
> third).

The code above is indeed slightly problematic.
I'd store the old value somewhere in such situations and restore later.

May `define-advice' be modified to work on variable values? That way,
one can override the variable value and later restore the old one using
the existing machinery.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> >> May you please elaborate? I am not sure what `kill-variable' calls you
> >> are referring to here.
> >
> > To those in `wdired-change-to-dired-mode:
> >
> > #+begin_src emacs-lisp
> >   (when wdired-search-replace-filenames
> >     (remove-function (local 'isearch-search-fun-function)
> >                      #'dired-isearch-search-filenames)
> >     (kill-local-variable 'replace-search-function)
> >     (kill-local-variable 'replace-re-search-function)
> >     ;; Restore dired hook
> >     (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t))
> > #+end_src
> >
> > This demonstrates a problem when not using an advice: to get rid of the
> > temporary new binding you simply kill the local variable, but this can
> > be problematic, especially when we did not create it (but maybe someone
> > third).
>
> The code above is indeed slightly problematic.
> I'd store the old value somewhere in such situations and restore later.

Now I'm a bit confused.  Is advising not exactly the perfect solution
for that problem because it allows to avoid explicit saving and
restoring (which is one reason why I'm suggesting to use advising, plus:
overlapping modification and restore operations don't collide), and:

> May `define-advice' be modified to work on variable values? That way,
> one can override the variable value and later restore the old one using
> the existing machinery.

What's wrong with `add-function' and `remove-function' (which work on
variable values)?

Seems you misunderstood what I'm suggesting, or you misunderstand how
advising works.

Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Do you refer to
>>  (add-function :before (local 'isearch-filter-predicate) #'foo)
>> ?
>
> Yes.
>
>> Two reasons:
>> 1. I only vaguely understand how this works
>
> This can be changed.  Should.

I tried
(add-function :before (local isearch-filter-predicate) #'org-fold-core--create-isearch-overlays)
and it somehow had no effect.

Steps I tried:

1. emacs -Q
2. Open a text file 1.txt with some text

A text file 1.txt with some text

3. M-: (defun yant/test (&rest _) (message "Foo"))
4. M-: (add-function :before (local isearch-filter-predicate) #'yant/test)
5. C-s txt <RET>
6. Switch to *Messages* buffer
7. Observe no "Foo" message

What am I missing?

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> May `define-advice' be modified to work on variable values? That way,
>> one can override the variable value and later restore the old one using
>> the existing machinery.
>
> What's wrong with `add-function' and `remove-function' (which work on
> variable values)?
>
> Seems you misunderstood what I'm suggesting, or you misunderstand how
> advising works.

I am suggesting variable non-function values.
For example, consider (defcustom foo "value1" ...) that should be replaced
with "value2" while a minor mode is active.

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

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Michael Heerdegen]

Ihor Radchenko <yantar92@posteo.net> writes:

> I tried
> (add-function :before (local isearch-filter-predicate)
> #'org-fold-core--create-isearch-overlays)
> [...]
> What am I missing?

Only the quote before the variable name:

(add-function :before (local 'isearch-filter-predicate) ...)
                             ^
Michael.

Re: Is it valid to call isearch-filter-predicate outside isearch?

[Ihor Radchenko]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> (add-function :before (local isearch-filter-predicate)
>> #'org-fold-core--create-isearch-overlays)
>> [...]
>> What am I missing?
>
> Only the quote before the variable name:
>
> (add-function :before (local 'isearch-filter-predicate) ...)

Thanks!