Merging the underline attribute at EOL

Merging the underline attribute at EOL

[Eli Zaretskii]

Jimmy,

The current code in extend_face_to_end_of_line says:

  /* Face extension extends the background and box of IT->extend_face_id
     to the end of the line.  If the background equals the background
     of the frame, we don't have to do anything.  */
  struct face *face = FACE_FROM_ID (f, (it->face_before_selective_p
                                        ? it->saved_face_id
                                        : extend_face_id));

  if (FRAME_WINDOW_P (f)
      && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
      && face->box == FACE_NO_BOX
      && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
#ifdef HAVE_WINDOW_SYSTEM
      && !face->stipple
#endif
      && !it->glyph_row->reversed_p
      && !Vdisplay_fill_column_indicator)
    return;

This has the effect that the underline property is not extended past
EOL, and neither are overline and strike-through.  Only the box
attribute is extended.  Was this how we intended things to be, or is
this just an oversight?

Currently, this creates some strange counter-intuitive effects.  For
example, try this in "emacs -Q":

  C-p
  M-x font-lock-mode RET
  M-: (add-text-properties (point) (1+ (point)) '(face (:underline t :extend t))) RET

You will see that the underline is limited only to the newline at
point, it is not extended to the edge of the window.  But if you now
do this:

  C-SPC
  C-n

suddenly the entire last line is underlined, in addition to having the
background color from the region face.

If you replace the :underline with :box in the above example, then the
last line has the box attribute extended to EOL even before setting
the region, as expected.

So I think this is a bug, and we should add conditions to the above
'if' clause.  Am I missing something?

Re: Merging the underline attribute at EOL

[Ergus]

Hi Eli:

I just saw your message. Actually the problem should be somewhere else a
bit latter in the same function. Because in terminal your code underlies
the entire line. I will give it a look tomorrow.

Best
Ergus

On Sat, Dec 14, 2019 at 10:28:45AM +0200, Eli Zaretskii wrote:
>Jimmy,
>
>The current code in extend_face_to_end_of_line says:
>
>  /* Face extension extends the background and box of IT->extend_face_id
>     to the end of the line.  If the background equals the background
>     of the frame, we don't have to do anything.  */
>  struct face *face = FACE_FROM_ID (f, (it->face_before_selective_p
>                                        ? it->saved_face_id
>                                        : extend_face_id));
>
>  if (FRAME_WINDOW_P (f)
>      && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>      && face->box == FACE_NO_BOX
>      && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>#ifdef HAVE_WINDOW_SYSTEM
>      && !face->stipple
>#endif
>      && !it->glyph_row->reversed_p
>      && !Vdisplay_fill_column_indicator)
>    return;
>
>This has the effect that the underline property is not extended past
>EOL, and neither are overline and strike-through.  Only the box
>attribute is extended.  Was this how we intended things to be, or is
>this just an oversight?
>
>Currently, this creates some strange counter-intuitive effects.  For
>example, try this in "emacs -Q":
>
>  C-p
>  M-x font-lock-mode RET
>  M-: (add-text-properties (point) (1+ (point)) '(face (:underline t :extend t))) RET
>
>You will see that the underline is limited only to the newline at
>point, it is not extended to the edge of the window.  But if you now
>do this:
>
>  C-SPC
>  C-n
>
>suddenly the entire last line is underlined, in addition to having the
>background color from the region face.
>
>If you replace the :underline with :box in the above example, then the
>last line has the box attribute extended to EOL even before setting
>the region, as expected.
>
>So I think this is a bug, and we should add conditions to the above
>'if' clause.  Am I missing something?
>

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Mon, 16 Dec 2019 04:12:24 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I just saw your message. Actually the problem should be somewhere else a
> bit latter in the same function. Because in terminal your code underlies
> the entire line.

On a text terminal there's no problem because of this:

   if (FRAME_WINDOW_P (f)  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
       && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
       && face->box == FACE_NO_BOX
       && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
  #ifdef HAVE_WINDOW_SYSTEM
       && !face->stipple
  #endif
       && !it->glyph_row->reversed_p
       && !Vdisplay_fill_column_indicator)
     return;

IOW, if we are on a TTY frame, we never return early here, we always
continue to extending the face.

The problem with the early return is only on GUI frames.

Re: Merging the underline attribute at EOL

[Ergus]

Hi Eli:

Actually this seems to be related with the face merge we do latter to
fill the space until the window edge and how we calculate
extend_face_id.

It is taking the text-properties in the call to face_at_pos just before
the condition you mention.

So the problem seems to be in face_at_buffer_position in these lines:

   /* Merge in attributes specified via text properties.  */
   if (!NILP (prop))
     merge_face_ref (w, f, prop, attrs, true, NULL, attr_filter);

So maybe that is the condition we need to extend...

As a dumb test I just did:

if (!NILP (prop) && attr_filter > 0)
     merge_face_ref (w, f, prop, attrs, true, NULL, attr_filter);

And it seems to solve this specific issue (in tui and gui)... but it is
inconsistent and I am not aware of the possible side effects...

Or probably we must fix this inside merge_face_ref. What do you think?



On Sat, Dec 14, 2019 at 10:28:45AM +0200, Eli Zaretskii wrote:
>Jimmy,
>
>The current code in extend_face_to_end_of_line says:
>
>  /* Face extension extends the background and box of IT->extend_face_id
>     to the end of the line.  If the background equals the background
>     of the frame, we don't have to do anything.  */
>  struct face *face = FACE_FROM_ID (f, (it->face_before_selective_p
>                                        ? it->saved_face_id
>                                        : extend_face_id));
>
>  if (FRAME_WINDOW_P (f)
>      && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>      && face->box == FACE_NO_BOX
>      && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>#ifdef HAVE_WINDOW_SYSTEM
>      && !face->stipple
>#endif
>      && !it->glyph_row->reversed_p
>      && !Vdisplay_fill_column_indicator)
>    return;
>
>This has the effect that the underline property is not extended past
>EOL, and neither are overline and strike-through.  Only the box
>attribute is extended.  Was this how we intended things to be, or is
>this just an oversight?
>
>Currently, this creates some strange counter-intuitive effects.  For
>example, try this in "emacs -Q":
>
>  C-p
>  M-x font-lock-mode RET
>  M-: (add-text-properties (point) (1+ (point)) '(face (:underline t :extend t))) RET
>
>You will see that the underline is limited only to the newline at
>point, it is not extended to the edge of the window.  But if you now
>do this:
>
>  C-SPC
>  C-n
>
>suddenly the entire last line is underlined, in addition to having the
>background color from the region face.
>
>If you replace the :underline with :box in the above example, then the
>last line has the box attribute extended to EOL even before setting
>the region, as expected.
>
>So I think this is a bug, and we should add conditions to the above
>'if' clause.  Am I missing something?
>

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Mon, 16 Dec 2019 04:49:01 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> Actually this seems to be related with the face merge we do latter to
> fill the space until the window edge and how we calculate
> extend_face_id.
> 
> It is taking the text-properties in the call to face_at_pos just before
> the condition you mention.

Yes.  there's no problem with the face calculation in face_at_pos.
The problem is that under some conditions we decide to return early
from this function, and thus don't extend the face computed by
face_at_pos.

> So the problem seems to be in face_at_buffer_position in these lines:
> 
>    /* Merge in attributes specified via text properties.  */
>    if (!NILP (prop))
>      merge_face_ref (w, f, prop, attrs, true, NULL, attr_filter);
> 
> So maybe that is the condition we need to extend...
> 
> As a dumb test I just did:
> 
> if (!NILP (prop) && attr_filter > 0)
>      merge_face_ref (w, f, prop, attrs, true, NULL, attr_filter);

When we call face_at_buffer_position from extend_face_to_end_of_line,
attr_filter is always non-zero, so I cannot see how this could change
anything.

> And it seems to solve this specific issue (in tui and gui)... but it is
> inconsistent and I am not aware of the possible side effects...
> 
> Or probably we must fix this inside merge_face_ref. What do you think?

I don't think the problem is in face_at_pos.  The problem is that
after we compute the face for extending, we don't apply it, but
instead return, because neither the background color nor the box
attribute tell us to continue with what the function does afterwards.

Re: Merging the underline attribute at EOL

[Ergus]

Hi Eli:

On the current master I can't reproduce the issue anymore. I don't
remember where was my last built master before. Did you fixed this?



On Sat, Dec 14, 2019 at 10:28:45AM +0200, Eli Zaretskii wrote:
>Jimmy,
>
>The current code in extend_face_to_end_of_line says:
>
>  /* Face extension extends the background and box of IT->extend_face_id
>     to the end of the line.  If the background equals the background
>     of the frame, we don't have to do anything.  */
>  struct face *face = FACE_FROM_ID (f, (it->face_before_selective_p
>                                        ? it->saved_face_id
>                                        : extend_face_id));
>
>  if (FRAME_WINDOW_P (f)
>      && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>      && face->box == FACE_NO_BOX
>      && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>#ifdef HAVE_WINDOW_SYSTEM
>      && !face->stipple
>#endif
>      && !it->glyph_row->reversed_p
>      && !Vdisplay_fill_column_indicator)
>    return;
>
>This has the effect that the underline property is not extended past
>EOL, and neither are overline and strike-through.  Only the box
>attribute is extended.  Was this how we intended things to be, or is
>this just an oversight?
>
>Currently, this creates some strange counter-intuitive effects.  For
>example, try this in "emacs -Q":
>
>  C-p
>  M-x font-lock-mode RET
>  M-: (add-text-properties (point) (1+ (point)) '(face (:underline t :extend t))) RET
>
>You will see that the underline is limited only to the newline at
>point, it is not extended to the edge of the window.  But if you now
>do this:
>
>  C-SPC
>  C-n
>
>suddenly the entire last line is underlined, in addition to having the
>background color from the region face.
>
>If you replace the :underline with :box in the above example, then the
>last line has the box attribute extended to EOL even before setting
>the region, as expected.
>
>So I think this is a bug, and we should add conditions to the above
>'if' clause.  Am I missing something?
>

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Mon, 16 Dec 2019 17:11:04 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> On the current master I can't reproduce the issue anymore. I don't
> remember where was my last built master before. Did you fixed this?

What did you try, exactly?  The issue I originally described, with
underline in a GUI frame, still exists.

Re: Merging the underline attribute at EOL

[Ergus]

On Mon, Dec 16, 2019 at 07:05:36PM +0200, Eli Zaretskii wrote:
>> Date: Mon, 16 Dec 2019 17:11:04 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> On the current master I can't reproduce the issue anymore. I don't
>> remember where was my last built master before. Did you fixed this?
>
>What did you try, exactly?  The issue I originally described, with
>underline in a GUI frame, still exists.
>

Ohh sorry.

I had build a wrong branch where I have actually added

NILP (face->lface[LFACE_EXTEND_INDEX])

to the condition you mention.

Actually the question is why there is only: 

face->box == FACE_NO_BOX

Maybe we have to remove it and rely only in the extend attribute?

What do you think?

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Mon, 16 Dec 2019 21:31:56 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I had build a wrong branch where I have actually added
> 
> NILP (face->lface[LFACE_EXTEND_INDEX])
> 
> to the condition you mention.
> 
> Actually the question is why there is only: 
> 
> face->box == FACE_NO_BOX
> 
> Maybe we have to remove it and rely only in the extend attribute?

If the extend attribute is set, but the background color is the same
as the default, and there's no box/underline/overline/strikethrough
attributes set, then we can still return early, no?

Re: Merging the underline attribute at EOL

[Ergus]

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

Hi Eli:

Please give a look to the attached patch I am still testing it. But just
to check if it solves all the issues you see.  

Best
Ergus

On Tue, Dec 17, 2019 at 05:22:28AM +0200, Eli Zaretskii wrote:
>> Date: Mon, 16 Dec 2019 21:31:56 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> I had build a wrong branch where I have actually added
>>
>> NILP (face->lface[LFACE_EXTEND_INDEX])
>>
>> to the condition you mention.
>>
>> Actually the question is why there is only:
>>
>> face->box == FACE_NO_BOX
>>
>> Maybe we have to remove it and rely only in the extend attribute?
>
>If the extend attribute is set, but the background color is the same
>as the default, and there's no box/underline/overline/strikethrough
>attributes set, then we can still return early, no?
>

[-- Attachment #2: test.patch --]
[-- Type: text/plain, Size: 864 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 08c6927052..fdf10894bf 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21596,13 +21596,17 @@ extend_face_to_end_of_line (struct it *it)
 
   if (FRAME_WINDOW_P (f)
       && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
-      && face->box == FACE_NO_BOX
       && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
 #ifdef HAVE_WINDOW_SYSTEM
       && !face->stipple
 #endif
       && !it->glyph_row->reversed_p
-      && !Vdisplay_fill_column_indicator)
+      && !Vdisplay_fill_column_indicator
+      && (NILP (face->lface[LFACE_EXTEND_INDEX])
+          ||(face->box == FACE_NO_BOX
+	     && face->underline == FACE_NO_UNDERLINE
+	     && face->overline_p == false
+	     && face->strike_through_p == false)))
     return;
 
   /* Set the glyph row flag indicating that the face of the last glyph

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Tue, 17 Dec 2019 15:13:45 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -21596,13 +21596,17 @@ extend_face_to_end_of_line (struct it *it)
>  
>    if (FRAME_WINDOW_P (f)
>        && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
> -      && face->box == FACE_NO_BOX
>        && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>  #ifdef HAVE_WINDOW_SYSTEM
>        && !face->stipple
>  #endif
>        && !it->glyph_row->reversed_p
> -      && !Vdisplay_fill_column_indicator)
> +      && !Vdisplay_fill_column_indicator
> +      && (NILP (face->lface[LFACE_EXTEND_INDEX])
> +          ||(face->box == FACE_NO_BOX
> +	     && face->underline == FACE_NO_UNDERLINE
> +	     && face->overline_p == false
> +	     && face->strike_through_p == false)))
>      return;

I don't think I understand the test for the :extend attribute being
non-nil.  Can you explain why you added it?  In general, faces that we
treat as "base face" when we start face merging don't need to have the
:extend attribute set (see, for example, the tool-bar face), but we
still want to apply them to the space past EOL.  Am I missing
something?  Did you bump into some scenario where this attribute
caused some regression or bad results?

Thanks.

Re: Merging the underline attribute at EOL

[Ergus]

On Tue, Dec 17, 2019 at 07:18:23PM +0200, Eli Zaretskii wrote:
>> Date: Tue, 17 Dec 2019 15:13:45 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> --- a/src/xdisp.c
>> +++ b/src/xdisp.c
>> @@ -21596,13 +21596,17 @@ extend_face_to_end_of_line (struct it *it)
>>
>>    if (FRAME_WINDOW_P (f)
>>        && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>> -      && face->box == FACE_NO_BOX
>>        && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>>  #ifdef HAVE_WINDOW_SYSTEM
>>        && !face->stipple
>>  #endif
>>        && !it->glyph_row->reversed_p
>> -      && !Vdisplay_fill_column_indicator)
>> +      && !Vdisplay_fill_column_indicator
>> +      && (NILP (face->lface[LFACE_EXTEND_INDEX])
>> +          ||(face->box == FACE_NO_BOX
>> +	     && face->underline == FACE_NO_UNDERLINE
>> +	     && face->overline_p == false
>> +	     && face->strike_through_p == false)))
>>      return;
>
>I don't think I understand the test for the :extend attribute being
>non-nil.  Can you explain why you added it?  In general, faces that we
>treat as "base face" when we start face merging don't need to have the
>:extend attribute set (see, for example, the tool-bar face), but we
>still want to apply them to the space past EOL.  Am I missing
>something?  Did you bump into some scenario where this attribute
>caused some regression or bad results?
>
>Thanks.
>
Hi Eli:

I just added that test (probably not the best one) for the case where
the face is the default face. Actually the merge we use (with the
filter) asserts that the resulting face will be :extend t or the default
face (AFAIR).

So at this point I thought (probably wrong) that only the default face
could have :extend nil and we could exit early in that case... But if
there are some cases that I am not considering we can remove that.

Do you think that removing it, the rest of the changes will be enough?

Best,
Ergus

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Thu, 19 Dec 2019 02:19:10 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> >>    if (FRAME_WINDOW_P (f)
> >>        && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
> >> -      && face->box == FACE_NO_BOX
> >>        && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
> >>  #ifdef HAVE_WINDOW_SYSTEM
> >>        && !face->stipple
> >>  #endif
> >>        && !it->glyph_row->reversed_p
> >> -      && !Vdisplay_fill_column_indicator)
> >> +      && !Vdisplay_fill_column_indicator
> >> +      && (NILP (face->lface[LFACE_EXTEND_INDEX])
> >> +          ||(face->box == FACE_NO_BOX
> >> +	     && face->underline == FACE_NO_UNDERLINE
> >> +	     && face->overline_p == false
> >> +	     && face->strike_through_p == false)))
> >>      return;
> >
> >I don't think I understand the test for the :extend attribute being
> >non-nil.  Can you explain why you added it?  In general, faces that we
> >treat as "base face" when we start face merging don't need to have the
> >:extend attribute set (see, for example, the tool-bar face), but we
> >still want to apply them to the space past EOL.  Am I missing
> >something?  Did you bump into some scenario where this attribute
> >caused some regression or bad results?
> >
> >Thanks.
> >
> Hi Eli:
> 
> I just added that test (probably not the best one) for the case where
> the face is the default face. Actually the merge we use (with the
> filter) asserts that the resulting face will be :extend t or the default
> face (AFAIR).

Mostly true (it could also be the "underlying" face when we render a
display string).

> So at this point I thought (probably wrong) that only the default face
> could have :extend nil and we could exit early in that case... But if
> there are some cases that I am not considering we can remove that.

But if the default face has underline or overline or box etc.,
attributes, we don't want to exit early, even though its background
color is the default, right?

Re: Merging the underline attribute at EOL

[Ergus]

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

Hi:

Sorry for the big latency in my replies. Busy days.

>
>Mostly true (it could also be the "underlying" face when we render a
>display string).
>
You're right

>
>But if the default face has underline or overline or box etc.,
>attributes, we don't want to exit early, even though its background
>color is the default, right?
>
Then the simpler solution is the attached patch I have been using for
some days... It just extends the conditions in the if (as you suggested
since the beginning).

If it is fine for you, then you can commit the patch for me. Of just
tell me if there are more issues to fix.

Best,
Ergus

[-- Attachment #2: test.patch --]
[-- Type: text/plain, Size: 1333 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 2dfc4cbfeb..f7d0df714f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21601,8 +21601,8 @@ extend_face_to_end_of_line (struct it *it)
      one extra blank where we could display the cursor.  */
   if ((it->current_x >= it->last_visible_x
        + (!FRAME_WINDOW_P (f)
-	  && it->glyph_row->reversed_p
-	  && !it->glyph_row->continued_p))
+          && it->glyph_row->reversed_p
+          && !it->glyph_row->continued_p))
       /* If the window has display margins, we will need to extend
 	 their face even if the text area is filled.  */
       && !(WINDOW_LEFT_MARGIN_WIDTH (it->w) > 0
@@ -21623,13 +21623,16 @@ extend_face_to_end_of_line (struct it *it)
 
   if (FRAME_WINDOW_P (f)
       && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
-      && face->box == FACE_NO_BOX
       && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
 #ifdef HAVE_WINDOW_SYSTEM
       && !face->stipple
 #endif
       && !it->glyph_row->reversed_p
-      && !Vdisplay_fill_column_indicator)
+      && !Vdisplay_fill_column_indicator
+      && face->box == FACE_NO_BOX
+      && face->underline == FACE_NO_UNDERLINE
+      && face->overline_p == false
+      && face->strike_through_p == false)
     return;
 
   /* Set the glyph row flag indicating that the face of the last glyph

Re: Merging the underline attribute at EOL

[Eli Zaretskii]

> Date: Sun, 22 Dec 2019 23:46:39 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> Sorry for the big latency in my replies. Busy days.

(Btw, messages to you frequently bounce.)

> >But if the default face has underline or overline or box etc.,
> >attributes, we don't want to exit early, even though its background
> >color is the default, right?
> >
> Then the simpler solution is the attached patch I have been using for
> some days... It just extends the conditions in the if (as you suggested
> since the beginning).
> 
> If it is fine for you, then you can commit the patch for me.

Done, thanks.