bug#1352: 23.0.60; isearch-success-function

bug#1352: 23.0.60; isearch-success-function

[Drew Adams]

The name of `isearch-success-function' seems wrong, if we believe the
doc string. The doc string suggests that it is a predicate that
filters the search hits that would otherwise be available. That's very
general. If that's true, then the name should reflect this meaning -
perhaps `isearch-predicate'.
 
Similarly, the name of `isearch-success-function-default' should be
something that suggests its very limited meaning: it filters search
hits to those that are visible, unless invisible text too can be
searched. It is a particular kind of search filter, and its name
should reflect that particularity.
 
Also, in `isearch-search', there is this:
 
;; Clear RETRY unless we matched some invisible text
;; and we aren't supposed to do that.
(if (or (not isearch-success)
        (bobp) (eobp)
        (= (match-beginning 0) (match-end 0))
        (funcall isearch-success-function
                 (match-beginning 0) (match-end 0)))
    (setq retry nil)))
 
The code here is general; it is not related to text visibility. So the
comment is not general enough. It is a vestige that is appropriate now
only when the value of `isearch-success-function' is
`isearch-success-function-default'. The comment should instead say
that we clear RETRY unless the search predicate says to skip this
search hit.
 
It seems like this is a partly finished enhancement to behavior that
was originally only for searching invisible text. Seems like this was
generalized to an arbitrary predicate that filters possible search
hits - but the job seems only half done. The code presumably works in
a general way, but the comments and symbol names are inappropriate.
 
The comments and the variable and function names need to reflect the
new behavior, and this behavior should be documented for users. Users
should know how to take advantage of this feature, defining their own
search predicates that filter the search hits that would normally be
available, so that hits that dissatisfy the predicate are skipped.
 

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-11-08 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'
 

bug#1352: 23.0.60; isearch-success-function

[Stefan Monnier]

> The name of `isearch-success-function' seems wrong, if we believe the
> doc string. The doc string suggests that it is a predicate that
> filters the search hits that would otherwise be available. That's very
> general. If that's true, then the name should reflect this meaning -
> perhaps `isearch-predicate'.

Actually, based on your description, I'd name it `isearch-filter-predicate'.
 

        Stefan

bug#1352: 23.0.60; isearch-success-function

[Drew Adams]

> > The name of `isearch-success-function' seems wrong, if we 
> > believe the doc string. The doc string suggests that it is
> > a predicate that filters the search hits that would
> > otherwise be available. That's very general. If that's true,
> > then the name should reflect this meaning -
> > perhaps `isearch-predicate'.
> 
> Actually, based on your description, I'd name it 
> `isearch-filter-predicate'.

Sound good to me. 

(Although by the "principe des belles oranges", an Isearch predicate could
pretty much only be for filtering.) 

bug#1352: 23.0.60; isearch-success-function

[Juri Linkov]

>> The name of `isearch-success-function' seems wrong, if we believe the
>> doc string. The doc string suggests that it is a predicate that
>> filters the search hits that would otherwise be available. That's very
>> general. If that's true, then the name should reflect this meaning -
>> perhaps `isearch-predicate'.
>
> Actually, based on your description, I'd name it `isearch-filter-predicate'.

The name `isearch-success-function' was created to be consistent
with other *-function variables and with `isearch-success',
but I agree that more clear names are better.  So the patch below
renames it to `isearch-filter-predicate' and also renames
`isearch-success-function-default' to `isearch-filter-invisible',
plus renames in Dired and Info, and doc fixes:

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.335
diff -c -r1.335 isearch.el
*** lisp/isearch.el	11 Nov 2008 20:11:34 -0000	1.335
--- lisp/isearch.el	16 Nov 2008 21:07:27 -0000
***************
*** 176,186 ****
    "Function to save a function restoring the mode-specific isearch state
  to the search status stack.")
  
! (defvar isearch-success-function 'isearch-success-function-default
!   "Function to report whether the new search match is considered successful.
! The function has two arguments: the positions of start and end of text
! matched by the search.  If this function returns nil, continue
! searching without stopping at this match.")
  
  ;; Search ring.
  
--- 176,187 ----
    "Function to save a function restoring the mode-specific isearch state
  to the search status stack.")
  
! (defvar isearch-filter-predicate 'isearch-filter-invisible
!   "Predicate that filters the search hits that would normally be available.
! Search hits that dissatisfy the predicate are skipped.  The function
! has two arguments: the positions of start and end of text matched by
! the search.  If this function returns nil, continue searching without
! stopping at this match.")
  
  ;; Search ring.
  
***************
*** 2257,2263 ****
  	    (isearch-no-upper-case-p isearch-string isearch-regexp)))
    (condition-case lossage
        (let ((inhibit-point-motion-hooks
! 	     (and (eq isearch-success-function 'isearch-success-function-default)
  		  search-invisible))
  	    (inhibit-quit nil)
  	    (case-fold-search isearch-case-fold-search)
--- 2258,2264 ----
  	    (isearch-no-upper-case-p isearch-string isearch-regexp)))
    (condition-case lossage
        (let ((inhibit-point-motion-hooks
! 	     (and (eq isearch-filter-predicate 'isearch-filter-invisible)
  		  search-invisible))
  	    (inhibit-quit nil)
  	    (case-fold-search isearch-case-fold-search)
***************
*** 2267,2278 ****
  	(while retry
  	  (setq isearch-success
  		(isearch-search-string isearch-string nil t))
! 	  ;; Clear RETRY unless we matched some invisible text
! 	  ;; and we aren't supposed to do that.
  	  (if (or (not isearch-success)
  		  (bobp) (eobp)
  		  (= (match-beginning 0) (match-end 0))
! 		  (funcall isearch-success-function
  			   (match-beginning 0) (match-end 0)))
  	      (setq retry nil)))
  	(setq isearch-just-started nil)
--- 2268,2279 ----
  	(while retry
  	  (setq isearch-success
  		(isearch-search-string isearch-string nil t))
! 	  ;; Clear RETRY unless the search predicate says
! 	  ;; to skip this search hit.
  	  (if (or (not isearch-success)
  		  (bobp) (eobp)
  		  (= (match-beginning 0) (match-end 0))
! 		  (funcall isearch-filter-predicate
  			   (match-beginning 0) (match-end 0)))
  	      (setq retry nil)))
  	(setq isearch-just-started nil)
***************
*** 2451,2460 ****
  		  nil)
  	      (setq isearch-hidden t)))))))
  
! (defun isearch-success-function-default (beg end)
!   "Default function to report if the new search match is successful.
! Returns t if search can match hidden text, or otherwise checks if some
! text from BEG to END is visible."
    (or (eq search-invisible t)
        (not (isearch-range-invisible beg end))))
  
--- 2452,2461 ----
  		  nil)
  	      (setq isearch-hidden t)))))))
  
! (defun isearch-filter-invisible (beg end)
!   "Default predicate to filter out invisible text.
! It filters search hits to those that are visible (at least partially),
! unless invisible text too can be searched."
    (or (eq search-invisible t)
        (not (isearch-range-invisible beg end))))
  
***************
*** 2640,2651 ****
  			  (if isearch-lazy-highlight-wrapped
  			      isearch-lazy-highlight-end
  			    (window-start))))))
! 	;; Use a loop like in `isearch-search'
  	(while retry
  	  (setq success (isearch-search-string
  			 isearch-lazy-highlight-last-string bound t))
  	  (if (or (not success)
! 		  (funcall isearch-success-function
  			   (match-beginning 0) (match-end 0)))
  	      (setq retry nil)))
  	success)
--- 2642,2655 ----
  			  (if isearch-lazy-highlight-wrapped
  			      isearch-lazy-highlight-end
  			    (window-start))))))
! 	;; Use a loop like in `isearch-search'.
  	(while retry
  	  (setq success (isearch-search-string
  			 isearch-lazy-highlight-last-string bound t))
+ 	  ;; Clear RETRY unless the search predicate says
+ 	  ;; to skip this search hit.
  	  (if (or (not success)
! 		  (funcall isearch-filter-predicate
  			   (match-beginning 0) (match-end 0)))
  	      (setq retry nil)))
  	success)

Index: lisp/dired-aux.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/dired-aux.el,v
retrieving revision 1.181
diff -c -r1.181 dired-aux.el
*** lisp/dired-aux.el	11 Nov 2008 20:12:44 -0000	1.181
--- lisp/dired-aux.el	16 Nov 2008 21:12:49 -0000
***************
*** 2303,2328 ****
    :group 'dired
    :version "23.1")
  
! (defvar dired-isearch-orig-success-function nil)
  
  (defun dired-isearch-filenames-toggle ()
    "Toggle file names searching on or off.
! When on, Isearch checks the success of the current matching point
! using the function `dired-isearch-success-function' that matches only
! at file names.  When off, it uses the default function
! `isearch-success-function-default'."
    (interactive)
!   (setq isearch-success-function
! 	(if (eq isearch-success-function 'dired-isearch-success-function)
! 	    'isearch-success-function-default
! 	  'dired-isearch-success-function))
    (setq isearch-success t isearch-adjusted t)
    (isearch-update))
  
--- 2303,2328 ----
    :group 'dired
    :version "23.1")
  
! (defvar dired-isearch-filter-predicate-orig nil)
  
  (defun dired-isearch-filenames-toggle ()
    "Toggle file names searching on or off.
! When on, Isearch skips matches outside file names using the predicate
! `dired-isearch-filter-filenames' that matches only at file names.
! When off, it uses the default predicate `isearch-filter-invisible'."
    (interactive)
!   (setq isearch-filter-predicate
! 	(if (eq isearch-filter-predicate 'dired-isearch-filter-filenames)
! 	    'isearch-filter-invisible
! 	  'dired-isearch-filter-filenames))
    (setq isearch-success t isearch-adjusted t)
    (isearch-update))
  
***************
*** 2330,2351 ****
  (defun dired-isearch-filenames-setup ()
    "Set up isearch to search in Dired file names.
  Intended to be added to `isearch-mode-hook'."
!   (when dired-isearch-filenames
      (define-key isearch-mode-map "\M-sf" 'dired-isearch-filenames-toggle)
!     (setq dired-isearch-orig-success-function
! 	  (default-value 'isearch-success-function))
!     (setq-default isearch-success-function 'dired-isearch-success-function)
      (add-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end nil t)))
  
  (defun dired-isearch-filenames-end ()
    "Clean up the Dired file name search after terminating isearch."
    (define-key isearch-mode-map "\M-sf" nil)
!   (setq-default isearch-success-function dired-isearch-orig-success-function)
    (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t))
  
! (defun dired-isearch-success-function (beg end)
    "Match only at visible regions with the text property `dired-filename'."
!   (and (isearch-success-function-default beg end)
         (if dired-isearch-filenames
  	   (text-property-not-all (min beg end) (max beg end)
  				  'dired-filename nil)
--- 2330,2355 ----
  (defun dired-isearch-filenames-setup ()
    "Set up isearch to search in Dired file names.
  Intended to be added to `isearch-mode-hook'."
!   (when (or (eq dired-isearch-filenames 'dired-filename)
      (define-key isearch-mode-map "\M-sf" 'dired-isearch-filenames-toggle)
!     (setq dired-isearch-filter-predicate-orig
! 	  (default-value 'isearch-filter-predicate))
!     (setq-default isearch-filter-predicate 'dired-isearch-filter-filenames)
      (add-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end nil t)))
  
  (defun dired-isearch-filenames-end ()
    "Clean up the Dired file name search after terminating isearch."
    (define-key isearch-mode-map "\M-sf" nil)
!   (setq-default isearch-filter-predicate dired-isearch-filter-predicate-orig)
    (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t))
  
! (defun dired-isearch-filter-filenames (beg end)
    "Match only at visible regions with the text property `dired-filename'."
!   (and (isearch-filter-invisible beg end)
         (if dired-isearch-filenames
  	   (text-property-not-all (min beg end) (max beg end)
  				  'dired-filename nil)

Index: lisp/info.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/info.el,v
retrieving revision 1.551
diff -u -r1.551 info.el
--- lisp/info.el	20 Oct 2008 02:23:01 -0000	1.551
+++ lisp/info.el	16 Nov 2008 21:10:12 -0000
@@ -1660,7 +1660,7 @@
 			      (point-max)))
 	  (while (and (not give-up)
 		      (or (null found)
-			  (not (funcall isearch-success-function beg-found found))))
+			  (not (funcall isearch-filter-predicate beg-found found))))
 	    (let ((search-spaces-regexp
 		   (if (or (not isearch-mode) isearch-regexp)
 		       Info-search-whitespace-regexp)))
@@ -1740,7 +1740,7 @@
 		(setq give-up nil found nil)
 		(while (and (not give-up)
 			    (or (null found)
-				(not (funcall isearch-success-function beg-found found))))
+				(not (funcall isearch-filter-predicate beg-found found))))
 		  (let ((search-spaces-regexp
 			 (if (or (not isearch-mode) isearch-regexp)
 			     Info-search-whitespace-regexp)))
@@ -1847,7 +1847,7 @@
 (defun Info-isearch-start ()
   (setq Info-isearch-initial-node nil))
 
-(defun Info-search-success-function (beg-found found)
+(defun Info-isearch-filter-predicate (beg-found found)
   "Skip invisible text, node header line and Tag Table node."
   (save-match-data
     (let ((backward (< found beg-found)))
@@ -3533,8 +3533,8 @@
        'Info-isearch-wrap)
   (set (make-local-variable 'isearch-push-state-function)
        'Info-isearch-push-state)
-  (set (make-local-variable 'isearch-success-function)
-       'Info-search-success-function)
+  (set (make-local-variable 'isearch-filter-predicate)
+       'Info-isearch-filter-predicate)
   (set (make-local-variable 'search-whitespace-regexp)
        Info-search-whitespace-regexp)
   (set (make-local-variable 'revert-buffer-function)

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#1352: 23.0.60; isearch-success-function

[Stefan Monnier]

> The name `isearch-success-function' was created to be consistent
> with other *-function variables and with `isearch-success',
> but I agree that more clear names are better.  So the patch below
> renames it to `isearch-filter-predicate' and also renames
> `isearch-success-function-default' to `isearch-filter-invisible',
> plus renames in Dired and Info, and doc fixes:

Sounds very good, thank you, please install it.


        Stefan


PS: I especially like the isearch-filter-invisible, so it says what the
function does, rather than how/where the function is expected to
be used.

bug#1352: 23.0.60; isearch-success-function

[Drew Adams]

Thanks, Juri. Looks better, to me anyway.

Some details you might want to consider -


1. This doc string:

> ! (defun isearch-filter-invisible (beg end)
> !   "Default predicate to filter out invisible text.
> ! It filters search hits to those that are visible (at least 
> partially),
> ! unless invisible text too can be searched."
>     (or (eq search-invisible t)
>         (not (isearch-range-invisible beg end))))

That explains how the predicate is used, not what it, itself, does.
I'd propose something like this:

 Tests whether the current search hit is visible to Isearch.
 Returns non-nil if `search-invisible' is t or the text from BEG to END
 is visible to Isearch as determined by `isearch-range-invisible'.


2. The predicate name.

A filter can be used either way: to allow to pass or to keep out.  The
Isearch filter allows whatever passes the predicate and disallows
whatever does not pass it.

Regardless of how a predicate might be used, its doc string and name
should reflect the quality for which the predicate is true: `bluep' is
true if its arg is blue, regardless of whether the predicate is used
to filter blueness out or in.

It is the quality of being isearch-visible that is tested here; the
predicate returns true when the search hit is visible to Isearch.  A
name that reflects that is `isearch-visible-p'.

Or, if we add the prefix `isearch-' systematically,
`isearch-isearch-visible-p'.  Or `isearch-visible-to-isearch-p'.
(This is bound to be somewhat confusing whatever name we use for it,
because of the notion of normally invisible text being visible to
Isearch.)

I'd probably go with just `isearch-visible-p'.


3. Similarly, I'd suggest `dired-isearch-filename-p' instead of
`dired-isearch-filter-filenames'.  Again, the predicate name should
reflect what the predicate does, not how it might be used.  In this
case, it returns true for a search hit that is a file name.  And the
doc string:

 Tests whether the current search hit is a file name.
 Returns non-nil if the text from BEG to END is part of a visible file
 name.


4. Likewise, something like `Info-isearch-visible-body-text-p' instead
of `Info-isearch-filter-predicate'.  Name it after the kind of search
hits that pass the test: body text (not header or tag-table text) that
is visible.  And the doc string:

 Tests whether the current search hit is a visible body text.
 Returns non-nil if the text from BEG to END is visible and is not in
 a header line or a tag table.

bug#1352: marked as done (23.0.60; isearch-success-function)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]


Your message dated Mon, 17 Nov 2008 02:46:02 +0200
with message-id <87tza7kwmt.fsf@jurta.org>
and subject line Re: bug#1352: 23.0.60; isearch-success-function
has caused the Emacs bug report #1352,
regarding 23.0.60; isearch-success-function
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact don@donarmstrong.com
immediately.)


-- 
1352: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=1352
Emacs Bug Tracking System
Contact don@donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 4862 bytes --]

From: "Drew Adams" <drew.adams@oracle.com>
To: <emacs-pretest-bug@gnu.org>
Subject: 23.0.60; isearch-success-function
Date: Sat, 15 Nov 2008 15:39:35 -0800
Message-ID: <005c01c9477b$6adb8290$0200a8c0@us.oracle.com>

The name of `isearch-success-function' seems wrong, if we believe the
doc string. The doc string suggests that it is a predicate that
filters the search hits that would otherwise be available. That's very
general. If that's true, then the name should reflect this meaning -
perhaps `isearch-predicate'.
 
Similarly, the name of `isearch-success-function-default' should be
something that suggests its very limited meaning: it filters search
hits to those that are visible, unless invisible text too can be
searched. It is a particular kind of search filter, and its name
should reflect that particularity.
 
Also, in `isearch-search', there is this:
 
;; Clear RETRY unless we matched some invisible text
;; and we aren't supposed to do that.
(if (or (not isearch-success)
        (bobp) (eobp)
        (= (match-beginning 0) (match-end 0))
        (funcall isearch-success-function
                 (match-beginning 0) (match-end 0)))
    (setq retry nil)))
 
The code here is general; it is not related to text visibility. So the
comment is not general enough. It is a vestige that is appropriate now
only when the value of `isearch-success-function' is
`isearch-success-function-default'. The comment should instead say
that we clear RETRY unless the search predicate says to skip this
search hit.
 
It seems like this is a partly finished enhancement to behavior that
was originally only for searching invisible text. Seems like this was
generalized to an arbitrary predicate that filters possible search
hits - but the job seems only half done. The code presumably works in
a general way, but the comments and symbol names are inappropriate.
 
The comments and the variable and function names need to reflect the
new behavior, and this behavior should be documented for users. Users
should know how to take advantage of this feature, defining their own
search predicates that filter the search hits that would normally be
available, so that hits that dissatisfy the predicate are skipped.
 

In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-11-08 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'
 




[-- Attachment #3: Type: message/rfc822, Size: 2227 bytes --]

From: Juri Linkov <juri@jurta.org>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 1352-done@emacsbugs.donarmstrong.com, Drew Adams <drew.adams@oracle.com>
Subject: Re: bug#1352: 23.0.60; isearch-success-function
Date: Mon, 17 Nov 2008 02:46:02 +0200
Message-ID: <87tza7kwmt.fsf@jurta.org>

> Sounds very good, thank you, please install it.

Done.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#1352: 23.0.60; isearch-success-function

[Juri Linkov]

> Thanks, Juri. Looks better, to me anyway.
>
> Some details you might want to consider -

Sorry for not responding to your last input after closing this bug.
I've carefully read you message as I saw it, but found no more
room for improvement.  The final change was based on your original
report and approved by Stefan.  I think further trying to achieve
the perfection will make this worse.

In particular, your latest suggestions assume that Isearch is a library of
functions with well-defined interfaces.  This is only partially true.
Rather it is an editor's feature with a set of subfeatures.  So it would
be more practical to use uniform function names that contain the same name
prefix for all related functions and variables. This will help better
recognizing the used subfeature in other features (like Dired and Info),
helping mentally connect any related function names to it.

In essence, what you suggest is using the prefix `isearch-visible'
instead of the current `isearch-filter' in the predicate names.
I see no preference for a property-based `visible' over an action-based
`filter'.  I think the word `visible' is more confusing since it can be
misinterpreted as "visible to the user" instead of "visible to isearch".
The existing name `isearch-range-invisible' has no such problem because
it tests whether the search hit is visible to the user.  So the name
`isearch-filter-invisible' connects two features and names together:

1. visible - "visible to the user"
2. filter  - "visible to isearch"

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#1352: 23.0.60; isearch-success-function

[Drew Adams]

> Sorry for not responding to your last input after closing this bug.
> I've carefully read you message as I saw it, but found no more
> room for improvement.  The final change was based on your original
> report and approved by Stefan.  I think further trying to achieve
> the perfection will make this worse.
> 
> In particular, your latest suggestions assume that Isearch is 
> a library of
> functions with well-defined interfaces.  This is only partially true.
> Rather it is an editor's feature with a set of subfeatures.  
> So it would
> be more practical to use uniform function names that contain 
> the same name
> prefix for all related functions and variables. This will help better
> recognizing the used subfeature in other features (like Dired 
> and Info),
> helping mentally connect any related function names to it.
> 
> In essence, what you suggest is using the prefix `isearch-visible'
> instead of the current `isearch-filter' in the predicate names.
> I see no preference for a property-based `visible' over an 
> action-based
> `filter'.  I think the word `visible' is more confusing since 
> it can be
> misinterpreted as "visible to the user" instead of "visible 
> to isearch".
> The existing name `isearch-range-invisible' has no such 
> problem because
> it tests whether the search hit is visible to the user.  So the name
> `isearch-filter-invisible' connects two features and names together:
> 
> 1. visible - "visible to the user"
> 2. filter  - "visible to isearch"

Thanks for replying. It's a shame that the bug tracker doesn't take such
followup mails into account. It apparently includes spam but omits messages in a
bug thread after the bug is closed. ;-)

Though I disagree in general, I see your argument, especially the possible
confusion about visibility, which I also pointed out. I'm OK with the convention
you chose, as long as we're consistent.

Minor point: `Info-isearch-filter-predicate' does not respect your naming
convention: "predicate" does not describe what the filter does. Using your
convention, you might call it instead `Info-isearch-filter-body-text' or
`Info-isearch-filter-visible-body-text' (which admittedly is a bit long).

Another thing you might think about is the `-p' ending. Shouldn't we follow that
convention for predicate names? 

Especially since the doc strings do not mention the return values. I think a
predicate's doc string should state when it returns nil vs non-nil, but if you
don't want to do that, then the name (`-p') would at least give a clue to the
type.

You might like to think of these predicates as action functions that perform
filtering, but they are not - they have no side effects. They are just
predicates that can be used by an action function to filter. It's the difference
between `delete-if' (filtering action function) and a predicate passed to it.

That distinction is the main point behind the doc strings and names I suggested.

I'm OK with your approach, but you might want to finish off some of these loose
ends (`-p'; name of the Info predicate; mention return values in doc strings).

Thx - Drew

bug#1352: 23.0.60; isearch-success-function

[Juri Linkov]

> Minor point: `Info-isearch-filter-predicate' does not respect your naming
> convention: "predicate" does not describe what the filter does. Using your
> convention, you might call it instead `Info-isearch-filter-body-text' or
> `Info-isearch-filter-visible-body-text' (which admittedly is a bit long).

I agree that the current name is inappropriate.  But your suggestions
are no better.  First, there is no such a term as `body' in the Info
documentation, so it will cause questions what is a body (answer: no header
and not tag node).  What is worse is that after adding more conditions
to this predicate we'll need to rename it.  I remember long ago you
proposed to filter out header lines.  Adding such a condition for the header
line would require renaming from a name like `Info-isearch-filter-node-text'
(if this function existed at that time) to what you just proposed.
In future when it makes sense to add more conditions like skipping `* Menu'
where actually the tag `* Menu' is part of the node's body, the name
you proposed becomes wrong and needs to be renamed to a name like
`Info-isearch-filter-body-text-without-menu-tag', and so on.

That's why I think we should find a general name for the default predicate.
I think it should be simply `Info-isearch-filter' without any specifics.

> Another thing you might think about is the `-p' ending. Shouldn't we
> follow that convention for predicate names?

`-p' is usually added to short names that have no other indication that
they are predicates (e.g. `display-images-p').  But filter predicates
already have a prefix `isearch-filter' that indicates that a function
is a filter.  Adding `-p' will make such names more ugly.

> Especially since the doc strings do not mention the return values.
> I think a predicate's doc string should state when it returns nil vs
> non-nil, but if you don't want to do that, then the name (`-p') would
> at least give a clue to the type.

I like your doc strings, so instead of adding `-p' I'll fix doc strings
using your variants :-)

BTW, I noticed that the name `isearch-filter-invisible' is logically
incorrect because the name should say whether the test is passed.
So I'll replace it with `isearch-filter-visible'.

-- 
Juri Linkov
http://www.jurta.org/emacs/

bug#1352: 23.0.60; isearch-success-function

[Drew Adams]

> > Minor point: `Info-isearch-filter-predicate' does not 
> > respect your naming convention: "predicate" does not
> > describe what the filter does. Using your
> > convention, you might call it instead 
> > `Info-isearch-filter-body-text' or
> > `Info-isearch-filter-visible-body-text' (which admittedly 
> > is a bit long).
> 
> I agree that the current name is inappropriate.  But your suggestions
> are no better.  First, there is no such a term as `body' in the Info
> documentation, so it will cause questions what is a body 
> (answer: no header and not tag node).  What is worse is that after
> adding more conditions to this predicate we'll need to rename it.
> I remember long ago you proposed to filter out header lines.  Adding
> such a condition for the header line would require renaming from a
> name like `Info-isearch-filter-node-text' (if this function existed
> at that time) to what you just proposed. In future when it makes
> sense to add more conditions like skipping `* Menu'
> where actually the tag `* Menu' is part of the node's body, the name
> you proposed becomes wrong and needs to be renamed to a name like
> `Info-isearch-filter-body-text-without-menu-tag', and so on.
> 
> That's why I think we should find a general name for the 
> default predicate. I think it should be simply `Info-isearch-filter'
> without any specifics.

Agreed.

> > Another thing you might think about is the `-p' ending. Shouldn't we
> > follow that convention for predicate names?
> 
> `-p' is usually added to short names that have no other indication
> that they are predicates (e.g. `display-images-p').

I don't think that qualification is part of the convention.

> But filter predicates already have a prefix `isearch-filter'
> that indicates that a function is a filter.

As I mentioned, in English, "filter" is ambiguous in this context. It can, as
you intend it, mean a filter (noun), which could be interpreted as a predicate.
(As I pointed out, though, even then it the name doesn't say what the predicate
quality is - what's being filtered in or out.)

But "filter" here can also mean the action (verb) to filter. These functions do
not do any filtering on their own - they have no side effects.

I think the suffix `-p' would help make clear that these are pure predicates,
nothing more. And I think that's the Emacs convention (without your added
qualification), but I could be mistaken about that.

> Adding `-p' will make such names more ugly.

Yes, but also clearer.

> > Especially since the doc strings do not mention the return values.
> > I think a predicate's doc string should state when it returns nil vs
> > non-nil, but if you don't want to do that, then the name 
> > (`-p') would at least give a clue to the type.
> 
> I like your doc strings, so instead of adding `-p' I'll fix 
> doc strings using your variants :-)
> 
> BTW, I noticed that the name `isearch-filter-invisible' is logically
> incorrect because the name should say whether the test is passed.
> So I'll replace it with `isearch-filter-visible'.

That's part of what I was saying in previous feedback I gave. The function name
should, if possible, indicate whether it is filtering out or filtering in (e.g.
keep-lines vs flush-lines).

Anyway, thanks for looking at this again. - Drew

bug#1352: 23.0.60; isearch-success-function

[Stefan Monnier]

> That's why I think we should find a general name for the default predicate.
> I think it should be simply `Info-isearch-filter' without any specifics.

Agreed.  Sounds like a good name.


        Stefan