bug#14714: 24.3.50; `isearch-filter-predicate(s)'

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Drew Adams]

Problem similar to that of bug #14712, regarding obsolescence, but I
think there is a code problem also.

1. `isearch-filter-predicates' should not be an _alias_ for
`isearch-filter-predicate'.  The former cannot just replace the latter.

2. I think one cannot currently use a single predicate as the value of
`isearch-filter-predicates'.  There is one place in the code where that
might work, because of this:

(if (consp isearch-filter-predicates)
    isearch-filter-predicates
  (list isearch-filter-predicates))

(But what if isearch-filter-predicates is nil?  You get `(nil)', which I
think is wrong.)

And in other places it should not work at all.  E.g.:

(run-hook-with-args-until-failure
  'isearch-filter-predicates
  (match-beginning 0) (match-end 0))

To make it work throughout, I think you would need to do something like this:

(when (and isearch-filter-predicates
           (atom isearch-filter-predicates))
  (setq isearch-filter-predicates (list isearch-filter-predicates)))

IOW, at the outset, convert an atomic (and non-nil) value to a cons.






In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-06-20 on ODIEONE
Bzr revision: 113100 eliz@gnu.org-20130620173624-w9v620tog4yacftk
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 CFLAGS=-O0 -g3 LDFLAGS=-Lc:/Devel/emacs/lib
 CPPFLAGS=-Ic:/Devel/emacs/include'

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

> 1. `isearch-filter-predicates' should not be an _alias_ for
> `isearch-filter-predicate'.  The former cannot just replace the latter.

Actually, shouldn't we revert this change and use
(add-function :before-while ...) on isearch-filter-predicate instead?
Admittedly, currently nadvice.el does not give you access to the list of
functions so you can't directly do the equivalent of

		   (mapconcat (lambda (s)
				(and (symbolp s)
				     (get s 'isearch-message-prefix)))
			      (if (consp isearch-filter-predicates)
				  isearch-filter-predicates
				(list isearch-filter-predicates))
			      "")

but that could be remedied (even using an advice's property rather than
a symbol property on the function added as advice).


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Drew Adams]

> Actually, shouldn't we revert this change and use
> (add-function :before-while ...) on isearch-filter-predicate instead?

Why should we do that?  Why is that preferable to using a list of predicates as we do now?

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

>> Actually, shouldn't we revert this change and use
>> (add-function :before-while ...) on isearch-filter-predicate instead?
> Why should we do that?

Because it's better.

The real question is "why shouldn't we".  Jury?


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Drew Adams]

> >> Actually, shouldn't we revert this change and use
> >> (add-function :before-while ...) on isearch-filter-predicate instead?
> >
> > Why should we do that?
> 
> Because it's better.

You think it is better, clearly.  No one questioned that.

Can you please say why you think so?  Do you imagine it is obvious?
It is not, to me at least.

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

Drew Adams wrote:
> Can you please say why you think so?

Please don't hijack this discussion.  Juri, could you answer
my question?


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Drew Adams]

> > Can you please say why you think so?
> > Do you imagine it is obvious?  It is not, to me at least.
> 
> Please don't hijack this discussion.

I am not hijacking anything.  I am the one who filed this bug report.

And I proposed a simple fix for it from the outset - a minor correction of what Juri had already coded and along the lines of what has been done in the past.

You proposed a different fix.  Fine.  But you gave no reason - nothing about any problems with the more usual approach or any advantages of your new approach.

I asked about that.  I am interested in learning the advantages of your novel approach of (IIUC) advising a single hook function as a substitute for allowing for a sequence of hook functions.

I did not say that your solution is not a good one.  I assume the contrary, in fact.  I am asking to understand why you think it is a better one.  That's all.

Understanding this can help with other code as well, presumably.  Is there a reason why you do not want to share your understanding?

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

>> > Can you please say why you think so?
>> > Do you imagine it is obvious?  It is not, to me at least.
>> Please don't hijack this discussion.
> I am not hijacking anything.

You are: the discussion is about problems with the way
isearch-filter-predicate was made obsolete and I suggest to simply fix
it by reverting this "obsoletion".

If you want to discuss the merits/demerits of add-function, do
it elsewhere.


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Juri Linkov]

> Actually, shouldn't we revert this change and use
> (add-function :before-while ...) on isearch-filter-predicate instead?

Currently I'm exploring possibilities of using `:before-while'
instead of `run-hook-with-args-until-failure'.

IIUC, it can be used to replace hooks, so that for example,
when someone wants to put an additional function on `find-file',
it's possible to use `add-function' instead of appending
that function to `find-file-hook' (or `find-file-hooks'
that is an obsolete alias of `find-file-hook').

But in case of `isearch-filter-predicate', I don't understand
where and on which function to put additional predicate functions.
`isearch-filter-predicate' is a variable, not a function,
so it can't be used for the PLACE argument of `add-function'.
What I'm trying is:

(defvar isearch-filter-predicate nil)
(add-function :before-while isearch-filter-predicate (lambda (b e) (message "b")))
(add-function :after-while  isearch-filter-predicate (lambda (b e) (message "a")))
(funcall isearch-filter-predicate 1 2)

This fails with the error: (void-function nil)

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

>> Actually, shouldn't we revert this change and use
>> (add-function :before-while ...) on isearch-filter-predicate instead?

> Currently I'm exploring possibilities of using `:before-while'
> instead of `run-hook-with-args-until-failure'.

> IIUC, it can be used to replace hooks, so that for example,
> when someone wants to put an additional function on `find-file',

No, you're thinking of advice-add.  add-function is different (tho it's
used internally by advice-add).
Basically, replace

    (add-hook 'isearch-filter-predicate #'foo nil t)
with
    (add-function :before-while (local isearch-filter-predicate) #'foo)

> (defvar isearch-filter-predicate nil)

For add-function to work well, you want to change this default value to
be a function, such as (lambda () t).


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Juri Linkov]

> Basically, replace
>
>     (add-hook 'isearch-filter-predicate #'foo nil t)
> with
>     (add-function :before-while (local isearch-filter-predicate) #'foo)
>
>> (defvar isearch-filter-predicate nil)
>
> For add-function to work well, you want to change this default value to
> be a function, such as (lambda () t).

I'm trying this, but not sure how to implement toggling like
in `dired-isearch-filenames-toggle':

  (setq isearch-filter-predicates
	(if (memq 'dired-isearch-filter-filenames isearch-filter-predicates)
	    (delq 'dired-isearch-filter-filenames isearch-filter-predicates)
	  (cons 'dired-isearch-filter-filenames isearch-filter-predicates)))

Whereas it's clear how to add and remove an advice function, it's unclear
how to check its existence instead of `memq'.  Is `advice-member-p' intended
to do what `memq' does?  When I tried

  (defvar-local isearch-filter-predicate (lambda (b e) t))
  (defun test-b (b e) (message "b"))
  (defun test-a (b e) (message "a"))
  (add-function :before-while (local isearch-filter-predicate) #'test-b)
  (add-function :after-while  (local isearch-filter-predicate) #'test-a)

then

  (funcall isearch-filter-predicate 1 2)

works fine, but

  (advice-member-p #'test-a 'isearch-filter-predicate)

returns nil.

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

> works fine, but
>   (advice-member-p #'test-a 'isearch-filter-predicate)
> returns nil.

Indeed, advice-member-p is for advice-add but there's no equivalent for
add-function.  Same kind of problem (tho reverted) for the new
advice-mapc.
Hmm... I think I see how to solve these problems, a patch should follow
"shortishly",


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

>> works fine, but
>> (advice-member-p #'test-a 'isearch-filter-predicate)
>> returns nil.

> Indeed, advice-member-p is for advice-add but there's no equivalent for
> add-function.  Same kind of problem (tho reverted) for the new
> advice-mapc.
> Hmm... I think I see how to solve these problems, a patch should follow
> "shortishly",

You should now be able to use

    (advice-function-member-p #'test-a isearch-filter-predicate)
and
    (advice-function-mapc <somefunc> isearch-filter-predicate)


        Stefan

bug#14714: 24.3.50; `isearch-filter-predicate(s)'

[Stefan Monnier]

I have now reverted to isearch-filter-predicate.

>> works fine, but
>> (advice-member-p #'test-a 'isearch-filter-predicate)
>> returns nil.

Actually, I changed the code in a way that doesn't require
advice-(function-)member-p.


        Stefan