Merging the underline attribute at EOL

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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?

Reply | Threaded
Open this post in threaded view
|

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?
>

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Ergus
In reply to this post by Eli Zaretskii
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?
>

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Ergus
In reply to this post by Eli Zaretskii
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?
>

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
In reply to this post by Ergus
> Date: Mon, 16 Dec 2019 04:12:24 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
In reply to this post by Ergus
> Date: Mon, 16 Dec 2019 04:49:01 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> 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.

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
In reply to this post by Ergus
> Date: Mon, 16 Dec 2019 17:11:04 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> 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.

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
>> Cc: [hidden email]
>>
>> 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?

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
> Date: Mon, 16 Dec 2019 21:31:56 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> 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?

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Ergus
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 <[hidden email]>
>> Cc: [hidden email]
>>
>> 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?
>

test.patch (888 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
> Date: Tue, 17 Dec 2019 15:13:45 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> --- 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.

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
>> Cc: [hidden email]
>>
>> --- 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

Reply | Threaded
Open this post in threaded view
|

Re: Merging the underline attribute at EOL

Eli Zaretskii
> Date: Thu, 19 Dec 2019 02:19:10 +0100
> From: Ergus <[hidden email]>
> Cc: [hidden email]
>
> >>    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?