bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

1. emacs -Q

2. Visit a file - anything other than a tiny one will do.

3. Use `customize-option' to set `lazy-highlight-cleanup' to nil
   and set `lazy-max-at-a-time' to nil.

4. Search once for a simple string that occurs multiple times throughout
   the buffer - e.g., `C-s the RET`.

5. Scroll down to see that the matches were not highlighted throughout
   the buffer.  `lazy-highlight-max-at-a-time' does not work as
   advertised.


In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2015-07-03 on LEG570
Bzr revision: 2b848fadd51e805b2f46da64c5958ea7f009048a
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --host=i686-pc-mingw32 --enable-checking=yes,glyphs'

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> 1. emacs -Q
> 2. Visit a file - anything other than a tiny one will do.
> 3. Use `customize-option' to set `lazy-highlight-cleanup' to nil
>    and set `lazy-max-at-a-time' to nil.
> 4. Search once for a simple string that occurs multiple times throughout
>    the buffer - e.g., `C-s the RET`.
> 5. Scroll down to see that the matches were not highlighted throughout
>    the buffer.  `lazy-highlight-max-at-a-time' does not work as
>    advertised.

[Typo in step 3: `lazy-max-at-a-time' should be
`lazy-highlight-max-at-a-time' (as in step 5).]

It would be great to hear back about this.  Is this a bug or am I
misunderstanding something?  The doc seems to say that you can expect
that all search hits will be highlighted when the value of
`lazy-highlight-max-at-a-time' is nil: "A value of nil means highlight
all matches."  Even the Customize value menu text says this.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Eli Zaretskii]

> Date: Thu, 27 Aug 2015 13:29:58 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> 
> > 1. emacs -Q
> > 2. Visit a file - anything other than a tiny one will do.
> > 3. Use `customize-option' to set `lazy-highlight-cleanup' to nil
> >    and set `lazy-max-at-a-time' to nil.
> > 4. Search once for a simple string that occurs multiple times throughout
> >    the buffer - e.g., `C-s the RET`.
> > 5. Scroll down to see that the matches were not highlighted throughout
> >    the buffer.  `lazy-highlight-max-at-a-time' does not work as
> >    advertised.
> 
> [Typo in step 3: `lazy-max-at-a-time' should be
> `lazy-highlight-max-at-a-time' (as in step 5).]
> 
> It would be great to hear back about this.  Is this a bug or am I
> misunderstanding something?

What do you mean by "scroll down" in step 5 above?  Scroll down how?

Scrolling (as any other command that exits I-search) removes all
highlighting, at least by default.  And "all matches" in the
variable's doc string means "all matches shown on the screen", as the
display engine never does anything beyond the displayed portion
(that's a simplification, but it will do for the purposes of this
discussion).

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > > 1. emacs -Q
> > > 2. Visit a file - anything other than a tiny one will do.
> > > 3. Use `customize-option' to set `lazy-highlight-cleanup' to nil
> > >    and set `lazy-highlight-max-at-a-time' to nil.
> > > 4. Search once for a simple string that occurs multiple times throughout
> > >    the buffer - e.g., `C-s the RET`.
> > > 5. Scroll down to see that the matches were not highlighted throughout
> > >    the buffer.  `lazy-highlight-max-at-a-time' does not work as
> > >    advertised.
> 
> What do you mean by "scroll down" in step 5 above?  Scroll down how?

C-v or whatever.  (Yes, this will exit Isearch,)

> Scrolling (as any other command that exits I-search) removes all
> highlighting, at least by default.

No, it does not.  Not if `lazy-highlight-cleanup' is nil - see step 3.
That's the point of that option, and of this bug report.

> And "all matches" in the
> variable's doc string means "all matches shown on the screen",

How could that be?  What's the point of the option value of nil -
or even a large value - in that case?  And why would we bother to
warn that a large (let alone a nil) value can make things slow, if
highlighting is always limited to what is visible "on the screen"?

How big a screen would someone have to have, to make any
consideration of performance important, if nil is _supposed_ to
highlight only all matches "on the screen"?

Where do you get your interpretation of "all matches"?  From the
display-engine code, I guess, but certainly not from the doc or the
Customize UI, which says just "All".  All matches means all matches;
it does not mean all matches that fit some unmentioned criterion.

> as the display engine never does anything beyond the displayed
> portion (that's a simplification, but it will do for the purposes
> of this discussion).

That description of the display-engine _implementation_, which I
don't doubt, does not jibe with the description of the option.

And as the description is quite deliberate about the possibilities
and behavior of both a LARGE value and a nil value, and about the
consequent risks to performance, it would seem that the _design_
of this feature does not fit the display-engine implementation.

Am I missing something else?  What's the point of such a design
(and its consequent doc), if it could _never be realized_ because
of display-engine limitations?  (I don't doubt the display-engine
design or implementation.  The question is about how this design
could possibly work, given the display engine as you say it is.)

Something else puzzles me.  I did not have the impression that
there was such a hard-and-fast display limitation.  Am I mistaken
that we have no problem forcing the display engine to font-lock
fontify a whole buffer, regardless of size?

If that is possible (I am thinking it is possible, but I could be
wrong) then why would it not also be possible to put lazy-highlight
highlighting on a whole buffer, regardless of size?  And I can call
a homemade function that highlights stuff throughout a LARGE buffer,
whether using font-lock or otherwise.  No problem, AFAICS.

Wrt the actual behavior I see: a nil value seems, indeed to have
no effect beyond the text that has already been shown.  That's
really too bad.

(And lazy highlighting, beyond just highlighting, also computes
all search matches, which can be useful.  But perhaps you will
say that it computes only all matches on the screen?)

I do hope that someone takes a close look at this and you don't
just dismiss it.

AFAICT, it is possible to highlight text throughout a buffer, even
a VERY large one, and using either overlays or text properties.
IOW, I don't understand - I don't think I see the display-engine
limitation you seem to be saying is in place.

IIUC, a lot has changed since lazy highlighting and these options
were first introduced - jit-lock, etc.  Perhaps this worked at one
time, and a regression was introduced since then?  (No, I don't see
that myself - the same bug is in Emacs 22.3.)

Or are you absolutely convinced that this could never have worked,
because of the way the display engine works?  If so then I'm quite
disappointed, but in that case please at least correct the doc
and the Customize UI, to make clear that the value (numeric or nil)
has no effect beyond what is/has been shown on the screen - that it
is not at all about highlighting "all matches".

And given that Lisp code apparently _can_ highlight a whole, large
buffer (AFAICT), if you have any tips on how I might make this work,
myself, for Isearch lazy highlighting, please pass them along.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Eli Zaretskii]

> Date: Fri, 28 Aug 2015 08:19:35 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 21092@debbugs.gnu.org
> 
> > > > 1. emacs -Q
> > > > 2. Visit a file - anything other than a tiny one will do.
> > > > 3. Use `customize-option' to set `lazy-highlight-cleanup' to nil
> > > >    and set `lazy-highlight-max-at-a-time' to nil.
> > > > 4. Search once for a simple string that occurs multiple times throughout
> > > >    the buffer - e.g., `C-s the RET`.
> > > > 5. Scroll down to see that the matches were not highlighted throughout
> > > >    the buffer.  `lazy-highlight-max-at-a-time' does not work as
> > > >    advertised.
> > 
> > What do you mean by "scroll down" in step 5 above?  Scroll down how?
> 
> C-v or whatever.  (Yes, this will exit Isearch,)
> 
> > Scrolling (as any other command that exits I-search) removes all
> > highlighting, at least by default.
> 
> No, it does not.  Not if `lazy-highlight-cleanup' is nil - see step 3.

Missed that, sorry.

> > And "all matches" in the
> > variable's doc string means "all matches shown on the screen",
> 
> How could that be?

I don't know, but that's what I clearly see.

> IIUC, a lot has changed since lazy highlighting and these options
> were first introduced - jit-lock, etc.  Perhaps this worked at one
> time, and a regression was introduced since then?  (No, I don't see
> that myself - the same bug is in Emacs 22.3.)

Do you see any previous version of Emacs where it worked as you
expect?  I don't.

I will now shut up and let people who actually know something about
isearch.el talk.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > IIUC, a lot has changed since lazy highlighting and these options
> > were first introduced - jit-lock, etc.  Perhaps this worked at one
> > time, and a regression was introduced since then?  (No, I don't see
> > that myself - the same bug is in Emacs 22.3.)
> 
> Do you see any previous version of Emacs where it worked as you
> expect?  I don't.

As I said, no, I don't either.

I am also guessing that few people have ever used a nil value of
`lazy-highlight-cleanup'.  I have it on/off as an Isearch toggle
key now, and with that easy access I find it quite useful.

> I will now shut up and let people who actually know something about
> isearch.el talk.

OK, but I am interested in the general question of whether the
display engine precludes highlighting more than what is "on screen".

Could you speak to that?  What am I missing?  AFAICT, it is possible
to highlight, with either text properties or overlays, a very large
buffer.  Do you agree?

If so, why should that not be possible for lazy highlighting also,
because of a display-engine limitation/design?  IOW, I don't
understand how I can highlight a large buffer but Isearch
lazy-highlight cannot do this because of the display engine.

This is not a rhetorical argument.  I would like to understand
whether it is possible to fix this bug, to let users control
the scope of lazy highlighting beyond what is "on the screen".

Perhaps neither you nor I "know something about isearch.el", but
you certainly know something about the display engine.  A little
help understanding this limitation, please.  Thx.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Eli Zaretskii]

> Date: Fri, 28 Aug 2015 08:59:34 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 21092@debbugs.gnu.org
> 
> I am interested in the general question of whether the
> display engine precludes highlighting more than what is "on screen".

You can, of course, put the overlays or properties with some face on
the entire buffer.  So no, the display engine doesn't preclude that.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > I am interested in the general question of whether the
> > display engine precludes highlighting more than what is "on screen".
> 
> You can, of course, put the overlays or properties with some face on
> the entire buffer.  So no, the display engine doesn't preclude that.

OK, great to hear.  So this is a real Isearch bug, and there is a
possibility for it to be fixed.

Hopefully someone knowledgable will contribute to a solution.
I can try to help, but it would be great if Juri or someone else
who understands the code well could lead.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> OK, great to hear.  So this is a real Isearch bug, and there is a
> possibility for it to be fixed.
> 
> Hopefully someone knowledgable will contribute to a solution.
> I can try to help, but it would be great if Juri or someone else
> who understands the code well could lead.

OK, I think I found the problem.  I have something working, but
will send it in the form of a patch in a while.

Essentially, it is the isearch.el code itself that imposes, in
a few places, not searching past (window-end) or before
(window-start), regardless of the value of
`isearch-lazy-highlight-max-at-a-time'.  This is what needs to
be fixed, to be (point-max) and (point-min) when
`isearch-lazy-highlight-max-at-a-time' is nil.

A more thoroughgoing fix would take into account a numeric
value of `isearch-lazy-highlight-max-at-a-time', but I won't
bother with that, for now.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

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

> OK, I think I found the problem.  I have something working,
> but will send it in the form of a patch in a while.
> 
> Essentially, it is the isearch.el code itself that imposes, in
> a few places, not searching past (window-end) or before
> (window-start), regardless of the value of
> `isearch-lazy-highlight-max-at-a-time'.  This is what needs to
> be fixed, to be (point-max) and (point-min) when
> `isearch-lazy-highlight-max-at-a-time' is nil.
> 
> A more thoroughgoing fix would take into account a numeric
> value of `isearch-lazy-highlight-max-at-a-time', but I won't
> bother with that, for now.

Attached is a quick patch that seems to be OK wrt fixing
things for a nil value of `isearch-lazy-highlight-max-at-a-time'.

Please give it a try.  If OK then I will update the doc and
defcustom to make clear that (for now) this works only for
nil and not for a large value.

Feel free to adjust the fix.  Or to extend it to also take a
numeric value into account.  But I will be happy if it is fixed
at least for nil.

[-- Attachment #2: isearch-2015-08-28.patch --]
[-- Type: application/octet-stream, Size: 1378 bytes --]

diff -uw isearch.el isearch-patched-2015-08-28.el
--- isearch.el	2015-08-28 09:47:45.431252800 -0700
+++ isearch-patched-2015-08-28.el	2015-08-28 09:51:40.683708500 -0700
@@ -3092,13 +3092,13 @@
 				(+ isearch-lazy-highlight-start
 				   ;; Extend bound to match whole string at point
 				   (1- (length isearch-lazy-highlight-last-string)))
-			      (window-end)))
+			      (if lazy-highlight-max-at-a-time (window-end) (point-max))))
 		     (max (or isearch-lazy-highlight-start-limit (point-min))
 			  (if isearch-lazy-highlight-wrapped
 			      (- isearch-lazy-highlight-end
 				 ;; Extend bound to match whole string at point
 				 (1- (length isearch-lazy-highlight-last-string)))
-			    (window-start))))))
+			    (if lazy-highlight-max-at-a-time (window-start) (point-max)))))))
 	;; Use a loop like in `isearch-search'.
 	(while retry
 	  (setq success (isearch-search-string
@@ -3142,12 +3142,12 @@
 			  (if isearch-lazy-highlight-forward
 			      (if (= mb (if isearch-lazy-highlight-wrapped
 					    isearch-lazy-highlight-start
-					  (window-end)))
+					  (if max (window-end) (point-max))))
 				  (setq found nil)
 				(forward-char 1))
 			    (if (= mb (if isearch-lazy-highlight-wrapped
 					  isearch-lazy-highlight-end
-					(window-start)))
+					(if max (window-start) (point-min))))
 				(setq found nil)
 			      (forward-char -1)))
 

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

> Am I missing something else?

Eli is right - lazy-highlight is designed to show matches
only on the screen.

> please at least correct the doc and the Customize UI, to make clear
> that the value (numeric or nil) has no effect beyond what is/has been
> shown on the screen - that it is not at all about highlighting "all matches".

A fix for the docstring of ‘lazy-highlight-max-at-a-time’ is to replace

  “A value of nil means highlight all matches.”

with

  “A value of nil means highlight all matches shown on the screen.”

> And given that Lisp code apparently _can_ highlight a whole, large
> buffer (AFAICT), if you have any tips on how I might make this work,
> myself, for Isearch lazy highlighting, please pass them along.

To highlight the whole buffer you can use ‘C-s the M-s h r’
(isearch-highlight-regexp).

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> Eli is right - lazy-highlight is designed to show matches
> only on the screen.

No, that is wrong.  And that is not what Eli said, in any case.
He said that the display engine (not lazy-highlight) is designed to
highlight only what is shown on the screen.

Lazy highlighting seems pretty clearly to have been designed from the
outset with the possibility that you can control how much it highlights,
for _performance_ reasons, and that you can, explicitly, choose to
highlight everywhere if you want.

By default, lazy highlighting does not extend far, and it is removed
when you finish searching.  But both of these default behaviors are
optional, changeable for situations where you want to highlight more
and you want to keep the highlighting after searching, respectively.
I think Eli was mistaken wrt each of these, but that's not important.
 
> A fix for the docstring of ‘lazy-highlight-max-at-a-time’ is to replace
>   “A value of nil means highlight all matches.” with
>   “A value of nil means highlight all matches shown on the screen.”

I disagree strongly that this is proper.  This is not a doc problem.
Clearly the intention of nil `lazy-highlight-max-at-a-time' is to
enable lazy highlighting throughout the search space.

> > And given that Lisp code apparently _can_ highlight a whole, large
> > buffer (AFAICT), if you have any tips on how I might make this work,
> > myself, for Isearch lazy highlighting, please pass them along.
> 
> To highlight the whole buffer you can use ‘C-s the M-s h r’
> (isearch-highlight-regexp).

My request was before I found a fix, and it was to help find a fix
for the bug, not to highlight a regexp everywhere.

What you mention here is totally unrelated to lazy highlighting, not
only in effect but in purpose.  I did not ask how to highlight a
regexp throughout a buffer.  I asked for tips about fixing the bug,
so that lazy highlighting could optionally be applied throughout the
buffer, as intended by a nil value of ‘lazy-highlight-max-at-a-time’.

Please see the simple patch I sent, which I think takes care of this.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Eli Zaretskii]

> Date: Fri, 28 Aug 2015 14:43:11 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 21092@debbugs.gnu.org
> 
> Please see the simple patch I sent, which I think takes care of this.

If we are going to extend highlighting beyond the displayed portion of
the buffer, then your proposed patch needs more work, IMO.  AFAIU,
with your patch, setting lazy-highlight-max-at-a-time to, say, 2000,
will still limit the highlighting to the displayed portion, which
makes little sense to me, as the probability of finding more than 2000
matches in a single window-full is practically zero, and so the user's
intent will not be honored (and the doc string will still be
misleading).

Instead, I suggest to use a special non-nil value, e.g. zero or -1, to
indicate a limit to the current window's end, and treat any other
value literally, disregarding the window limits.  (This will need to
be reflected in the documentation, of course.)  That special value
should IMO be the default.  Or maybe introduce a separate predicate
option for whether to limit to window start and end.

Yes, this would be a backward-incompatible change, but I don't think
the difference will matter too much in most use cases, especially with
the default value of lazy-highlight-cleanup.

Btw, another issue that arises in this regard is whether to highlight
beyond the screen only in the direction of the search (which AFAICS is
what your proposed patch does) or in both directions, especially when
the value of lazy-highlight-max-at-a-time is nil.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > Please see the simple patch I sent, which I think takes care of this.
> 
> If we are going to extend highlighting beyond the displayed portion of
> the buffer, then your proposed patch needs more work, IMO.  AFAIU,
> with your patch, setting lazy-highlight-max-at-a-time to, say, 2000,
> will still limit the highlighting to the displayed portion, which
> makes little sense to me, as the probability of finding more than 2000
> matches in a single window-full is practically zero, and so the user's
> intent will not be honored (and the doc string will still be
> misleading).

Yes.  As I said, more than once, the patch I submitted fixes the bug
only "for a nil value".  And I added:

  Feel free to adjust the fix.  Or to extend it to also take a
  numeric value into account.  But I will be happy if it is fixed
  at least for nil.

My immediate need, for which there is an easy, quick fix, is to make
the option work for nil.

I certainly agree that it would be good to also make it work for large
numeric values - which is why I invited such an additional fix.  That
will be useful.

> Instead, I suggest to use a special non-nil value, e.g. zero or -1, to
> indicate a limit to the current window's end, and treat any other
> value literally, disregarding the window limits.  (This will need to
> be reflected in the documentation, of course.)  That special value
> should IMO be the default.  Or maybe introduce a separate predicate
> option for whether to limit to window start and end.

That sounds OK to me.

Except that you did not explicitly mention nil.  Do you mean, by treat
it literally, that it should behave for nil as it is advertised now:
no limit at all?  If so then I agree.

IOW, I would like nil to behave as advertised: no limit.  This is the
use case that prompted me to file the bug and look for a solution.

> Yes, this would be a backward-incompatible change, but I don't think
> the difference will matter too much in most use cases, especially with
> the default value of lazy-highlight-cleanup.

Because this has never worked before for areas beyond the window, it
does not really present backward compatibility problems.  Unless you
are considering someone who has set the value to 99999999 and continues
to expect highlighting to be limited to the window.

> Btw, another issue that arises in this regard is whether to highlight
> beyond the screen only in the direction of the search (which AFAICS is
> what your proposed patch does) or in both directions, especially when
> the value of lazy-highlight-max-at-a-time is nil.

Good point.  My patch is not good enough here, though it is enough to
temporarily switch directions (e.g., from C-s to C-r) to work around
this incompleteness.

That is the first (next) thing to do, I think: fix things completely
for nil, so that it highlights all search hits (both directions).
I will take another look when I get a chance, if no one beats me
to it.

That would also give users a real difference in behavior between a
very large number and nil, which is good.  There should be a way,
which a very large number would provide, of getting a no-limit
behavior for only the current search direction.   (My patch
currently does this (for nil), but that is not the behavior nil
should provide, IMO.)

I appreciate your interest in this and hope that we do get it
fixed.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

> IOW, I would like nil to behave as advertised: no limit.  This is the
> use case that prompted me to file the bug and look for a solution.

There is no bug.  I have ‘lazy-highlight-max-at-a-time’ set to nil
in my .emacs for years, and it worked as intended to optimize the
performance of the screen-limited lazy-highlighting.  Please don't break
this useful option.  With your proposed changes Isearch will be horribly slow
to highlight all matches in a large buffer on every search state change.

There is no need to highlight all matches in the buffer during Isearch.

If you want a new feature, please create a new feature request
with a subject like ‘Feature request: lazy-hi-lock on Isearch exit’
that will highlight all matches in the buffer after you exit Isearch.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  21092@debbugs.gnu.org
> Date: Sun, 30 Aug 2015 00:10:10 +0300
> 
> > IOW, I would like nil to behave as advertised: no limit.  This is the
> > use case that prompted me to file the bug and look for a solution.
> 
> There is no bug.  I have ‘lazy-highlight-max-at-a-time’ set to nil
> in my .emacs for years, and it worked as intended to optimize the
> performance of the screen-limited lazy-highlighting.  Please don't break
> this useful option.  With your proposed changes Isearch will be horribly slow
> to highlight all matches in a large buffer on every search state change.
> 
> There is no need to highlight all matches in the buffer during Isearch.

What if someone wants to?

> If you want a new feature, please create a new feature request
> with a subject like ‘Feature request: lazy-hi-lock on Isearch exit’
> that will highlight all matches in the buffer after you exit Isearch.

How about leaving the current meaning of nil as it is, i.e. limit the
highlighting to the visible portion of the buffer, and introducing a
special value like zero or -1 to mean highlight the whole buffer?

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > > IOW, I would like nil to behave as advertised: no limit.  This is the
> > > use case that prompted me to file the bug and look for a solution.
> >
> > There is no bug.  I have ‘lazy-highlight-max-at-a-time’ set to nil
> > in my .emacs for years, and it worked as intended to optimize the
> > performance of the screen-limited lazy-highlighting.  Please don't break
> > this useful option.  With your proposed changes Isearch will be horribly
> > slow to highlight all matches in a large buffer on every search state
> > change.
> >
> > There is no need to highlight all matches in the buffer during Isearch.
> 
> What if someone wants to?
> 
> > If you want a new feature, please create a new feature request
> > with a subject like ‘Feature request: lazy-hi-lock on Isearch exit’
> > that will highlight all matches in the buffer after you exit Isearch.
> 
> How about leaving the current meaning of nil as it is, i.e. limit the
> highlighting to the visible portion of the buffer, and introducing a
> special value like zero or -1 to mean highlight the whole buffer?

As I said earlier, I'm all for adding additional specific arguments
for different behaviors - whatever is needed.

But "the current meaning of nil", as it is documented, is not what
the behavior is.  I respect Juri's argument that the bug is quite
old and that some people might be used to nil not doing what has been
advertised for it and want it to continue doing what it has long been
doing.  That's a reasonable argument.

Personally, I would prefer that nil be made to act as advertised, but
it is OK with me if the decision is to keep the nil behavior as it is
now, and so change its meaning from what is currently documented (IOW,
change the doc and Customize UI so that they fit what nil currently
does), as long as there is some (new) value for the option that does
what the doc has been saying nil does: highlight all search hits -
no limit on the highlighting.

I don't care whether you call this a new feature or a bug fix.
To me it is a bug fix, as indications are that the fixed behavior
was the originally intended behavior.  But who really knows?  It
doesn't matter to me what we call it.

To me it is very useful to be able to have lazy highlighting
highlight all search hits.  To mention a use case, in case it makes
any difference to you:

I have code that lets you hit a key to search again (different
search pattern) during the same or a subsequent Isearch invocation,
and have this second search be limited _within_ the previous
search hits.  That is, the lazy-highlight zones from one search act
as the search contexts (limits) for the next search.  You can
repeat this etc.

This is a kind of progressive narrowing.

And I have code that lets you search within defined areas (think
of multiple regions), and there too you can hit a (different)
key to refine the search.  In this case, whole contexts that did
not have a search hit from the first search are removed as
candidates for the next search.  IOW, the current search hits
narrow the set of contexts for subsequent searching.

This is another kind of progressive narrowing.

In both cases, subsequent searches may well hit places that
were offscreen during previous searches, because the addition
of constraints (reduction of contexts) can reduce the matches.

In this use case, it can be (and typically is) useful to
lazy-highlight all search hits and (typically) to turn off
`lazy-highlight-cleanup'.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

>> There is no need to highlight all matches in the buffer during Isearch.
>
> What if someone wants to?

Why would someone want such a strange thing?

Drew wanted to highlight all matches in the buffer only
on quitting Isearch that is easily achievable with just

(add-hook 'isearch-mode-end-hook
	  (lambda () (highlight-regexp (regexp-quote isearch-string)
                                       'lazy-highlight)))

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> >> There is no need to highlight all matches in the buffer
> >> during Isearch.
> >
> > What if someone wants to?
> 
> Why would someone want such a strange thing?

I gave a pretty detailed explanation of a reasonable use
case.

> Drew wanted to highlight all matches in the buffer only
> on quitting Isearch

Not at all.  The narrowing I mentioned can be done, as I
said, during the same Isearch invocation.  That is the
more typical case, in fact.  It is why I bind Isearch keys
to the narrowing operations.

Tt doesn't seem that you really want to know why anyone
would want "such a strange thing".  _You_ don't want it.
That much seems clear.

Sigh, another feature that will remain only for Isearch+,
I guess.  To me, this is not an additional feature.  It is
a bug fix.  But I won't argue about it, and it's beside
the point now.  You clearly do not want this done.

You have not wanted a boatload of features, whether it's
searching within the active region (not needed, since a
user can always narrow the buffer); search within text-
or overlay-property zones; on-demand replacement of
search hits during search;...  NIH?

Please at least fix the doc so that it matches what the
code does, whether or not that matches what the original
intention was.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

> You clearly do not want this done.

Not at all.  I only disagree that it's a bug.
‘lazy-highlight-max-at-a-time’ is just an optimization variable
along with ‘lazy-highlight-initial-delay’, ‘lazy-highlight-interval’
that users might want to tweak to improve performance where nil of
‘lazy-highlight-max-at-a-time’ allows highlighting in one loop
that avoids delays on fast processors (it makes sense to change
its default value to nil to match the modern hardware).

A new feature to highlight the whole buffer would be useful as well
to reduce flickering during Isearch.  Currently when e.g. scrolling
by one line lazy-highlight clears all matches on the screen, and
re-highlights them with an additional line taken into account.
That's because lazy-highlight is non-incremental.

Highlighting the whole buffer could fix this visual effect.
So unlike the current screen-limited lazy-highlighting that reacts
to window-start/window-end changes, once whole-buffer lazy-highlighting
finds all matches in the buffer it doesn't need to re-highlight them
during Isearch navigation.

A new option could be named e.g. (defcustom lazy-highlight-buffer nil)
Again, its default value is depending on the average hardware.
While I believe that highlighting all matches on the screen
is suitable for most users, I'm not sure about highlighting
all matches in a (possibly large) buffer in one loop.

> You have not wanted a boatload of features,

Yes, I resist a bloatload of features, but I'm happy with adding
features that solve a complex usability problem in a simple way.

> whether it's searching within the active region (not needed,
> since a user can always narrow the buffer);

Because it's more useful to extend the bounds of the active region
using Isearch.

> search within text- or overlay-property zones;

There is an unfinished feature request bug#15245.

> on-demand replacement of search hits during search

Can you find a bug# for this?

> Please at least fix the doc so that it matches what the
> code does, whether or not that matches what the original
> intention was.

Then the docstring could point to the new option ‘lazy-highlight-buffer’.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> > You clearly do not want this done.
> 
> Not at all.  I only disagree that it's a bug.

Good to hear.  But it sure didn't sound like it:

  J >>> There is no need to highlight all matches in the buffer
  J >>> during Isearch.
  E >> What if someone wants to?
  J > Why would someone want such a strange thing?

Anyway, I'm glad you agree now that such a strange thing can be useful.

> ‘lazy-highlight-max-at-a-time’ is just an optimization variable
> along with ‘lazy-highlight-initial-delay’, ‘lazy-highlight-interval’
> that users might want to tweak to improve performance where nil of
> ‘lazy-highlight-max-at-a-time’ allows highlighting in one loop
> that avoids delays on fast processors (it makes sense to change
> its default value to nil to match the modern hardware).
> 
> A new feature to highlight the whole buffer would be useful as well
> to reduce flickering during Isearch.  Currently when e.g. scrolling
> by one line lazy-highlight clears all matches on the screen, and
> re-highlights them with an additional line taken into account.
> That's because lazy-highlight is non-incremental.

Whole buffer OR rest-of-buffer forward or backward.  Au choix.
Both behaviors are useful.

> Highlighting the whole buffer could fix this visual effect.
> So unlike the current screen-limited lazy-highlighting that reacts
> to window-start/window-end changes, once whole-buffer lazy-highlighting
> finds all matches in the buffer it doesn't need to re-highlight them
> during Isearch navigation.
> 
> A new option could be named e.g. (defcustom lazy-highlight-buffer nil)
> Again, its default value is depending on the average hardware.
> While I believe that highlighting all matches on the screen
> is suitable for most users, I'm not sure about highlighting
> all matches in a (possibly large) buffer in one loop.

It's not clear how you see the two options interacting (or not).

If you are telling Isearch to highlight all matches and you are
also telling it to highlight at most 10, for example.  And what
about the current nil value of `lazy-highlight-max-at-a-time',
which also says (currently) that it means highlight "all"?

Not clear why you want a separate option for this.  It cannot be
used in combination with `lazy-highlight-max-at-a-time', can it?

If the behaviors are mutually exclusive, then why not just add
another value or two for `lazy-highlight-max-at-a-time'?

A second question concerns how to control whether this acts
only in the current search direction or in both directions.

In the patch I sent, a user gets all matches in the search
direction.  If s?he wants all matches in both directions it is
enough to briefly hit the other direction key (e.g. hit `C-r'
if searching forward).

The use I make of the all-matches behavior will be togglable, 
and I will want to control what the behaviors toggled are.

For exmaple, I might want a toggle between max=10 and
whole-buffer.  Or between whole-buffer and remainder-of-buffer
(in search direction).

> > whether it's searching within the active region (not needed,
> > since a user can always narrow the buffer);
>
> Because it's more useful to extend the bounds of the active
> region using Isearch.

Dunno what that means.  Please elaborate.  And please say why
you think it is always more useful than limiting search to the
region.

Whatever you mean by it, why should it be a question only of
design-time either-or?

In Isearch+, limitation of search to the active region is
optional, and you can change the behavior anytime on the fly
(toggle `C-x n').  (Deactivation of the active region is also
optional.)

> > search within text- or overlay-property zones;
> 
> There is an unfinished feature request bug#15245.

On-demand replacement of search hits is available in Isearch+.
At the time I replied in that bug thread, I guess I had not yet
added this feature.  It seems I added it a month after I posted
there.  Anyway, that too is available.  AFAIK, with Isearch+ you
can do all that is requested in the bug.

And as you say in the bug thread, you can also limit
`query-replace' to such text zones using `isearch-filter-predicate'.

> > on-demand replacement of search hits during search
> 
> Can you find a bug# for this?

It's not a bug, AFAIK.  It's an Isearch+ feature.
http://www.emacswiki.org/emacs/IsearchPlus#isearchp-replace-on-demand

You can perform an arbitrary action on the current search hit,
on demand.  By default, the action is replacement.

> > Please at least fix the doc so that it matches what the
> > code does, whether or not that matches what the original
> > intention was.
> 
> Then the docstring could point to the new option
> ‘lazy-highlight-buffer’.

See above.  I'm all for adding the ability to highlight beyond
the current window, and I am glad to hear you are also for it.

But it's unclear why a separate option should be used, instead
of additional option values for `lazy-highlight-max-at-a-time'.

And it's unclear how a user can get up-to-the-buffer/region-limit
behavior in one direction only.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

>> > You clearly do not want this done.
>>
>> Not at all.  I only disagree that it's a bug.
>
> Good to hear.  But it sure didn't sound like it:
>
>   J >>> There is no need to highlight all matches in the buffer
>   J >>> during Isearch.
>   E >> What if someone wants to?
>   J > Why would someone want such a strange thing?
>
> Anyway, I'm glad you agree now that such a strange thing can be useful.

I tried to find a useful application for this feature, and finally found.

> Whole buffer OR rest-of-buffer forward or backward.  Au choix.
> Both behaviors are useful.

Since you can scroll up or down, highlighting the whole buffer is unavoidable.

> Not clear why you want a separate option for this.  It cannot be
> used in combination with `lazy-highlight-max-at-a-time', can it?

Of course, it is useful in combination with ‘lazy-highlight-max-at-a-time’
that can define the incrementality (how many steps to do) regardless
whether highlighting the whole buffer or only matches on the screen.

> A second question concerns how to control whether this acts
> only in the current search direction or in both directions.

We can highlight only in the search direction because regexp
search might match different results depending on direction.
So you need to switch directions with ‘C-r’ to re-highlight
the buffer.

> For exmaple, I might want a toggle between max=10 and
> whole-buffer.

This will be possible with two separate variables:
the existing ‘lazy-highlight-max-at-a-time’
to toggle between max=10 and max=nil
and the new ‘lazy-highlight-buffer’
to toggle between whole-buffer and screen-only
implemented by this small patch:

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 8d4bf24..a53b314 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -332,6 +332,13 @@ (defcustom lazy-highlight-max-at-a-time 20
 		 (integer :tag "Some"))
   :group 'lazy-highlight)
 
+(defcustom lazy-highlight-buffer nil
+  "Highlight the whole buffer (for `lazy-highlight').
+A value of nil means highlight all matches on the screen.
+A value of t means highlight all matches in the whole buffer."
+  :type 'boolean
+  :group 'lazy-highlight)
+
 (defface lazy-highlight
   '((((class color) (min-colors 88) (background light))
      (:background "paleturquoise"))
@@ -3029,10 +3040,12 @@ (defun isearch-lazy-highlight-new-loop (&optional beg end)
 			  isearch-lax-whitespace))
 		 (not (eq isearch-lazy-highlight-regexp-lax-whitespace
 			  isearch-regexp-lax-whitespace))
-                 (not (= (window-start)
-                         isearch-lazy-highlight-window-start))
-                 (not (= (window-end)   ; Window may have been split/joined.
-			 isearch-lazy-highlight-window-end))
+                 (if lazy-highlight-buffer nil
+                   (not (= (window-start)
+                           isearch-lazy-highlight-window-start)))
+                 (if lazy-highlight-buffer nil
+                   (not (= (window-end) ; Window may have been split/joined.
+                           isearch-lazy-highlight-window-end)))
 		 (not (eq isearch-forward
 			  isearch-lazy-highlight-forward))
 		 ;; In case we are recovering from an error.
@@ -3092,13 +3105,13 @@ (defun isearch-lazy-highlight-search ()
 				(+ isearch-lazy-highlight-start
 				   ;; Extend bound to match whole string at point
 				   (1- (length isearch-lazy-highlight-last-string)))
-			      (window-end)))
+			      (if lazy-highlight-buffer (point-max) (window-end))))
 		     (max (or isearch-lazy-highlight-start-limit (point-min))
 			  (if isearch-lazy-highlight-wrapped
 			      (- isearch-lazy-highlight-end
 				 ;; Extend bound to match whole string at point
 				 (1- (length isearch-lazy-highlight-last-string)))
-			    (window-start))))))
+			    (if lazy-highlight-buffer (point-min) (window-start)))))))
 	;; Use a loop like in `isearch-search'.
 	(while retry
 	  (setq success (isearch-search-string
@@ -3142,12 +3155,12 @@ (defun isearch-lazy-highlight-update ()
 			  (if isearch-lazy-highlight-forward
 			      (if (= mb (if isearch-lazy-highlight-wrapped
 					    isearch-lazy-highlight-start
-					  (window-end)))
+					  (if lazy-highlight-buffer (point-max) (window-end))))
 				  (setq found nil)
 				(forward-char 1))
 			    (if (= mb (if isearch-lazy-highlight-wrapped
 					  isearch-lazy-highlight-end
-					(window-start)))
+					(if lazy-highlight-buffer (point-min) (window-start))))
 				(setq found nil)
 			      (forward-char -1)))
 
@@ -3174,12 +3187,12 @@ (defun isearch-lazy-highlight-update ()
 		      (setq isearch-lazy-highlight-wrapped t)
 		      (if isearch-lazy-highlight-forward
 			  (progn
-			    (setq isearch-lazy-highlight-end (window-start))
+			    (setq isearch-lazy-highlight-end (if lazy-highlight-buffer (point-min) (window-start)))
 			    (goto-char (max (or isearch-lazy-highlight-start-limit (point-min))
-					    (window-start))))
-			(setq isearch-lazy-highlight-start (window-end))
+					    (if lazy-highlight-buffer (point-min) (window-start)))))
+			(setq isearch-lazy-highlight-start (if lazy-highlight-buffer (point-max) (window-end)))
 			(goto-char (min (or isearch-lazy-highlight-end-limit (point-max))
-					(window-end))))))))
+					(if lazy-highlight-buffer (point-max) (window-end)))))))))
 	    (unless nomore
 	      (setq isearch-lazy-highlight-timer
 		    (run-at-time lazy-highlight-interval nil

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Drew Adams]

> I tried to find a useful application for this feature, and finally found.

Great.  What's the application?

> > Whole buffer OR rest-of-buffer forward or backward.  Au choix.
> > Both behaviors are useful.
> 
> Since you can scroll up or down, highlighting the whole buffer is
> unavoidable.

I don't understand.  Can you elaborate a bit?  With the patch I
sent it seemed to be avoided.  When searching forward, the only
matches before point that got highlighted were those in the window.

IIUC, this is not important to me, but I would like to understand.

> > Not clear why you want a separate option for this.  It cannot be
> > used in combination with `lazy-highlight-max-at-a-time', can it?
> 
> Of course, it is useful in combination with ‘lazy-highlight-max-at-a-time’
> that can define the incrementality (how many steps to do) regardless
> whether highlighting the whole buffer or only matches on the screen.

I see now that I misunderstood what was meant by "at a time" in
"max-at-a-time".

I was thinking that it referred to all of the highlighting for
a given input (e.g. after an input change).  Instead, it refers
to one invocation of `isearch-lazy-highlight-update', whose loop
runs only `lazy-highlight-max-at-a-time' iterations.  And `*-update'
can get reinvoked by the timer to complete the highlighting for the
input.

> > A second question concerns how to control whether this acts
> > only in the current search direction or in both directions.
> 
> We can highlight only in the search direction because regexp
> search might match different results depending on direction.
> So you need to switch directions with ‘C-r’ to re-highlight
> the buffer.

You just said that "highlighting the whole buffer is unavoidable",
so I am a bit confused.  Now you seem to be saying that we can
highlight only in one direction at a time.  Maybe you could
elaborate a bit here?

> > For exmaple, I might want a toggle between max=10 and
> > whole-buffer.
> 
> This will be possible with two separate variables:
> the existing ‘lazy-highlight-max-at-a-time’
> to toggle between max=10 and max=nil
> and the new ‘lazy-highlight-buffer’
> to toggle between whole-buffer and screen-only
> implemented by this small patch:

I misunderstood max-at-a-time.  So maybe the doc for it needs
clarifying wrt what is meant by at a time etc.

The doc and defcustom need fixing anyway, for the confusion
over the meaning of "all matches" etc., discussed previously.

Anyway, your patch looks good.  It is essentially the same
as mine, but using `lazy-highlight-buffer' instead of a nil
value of `lazy-highlight-max-at-a-time'.  I understand now
that they have different purposes.

The difference is that I did not change the occurrences of
`window-(start|end)' in `isearch-lazy-highlight-new-loop'.
I wasn't sure it was needed (not fully understanding it),
and I didn't notice a problem without it.  But I'm glad that
you, knowing more, made the same change there as well.

Thx - Drew

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

>> I tried to find a useful application for this feature, and finally found.
>
> Great.  What's the application?

Reducing flicker.

>> > Whole buffer OR rest-of-buffer forward or backward.  Au choix.
>> > Both behaviors are useful.
>>
>> Since you can scroll up or down, highlighting the whole buffer is
>> unavoidable.
>
> I don't understand.  Can you elaborate a bit?

With ‘isearch-allow-scroll’ you can scroll backwards to previous
parts of the buffer that need to display highlighted matches too.

>> > A second question concerns how to control whether this acts
>> > only in the current search direction or in both directions.
>>
>> We can highlight only in the search direction because regexp
>> search might match different results depending on direction.
>> So you need to switch directions with ‘C-r’ to re-highlight
>> the buffer.
>
> You just said that "highlighting the whole buffer is unavoidable",
> so I am a bit confused.  Now you seem to be saying that we can
> highlight only in one direction at a time.  Maybe you could
> elaborate a bit here?

Highlighting the whole buffer needs to be in the same direction
as the current search direction.  But ‘isearch-lazy-highlight-update’
already takes care about this.

> The difference is that I did not change the occurrences of
> `window-(start|end)' in `isearch-lazy-highlight-new-loop'.
> I wasn't sure it was needed (not fully understanding it),
> and I didn't notice a problem without it.

You can see the difference when scrolling by one line
with e.g. ‘scroll-down-line’.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

> the new ‘lazy-highlight-buffer’ to toggle between whole-buffer and
> screen-only implemented by this small patch:

Commenting out window-local lazy-highlighting overlay in bug#17453

 			  ;; 1000 is higher than ediff's 100+,
 			  ;; but lower than isearch main overlay's 1001
 			  (overlay-put ov 'priority 1000)
-			  (overlay-put ov 'face lazy-highlight-face)
-			  (overlay-put ov 'window (selected-window))))
+			  (overlay-put ov 'face lazy-highlight-face)))
+			  ;(overlay-put ov 'window (selected-window))))

produces such deficiency that when two windows are displaying different parts
of the same buffer (without using follow-mode), sometimes lazy-highlighted
matches are partially (i.e. not all matches) displayed in non-current
window when lazy-highlighting boundary of the selected window is in the
middle of other window.  This causes false impression that other window is
responsible to highlight all visible matches but fails to do so.

A solution is to install the 3rd patch posted by Artur in
http://debbugs.gnu.org/17453#89 that adds a new option ‘all-windows’ to
‘isearch-lazy-highlight’.  Then we could setq-local it to ‘all-windows’
in ‘follow-mode’, and don't highlight matches in other windows by default
(keeping its old behavior).

The same solution could be applied to bug#21092, to add a new option
‘buffer’ to ‘isearch-lazy-highlight’ to lazy-highlight the whole buffer.
Then ‘follow-mode’ should check it, and override it with buffer-local
‘all-windows’ only when it was ‘t’ initially.

bug#21092: 25.0.50; Option `lazy-highlight-max-at-a-time' does not work

[Juri Linkov]

> The same solution could be applied to bug#21092, to add a new option
> ‘buffer’ to ‘isearch-lazy-highlight’ to lazy-highlight the whole buffer.
> Then ‘follow-mode’ should check it, and override it with buffer-local
> ‘all-windows’ only when it was ‘t’ initially.

I fixed the docstring of ‘lazy-highlight-max-at-a-time’
mentioned in the subject line, and closing this bug.

Drew, if you think we need to add a new option ‘buffer’ to
‘isearch-lazy-highlight’, then please create a separate feature request.