* RE: Please add comments to isearch.el
[not found] <<83fuzoojcn.fsf@gnu.org>
@ 2015-11-29 16:54 ` Drew Adams
0 siblings, 0 replies; 27+ messages in thread
From: Drew Adams @ 2015-11-29 16:54 UTC (permalink / raw
To: Eli Zaretskii, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Please add comments to isearch.el
@ 2015-11-29 16:34 Eli Zaretskii
2015-12-11 8:13 ` Eli Zaretskii
0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2015-11-29 16:34 UTC (permalink / raw
To: emacs-devel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-11-29 16:34 Eli Zaretskii
@ 2015-12-11 8:13 ` Eli Zaretskii
2015-12-11 12:16 ` Alan Mackenzie
0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-11 8:13 UTC (permalink / raw
To: emacs-devel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 8:13 ` Eli Zaretskii
@ 2015-12-11 12:16 ` Alan Mackenzie
2015-12-11 12:39 ` Eli Zaretskii
0 siblings, 1 reply; 27+ messages in thread
From: Alan Mackenzie @ 2015-12-11 12:16 UTC (permalink / raw
To: Eli Zaretskii; +Cc: emacs-devel
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).
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 12:16 ` Alan Mackenzie
@ 2015-12-11 12:39 ` Eli Zaretskii
2015-12-11 16:00 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-11 12:39 UTC (permalink / raw
To: Alan Mackenzie; +Cc: emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 12:39 ` Eli Zaretskii
@ 2015-12-11 16:00 ` Artur Malabarba
2015-12-11 16:20 ` Eli Zaretskii
0 siblings, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-11 16:00 UTC (permalink / raw
To: Eli Zaretskii; +Cc: Alan Mackenzie, emacs-devel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 16:00 ` Artur Malabarba
@ 2015-12-11 16:20 ` Eli Zaretskii
2015-12-11 16:54 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-11 16:20 UTC (permalink / raw
To: bruce.connor.am; +Cc: acm, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 16:20 ` Eli Zaretskii
@ 2015-12-11 16:54 ` Artur Malabarba
2015-12-11 16:56 ` Drew Adams
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Artur Malabarba @ 2015-12-11 16:54 UTC (permalink / raw
To: Eli Zaretskii; +Cc: Alan Mackenzie, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: Please add comments to isearch.el
2015-12-11 16:54 ` Artur Malabarba
@ 2015-12-11 16:56 ` Drew Adams
2015-12-11 18:33 ` Eli Zaretskii
2015-12-11 22:59 ` Juri Linkov
2 siblings, 0 replies; 27+ messages in thread
From: Drew Adams @ 2015-12-11 16:56 UTC (permalink / raw
To: bruce.connor.am, Eli Zaretskii; +Cc: Alan Mackenzie, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 16:54 ` Artur Malabarba
2015-12-11 16:56 ` Drew Adams
@ 2015-12-11 18:33 ` Eli Zaretskii
2015-12-11 22:59 ` Juri Linkov
2 siblings, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-11 18:33 UTC (permalink / raw
To: bruce.connor.am; +Cc: acm, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 16:54 ` Artur Malabarba
2015-12-11 16:56 ` Drew Adams
2015-12-11 18:33 ` Eli Zaretskii
@ 2015-12-11 22:59 ` Juri Linkov
2015-12-11 23:24 ` Artur Malabarba
` (2 more replies)
2 siblings, 3 replies; 27+ messages in thread
From: Juri Linkov @ 2015-12-11 22:59 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 22:59 ` Juri Linkov
@ 2015-12-11 23:24 ` Artur Malabarba
2015-12-11 23:55 ` Juri Linkov
2015-12-12 7:25 ` Eli Zaretskii
[not found] ` <<83d1uc6sdq.fsf@gnu.org>
2 siblings, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-11 23:24 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 23:24 ` Artur Malabarba
@ 2015-12-11 23:55 ` Juri Linkov
0 siblings, 0 replies; 27+ messages in thread
From: Juri Linkov @ 2015-12-11 23:55 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
>> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-11 22:59 ` Juri Linkov
2015-12-11 23:24 ` Artur Malabarba
@ 2015-12-12 7:25 ` Eli Zaretskii
2015-12-12 23:27 ` Juri Linkov
[not found] ` <<83d1uc6sdq.fsf@gnu.org>
2 siblings, 1 reply; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-12 7:25 UTC (permalink / raw
To: Juri Linkov; +Cc: acm, bruce.connor.am, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-12 7:25 ` Eli Zaretskii
@ 2015-12-12 23:27 ` Juri Linkov
2015-12-13 1:01 ` Artur Malabarba
2015-12-13 3:38 ` Eli Zaretskii
0 siblings, 2 replies; 27+ messages in thread
From: Juri Linkov @ 2015-12-12 23:27 UTC (permalink / raw
To: Eli Zaretskii; +Cc: acm, bruce.connor.am, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-12 23:27 ` Juri Linkov
@ 2015-12-13 1:01 ` Artur Malabarba
2015-12-14 0:16 ` Juri Linkov
2015-12-13 3:38 ` Eli Zaretskii
1 sibling, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-13 1:01 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-13 1:01 ` Artur Malabarba
@ 2015-12-14 0:16 ` Juri Linkov
2015-12-14 1:19 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Juri Linkov @ 2015-12-14 0:16 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-14 0:16 ` Juri Linkov
@ 2015-12-14 1:19 ` Artur Malabarba
2015-12-14 23:51 ` Juri Linkov
0 siblings, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-14 1:19 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-14 1:19 ` Artur Malabarba
@ 2015-12-14 23:51 ` Juri Linkov
2015-12-15 10:26 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Juri Linkov @ 2015-12-14 23:51 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
>> 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’?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-14 23:51 ` Juri Linkov
@ 2015-12-15 10:26 ` Artur Malabarba
2015-12-16 0:51 ` Juri Linkov
0 siblings, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-15 10:26 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-15 10:26 ` Artur Malabarba
@ 2015-12-16 0:51 ` Juri Linkov
2015-12-16 9:06 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Juri Linkov @ 2015-12-16 0:51 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
> 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?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-16 0:51 ` Juri Linkov
@ 2015-12-16 9:06 ` Artur Malabarba
2015-12-17 0:55 ` Juri Linkov
0 siblings, 1 reply; 27+ messages in thread
From: Artur Malabarba @ 2015-12-16 9:06 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-16 9:06 ` Artur Malabarba
@ 2015-12-17 0:55 ` Juri Linkov
2015-12-17 10:19 ` Artur Malabarba
0 siblings, 1 reply; 27+ messages in thread
From: Juri Linkov @ 2015-12-17 0:55 UTC (permalink / raw
To: Artur Malabarba; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
> 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?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-17 0:55 ` Juri Linkov
@ 2015-12-17 10:19 ` Artur Malabarba
0 siblings, 0 replies; 27+ messages in thread
From: Artur Malabarba @ 2015-12-17 10:19 UTC (permalink / raw
To: Juri Linkov; +Cc: Alan Mackenzie, Eli Zaretskii, emacs-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: Please add comments to isearch.el
2015-12-12 23:27 ` Juri Linkov
2015-12-13 1:01 ` Artur Malabarba
@ 2015-12-13 3:38 ` Eli Zaretskii
1 sibling, 0 replies; 27+ messages in thread
From: Eli Zaretskii @ 2015-12-13 3:38 UTC (permalink / raw
To: Juri Linkov; +Cc: acm, bruce.connor.am, emacs-devel
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
[parent not found: <<83d1uc6sdq.fsf@gnu.org>]
* RE: Please add comments to isearch.el
[not found] ` <<83d1uc6sdq.fsf@gnu.org>
@ 2015-12-12 16:20 ` Drew Adams
2015-12-12 23:04 ` John Wiegley
0 siblings, 1 reply; 27+ messages in thread
From: Drew Adams @ 2015-12-12 16:20 UTC (permalink / raw
To: Eli Zaretskii, Juri Linkov; +Cc: acm, bruce.connor.am, emacs-devel
> > > 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.)
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-12-17 10:19 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <<83fuzoojcn.fsf@gnu.org>
2015-11-29 16:54 ` Please add comments to isearch.el Drew Adams
2015-11-29 16:34 Eli Zaretskii
2015-12-11 8:13 ` Eli Zaretskii
2015-12-11 12:16 ` Alan Mackenzie
2015-12-11 12:39 ` Eli Zaretskii
2015-12-11 16:00 ` Artur Malabarba
2015-12-11 16:20 ` Eli Zaretskii
2015-12-11 16:54 ` Artur Malabarba
2015-12-11 16:56 ` Drew Adams
2015-12-11 18:33 ` Eli Zaretskii
2015-12-11 22:59 ` Juri Linkov
2015-12-11 23:24 ` Artur Malabarba
2015-12-11 23:55 ` Juri Linkov
2015-12-12 7:25 ` Eli Zaretskii
2015-12-12 23:27 ` Juri Linkov
2015-12-13 1:01 ` Artur Malabarba
2015-12-14 0:16 ` Juri Linkov
2015-12-14 1:19 ` Artur Malabarba
2015-12-14 23:51 ` Juri Linkov
2015-12-15 10:26 ` Artur Malabarba
2015-12-16 0:51 ` Juri Linkov
2015-12-16 9:06 ` Artur Malabarba
2015-12-17 0:55 ` Juri Linkov
2015-12-17 10:19 ` Artur Malabarba
2015-12-13 3:38 ` Eli Zaretskii
[not found] ` <<83d1uc6sdq.fsf@gnu.org>
2015-12-12 16:20 ` Drew Adams
2015-12-12 23:04 ` John Wiegley
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.