RE: Please add comments to isearch.el

RE: Please add comments to isearch.el

[Drew Adams]

> Would someone "in the know" please add commentary to isearch.el to
> explain how the various options are passed to the commands defined
> there and affect or not affect them?
> 
> The isearch.el commands are implemented using complex multi-layered
> system of functions and macros that make it very hard to figure out,
> just by looking at the code, which options affect what commands and in
> what ways.  About the only way to find that out is by trying each
> command, which is very inefficient.  I think it will help make this
> file much more maintainable if commentary were added there explaining
> how all of this works.  Thanks in advance.

+1

Particularly as regards the state structure and updating it.

Please add comments to isearch.el

[Eli Zaretskii]

Would someone "in the know" please add commentary to isearch.el to
explain how the various options are passed to the commands defined
there and affect or not affect them?

The isearch.el commands are implemented using complex multi-layered
system of functions and macros that make it very hard to figure out,
just by looking at the code, which options affect what commands and in
what ways.  About the only way to find that out is by trying each
command, which is very inefficient.  I think it will help make this
file much more maintainable if commentary were added there explaining
how all of this works.  Thanks in advance.

Re: Please add comments to isearch.el

[Eli Zaretskii]

Ping!

> Date: Sun, 29 Nov 2015 18:34:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Would someone "in the know" please add commentary to isearch.el to
> explain how the various options are passed to the commands defined
> there and affect or not affect them?
> 
> The isearch.el commands are implemented using complex multi-layered
> system of functions and macros that make it very hard to figure out,
> just by looking at the code, which options affect what commands and in
> what ways.  About the only way to find that out is by trying each
> command, which is very inefficient.  I think it will help make this
> file much more maintainable if commentary were added there explaining
> how all of this works.  Thanks in advance.

Re: Please add comments to isearch.el

[Alan Mackenzie]

Hello, Eli.

On Fri, Dec 11, 2015 at 10:13:21AM +0200, Eli Zaretskii wrote:
> Ping!

> > Date: Sun, 29 Nov 2015 18:34:16 +0200
> > From: Eli Zaretskii <eliz@gnu.org>

> > Would someone "in the know" please add commentary to isearch.el to
> > explain how the various options are passed to the commands defined
> > there and affect or not affect them?

i'm not sure I count as someone in the know, but ....

> > The isearch.el commands are implemented using complex multi-layered
> > system of functions and macros that make it very hard to figure out,
> > just by looking at the code, which options affect what commands and in
> > what ways.  About the only way to find that out is by trying each
> > command, which is very inefficient.  I think it will help make this
> > file much more maintainable if commentary were added there explaining
> > how all of this works.  Thanks in advance.

.... perhaps you could be a little more explicit about which "complex
multi-layered system of functions and macros" you were thinking about in
particular?  Was it the one I was complaining of a few days ago, or were
you thinking of the state stack (that Drew explicitly mentioned), or
something else?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Please add comments to isearch.el

[Eli Zaretskii]

> Date: Fri, 11 Dec 2015 12:16:17 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > The isearch.el commands are implemented using complex multi-layered
> > > system of functions and macros that make it very hard to figure out,
> > > just by looking at the code, which options affect what commands and in
> > > what ways.  About the only way to find that out is by trying each
> > > command, which is very inefficient.  I think it will help make this
> > > file much more maintainable if commentary were added there explaining
> > > how all of this works.  Thanks in advance.
> 
> .... perhaps you could be a little more explicit about which "complex
> multi-layered system of functions and macros" you were thinking about in
> particular?  Was it the one I was complaining of a few days ago, or were
> you thinking of the state stack (that Drew explicitly mentioned), or
> something else?

For example, start with isearch-mode, and then try to figure out what
each one of the following variables are in what use cases:
regexp-function and search-default-regexp-mode (as a function).  You
will see that each one can be assigned to a variable that names a
function that has a default value that can be a function that...

Similarly with other isearch commands.  This makes the source
impenetrable to uninitiated, much harder than just stepping with
Edebug through the code.  IMO, if it is easier to understand code by
stepping through it in a debugger than by reading it, that code must
be refactored or documented the heck of.

Re: Please add comments to isearch.el

[Artur Malabarba]

2015-12-11 12:39 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
> For example, start with isearch-mode, and then try to figure out what
> each one of the following variables are in what use cases:
> regexp-function and search-default-regexp-mode (as a function).  You
> will see that each one can be assigned to a variable that names a
> function that has a default value that can be a function that...

I didn't reply to your initial message for lack of time. I can add
some comments here and there on the parts I understand. (some time in
the coming months)

> Similarly with other isearch commands.  This makes the source
> impenetrable to uninitiated, much harder than just stepping with
> Edebug through the code.  IMO, if it is easier to understand code by
> stepping through it in a debugger than by reading it, that code must
> be refactored or documented the heck of.

Honestly, I'd love to overhaul isearch. But I couldn't do it
backwards-compatibly.

Re: Please add comments to isearch.el

[Eli Zaretskii]

> Date: Fri, 11 Dec 2015 16:00:27 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: Alan Mackenzie <acm@muc.de>, emacs-devel <emacs-devel@gnu.org>
> 
> 2015-12-11 12:39 GMT+00:00 Eli Zaretskii <eliz@gnu.org>:
> > For example, start with isearch-mode, and then try to figure out what
> > each one of the following variables are in what use cases:
> > regexp-function and search-default-regexp-mode (as a function).  You
> > will see that each one can be assigned to a variable that names a
> > function that has a default value that can be a function that...
> 
> I didn't reply to your initial message for lack of time. I can add
> some comments here and there on the parts I understand. (some time in
> the coming months)

Thanks in advance.

> > Similarly with other isearch commands.  This makes the source
> > impenetrable to uninitiated, much harder than just stepping with
> > Edebug through the code.  IMO, if it is easier to understand code by
> > stepping through it in a debugger than by reading it, that code must
> > be refactored or documented the heck of.
> 
> Honestly, I'd love to overhaul isearch. But I couldn't do it
> backwards-compatibly.

That's what master is for.

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 11 Dec 2015 4:20 pm, "Eli Zaretskii" <eliz@gnu.org> wrote:
> >
> > Honestly, I'd love to overhaul isearch. But I couldn't do it
> > backwards-compatibly.
>
> That's what master is for.

I don't mind breaking things on master.
The point is that Isearch is probably one of the most used emacs features.
I expect there are lots of packages out there using its api. I'm not sure
overhauling it would be worth the pain it would cause to everyone who's
maintaining/using those packages.

[-- Attachment #2: Type: text/html, Size: 649 bytes --]

RE: Please add comments to isearch.el

[Drew Adams]

> I don't mind breaking things on master. 
> The point is that Isearch is probably one of the most used
> emacs features. I expect there are lots of packages out there
> using its api. I'm not sure overhauling it would be worth the
> pain it would cause to everyone who's maintaining/using those
> packages.

Indeed.

Re: Please add comments to isearch.el

[Eli Zaretskii]

> Date: Fri, 11 Dec 2015 16:54:20 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: emacs-devel <emacs-devel@gnu.org>, Alan Mackenzie <acm@muc.de>
> 
> The point is that Isearch is probably one of the most used emacs features. I
> expect there are lots of packages out there using its api. I'm not sure
> overhauling it would be worth the pain it would cause to everyone who's
> maintaining/using those packages. 

If it makes the code much cleaner, IMO it's worth it.

Re: Please add comments to isearch.el

[Juri Linkov]

> I don't mind breaking things on master.
> The point is that Isearch is probably one of the most used emacs features.
> I expect there are lots of packages out there using its api. I'm not sure
> overhauling it would be worth the pain it would cause to everyone who's
> maintaining/using those packages.

+1

For example, the recent removal of a layer of indirection for lax-whitespace
broke customizations that allowed overriding search-forward-lax-whitespace
with own implementation to ignore all possible whitespace instead of
just spaces in the search string.

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 11 Dec 2015 10:59 pm, "Juri Linkov" <juri@linkov.net> wrote:
>
> For example, the recent removal of a layer of indirection for
lax-whitespace
> broke customizations that allowed overriding search-forward-lax-whitespace
> with own implementation to ignore all possible whitespace instead of
> just spaces in the search string.

How recent? Was that reported?

[-- Attachment #2: Type: text/html, Size: 488 bytes --]

Re: Please add comments to isearch.el

[Juri Linkov]

>> For example, the recent removal of a layer of indirection for lax-whitespace
>> broke customizations that allowed overriding search-forward-lax-whitespace
>> with own implementation to ignore all possible whitespace instead of
>> just spaces in the search string.
>
> How recent? Was that reported?

Very recent.  I reported it now in bug#22147.

Re: Please add comments to isearch.el

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 12 Dec 2015 00:59:38 +0200
> Cc: Alan Mackenzie <acm@muc.de>, Eli Zaretskii <eliz@gnu.org>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> > I don't mind breaking things on master.
> > The point is that Isearch is probably one of the most used emacs features.
> > I expect there are lots of packages out there using its api. I'm not sure
> > overhauling it would be worth the pain it would cause to everyone who's
> > maintaining/using those packages.
> 
> +1
> 
> For example, the recent removal of a layer of indirection for lax-whitespace
> broke customizations that allowed overriding search-forward-lax-whitespace
> with own implementation to ignore all possible whitespace instead of
> just spaces in the search string.

Hey, I didn't suggest refactoring to begin with.  I suggested to add
commentary to explain how things work there.  The example you give is
just another confirmation of my observation that the code in
isearch.el has long ago crossed the line of being unmaintainable, and
I think we should fix that ASAP.

Thanks.

Re: Please add comments to isearch.el

[Juri Linkov]

> Hey, I didn't suggest refactoring to begin with.  I suggested to add
> commentary to explain how things work there.  The example you give is
> just another confirmation of my observation that the code in
> isearch.el has long ago crossed the line of being unmaintainable, and
> I think we should fix that ASAP.
>
> Thanks.

I completely agree, and as a first step in this direction I propose
to fix the terminology used in isearch that should help to better
understand the code in isearch.el.

1. Rename ‘lax’ to ‘lax-boundary’ to distinguish between lax at the word/symbol
   boundary and lax-whitespace to avoid confusion, so rename the
   arg ‘lax’ to ‘lax-boundary’ in all regexp-producing functions,
   isearch--lax-regexp-function-p to
   isearch--lax-boundary-regexp-function-p, etc.

2. Try to find a better common naming scheme used for the
   regexp-producing functions word-search-regexp, isearch-symbol-regexp,
   character-fold-to-regexp.

3. Use well established terminology that shortens the prefixes
   of character-related functions to just char- to use char-fold.
   ‘C-h f char TAB’ or ‘C-h v char TAB’ shows the standard
   Emacs naming convention.

4. Use the upper-case standard name “Unicode” in the documentation.

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 12 Dec 2015 11:27 pm, "Juri Linkov" <juri@linkov.net> wrote:
> 1. Rename ‘lax’ to ‘lax-boundary’ to distinguish between lax at the
word/symbol
>    boundary and lax-whitespace to avoid confusion, so rename the
>    arg ‘lax’ to ‘lax-boundary’ in all regexp-producing functions,
>    isearch--lax-regexp-function-p to
>    isearch--lax-boundary-regexp-function-p, etc.

+1

> 2. Try to find a better common naming scheme used for the
>    regexp-producing functions word-search-regexp, isearch-symbol-regexp,
>    character-fold-to-regexp.

I think the first two shouldn't even be in isearch.el. There's a ton of
stuff in isearch.el that's just generally useful in searches (nothing
specific to incremental search), and should be moved to some search.el
file.

> 3. Use well established terminology that shortens the prefixes
>    of character-related functions to just char- to use char-fold.
>    ‘C-h f char TAB’ or ‘C-h v char TAB’ shows the standard
>    Emacs naming convention.
>
> 4. Use the upper-case standard name “Unicode” in the documentation.

I don't see what these have to do with the readability of isearch el, but
by all means, feel free to do these. 👍

[-- Attachment #2: Type: text/html, Size: 1497 bytes --]

Re: Please add comments to isearch.el

[Juri Linkov]

> On 12 Dec 2015 11:27 pm, "Juri Linkov" <juri@linkov.net> wrote:
>> 1. Rename ‘lax’ to ‘lax-boundary’ to distinguish between lax at the
> word/symbol
>>    boundary and lax-whitespace to avoid confusion, so rename the
>>    arg ‘lax’ to ‘lax-boundary’ in all regexp-producing functions,
>>    isearch--lax-regexp-function-p to
>>    isearch--lax-boundary-regexp-function-p, etc.
>
> +1
>
>> 2. Try to find a better common naming scheme used for the
>>    regexp-producing functions word-search-regexp, isearch-symbol-regexp,
>>    character-fold-to-regexp.
>
> I think the first two shouldn't even be in isearch.el. There's a ton of
> stuff in isearch.el that's just generally useful in searches (nothing
> specific to incremental search), and should be moved to some search.el
> file.

Yes, everything with the prefix ‘search-’ in isearch.el
(as well as occur commands in replace.el) are here for historical reasons.

>> 3. Use well established terminology that shortens the prefixes
>>    of character-related functions to just char- to use char-fold.
>>    ‘C-h f char TAB’ or ‘C-h v char TAB’ shows the standard
>>    Emacs naming convention.
>>
>> 4. Use the upper-case standard name “Unicode” in the documentation.

Ohh, and I'd add also search-default-regexp-mode -> search-default-regexp-function
that currently causes confusion due to its similarity with isearch-regexp-function.

> I don't see what these have to do with the readability of isearch el, but
> by all means, feel free to do these.

Maybe better to wait for establishing of regular merges from emacs-25 to
master, because I'm afraid that renaming character-fold.el to char-fold.el
now might cause merge conflicts on the first such merge.

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 14 Dec 2015 12:16 am, "Juri Linkov" <juri@linkov.net> wrote:
> Ohh, and I'd add also search-default-regexp-mode ->
search-default-regexp-function
> that currently causes confusion due to its similarity with
isearch-regexp-function.

The thing is that this variable is not necessarily a function, it can be
nil or t too. I'd rather reserve the suffix "-function" for variables that
necessarily hold a function.

> > I don't see what these have to do with the readability of isearch el,
but
> > by all means, feel free to do these.
>
> Maybe better to wait for establishing of regular merges from emacs-25 to
> master, because I'm afraid that renaming character-fold.el to char-fold.el
> now might cause merge conflicts on the first such merge.

As long as no changes have been made to this file on the master branch,
there shouldn't be any conflict. But it's probably best to wait.

[-- Attachment #2: Type: text/html, Size: 1099 bytes --]

Re: Please add comments to isearch.el

[Juri Linkov]

>> Ohh, and I'd add also search-default-regexp-mode -> search-default-regexp-function
>> that currently causes confusion due to its similarity with isearch-regexp-function.
>
> The thing is that this variable is not necessarily a function, it can be
> nil or t too. I'd rather reserve the suffix "-function" for variables that
> necessarily hold a function.

A variable with the suffix "-function" is allowed to have a nil value,
but not t, indeed.  What about using a function instead of t, maybe ‘identity’?

Re: Please add comments to isearch.el

[Artur Malabarba]

2015-12-14 23:51 GMT+00:00 Juri Linkov <juri@linkov.net>:
>>> Ohh, and I'd add also search-default-regexp-mode -> search-default-regexp-function
>>> that currently causes confusion due to its similarity with isearch-regexp-function.
>>
>> The thing is that this variable is not necessarily a function, it can be
>> nil or t too. I'd rather reserve the suffix "-function" for variables that
>> necessarily hold a function.
>
> A variable with the suffix "-function" is allowed to have a nil value,
> but not t, indeed.

Not all do. None of the font-lock-*-function variables allow a nil
value. And I'd rather push in that direction. One of the advantages of
-function variables is that you can use `add-function' on them, which
is a powerful and convenient interface. Allowing the -function
variable to have a nil value prevents the easy use of `add-function'.

> What about using a function instead of t, maybe ‘identity’?

I've mentioned here before that I'd like to obsolete `isearch-regexp'
and simply use `identity' for the regexp-function in that case.
However, the problem is that doing a regexp isearch is not exactly the
same as doing an isearch with `identity' as the regexp function.
Isearch has some special handlings for regexp input, that only kick in
if `isearch-regexp' is non-nil. If we generalize these handlings a bit
so they play nice with isearch-regexp-function, then we can obsolete
isearch-regexp.

If we go that route, then we might as well use `regexp-quote' instead
of the nil value too.

Re: Please add comments to isearch.el

[Juri Linkov]

> If we go that route, then we might as well use `regexp-quote' instead
> of the nil value too.

Then another idea: what about broadening the customization variable
search-default-regexp-mode to search-default-mode and allowing to
customize a list of default values for other search parameters defined by
isearch-define-mode-toggle, e.g. lax-whitespace, case-fold, invisible?

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 16 Dec 2015 12:51 am, "Juri Linkov" <juri@linkov.net> wrote:
>
> > If we go that route, then we might as well use `regexp-quote' instead
> > of the nil value too.
>
> Then another idea: what about broadening the customization variable
> search-default-regexp-mode to search-default-mode and allowing to
> customize a list of default values for other search parameters defined by
> isearch-define-mode-toggle, e.g. lax-whitespace, case-fold, invisible?

I don't really like custom variables that try to do everything and have
complex structures. They tend to be intimidating to new users, precisely
those who would benefit the most from the custom interface.
They are also harder to use and harder to write docstrings for. Finally,
they offer no benefit over just defining N streamlined variables whose
docstrings mention each other. :-)

[-- Attachment #2: Type: text/html, Size: 1001 bytes --]

Re: Please add comments to isearch.el

[Juri Linkov]

> I don't really like custom variables that try to do everything and have
> complex structures. They tend to be intimidating to new users, precisely
> those who would benefit the most from the custom interface.
> They are also harder to use and harder to write docstrings for. Finally,
> they offer no benefit over just defining N streamlined variables whose
> docstrings mention each other. :-)

Then what do you think about two customizable variables:

1. search-default-regexp-function that always holds a function;

2. search-default-regexp-mode (or just search-default-mode)
   that defines whether to use search-default-regexp-function,
   or use a regexp search, or use a literal search?

Re: Please add comments to isearch.el

[Artur Malabarba]

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

On 17 Dec 2015 12:55 am, "Juri Linkov" <juri@linkov.net> wrote:
>
> > I don't really like custom variables that try to do everything and have
> > complex structures.
>
> Then what do you think about two customizable variables:
>
> 1. search-default-regexp-function that always holds a function;
>
> 2. search-default-regexp-mode (or just search-default-mode)
>    that defines whether to use search-default-regexp-function,
>    or use a regexp search, or use a literal search?

I'm fine with this. And, just to be clear, I'm also fine with the
suggestion of having a single variable and using the identity to indicate
regexp-search (like I said, it would just take a bit of care to not lose
any functionality).

My objection about huge variables was just with respect to the suggestion
of also bundling other options into that variable, like lax-whitespace,
invisible, case-fold.

[-- Attachment #2: Type: text/html, Size: 1097 bytes --]

Re: Please add comments to isearch.el

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: bruce.connor.am@gmail.com,  acm@muc.de,  emacs-devel@gnu.org
> Date: Sun, 13 Dec 2015 01:27:45 +0200
> 
> > Hey, I didn't suggest refactoring to begin with.  I suggested to add
> > commentary to explain how things work there.  The example you give is
> > just another confirmation of my observation that the code in
> > isearch.el has long ago crossed the line of being unmaintainable, and
> > I think we should fix that ASAP.
> >
> > Thanks.
> 
> I completely agree, and as a first step in this direction I propose
> to fix the terminology used in isearch that should help to better
> understand the code in isearch.el.
> 
> 1. Rename ‘lax’ to ‘lax-boundary’ to distinguish between lax at the word/symbol
>    boundary and lax-whitespace to avoid confusion, so rename the
>    arg ‘lax’ to ‘lax-boundary’ in all regexp-producing functions,
>    isearch--lax-regexp-function-p to
>    isearch--lax-boundary-regexp-function-p, etc.
> 
> 2. Try to find a better common naming scheme used for the
>    regexp-producing functions word-search-regexp, isearch-symbol-regexp,
>    character-fold-to-regexp.
> 
> 3. Use well established terminology that shortens the prefixes
>    of character-related functions to just char- to use char-fold.
>    ‘C-h f char TAB’ or ‘C-h v char TAB’ shows the standard
>    Emacs naming convention.
> 
> 4. Use the upper-case standard name “Unicode” in the documentation.

Fine with me (please go ahead), as long as we don't stop there.
Because terminology was not my problem when I tried to figure out who
does what there.

Thanks.

RE: Please add comments to isearch.el

[Drew Adams]

> > > The point is that Isearch is probably one of the most used emacs
> > > features.  I expect there are lots of packages out there using
> > > its api. I'm not sure overhauling it would be worth the pain it
> > > would cause to everyone who's maintaining/using those packages.
> >
> > +1
> >
> > For example, the recent removal of a layer of indirection for
> > lax-whitespace broke customizations that allowed overriding
> > search-forward-lax-whitespace with own implementation to ignore
> > all possible whitespace instead of just spaces in the search string.
> 
> Hey, I didn't suggest refactoring to begin with.  I suggested to add
> commentary to explain how things work there.  The example you give is
> just another confirmation of my observation that the code in
> isearch.el has long ago crossed the line of being unmaintainable, and
> I think we should fix that ASAP.

FWIW: My +1 above was for caution wrt a major rewrite of the code,
not wrt minor cleanup that improves readability and clearly does
not affect behavior or (preferably) compatibility with the
previous code.

And as I said before that, +1 especially for adding the design
comments that Eli has requested.  And +1 to his suggestion ("to
begin with") that such commenting should come before any code
cleanup.  (Any such comments will also help with any subsequent
proposed code cleanup and discussions about it.)

Re: Please add comments to isearch.el

[John Wiegley]

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

>>>>> Drew Adams <drew.adams@oracle.com> writes:

> And as I said before that, +1 especially for adding the design comments that
> Eli has requested. And +1 to his suggestion ("to begin with") that such
> commenting should come before any code cleanup. (Any such comments will also
> help with any subsequent proposed code cleanup and discussions about it.)

Adding more +1s here, as necessary. :)

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]