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

[Drew Adams <drew.adams@oracle.com>]

> > 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