bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

1. Non-nil `isearch-allow-scroll' lets you use a scroll command
(e.g. `C-v') without exiting Isearch.  Unfortunately, this is coupled
with the hard-coded behavior that you cannot scroll far enough in either
direction that point would be moved off screen.

That restriction is general for Emacs, and it generally makes sense.
It does not necessarily make sense during Isearch, however.  Why?

Because the search position is one thing, and it is not lost.  What you
might want to look at temporarily is another thing.

It's a bit like using `C-SPC' in a buffer, scrolling up a couple of
screenfuls to look at something, and then using `C-u C-SPC' to return.
But in Isearch there is no need for `C-SPC' or `C-u C-SPC': the search
position is recorded.  Search resumes from that same position, no
matter how far away one might have scrolled.

The enhancement request is to let users choose whether non-nil
`isearch-allow-scroll' should limit you to scrolling only enough to keep
point in the window or should not limit you.  This could be done by
recognizing different non-nil values.

2. What's more, the lazy highlighting of search hits is even more
limited currently.  When you scroll to the current limit, there can be
lots of search hits that are not highlighted.

One of the reasons for scrolling is to see what search hits lie beyond
the currently shown text.  This is already partly defeated, in that
even when you scroll some of the hits are not highlighted.  This seems
more like a bug, but if the enhancement of #1 is done then it should
(it needs to) take care of #2 as well: Whatever text is shown should
have all of its search hits highlighted with lazy highlighting.

In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-10-19 on LEG570
Bzr revision: 114715 rgm@gnu.org-20131019023520-s8mwtib7xcx9e05w
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking 'CFLAGS=-O0 -g3' CPPFLAGS=-DGLYPH_DEBUG=1'

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> 1. Non-nil `isearch-allow-scroll' lets you use a scroll command
> (e.g. `C-v') without exiting Isearch.  Unfortunately, this is coupled
> with the hard-coded behavior that you cannot scroll far enough in either
> direction that point would be moved off screen.

You can do what you want with just:

  (advice-add 'isearch-post-command-hook :override (lambda ()))

And if you want more commands to escape this restriction:

  (mapc (lambda (c) (put c 'isearch-scroll t))
        '(forward-char backward-char right-char left-char
          forward-word backward-word right-word left-word
          forward-sexp backward-sexp forward-paragraph backward-paragraph
          move-end-of-line end-of-visual-line move-beginning-of-line
          beginning-of-visual-line next-line previous-line))

> That restriction is general for Emacs, and it generally makes sense.
> It does not necessarily make sense during Isearch, however.  Why?

Because it is too confusing for users.  This is like leaving point
in one place, and scrolling without changing the position of point
(with inactive Isearch).  Isearch should not be different from the
default Emacs behavior.

> It's a bit like using `C-SPC' in a buffer, scrolling up a couple of
> screenfuls to look at something, and then using `C-u C-SPC' to return.
> But in Isearch there is no need for `C-SPC' or `C-u C-SPC': the search
> position is recorded.  Search resumes from that same position, no
> matter how far away one might have scrolled.

It makes sense to resume search from a new position like you can see
using code above.

> The enhancement request is to let users choose whether non-nil
> `isearch-allow-scroll' should limit you to scrolling only enough to keep
> point in the window or should not limit you.  This could be done by
> recognizing different non-nil values.

Maybe a new option of `isearch-allow-scroll' could allow this.

> 2. What's more, the lazy highlighting of search hits is even more
> limited currently.  When you scroll to the current limit, there can be
> lots of search hits that are not highlighted.

When scrolling outside the window boundaries will be allowed then
lazy highlighting should highlight the whole buffer so you could see
all matches when you quickly scroll the buffer.  But in this case
lazy highlighting will become more like hi-lock mode.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > 1. Non-nil `isearch-allow-scroll' lets you use a scroll command
> > (e.g. `C-v') without exiting Isearch.  Unfortunately, this is
> > coupled with the hard-coded behavior that you cannot scroll far
> > enough in either direction that point would be moved off screen.
> 
> You can do what you want with just:
>   (advice-add 'isearch-post-command-hook :override (lambda ()))

Good to know that.  But see also the bug(s) mentioned below.
Such advice will not suffice currently, for the reasons given
there.  There is neither the proper management of point nor
the proper lazy highlighting of the scrolled area.

But anyway, I am not looking for a way for an individual user to
advise the code - in this case, dynamically neutering the
post-command scrolling limitation code.  That's not the point
(besides not being very elegant).

I am asking for a way for users to customize an option, namely
the already existing option `isearch-allow-scroll'.  I am asking
for two scroll-allowing behaviors, giving users an easy choice.

> And if you want more commands to escape this restriction:
>   (mapc (lambda (c) (put c 'isearch-scroll t))
>         '(forward-char backward-char right-char left-char
>           forward-word backward-word right-word left-word
>           forward-sexp backward-sexp forward-paragraph backward-
>           paragraph
>           move-end-of-line end-of-visual-line move-beginning-of-line
>           beginning-of-visual-line next-line previous-line))

I'm not asking how to define additional scrolling commands.  The
problem is not insufficient commands that can scroll.  The problem
is the limitation of what the scrolling behavior is.

> > That restriction is general for Emacs, and it generally makes
> > sense.  It does not necessarily make sense during Isearch,
> > however.  Why?

Note the question (which I answered): Why does this restriction,
which generally makes sense for general Emacs, NOT necessarily make
sense for Isearch.

Is this your answer to that question (?):

> Because it is too confusing for users.  This is like leaving point
> in one place, and scrolling without changing the position of point
> (with inactive Isearch).  Isearch should not be different from the
> default Emacs behavior.

I gave a reason why it should be different.  Or rather, why it
should be allowed to be different (au choix).

You gave no reason - you just repeated that it should be the same.
At most, you gave the same reason as for Emacs in general, which I
explained is not so relevant here.  That is, it is not cut & dried.

Some users can well prefer to be able to see more context, for the
reasons I gave.  And that is not so confusing as it is for general
Emacs, for the reasons I gave - in particular, resumption from the
same search position, with attendent automatic window return to
that position.

In Emacs in general, if scrolling moved beyond point, and if there
were nothing that brought the window back to point when you acted
at point (e.g., when finished temporarily scrolling) then you
would not see what was happening at point.  That would not be good
at all, under any circumstances.

But even in Emacs in general, if there were such an thing as 
temporary scrolling, with a well defined finish, and if the window
were then automatically restored to show point, which *did not
change* by scrolling, that too would not be so confusing.  We do
not have such a temporary-scrolling behavior in Emacs in general,
however.

You cannot reasonably compare general Emacs, without any such
temporary scrolling and window restoration, with what I proposed
for Isearch.  Apples & oranges.

> > It's a bit like using `C-SPC' in a buffer, scrolling up a couple
> > of screenfuls to look at something, and then using `C-u C-SPC'
> > to return.  But in Isearch there is no need for `C-SPC' or
> > `C-u C-SPC': the search position is recorded.  Search resumes
> > from that same position, no matter how far away one might have
> > scrolled.
> 
> It makes sense to resume search from a new position like you can
> see using code above.

I think you are suggesting that the search position should change
when you make use of the temporary scrolling?  If so, I strongly
disagree.

From my point of view, the current search hit (i.e., point) does
not change now, and it should never change, when you scroll.
The only thing that would change with my proposal is what is
shown in the window.  The scrolling would be temporary, and when
search is resumed the window would be restored to where it will
show the current search hit again.

What should happen if, after scrolling, a user quits Isearch?
For that case we have a design choice: either move point to
somewhere inside where the window is currently (scrolled), or
move the window back to show the current search hit, i.e.,
point (which has not changed).

I would be in favor of the latter.  I think the former possibility
would be problematic.  Move point to which position in the window,
for instance?

I'm open to suggestions about that.  But a priori, I would suggest
that in all cases when scrolling is done during Isearch the window
be restored to show the current search hit.

I feel more strongly about resumption of search after scrolling:
it should definitely continue where it was.  I think (less
strongly) that if search is quit after scrolling then the behavior
should be the same as quitting at the current search position.

> > The enhancement request is to let users choose whether non-nil
> > `isearch-allow-scroll' should limit you to scrolling only enough
> > to keep point in the window or should not limit you.  This could
> > be done by recognizing different non-nil values.
> 
> Maybe a new option of `isearch-allow-scroll' could allow this.

That's exactly what I suggested.  Non-nil always means allow
scrolling, and the particular non-nil value would specify the
scrolling behavior: limited or not.  But see above and below about
what unlimited scrolling behavior should be like (highlighting, and
no change in point).
 
> > 2. What's more, the lazy highlighting of search hits is even more
> > limited currently.  When you scroll to the current limit, there
> > can be lots of search hits that are not highlighted.
> 
> When scrolling outside the window boundaries will be allowed then
> lazy highlighting should highlight the whole buffer so you could see
> all matches when you quickly scroll the buffer.  But in this case
> lazy highlighting will become more like hi-lock mode.

I cannot speak to the implementation - whether it is better to
highlight the whole buffer or just the part that is currently visible.
I would imagine that the latter is better, especially for large
buffers.  But I'm no expert on the implementation of Isearch.

I can say that without the continuation of lazy highlighting into the
scrolled area (e.g., what happens with your advice, above), there is
little use to scrolling.  The point of scrolling is, especially, to
see search hits farther down or up the buffer.

3. I just noticed the following problem (emacs -Q).  I can file a
separate bug report for it, if you prefer.

a. Set `isearch-allow-scroll' to t.

b. Go to the middle of a long (multi-screenful) buffer, such as
isearch.el.

c. Search for something common, such as "ear".

d. `C-v' a couple times, until you hit the limit.

e. `M-v' repeatedly.  There is no such limit in this direction.
Why not?  There should be.  Not really consistent for the user.

f. `C-v'.  The entire window fills with face `isearch' (which should
be only for `ear' search hits).

Both (e) - its difference from (d) - and (f) seem like bugs to me.

With my request (#1), both `C-v' and `M-v' would allow unlimited
scrolling.  And the current search position would not change by
scrolling.

It seems that currently `M-v' (past what should be the limit)
changes the search position.  Whether that's a third bug - call it
(g) or is part of the (e) behavior I don't know.

And that is also the behavior for `C-v' when I advise the
post-command hook as you suggested: point is kept in the window,
instead of staying at the current search hit.  Isearch behavior
should always be that point remains at the current search hit.

IMO, when scrolling is restricted, that restriction should apply
to both `C-v' and `M-v', and the search position should not change.
When scrolling is not restricted (request #1), that non-restriction
should apply to both `C-v' and `M-v',  and the search position
should (still) not change.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Stefan Monnier]

> That restriction is general for Emacs, and it generally makes sense.

It makes sense, but it would also be desirable to make it customizable.

> It does not necessarily make sense during Isearch, however.  Why?

To some extent I can see that it is slightly different in the Isearch
context.  I'm not sure if it is sufficiently different to justify
changing the default in Isearch, but I'm not necessarily opposed to
it either.

IOW, I agree with both feature requests.

> 2. What's more, the lazy highlighting of search hits is even more
> limited currently.  When you scroll to the current limit, there can be
> lots of search hits that are not highlighted.

Yes, that's a bug.


        Stefan

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > That restriction is general for Emacs, and it generally makes
> > sense.
> 
> It makes sense, but it would also be desirable to make it
> customizable.

I agree.

> > It does not necessarily make sense during Isearch, however.  Why?
> 
> To some extent I can see that it is slightly different in the
> Isearch context.  I'm not sure if it is sufficiently different to
> justify changing the default in Isearch, but I'm not necessarily
> opposed to it either.

No need to change the default.  `isearch-allow-scroll' should default
to nil (less confusing, probably).  The only change for it would be
to provide a special value, e.g., `unlimited', which would not impose
a limit on how much you can scroll.

(Scrolling beyond the current limit when the value is `unlimited'
should lazy-highlight whatever is shown, and resuming search should
resume from the search hit current before scrolling.  IOW, scrolling
should not change which search hit is current.)

> IOW, I agree with both feature requests.

Great.

> > 2. What's more, the lazy highlighting of search hits is even more
> > limited currently.  When you scroll to the current limit, there
> > can be lots of search hits that are not highlighted.
> 
> Yes, that's a bug.

I don't see that all the time, BTW.  I don't have a recipe to repro it.

The more important bugs are these:

a. Scrolling backwards is not limited currently (it should be
unlimited only when the option value is `unlimited', i.e., after the
requested enhancement).

b. Forward scrolling after backward scrolling throws everything off
currently: the highlighting that should apply only to the current
search hit (face `isearch') is applied to the entire window (buffer?).

Bug (b) is the most serious, but I'm guessing that (b) and (a) are
due to the same code problem.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> The more important bugs are these:
> 
> a. Scrolling backwards is not limited currently (it should be
> unlimited only when the option value is `unlimited', i.e., after the
> requested enhancement).
> 
> b. Forward scrolling after backward scrolling throws everything off
> currently: the highlighting that should apply only to the current
> search hit (face `isearch') is applied to the entire window
> (buffer?).
> 
> Bug (b) is the most serious, but I'm guessing that (b) and (a) are
> due to the same code problem.

(BTW, (b) is the case symmetrically: if you start searching backward
and then scroll up (in the search direction) and then scroll down, you
get the same extension of `isearch-overlay' across the whole window.)

I've tried to look into (b) a bit.  I tried adding some code that calls
`isearch-dehighlight' during scrolling and `isearch-highlight' when
finished scrolling, just to see.  I added calls to `message' to print
the value of `isearch-overlay' at various points.

It turns out that (because of the call I added to dehighlight) the
overlay is properly deleted (overlay in no buffer) but it is still
showing.  (And it extends across the window.)  I tried adding
(redisplay t), but that had no effect.

So this is apparently a case where a deleted overlay is showing.
What's a way to prevent this phenomenon?

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> 2. What's more, the lazy highlighting of search hits is even more
> limited currently.  When you scroll to the current limit, there can be
> lots of search hits that are not highlighted.
>
> One of the reasons for scrolling is to see what search hits lie beyond
> the currently shown text.  This is already partly defeated, in that
> even when you scroll some of the hits are not highlighted.  This seems
> more like a bug, but if the enhancement of #1 is done then it should
> (it needs to) take care of #2 as well: Whatever text is shown should
> have all of its search hits highlighted with lazy highlighting.

Drew, do you agree that this your request is implemented now
with lazy-highlight-buffer, so bug#15839 could be closed?

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > 2. What's more, the lazy highlighting of search hits is even more
> > limited currently.  When you scroll to the current limit, there can be
> > lots of search hits that are not highlighted.
> >
> > One of the reasons for scrolling is to see what search hits lie beyond
> > the currently shown text.  This is already partly defeated, in that
> > even when you scroll some of the hits are not highlighted.  This seems
> > more like a bug, but if the enhancement of #1 is done then it should
> > (it needs to) take care of #2 as well: Whatever text is shown should
> > have all of its search hits highlighted with lazy highlighting.
> 
> Drew, do you agree that this your request is implemented now
> with lazy-highlight-buffer, so bug#15839 could be closed?

Not as far as I understand.  That #2 is only one point (one
part) of several making up the bug report.

If scrolling forward and backward always shows all search hits
lazy-highlighted, then that one part of the bug would seem to
be fixed.

But that fixed behavior is (needs to be) anyway independent of
any customization of `lazy-highlight-buffer'.  So no, the
addition of option `lazy-highlight-buffer' in no way fixes this
bug.

Perhaps some infrastructure that has been added now provides
for a way to fix part of this bug - dunno.  By that I mean only
that now Isearch has ways of highlighting more than just a
windowful of the buffer.  Dunno whether that is helpful here.

And as I said in the bug report, even for this one part of it
(all visible search hits are not necessarily lazy-highlighted),
the lazy-highlighting needed for scrolling could be limited to
what is actually made visible by scrolling - not the whole
buffer.  Dunno whether that is a good optimization to make or
not.

What counts is the user perspective: scrolling should show all
hits highlighted, no matter how far you scroll and no matter
which directions.  And to get that behavior a user should not
need to change option `lazy-highlight-buffer'.  That option
should have no effect on this behavior, and vice versa.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> What counts is the user perspective: scrolling should show all
> hits highlighted, no matter how far you scroll and no matter
> which directions.  And to get that behavior a user should not
> need to change option `lazy-highlight-buffer'.  That option
> should have no effect on this behavior, and vice versa.

The first part of your request is already implemented as well:
you can customize `search-exit-option' to the value `nil'
(its tag text is "Don't terminate incremental search"),
then it works exactly like you want.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > What counts is the user perspective: scrolling should show all
> > hits highlighted, no matter how far you scroll and no matter
> > which directions.  And to get that behavior a user should not
> > need to change option `lazy-highlight-buffer'.  That option
> > should have no effect on this behavior, and vice versa.
> 
> The first part of your request is already implemented as well:
> you can customize `search-exit-option' to the value `nil'
> (its tag text is "Don't terminate incremental search"),
> then it works exactly like you want.

Sorry, but I don't really understand.

There are several parts to this bug report.  I don't know
what "the first part" refers to.

I don't want to have to customize anything (other than a
new specific `isearch-allow-scroll' value) to work around
the problem.

In the Emacs release I have (26.1) there is no such value
for `search-exit-option'.  And it's not clear to me why
I would want to change the value of this option.  (I don't
even understand the (Emacs 26) doc string of the option:
"Non-nil means random control characters terminate
incremental search."  Random control characters?

I think I described the problem pretty clearly.  I don't
find your description of the solution (or solutions, to
parts apparently) clear.  What I mean is I don't understand
the proposed solution(s), to start with.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> Sorry, but I don't really understand.
>
> There are several parts to this bug report.  I don't know
> what "the first part" refers to.

It refers to your original request cited below:

> 1. Non-nil `isearch-allow-scroll' lets you use a scroll command
> (e.g. `C-v') without exiting Isearch.  Unfortunately, this is coupled
> with the hard-coded behavior that you cannot scroll far enough in either
> direction that point would be moved off screen.
>
> That restriction is general for Emacs, and it generally makes sense.
> It does not necessarily make sense during Isearch, however.  Why?
>
> Because the search position is one thing, and it is not lost.  What you
> might want to look at temporarily is another thing.
>
> It's a bit like using `C-SPC' in a buffer, scrolling up a couple of
> screenfuls to look at something, and then using `C-u C-SPC' to return.
> But in Isearch there is no need for `C-SPC' or `C-u C-SPC': the search
> position is recorded.  Search resumes from that same position, no
> matter how far away one might have scrolled.

This is implemented now with an option `nil' of the customizable
variable `search-exit-option'.  It lets you use a scroll command
(e.g. `C-v') without exiting Isearch, so you can scroll far enough
in either direction that point would be moved off screen.

> 2. What's more, the lazy highlighting of search hits is even more
> limited currently.  When you scroll to the current limit, there can be
> lots of search hits that are not highlighted.
>
> One of the reasons for scrolling is to see what search hits lie beyond
> the currently shown text.  This is already partly defeated, in that
> even when you scroll some of the hits are not highlighted.  This seems
> more like a bug, but if the enhancement of #1 is done then it should
> (it needs to) take care of #2 as well: Whatever text is shown should
> have all of its search hits highlighted with lazy highlighting.

This is implemented now with the customizable variable
`lazy-highlight-buffer'.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > Sorry, but I don't really understand.
> >
> > There are several parts to this bug report.  I don't know
> > what "the first part" refers to.
> 
> It refers to your original request cited below:

Thanks for clarifying what you meant.

But those points 1,2,... in the bug report are not
independent.  They are meant to make a logical argument
for what's being requested.  They are all of a piece.
They describe different, but related, aspects of the
inconvenience that the requested feature would
alleviate.

I don't want to customize one variable to be able to
scroll farther, and another variable to have what's
shown by that scrolling have lazy-highlighting
(especially if the latter requires lazy-highlighting
the entire buffer, rather than just what I see when
scrolling).

I want to be able to use `isearch-allow-scroll' to
let me scroll as far as I want, and see search hits
lazy-highlighted in what parts of the buffer I
scroll to.

> > 1. Non-nil `isearch-allow-scroll' lets you use a scroll command
> > (e.g. `C-v') without exiting Isearch.  Unfortunately, this is coupled
> > with the hard-coded behavior that you cannot scroll far enough in
> > either direction that point would be moved off screen.
> >
> > That restriction is general for Emacs, and it generally makes sense.
> > It does not necessarily make sense during Isearch, however.  Why?
> >
> > Because the search position is one thing, and it is not lost.  What
> > you might want to look at temporarily is another thing.
> >
> > It's a bit like using `C-SPC' in a buffer, scrolling up a couple of
> > screenfuls to look at something, and then using `C-u C-SPC' to
> > return.  But in Isearch there is no need for `C-SPC' or `C-u C-SPC':
> > the search position is recorded.  Search resumes from that same
> > position, no matter how far away one might have scrolled.
> 
> This is implemented now with an option `nil' of the customizable
> variable `search-exit-option'.  It lets you use a scroll command
> (e.g. `C-v') without exiting Isearch, so you can scroll far enough
> in either direction that point would be moved off screen.
> 
> > 2. What's more, the lazy highlighting of search hits is even more
> > limited currently.  When you scroll to the current limit, there can
> > be lots of search hits that are not highlighted.
> >
> > One of the reasons for scrolling is to see what search hits lie
> > beyond the currently shown text.  This is already partly defeated,
> > in that even when you scroll some of the hits are not highlighted.
> > This seems more like a bug, but if the enhancement of #1 is done
> > then it should (it needs to) take care of #2 as well: Whatever
> > text is shown should have all of its search hits highlighted with
> > lazy highlighting.
> 
> This is implemented now with the customizable variable
> `lazy-highlight-buffer'.

I do appreciate your letting me know what's newly
available; thanks.  And thanks for making those
improvements.  But no, I don't think we have what
I requested, and I don't think this bug (enhancement
request) should be closed.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> I don't want to customize one variable to be able to
> scroll farther, and another variable to have what's
> shown by that scrolling have lazy-highlighting
> (especially if the latter requires lazy-highlighting
> the entire buffer, rather than just what I see when
> scrolling).

Fine.  If you are lazy to customize two variables,
after you customize one variable we could automatically
change the value of the second variable.

> I want to be able to use `isearch-allow-scroll' to
> let me scroll as far as I want, and see search hits
> lazy-highlighted in what parts of the buffer I
> scroll to.

Fine, we could allow the same feature to be enabled
by two different variables.

diff --git a/lisp/isearch.el b/lisp/isearch.el
index eb0b25f9b1..ac809e4980 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2747,8 +2747,15 @@ isearch-allow-scroll
   "Whether scrolling is allowed during incremental search.
 If non-nil, scrolling commands can be used in Isearch mode.
 However, the current match will never scroll offscreen.
+If `unlimited', the current match can scroll offscreen.
+This has the same effect as the value `nil' of `search-exit-option'.
 If nil, scrolling commands will first cancel Isearch mode."
-  :type 'boolean
+  :type '(choice boolean
+                 (const :tag "Can scroll offscreen" unlimited))
+  :set (lambda (sym val)
+	 (set sym val)
+         (when (eq val 'unlimited)
+           (setq lazy-highlight-buffer t)))
   :group 'isearch)
 
 (defcustom isearch-allow-prefix t
@@ -2841,11 +2848,11 @@ isearch-pre-command-hook
      ((or (and isearch-allow-prefix
                (memq this-command '(universal-argument universal-argument-more
 				    digit-argument negative-argument)))
-	  (and isearch-allow-scroll
+	  (and isearch-allow-scroll (not (eq isearch-allow-scroll 'unlimited))
 	       (symbolp this-command)
 	       (or (eq (get this-command 'isearch-scroll) t)
 		   (eq (get this-command 'scroll-command) t))))
-      (when isearch-allow-scroll
+      (when (and isearch-allow-scroll (not (eq isearch-allow-scroll 'unlimited)))
 	(setq isearch-pre-scroll-point (point))))
      ;; A mouse click on the isearch message starts editing the search string.
      ((and (eq (car-safe main-event) 'down-mouse-1)
@@ -2853,6 +2860,7 @@ isearch-pre-command-hook
       ;; Swallow the up-event.
       (read-event)
       (setq this-command 'isearch-edit-string))
+     ((eq isearch-allow-scroll 'unlimited))
      ;; Don't terminate the search for motion commands.
      ((or (and (eq search-exit-option 'move)
                (symbolp this-command)

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > I don't want to customize one variable to be able to
> > scroll farther, and another variable to have what's
> > shown by that scrolling have lazy-highlighting
> > (especially if the latter requires lazy-highlighting
> > the entire buffer, rather than just what I see when
> > scrolling).
> 
> Fine.  If you are lazy to customize two variables,
> after you customize one variable we could automatically
> change the value of the second variable.

That was not my point.

First is a question: Would customizing those two
variables in that way affect ONLY the specific
behavior I requested?  I don't expect so.

But even if the answer to that question is yes,
it's not clear from the descriptions of those
variables that that behavior follows as the only,
specific behavior.

If you implement code that does what I requested,
providing a single place for users to control it
(which should logically be `isearch-allow-scroll',
I think) - and that controls only it (does not
affect some other behavior) that will be great.

(Maybe you've done that - if you say so then I'll
take a closer look.)

IOW, how that implementation gets done under the
covers is not so important to a user.  If it's
best that it somehow make use of existing code -
even existing user options, that's OK.

It's not clear to me that the purpose of those
two variables is to realize the feature requested
here.  Is it, really?

But users should preferably not need to worry
about variable interactions.  The doc for a given
variable should make clear just what it does, and
each variable should preferably have one behavior
(per value chosen).

You should not, e.g., discover that by choosing a
var value to make a background red you have also
inadvertently turned off the ability to have blue
foreground text.

> > I want to be able to use `isearch-allow-scroll' to
> > let me scroll as far as I want, and see search hits
> > lazy-highlighted in what parts of the buffer I
> > scroll to.
> 
> Fine, we could allow the same feature to be enabled
> by two different variables.
> 
> @@ -2747,8 +2747,15 @@ isearch-allow-scroll
>    "Whether scrolling is allowed during incremental search.
>  If non-nil, scrolling commands can be used in Isearch mode.
>  However, the current match will never scroll offscreen.
> +If `unlimited', the current match can scroll offscreen.

Those last two sentences should be rephrased in
the usual manner: `unlimited' means... Any other
non-nil value means... 

> +This has the same effect as the value `nil'
> of `search-exit-option'.

Does it really?  If that's the only effect of
`search-exit-option' then yes, we don't need a
separate option.  But if that were true then
why did `search-exit-option' exist previously,
and why didn't it do this previously?  And why
was it called `search...' and not `isearch...'?

I'm guessing that nil `search-exit-option' does
not just have "the same effect".  But (see above)
even if it does, that doesn't mean that option
`search-exit-option' has the same effect, because
setting it to non-nil, ONLY to NOT have the
effect of `unlimited' `isearch-allow-scroll',
would presumably also have some other effect
unrelated to allowing scrolling.

Sorry that I'm arguing from ignorance here so
far, and not bothering to get informed in detail
about these existing options.

I'm essentially guessing that trying to repurpose
some combination of them to achieve this request
is not a great idea.  If I'm wrong, please try to
present the counter argument straightforwardly.
I'm open to being convinced, but a first sense is
that this is likely not the right approach.

If you just tell me I'm wrong then I'll take a
closer look at what you're proposing.  But first
please consider what I say here, in case it's
valid.

>  If nil, scrolling commands will first cancel
>  Isearch mode."

BTW, why do we say "first" there?  First before
what?  There is no other action after this, is
there?  I.e., there is no "second" or "and" that
follows this "first", right?  Or is there some
additional context that makes that "first" mean
something?

Thanks for working on this.  Let me know, if you
think I'm wrong and should just take a closer
look at what you've proposed.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

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

> But users should preferably not need to worry
> about variable interactions.  The doc for a given
> variable should make clear just what it does, and
> each variable should preferably have one behavior
> (per value chosen).

I agree that it's better to make it clear in the docstring.
Fixed in a new patch below.

> I'm guessing that nil `search-exit-option' does
> not just have "the same effect".  But (see above)
> even if it does, that doesn't mean that option
> `search-exit-option' has the same effect, because
> setting it to non-nil, ONLY to NOT have the
> effect of `unlimited' `isearch-allow-scroll',
> would presumably also have some other effect
> unrelated to allowing scrolling.

I agree that `search-exit-option' is too confusing variable
for such features.  So we have to offload it from all
unrelated features.

As the first step, I moved the recently added shift-select
feature from `search-exit-option' to its own clearly named
customizable variable `isearch-allow-shift-select'.

For the same reason, unlimited scrolling was moved to the new option
`unlimited' of `isearch-allow-scroll'.

Now finally everything looks right.  Please try a new patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-allow-scroll-offscreen.2.patch --]
[-- Type: text/x-diff, Size: 5033 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index eb0b25f9b1..8c54ab92db 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -72,21 +72,11 @@ search-exit-option
 If t, random control and meta characters terminate the search
 and are then executed normally.
 If `edit', edit the search string instead of exiting.
-If `move', extend the search string by motion commands
-that have the `isearch-move' property on their symbols
-equal to `enabled', or the shift-translated command is
-not disabled by the value `disabled' of the same property.
-If `shift-move', extend the search string by motion commands
-while holding down the shift key.
-Both `move' and `shift-move' extend the search string by yanking text
-that ends at the new position after moving point in the current buffer.
 If `append', the characters which you type that are not interpreted by
 the incremental search are simply appended to the search string.
 If nil, run the command without exiting Isearch."
   :type '(choice (const :tag "Terminate incremental search" t)
                  (const :tag "Edit the search string" edit)
-                 (const :tag "Extend the search string by motion commands" move)
-                 (const :tag "Extend the search string by shifted motion keys" shift-move)
                  (const :tag "Append control characters to the search string" append)
                  (const :tag "Don't terminate incremental search" nil))
   :version "27.1")
@@ -2747,8 +2737,12 @@ isearch-allow-scroll
   "Whether scrolling is allowed during incremental search.
 If non-nil, scrolling commands can be used in Isearch mode.
 However, the current match will never scroll offscreen.
+If `unlimited', the current match can scroll offscreen.
+You may want to enable `lazy-highlight-buffer' as well.
 If nil, scrolling commands will first cancel Isearch mode."
-  :type 'boolean
+  :type '(choice (const :tag "Disable scrolling" nil)
+                 (const :tag "Allow scrolling within screen" t)
+                 (const :tag "Allow scrolling offscreen" unlimited))
   :group 'isearch)
 
 (defcustom isearch-allow-prefix t
@@ -2812,6 +2806,21 @@ isearch-back-into-window
 (defvar isearch-pre-scroll-point nil)
 (defvar isearch-pre-move-point nil)
 
+(defcustom isearch-allow-shift-select nil
+  "Whether motion is allowed to select text during incremental search.
+If t, extend the search string by motion commands while holding down
+the shift key.  The search string is extended by yanking text that
+ends at the new position after moving point in the current buffer.
+If `move', extend the search string without the shift key pressed
+by motion commands that have the `isearch-move' property on their
+symbols equal to `enabled', or the shift-translated command is
+not disabled by the value `disabled' of the same property."
+  :type '(choice (const :tag "Disable shift selection" nil)
+                 (const :tag "Shifted motion keys extend the search string" t)
+                 (const :tag "Motion keys extend the search string" move))
+  :group 'isearch
+  :version "27.1")
+
 (defun isearch-pre-command-hook ()
   "Decide whether to exit Isearch mode before executing the command.
 Don't exit Isearch if the key sequence that invoked this command
@@ -2845,7 +2854,7 @@ isearch-pre-command-hook
 	       (symbolp this-command)
 	       (or (eq (get this-command 'isearch-scroll) t)
 		   (eq (get this-command 'scroll-command) t))))
-      (when isearch-allow-scroll
+      (when (and isearch-allow-scroll (not (eq isearch-allow-scroll 'unlimited)))
 	(setq isearch-pre-scroll-point (point))))
      ;; A mouse click on the isearch message starts editing the search string.
      ((and (eq (car-safe main-event) 'down-mouse-1)
@@ -2854,13 +2863,13 @@ isearch-pre-command-hook
       (read-event)
       (setq this-command 'isearch-edit-string))
      ;; Don't terminate the search for motion commands.
-     ((or (and (eq search-exit-option 'move)
+     ((or (and (eq isearch-allow-shift-select 'move)
                (symbolp this-command)
                (or (eq (get this-command 'isearch-move) 'enabled)
                    (and (not (eq (get this-command 'isearch-move) 'disabled))
                         (stringp (nth 1 (interactive-form this-command)))
                         (string-match-p "^^" (nth 1 (interactive-form this-command))))))
-          (and (eq search-exit-option 'shift-move)
+          (and isearch-allow-shift-select (not (eq isearch-allow-shift-select 'move))
                this-command-keys-shift-translated))
       (setq this-command-keys-shift-translated nil)
       (setq isearch-pre-move-point (point)))
@@ -2883,7 +2892,7 @@ isearch-post-command-hook
 	(goto-char isearch-pre-scroll-point)))
     (setq isearch-pre-scroll-point nil)
     (isearch-update))
-   ((memq search-exit-option '(move shift-move))
+   (isearch-allow-shift-select
     (when (and isearch-pre-move-point
                (not (eq isearch-pre-move-point (point))))
       (let ((string (buffer-substring-no-properties

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > But users should preferably not need to worry
> > about variable interactions.  The doc for a given
> > variable should make clear just what it does, and
> > each variable should preferably have one behavior
> > (per value chosen).
> 
> I agree that it's better to make it clear in the docstring.
> Fixed in a new patch below.
> 
> > I'm guessing that nil `search-exit-option' does
> > not just have "the same effect".  But (see above)
> > even if it does, that doesn't mean that option
> > `search-exit-option' has the same effect, because
> > setting it to non-nil, ONLY to NOT have the
> > effect of `unlimited' `isearch-allow-scroll',
> > would presumably also have some other effect
> > unrelated to allowing scrolling.
> 
> I agree that `search-exit-option' is too confusing variable
> for such features.  So we have to offload it from all
> unrelated features.
> 
> As the first step, I moved the recently added shift-select
> feature from `search-exit-option' to its own clearly named
> customizable variable `isearch-allow-shift-select'.
> 
> For the same reason, unlimited scrolling was moved to the new option
> `unlimited' of `isearch-allow-scroll'.
> 
> Now finally everything looks right.  Please try a new patch:

I agree with your summary here, that is, I think we're
in agreement.

I didn't try to apply your patch, but I took a quick
look at it.  Here are some questions/comments, in
case they help.

1. I'm still puzzled about this:

  However, the current match will never scroll offscreen.
  If `unlimited', the current match can scroll offscreen.

Those two sentences don't make sense together.
If the current match never scrolls offscreen then
it can't be true that the current match can scroll
offscreen.  Something different needs to be said
here.

2. And the term "offscreen" doesn't seem like a
good choice.  Don't we mean just off the window,
i.e., out of view?  So this too bothers/confuses me:

  "Allow scrolling within screen"

I don't know what is meant by "screen" here.
What are the limits of the "screen" within
which I can scroll with this option value?

3. I'm also puzzled by this:

  You may want to enable `lazy-highlight-buffer'
  as well.

Why say that?  I think it confuses people, by
suggesting that for some reason you might need
to do that, in order to see such highlighting
if you scroll (e.g. "offscreen").

That's not the case, is it?  (I hope not.)  I
thought that the implementation of `unlimited'
automatically lazy-highlights everything that
you scroll to.  Is that not the case?

That's part of this enhancement request: scroll
as far as you want, and have search hits
highlighted no matter how far you scroll.

If that highlighting-as-needed is implemented then
I see no need to mention `lazy-highlight-buffer',
and I think it's wrong to do so.

There should be no more need to enable full-buffer
highlighting in the `unlimited' scroll case than
in any other case.  You enable `lazy-highlight-buffer'
when you want to ensure that all search hits are
highlighted, even ones you may never look at.

If `unlimited' does NOT highlight all hits that
come in view then this enhancement request is
still not realized, as that's a necessary part
of it.

4. I think it would be better for the :tag
"Disable scrolling" to say something like this:
"Scrolling exits Isearch".

It's not that you cannot scroll.  It's just that
if you do scroll you stop searching.  You could
say "Disable scrolling while searching", if you
prefer to keep the parallel structure of the :tags.

5. Doc string of `*-allow-shift-selection':

  Whether motion is allowed to select text during
  incremental search.

That will possibly make users think that this is
about activating the region (selecting text).

The first doc-string line should kind of stand on
its own, to give an overall idea.  I'd say
something more like this:

  Motion keys yank text to the search string.

6. `*-allow-shift-selection' behavior:

Why is the value `move'?  Is that the best word?

What happens with `move' if you use a shifted
motion key?  If it acts the same as `t' then what
you call "shifted" is really "-only_ when shifted".
What you call just "motion" seems like just both
shifted and unshifted.

Why do we even have two such choices?  Why would
someone who wants to yank chars by moving over
them want to have to use Shift, ever?  Is it so
that they can use unshifted to exit Isearch and
move the cursor?  I guess so.  Maybe that could
be made clearer (dunno).

7. OK, no, `move' is apparently more complicated
than that (all the more reason why it shouldn't
be called `move'.)  This text is a mouthful:

  by motion commands that have the `isearch-move'
  property on their symbols equal to `enabled',
  or [for which] the shift-translated command is
  not disabled by the value `disabled' of the same
  property.

That sounds a bit like a legal text. You can just
repeat "property `isearch-move'" instead of saying
"the same property" - that would be clearer.  But
the sentence probably needs to be split.  And it
may be a sign that the behavior is too complicated.

Why do we have this complicated behavior?  Why
not just have a `move' value that lets you yank
text without having to use Shift (i.e. using Shift
or not)?  What's the point of having to put such a
property on some command symbols?

And if there really is a use case for doing that
to certain commands, so that _only they_ manifest
a difference between `t' and `move' behavior,
then why not have only the `move' behavior (no `t'
behavior)?

I really don't understand the motivations here,
sorry.  What problem is this option trying to solve?

8.This option should not be called `*-shift-selection'
if it is not necessarily about Shift selection.
The option is apparently about yanking text you
move the cursor over.

9. Again, I'm not crazy about this :tag, for the
same reason as above:

  Disable shift selection

A nil option value doesn't disable anything.  It
just means that cursor motion ends Isearch, so you
just move over the buffer text.

I apologize in advance for not following some
of this.  Probably at least some of what I wrote
is nonsense based on misunderstanding.

At the very least, though, perhaps some of that
misunderstanding might indicate that some of the
descriptions, or some of the behaviors, are more
complicated than necessary.

Hope this helps, and thanks for all your work on
this stuff.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

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

> 1. I'm still puzzled about this:
>
>   However, the current match will never scroll offscreen.
>   If `unlimited', the current match can scroll offscreen.
>
> Those two sentences don't make sense together.
> If the current match never scrolls offscreen then
> it can't be true that the current match can scroll
> offscreen.  Something different needs to be said
> here.

Do you think this is better?

  "If non-nil, scrolling commands can be used in Isearch mode.
  However, the current match can't scroll offscreen if the value is t.
  But if it's `unlimited', the current match can scroll offscreen.
  You may want to enable `lazy-highlight-buffer' in this case.
  If nil, scrolling commands will cancel Isearch mode."

If you don't agree, please suggest a better wording.

> 2. And the term "offscreen" doesn't seem like a
> good choice.  Don't we mean just off the window,
> i.e., out of view?  So this too bothers/confuses me:
>
>   "Allow scrolling within screen"
>
> I don't know what is meant by "screen" here.
> What are the limits of the "screen" within
> which I can scroll with this option value?

I guess a screen is part of the buffer visible in the window.
We use the word "screen" in other docstrings as well.

> 3. I'm also puzzled by this:
>
>   You may want to enable `lazy-highlight-buffer'
>   as well.
>
> Why say that?  I think it confuses people, by
> suggesting that for some reason you might need
> to do that, in order to see such highlighting
> if you scroll (e.g. "offscreen").
>
> That's not the case, is it?  (I hope not.)  I
> thought that the implementation of `unlimited'
> automatically lazy-highlights everything that
> you scroll to.  Is that not the case?

No, this is not the case.  This is very difficult
to implement, and not worth the effort because this feature
is already available by customizing lazy-highlight-buffer.

Moreover, even if implemented, it still makes no sense
because it can't highlight as quick as you scroll
thru the buffer.

The problem is that you haven't tried my patch
with lazy-highlight-buffer enabled.  If you tried
you wouldn't want any other implementation.

> 4. I think it would be better for the :tag
> "Disable scrolling" to say something like this:
> "Scrolling exits Isearch".

Thanks, fixed.

> 5. Doc string of `*-allow-shift-selection':
>
>   Whether motion is allowed to select text during
>   incremental search.
>
> That will possibly make users think that this is
> about activating the region (selecting text).
>
> The first doc-string line should kind of stand on
> its own, to give an overall idea.  I'd say
> something more like this:
>
>   Motion keys yank text to the search string.

Thanks, fixed.

> 6. `*-allow-shift-selection' behavior:
>
> Why is the value `move'?  Is that the best word?
>
> What happens with `move' if you use a shifted
> motion key?  If it acts the same as `t' then what
> you call "shifted" is really "-only_ when shifted".
> What you call just "motion" seems like just both
> shifted and unshifted.
>
> Why do we even have two such choices?  Why would
> someone who wants to yank chars by moving over
> them want to have to use Shift, ever?  Is it so
> that they can use unshifted to exit Isearch and
> move the cursor?  I guess so.  Maybe that could
> be made clearer (dunno).
>
> 7. OK, no, `move' is apparently more complicated
> than that (all the more reason why it shouldn't
> be called `move'.)  This text is a mouthful:
>
>   by motion commands that have the `isearch-move'
>   property on their symbols equal to `enabled',
>   or [for which] the shift-translated command is
>   not disabled by the value `disabled' of the same
>   property.
>
> That sounds a bit like a legal text. You can just
> repeat "property `isearch-move'" instead of saying
> "the same property" - that would be clearer.  But
> the sentence probably needs to be split.  And it
> may be a sign that the behavior is too complicated.
>
> Why do we have this complicated behavior?  Why
> not just have a `move' value that lets you yank
> text without having to use Shift (i.e. using Shift
> or not)?  What's the point of having to put such a
> property on some command symbols?
>
> And if there really is a use case for doing that
> to certain commands, so that _only they_ manifest
> a difference between `t' and `move' behavior,
> then why not have only the `move' behavior (no `t'
> behavior)?

Sorry, I don't understand what do you suggest here.

> 8.This option should not be called `*-shift-selection'
> if it is not necessarily about Shift selection.
> The option is apparently about yanking text you
> move the cursor over.

So I renamed it to `isearch-allow-yank-on-move',
and its options to `no-shift' and `shift'.

> 9. Again, I'm not crazy about this :tag, for the
> same reason as above:
>
>   Disable shift selection
>
> A nil option value doesn't disable anything.  It
> just means that cursor motion ends Isearch, so you
> just move over the buffer text.

Thanks for the suggestion, I changed it to
"Motion keys exits Isearch".


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-allow-scroll-offscreen.3.patch --]
[-- Type: text/x-diff, Size: 5185 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index eb0b25f9b1..b0284a5972 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -72,21 +72,11 @@ search-exit-option
 If t, random control and meta characters terminate the search
 and are then executed normally.
 If `edit', edit the search string instead of exiting.
-If `move', extend the search string by motion commands
-that have the `isearch-move' property on their symbols
-equal to `enabled', or the shift-translated command is
-not disabled by the value `disabled' of the same property.
-If `shift-move', extend the search string by motion commands
-while holding down the shift key.
-Both `move' and `shift-move' extend the search string by yanking text
-that ends at the new position after moving point in the current buffer.
 If `append', the characters which you type that are not interpreted by
 the incremental search are simply appended to the search string.
 If nil, run the command without exiting Isearch."
   :type '(choice (const :tag "Terminate incremental search" t)
                  (const :tag "Edit the search string" edit)
-                 (const :tag "Extend the search string by motion commands" move)
-                 (const :tag "Extend the search string by shifted motion keys" shift-move)
                  (const :tag "Append control characters to the search string" append)
                  (const :tag "Don't terminate incremental search" nil))
   :version "27.1")
@@ -2746,9 +2736,13 @@ isearch-fallback
 (defcustom isearch-allow-scroll nil
   "Whether scrolling is allowed during incremental search.
 If non-nil, scrolling commands can be used in Isearch mode.
-However, the current match will never scroll offscreen.
-If nil, scrolling commands will first cancel Isearch mode."
-  :type 'boolean
+However, the current match can't scroll offscreen if the value is t.
+But if it's `unlimited', the current match can scroll offscreen.
+You may want to enable `lazy-highlight-buffer' in this case.
+If nil, scrolling commands will cancel Isearch mode."
+  :type '(choice (const :tag "Scrolling exits Isearch" nil)
+                 (const :tag "Allow scrolling within screen" t)
+                 (const :tag "Allow scrolling offscreen" unlimited))
   :group 'isearch)
 
 (defcustom isearch-allow-prefix t
@@ -2812,6 +2806,21 @@ isearch-back-into-window
 (defvar isearch-pre-scroll-point nil)
 (defvar isearch-pre-move-point nil)
 
+(defcustom isearch-allow-yank-on-move nil
+  "Motion keys yank text to the search string.
+If t, extend the search string by motion commands while holding down
+the shift key.  The search string is extended by yanking text that
+ends at the new position after moving point in the current buffer.
+If `move', extend the search string without the shift key pressed
+by motion commands that have the `isearch-move' property on their
+symbols equal to `enabled', or for which the shift-translated command
+is not disabled by the value `disabled' of property `isearch-move'."
+  :type '(choice (const :tag "Motion keys exits Isearch" nil)
+                 (const :tag "Motion keys extend the search string" no-shift)
+                 (const :tag "Shifted motion keys extend the search string" shift))
+  :group 'isearch
+  :version "27.1")
+
 (defun isearch-pre-command-hook ()
   "Decide whether to exit Isearch mode before executing the command.
 Don't exit Isearch if the key sequence that invoked this command
@@ -2845,7 +2854,7 @@ isearch-pre-command-hook
 	       (symbolp this-command)
 	       (or (eq (get this-command 'isearch-scroll) t)
 		   (eq (get this-command 'scroll-command) t))))
-      (when isearch-allow-scroll
+      (when (and isearch-allow-scroll (not (eq isearch-allow-scroll 'unlimited)))
 	(setq isearch-pre-scroll-point (point))))
      ;; A mouse click on the isearch message starts editing the search string.
      ((and (eq (car-safe main-event) 'down-mouse-1)
@@ -2854,13 +2863,13 @@ isearch-pre-command-hook
       (read-event)
       (setq this-command 'isearch-edit-string))
      ;; Don't terminate the search for motion commands.
-     ((or (and (eq search-exit-option 'move)
+     ((or (and (eq isearch-allow-yank-on-move 'no-shift)
                (symbolp this-command)
                (or (eq (get this-command 'isearch-move) 'enabled)
                    (and (not (eq (get this-command 'isearch-move) 'disabled))
                         (stringp (nth 1 (interactive-form this-command)))
                         (string-match-p "^^" (nth 1 (interactive-form this-command))))))
-          (and (eq search-exit-option 'shift-move)
+          (and (eq isearch-allow-yank-on-move 'shift)
                this-command-keys-shift-translated))
       (setq this-command-keys-shift-translated nil)
       (setq isearch-pre-move-point (point)))
@@ -2883,7 +2892,7 @@ isearch-post-command-hook
 	(goto-char isearch-pre-scroll-point)))
     (setq isearch-pre-scroll-point nil)
     (isearch-update))
-   ((memq search-exit-option '(move shift-move))
+   (isearch-allow-yank-on-move
     (when (and isearch-pre-move-point
                (not (eq isearch-pre-move-point (point))))
       (let ((string (buffer-substring-no-properties

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > 1. I'm still puzzled about this:
> >
> >   However, the current match will never scroll offscreen.
> >   If `unlimited', the current match can scroll offscreen.
> >
> > Those two sentences don't make sense together.
> > If the current match never scrolls offscreen then
> > it can't be true that the current match can scroll
> > offscreen.  Something different needs to be said
> > here.
> 
> Do you think this is better?
> 
>   "If non-nil, scrolling commands can be used in Isearch mode.
>   However, the current match can't scroll offscreen if the value is t.
>   But if it's `unlimited', the current match can scroll offscreen.
>   You may want to enable `lazy-highlight-buffer' in this case.
>   If nil, scrolling commands will cancel Isearch mode."
>
> If you don't agree, please suggest a better wording.

I prefer the standard approach: say first what the default
(nil) does.  Then say what non-nil does.  Then state that
specific non-nil values have specific non-nil behaviors -
IOW, they all do what the non-nil description says, but
with some differences.  For example:

 By default (nil value) scrolling exits Isearch mode.
 Non-nil means you can scroll while searching, as follows:

 `unlimited'          You can scroll any distance.
 other non-nil value  You cannot scroll far enough that the
                      current search hit is no longer visible
                      (is off screen).

(BTW, is it only t or is it any other non-nil value?)

> > 2. And the term "offscreen" doesn't seem like a
> > good choice.  Don't we mean just off the window,
> > i.e., out of view?  So this too bothers/confuses me:
> >
> >   "Allow scrolling within screen"
> >
> > I don't know what is meant by "screen" here.
> > What are the limits of the "screen" within
> > which I can scroll with this option value?
> 
> I guess a screen is part of the buffer visible in the window.
> We use the word "screen" in other docstrings as well.

Yes, but here I think we need to be clear that the
part of the buffer visible in the window is any
part that _includes the current search hit_, no?

Isn't that what this is about - scrolling so far
that that hit is "off screen" means scrolling so
far that it is no longer visible in the window.
The "screenful" that defines the scrolling limits
is not something defined independently of the
position of the current search hit.

> > 3. I'm also puzzled by this:
> >
> >   You may want to enable `lazy-highlight-buffer'
> >   as well.
> >
> > Why say that?  I think it confuses people, by
> > suggesting that for some reason you might need
> > to do that, in order to see such highlighting
> > if you scroll (e.g. "offscreen").
> >
> > That's not the case, is it?  (I hope not.)  I
> > thought that the implementation of `unlimited'
> > automatically lazy-highlights everything that
> > you scroll to.  Is that not the case?
> 
> No, this is not the case.  This is very difficult
> to implement, and not worth the effort because this feature
> is already available by customizing lazy-highlight-buffer.

I see.  That disappoints me.  We have lazy
highlighting but we can't use it lazily, except for
the original design of being limited to a windowful?

Can't we "just" let scrolling move the "window limits"
as you scroll?  (Yes, I'm making this up, without
looking at the code.)

This highlight-as-you-go should really be considered
part of this enhancement request, IMO.  And I mentioned
it as such.  But if you don't want to tackle that part
initially, or if indeed it ends up being too complex in
the end, then so be it, for now.

> Moreover, even if implemented, it still makes no sense
> because it can't highlight as quick as you scroll
> thru the buffer.

I don't think I agree that it "makes no sense".

1. You might well scroll slowly sometimes, looking
   closely at search hits.  Being able to see hits
   highlighted is always better than not.

2. If lazy highlighting can generally keep up with
   holding down `C-s', which it does pretty well now,
   then it should be able to do about the same wrt
   scrolling.  Sure, there can be many search hits
   within a small area of text, but even so, I don't
   see this as a problem.  And just as we (I guess)
   do for fast highlighting when you hold `C-s' down,
   we can presumably skip highlighting any hits that
   are no longer on the screen because you've moved
   past them.

3. Even if highlighting were sometimes slower than
   what you might scroll, it would catch up when you
   stop.  Heck, if the alternative is full-buffer
   highlighting, that has to be even slower, no?

> The problem is that you haven't tried my patch
> with lazy-highlight-buffer enabled.  If you tried
> you wouldn't want any other implementation.

OK, I'll take your word for it, I guess.  But I don't
see why highlighting only a portion of the buffer
(and it might often be a tiny portion) should be
less performant than highlighting the entire buffer.
Is that really the case?

> > 6. `*-allow-shift-selection' behavior:
> >
> > Why is the value `move'?  Is that the best word?
> >
> > What happens with `move' if you use a shifted
> > motion key?  If it acts the same as `t' then what
> > you call "shifted" is really "-only_ when shifted".
> > What you call just "motion" seems like just both
> > shifted and unshifted.
> >
> > Why do we even have two such choices?  Why would
> > someone who wants to yank chars by moving over
> > them want to have to use Shift, ever?  Is it so
> > that they can use unshifted to exit Isearch and
> > move the cursor?  I guess so.  Maybe that could
> > be made clearer (dunno).
> >
> > 7. OK, no, `move' is apparently more complicated
> > than that (all the more reason why it shouldn't
> > be called `move'.)  This text is a mouthful:
> >
> >   by motion commands that have the `isearch-move'
> >   property on their symbols equal to `enabled',
> >   or [for which] the shift-translated command is
> >   not disabled by the value `disabled' of the same
> >   property.
> >
> > That sounds a bit like a legal text. You can just
> > repeat "property `isearch-move'" instead of saying
> > "the same property" - that would be clearer.  But
> > the sentence probably needs to be split.  And it
> > may be a sign that the behavior is too complicated.

Is that part of what I wrote not clear, at least?
I do advise splitting that sentence up, at a minimum.

> > Why do we have this complicated behavior?  Why
> > not just have a `move' value that lets you yank
> > text without having to use Shift (i.e. using Shift
> > or not)?  What's the point of having to put such a
> > property on some command symbols?
> >
> > And if there really is a use case for doing that
> > to certain commands, so that _only they_ manifest
> > a difference between `t' and `move' behavior,
> > then why not have only the `move' behavior (no `t'
> > behavior)?
> 
> Sorry, I don't understand what do you suggest here.

It's not clear to me what the behavior is or what
problem it's trying to solve.

You want to be able to yank text, that you move
the cursor over in the buffer, onto the search
string.

Case `t':

Some people want to do that using Shift selection.
Why shift?  Why wouldn't they just want to do it
by moving the cursor (without bothering to shift)?

Is it because they want to be able to, separately,
move the unshifted cursor to exit Isearch and move
over buffer text?  Is that the idea?  I guess so.

Case `move':

And for those people who don't want to give up
having cursor movement exit Isearch, and who don't
want to have to use Shift, we offer another way to
yank to the search string by moving the cursor:
Users or libraries can specify, for certain cursor
movement commands, that mere unshifted cursor
movement will yank.  Is that right?

This all sounds complicated, but if there really
are such use cases (e.g., different users) then OK.

For case `move', why do we have two different ways
to specify the motion commands that we want to let
yank text?

1. commands that have the `isearch-move' property on
   their symbols equal to `enabled'

2. shift-translated commands that have the `isearch-move'
   property on their symbols not equal to `disabled'

I don't even follow that, especially #2.  I guess
there are at least 4 possibilities for property
`isearch-move'?

* `enabled'
* not `disabled' (on symbol of shift-translated command)
* nil
* something else

What other values for symbol of shift-translated
command, besides `disabled', and what do they do?

Why two ways to specify the commands that we let
yank by moving the cursor?

And what about the use case I mentioned: just be
able to use any motion commands to yank text, without
using Shift?  Why assume that everyone wants to be
able to use at least some cursor-motion commands to
exit Isearch?  This case is like implicitly putting
the required `isearch-move' value on all commands.

I'm not really arguing for any given behavior.
I'm saying I don't really understand where all of
this is coming from.  Have different users actually
requested something for these different possible
use cases?  Is some other library involved perhaps?
What's behind this?  It seems like a lot, just to
let you yank buffer text by moving the cursor.

FWIW, I can give an example of something a bit similar
that I have in my code.  (I don't know how useful it is.)
I mention it because your use of a property value on a
command made me think of it.  I can see the value of
letting only some motion commands (chosen by a user) do
something other than exit Isearch.

It's option `isearchp-initiate-edit-commands'.  Default:
(backward-char left-char backward-sexp backward-word left-word)

  Commands whose key bindings initiate Isearch edit.
  When invoked by a key sequence, Isearch edits the
  search string, applying the command to it immediately.

  Commands you might want to include here are typically
  commands that move point to the left, possibly deleting
  text along the way.

  Set this to `nil' if you always want all such commands
  to exit Isearch and act on the buffer text.

IOW, `C-b' (by default), does not exit Isearch but
instead, it's short for `M-e C-b'.  The option is
intended only for backward motion commands.  But the
idea seems similar to what you have for value `move':
limit the behavior to particular commands.  You do it
with a symbol property; I did it with a list-valued
option.

(BTW, with `*-allow-shift-selection', if a user moves
the cursor backward do you yank those chars onto the
search string?  I guess so.  In which order: order
of traversal or order of appearance in the buffer?)

My questions about `*-allow-shift-selection' are
first about the use cases, only secondarily about the
doc.  Both seem complicated to me, and I particularly
wonder why the use cases.

But I don't really need to understand.  I reviewed
what you wrote about that only because you included
it in a reply to this (other) bug report.  Ignore my
feedback about `*-allow-shift-selection' if you like
- no problem.

> > The option is apparently about yanking text you
> > move the cursor over.
> 
> So I renamed it to `isearch-allow-yank-on-move',
> and its options to `no-shift' and `shift'.

I would remove the "allow".  The option is named
after the non-nil behavior (as is common), which
is to yank whatever you move the cursor over.

> Thanks for the suggestion, I changed it to
> "Motion keys exits Isearch".

exits -> exit

Thx - Drew

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Eli Zaretskii]

> Date: Thu, 29 Nov 2018 16:27:13 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 15839@debbugs.gnu.org
> 
> >   "If non-nil, scrolling commands can be used in Isearch mode.
> >   However, the current match can't scroll offscreen if the value is t.
> >   But if it's `unlimited', the current match can scroll offscreen.
> >   You may want to enable `lazy-highlight-buffer' in this case.
> >   If nil, scrolling commands will cancel Isearch mode."
> >
> > If you don't agree, please suggest a better wording.
> 
> I prefer the standard approach: say first what the default
> (nil) does.  Then say what non-nil does.

I disagree that this should be a guideline.  The following two
variants are equivalently good documentation, IMO:

Variant 1:

  "If non-nil, scrolling commands can be used in Isearch mode.
  If nil, the default, scrolling commands will cancel Isearch mode.

  If the value is t, the current match cannot be scrolled off-screen,
  but that limitation is removed if the value is `unlimited'.
  You may want to enable `lazy-highlight-buffer' in this case."

Variant 2:

  "If nil, the default, scrolling commands will cancel Isearch mode.
  If non-nil, scrolling commands can be used in Isearch mode.
  
  If the value is t, the current match cannot be scrolled off-screen,
  but that limitation is removed if the value is `unlimited'.
  You may want to enable `lazy-highlight-buffer' in this case."

And I think I slightly prefer the first one.  Note that I rephrased
the last part.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > >   "If non-nil, scrolling commands can be used in Isearch mode.
> > >   However, the current match can't scroll offscreen if the value is
> > >   t.
> > >   But if it's `unlimited', the current match can scroll offscreen.
> > >   You may want to enable `lazy-highlight-buffer' in this case.
> > >   If nil, scrolling commands will cancel Isearch mode."
> > >
> > > If you don't agree, please suggest a better wording.
> >
> > I prefer the standard approach: say first what the default
> > (nil) does.  Then say what non-nil does.
> 
> I disagree that this should be a guideline.

I should have said "a more typical approach".

> The following two variants are equivalently good documentation, IMO:
> 
> Variant 1:
> 
>   "If non-nil, scrolling commands can be used in Isearch mode.
>   If nil, the default, scrolling commands will cancel Isearch mode.
> 
>   If the value is t, the current match cannot be scrolled off-screen,
>   but that limitation is removed if the value is `unlimited'.
>   You may want to enable `lazy-highlight-buffer' in this case."
> 
> Variant 2:
> 
>   "If nil, the default, scrolling commands will cancel Isearch mode.
>   If non-nil, scrolling commands can be used in Isearch mode.
> 
>   If the value is t, the current match cannot be scrolled off-screen,
>   but that limitation is removed if the value is `unlimited'.
>   You may want to enable `lazy-highlight-buffer' in this case."
> 
> And I think I slightly prefer the first one.  Note that I rephrased
> the last part.

Yes, both of those variants are also clear.

In any case, the fact that nil is the default should
be stated clearly, and the nil and non-nil cases should
 be stated up front.  That different non-nil cases exist
comes afterward, so it is clear that they have a common
non-nil behavior.

I think it's can sometimes be clearer to put all of the
description(s) of non-nil behavior together, but it's
not a requirement for a clear description.

Describing the general nil vs non-nil together is
usually helpful, I think, before mentioning any
particular non-nil behavior (unless it is the default).

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> But I don't really need to understand.  I reviewed
> what you wrote about that only because you included
> it in a reply to this (other) bug report.  Ignore my
> feedback about `*-allow-shift-selection' if you like
> - no problem.

Thank you for your feedback, all your remarks were taken into consideration,
committed to master, and this enhancement request closed.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > But I don't really need to understand.  I reviewed
> > what you wrote about that only because you included
> > it in a reply to this (other) bug report.  Ignore my
> > feedback about `*-allow-shift-selection' if you like
> > - no problem.
> 
> Thank you for your feedback, all your remarks were
> taken into consideration, committed to master, and
> this enhancement request closed.

I'm amazed by this, Juri.  You introduced stuff
into this bug thread that is about another bug
entirely (`*-allow-shift-selection').

I replied to your feedback about this bug, point by
point.  And, hoping to help, I also provided some
feedback about that other, unrelated bug.

What you quote above is about that _other_ bug, not
this one.  I said that you can, if you like, ignore
my feedback about `*-allow-shift-selection'.  That
bug is not something I'm really concerned about -
my feedback about it was just trying to be helpful.

As for this bug, I made clear that all that it
calls for does not yet seem to be provided by
your proposed changes.

Yet you reply (only) to my comment that you can
ignore my feedback about `*-allow-shift-selection',
to say that you have therefore closed _this_ bug.

I'm amazed - unless you did this accidentally,
confusing the two bugs and meaning to close the
other bug instead.

If you really intended to close this bug, please
say how "all [my] remarks were taken into
consideration" and how this enhancement request
has been entirely fulfilled.  That's not clear
to me.

From my point of view, it seems like you've made
changes to fix other bugs that incidentally make
progress toward also realizing the enhancement
requested by this bug.  That's all good.

But I don't see that this enhancement has been
realized.  I addressed this in specific ways,
none of which you've responded to in this
closure message.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> -----Original Message-----
> From: Drew Adams
> Sent: Tuesday, December 4, 2018 6:46 AM
> To: Juri Linkov <juri@linkov.net>
> Cc: 15839-done@debbugs.gnu.org
> Subject: bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll
> point off screen temporarily
> 
> > > But I don't really need to understand.  I reviewed
> > > what you wrote about that only because you included
> > > it in a reply to this (other) bug report.  Ignore my
> > > feedback about `*-allow-shift-selection' if you like
> > > - no problem.
> >
> > Thank you for your feedback, all your remarks were
> > taken into consideration, committed to master, and
> > this enhancement request closed.
> 
> I'm amazed by this, Juri.  You introduced stuff
> into this bug thread that is about another bug
> entirely (`*-allow-shift-selection').
> 
> I replied to your feedback about this bug, point by
> point.  And, hoping to help, I also provided some
> feedback about that other, unrelated bug.
> 
> What you quote above is about that _other_ bug, not
> this one.  I said that you can, if you like, ignore
> my feedback about `*-allow-shift-selection'.  That
> bug is not something I'm really concerned about -
> my feedback about it was just trying to be helpful.
> 
> As for this bug, I made clear that all that it
> calls for does not yet seem to be provided by
> your proposed changes.
> 
> Yet you reply (only) to my comment that you can
> ignore my feedback about `*-allow-shift-selection',
> to say that you have therefore closed _this_ bug.
> 
> I'm amazed - unless you did this accidentally,
> confusing the two bugs and meaning to close the
> other bug instead.
> 
> If you really intended to close this bug, please
> say how "all [my] remarks were taken into
> consideration" and how this enhancement request
> has been entirely fulfilled.  That's not clear
> to me.
> 
> From my point of view, it seems like you've made
> changes to fix other bugs that incidentally make
> progress toward also realizing the enhancement
> requested by this bug.  That's all good.
> 
> But I don't see that this enhancement has been
> realized.  I addressed this in specific ways,
> none of which you've responded to in this
> closure message.

Apologies, as I seem to have misunderstood.  I thought
that you were just saying that what you had come up
with last time is what you installed.

Regarding that, this is what I thought was unfinished:

>> No, this is not the case.  This is very difficult
>> to implement, and not worth the effort because this feature
>> is already available by customizing lazy-highlight-buffer.
>
> I see.  That disappoints me.  We have lazy
> highlighting but we can't use it lazily, except for
> the original design of being limited to a windowful?
>
> Can't we "just" let scrolling move the "window limits"
> as you scroll?  (Yes, I'm making this up, without
> looking at the code.)
>
> This highlight-as-you-go should really be considered
> part of this enhancement request, IMO.  And I mentioned
> it as such.  But if you don't want to tackle that part
> initially, or if indeed it ends up being too complex in
> the end, then so be it, for now.

I thought what you installed still included that telling
users that to ensure they get lazy highlighting everywhere
they scroll they may need to customize `lazy-highlight-buffer.

As you didn't say anything different I assumed that the
"very difficult to implement, and not worth the effort"
was what was reflected in what you installed.

I've now tried what you ended up installing, and I see
that that's not the case (and that you removed the advice
that you might need to set `lazy-highlight-buffer' to t
to see highlighting when scrolling).

What you installed does indeed seem to implement this
enhancement.  Thank you for that, and sorry for not
understanding and reacting negatively.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> I've now tried what you ended up installing, and I see
> that that's not the case (and that you removed the advice
> that you might need to set `lazy-highlight-buffer' to t
> to see highlighting when scrolling).
>
> What you installed does indeed seem to implement this
> enhancement.  Thank you for that, and sorry for not
> understanding and reacting negatively.

Sorry for replying to a wrong comment when closing this request,
there are so many comments that I picked a wrong one.

I completely implemented all points from your original request,
and this new feature works so well, that I already customized
`isearch-allow-scroll' to `unlimited' and started using it
even without enabling `lazy-highlight-buffer'.

BTW, regarding another feature that you helped to develop,
I just realized that the variable name `isearch-yank-on-move' is not
the best one.  I propose a better name `isearch-move-to-yank'.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > I've now tried what you ended up installing, and I see
> > that that's not the case (and that you removed the advice
> > that you might need to set `lazy-highlight-buffer' to t
> > to see highlighting when scrolling).
> >
> > What you installed does indeed seem to implement this
> > enhancement.  Thank you for that, and sorry for not
> > understanding and reacting negatively.
> 
> Sorry for replying to a wrong comment when closing this request,
> there are so many comments that I picked a wrong one.

I understand.  I too am lost in several threads (not to mention work ;-)).

> I completely implemented all points from your original request,
> and this new feature works so well, that I already customized
> `isearch-allow-scroll' to `unlimited' and started using it
> even without enabling `lazy-highlight-buffer'.

Yes, it works fine without `lazy-highlight-buffer', AFAICT.

What happened to "This is very difficult to
implement"? You seem to have silently (and
quickly) solved that one somehow.

> BTW, regarding another feature that you helped to develop,
> I just realized that the variable name `isearch-yank-on-move' is not
> the best one.  I propose a better name `isearch-move-to-yank'.

Sounds good.

Except that "move to" has a connotation of moving
to some place, e.g., move to a place where something
was yanked or will be yanked.

What about something like `cursor-movement-yanks'
or `movement-yanks' or `moving-yanks'?

---

FWIW, all such names, including `move-to-yank' would
be clearer with the RMS convention of having suffix
`-flag'.

That was the Emacs convention, until Stefan I think.
I still follow it.  And I use suffix `-p' for a
non-option variable.  (And yes, a Boolean variable
can be thought of as a (nullary) predicate.)

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

>> > I've now tried what you ended up installing, and I see
>> > that that's not the case (and that you removed the advice
>> > that you might need to set `lazy-highlight-buffer' to t
>> > to see highlighting when scrolling).
>> >
>> > What you installed does indeed seem to implement this
>> > enhancement.  Thank you for that, and sorry for not
>> > understanding and reacting negatively.
>>
>> Sorry for replying to a wrong comment when closing this request,
>> there are so many comments that I picked a wrong one.
>
> I understand.  I too am lost in several threads (not to mention work ;-)).
>
>> I completely implemented all points from your original request,
>> and this new feature works so well, that I already customized
>> `isearch-allow-scroll' to `unlimited' and started using it
>> even without enabling `lazy-highlight-buffer'.
>
> Yes, it works fine without `lazy-highlight-buffer', AFAICT.
>
> What happened to "This is very difficult to
> implement"? You seem to have silently (and
> quickly) solved that one somehow.

This is because I misunderstood what you described at first.
Later I realized what it's about.

>> BTW, regarding another feature that you helped to develop,
>> I just realized that the variable name `isearch-yank-on-move' is not
>> the best one.  I propose a better name `isearch-move-to-yank'.
>
> Sounds good.
>
> Except that "move to" has a connotation of moving
> to some place, e.g., move to a place where something
> was yanked or will be yanked.

I see, its ambiguity is like in `move-to-column'.

> What about something like `cursor-movement-yanks'
> or `movement-yanks' or `moving-yanks'?
>
> ---
>
> FWIW, all such names, including `move-to-yank' would
> be clearer with the RMS convention of having suffix
> `-flag'.
>
> That was the Emacs convention, until Stefan I think.
> I still follow it.  And I use suffix `-p' for a
> non-option variable.  (And yes, a Boolean variable
> can be thought of as a (nullary) predicate.)

Isn't the suffix `-p' allowed only for predicate functions?

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Drew Adams]

> > FWIW, all such names, including `move-to-yank' would
> > be clearer with the RMS convention of having suffix
> > `-flag'.
> >
> > That was the Emacs convention, until Stefan I think.
> > I still follow it.  And I use suffix `-p' for a
> > non-option variable.  (And yes, a Boolean variable
> > can be thought of as a (nullary) predicate.)
> 
> Isn't the suffix `-p' allowed only for predicate functions?

I believe that's the Emacs argument, yes.  In
particular, I believe it's Stefan's argument - dunno
whether it is in some way "official".

What I was saying was the _I_ use suffix `-flag' for
an option and suffix `-p' (or `p') for a non-option
variable.  That makes things very clear (to me).

Add `-flag' to any of the names considered above,
and the ambiguity and lack of clarity is diminished,
if not dispelled altogether.

And suffix `-flag' is not "disallowed", AFAIK.  Of
course, Isearch already has other boolean options
that do not use such a suffix.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Thank you for your feedback, all your remarks were taken into
> consideration, committed to master, and this enhancement request
> closed.

Thank you for working on this.

I have a question: I customized isearch-yank-on-move now to shift, but
left and right still exit isearch, I think because I (well, I guess, not
only I) have a global binding of shift-left and shift-right.

Is there a way to make shift-left and shift-right work nonetheless?  I
tried

#+begin_src emacs-lisp
  (define-key isearch-mode-map [(shift left)]  nil)
  (define-key isearch-mode-map [(shift right)] nil)
#+end_src

but that doesn't seem to do it.


Thanks,

Michael.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

> I have a question: I customized isearch-yank-on-move now to shift, but
> left and right still exit isearch, I think because I (well, I guess, not
> only I) have a global binding of shift-left and shift-right.

Isn't this binding of shift-left and shift-right from Org mode?
I still have no solution how to use this feature in Org mode.

Or maybe your shift-left and shift-right bindings come from
windmove-default-keybindings?  There is still no solution
because windmove's global shift-binding exits Isearch
and moves to another window.

> Is there a way to make shift-left and shift-right work nonetheless?  I
> tried
>
> #+begin_src emacs-lisp
>   (define-key isearch-mode-map [(shift left)]  nil)
>   (define-key isearch-mode-map [(shift right)] nil)
> #+end_src
>
> but that doesn't seem to do it.

I tried their original keybindings

(define-key isearch-mode-map [(shift left)]  'left-char)
(define-key isearch-mode-map [(shift right)] 'right-char)

but it still doesn't work.  Maybe we should support shift-key in isearch
explicitly, not relying on this-command-keys-shift-translated.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Isn't this binding of shift-left and shift-right from Org mode?

No, I simply have personal global bindings for these:

(global-set-key [(shift left)]  #'backward-char)
(global-set-key [(shift right)] #'forward-char)

> I tried their original keybindings
>
> (define-key isearch-mode-map [(shift left)]  'left-char)
> (define-key isearch-mode-map [(shift right)] 'right-char)
>
> but it still doesn't work.  Maybe we should support shift-key in
> isearch explicitly, not relying on this-command-keys-shift-translated.

Maybe, I dunno.  If something prominent as org-mode interferes with
that, it would probably good to find a solution.


Michael.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

>> Isn't this binding of shift-left and shift-right from Org mode?
>
> No, I simply have personal global bindings for these:
>
> (global-set-key [(shift left)]  #'backward-char)
> (global-set-key [(shift right)] #'forward-char)

Your keybindings are related to cursor motion,
so it makes sense to allow them to work in isearch-mode.

One way to do this:

(define-key isearch-mode-map [(shift left)]
  (lambda (&optional n)
    (interactive "^p")
    (setq isearch-pre-move-point (point))
    (call-interactively 'backward-char)))

(define-key isearch-mode-map [(shift right)]
  (lambda (&optional n)
    (interactive "^p")
    (setq isearch-pre-move-point (point))
    (call-interactively 'forward-char)))

Or do you want to be able to put properties to allow a command, e.g.:

(put 'backward-char 'isearch-move 'enabled)
(put 'forward-char 'isearch-move 'enabled)

with such patch

diff --git a/lisp/isearch.el b/lisp/isearch.el
index dcd119a517..6b0d9f03af 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2871,7 +2871,11 @@ isearch-pre-command-hook
                         (stringp (nth 1 (interactive-form this-command)))
                         (string-match-p "^^" (nth 1 (interactive-form this-command))))))
           (and (eq isearch-yank-on-move 'shift)
-               this-command-keys-shift-translated))
+               (or (and this-command-keys-shift-translated
+                        (symbolp this-command)
+                        (not (eq (get this-command 'isearch-move) 'disabled)))
+                   (and (symbolp this-command)
+                        (eq (get this-command 'isearch-move) 'enabled)))))
       (setq this-command-keys-shift-translated nil)
       (setq isearch-pre-move-point (point)))
      ;; Append control characters to the search string


I'm still not sure if this needed to be generalized more, and how far.
For example, I think that shift-left and shift-right
from org-mode should exit Isearch as well as windmove shift-keys
should exit Isearch and move focus to another window.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> +               (or (and this-command-keys-shift-translated
> +                        (symbolp this-command)
> +                        (not (eq (get this-command 'isearch-move) 'disabled)))
> +                   (and (symbolp this-command)
> +                        (eq (get this-command 'isearch-move) 'enabled)))))

Yes, that would probably be nice.

> I'm still not sure if this needed to be generalized more, and how far.
> For example, I think that shift-left and shift-right
> from org-mode should exit Isearch as well as windmove shift-keys
> should exit Isearch and move focus to another window.

This isearch-yank-on-move -> 'shift thing is a difficult matter.  I'm
not sure any more if it's a good idea.  It's confusing if I get a
different behavior depending on the current major mode.  As a user I
would rather expect that the rule would be that Isearch would test any
shift binding to see what the binding without shift is doing, and invoke
that if it is a moving command.


Thanks,

Michael.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

>> I'm still not sure if this needed to be generalized more, and how far.
>> For example, I think that shift-left and shift-right
>> from org-mode should exit Isearch as well as windmove shift-keys
>> should exit Isearch and move focus to another window.
>
> This isearch-yank-on-move -> 'shift thing is a difficult matter.  I'm
> not sure any more if it's a good idea.  It's confusing if I get a
> different behavior depending on the current major mode.  As a user I
> would rather expect that the rule would be that Isearch would test any
> shift binding to see what the binding without shift is doing, and invoke
> that if it is a moving command.

This is what this-command-keys-shift-translated was intended to do
together with ^ in interactive spec.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> > This isearch-yank-on-move -> 'shift thing is a difficult matter.
> > I'm not sure any more if it's a good idea.  It's confusing if I get
> > a different behavior depending on the current major mode.  As a user
> > I would rather expect that the rule would be that Isearch would test
> > any shift binding to see what the binding without shift is doing,
> > and invoke that if it is a moving command.
>
> This is what this-command-keys-shift-translated was intended to do
> together with ^ in interactive spec.

I don't follow.  If a shifted key, like shift-left, is bound, there is
no shift translation happening.  So your code doesn't kick in because
your condition isn't met, instead of calling the binding of the
unshifted key (left in my example).


Michael.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

>> > This isearch-yank-on-move -> 'shift thing is a difficult matter.
>> > I'm not sure any more if it's a good idea.  It's confusing if I get
>> > a different behavior depending on the current major mode.  As a user
>> > I would rather expect that the rule would be that Isearch would test
>> > any shift binding to see what the binding without shift is doing,
>> > and invoke that if it is a moving command.
>>
>> This is what this-command-keys-shift-translated was intended to do
>> together with ^ in interactive spec.
>
> I don't follow.  If a shifted key, like shift-left, is bound, there is
> no shift translation happening.  So your code doesn't kick in because
> your condition isn't met, instead of calling the binding of the
> unshifted key (left in my example).

The problem is that some commands that are called with shift-key
are unsuitable for isearch, e.g. S-M-< (beginning-of-buffer)
should not put all text from the beginning of the buffer
to the search string.

It seems we can't detect such commands automatically,
so one way to support a command is to put a property
with the patch that I sent recently, e.g.

(put 'backward-char 'isearch-move 'enabled)
(put 'forward-char 'isearch-move 'enabled)

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> The problem is that some commands that are called with shift-key
> are unsuitable for isearch, e.g. S-M-< (beginning-of-buffer)
> should not put all text from the beginning of the buffer
> to the search string.
>
> It seems we can't detect such commands automatically,
> so one way to support a command is to put a property
> with the patch that I sent recently, e.g.
>
> (put 'backward-char 'isearch-move 'enabled)
> (put 'forward-char 'isearch-move 'enabled)

That sounds ok to me.  It would even give the user a way to configure
which commands are ok.

Michael.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Juri Linkov]

tags 15839 fixed
close 15839 27.1
thanks

>> It seems we can't detect such commands automatically,
>> so one way to support a command is to put a property
>> with the patch that I sent recently, e.g.
>>
>> (put 'backward-char 'isearch-move 'enabled)
>> (put 'forward-char 'isearch-move 'enabled)
>
> That sounds ok to me.  It would even give the user a way to configure
> which commands are ok.

Pushed to master.  Please try.

bug#15839: 24.3.50; `isearch-allow-scroll': be able to scroll point off screen temporarily

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Pushed to master.  Please try.

Works for me - thanks!


Michael.