bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Alan Mackenzie]

Hello, Emacs.

In the Emacs master branch
(i) emacs -Q
(ii) C-x C-f emacs/src/syntax.c<CR>
(iii) C-M-s \(inc\|dec\)_both
(iv) Press C-s a few times.

Notice that the highlighting of the found match has the purple
background over the correct characters, but the characters displayed in
the foreground are "   _both".  This incorrect display is a bug.

(v) Press <backspace> a few times.  The lazy highlighting is now
correctly displayed each time.
(vi) Note that if one starts at the end of the buffer with C-M-r, etc.,
the same problem occurs.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Eli Zaretskii]

> Date: Tue, 29 Sep 2020 12:13:17 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: acm@muc.de
> 
> In the Emacs master branch
> (i) emacs -Q
> (ii) C-x C-f emacs/src/syntax.c<CR>
> (iii) C-M-s \(inc\|dec\)_both
> (iv) Press C-s a few times.
> 
> Notice that the highlighting of the found match has the purple
> background over the correct characters, but the characters displayed in
> the foreground are "   _both".  This incorrect display is a bug.

Here, I don't see any incorrect display, I see a new feature at work.
If you don't like it, set search-highlight-submatches to nil.

How many colors do you have on the terminal where you see the problem?

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Alan Mackenzie]

Hello, Eli.

On Tue, Sep 29, 2020 at 17:43:35 +0300, Eli Zaretskii wrote:
> > Date: Tue, 29 Sep 2020 12:13:17 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: acm@muc.de

> > In the Emacs master branch
> > (i) emacs -Q
> > (ii) C-x C-f emacs/src/syntax.c<CR>
> > (iii) C-M-s \(inc\|dec\)_both
> > (iv) Press C-s a few times.

> > Notice that the highlighting of the found match has the purple
> > background over the correct characters, but the characters displayed in
> > the foreground are "   _both".  This incorrect display is a bug.

> Here, I don't see any incorrect display, I see a new feature at work.
> If you don't like it, set search-highlight-submatches to nil.

Ah.  I wasn't aware of this new feature.

> How many colors do you have on the terminal where you see the problem?

16.  The problem was that the submatch highlight was light magenta on
dark magenta, which I just didn't see.  Maybe the colours used for
isearch-group-1 on a 16 colour terminal are suboptimal.  They certainly
don't work well on my Linux virtual tty.

However, on pressing backspace to go back to previous matches, this new
highlighting is no longer there.

So, perhaps there is/are (a) bug(s) here after all, just not the one I
thought I'd found.

Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Eli Zaretskii]

> Date: Tue, 29 Sep 2020 15:33:55 +0000
> Cc: 43702@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > How many colors do you have on the terminal where you see the problem?
> 
> 16.  The problem was that the submatch highlight was light magenta on
> dark magenta, which I just didn't see.  Maybe the colours used for
> isearch-group-1 on a 16 colour terminal are suboptimal.  They certainly
> don't work well on my Linux virtual tty.

I think on low-color-number TTYs the colors for all the subgroups
should be identical, thus effectively disabling the feature.  We don't
have enough colors to show the differences on such terminals.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> 16.  The problem was that the submatch highlight was light magenta on
> dark magenta, which I just didn't see.  Maybe the colours used for
> isearch-group-1 on a 16 colour terminal are suboptimal.  They certainly
> don't work well on my Linux virtual tty.

I've now changed the face specs to inherit from the isearch face if
there's fewer than 88 colours.

> However, on pressing backspace to go back to previous matches, this new
> highlighting is no longer there.
>
> So, perhaps there is/are (a) bug(s) here after all, just not the one I
> thought I'd found.

Yup; I can reproduce that bug.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> 16.  The problem was that the submatch highlight was light magenta on
>> dark magenta, which I just didn't see.  Maybe the colours used for
>> isearch-group-1 on a 16 colour terminal are suboptimal.  They certainly
>> don't work well on my Linux virtual tty.
>
> I've now changed the face specs to inherit from the isearch face if
> there's fewer than 88 colours.

There is another problem: currently isearch-group-3 is indistinguishable
visually from the default isearch face (on X with more than 88 colours).

Also I don't understand why users would need so many faces (9!)

Maybe better to do what Drew proposed: to distinguish the odd groups
from the even groups, i.e. to have only 2 additional faces
(a brighter face like the current isearch-group-1 for the odd groups,
and a darker face for the even groups).

>> However, on pressing backspace to go back to previous matches, this new
>> highlighting is no longer there.
>>
>> So, perhaps there is/are (a) bug(s) here after all, just not the one I
>> thought I'd found.
>
> Yup; I can reproduce that bug.

The problem is that isearch-delete-char doesn't run the search again
that would set match-data.  It just restores an old position
(isearch-other-end and point) that isearch-highlight uses.

One solution is to call isearch-search in isearch-delete-char
before isearch-update.  But I can't predict all dire consequences.

So a better solution is maybe to save match-data on the isearch stack
and restore in isearch-pop-state (called by isearch-delete-char).
I could try to do this.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> Also I don't understand why users would need so many faces (9!)
> 
> Maybe better to do what Drew proposed: to distinguish the odd groups
> from the even groups, i.e. to have only 2 additional faces
> (a brighter face like the current isearch-group-1 for the odd groups,
> and a darker face for the even groups).

To be clear, I didn't propose that.  My code has 8 levels
(groups), with 8 faces.

What I mentioned about even and odd is for lazy-highlighting.

I use a different face, `isearchp-lazy-odd-regexp-groups',
for odd subgroups - for lazy-highlighting only.

(And that face is used, like the group highlighting for the
current search hit, only when option
`isearchp-highlight-regexp-group-levels-flag' is non-nil.)

I provide 8 levels/groups for the current search hit.
I agree that most regexp searches don't use anywhere near
that many groups.  But it costs nothing to provide for them.

Here's a screenshot with 5 levels shown:

https://www.emacswiki.org/emacs/Icicles_-_Search_Commands%2c_Overview#SearchHighlightingContextLevels

(That's an Icicles-search screenshot, but the effect is the
same.  I added this regexp-group highlighting to Icicles
nine years before I got around to adding it to Isearch+.)

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> There is another problem: currently isearch-group-3 is indistinguishable
> visually from the default isearch face (on X with more than 88 colours).
>
> Also I don't understand why users would need so many faces (9!)

Yeah, it's pretty excessive...  but consistent.

> Maybe better to do what Drew proposed: to distinguish the odd groups
> from the even groups, i.e. to have only 2 additional faces
> (a brighter face like the current isearch-group-1 for the odd groups,
> and a darker face for the even groups).

I like the idea.

> So a better solution is maybe to save match-data on the isearch stack
> and restore in isearch-pop-state (called by isearch-delete-char).
> I could try to do this.

Please do.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> Maybe better to do what Drew proposed: to distinguish the odd groups
>> from the even groups, i.e. to have only 2 additional faces
>> (a brighter face like the current isearch-group-1 for the odd groups,
>> and a darker face for the even groups).
>
> To be clear, I didn't propose that.  My code has 8 levels
> (groups), with 8 faces.

My intention was to give credit where credit is due.
Thank you for the idea.

> What I mentioned about even and odd is for lazy-highlighting.

Both ordinary and lazy-highlighting matches could be based on
the same idea of odd/even faces.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

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

>> Maybe better to do what Drew proposed: to distinguish the odd groups
>> from the even groups, i.e. to have only 2 additional faces
>> (a brighter face like the current isearch-group-1 for the odd groups,
>> and a darker face for the even groups).
>
> I like the idea.

Implemented now on the trunk where isearch-group-odd is created
from the isearch-group-1 face, and isearch-group-even
from the isearch-group-4 face.

The result is quite nice:


[-- Attachment #2: search-highlight-submatches.png --]
[-- Type: image/png, Size: 14834 bytes --]

[-- Attachment #3: Type: text/plain, Size: 304 bytes --]


>> So a better solution is maybe to save match-data on the isearch stack
>> and restore in isearch-pop-state (called by isearch-delete-char).
>> I could try to do this.
>
> Please do.

I'm on it (also adding even/odd faces to lazy-isearch - optionally
on the new user option lazy-highlight-submatches).

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Implemented now on the trunk where isearch-group-odd is created
> from the isearch-group-1 face, and isearch-group-even
> from the isearch-group-4 face.
>
> The result is quite nice:

Yup; looks good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 01 Oct 2020 22:20:07 +0300
> Cc: Alan Mackenzie <acm@muc.de>, 43702@debbugs.gnu.org
> 
> >> Maybe better to do what Drew proposed: to distinguish the odd groups
> >> from the even groups, i.e. to have only 2 additional faces
> >> (a brighter face like the current isearch-group-1 for the odd groups,
> >> and a darker face for the even groups).
> >
> > I like the idea.
> 
> Implemented now on the trunk where isearch-group-odd is created
> from the isearch-group-1 face, and isearch-group-even
> from the isearch-group-4 face.
> 
> The result is quite nice:

Nice is in the eyes of the beholder: I liked the previous way better.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> >> Maybe better to do what Drew proposed: to distinguish the odd groups
> >> from the even groups, i.e. to have only 2 additional faces
> >> (a brighter face like the current isearch-group-1 for the odd groups,
> >> and a darker face for the even groups).
> >
> > To be clear, I didn't propose that.  My code has 8 levels
> > (groups), with 8 faces.
> 
> My intention was to give credit where credit is due.
> Thank you for the idea.
> 
> > What I mentioned about even and odd is for lazy-highlighting.
> 
> Both ordinary and lazy-highlighting matches could be based on
> the same idea of odd/even faces.

They could.  But they shouldn't.

For the current search hit, it makes sense to provide
as much info as possible visually.  That is, in this
context (regexp matching), highlight each of the groups
in a unique way, so they can be easily identified in
the search hit.

The idea behind even/odd lazy-highlighting is to just
provide a rough idea of some of the matching.  The
current search hit should show accurate, complete info
about the match.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> >> Maybe better to do what Drew proposed: to distinguish the odd groups
> >> from the even groups, i.e. to have only 2 additional faces
> >> (a brighter face like the current isearch-group-1 for the odd groups,
> >> and a darker face for the even groups).
> >
> > I like the idea.
> 
> Implemented now on the trunk where isearch-group-odd is created
> from the isearch-group-1 face, and isearch-group-even
> from the isearch-group-4 face.
> 
> The result is quite nice:

Sorry, but IMO that's horrible.  And you really should
show more regexps, not just a sequence of groups, but
nested groups etc.

Regexp matching is complex.  And that's the point:
WYSIWYG visualization of a user's regexp on the fly.

My suggestion: provide a fair number of group faces,
to make it easy to identify which parts of a regexp
correspond to which parts of the current search hit.

This should be simple.  I've done it.  And used it,
for a long time.  Why reinvent the wheel, replacing
a circle with an octagon?

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

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

FWIW, attached are three screenshots using Isearch+
for the same regexp search as Juri's screenshot, and
starting with emacs -Q (just loading isearch+.el).

I can't tell from Juri's screenshot just what regexp
he used.  Seven regexp groups are shown in the echo
area, but the non-lazy highlighting seems to show 13
groups.

The first two attached screenshots are with 7 and 13
groups, respectively.  They show the default
highlighting for both the current search hit and the
other search hits (lazy highlighting).

I think you can see the advantage of having lazy
highlighting indicate groups, at least in a gross
way (even/odd).

Because Isearch+ has only 8 group faces defined,
matches beyond the 8th are not indicated in the
current search hit, in these first two screenshots.

As I mentioned, it's simple to just repeat the use
of the same 8 faces, for 9-16, 17-24, etc.  But
that won't help you distinguish group 1 from group
9 etc.

I've added a third screenshot, where I did this:
recycle the faces of groups 1-5 for groups 9-13.
___

The faces for the overall current search hit and
the even lazy-highlight hits are the vanilla
default faces, `isearch' and `lazy-highlight'.

The default values of the group faces are also
used.  By default, only the background color is
defined for each group face.

The foreground for each group is apparently taken
from the foreground of face `isearch'.  I guess I
hadn't paid attention to that.  I think that's
unfortunate, in general.  I probably didn't notice
it because in my own setup I've customized face
`isearch' to use a black foreground (and a green
background).

I think the default foreground for face `isearch'
should be something that works well with each of
the default group faces, and that's not likely to
be `lightskyblue1'.  Change it to `black' and the
regexp group highlighting is quite good.  You need
to be able to see the text that the regexp parts
apply to.

I also tried just removing a foreground spec from
the vanilla default for face `isearch'.  In the
case shown, which is *scratch*, the text has face
`font-lock-comment-face'.  And if face `isearch'
doesn't have a good foreground spec (e.g. `black')
then the result isn't great.  (But at least it's
better than with foreground `lightskyblue1'.)

As I've just noticed this problem now, and as I
don't want to impose any default change on face
`isearch' in my code, I've now changed the default
regexp group faces to each specify a `black'
foreground (instead of no foreground), for a light
background mode.

The third screenshot (which recycles faces 1-5 for
9-13) shows this: a black foreground for each face
used for the current search hit (but not for lazy
highlighting).
___

The regexp used by Juri is extremely atypical, IMO.

A more typical regexp that uses multiple groups
involves some group nesting, and in that case there's
little (typically no) problem distinguishing group 1
from group 9, group 2 from group 10, etc.

[-- Attachment #2: throw-emacs-isearch+-regexp-highlighting.png --]
[-- Type: image/png, Size: 27278 bytes --]

[-- Attachment #3: throw-emacs-isearch+-regexp-highlighting-2.png --]
[-- Type: image/png, Size: 26583 bytes --]

[-- Attachment #4: throw-emacs-isearch+-regexp-highlighting-3.png --]
[-- Type: image/png, Size: 25527 bytes --]

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

> Why reinvent the wheel, replacing a circle with an octagon?

I'm working on the patch that would satisfy everyone.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

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

> As I mentioned, it's simple to just repeat the use
> of the same 8 faces, for 9-16, 17-24, etc.

This is a very good idea.  Now this is implemented in isearch.

> I've added a third screenshot, where I did this:
> recycle the faces of groups 1-5 for groups 9-13.

Yes, recycling is nice, this is what now is used in isearch.

> The foreground for each group is apparently taken
> from the foreground of face `isearch'.  I guess I
> hadn't paid attention to that.  I think that's
> unfortunate, in general.  I probably didn't notice
> it because in my own setup I've customized face
> `isearch' to use a black foreground (and a green
> background).
>
> I think the default foreground for face `isearch'
> should be something that works well with each of
> the default group faces, and that's not likely to
> be `lightskyblue1'.  Change it to `black' and the
> regexp group highlighting is quite good.  You need
> to be able to see the text that the regexp parts
> apply to.
>
> I also tried just removing a foreground spec from
> the vanilla default for face `isearch'.  In the
> case shown, which is *scratch*, the text has face
> `font-lock-comment-face'.  And if face `isearch'
> doesn't have a good foreground spec (e.g. `black')
> then the result isn't great.  (But at least it's
> better than with foreground `lightskyblue1'.)
>
> As I've just noticed this problem now, and as I
> don't want to impose any default change on face
> `isearch' in my code, I've now changed the default
> regexp group faces to each specify a `black'
> foreground (instead of no foreground), for a light
> background mode.

You see?  So many problems when you tried to design
a color palette for many isearch faces.

I suppose you agree that the previous color palette in isearch
with a color gradient is not suitable for highlighting of group matches
because while looking at highlighted matches it's quite impossible to
say which color corresponds to which group number.
I meant when colors looks like a heatmap, for example:


[-- Attachment #2: heatmap.png --]
[-- Type: image/png, Size: 58714 bytes --]

[-- Attachment #3: Type: text/plain, Size: 282 bytes --]


would anyone be able to say a number while looking at a color?

I see that you are using more distinct colors,
but still these colors are bleak.

Other examples of color scheme are in re-builder and hi-lock faces.

Personally, I'd like to use rainbow colors in own customization:


[-- Attachment #4: rainbow.png --]
[-- Type: image/png, Size: 18239 bytes --]

[-- Attachment #5: Type: text/plain, Size: 554 bytes --]


But frankly speaking such color palette looks like angry fruit salad.

What is the most reasonable thing to do is to delegate the decision
about the color scheme selection to theme authors and to user customization.

So now isearch provides two distinct recycling colors based on the default
isearch color scheme.

> The third screenshot (which recycles faces 1-5 for
> 9-13) shows this: a black foreground for each face
> used for the current search hit (but not for lazy
> highlighting).

Recycling is now implemented in isearch.
Thanks for the idea.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>>> So a better solution is maybe to save match-data on the isearch stack
>>> and restore in isearch-pop-state (called by isearch-delete-char).
>>> I could try to do this.
>>
>> Please do.

This is fixed now.

> I'm on it (also adding even/odd faces to lazy-isearch - optionally
> on the new user option lazy-highlight-submatches).

I started implementing group matches highlighting for lazy-highlight and also
for query-replace, but then realized neither lazy-highlight nor query-replace
need no such thing because this highlighting is only useful while
incrementally constructing/inspecting a complex regexp for the current match
in incremental isearch mode.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

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

> > As I mentioned, it's simple to just repeat the use
> > of the same 8 faces, for 9-16, 17-24, etc.
> 
> This is a very good idea.  Now this is implemented in isearch.
>
> > I've added a third screenshot, where I did this:
> > recycle the faces of groups 1-5 for groups 9-13.
> 
> Yes, recycling is nice, this is what now is used in isearch.

Good to hear.

> You see?  So many problems when you tried to design
> a color palette for many isearch faces.

Did you say that before & I disagreed?  I don't think so.

(What I mentioned in my previous mail here was that I
didn't take into account that the foreground of face
`isearch' got used for all of the regexp-group faces,
because their default values didn't specify a foreground.)

> I suppose you agree that the previous color palette in
> isearch with a color gradient

I don't know what palette you're referring to, sorry.

> is not suitable for highlighting of group matches
> because while looking at highlighted matches it's quite impossible to
> say which color corresponds to which group number.
> I meant when colors looks like a heatmap, for example:

I'm not sure what to make of your heat-map image.
Is it supposed to show highlighting of regexps?
Certainly there are many, many fine gradations of
color in that image.

If your point is that it's hard to distinguish very
similar colors (e.g. your "-0.69" and "0.12"), then
yes, of course.

Your rainbow screenshot presumably is for matching
sequential regexp groups (again, not very typical).

It's OK, but it suffers from using a set of faces
with light and dark backgrounds.  The black foreground
is good against some of the backgrounds and bad against
others.

See attached, which are the faces I use by default
(for a light background mode).

The series runs first (groups 1-4) through different
pastel hues at the same saturation & value, and then
(groups 5-8) through the same series of hues but with
a lighter saturation & value (i.e., more pastel).

I think that helps, when trying to match regexp group
highlighting against group numbers.  There's only a
slight difference between the colors for the same hue
(different degrees of lightness), but it's pretty easy
to tell faces of different hue from each other.

I.e., it's easy to tell group 3 from groups 1, 2, and
4, and it's easy to tell 7 from 5, 6, and 7.  It's
harder to tell 3 from 7, but needing to do that will
be relatively rare.

> I started implementing group matches highlighting for lazy-highlight and also
> for query-replace, but then realized neither lazy-highlight nor query-replace
> need no such thing because this highlighting is only useful while
> incrementally constructing/inspecting a complex regexp for the current match
> in incremental isearch mode.

I don't agree, but won't argue about it.  I redefined
`replace-highlight' and `replace-dehighlight' so they
do highlight regexp groups, and I think it's helpful.

The same user option controls whether regexp groups
should be highlighted, for Isearch and replace:
`isearchp-highlight-regexp-group-levels-flag'.  And
you can toggle that during Isearch with `M-s h R'.

Anyway, thanks for working on this feature.  I think
it's helpful and will be welcome.

[-- Attachment #2: throw-isearch+-regexp-group-faces.png --]
[-- Type: image/png, Size: 72015 bytes --]

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 06 Oct 2020 23:01:16 +0300
> Cc: Alan Mackenzie <acm@muc.de>, Lars Ingebrigtsen <larsi@gnus.org>,
>  43702@debbugs.gnu.org
> 
> > As I mentioned, it's simple to just repeat the use
> > of the same 8 faces, for 9-16, 17-24, etc.
> 
> This is a very good idea.  Now this is implemented in isearch.

I don't think I understand the change, and the documentation doesn't
seem to help.  It seems like now there are just two faces that are
used alternately, i.e. one face for odd-numbered groups, the other for
even-numbered groups?  If this is the case, why doesn't the
documentation say so explicitly?  And if this is not the case, what
did I miss?

> I suppose you agree that the previous color palette in isearch
> with a color gradient is not suitable for highlighting of group matches
> because while looking at highlighted matches it's quite impossible to
> say which color corresponds to which group number.

And with the current implementation on master it is possible?  If so,
how?

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> > As I mentioned, it's simple to just repeat the use
>> > of the same 8 faces, for 9-16, 17-24, etc.
>>
>> This is a very good idea.  Now this is implemented in isearch.
>
> I don't think I understand the change, and the documentation doesn't
> seem to help.  It seems like now there are just two faces that are
> used alternately, i.e. one face for odd-numbered groups, the other for
> even-numbered groups?  If this is the case, why doesn't the
> documentation say so explicitly?  And if this is not the case, what
> did I miss?

Yes, by default isearch-group-1 is used for odd-numbered groups,
and isearch-group-2 for even-numbered groups.

But when a user or a theme author will define a new face
isearch-group-3, then isearch-group-3 will be used to highlight
groups 3, 6, 9, 12, ..., the isearch-group-2 face to highlight
groups 2, 5, 8, 11, ..., the isearch-group-1 face to highlight
groups 1, 4, 7, 10, ...

A user or a theme author can define more faces, e.g.
isearch-group-4, isearch-group-5, isearch-group-6,
and these faces will be recycled:

submatches:  1 2 3 4 5 6 7 8 9 10 11 12
group faces: 1 2 3 4 5 6 1 2 3  4  5  6

>> I suppose you agree that the previous color palette in isearch
>> with a color gradient is not suitable for highlighting of group matches
>> because while looking at highlighted matches it's quite impossible to
>> say which color corresponds to which group number.
>
> And with the current implementation on master it is possible?  If so,
> how?

With only 2 default group faces it's possible to see that one of them
isearch-group-1 is brighter than the default isearch face, and another
isearch-group-2 is darker than the default isearch face.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

> See attached, which are the faces I use by default
> (for a light background mode).
>
> It's harder to tell 3 from 7, but needing to do that will be
> relatively rare.

I agree, it's hard to tell 3 from 7 when highlighting a complex nested regexp.

>> I started implementing group matches highlighting for lazy-highlight and also
>> for query-replace, but then realized neither lazy-highlight nor query-replace
>> need no such thing because this highlighting is only useful while
>> incrementally constructing/inspecting a complex regexp for the current match
>> in incremental isearch mode.
>
> I don't agree, but won't argue about it.  I redefined
> `replace-highlight' and `replace-dehighlight' so they
> do highlight regexp groups, and I think it's helpful.

After query-replace is started, the regexp is already created
(either with the help of isearch group submatches highlighting
or typed directly on the query-replace prompt), so during replacements
the user has only to decide for each match whether to replace it or not
by answering the y/n question for the currently highlighted regexp.

How highlighting the replacement submatches will help the user to decide
whether to replace the current match or not?

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: drew.adams@oracle.com,  acm@muc.de,  larsi@gnus.org,  43702@debbugs.gnu.org
> Date: Wed, 07 Oct 2020 11:09:09 +0300
> 
> > I don't think I understand the change, and the documentation doesn't
> > seem to help.  It seems like now there are just two faces that are
> > used alternately, i.e. one face for odd-numbered groups, the other for
> > even-numbered groups?  If this is the case, why doesn't the
> > documentation say so explicitly?  And if this is not the case, what
> > did I miss?
> 
> Yes, by default isearch-group-1 is used for odd-numbered groups,
> and isearch-group-2 for even-numbered groups.
> 
> But when a user or a theme author will define a new face
> isearch-group-3, then isearch-group-3 will be used to highlight
> groups 3, 6, 9, 12, ..., the isearch-group-2 face to highlight
> groups 2, 5, 8, 11, ..., the isearch-group-1 face to highlight
> groups 1, 4, 7, 10, ...

OK, I clarified this in the docs.

> >> I suppose you agree that the previous color palette in isearch
> >> with a color gradient is not suitable for highlighting of group matches
> >> because while looking at highlighted matches it's quite impossible to
> >> say which color corresponds to which group number.
> >
> > And with the current implementation on master it is possible?  If so,
> > how?
> 
> With only 2 default group faces it's possible to see that one of them
> isearch-group-1 is brighter than the default isearch face, and another
> isearch-group-2 is darker than the default isearch face.

No, I meant to ask if it's possible to say which color corresponds to
which group number.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> > I don't agree, but won't argue about it.  I redefined
> > `replace-highlight' and `replace-dehighlight' so they
> > do highlight regexp groups, and I think it's helpful.
> 
> After query-replace is started, the regexp is already created
> (either with the help of isearch group submatches highlighting
> or typed directly on the query-replace prompt), so during replacements
> the user has only to decide for each match whether to replace it or not
> by answering the y/n question for the currently highlighted regexp.
> 
> How highlighting the replacement submatches will help the user to decide
> whether to replace the current match or not?

Whether to replace a given match is not the only thing
a user can do - not the only thing to decide/consider.

Highlighting groups helps you see _how_ a regexp matches.

And you can always change a regexp and search/replace
again, if it's either not matching something you want
to match or matching too many things you don't want to
match.

It's helpful in pretty much any context where regexps
are matched against text, IMO.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> >> I suppose you agree that the previous color palette in isearch
>> >> with a color gradient is not suitable for highlighting of group matches
>> >> because while looking at highlighted matches it's quite impossible to
>> >> say which color corresponds to which group number.
>> >
>> > And with the current implementation on master it is possible?  If so,
>> > how?
>>
>> With only 2 default group faces it's possible to see that one of them
>> isearch-group-1 is brighter than the default isearch face, and another
>> isearch-group-2 is darker than the default isearch face.
>
> No, I meant to ask if it's possible to say which color corresponds to
> which group number.

The default two colors work well for simple regexps,
but degrade with increase of regexp complexity.
And defining more colors with same color gradations
doesn't help.  Even the color scheme designed by Drew
has problems.  So users and theme authors need to decide
themselves how many and what colors they want to use.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> Even the color scheme designed by Drew has problems.

What problems?

> So users and theme authors need to decide themselves
> how many and what colors they want to use.

I certainly agree with that.

It's sufficient to define a set of faces (I suggested
eight) and use those.

Anyone who want the effect of fewer faces can just
customize any of them to repeat any of the others.

It's easy to get the effect of using only 2 faces
by customizing the eight faces provided.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> Even the color scheme designed by Drew has problems.
>
> What problems?

On the screenshot you sent it's hard to tell 3 from 7,
1 from 5, 2 from 6, 4 from 8.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> >> Even the color scheme designed by Drew has problems.
> >
> > What problems?
> 
> On the screenshot you sent it's hard to tell 3 from 7,
> 1 from 5, 2 from 6, 4 from 8.

It's intentional.  I explained why it's not a
problem in practice.  Few regexps use more than
4 groups.

1. Groups near each other, in group order, are
   easily distinguishable, regardless of where
   they are in the list.  E.g., (2,3,4), (4,5 6),
   or (8,1,2).

2. Most important is distinguishing groups 1-4,
   without using faces that are too in-your-face
   or conflict with most other highlighting.

3. Hue, at the same saturation & value, is the
   best way to distinguish without violating #2.

4. Groups 5-8 are paler and more rarely used.
   Their differences need not be as pronounced
   as 1-4.

But as you say, being able to customize is what's
most important.

And by doing so, to, in effect, shrink the number of
faces in the cycle (by using identical definitions).

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

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

>> How highlighting the replacement submatches will help the user to decide
>> whether to replace the current match or not?
>
> Whether to replace a given match is not the only thing
> a user can do - not the only thing to decide/consider.
>
> Highlighting groups helps you see _how_ a regexp matches.
>
> And you can always change a regexp and search/replace
> again, if it's either not matching something you want
> to match or matching too many things you don't want to
> match.
>
> It's helpful in pretty much any context where regexps
> are matched against text, IMO.

I think you are right.  So here is a patch that implements this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: query-replace-highlight-submatches.patch --]
[-- Type: text/x-diff, Size: 3756 bytes --]

diff --git a/lisp/replace.el b/lisp/replace.el
index e363924501..f9ffa36f54 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -126,6 +126,18 @@ query-replace-highlight
   :type 'boolean
   :group 'matching)
 
+(defcustom query-replace-highlight-submatches t
+  "Whether to highlight regexp subexpressions during query replacement.
+The faces used to do the highlights are named `isearch-group-1',
+`isearch-group-2', etc.  (By default, only these 2 are defined.)
+When there are more matches than faces, then faces are reused from the
+beginning, in a cyclical manner, so the `isearch-group-1' face is
+isreused for the third match.  If you want to use more distinctive colors,
+you can define more of these faces using the same numbering scheme."
+  :type 'boolean
+  :group 'matching
+  :version "28.1")
+
 (defcustom query-replace-lazy-highlight t
   "Controls the lazy-highlighting during query replacements.
 When non-nil, all text in the buffer matching the current match
@@ -2403,16 +2427,36 @@ replace-search
     (funcall search-function search-string limit t)))
 
 (defvar replace-overlay nil)
+(defvar replace-submatches-overlays nil)
 
 (defun replace-highlight (match-beg match-end range-beg range-end
 			  search-string regexp-flag delimited-flag
-			  case-fold &optional backward)
+			  case-fold &optional backward match-data)
   (if query-replace-highlight
       (if replace-overlay
 	  (move-overlay replace-overlay match-beg match-end (current-buffer))
 	(setq replace-overlay (make-overlay match-beg match-end))
 	(overlay-put replace-overlay 'priority 1001) ;higher than lazy overlays
 	(overlay-put replace-overlay 'face 'query-replace)))
+
+  (when (and query-replace-highlight-submatches
+	     regexp-flag)
+    (mapc 'delete-overlay replace-submatches-overlays)
+    (setq replace-submatches-overlays nil)
+    (let ((submatch-data (cddr (butlast match-data)))
+          (group 0)
+          ov face)
+      (while submatch-data
+        (setq group (1+ group))
+        (setq ov (make-overlay (pop submatch-data) (pop submatch-data))
+              face (intern-soft (format "isearch-group-%d" group)))
+        ;; Recycle faces from beginning.
+        (unless (facep face)
+          (setq group 1 face 'isearch-group-1))
+        (overlay-put ov 'face face)
+        (overlay-put ov 'priority 1002)
+        (push ov replace-submatches-overlays))))
+
   (if query-replace-lazy-highlight
       (let ((isearch-string search-string)
 	    (isearch-regexp regexp-flag)
@@ -2433,6 +2477,9 @@ replace-highlight
 (defun replace-dehighlight ()
   (when replace-overlay
     (delete-overlay replace-overlay))
+  (when query-replace-highlight-submatches
+    (mapc 'delete-overlay replace-submatches-overlays)
+    (setq replace-submatches-overlays nil))
   (when query-replace-lazy-highlight
     (lazy-highlight-cleanup lazy-highlight-cleanup)
     (setq isearch-lazy-highlight-last-string nil))
@@ -2694,7 +2741,7 @@ perform-replace
 		    (replace-highlight
 		     (nth 0 real-match-data) (nth 1 real-match-data)
 		     start end search-string
-		     regexp-flag delimited-flag case-fold-search backward))
+		     regexp-flag delimited-flag case-fold-search backward real-match-data))
 		  (setq noedit
 			(replace-match-maybe-edit
 			 next-replacement nocasify literal
@@ -2719,7 +2766,7 @@ perform-replace
                   (replace-highlight
 		   (match-beginning 0) (match-end 0)
 		   start end search-string
-		   regexp-flag delimited-flag case-fold-search backward)
+		   regexp-flag delimited-flag case-fold-search backward real-match-data)
                   ;; Obtain the matched groups: needed only when
                   ;; regexp-flag non nil.
                   (when (and last-was-undo regexp-flag)

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> > It's helpful in pretty much any context where regexps
> > are matched against text, IMO.
> 
> I think you are right.  So here is a patch that implements this:

It's probably not my place to criticize your patch.
But in case it helps:

I don't understand why you change the signature of
`replace-highlight' by adding parameter MATCH-DATA.

I just use (match-beginning GROUP) and (match-end GROUP),
instead of explicitly passing the match data and then
copying it all with `butlast' etc. just to find the
group's match beginning and end.

https://www.emacswiki.org/emacs/download/replace%2b.el

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

>> > It's helpful in pretty much any context where regexps
>> > are matched against text, IMO.
>>
>> I think you are right.  So here is a patch that implements this:
>
> It's probably not my place to criticize your patch.
> But in case it helps:
>
> I don't understand why you change the signature of
> `replace-highlight' by adding parameter MATCH-DATA.
>
> I just use (match-beginning GROUP) and (match-end GROUP),
> instead of explicitly passing the match data and then
> copying it all with `butlast' etc. just to find the
> group's match beginning and end.

But as you can see in the patch, it doesn't use (match-data).
It uses the variable 'real-match-data'.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> > I don't understand why you change the signature of
> > `replace-highlight' by adding parameter MATCH-DATA.
> >
> > I just use (match-beginning GROUP) and (match-end GROUP),
> > instead of explicitly passing the match data and then
> > copying it all with `butlast' etc. just to find the
> > group's match beginning and end.
> 
> But as you can see in the patch, it doesn't use (match-data).
> It uses the variable 'real-match-data'.

Yes, I saw that.  I suppose it must be
relevant, but it's not clear to me why.

I haven't noticed any problem with just using
the latest match data, but I haven't tested much.

It would be good to add a code comment to cite a
case or specify the cases where it matters.  Thx.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Juri Linkov]

tags 43702 fixed
close 43702 28.0.50
thanks

> I haven't noticed any problem with just using
> the latest match data, but I haven't tested much.

You are right - there are no problems with using the latest match data.
But using real-match-data causes problems - sometimes real-match-data
contains nils at the end of the list, e.g. (1 2 3 4 <*buffer*> nil nil nil nil)

So I pushed the patch that uses the latest match data.

bug#43702: Emacs master: Incorrect highlighting in regexp isearch.

[Drew Adams]

> tags 43702 fixed
> close 43702 28.0.50
> thanks
> 
> > I haven't noticed any problem with just using
> > the latest match data, but I haven't tested much.
> 
> You are right - there are no problems with using the latest match data.
> But using real-match-data causes problems - sometimes real-match-data
> contains nils at the end of the list, e.g. (1 2 3 4 <*buffer*> nil nil
> nil nil)
> 
> So I pushed the patch that uses the latest match data.

Good to know.  I wasn't sure - I was on the verge of
thinking there must be some reason why we need to pass
`real-match-data' here.