bug#9918: 24.0.90; lazy-highlighting in Info mode

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

From "emacs -Q":
1. Open Info node "(emacs)Top" (C-h r).
2. Move point to the end of the first paragraph.
3. Type "C-s extensible".

The word "extensible" (in the first line of the paragraph) should be
lazy-highlighted, as happens if you repeat the experiment after doing
"M-x text-mode".


In GNU Emacs 24.0.90.1 (i386-mingw-nt6.1.7601)
 of 2011-10-27 on DANI-PC
Windowing system distributor `Microsoft Corp.', version 6.1.7601
configured using `configure --with-gcc (4.5)'

-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Eli Zaretskii]

> Date: Mon, 31 Oct 2011 15:46:46 +0100
> From: Dani Moncayo <dmoncayo@gmail.com>
> 
> >From "emacs -Q":
> 1. Open Info node "(emacs)Top" (C-h r).
> 2. Move point to the end of the first paragraph.
> 3. Type "C-s extensible".
> 
> The word "extensible" (in the first line of the paragraph) should be
> lazy-highlighted, as happens if you repeat the experiment after doing
> "M-x text-mode".

This is a feature.  info.el sets isearch-search-fun-function to
Info-isearch-search, which by default searches in a special way (you
should see the "Failing I-search" message in the echo are).  If you
don't want this behavior, try customizing Info-isearch-search to nil.

I think this bug report can be closed.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Drew Adams]

> > >From "emacs -Q":
> > 1. Open Info node "(emacs)Top" (C-h r).
> > 2. Move point to the end of the first paragraph.
> > 3. Type "C-s extensible".
> > 
> > The word "extensible" (in the first line of the paragraph) should be
> > lazy-highlighted, as happens if you repeat the experiment 
> > after doing "M-x text-mode".
> 
> This is a feature.  info.el sets isearch-search-fun-function to
> Info-isearch-search, which by default searches in a special way (you
> should see the "Failing I-search" message in the echo are).  If you
> don't want this behavior, try customizing Info-isearch-search to nil.
> 
> I think this bug report can be closed.

Hm.  Obviously this is _not_ TRT from a user point of view.  This happens _only_
when there is only one match of the search pattern and that match is before
point.  And AFAICT it happens _only_ in Info.

Yes, Isearch forward means Isearch _forward_.  But as Dani points out, in other
buffers even a singleton occurrence that precedes point gets highlighted.

This is a bug, IMHO.  It should not be closed.  Whether it is a high priority
for being fixed is another matter, but it should not be closed just because
Isearch is a bit special in Info.  That's just a cop-out.  Feature, indeed!
What we should say is "Good catch, Dani - thanks."

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

>> >From "emacs -Q":
>> 1. Open Info node "(emacs)Top" (C-h r).
>> 2. Move point to the end of the first paragraph.
>> 3. Type "C-s extensible".
>>
>> The word "extensible" (in the first line of the paragraph) should be
>> lazy-highlighted, as happens if you repeat the experiment after doing
>> "M-x text-mode".
>
> This is a feature.  info.el sets isearch-search-fun-function to
> Info-isearch-search, which by default searches in a special way

IMO, it would be better if that special way of searching was
consistent with the standard Isearch wrt the lazy highlighting.

In step #3 above, the lazy highlighting is performed well until the
search begins to fail, point at which the lazy highlighting is
deactivated.  As I said, this behavior is (a) inconsistent with the
standard Isearch (try to do the same in text mode) and (b) less
informative for the user (there is indeed a match of "extensible" that
should be advised to the user).


-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Drew Adams]

> In step #3 above, the lazy highlighting is performed well until the
> search begins to fail, point at which the lazy highlighting is
> deactivated.  As I said, this behavior is (a) inconsistent with the
> standard Isearch (try to do the same in text mode) and (b) less
> informative for the user (there is indeed a match of "extensible" that
> should be advised to the user).

Note that the problem arises only when there is only one match for the search
pattern, and that match occurs before point.

E.g.: `g Intro', put point in the 3rd paragraph, then search for `extensible'.
There is no problem highlighting occurrences that are before point.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

> Note that the problem arises only when there is only one match for the search
> pattern, and that match occurs before point.
>
> E.g.: `g Intro', put point in the 3rd paragraph, then search for `extensible'.
> There is no problem highlighting occurrences that are before point.

In that case there is no problem, but I think that the reason is that
the search is succeeding (there is a match in the last paragraph).
Try to do the same starting from the middle of the last paragraph, and
you will see the problem again.


-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Drew Adams]

> > Note that the problem arises only when there is only one 
> > match for the search pattern, and that match occurs before point.
> >
> > E.g.: `g Intro', put point in the 3rd paragraph, then 
> > search for `extensible'.  There is no problem highlighting
> > occurrences that are before point.
> 
> In that case there is no problem, but I think that the reason is that
> the search is succeeding (there is a match in the last paragraph).
> Try to do the same starting from the middle of the last paragraph, and
> you will see the problem again.

Yes.  But hit C-s again and it then correctly highlights all of the occurrences
(just as it should have done initially, and which happens in other modes).

It is when there is only a single match that it immediately moves on to the next
node that has a match, instead of highlighting the match in the current node.
At least when there is more than one match a second C-s will highlight them all.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> From "emacs -Q":
> 1. Open Info node "(emacs)Top" (C-h r).
> 2. Move point to the end of the first paragraph.
> 3. Type "C-s extensible".
>
> The word "extensible" (in the first line of the paragraph) should be
> lazy-highlighted, as happens if you repeat the experiment after doing
> "M-x text-mode".

The difference between this recipe in a normal buffer and an Info buffer
is in the line

    (when (not isearch-error)

in `isearch-lazy-highlight-new-loop'.  A normal Isearch starts
a new lazy highlighting loop even if there is no success
(`isearch-success' is nil) and no errors (`isearch-error' is nil).

But Info Isearch sets `isearch-error' to "initial node", so
`isearch-lazy-highlight-new-loop' doesn't start lazy-highlighting.
It seems with the error "initial node", it's still possible to
lazy-highlight other matches.  So it's not a normal error,
and we can make an exception for this type of "error".
This will work with the following patch:

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-10-30 14:41:20 +0000
+++ lisp/isearch.el	2011-10-31 23:12:01 +0000
@@ -2721,7 +2721,7 @@ (defun isearch-lazy-highlight-new-loop (
     ;; something important did indeed change
     (lazy-highlight-cleanup t) ;kill old loop & remove overlays
     (setq isearch-lazy-highlight-error isearch-error)
-    (when (not isearch-error)
+    (when (or (not isearch-error) (equal isearch-error "initial node"))
       (setq isearch-lazy-highlight-start-limit beg
 	    isearch-lazy-highlight-end-limit end)
       (setq isearch-lazy-highlight-window       (selected-window)

But it's not good to hardcode it, so perhaps we should introduce
a special property for errors that are not quite an error,
and set it just for "initial node".

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

> But Info Isearch sets `isearch-error' to "initial node", so
> `isearch-lazy-highlight-new-loop' doesn't start lazy-highlighting.
> It seems with the error "initial node", it's still possible to
> lazy-highlight other matches.  So it's not a normal error,
> and we can make an exception for this type of "error".
> This will work with the following patch:
>
> [...]
>
> But it's not good to hardcode it, so perhaps we should introduce
> a special property for errors that are not quite an error,
> and set it just for "initial node".

Indeed.

IIUC, "initial node" just inform the user that the current search is
failing inside this node (where the search started), but this should
not be considered an error (like "incomplete input" or "No previous
search string"), because "initial node", unlike "incomplete input",
does not deactivate the current Isearch.

So, I think that maybe TRT would be to move the "inital node" message
to the Isearch prompt, so that such prompt would be "Failing I-search
(initial node): ".

BTW: while looking for sentences that set the value of `isearch-error'
inside lisp/isearch.el, I've noticed that some errors begins with an
uppercase letter ("No previous search string") and others don't
("incomplete input").  This should be fixed to make it consistent for
all error messages.

-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

BTW, whatever the patch be, there is a second issue (discovered by
Drew) that should be addressed:

The first time Isearch is fails inside an Info node, you get the
"initial node" message, and then, typing another "C-s" have the effect
of continuing the Isearch across subsequent info nodes.  But this
behavior sometimes fails.  For example:

1. Go to Info node "(emacs)Intro".
2. Type "C-s extensible C-s C-s".
--> Here you've got the "initial node" message. OK.
3. Type "C-s".
--> (Observed) The "initial node" message is removed, and everything
remains the same.  You have to type "C-s" again to continue the search
in subsequent nodes.
--> (Expected) The Isearch is continued across subsequent nodes,
without the need of an extra "C-s".


-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> IIUC, "initial node" just inform the user that the current search is
> failing inside this node (where the search started), but this should
> not be considered an error (like "incomplete input" or "No previous
> search string"), because "initial node", unlike "incomplete input",
> does not deactivate the current Isearch.

If it's not possible to code this situation with the combination
of existing variables `isearch-success' and `isearch-error',
then perhaps we should introduce a new variable `isearch-warning'.

> BTW: while looking for sentences that set the value of `isearch-error'
> inside lisp/isearch.el, I've noticed that some errors begins with an
> uppercase letter ("No previous search string") and others don't
> ("incomplete input").  This should be fixed to make it consistent for
> all error messages.

This is intentional because when you see "No previous search string" the
search string is empty, so the message should look like the beginning of
a new sentence, not like the continuation of a non-empty search string.

> BTW, whatever the patch be, there is a second issue (discovered by
> Drew) that should be addressed:
>
> The first time Isearch is fails inside an Info node, you get the
> "initial node" message, and then, typing another "C-s" have the effect
> of continuing the Isearch across subsequent info nodes.  But this
> behavior sometimes fails.  For example:
>
> 1. Go to Info node "(emacs)Intro".
> 2. Type "C-s extensible C-s C-s".
> --> Here you've got the "initial node" message. OK.
> 3. Type "C-s".
> --> (Observed) The "initial node" message is removed, and everything
> remains the same.  You have to type "C-s" again to continue the search
> in subsequent nodes.
> --> (Expected) The Isearch is continued across subsequent nodes,
> without the need of an extra "C-s".

The current behavior is correct.  When you see the message about failing
Isearch without the "initial node" suffix, that means that it reached
the end of the whole manual and failed completely.  The subsequent `C-s'
starts searching from the beginning of the manual.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

>> 1. Go to Info node "(emacs)Intro".
>> 2. Type "C-s extensible C-s C-s".
>> --> Here you've got the "initial node" message. OK.
>> 3. Type "C-s".
>> --> (Observed) The "initial node" message is removed, and everything
>> remains the same.  You have to type "C-s" again to continue the search
>> in subsequent nodes.
>> --> (Expected) The Isearch is continued across subsequent nodes,
>> without the need of an extra "C-s".
>
> The current behavior is correct.  When you see the message about failing
> Isearch without the "initial node" suffix, that means that it reached
> the end of the whole manual and failed completely.  The subsequent `C-s'
> starts searching from the beginning of the manual.

Ah Ok.

Then, I think that, in this case, a message like "end of the manual"
(analogous to "initial node") would be good, for the user to know
what's going on.


-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> Then, I think that, in this case, a message like "end of the manual"
> (analogous to "initial node") would be good, for the user to know
> what's going on.

This is possible to do with a small patch:

=== modified file 'lisp/info.el'
--- lisp/info.el	2011-11-02 10:46:18 +0000
+++ lisp/info.el	2011-11-03 09:06:57 +0000
@@ -1844,7 +1844,7 @@ (defun Info-search (regexp &optional bou
 		    (setq list nil)))
 	      (if found
 		  (message "")
-		(signal 'search-failed (list regexp))))
+		(signal 'search-failed (list regexp "end of the manual"))))
 	  (if (not found)
 	      (progn (Info-read-subfile osubfile)
 		     (goto-char opoint)

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

>> Then, I think that, in this case, a message like "end of the manual"
>> (analogous to "initial node") would be good, for the user to know
>> what's going on.
>
> This is possible to do with a small patch:

Looks good!  Thanks!

So, the remaining issue is to handle "initial node" and "end of the
manual", not as errors (they *don't* deactivate the Isearch), but as
informative messages.

As I pointed out in a previous post, I think that it would be
unsuitable to show these informative messages the same way as error
messages (with "[...]" suffix), because they have a different nature
(ones deactivate Isearch, the others don't).

I think that these two informative messages ("initial node", "end of
the manual") fall into the same category as the messages that are
added to the Isearch prompt ("Failing", "Wrapped", "Overwrapped",
"Failing overwrapped", ...).  So I propose to present all these
"Isearch flags" to the user using a common pattern. For example:
* "Isearch:"
* "Isearch [failing]:"
* "Isearch [wrapped]:"
* "Isearch [overwrapped]:"
* "Isearch [failing, overwrapped]:"
* "Isearch [failing, initial node]:"
* "Isearch [failing, end of manual]:"

It would add consistency to the Isearch visual feedback.

-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

> I think that these two informative messages ("initial node", "end of
> the manual") fall into the same category as the messages that are
> added to the Isearch prompt ("Failing", "Wrapped", "Overwrapped",
> "Failing overwrapped", ...).  So I propose to present all these
> "Isearch flags" to the user using a common pattern. For example:
> * "Isearch:"
> * "Isearch [failing]:"
> * "Isearch [overwrapped]:"
> * "Isearch [failing, overwrapped]:"
> * "Isearch [failing, initial node]:"
>
> It would add consistency to the Isearch visual feedback.

... however a downside of this approach is that the position of the
search string varies each time a flag is added to or removed from the
prompt.  I really dislike that, and btw, this effect occurs currently
("I-search: foo" -> "Failing I-search: foo" ...).

So, I rather would write the flags at the right side of the search
string, like the error messages are showed right now:
* "Isearch: foo"
* "Isearch: foo [failing]"
* "Isearch: foo [failing, overwrapped]"
* "Isearch: foo [failing, initial node]"
...

This approach raises a question: how to show the _error_ flags to make
them distinguishable from the _informative_ flags?

One possibility is to show all the flags the same way.  After all, the
fact that the errors deactivate the Isearch should be enough for the
user to know what's going on, I think.

-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> * "Isearch: foo"
> * "Isearch: foo [failing]"
> * "Isearch: foo [failing, overwrapped]"
> * "Isearch: foo [failing, initial node]"

Yes, moving all prefixes into suffixes and always displaying the
search string at the fixed position in the message avoids this problem
where prefixes shift the search string to the right in the message, so
its position jumps left and right during Isearch depending on the length
of the composite prefix that makes more difficult to concentrate on
the search string displayed in the message.

> This approach raises a question: how to show the _error_ flags to make
> them distinguishable from the _informative_ flags?
>
> One possibility is to show all the flags the same way.  After all, the
> fact that the errors deactivate the Isearch should be enough for the
> user to know what's going on, I think.

I think to show all the flags the same way is better.

Of course, this should be postponed for 24.2.  But now I'd like to fix
the original problem you reported.  I think we should remove
`(not isearch-error)' from `isearch-lazy-highlight-new-loop'.
This is over-optimization.  It can't know for sure beforehand
whether lazy-highlighting will be able to highlight matches
even when isearch-error is non-nil.

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2011-10-30 01:56:03 +0000
+++ lisp/isearch.el	2011-11-03 19:55:07 +0000
@@ -2674,7 +2721,6 @@ (defun isearch-lazy-highlight-new-loop (
     ;; something important did indeed change
     (lazy-highlight-cleanup t) ;kill old loop & remove overlays
     (setq isearch-lazy-highlight-error isearch-error)
-    (when (not isearch-error)
       (setq isearch-lazy-highlight-start-limit beg
 	    isearch-lazy-highlight-end-limit end)
       (setq isearch-lazy-highlight-window       (selected-window)
@@ -2692,7 +2738,7 @@ (defun isearch-lazy-highlight-new-loop (
       (unless (equal isearch-string "")
 	(setq isearch-lazy-highlight-timer
 	      (run-with-idle-timer lazy-highlight-initial-delay nil
-				   'isearch-lazy-highlight-update))))))
+				   'isearch-lazy-highlight-update)))))
 
 (defun isearch-lazy-highlight-search ()
   "Search ahead for the next or previous match, for lazy highlighting.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Dani Moncayo]

> I'd like to fix
> the original problem you reported.  I think we should remove
> `(not isearch-error)' from `isearch-lazy-highlight-new-loop'.
> This is over-optimization.  It can't know for sure beforehand
> whether lazy-highlighting will be able to highlight matches
> even when isearch-error is non-nil.

Ok, the original problem is fixed by your patch.  Install it when you want.

And let's keep this bug report open, until the other proposed change
be made (in 24.2).

Thanks a lot.

-- 
Dani Moncayo

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> And let's keep this bug report open, until the other proposed change
> be made (in 24.2).

I propose also to fix the clarity and unambiguousness of final messages
now before the next release:

=== modified file 'lisp/info.el'
--- lisp/info.el	2011-10-24 05:47:05 +0000
+++ lisp/info.el	2011-11-03 21:09:57 +0000
@@ -1846,7 +1844,7 @@ (defun Info-search (regexp &optional bou
 		    (setq list nil)))
 	      (if found
 		  (message "")
-		(signal 'search-failed (list regexp))))
+		(signal 'search-failed (list regexp "end of the manual"))))
 	  (if (not found)
 	      (progn (Info-read-subfile osubfile)
 		     (goto-char opoint)

=== modified file 'lisp/misearch.el'
--- lisp/misearch.el	2011-04-19 13:44:55 +0000
+++ lisp/misearch.el	2011-11-03 21:08:09 +0000
@@ -173,8 +173,8 @@ (defun multi-isearch-search-fun ()
 		   found)
 	       ;; Return nil when multi-isearch-next-buffer-current-function fails
 	       ;; (`with-current-buffer' raises an error for nil returned from it).
-	       (error nil))
-	   (signal 'search-failed (list string "Repeat for next buffer"))))))))
+	       (error (signal 'search-failed (list string "end of multi"))))
+	   (signal 'search-failed (list string "repeat for next buffer"))))))))
 
 (defun multi-isearch-wrap ()
   "Wrap the multiple buffers search when search is failed.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> But now I'd like to fix the original problem you reported.
> I think we should remove `(not isearch-error)' from
> `isearch-lazy-highlight-new-loop'.  This is over-optimization.
> It can't know for sure beforehand whether lazy-highlighting will be
> able to highlight matches even when isearch-error is non-nil.

I installed this fix.

> I propose also to fix the clarity and unambiguousness of final messages

And this one as well.

bug#9918: 24.0.90; lazy-highlighting in Info mode

[Juri Linkov]

> I propose also to fix the clarity and unambiguousness of final messages

I noticed that "end of manual" is displayed only in multi-file
Info manuals.  To display it in a single-file Info manual,
the following patch is necessary as well:

=== modified file 'lisp/info.el'
--- lisp/info.el	2011-11-23 07:03:56 +0000
+++ lisp/info.el	2011-12-01 07:45:18 +0000
@@ -1769,12 +1774,14 @@ (defun Info-search (regexp &optional bou
       ;; If no subfiles, give error now.
       (if give-up
 	  (if (null Info-current-subfile)
-	      (let ((search-spaces-regexp
-		     (if (or (not isearch-mode) isearch-regexp)
-			 Info-search-whitespace-regexp)))
-		(if backward
-		    (re-search-backward regexp)
-		  (re-search-forward regexp)))
+	      (if isearch-mode
+		  (signal 'search-failed (list regexp "end of manual"))
+		(let ((search-spaces-regexp
+		       (if (or (not isearch-mode) isearch-regexp)
+			   Info-search-whitespace-regexp)))
+		  (if backward
+		      (re-search-backward regexp)
+		    (re-search-forward regexp))))
 	    (setq found nil)))
 
       (if (and bound (not found))