bug#47599: 28.0.50; Feature request improve/update isearch

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Emacs - Bugs mailing list
Hi:

Just to follow this:
https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg00080.html

I open a feature request to suggest some updates in the isearch code and
the addition of some simple functionalities like:

1) Option or command to automatically go to the other end on exit.

2) Convert isearch-wrap-function, isearch-push-state-function,
isearch-filter-predicate into customs instead of use defvar, improve a
bit their documentation and provide some handy options.

3) Make some general refactor of isearch code to simplify and remove
some redundant or useless checks, vars and code. Update the code to use
some useful modern api like define-minor-mode or easy-mmode-defmap.

There are some external packages that reimplement the isearch
functionalities or hack it to produce some of these
functionalities/behavior and maybe (as an extra) we could consider add
some customs to make isearch behave like a bit like that; because most
of them are not very different to what isearch already does now.

Example: https://github.com/raxod502/ctrlf 

Some of the differences where mentioned already here:

https://lists.gnu.org/archive/html/emacs-devel/2021-04/msg00108.html




Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2

>>> 1) Option or command to automatically go to the other end on exit.
>>
>> It seems the conclusion was that it should not be an option. As for a
>> command, there were objections against binding it to a key, but without
>> a keybinding such command has little sense.
>
> I attach a patch which implements these two "options" in isearch-exit.
>

And of course my patch had an error ;-)

0001-Add-options-when-exiting-isearch.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2

>>>> 1) Option or command to automatically go to the other end on exit.
>>>
>>> It seems the conclusion was that it should not be an option. As for a
>>> command, there were objections against binding it to a key, but
>>> without a keybinding such command has little sense.
>>
>> I attach a patch which implements these two "options" in isearch-exit.
>
> And of course my patch had an error ;-)
>
While we're improving isearch, there is another minor change that I think
would improve its behavior, namely to go to the next/previous match when
changing the search direction, instead of hitting the same match before
moving to the next/previous one with the next C-s/C-r.  This change is so
small that I'm not sure it's worth creating a defcustom for it, but you
might have a different opinion.

0001-Find-another-match-when-changing-direction-in-isearc.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#47599: [External] : bug#47599: 28.0.50; Feature request improve/update isearch

Drew Adams
> While we're improving isearch, there is another minor change that I think
> would improve its behavior, namely to go to the next/previous match when
> changing the search direction, instead of hitting the same match before
> moving to the next/previous one with the next C-s/C-r.  This change is so
> small that I'm not sure it's worth creating a defcustom for it, but you
> might have a different opinion.

1. Please file a separate bug report when you move the goal post.

2. Please do NOT do this.  There are reasons we want to
   stay at the same search hit.  One of them has already
   surfaced in these (lamentable) discussions of isearch
   "improvements": namely, use `C-r RET' to move to the
   beginning of the search hit (when searching forward).




Reply | Threaded
Open this post in threaded view
|

bug#47599: [External] : bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2

>> While we're improving isearch, there is another minor change that I
>> think would improve its behavior, namely to go to the next/previous
>> match when changing the search direction, instead of hitting the same
>> match before moving to the next/previous one with the next C-s/C-r.
>> This change is so small that I'm not sure it's worth creating a
>> defcustom for it, but you might have a different opinion.
>
> Please do NOT do this.  There are reasons we want to stay at the same
> search hit.
>

Okay, so I have the answer to my own question: this should become yet
another user option.

>
> One of them has already surfaced in these (lamentable) discussions of
> isearch "improvements":
>

Why "lamentable"???



Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2
In reply to this post by Emacs - Bugs mailing list

>>> 1) Option or command to automatically go to the other end on exit.
>>
>> It seems the conclusion was that it should not be an option. As for a
>> command, there were objections against binding it to a key, but without
>> a keybinding such command has little sense.
>
> I attach a patch which implements these two "options" in isearch-exit.
>

And here is an updated patch which adds three options to isearch-exit: C-u
RET moves point to the other end, C-u C-u RET activates region around
match, C-u C-u C-u RET moves point to the other end and activate region
around match.

0001-Add-options-when-exiting-isearch.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2

>> And here is an updated patch which adds three options to isearch-exit:
>> C-u RET moves point to the other end, C-u C-u RET activates region
>> around match, C-u C-u C-u RET moves point to the other end and activate
>> region around match.
>
> Hi Gregory:
>
> Maybe I am wrong but I think it makes no sense to add C-u RET for this;
> because it is not better than C-r RET. Same applies to the other two
> commands.
>

I have no strong opinion, I proposed this patch because of your request.
But C-u RET is better I think, it's a single command instead of two (C-r
RET and C-s RET).  For the two other commands, I don't know an easy way to
mark the match.

>
> I am only interested in the first one really;
>

Yes, I was proposing something more general, with what I understood from
the discussion.

>
> But IMO the C-u C-u C-u is not the way we should go..
>

I think it's easy to type, much easier than M-s RET for example, but as I
sait I have no strong opinion on this, it's fine if my patch isn't
applied.



Reply | Threaded
Open this post in threaded view
|

bug#47599: [External] : bug#47599: 28.0.50; Feature request improve/update isearch

Juri Linkov-2
In reply to this post by Gregory Heytings-2
>>>> While we're improving isearch, there is another minor change that
>>>> I think would improve its behavior, namely to go to the next/previous
>>>> match when changing the search direction, instead of hitting the same
>>>> match before moving to the next/previous one with the next
>>>> C-s/C-r. This change is so small that I'm not sure it's worth creating
>>>> a defcustom for it, but you might have a different opinion.
>>> Please do NOT do this.  There are reasons we want to stay at the same
>>> search hit.
>>
>> Okay, so I have the answer to my own question: this should become yet
>> another user option.
>
> And here is the corresponding patch.

Thanks, finally there is an option to avoid typing extra C-r.

> +(defcustom isearch-direction-change-changes-match nil
> +  "Whether a direction change should move to another match.
> +When `nil', the default, a direction change moves point to the other
> +end of the current search match.
> +When `t', a direction change moves to another search match, if there
> +is one."
> +  :type '(choice (const :tag "Remain on the same match" nil)
> +                 (const :tag "Move to another match" t))

Is it possible to find a clearer name?
Maybe isearch-repeat-on-direction-change would be better
with the prefix 'isearch-repeat-' to hint that it applies
to the commands 'isearch-repeat-*'?

>      ;; C-s in reverse or C-r in forward, change direction.
> +    (if (and isearch-other-end isearch-direction-change-changes-match)
> +        (goto-char isearch-other-end))

This breaks the following feature:

When isearch-forward is t:
- C-1 C-r moves to the previous match (like your patch does without 'C-1')
- C-2 C-r moves to the second previous match
- C-u -1 C-r moves to the next match
- C-u -2 C-r moves to the second next match

This is due to these lines in isearch-repeat-backward:

               ;; Reverse the direction back
               (isearch-repeat 'backward))
              (t
               ;; Take into account one iteration to reverse direction
               (when isearch-forward (setq count (1+ count)))

When the new option is non-nil, there is no need to increment 'count'.
Also the new option should be let-bound to nil around the call to
'(isearch-repeat 'backward)' above to just change the direction back
without moving to the next match.

The same applies to isearch-repeat-forward and when isearch-forward is nil.



Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Gregory Heytings-2

>
> Thanks, finally there is an option to avoid typing extra C-r.
>

:-)  And thanks for your feedback!

>
> Is it possible to find a clearer name?
> Maybe isearch-repeat-on-direction-change would be better with the prefix
> 'isearch-repeat-' to hint that it applies to the commands
> 'isearch-repeat-*'?
>

Done.

>
> This breaks the following feature:
>
> When isearch-forward is t:
> - C-1 C-r moves to the previous match (like your patch does without 'C-1')
> - C-2 C-r moves to the second previous match
> - C-u -1 C-r moves to the next match
> - C-u -2 C-r moves to the second next match
>
> This is due to these lines in isearch-repeat-backward:
>
>               ;; Reverse the direction back
>               (isearch-repeat 'backward))
>              (t
>               ;; Take into account one iteration to reverse direction
>               (when isearch-forward (setq count (1+ count)))
>
> When the new option is non-nil, there is no need to increment 'count'.
> Also the new option should be let-bound to nil around the call to
> '(isearch-repeat 'backward)' above to just change the direction back
> without moving to the next match.
>
> The same applies to isearch-repeat-forward and when isearch-forward is nil.
>
Fixed, thank you!

0001-User-option-to-move-to-another-match-when-changing-d.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#47599: 28.0.50; Feature request improve/update isearch

Juri Linkov-2
>> Is it possible to find a clearer name?
>> Maybe isearch-repeat-on-direction-change would be better with the prefix
>> 'isearch-repeat-' to hint that it applies to the commands
>> 'isearch-repeat-*'?
>
> Done.

Now your patch is pushed in 972bab0981.  Thanks for such useful feature!