Re: Question about display engine

[Eli Zaretskii]

> Date: Mon, 7 Oct 2019 17:40:55 +0200
> From: Ergus <address@hidden>
> Cc: address@hidden, address@hidden
> 
> I made some of the fixes you suggested in the last email about this
> topic.

Thanks, see a couple of comments below.

> The reduction of the underline-p + underline-stile into a single
> underline variable in the struct for me still seems like a good change
> for readability and simplification in all the code. Any way in the
> future I will try to avoid this kind of modifications even when they
> seems to be an improvement... but could you let it pass this time?

I don't understand what makes this time special, but OK.

> The other changes that seemed to be stylistic too, were actually in
> portions of code I wrote for dfci and It was there because I copied some
> of it from the previous if-else section... but I can correct it now
> right?... so no problem?
> 
> If this is fine may I merge in master?

It's OK to merge, after fixing the following nits:

> ++++
> +** There is a new face attribute :extend to use the face attributes to
> +extend after the end of the line until the end of the window.  Such
> +:extend is set to nil by default in all faces except for `hl-line` and
> +`region` because those extend until the end of the window by default.

Please quote 'like this' in NEWS, not `like this`.

Also, this NEWS entry should have a header line:

  ** New face attribute ':extend' to control face extension at EOL.

> +      /* The stretch width needs to considet the latter
                                     ^^^^^^^^
A typo.

>        /* Display fill-column indicator if needed.  */
> -      int indicator_column = fill_column_indicator_column (it);
> -      if (indicator_column >= 0
> -         && INT_ADD_WRAPV (it->lnum_pixel_width, indicator_column,
> -                           &indicator_column))
> -       indicator_column = -1;
> +      const int indicator_column =
> +       fill_column_indicator_column (it, 1) - 1;

Why did you need to subtract 1 in the last line?  If this is indeed
needed, it needs a comment to explain it.

> +   ATTR_FILTER is the index of a parameter that conditions the merging
> +   for named faces (case 1) to only the face_ref where
> +   lface[merge_face_ref] is non-nil. To merge unconditionally set this
> +   value to 0.                     ^^

Two spaces between sentences, please.

Thanks.