Re: Question about display engine

[Ergus]

On Sat, Sep 28, 2019 at 01:35:30PM +0300, Eli Zaretskii wrote:
Hi Eli:
> Thanks, I took a look.  I think the branch is almost ready to be
> merged, once we address the following minor comments:
>
> . I don't understand why you changed the underlined_p and
>   underline_type stuff in struct face.  What were the reasons for
>   that part of the changeset?  Its sole effect seems to be to
>   generate some redundant changes, or am I missing something?
Use double variable to describe the same is an error prone
practice. This change simplifies the checks in all the code related (as
there will be only one variable to set/check), reduces one name and
member in the struct and avoids errors related to changing one value and
not the other.

> . In this hunk from append_space_for_newline, why did you change
>   FACE_FROM_ID_OR_NULL to FACE_FROM_ID?
>
>     -         int local_default_face_id =
>     +      int char_width = 1;
>     +
>     +      if (default_face_p
>     +#ifdef HAVE_WINDOW_SYSTEM
>     +          || FRAME_WINDOW_P (it->f)
>     +#endif
>     +         )
>     +       {
>     +         const int local_default_face_id =
>                  lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
>                struct face* default_face =
>     -           FACE_FROM_ID_OR_NULL (it->f, local_default_face_id);
>     +           FACE_FROM_ID (it->f, local_default_face_id);

The call is made after a lookup_basic_face and it's for DEFAULT_FACE_ID
is it needed the check? Normally for other faces if it is NULL then we
use the DEFAULT_FACE_ID. In this case it should be unneeded right?

> . Please use comments /* in this style */, not // like this.
Fixed
> . In this hunk from extend_face_to_end_of_line you lost the
>   comment.  Is that comment no longer correct?
>
>     -             int column_x;
>
>     -             if (!INT_MULTIPLY_WRAPV (indicator_column, char_width, 
> &column_x)
>     -                 && !INT_ADD_WRAPV (it->lnum_pixel_width, column_x, 
> &column_x)
>     -                 && column_x >= it->current_x
>     -                 && column_x <= it->last_visible_x)
>     -               {
>     +         const int indicator_column =
>     +           fill_column_indicator_column (it, char_width);
>     +
>                const char saved_char = it->char_to_display;
>                const struct text_pos saved_pos = it->position;
>                const bool saved_avoid_cursor = it->avoid_cursor_p;
>     -                 const int saved_face_id = it->face_id;
>                const bool saved_box_start = it->start_of_box_run_p;
>                Lisp_Object save_object = it->object;
>     +         const int saved_face_id = it->face_id;
>
>     -                 /* The stretch width needs to considet the latter
>     -                    added glyph.  */
>     -                 const int stretch_width
>     -                   = column_x - it->current_x - char_width;
>     -
>     -                 memset (&it->position, 0, sizeof it->position);
>     +         it->face_id = extend_face_id;
>                it->avoid_cursor_p = true;
>                it->object = Qnil;
>
>     +         if (indicator_column >= 0
>     +             && indicator_column > it->current_x
>     +             && indicator_column < it->last_visible_x)
>     +            {
>     +
>     +             const int stretch_width =
>     +               indicator_column - it->current_x - char_width;
>     +
>     +             memset (&it->position, 0, sizeof it->position);
>     +
The comment is now in the function fill_column_indicator_column where
the calculation is performed now.
> . And here the comment was not moved with the code which it
>   describes:
>
>                /* Restore the face after the indicator was generated.  */
>     -                 it->face_id = saved_face_id;
>
>                /* If there is space after the indicator generate an
>                   extra empty glyph to restore the face.  Issue was
>     @@ -20634,8 +20633,8 @@ extend_face_to_end_of_line (struct it *it)
>                it->avoid_cursor_p = saved_avoid_cursor;
>                it->start_of_box_run_p = saved_box_start;
>                it->object = save_object;
>     -               }
>     -            }
>     +         it->face_id = saved_face_id;
Fixed.

> . This hunk looks redundant to me:
>
>     @@ -20681,10 +20680,9 @@ extend_face_to_end_of_line (struct it *it)
>                    /* The last row's stretch glyph should get the default
>                       face, to avoid painting the rest of the window with
>                       the region face, if the region ends at ZV.  */
>     -             if (it->glyph_row->ends_at_zv_p)
>     -               it->face_id = default_face->id;
>     -             else
>     -               it->face_id = face->id;
>     +             it->face_id = (it->glyph_row->ends_at_zv_p ?
>     +                            default_face->id : face->id);
>
>   (there's at least one more like it in the changeset).
For me this looked better (shorter and clearer), but I will restore it
if you prefer that.

> . This also looks redundant:
>
>     @@ -25436,8 +25423,8 @@ display_string (const char *string, Lisp_Object 
> lisp_str
>
>         /* Initialize the iterator IT for iteration over STRING beginning
>            with index START.  */
>     -  reseat_to_string (it, NILP (lisp_string) ? string : NULL, lisp_string, 
> start,
>     -                   precision, field_width, multibyte);
>     +  reseat_to_string (it, NILP (lisp_string) ? string : NULL, lisp_string,
>     +                    start, precision, field_width, multibyte);
The line was too long. I just tried to reduce it a bit.

> . In this commentary, please upcase attr_filter, as that is our
>   convention for describing function arguments in comments:
>
>     @@ -2269,6 +2282,11 @@ filter_face_ref (Lisp_Object face_ref,
>          of ERR_MSGS).  Use NAMED_MERGE_POINTS to detect loops in face
>          inheritance or list structure; it may be 0 for most callers.
>
>     +   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.
>
> . Likewise here:
>
>     @@ -5988,6 +6039,8 @@ compute_char_face (struct frame *f, int ch, 
> Lisp_Object pr
>          which a different face is needed, as far as text properties and
>          overlays are concerned.  W is a window displaying current_buffer.
>
>     +   attr_filter is passed merge_face_ref.
>
>   The sentence you added sounds incomplete here: did you mean to say
>   "passed to merge_face_ref"?
Fixed both.

> . This hunk looks redundant:
>
>     @@ -4505,7 +4552,8 @@ lookup_face (struct frame *f, Lisp_Object *attr)
>          suitable face is found, realize a new one.  */
>
>      int
>     -face_for_font (struct frame *f, Lisp_Object font_object, struct face 
> *base_face
>     +face_for_font (struct frame *f, Lisp_Object font_object,
>     +               struct face *base_face)
The line was too long. 85 chars long.

> . This also looks redundant:
>
>     @@ -6068,19 +6122,18 @@ face_at_buffer_position (struct window *w, 
> ptrdiff_t pos
>         }
>
>         /* Optimize common cases where we can use the default face.  */
>     -  if (noverlays == 0
>     -      && NILP (prop))
>     +  if (noverlays == 0 && NILP (prop))
>
> . And this one:
>
>         /* Begin with attributes from the default face.  */
>     -  memcpy (attrs, default_face->lface, sizeof attrs);
>     +  memcpy (attrs, default_face->lface, sizeof(attrs));
>
> . Finally, please write some short description for NEWS, as this
>   feature definitely needs to be mentioned there.
It seems unneeded two lines for this; but fixed. 

> Thanks again for working on this, and sorry for my unusually slow
> responses.

Thank to you.