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

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

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