bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Pip Cet]

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

...and other things. It's probably a case of "if it hurts, don't do that".

There seems to be a general problem using such regexps in the
composition-function-table.

If I evaluate this in emacs -Q (by placing point after it and hitting C-x C-e)

(custom-set-faces
 '(default ((t (:family "Libertinus Serif" :height 330)))))
(set-char-table-range composition-function-table t '([".+" 0
font-shape-gstring]))

the font correctly changes to a very large Libertinus font. I then hit
C-b C-d ) and the entire last line is highlighted, not just the
opening parenthesis. After the blink delay is over, the opening
parenthesis and the "s" following it are unhighlighted, but the rest
of the line is not. It stays like that permanently (screenshot
attached).

[-- Attachment #2: paren.png --]
[-- Type: image/png, Size: 16755 bytes --]

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 22 May 2020 12:50:13 +0000
> 
> There seems to be a general problem using such regexps in the
> composition-function-table.

Could be, because it's a very unusual thing to do.

> If I evaluate this in emacs -Q (by placing point after it and hitting C-x C-e)
> 
> (custom-set-faces
>  '(default ((t (:family "Libertinus Serif" :height 330)))))
> (set-char-table-range composition-function-table t '([".+" 0
> font-shape-gstring]))
> 
> the font correctly changes to a very large Libertinus font. I then hit
> C-b C-d ) and the entire last line is highlighted, not just the
> opening parenthesis. After the blink delay is over, the opening
> parenthesis and the "s" following it are unhighlighted, but the rest
> of the line is not. It stays like that permanently (screenshot
> attached).

I think this is expected, since you told Emacs all those characters
are a single grapheme cluster.

The regexps in composition-function-table should match only characters
that are supposed to be composed.

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Pip Cet]

On Fri, May 22, 2020 at 1:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 22 May 2020 12:50:13 +0000
> >
> > There seems to be a general problem using such regexps in the
> > composition-function-table.
>
> Could be, because it's a very unusual thing to do.

I also see the problem when I change the pattern to "(a)" and enter
"(a)", so it's not a problem with variable-length regexps.

> I think this is expected, since you told Emacs all those characters
> are a single grapheme cluster.

I don't think it's expected for characters to stay highlighted after
the blink delay is over.

> The regexps in composition-function-table should match only characters
> that are supposed to be composed.

So it's invalid to have a regexp for a composition not supported by
the current font?

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 22 May 2020 13:18:09 +0000
> Cc: 41454@debbugs.gnu.org
> 
> On Fri, May 22, 2020 at 1:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > From: Pip Cet <pipcet@gmail.com>
> > > Date: Fri, 22 May 2020 12:50:13 +0000
> > >
> > > There seems to be a general problem using such regexps in the
> > > composition-function-table.
> >
> > Could be, because it's a very unusual thing to do.
> 
> I also see the problem when I change the pattern to "(a)" and enter
> "(a)", so it's not a problem with variable-length regexps.

I think I know why, but I will look into this as soon as I have time.

> > The regexps in composition-function-table should match only characters
> > that are supposed to be composed.
> 
> So it's invalid to have a regexp for a composition not supported by
> the current font?

No, it's not invalid.  Whether the font supports a ligature is not the
issue here (if the font doesn't support it, the characters will be
displayed as usual).  What is the issue is whether we at all want the
sequence to be displayed as a ligature and behave as one, whether the
font does or doesn't support it.

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 22 May 2020 12:50:13 +0000
> 
> There seems to be a general problem using such regexps in the
> composition-function-table.
> 
> If I evaluate this in emacs -Q (by placing point after it and hitting C-x C-e)
> 
> (custom-set-faces
>  '(default ((t (:family "Libertinus Serif" :height 330)))))
> (set-char-table-range composition-function-table t '([".+" 0
> font-shape-gstring]))
> 
> the font correctly changes to a very large Libertinus font. I then hit
> C-b C-d ) and the entire last line is highlighted, not just the
> opening parenthesis. After the blink delay is over, the opening
> parenthesis and the "s" following it are unhighlighted, but the rest
> of the line is not. It stays like that permanently (screenshot
> attached).

This is unrelated to the fact that characters to be composed are
specified with regexps.  The problem is that the entire sequence of
characters that matches the regexp is passed to the shaper, and the
result is stored as a single composition.  And we have special code in
the display engine not to break sequences of potentially-composed
characters when handling faces (see compute_stop_pos).  So I think
there might be some bug there which shows when a composition comprises
more than a single grapheme cluster, and some face is applied to them.
It could even be that this is a side effect of recent changes in that
area of the display engine that fixed much more serious problems with
compositions (see bug#28312 and commit c42c4e9c5 which fixed it).  I
couldn't verify that this problem doesn't appear in Emacs 26 or older,
because Emacs only recently learned not to infloop or crash when
attempting to compose pure-ASCII text.

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Eli Zaretskii]

> Date: Sun, 31 May 2020 20:50:07 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 41454@debbugs.gnu.org
> 
> The problem is that the entire sequence of characters that matches
> the regexp is passed to the shaper, and the result is stored as a
> single composition.  And we have special code in the display engine
> not to break sequences of potentially-composed characters when
> handling faces (see compute_stop_pos).  So I think there might be
> some bug there which shows when a composition comprises more than a
> single grapheme cluster, and some face is applied to them.

On second thought, I'm not sure this is the right place to look at.
It could be fill_gstring_glyph_string and BUILD_GSTRING_GLYPH_STRING
instead (and the respective *term.c display functions).

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Pip Cet]

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

On Sun, May 31, 2020 at 6:01 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Sun, 31 May 2020 20:50:07 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 41454@debbugs.gnu.org
> >
> > The problem is that the entire sequence of characters that matches
> > the regexp is passed to the shaper, and the result is stored as a
> > single composition.  And we have special code in the display engine
> > not to break sequences of potentially-composed characters when
> > handling faces (see compute_stop_pos).  So I think there might be
> > some bug there which shows when a composition comprises more than a
> > single grapheme cluster, and some face is applied to them.
>
> On second thought, I'm not sure this is the right place to look at.
> It could be fill_gstring_glyph_string and BUILD_GSTRING_GLYPH_STRING
> instead (and the respective *term.c display functions).

Thanks for the hint! You were absolutely correct.

[-- Attachment #2: 0001-Don-t-get-confused-by-mid-gstring-face-changes-bug-4.patch --]
[-- Type: text/x-patch, Size: 789 bytes --]

From c1d4778001567f7dfc7825b69193089ec69897bb Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 31 May 2020 19:55:48 +0000
Subject: [PATCH] Don't get confused by mid-gstring face changes (bug#41454)

* src/xdisp.c (fill_gstring_glyph_string): Don't extend the glyph
string past face changes.
---
 src/xdisp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/xdisp.c b/src/xdisp.c
index db0ec68315..989958fa11 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -27698,6 +27698,7 @@ fill_gstring_glyph_string (struct glyph_string *s, int face_id,
   while (glyph < last
 	 && glyph->u.cmp.automatic
 	 && glyph->u.cmp.id == s->cmp_id
+	 && glyph->face_id == face_id
 	 && s->cmp_to == glyph->slice.cmp.from)
     {
       s->width += glyph->pixel_width;
-- 
2.27.0.rc0

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 31 May 2020 19:58:09 +0000
> Cc: 41454@debbugs.gnu.org
> 
> > On second thought, I'm not sure this is the right place to look at.
> > It could be fill_gstring_glyph_string and BUILD_GSTRING_GLYPH_STRING
> > instead (and the respective *term.c display functions).
> 
> Thanks for the hint! You were absolutely correct.
> 
> diff --git a/src/xdisp.c b/src/xdisp.c
> index db0ec68315..989958fa11 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -27698,6 +27698,7 @@ fill_gstring_glyph_string (struct glyph_string *s, int face_id,
>    while (glyph < last
>  	 && glyph->u.cmp.automatic
>  	 && glyph->u.cmp.id == s->cmp_id
> +	 && glyph->face_id == face_id
>  	 && s->cmp_to == glyph->slice.cmp.from)
>      {
>        s->width += glyph->pixel_width;

LGTM, thanks.

(Note that the sibling function fill_composite_glyph_string already
does that for static compositions.)

bug#41454: 28.0.50; [".+" 0 font-shape-gstring] composition rule breaks paren highlighting

[Pip Cet]

Eli Zaretskii <eliz@gnu.org> writes:

> LGTM, thanks.

Pushed, closing this bug.